From 08ca3a3cf78c2dd42496514e1477d9cf5ab1d3ab Mon Sep 17 00:00:00 2001 From: thomas Date: Sun, 19 Jul 2026 19:04:59 +0800 Subject: [PATCH] fix(cwd): close the agent-cwd bypass and validate inherited dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups to the `local_cwd` gate. The agent-reported cwd sat ahead of `local_cwd` in the git-status chain and bypassed the gate entirely. A native-SSH pane keeps sentinel-sourced agent state on purpose, so an agent running *on the remote host* reported a remote path that won unconditionally and reached the local `git` — the exact collision the gate exists to prevent, on its most likely trigger (running `claude` in an SSH pane). Route the agent's report through the same remote check. Completion's fallback to `std::env::current_dir()` meant a remote pane now offers *this* machine's filenames for insertion into a remote command line, where before the remote path simply failed `read_dir` and produced nothing. `complete` takes `Option<&Path>` so "no local filesystem" is an explicit contract: command completion still runs, path and signature sources are skipped. `apply_remote_context` left `st.cwd` pointing into the namespace it just left, so after `exit` from `ssh` a local shell without shell integration kept serving the remote's last path to the local `git` probe. Clear it on both sides of the boundary; `DaemonMsg::Cwd` has no cleared form, so the client mirrors it off `RemoteContext`. Finally, validate in `initial_working_directory`: whatever wins must be a directory *here*. This bounds the whole class rather than one shell's spelling — an unresolvable path now falls through to the next candidate instead of failing the spawn with "The directory name is invalid". Co-Authored-By: Claude Opus 4.8 --- src/daemon/pane.rs | 64 ++++++++++++++++++++++++++++- src/terminal/completion.rs | 83 +++++++++++++++++++++++++++++--------- src/terminal/remote.rs | 11 +++++ src/terminal/view.rs | 38 +++++++++++------ src/ui/app.rs | 7 +--- 5 files changed, 164 insertions(+), 39 deletions(-) diff --git a/src/daemon/pane.rs b/src/daemon/pane.rs index 4072e27a..3d46eda4 100644 --- a/src/daemon/pane.rs +++ b/src/daemon/pane.rs @@ -212,7 +212,17 @@ fn initial_working_directory(cwd: Option) -> Option { // client didn't pass an explicit cwd (tab-inherit / session restore still // win). Inherit -> `forced` is `None`, so we keep the fallback as before. let forced = crate::core::config::working_directory_base(); - cwd.or(forced).or(fallback) + // Whatever wins must actually be a directory *here*. A client cwd is only + // as good as the OSC 7 that produced it, and a shell that reports a path + // this machine cannot resolve — a remote namespace, or an msys path like + // `/c/Users/x` that Windows reads as drive-relative — would otherwise turn + // a new tab or split into a hard spawn failure ("The directory name is + // invalid") instead of quietly falling back. Cheap to check, and it bounds + // the whole class rather than one shell's spelling at a time. + [cwd, forced, fallback] + .into_iter() + .flatten() + .find(|d| d.is_dir()) } fn apply_common_command_setup(cmd: &mut CommandBuilder, initial_cwd: &Option) { @@ -1736,6 +1746,16 @@ fn apply_remote_context(st: &mut PaneState, remote: Option) { if st.remote == remote { return; } + // The cwd belonged to whichever side we are leaving, and it does not + // survive the crossing: a remote path is meaningless locally, and a local + // one is meaningless on the remote. Drop it so the pane reports no cwd + // until the new shell's OSC 7 lands, rather than attributing the old + // namespace's directory to the new one — otherwise a local shell without + // shell integration keeps serving the remote's last path to the local + // `git` probe for the rest of its life. + // `DaemonMsg::Cwd` carries a bare path with no "cleared" form, so the + // client mirrors this on its own when it sees the `RemoteContext` below. + st.cwd = None; if let Some(sub) = &st.subscriber { let _ = sub.send(DaemonMsg::RemoteContext(remote.clone())); } @@ -2102,6 +2122,48 @@ fn proc_name(pid: i32) -> Option { #[cfg(test)] mod tests { use super::*; + use std::path::Path; + + /// A cwd the client reports is only as trustworthy as the OSC 7 behind it. + /// Passing one this machine cannot resolve straight to `cmd.cwd()` turns a + /// new tab or split into a hard spawn failure, so anything that isn't a + /// real directory here must fall through to the next candidate instead. + #[test] + fn initial_working_directory_skips_paths_that_are_not_directories() { + let real = std::env::temp_dir(); + assert!(real.is_dir(), "temp dir should exist"); + + // A usable client cwd still wins outright. + assert_eq!( + initial_working_directory(Some(real.clone())), + Some(real.clone()) + ); + + // A remote-namespace path, and the msys shape Windows reads as + // drive-relative: neither resolves here, so neither may be used. + for bogus in [ + "/home/someone/definitely-not-here", + "/c/Users/definitely-not-here", + ] { + let got = initial_working_directory(Some(PathBuf::from(bogus))); + assert_ne!( + got.as_deref(), + Some(Path::new(bogus)), + "{bogus} is not a directory here and must not be handed to spawn" + ); + // Whatever we fall back to must itself be usable. + if let Some(d) = got { + assert!(d.is_dir(), "fallback {d:?} must be a real directory"); + } + } + + // A file is not a directory either. + let file = real.join("tty7-iwd-probe"); + std::fs::write(&file, b"x").expect("write probe file"); + let got = initial_working_directory(Some(file.clone())); + assert_ne!(got.as_deref(), Some(file.as_path())); + let _ = std::fs::remove_file(&file); + } /// End-to-end check of the *live* agent-detection chain this feature rides /// on macOS/Linux: spawn a real PTY child whose `argv[0]` names a coding diff --git a/src/terminal/completion.rs b/src/terminal/completion.rs index 119328bb..76cce717 100644 --- a/src/terminal/completion.rs +++ b/src/terminal/completion.rs @@ -121,7 +121,11 @@ const MAX_CANDIDATES: usize = 400; /// Compute completions for `line` at char position `cursor`, resolving relative /// paths against `cwd`: command names in command position, filesystem paths /// elsewhere. Returns `None` when there's nothing to offer. -pub fn complete(line: &str, cursor: usize, cwd: &Path) -> Option { +/// +/// `cwd` is `None` when the pane has no directory on *this* machine — a remote +/// pane. Command completion still runs; everything that would touch the local +/// filesystem is skipped rather than answered from the wrong machine. +pub fn complete(line: &str, cursor: usize, cwd: Option<&Path>) -> Option { let chars: Vec = line.chars().collect(); let cursor = cursor.min(chars.len()); @@ -141,9 +145,18 @@ pub fn complete(line: &str, cursor: usize, cwd: &Path) -> Option { // back to filesystem paths. A signature slot that declares suggestions // or generators owns the position: it returns `Some` (possibly with no // sync candidates but pending scripts) rather than ceding to paths. - match complete_signature(&chars, word_start, &word, cwd) { - Some(sig) => (sig.cands, sig.pending), - None => (complete_path(&word, cwd), Vec::new()), + match cwd { + // No local cwd — a remote pane. The filesystem here is not the one + // the command will run against, so offer nothing rather than this + // machine's names, which would be inserted into a remote command + // line where they do not exist. Command completion above still + // works; real remote-aware path completion would have to source + // the listing from the remote and is out of scope. + None => (Vec::new(), Vec::new()), + Some(cwd) => match complete_signature(&chars, word_start, &word, cwd) { + Some(sig) => (sig.cands, sig.pending), + None => (complete_path(&word, cwd), Vec::new()), + }, } }; let candidates: Vec = word_cands @@ -688,7 +701,7 @@ mod tests { /// The candidate texts `complete` returns for `line` with the cursor at the /// end, or an empty vec when it offers nothing. fn texts(line: &str) -> Vec { - complete(line, line.chars().count(), Path::new("/")) + complete(line, line.chars().count(), Some(Path::new("/"))) .map(|c| c.candidates.into_iter().map(|c| c.text).collect()) .unwrap_or_default() } @@ -699,7 +712,7 @@ mod tests { assert!(t.iter().any(|s| s == "commit"), "git subcommands: {t:?}"); assert!(t.iter().any(|s| s == "status")); // Descriptions ride along for the menu's second column. - let c = complete("git ", 4, Path::new("/")).unwrap(); + let c = complete("git ", 4, Some(Path::new("/"))).unwrap(); let commit = c.candidates.iter().find(|c| c.text == "commit").unwrap(); assert_eq!(commit.kind, CandidateKind::Value); assert!(commit.description.is_some()); @@ -714,7 +727,7 @@ mod tests { #[test] fn signature_offers_flags_for_the_active_subcommand() { - let c = complete("git commit --", 13, Path::new("/")).unwrap(); + let c = complete("git commit --", 13, Some(Path::new("/"))).unwrap(); let msg = c.candidates.iter().find(|c| c.text == "--message").unwrap(); assert_eq!(msg.kind, CandidateKind::Flag); assert_eq!( @@ -741,7 +754,8 @@ mod tests { // position — it returns the generator scripts and no path candidates. let dir = temp_tree("gen-checkout", &[("sentinel.txt", false), ("subdir", true)]); let line = "git checkout "; - let c = complete(line, line.chars().count(), &dir).expect("generator slot is a completion"); + let c = complete(line, line.chars().count(), Some(dir.as_path())) + .expect("generator slot is a completion"); assert!( !c.pending.is_empty(), "the branch/tag generators ride along as pending scripts" @@ -767,7 +781,7 @@ mod tests { fn generator_script_tokens_join_with_single_spaces() { // The converter word-split original string scripts; joining restores a // single `/bin/sh -c` command. - let c = complete("git checkout ", 13, Path::new("/")).unwrap(); + let c = complete("git checkout ", 13, Some(Path::new("/"))).unwrap(); let branch = c .pending .iter() @@ -825,7 +839,12 @@ mod tests { fn unknown_command_falls_back_to_paths() { // A command with no signature still path-completes (no panic, no menu here). let dir = temp_tree("fallback", &[("readme.md", false)]); - let c = complete("frobnicate read", "frobnicate read".chars().count(), &dir).unwrap(); + let c = complete( + "frobnicate read", + "frobnicate read".chars().count(), + Some(dir.as_path()), + ) + .unwrap(); assert_eq!(c.candidates[0].text, "readme.md"); } @@ -925,7 +944,7 @@ mod tests { #[test] fn command_position_offers_builtins_with_word_range() { - let c = complete("ech", 3, Path::new("/")).unwrap(); + let c = complete("ech", 3, Some(Path::new("/"))).unwrap(); let echo = c.candidates.iter().find(|c| c.text == "echo").unwrap(); assert_eq!(echo.kind, CandidateKind::Command); assert_eq!((echo.start, echo.end), (0, 3)); // replaces the word "ech" @@ -938,7 +957,7 @@ mod tests { &[("apple.txt", false), ("apply.sh", false), ("assets", true)], ); let line = "cat a"; - let c = complete(line, line.chars().count(), &dir).unwrap(); + let c = complete(line, line.chars().count(), Some(dir.as_path())).unwrap(); let names: Vec<&str> = c.candidates.iter().map(|c| c.text.as_str()).collect(); // Closeness order: assets(6) < apply.sh(8) < apple.txt(9). assert_eq!(names, vec!["assets", "apply.sh", "apple.txt"]); @@ -952,7 +971,7 @@ mod tests { let dir = temp_tree("nested", &[("sub", true)]); std::fs::write(dir.join("sub/file.rs"), b"").unwrap(); let line = "cat sub/f"; - let c = complete(line, line.chars().count(), &dir).unwrap(); + let c = complete(line, line.chars().count(), Some(dir.as_path())).unwrap(); assert_eq!(c.candidates[0].text, "sub/file.rs"); assert_eq!(c.candidates[0].start, 4); } @@ -960,9 +979,9 @@ mod tests { #[test] fn hidden_files_only_with_dot_prefix() { let dir = temp_tree("hidden", &[(".secret", false), ("visible", false)]); - let c = complete("ls v", 4, &dir).unwrap(); + let c = complete("ls v", 4, Some(dir.as_path())).unwrap(); assert!(c.candidates.iter().all(|c| !c.text.starts_with('.'))); - let c = complete("ls .", 4, &dir).unwrap(); + let c = complete("ls .", 4, Some(dir.as_path())).unwrap(); assert!(c.candidates.iter().any(|c| c.text == ".secret")); } @@ -978,18 +997,42 @@ mod tests { ], ); let line = "cat x"; - let c = complete(line, line.chars().count(), &dir).unwrap(); + let c = complete(line, line.chars().count(), Some(dir.as_path())).unwrap(); let names: Vec<&str> = c.candidates.iter().map(|c| c.text.as_str()).collect(); assert_eq!(names, vec!["xa", "xy", "xyz", "xyzzy"]); } + /// A remote pane has no local cwd. Path candidates must come back empty + /// rather than from tty7's own directory — inserting a local filename into + /// a remote command line names a file that isn't there. Command completion + /// is unaffected: it reads `$PATH`, not the cwd. + #[test] + fn a_remote_pane_completes_commands_but_never_local_paths() { + let dir = temp_tree("remote", &[("only-here.txt", false), ("subdir", true)]); + + // With a local cwd the file is offered... + let c = complete("cat only", 8, Some(dir.as_path())).expect("local pane completes paths"); + assert!(c.candidates.iter().any(|c| c.text.starts_with("only-here"))); + + // ...and with none it is not, from the same line. + assert!(complete("cat only", 8, None).is_none()); + // Nor does a bare argument position dump anything. + assert!(complete("cat ", 4, None).is_none()); + // A signature-owned slot must not shell out to a local generator either. + assert!(complete("git checkout ", 13, None).is_none()); + + // Command position still works — that source never touches the cwd. + let c = complete("ech", 3, None).expect("command completion needs no cwd"); + assert!(c.candidates.iter().any(|c| c.text == "echo")); + } + #[test] fn no_candidates_returns_none() { let dir = temp_tree("empty", &[("zzz", false)]); - assert!(complete("cat q", 5, &dir).is_none()); + assert!(complete("cat q", 5, Some(dir.as_path())).is_none()); // A blank line offers nothing (no dump of every command on bare Tab). - assert!(complete("", 0, &dir).is_none()); - assert!(complete(" ", 3, &dir).is_none()); + assert!(complete("", 0, Some(dir.as_path())).is_none()); + assert!(complete(" ", 3, Some(dir.as_path())).is_none()); } #[test] @@ -997,7 +1040,7 @@ mod tests { // Caret sits right after "ap" with more text following; the candidate // replaces only `word_start..cursor`, leaving the tail untouched. let dir = temp_tree("midline", &[("apple.txt", false)]); - let c = complete("cat ap x.log", 6, &dir).unwrap(); + let c = complete("cat ap x.log", 6, Some(dir.as_path())).unwrap(); let apple = c.candidates.iter().find(|c| c.text == "apple.txt").unwrap(); assert_eq!((apple.start, apple.end), (4, 6)); // Applying it splices over just that range. diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 0031248c..fa4e20ec 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -668,6 +668,17 @@ impl RemoteTerminal { } DaemonMsg::RemoteContext(ctx) => { flush_batch!(); + // Crossing the local/remote boundary invalidates + // the cwd: it names a directory in the namespace + // we just left. Drop it so the pane reports none + // until the new shell's OSC 7 lands — otherwise + // an `exit` from `ssh` leaves the remote's last + // path in place, and a local shell without shell + // integration never overwrites it, so the local + // `git` probe keeps running against it. + if let Ok(mut guard) = cwd.lock() { + *guard = None; + } if let Ok(mut guard) = remote.lock() { *guard = ctx; } diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 27257844..37f2c11d 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -2600,11 +2600,22 @@ impl TerminalView { // doesn't (Windows). The claim dies with the session (`session-end` // clears it, and the agent leaving the foreground drops the whole // state), so an exited agent falls back to the pane's real directory. + // The agent's report goes through the same remote gate as the pane's own + // cwd. A native-SSH pane keeps sentinel-sourced agent state on purpose + // (`spawn_native_ssh`), so an agent running *on the remote host* reports + // a remote path — and being first in the chain it would win over + // `local_cwd` unconditionally and hand that path straight to the local + // `git`, which is the collision `local_cwd` exists to prevent. let cwd_now = self - .terminal - .agent_session() - .and_then(|s| s.cwd) - .or_else(|| self.local_cwd()); + .remote_context() + .is_none() + .then(|| { + self.terminal + .agent_session() + .and_then(|s| s.cwd) + .or_else(|| self.cwd()) + }) + .flatten(); if cwd_now.as_ref() != self.git_status_cwd.as_ref() || cmd_finished || turn_finished { self.refresh_git_status(cwd_now, cx); } @@ -3344,17 +3355,17 @@ impl TerminalView { } // Fresh completion. Path candidates come off the local filesystem, so - // the cwd must be a local one; on a remote pane this falls back to - // tty7's own dir, which is what already happened there before OSC 7 - // landed. Command/history completion is unaffected either way. Real - // remote-aware path completion would need the listing to come from the - // remote and is out of scope here. - let Some(cwd) = self.local_cwd().or_else(|| std::env::current_dir().ok()) else { - return; + // they need a local cwd — a remote pane passes `None` and gets command + // completion only. Falling back to tty7's own directory there would + // offer *this* machine's filenames for insertion into a remote command + // line, where they don't exist. + let cwd = match self.remote_context() { + Some(_) => None, + None => self.local_cwd().or_else(|| std::env::current_dir().ok()), }; let line = self.cmd.text(); let cursor = self.cmd.cursor(); - let Some(comp) = super::completion::complete(&line, cursor, &cwd) else { + let Some(comp) = super::completion::complete(&line, cursor, cwd.as_deref()) else { return; }; @@ -3404,6 +3415,9 @@ impl TerminalView { // Kick off each generator on the background executor and merge results // back on the main thread, tagged with this session's generation. + // Generators are local shell-outs and only ever come from `complete`'s + // `Some(cwd)` branch, so a remote pane has none to run. + let Some(cwd) = cwd else { return }; for pending in comp.pending { let script = pending.script; let cwd = cwd.clone(); diff --git a/src/ui/app.rs b/src/ui/app.rs index 677dbf46..9e86a525 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -2667,12 +2667,7 @@ impl Tty7App { /// for the worktree operations, which shell out to a local `git`. "Copy /// Working Directory" deliberately keeps using `tab_cwd`: copying a remote /// pane's remote path is exactly what the user wants there. - fn tab_local_cwd( - &self, - index: usize, - window: &Window, - cx: &App, - ) -> Option { + fn tab_local_cwd(&self, index: usize, window: &Window, cx: &App) -> Option { self.tabs .get(index)? .pane