fix: resolve ls DIR/ clickable filenames against listed directory#9974
fix: resolve ls DIR/ clickable filenames against listed directory#9974rndjams wants to merge 4 commits intowarpdotdev:masterfrom
Conversation
…olution 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).
…directory Fixes warpdotdev#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 (warpdotdev#9909 — box-drawing characters not in `FILE_LINK_SEPARATORS`). Even after this fix lands, `tree` only benefits once warpdotdev#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.
…olution Address gaps identified in Oz spec review (PR warpdotdev#9973) for issue warpdotdev#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.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @rndjams on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Reviewed the directory-aware clickable filename resolution for listing command output, including the new parsing helper, link-detection integration, and tests.
Concerns
ls -d DIR/--directoryoutput names the operand itself rather than entries inside the operand, but the new heuristic still roots bare names atDIR.- Multi-operand commands such as
ls DIR FILEcan emit bare file operands rooted at the block CWD alongside entries rooted atDIR; the current helper only rejects the second operand when it is also a directory.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // Reject recursive listings — output has per-section roots we can't resolve. | ||
| if token == "--recursive" || (!token.starts_with("--") && token.contains('R')) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
ls -d DIR / --directory prints the directory operand itself, not entries under it; resolving that bare DIR against DIR first can open DIR/DIR when it exists. Reject directory-as-entry modes the same way recursive listings are rejected.
| // Reject recursive listings — output has per-section roots we can't resolve. | |
| if token == "--recursive" || (!token.starts_with("--") && token.contains('R')) { | |
| return None; | |
| } | |
| // Reject modes whose output is not a single directory's entries. | |
| if token == "--recursive" | |
| || token == "--directory" | |
| || (!token.starts_with("--") && (token.contains('R') || token.contains('d'))) | |
| { | |
| return None; | |
| } |
| 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; | ||
| } |
There was a problem hiding this comment.
ls output mix roots, not just second directory operands. For ls a somefile, somefile is rooted at the CWD while entries under a are rooted at a; if a/somefile exists, this code will link to the wrong file. Reject all multi-operand listing commands before applying listing-dir resolution.
| 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; |
…mands 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.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR adds a warp_util helper to recognize simple directory-listing commands with a single directory operand and wires terminal file-link resolution to try that listed directory before the block CWD for bare output filenames.
Concerns
- No blocking correctness or security concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
oz-for-oss
Bot
requested review from
a team and
zachbai
and removed request for
a team
1 participant
Description
Fixes clickable filename resolution in
ls DIR/output. Previously, bare filenames were resolved against the block's CWD, causing silent misresolution (opening the wrong same-named file) or false negatives (file not clickable at all). Now, when a block's command is a directory-listing command with a directory argument, bare filenames resolve against that directory first.Components:
crates/warp_util/src/listing_command.rs— pure-logic helper that parses a command line to extract the listing directory argument.app/src/terminal/view/link_detection.rs— wire-up: reads block command + alias-resolved name, tries listing-dir resolution before CWD fallback.Linked Issue
Closes #9908
ready-to-specorready-to-implement.Testing
cargo clippy -p warp_util --lib -- -D warnings— clean.cargo fmt -- --check— clean.listing_command_test.rscovering all edge cases from the product spec.ls subdir/→ clickingREADME.mdopenssubdir/README.md(not CWD's copy).link_detection.rs.Agent Mode
CHANGELOG-BUG-FIX: Fixed clickable filenames in
ls DIR/output resolving against CWD instead of the listed directory, which could silently open the wrong file.