From 0a3b34a29d1c414135f001731e8209a67e187ea8 Mon Sep 17 00:00:00 2001 From: Randy James Date: Sat, 2 May 2026 09:19:26 -0700 Subject: [PATCH 1/4] feat(warp_util): add listing_command module for ls-style argument resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces a narrow helper that parses a shell command line, identifies whether it's a directory-listing command (`ls`, `exa`, `eza`, `lsd` by default), and returns the first positional path argument joined to the block's CWD when that path resolves to an existing directory on disk. This is the pure-logic building block used by the terminal's file-link detector to fix a bug where clicking a bare filename in `ls DIR/` output opens the same-named file from the block's CWD instead of from DIR. The actual wiring into link detection lands in a follow-up commit. The helper: - Uses `shlex::split` for POSIX-correct shell tokenization (quotes, escapes). - Skips leading `KEY=VALUE` env-var prefixes (`LS_COLORS=auto ls DIR/`). - Skips flag tokens (`-*`); the first non-flag positional wins. - Accepts an optional `resolved_command_name` override so callers who have already resolved shell aliases (e.g. via `Block::top_level_command`) can pass the alias-resolved name — making `alias ll='ls -l'; ll DIR/` trigger the fix without requiring `ll` in `DEFAULT_LISTING_COMMANDS`. - Expands leading `~` / `~/` via `dirs::home_dir()` so tilde-prefixed args resolve without relying on shell-level expansion. - Gates the return on `is_dir()` so non-existent paths, files, and non-directory typos don't mislead the caller. `DEFAULT_LISTING_COMMANDS` is a public `&[&str]` constant — intentionally a const (not a setting) for V1 so the bug fix doesn't carry a settings-design discussion with it. Making the set user-configurable via TOML is a natural follow-up. 22 unit tests cover realistic `ls` variants (bare path, trailing slash, absolute path, tilde expansion, quoted args with spaces, multi-arg/first-wins, `--color=always DIR`, env-var prefixes), malformed input, custom command sets, negative tests for `find`/`cat`/`git`, and the 5 alias-resolution paths (override triggers match, override is authoritative, baked-in-positional limitation, `None` fallback). Adds `shlex` and `tempfile` as dependencies of `warp_util`. Both are small, widely-used, and already in the workspace dep graph (`shlex` is a direct dep of the `app` crate; `tempfile` is workspace-declared). --- Cargo.lock | 2 + crates/warp_util/Cargo.toml | 2 + crates/warp_util/src/lib.rs | 1 + crates/warp_util/src/listing_command.rs | 158 +++++++++ crates/warp_util/src/listing_command_test.rs | 351 +++++++++++++++++++ 5 files changed, 514 insertions(+) create mode 100644 crates/warp_util/src/listing_command.rs create mode 100644 crates/warp_util/src/listing_command_test.rs diff --git a/Cargo.lock b/Cargo.lock index df968c662..577844b2e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14878,6 +14878,8 @@ dependencies = [ "regex", "serde", "serde_json", + "shlex", + "tempfile", "thiserror 2.0.17", "typed-path 0.10.0", "warpui", diff --git a/crates/warp_util/Cargo.toml b/crates/warp_util/Cargo.toml index 9996cff86..d54934161 100644 --- a/crates/warp_util/Cargo.toml +++ b/crates/warp_util/Cargo.toml @@ -18,6 +18,7 @@ hex.workspace = true lazy_static.workspace = true mime_guess.workspace = true regex.workspace = true +shlex = "1.3.0" thiserror.workspace = true typed-path.workspace = true dunce.workspace = true @@ -33,4 +34,5 @@ windows.workspace = true gloo.workspace = true [dev-dependencies] +tempfile.workspace = true warpui.workspace = true diff --git a/crates/warp_util/src/lib.rs b/crates/warp_util/src/lib.rs index 49fd688c6..b8c917bbe 100644 --- a/crates/warp_util/src/lib.rs +++ b/crates/warp_util/src/lib.rs @@ -7,6 +7,7 @@ pub mod assets; pub mod content_version; pub mod file; pub mod file_type; +pub mod listing_command; pub mod on_cancel; pub mod path; pub mod standardized_path; diff --git a/crates/warp_util/src/listing_command.rs b/crates/warp_util/src/listing_command.rs new file mode 100644 index 000000000..623e484eb --- /dev/null +++ b/crates/warp_util/src/listing_command.rs @@ -0,0 +1,158 @@ +//! Helpers for understanding the structure of shell commands whose output contains +//! bare filenames rooted at a directory argument (e.g. `ls SUBDIR/`, `tree SUBDIR`). +//! +//! When the terminal detects a clickable filename in the output of such a command, +//! resolving the candidate against the block's CWD alone is incorrect: bare +//! filenames listed by `ls SUBDIR/` are rooted at `SUBDIR`, not at the CWD. If the +//! user happens to have a same-named file in the CWD (very common for `README.md`), +//! the detector would silently open the wrong file. See sibling module docs for the +//! full bug class. +//! +//! This module provides a narrow helper that parses a command line to extract the +//! first positional path argument, joins it to the block's CWD, and returns it only +//! if the resolved path is a directory on disk. + +use std::path::{Path, PathBuf}; + +/// Default set of directory-listing commands whose bare-filename output should be +/// resolved against their directory argument. +/// +/// Exposed as a constant so callers can start from this set and extend it from user +/// settings. Deliberately conservative: `ls` is the canonical case. Modern ls +/// replacements (`exa`, `eza`, `lsd`) behave identically for plain-arg output and +/// are included. +/// +/// Not included by default: +/// - `tree` produces output with box-drawing characters that the grid tokenizer +/// currently does not recognize as link separators (tracked separately). Adding +/// `tree` here has no effect until that tokenizer fix lands. +/// - Recursive listings (`ls -R`, `tree`) have multiple root directories within a +/// single block and need per-section resolution, which is out of scope here. +pub const DEFAULT_LISTING_COMMANDS: &[&str] = &["ls", "exa", "eza", "lsd"]; + +/// Given a shell command line and the block's CWD, return the directory argument +/// (joined to CWD) if the command is a directory-listing command from `listing_commands` +/// and the first positional argument resolves to an existing directory on disk. +/// +/// Returns `None` if: +/// - The command cannot be tokenized. +/// - The command name is not in `listing_commands`. +/// - There is no positional argument (e.g. plain `ls`). +/// - The positional argument does not resolve to an existing directory. +/// +/// `resolved_command_name` is an optional override for the command-name match. When +/// the caller has already resolved shell aliases (e.g. via `Block::top_level_command`), +/// pass the alias-resolved name here and we match against it instead of the raw first +/// token. Positional-argument extraction still uses the raw command tokens, which is +/// correct for the common case where aliases only add flags (`alias ll='ls -l'` → +/// `ll DIR/` still has `DIR/` as the first positional). Aliases that introduce their +/// own positional arguments (`alias lsd='ls /tmp'`) are a known limitation: we'd pick +/// the user's typed arg, missing the aliased one. Rare and documented. +/// +/// ## Examples +/// +/// ```text +/// "ls -la subdir/" + cwd=/a/b -> Some(/a/b/subdir) (if /a/b/subdir is a dir) +/// "ls --color=always subdir" + cwd=/a/b -> Some(/a/b/subdir) (if dir) +/// "ls /etc/" + cwd=/a/b -> Some(/etc) (if dir) +/// "ls" + cwd=/a/b -> None +/// "ls subdir1/ subdir2/" + cwd=/a/b -> Some(/a/b/subdir1) (first arg wins) +/// "cat subdir/foo" + cwd=/a/b -> None (cat not in listing_commands) +/// ``` +/// +/// With `resolved_command_name`: +/// +/// ```text +/// "ll subdir/", resolved="ls" + cwd=/a/b -> Some(/a/b/subdir) +/// "ll subdir/", resolved=None + cwd=/a/b -> None (ll not in listing_commands) +/// ``` +/// +/// ## Shell parsing +/// +/// Uses `shlex` to split the command into tokens. This handles POSIX-style quoting +/// (`'...'`, `"..."`, and backslash escapes). It does NOT handle: +/// - Shell variable expansion (`$HOME`, `$VAR`). If a user runs `ls $HOME/foo/`, the +/// command as stored in the block may be either pre- or post-expansion depending on +/// how Warp captures it; this function sees whatever is in the string and will fail +/// gracefully if `$HOME/foo` is not a valid directory name. +/// - Aliases with baked-in positional arguments. `alias lsd='ls /tmp'; lsd DIR/` will +/// resolve to `DIR/` (the user-typed arg), not `/tmp` (the aliased arg). This is a +/// rare edge case; if it matters, callers can expand the full alias upstream and +/// pass the expanded command string. +/// - Environment variable prefixes (`FOO=bar ls DIR/`). Handled by skipping leading +/// `KEY=VALUE`-shaped tokens. +/// - Compound shells (`cd /x && ls DIR/`). We only inspect the first command. +/// +/// ## Flag heuristic +/// +/// After identifying a listing command, we skip all tokens that start with `-` as +/// flag tokens. This is a simplification: `ls` flags with values like +/// `--color=always` are a single token that starts with `-` (handled correctly); +/// `--color always` (two tokens) would incorrectly consume `always` as a flag, but no +/// `ls` flag actually takes a path-like separate value, so this is safe for `ls`. +pub fn listing_command_argument_dir( + command: &str, + resolved_command_name: Option<&str>, + pwd: &Path, + listing_commands: &[&str], +) -> Option { + let tokens = shlex::split(command)?; + let mut iter = tokens.iter(); + + // First non-empty token is the command name. Skip leading env-var assignments + // (e.g. `FOO=bar ls DIR/`) by stepping past any `KEY=VALUE`-shaped tokens. + let raw_command_name = loop { + let token = iter.next()?; + if token.contains('=') && !token.starts_with('-') { + continue; + } + break token.as_str(); + }; + + // Use the alias-resolved name for the listing-command match if provided; otherwise + // fall back to the raw first token. Positional extraction below always uses the + // raw tokens regardless. + let name_for_match = resolved_command_name.unwrap_or(raw_command_name); + if !listing_commands.contains(&name_for_match) { + return None; + } + + // Find the first positional argument (skipping flags). + let first_positional = iter.find(|t| !t.starts_with('-'))?; + + // Expand tilde if present. Without shell-level `$HOME`, `~` by itself or `~/foo` + // won't resolve otherwise. We reuse the minimal expansion here rather than + // depending on `shellexpand` to keep warp_util lean. + let expanded = expand_leading_tilde(first_positional); + + // Join with pwd if relative; leave absolute paths alone. + let candidate = if expanded.is_absolute() { + expanded + } else { + pwd.join(&expanded) + }; + + // Only return the path if it actually resolves to a directory on disk. This + // guards against the user typing a typo'd path, or a path that exists as a file + // (which wouldn't be a listing target anyway). + candidate.is_dir().then_some(candidate) +} + +/// Expands a leading `~` or `~/` in a path to `$HOME` / `$HOME/`. Returns the input +/// unchanged if it does not start with `~`, or if `$HOME` cannot be determined. +fn expand_leading_tilde(token: &str) -> PathBuf { + if let Some(rest) = token.strip_prefix("~/") { + if let Some(home) = dirs::home_dir() { + return home.join(rest); + } + } else if token == "~" { + if let Some(home) = dirs::home_dir() { + return home; + } + } + PathBuf::from(token) +} + +#[cfg(test)] +#[path = "listing_command_test.rs"] +mod tests; diff --git a/crates/warp_util/src/listing_command_test.rs b/crates/warp_util/src/listing_command_test.rs new file mode 100644 index 000000000..c69f9d567 --- /dev/null +++ b/crates/warp_util/src/listing_command_test.rs @@ -0,0 +1,351 @@ +use super::*; +use std::fs; +use std::path::{Path, PathBuf}; +use tempfile::tempdir; + +// Small convenience: set of listing commands for tests. Matches the default set +// unless a test specifically overrides it. +const LS_ONLY: &[&str] = &["ls"]; +const DEFAULTS: &[&str] = DEFAULT_LISTING_COMMANDS; + +/// Test-only wrapper matching the pre-alias signature of `listing_command_argument_dir`. +/// Most tests don't care about alias resolution, so pass `None` by default. Tests that +/// specifically exercise alias handling call `listing_command_argument_dir` directly. +fn listing_command_argument_dir( + command: &str, + pwd: &Path, + listing_commands: &[&str], +) -> Option { + super::listing_command_argument_dir(command, None, pwd, listing_commands) +} + +#[test] +fn returns_none_for_non_listing_command() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + assert_eq!( + listing_command_argument_dir("cat subdir/foo", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("git status", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("find . -name foo", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn returns_none_for_ls_with_no_arg() { + let tmp = tempdir().unwrap(); + assert_eq!( + listing_command_argument_dir("ls", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls -la", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls --color=always", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn returns_dir_for_ls_subdir() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + let got = listing_command_argument_dir("ls subdir", tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("subdir"))); +} + +#[test] +fn returns_dir_for_ls_subdir_trailing_slash() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + let got = listing_command_argument_dir("ls subdir/", tmp.path(), LS_ONLY); + // PathBuf::join(Path::new("subdir/")) normalizes to "subdir", so compare canonically. + assert_eq!( + got.map(|p| p.canonicalize().unwrap()), + Some(tmp.path().join("subdir").canonicalize().unwrap()) + ); +} + +#[test] +fn skips_flags_before_positional() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + for cmd in &[ + "ls -la subdir", + "ls -la -A subdir", + "ls --color=always subdir", + "ls -l --color=always -A subdir", + ] { + let got = listing_command_argument_dir(cmd, tmp.path(), LS_ONLY); + assert_eq!( + got, + Some(tmp.path().join("subdir")), + "command {cmd:?} should resolve to subdir" + ); + } +} + +#[test] +fn returns_none_if_positional_does_not_exist() { + let tmp = tempdir().unwrap(); + // no subdir created + assert_eq!( + listing_command_argument_dir("ls nonexistent", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn returns_none_if_positional_is_a_file_not_a_dir() { + let tmp = tempdir().unwrap(); + fs::write(tmp.path().join("somefile"), b"hi").unwrap(); + + // `ls somefile` is valid usage but it's listing a file, not a directory; a + // file argument gives no useful directory root for output, so we return None + // and let the default resolution (block.pwd) apply. + assert_eq!( + listing_command_argument_dir("ls somefile", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn absolute_path_argument() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + let abs = tmp.path().join("subdir"); + + // Pass an unrelated pwd; the absolute arg should win. + let unrelated = tempdir().unwrap(); + let got = + listing_command_argument_dir(&format!("ls {}", abs.display()), unrelated.path(), LS_ONLY); + assert_eq!(got, Some(abs)); +} + +#[test] +fn quoted_argument_with_spaces() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("dir with spaces")).unwrap(); + + let got = listing_command_argument_dir(r#"ls "dir with spaces""#, tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("dir with spaces"))); + + let got_single = listing_command_argument_dir(r#"ls 'dir with spaces'"#, tmp.path(), LS_ONLY); + assert_eq!(got_single, Some(tmp.path().join("dir with spaces"))); +} + +#[test] +fn first_arg_wins_when_multiple() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("a")).unwrap(); + fs::create_dir(tmp.path().join("b")).unwrap(); + + let got = listing_command_argument_dir("ls a b", tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("a"))); + + let got_flagged = listing_command_argument_dir("ls -la a b", tmp.path(), LS_ONLY); + assert_eq!(got_flagged, Some(tmp.path().join("a"))); +} + +#[test] +fn returns_none_for_malformed_command() { + let tmp = tempdir().unwrap(); + // shlex::split returns None for unclosed quotes. + assert_eq!( + listing_command_argument_dir(r#"ls "unterminated"#, tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn returns_none_for_empty_command() { + let tmp = tempdir().unwrap(); + assert_eq!(listing_command_argument_dir("", tmp.path(), LS_ONLY), None); + assert_eq!( + listing_command_argument_dir(" ", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn honors_custom_listing_command_set() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + // By default, `eza` is a listing command via DEFAULTS. + assert_eq!( + listing_command_argument_dir("eza subdir", tmp.path(), DEFAULTS), + Some(tmp.path().join("subdir")) + ); + + // But if the user narrows to just "ls", `eza` no longer triggers. + assert_eq!( + listing_command_argument_dir("eza subdir", tmp.path(), LS_ONLY), + None + ); + + // A user-defined alias like "ll" can be added to the set. + let user_set: &[&str] = &["ls", "ll"]; + assert_eq!( + listing_command_argument_dir("ll subdir", tmp.path(), user_set), + Some(tmp.path().join("subdir")) + ); +} + +#[test] +fn skips_leading_env_var_assignments() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + let got = listing_command_argument_dir("LS_COLORS=auto ls subdir", tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("subdir"))); + + let got_multi = listing_command_argument_dir("FOO=1 BAR=2 ls subdir", tmp.path(), LS_ONLY); + assert_eq!(got_multi, Some(tmp.path().join("subdir"))); +} + +#[test] +fn tilde_expansion_of_argument() { + // Only run if HOME is set to a real directory that exists. + let Some(home) = dirs::home_dir() else { + return; + }; + if !home.is_dir() { + return; + } + + let got = listing_command_argument_dir("ls ~", home.parent().unwrap_or(&home), LS_ONLY); + assert_eq!(got, Some(home.clone())); + + // `~/` on its own expands to $HOME. + let got_slash = listing_command_argument_dir("ls ~/", home.parent().unwrap_or(&home), LS_ONLY); + assert_eq!(got_slash, Some(home)); +} + +#[test] +fn does_not_double_join_when_output_paths_are_already_rooted() { + // This test documents the intended contract: `listing_command_argument_dir` + // only extracts the command's literal directory argument. It does NOT and + // MUST NOT try to be clever about commands like `find DIR -name foo` where + // the output already contains `DIR/foo` — for those, the caller should not + // invoke this helper (or the command should not be in `listing_commands`). + // + // This is enforced by `listing_commands` being an allowlist: callers + // opt commands in explicitly. `find` is not in the default set. + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + assert_eq!( + listing_command_argument_dir("find subdir -name foo", tmp.path(), DEFAULTS), + None, + "find is not in DEFAULT_LISTING_COMMANDS — confirms allowlist semantics" + ); +} + +#[test] +fn realistic_ls_variants() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("specs")).unwrap(); + + // The exact forms users type every day. + let variants = &[ + "ls specs", + "ls specs/", + "ls -la specs", + "ls -la specs/", + "ls -A specs", + "ls --color=always specs", + "ls -l --color=always specs", + ]; + for v in variants { + let got = listing_command_argument_dir(v, tmp.path(), LS_ONLY); + assert_eq!( + got.map(|p| p.canonicalize().unwrap()), + Some(tmp.path().join("specs").canonicalize().unwrap()), + "variant {v:?} should resolve to specs" + ); + } +} + +// -- Alias resolution tests -- +// +// These tests exercise the `resolved_command_name` override that callers use when +// they've already resolved shell aliases upstream (in Warp, via +// `Block::top_level_command(sessions)`). + +#[test] +fn alias_resolved_name_triggers_listing_lookup() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + // Raw command is `ll subdir` (the user's alias); resolved name is `ls`. + // `ll` is not in LS_ONLY, so without the resolved-name override this would + // return None. With the override, it resolves as if the command were `ls subdir`. + let got = super::listing_command_argument_dir("ll subdir", Some("ls"), tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("subdir"))); +} + +#[test] +fn alias_resolved_name_respects_flags_in_raw_command() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + // Common alias: `alias la='ls -A'`. The user types `la subdir`; after alias + // expansion the effective command is `ls -A subdir`. We don't need to see the + // `-A` to parse correctly — our tokenizer still skips flags in the raw command + // and picks up `subdir` as the first positional. + let got = super::listing_command_argument_dir("la subdir", Some("ls"), tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("subdir"))); +} + +#[test] +fn alias_resolved_name_overrides_raw_token_for_matching_only() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + // If the resolved name is NOT a listing command, we return None even if the + // raw first token (`ls`) would have matched. The resolved name is authoritative. + let got = super::listing_command_argument_dir("ls subdir", Some("bat"), tmp.path(), LS_ONLY); + assert_eq!(got, None); +} + +#[test] +fn alias_resolution_uses_raw_tokens_for_positional_extraction() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("user_typed_dir")).unwrap(); + + // Known limitation: aliases with baked-in positional args are not expanded. + // `alias lsd='ls /tmp'; lsd user_typed_dir` → raw tokens are + // `["lsd", "user_typed_dir"]`, resolved name is `ls`. We pick + // `user_typed_dir` as the first positional (the user's typed arg), not + // `/tmp` (the aliased arg). This test documents that behavior. + let got = + super::listing_command_argument_dir("lsd user_typed_dir", Some("ls"), tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("user_typed_dir"))); +} + +#[test] +fn alias_resolution_with_no_resolved_name_falls_back_to_raw_token() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + // When `resolved_command_name` is `None`, behavior matches the pre-alias + // contract (match against raw first token). + let got = super::listing_command_argument_dir("ls subdir", None, tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("subdir"))); + + let got_miss = super::listing_command_argument_dir("ll subdir", None, tmp.path(), LS_ONLY); + assert_eq!(got_miss, None, "without resolved name, ll misses"); +} From f2890f97e87b31af10ce06c559fff802d59f1587 Mon Sep 17 00:00:00 2001 From: Randy James Date: Sat, 2 May 2026 09:19:57 -0700 Subject: [PATCH 2/4] fix(link_detection): resolve ls DIR/ output filenames against listed directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #9908. Before this change, clicking a bare filename in `ls DIR/` block output resolved the candidate against the block's CWD only. If a same-named file happened to exist in CWD (very common for README.md in nested repos), the click would silently open that wrong file. Files that existed only in DIR were not clickable at all, and directories listed by `ls DIR/` weren't clickable even though directories listed by `ls` (no arg) correctly open in Finder. The root cause was in `scan_for_file_path`: the block's pwd was the sole resolution root passed to `absolute_path_if_valid`. The block's command string — which carries the listed-argument directory — was never consulted. This commit threads a second, higher-priority resolution root through the existing resolver: - `scan_for_file_path` reads the hovered block's `command_to_string()` and `top_level_command(sessions)` (the alias-resolved command name). - It calls `warp_util::listing_command::listing_command_argument_dir` to decide whether the block ran a listing command with a directory argument, and if so, what resolved directory on disk the output is rooted at. - The resulting `listing_dir_to_scan` is passed into the thread-spawned `compute_valid_paths`, which tries it first via a new `try_resolve` closure before falling through to the existing `ShellNative(pwd)` path. The change is strictly additive: - AltScreen clicks pass `listing_dir = None`, preserving exact current behavior for full-screen apps like vim/nano. - Blocks whose command isn't a listing command, or is a listing command with no directory arg, return `None` from the helper and the existing pwd-only resolution applies unchanged. - Blocks where the argument-dir resolution misses (e.g. the user clicked a token that isn't in DIR) fall through to pwd-based resolution, so no previously-clickable link becomes unclickable. Scrollback and session restore work transparently because each block stores its own pwd and command (`SerializedBlock`), and the helper is called per-hover against the hovered block. Known V1 limitations (documented in the issue): - Aliases with baked-in positional args (`alias lsd='ls /tmp'; lsd DIR/`) resolve to the user-typed DIR/, not the aliased /tmp. Rare in practice. - Recursive listings (`ls -R DIR/`) and multi-arg (`ls D1/ D2/`) need per-section root tracking; first arg wins for multi-arg today. - `tree` output is unclickable due to a sibling tokenizer bug (#9909 — box-drawing characters not in `FILE_LINK_SEPARATORS`). Even after this fix lands, `tree` only benefits once #9909 lands too. The list of listing commands is hard-coded for V1 (`DEFAULT_LISTING_COMMANDS = ["ls", "exa", "eza", "lsd"]`). Making it user-configurable via TOML is a natural follow-up but not bundled here. --- app/src/terminal/view/link_detection.rs | 97 +++++++++++++++++++------ 1 file changed, 73 insertions(+), 24 deletions(-) diff --git a/app/src/terminal/view/link_detection.rs b/app/src/terminal/view/link_detection.rs index 916bf825a..4527236bf 100644 --- a/app/src/terminal/view/link_detection.rs +++ b/app/src/terminal/view/link_detection.rs @@ -27,6 +27,9 @@ cfg_if::cfg_if! { util::openable_file_type::FileTarget, }; use std::path::PathBuf; + use warp_util::listing_command::{ + listing_command_argument_dir, DEFAULT_LISTING_COMMANDS, + }; use warp_util::path::CleanPathResult; use warp_util::path::LineAndColumnArg; } @@ -443,15 +446,49 @@ impl super::TerminalView { ) { // For AltScreen we scan for relative path with the current working directory. // For BlockList we scan for relative path with the pwd of the hovered block. - let pwd_to_scan_for = match position { - WithinModel::AltScreen(_) => self.pwd_if_local(ctx), - WithinModel::BlockList(inner) => self - .model - .lock() - .block_list() - .block_at(inner.block_index) - .filter(|block| !self.is_block_considered_remote(block.session_id(), None, ctx)) // Don't scan for file links if the block is on remote sessions - .and_then(|block| block.pwd().map(String::from)), + // + // For BlockList we also look at the block's command: if the block ran a + // directory-listing command (`ls DIR/`, `eza DIR/`, etc.), bare filenames in + // its output are rooted at DIR, not at the block's pwd. `listing_dir_to_scan` + // holds the resolved argument directory (`pwd.join(DIR)`) in that case, and is + // tried first during candidate resolution. If the block's command is not a + // listing command, or has no directory argument, this is `None` and the + // existing pwd-only resolution is used. + // + // `top_level_command` resolves shell aliases (e.g. `ll` → `ls`) via + // `Block::top_level_command`, which delegates to the active session's alias + // table. We pass the resolved name to `listing_command_argument_dir` so + // user-aliased listing commands trigger the fix without needing their alias + // name in the `DEFAULT_LISTING_COMMANDS` list. + let (pwd_to_scan_for, listing_dir_to_scan) = match position { + WithinModel::AltScreen(_) => (self.pwd_if_local(ctx), None), + WithinModel::BlockList(inner) => { + let sessions = self.sessions.as_ref(ctx); + let model_guard = self.model.lock(); + let (pwd, command, top_level) = model_guard + .block_list() + .block_at(inner.block_index) + .filter(|block| !self.is_block_considered_remote(block.session_id(), None, ctx)) + .map(|block| { + ( + block.pwd().map(String::from), + block.command_to_string(), + block.top_level_command(sessions), + ) + }) + .unwrap_or((None, String::new(), None)); + drop(model_guard); + + let listing_dir = pwd.as_deref().and_then(|pwd_str| { + listing_command_argument_dir( + &command, + top_level.as_deref(), + std::path::Path::new(pwd_str), + DEFAULT_LISTING_COMMANDS, + ) + }); + (pwd, listing_dir) + } }; match pwd_to_scan_for { @@ -473,6 +510,7 @@ impl super::TerminalView { .spawn(move || { let paths = Self::compute_valid_paths( &path, + listing_dir_to_scan.as_deref(), possible_paths, max_columns, shell_launch_data, @@ -499,20 +537,39 @@ impl super::TerminalView { fn compute_valid_paths( working_directory: &str, + listing_dir: Option<&std::path::Path>, possible_paths: impl Iterator>, max_columns: usize, shell_launch_data: Option, ) -> Option { + // Try to resolve `clean_path` against the block's listing-command argument + // directory first (if any), then against the block's working directory. The + // listing directory takes precedence so bare filenames from `ls DIR/` output + // resolve into DIR, not into CWD — fixing the silent-misresolution bug where + // a same-named file in CWD would otherwise win. + let try_resolve = |clean_path: &CleanPathResult| -> Option { + if let Some(listing_dir) = listing_dir { + if let Some(resolved) = absolute_path_if_valid( + clean_path, + ShellPathType::PlatformNative(listing_dir.to_path_buf()), + shell_launch_data.as_ref(), + ) { + return Some(resolved); + } + } + absolute_path_if_valid( + clean_path, + ShellPathType::ShellNative(working_directory.to_string()), + shell_launch_data.as_ref(), + ) + }; + let mut link = None; 'path_loop: for within_model_possible_path in possible_paths { let possible_path = within_model_possible_path.get_inner(); // We want to check if the clean path result is a valid path and get the canonical // absolute path back. - let absolute_path = absolute_path_if_valid( - &possible_path.path, - ShellPathType::ShellNative(working_directory.to_string()), - shell_launch_data.as_ref(), - ); + let absolute_path = try_resolve(&possible_path.path); if let Some(absolute_path) = absolute_path { link = Some(Self::create_valid_link( @@ -530,11 +587,7 @@ impl super::TerminalView { path: new_possible_path.into(), line_and_column_num: possible_path.path.line_and_column_num, }; - let absolute_path = absolute_path_if_valid( - &new_possible_cleaned_path, - ShellPathType::ShellNative(working_directory.to_string()), - shell_launch_data.as_ref(), - ); + let absolute_path = try_resolve(&new_possible_cleaned_path); // check if new_possible_path is valid if let Some(absolute_path) = absolute_path { @@ -562,11 +615,7 @@ impl super::TerminalView { path: new_possible_path.into(), line_and_column_num: possible_path.path.line_and_column_num, }; - let absolute_path = absolute_path_if_valid( - &new_possible_cleaned_path, - ShellPathType::ShellNative(working_directory.to_string()), - shell_launch_data.as_ref(), - ); + let absolute_path = try_resolve(&new_possible_cleaned_path); // check if new_possible_path is valid if let Some(absolute_path) = absolute_path { From 716b9b72079a23eee91698e326134913a766aa3d Mon Sep 17 00:00:00 2001 From: Randy James Date: Sun, 3 May 2026 06:26:35 -0700 Subject: [PATCH 3/4] fix(listing_command): reject recursive/multi-dir, guard bare-name resolution Address gaps identified in Oz spec review (PR #9973) for issue #9908: - Reject -R/--recursive flags in listing_command_argument_dir (recursive listings have per-section roots that can't be resolved with a single dir). - Reject multi-directory operands (ls dir1/ dir2/) when both resolve to existing directories, to avoid silently picking the wrong section root. - Add bare-name guard in link_detection try_resolve: only attempt listing-dir resolution for candidates without path separators (no '/', '\', or leading '~'), preventing double-join for path-like candidates. - Add tests: rejects_recursive_flag_short, rejects_recursive_flag_long, rejects_multi_directory_operands, allows_single_dir_with_file_second_arg. --- app/src/terminal/view/link_detection.rs | 22 ++++++-- crates/warp_util/src/listing_command.rs | 38 +++++++++++-- crates/warp_util/src/listing_command_test.rs | 59 ++++++++++++++++++-- 3 files changed, 104 insertions(+), 15 deletions(-) diff --git a/app/src/terminal/view/link_detection.rs b/app/src/terminal/view/link_detection.rs index 4527236bf..533bbaa39 100644 --- a/app/src/terminal/view/link_detection.rs +++ b/app/src/terminal/view/link_detection.rs @@ -548,13 +548,23 @@ impl super::TerminalView { // resolve into DIR, not into CWD — fixing the silent-misresolution bug where // a same-named file in CWD would otherwise win. let try_resolve = |clean_path: &CleanPathResult| -> Option { + // Only attempt listing-dir resolution for bare entry names (no path + // separators). Candidates like `subdir/README.md`, `./foo`, or absolute + // paths already resolve correctly against CWD and must not be double-joined + // with the listing directory. if let Some(listing_dir) = listing_dir { - if let Some(resolved) = absolute_path_if_valid( - clean_path, - ShellPathType::PlatformNative(listing_dir.to_path_buf()), - shell_launch_data.as_ref(), - ) { - return Some(resolved); + let path_str = &clean_path.path; + let is_bare_name = !path_str.contains('/') + && !path_str.contains('\\') + && !path_str.starts_with('~'); + if is_bare_name { + if let Some(resolved) = absolute_path_if_valid( + clean_path, + ShellPathType::PlatformNative(listing_dir.to_path_buf()), + shell_launch_data.as_ref(), + ) { + return Some(resolved); + } } } absolute_path_if_valid( diff --git a/crates/warp_util/src/listing_command.rs b/crates/warp_util/src/listing_command.rs index 623e484eb..c426fa3af 100644 --- a/crates/warp_util/src/listing_command.rs +++ b/crates/warp_util/src/listing_command.rs @@ -56,7 +56,7 @@ pub const DEFAULT_LISTING_COMMANDS: &[&str] = &["ls", "exa", "eza", "lsd"]; /// "ls --color=always subdir" + cwd=/a/b -> Some(/a/b/subdir) (if dir) /// "ls /etc/" + cwd=/a/b -> Some(/etc) (if dir) /// "ls" + cwd=/a/b -> None -/// "ls subdir1/ subdir2/" + cwd=/a/b -> Some(/a/b/subdir1) (first arg wins) +/// "ls subdir1/ subdir2/" + cwd=/a/b -> None (multi-dir rejected) /// "cat subdir/foo" + cwd=/a/b -> None (cat not in listing_commands) /// ``` /// @@ -117,8 +117,20 @@ pub fn listing_command_argument_dir( return None; } - // Find the first positional argument (skipping flags). - let first_positional = iter.find(|t| !t.starts_with('-'))?; + // Collect remaining tokens, rejecting recursive flags and detecting multi-dir. + let mut positionals = Vec::new(); + for token in iter { + if token.starts_with('-') { + // Reject recursive listings — output has per-section roots we can't resolve. + if token == "--recursive" || (!token.starts_with("--") && token.contains('R')) { + return None; + } + continue; + } + positionals.push(token.as_str()); + } + + let first_positional = positionals.first()?; // Expand tilde if present. Without shell-level `$HOME`, `~` by itself or `~/foo` // won't resolve otherwise. We reuse the minimal expansion here rather than @@ -135,7 +147,25 @@ pub fn listing_command_argument_dir( // Only return the path if it actually resolves to a directory on disk. This // guards against the user typing a typo'd path, or a path that exists as a file // (which wouldn't be a listing target anyway). - candidate.is_dir().then_some(candidate) + if !candidate.is_dir() { + return None; + } + + // Reject multi-directory operands — output has per-section roots and picking + // just the first would misresolve entries from subsequent sections. + if positionals.len() > 1 { + let second = expand_leading_tilde(positionals[1]); + let second_candidate = if second.is_absolute() { + second + } else { + pwd.join(&second) + }; + if second_candidate.is_dir() { + return None; + } + } + + Some(candidate) } /// Expands a leading `~` or `~/` in a path to `$HOME` / `$HOME/`. Returns the input diff --git a/crates/warp_util/src/listing_command_test.rs b/crates/warp_util/src/listing_command_test.rs index c69f9d567..aa71165d7 100644 --- a/crates/warp_util/src/listing_command_test.rs +++ b/crates/warp_util/src/listing_command_test.rs @@ -147,16 +147,31 @@ fn quoted_argument_with_spaces() { } #[test] -fn first_arg_wins_when_multiple() { +fn rejects_multi_directory_operands() { let tmp = tempdir().unwrap(); fs::create_dir(tmp.path().join("a")).unwrap(); fs::create_dir(tmp.path().join("b")).unwrap(); - let got = listing_command_argument_dir("ls a b", tmp.path(), LS_ONLY); - assert_eq!(got, Some(tmp.path().join("a"))); + // Both positionals are directories → ambiguous, return None. + assert_eq!( + listing_command_argument_dir("ls a b", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls -la a b", tmp.path(), LS_ONLY), + None + ); +} - let got_flagged = listing_command_argument_dir("ls -la a b", tmp.path(), LS_ONLY); - assert_eq!(got_flagged, Some(tmp.path().join("a"))); +#[test] +fn allows_single_dir_with_file_second_arg() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("a")).unwrap(); + fs::write(tmp.path().join("somefile"), b"hi").unwrap(); + + // First positional is a dir, second is a file → not ambiguous multi-dir. + let got = listing_command_argument_dir("ls a somefile", tmp.path(), LS_ONLY); + assert_eq!(got, Some(tmp.path().join("a"))); } #[test] @@ -349,3 +364,37 @@ fn alias_resolution_with_no_resolved_name_falls_back_to_raw_token() { let got_miss = super::listing_command_argument_dir("ll subdir", None, tmp.path(), LS_ONLY); assert_eq!(got_miss, None, "without resolved name, ll misses"); } + +#[test] +fn rejects_recursive_flag_short() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + assert_eq!( + listing_command_argument_dir("ls -R subdir", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls -lR subdir", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls -laR subdir", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn rejects_recursive_flag_long() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + assert_eq!( + listing_command_argument_dir("ls --recursive subdir", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls -la --recursive subdir", tmp.path(), LS_ONLY), + None + ); +} From c1e33ccf28badf80c94c23d2b08be5a7c133968c Mon Sep 17 00:00:00 2001 From: Randy James Date: Sun, 3 May 2026 07:09:18 -0700 Subject: [PATCH 4/4] fix(listing_command): reject -d/--directory and all multi-operand commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Oz review feedback: - Reject -d/--directory flag: output names the operand itself, not entries under it, so listing-dir resolution would be incorrect. - Reject ALL multi-operand commands (not just multi-directory): 'ls DIR FILE' mixes roots — entries under DIR alongside bare file operands rooted at CWD — so a single listing root would misresolve some names. --- crates/warp_util/src/listing_command.rs | 27 ++++++------ crates/warp_util/src/listing_command_test.rs | 45 +++++++++++++++----- 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/crates/warp_util/src/listing_command.rs b/crates/warp_util/src/listing_command.rs index c426fa3af..d8d35950b 100644 --- a/crates/warp_util/src/listing_command.rs +++ b/crates/warp_util/src/listing_command.rs @@ -39,6 +39,9 @@ pub const DEFAULT_LISTING_COMMANDS: &[&str] = &["ls", "exa", "eza", "lsd"]; /// - The command name is not in `listing_commands`. /// - There is no positional argument (e.g. plain `ls`). /// - The positional argument does not resolve to an existing directory. +/// - The command uses `-R`/`--recursive` (per-section roots). +/// - The command uses `-d`/`--directory` (lists the operand itself, not entries). +/// - The command has multiple positional operands (mixed roots). /// /// `resolved_command_name` is an optional override for the command-name match. When /// the caller has already resolved shell aliases (e.g. via `Block::top_level_command`), @@ -117,12 +120,17 @@ pub fn listing_command_argument_dir( return None; } - // Collect remaining tokens, rejecting recursive flags and detecting multi-dir. + // Collect remaining tokens, rejecting modes whose output is not a single + // directory's entries. let mut positionals = Vec::new(); for token in iter { if token.starts_with('-') { // Reject recursive listings — output has per-section roots we can't resolve. - if token == "--recursive" || (!token.starts_with("--") && token.contains('R')) { + // Reject -d/--directory — output names the operand itself, not entries under it. + if token == "--recursive" + || token == "--directory" + || (!token.starts_with("--") && (token.contains('R') || token.contains('d'))) + { return None; } continue; @@ -151,18 +159,11 @@ pub fn listing_command_argument_dir( return None; } - // Reject multi-directory operands — output has per-section roots and picking - // just the first would misresolve entries from subsequent sections. + // Reject multi-operand commands — `ls DIR FILE` mixes roots (entries under DIR + // alongside bare file operands rooted at CWD), so picking a single listing root + // would misresolve some entries. if positionals.len() > 1 { - let second = expand_leading_tilde(positionals[1]); - let second_candidate = if second.is_absolute() { - second - } else { - pwd.join(&second) - }; - if second_candidate.is_dir() { - return None; - } + return None; } Some(candidate) diff --git a/crates/warp_util/src/listing_command_test.rs b/crates/warp_util/src/listing_command_test.rs index aa71165d7..702fc5de7 100644 --- a/crates/warp_util/src/listing_command_test.rs +++ b/crates/warp_util/src/listing_command_test.rs @@ -147,10 +147,11 @@ fn quoted_argument_with_spaces() { } #[test] -fn rejects_multi_directory_operands() { +fn rejects_multi_operand_commands() { let tmp = tempdir().unwrap(); fs::create_dir(tmp.path().join("a")).unwrap(); fs::create_dir(tmp.path().join("b")).unwrap(); + fs::write(tmp.path().join("somefile"), b"hi").unwrap(); // Both positionals are directories → ambiguous, return None. assert_eq!( @@ -161,17 +162,12 @@ fn rejects_multi_directory_operands() { listing_command_argument_dir("ls -la a b", tmp.path(), LS_ONLY), None ); -} - -#[test] -fn allows_single_dir_with_file_second_arg() { - let tmp = tempdir().unwrap(); - fs::create_dir(tmp.path().join("a")).unwrap(); - fs::write(tmp.path().join("somefile"), b"hi").unwrap(); - // First positional is a dir, second is a file → not ambiguous multi-dir. - let got = listing_command_argument_dir("ls a somefile", tmp.path(), LS_ONLY); - assert_eq!(got, Some(tmp.path().join("a"))); + // Dir + file → still multi-operand, output mixes roots, return None. + assert_eq!( + listing_command_argument_dir("ls a somefile", tmp.path(), LS_ONLY), + None + ); } #[test] @@ -398,3 +394,30 @@ fn rejects_recursive_flag_long() { None ); } + +#[test] +fn rejects_directory_flag_short() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + // -d lists the directory operand itself, not entries under it. + assert_eq!( + listing_command_argument_dir("ls -d subdir", tmp.path(), LS_ONLY), + None + ); + assert_eq!( + listing_command_argument_dir("ls -ld subdir", tmp.path(), LS_ONLY), + None + ); +} + +#[test] +fn rejects_directory_flag_long() { + let tmp = tempdir().unwrap(); + fs::create_dir(tmp.path().join("subdir")).unwrap(); + + assert_eq!( + listing_command_argument_dir("ls --directory subdir", tmp.path(), LS_ONLY), + None + ); +}