-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix: resolve ls DIR/ clickable filenames against listed directory #9974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
rndjams
wants to merge
4
commits into
warpdotdev:master
Choose a base branch
from
rndjams:rndjams/fix-ls-dir-link-resolution
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
0a3b34a
feat(warp_util): add listing_command module for ls-style argument res…
rndjams f2890f9
fix(link_detection): resolve ls DIR/ output filenames against listed …
rndjams 716b9b7
fix(listing_command): reject recursive/multi-dir, guard bare-name res…
rndjams c1e33cc
fix(listing_command): reject -d/--directory and all multi-operand com…
rndjams File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| //! 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. | ||
| /// - 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`), | ||
| /// 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 -> None (multi-dir rejected) | ||
| /// "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<PathBuf> { | ||
| 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; | ||
| } | ||
|
|
||
| // 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. | ||
| // 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; | ||
| } | ||
| 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 | ||
| // 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). | ||
| if !candidate.is_dir() { | ||
| return None; | ||
| } | ||
|
|
||
| // 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 { | ||
| return None; | ||
| } | ||
|
|
||
| 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; | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ls -d DIR/--directoryprints the directory operand itself, not entries under it; resolving that bareDIRagainstDIRfirst can openDIR/DIRwhen it exists. Reject directory-as-entry modes the same way recursive listings are rejected.