Support warp://action/open_file_editor URIs#10233
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: poper.
|
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
Adds parsing and handling for warp://action/open_file URIs and forwards them into the existing URI file-opening flow with optional line/column data.
Concerns
- The new custom URI action reuses the existing
file://opener, which can route runnable shell scripts into the session execution path. The URI action should be constrained to opening files in viewers/editors, or explicitly reject paths that would becomeExecuteInSession. - This is user-visible deep-link behavior and the PR description does not include screenshots or a short video showing the end-to-end result. For faster review, please upload visual evidence of the feature working end to end.
Security
warp://action/open_filecurrently reaches the same runnable-script behavior asfile://, expanding the protocol surface that can trigger local script execution behavior.
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
| open_file(window_id, path, None, false, ctx); | ||
| } | ||
| Self::OpenFile { path, line_col } => { | ||
| open_file(primary_window_id, path.clone(), *line_col, true, ctx); |
There was a problem hiding this comment.
open_file reuses the file:// routing path, which can send runnable shell scripts through execute_file; a warp://action/open_file?... link should only open files in an editor/viewer or reject ExecuteInSession paths.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: poper.
|
Add warp://action/open_file URI parsing and route it through the editor/notebook-only file opening path with optional line and column targeting. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
niyangup
force-pushed
the
issue-9561-open-file-uri
branch
from
33c5c4a to
4c84c9c
Compare
|
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 @niyangup 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 |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
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: 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 URI parsing and routing for warp://action/open_file with a required absolute path and optional positive line/column parameters. It reuses the existing file-opening flow while distinguishing the new URI origin so executable/session-opening paths are blocked for this action.
Concerns
- None found.
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
seemeroland
left a comment
seemeroland
left a comment
There was a problem hiding this comment.
On second thought maybe we should rename the URI warp://action/open_file_editor to be clear what this does
You can test with ./script/run and testing the URI with warplocal://uri prefix
| }); | ||
|
|
||
| let action = classify_open_file_action(&path); | ||
| if !should_handle_open_file_action(action, origin) { |
There was a problem hiding this comment.
This looks like it'll ignore the open_file URI for a file that is executable. I think we should open the executable file in the editor
There was a problem hiding this comment.
Also looks like this comment wasn't addressed
| } | ||
|
|
||
| #[test] | ||
| fn test_action_open_file_parse_with_line_and_column() { |
There was a problem hiding this comment.
It'd be nice to add a test that we also support line only, no column
There was a problem hiding this comment.
This one also was not addressed
| /// * For other files, open a new session at the parent directory path, then possibly execute the | ||
| /// file. | ||
| fn open_file(window_id: Option<WindowId>, path: PathBuf, ctx: &mut AppContext) { | ||
| fn open_file( |
There was a problem hiding this comment.
This function is used for the file:// URI which decides what to do based on the file type.
For the URI you are adding, I thought the intention was to always open the file in warp's file editor at the specified line and column number? Then we should probably use a separate function which is basically always the OpenFileAction::Editor path
There was a problem hiding this comment.
I don't think this was addressed? Would prefer not to change the signature of this function, and instead have a separate handler for the open file editor URI
Use open_file_editor to make the URI action's editor-only behavior explicit and keep the script/session execution boundary clear. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Updated the URI action name from Validation performed: cargo fmt --all --check
MACOSX_DEPLOYMENT_TARGET=14.0 cargo test test_action_open_file_editor_parse --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
MACOSX_DEPLOYMENT_TARGET=14.0 cargo test test_open_file_executable_sh_routes_to_execute --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
./script/run
open 'warposs://action/open_file_editor?path=/Users/niyangup/WorkSpace/warp/app/build.rs&line=109&column=1'Note: this local OSS build registers I also recorded the manual verification and will attach the video to this PR thread. |
warp-fix.mp4 |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review 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 |
There was a problem hiding this comment.
Overview
This PR adds a warp://action/open_file_editor URI action that parses an absolute path plus optional positive line/column query parameters and routes openable files through the existing Warp file-opening flow.
Concerns
- The project README is being updated with branch-local PR status notes and implementation details that should not be merged into the repository documentation.
- Manual testing evidence is missing for the user-visible URI flow. Please include screenshots or a screen recording showing the URI opening a file and jumping to the requested position, or justify why manual testing is not possible.
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
| * [Core-foundation](https://github.com/servo/core-foundation-rs) | ||
| * [Smol](https://github.com/smol-rs/smol) | ||
|
|
||
| ## Local Work Summary: `warp://action/open_file_editor` |
There was a problem hiding this comment.
Keep PR status and validation details in the GitHub thread instead of repository documentation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Removed the branch-local PR notes from Fresh validation after that cleanup: cargo fmt --all --check
MACOSX_DEPLOYMENT_TARGET=14.0 cargo test test_action_open_file_editor_parse --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
MACOSX_DEPLOYMENT_TARGET=14.0 cargo test test_open_file_executable_sh_routes_to_execute --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.tomlManual testing evidence is attached above: https://github.com/user-attachments/assets/3f1f3823-bd17-434e-b9ff-f823d2d0aca5 |
| .ok_or_else(|| anyhow!("missing path for open_file_editor action"))?; | ||
| let path = PathBuf::from(raw_path.into_owned()); | ||
| ensure!( | ||
| path.is_absolute(), |
There was a problem hiding this comment.
It'd be nice to expand home like ~/dir. We did similar in #9277
There was a problem hiding this comment.
This comment was not addressed
There was a problem hiding this comment.
This comment still was not addressed
There was a problem hiding this comment.
Addressed in the latest changes: parse_open_file_editor_url(...) now expands ~/... with shellexpand::tilde(...) before validating that the resulting path is absolute.
There was a problem hiding this comment.
Confirmed locally as well: open_file_editor successfully expands ~/... and opens the resolved absolute path. I verified it with warposs://action/open_file_editor?path=~/WorkSpace/warp/app/src/uri/mod.rs&line=739&column=1 on the current branch.
| /// * For other files, open a new session at the parent directory path, then possibly execute the | ||
| /// file. | ||
| fn open_file(window_id: Option<WindowId>, path: PathBuf, ctx: &mut AppContext) { | ||
| fn open_file( |
There was a problem hiding this comment.
I don't think this was addressed? Would prefer not to change the signature of this function, and instead have a separate handler for the open file editor URI
| }); | ||
|
|
||
| let action = classify_open_file_action(&path); | ||
| if !should_handle_open_file_action(action, origin) { |
There was a problem hiding this comment.
Also looks like this comment wasn't addressed
Keep the new URI action on the editor path, preserve legacy file opening behavior, and cover the missing URI parsing cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Updated the PR to address the open_file_editor review comments:
Validation: cargo fmt --all --check
cargo test test_action_open_file_editor_parse --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_executable_sh_routes_to_execute --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml/oz-review |
|
I'm re-reviewing this pull request in response to a review 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 |
There was a problem hiding this comment.
Overview
This PR adds a new custom URI action for opening local files at optional line/column positions and routes it through the in-app editor flow.
Concerns
- The registered action path is
/open_file_editor, so the documented and requestedwarp://action/open_file?...URI will still fail to parse. - The new URI path is untrusted input and is passed directly into the code editor path without the existing file-type/special-file safeguards.
Security
- A crafted URI can point at a binary, device, FIFO, or other non-text absolute path and cause Warp to try to load it via the code editor instead of rejecting or falling back safely.
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
| match url.path() { | ||
| "/new_tab" => Ok(Self::NewTab), | ||
| "/new_window" => Ok(Self::NewWindow), | ||
| "/open_file_editor" => { |
There was a problem hiding this comment.
/open_file_editor, but the PR advertises warp://action/open_file?..., so the requested URI will still hit the unexpected-action error; register /open_file or alias both names and update the tests.
| }; | ||
|
|
||
| let editor_settings = EditorSettings::as_ref(ctx); | ||
| let target = resolve_file_target_to_open_in_warp(&path, editor_settings, None); |
There was a problem hiding this comment.
CodeEditor for binary or special files, a crafted URI can make Warp try to read arbitrary non-text paths such as devices/FIFOs. Reuse the existing open_file classification or validate path.is_file() and openability before loading it in the editor.
Keep custom URI editor opens constrained to files Warp can safely open while preserving legacy file:// execution routing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
niyangup
changed the title
|
Updated this PR to address the latest open_file_editor review feedback:
Validation: cargo fmt --all --check
cargo test test_action_open_file_editor_parse --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_editor --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_executable_sh_routes_to_execute --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml/oz-review |
Resolve PR merge conflict after upstream renamed URI tests while keeping open_file_editor safety coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
/oz-review |
| .ok_or_else(|| anyhow!("missing path for open_file_editor action"))?; | ||
| let path = PathBuf::from(raw_path.into_owned()); | ||
| ensure!( | ||
| path.is_absolute(), |
There was a problem hiding this comment.
This comment was not addressed
| if path.is_file() { | ||
| if is_runnable_shell_script(path) { | ||
| return OpenFileAction::ExecuteInSession; | ||
| return None; |
There was a problem hiding this comment.
This looks wrong. We should still be able to open a script in the file editor even if it's executable
| } | ||
|
|
||
| /// Handle an incoming `file://` URL. | ||
| /// Handle an incoming file path from a URI. |
There was a problem hiding this comment.
nit: no need to change this comment
There was a problem hiding this comment.
Fixed in 275234f: restored this comment to Handle an incoming file:// URL. since open_file(...) remains the legacy file:// path and open_file_editor(...) handles the new action separately.
| openable_file_type::resolve_file_target_to_open_in_warp, | ||
| }; | ||
|
|
||
| if classify_open_file_editor_action(&path).is_none() { |
There was a problem hiding this comment.
I don't think this function needs to do any classification. It only needs to check if it's possible to open this file in a file editor
We can remove the changes to classify_open_file_action which is used for file:// URI and just do the checks required here
Keep file:// routing focused on notebook/editor/execute behavior while validating open_file_editor URIs with a dedicated openability check. This preserves the shebang fallback for legacy file:// handling and allows editor-openable executable scripts through the open_file_editor path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed the latest Changes:
Validation: cargo fmt --all --check
cargo test test_action_open_file_editor_parse --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_editor --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_executable_sh_routes_to_execute --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_non_executable_sh_routes_to_editor --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml
cargo test test_open_file_non_runnable_shebang_routes_to_editor --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review 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 |
There was a problem hiding this comment.
Overview
This PR adds parsing and handling for warp://action/open_file_editor URIs, including absolute path validation and optional line/column support, and routes accepted files into Warp's editor-opening flow.
Concerns
- This is a user-facing behavior change, but the PR description does not include screenshots or a screen recording showing the URI opening a file end to end. For this user-facing change, please include screenshots or a screen recording demonstrating it working end to end.
Verdict
Found: 0 critical, 1 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
|
Updated manual verification for the final open_file_editor behavior. This screen recording shows the current end-to-end URI flow:
Fresh verification for the final changes: cargo fmt --all --check |
seemeroland
left a comment
seemeroland
left a comment
There was a problem hiding this comment.
Thanks for iterating! Looks good but missed a few comments
| .ok_or_else(|| anyhow!("missing path for open_file_editor action"))?; | ||
| let path = PathBuf::from(raw_path.into_owned()); | ||
| ensure!( | ||
| path.is_absolute(), |
There was a problem hiding this comment.
This comment still was not addressed
| } | ||
|
|
||
| /// Handle an incoming `file://` URL. | ||
| /// Handle an incoming file path from a URI. |
2 participants
Summary
warp://action/open_file?path=/absolute/path&line=120&column=8.pathand optional positiveline/columnquery parameters.Test Plan
MACOSX_DEPLOYMENT_TARGET=14.0 cargo fmt --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.toml --all --checkMACOSX_DEPLOYMENT_TARGET=14.0 cargo test test_action_open_file_parse --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.tomlMACOSX_DEPLOYMENT_TARGET=14.0 cargo check --manifest-path /Users/niyangup/WorkSpace/warp/Cargo.tomlCloses #9561