spec: define listing-aware file link resolution#9973
spec: define listing-aware file link resolution#9973oz-for-oss[bot] wants to merge 1 commit intomasterfrom
Conversation
Co-Authored-By: Randy James <216760973+rndjams@users.noreply.github.com>
|
@oz-for-oss[bot] I'm starting a first review of this spec-only 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
This spec defines listing-aware file-link resolution for ls DIR/-style BlockList output and covers the main product behavior, code touchpoints, risks, and validation strategy.
Concerns
- Compound and piped command opt-outs are not specified in a way the technical plan can enforce, so output transformed after
ls DIR/could still resolve against the listed directory. - Directory-as-operand flags such as
ls -dare not excluded, leaving a path to the same silent misresolution the feature is meant to prevent. - The proposed listed-root type bypasses the existing shell-native path conversion path unless the spec defines how to convert it with the block session's launch data.
Verdict
Found: 0 critical, 3 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
| - The first path-like positional is not an existing directory. | ||
| - The command uses multiple directory operands in one invocation. | ||
| - The command is recursive, such as `ls -R subdir/`. | ||
| - The output is in a different block from the listing command because of pipes, redirects, scripts, or `cat` of saved output. |
There was a problem hiding this comment.
ls subdir | sed ... is not resolved against subdir.
| - Skip leading `KEY=VALUE` environment variable assignments before checking the command name. | ||
| - Check the command name against `listing_commands`. | ||
| - Skip flags and flag values that are known not to be directory operands. | ||
| - Reject recursive mode for `ls -R` and `ls --recursive`. |
There was a problem hiding this comment.
-d/--directory and equivalent replacement-command flags here too; those commands print the directory operand itself rather than entries under it, so ls -d subdir could resolve output token subdir as subdir/subdir when that nested path exists.
| - Existing root: `working_directory: &str`. | ||
| - New root: `listing_directory: Option<PathBuf>`. | ||
| For each candidate path: | ||
| 1. If `listing_directory` exists and the candidate is not already absolute and does not already include a path component that should resolve correctly against `working_directory`, try `absolute_path_if_valid(candidate, ShellPathType::PlatformNative(listing_directory.clone()), shell_launch_data)`. |
There was a problem hiding this comment.
listing_directory as platform-native even though the current resolver receives block.pwd() as a shell-native string. Specify how the helper converts the listed root with the block session's ShellLaunchData, or keep the listed root shell-native, so WSL/MSYS-style local shells do not lose correct resolution.
1 participant
Summary
ls DIR/-style output against the listed directory.Validation
Related issue: #9908