diff --git a/AGENTS.md b/AGENTS.md index 1a78c93..76a6676 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -471,6 +471,22 @@ written, and the fold uses that same predicate to decide a reply is settled. `generate` now fails the turn for both -- a reply that stops early is not a reply, and the transcript keeps whatever arrived before it. +- **A path is stored as it was typed, and `~` is expanded where it is used.** + `~/repos/x` and `/home/someone/repos/x` are a path and a snapshot of where it + pointed, and the snapshot is what breaks when an account is renamed or the + value is read on another machine -- so nothing at the boundary rewrites one + in either direction (`machines::tidy` used to expand and `shorten_home` used + to contract; both are gone). Expansion belongs to the machine the path is on: + `ssh::quote_path` and `files::PATH_PRELUDE` for a remote one, + `ssh::expand_home` for one here. The exception that proves it is + **`llama-server`'s tools**, which take the working directory as an + `x-tool-cwd` header and `chdir` to it with no shell in the way: a `~` arrives + there as a directory of that name and *every* tool using one answers "failed + to spawn process\n[exit code: -1]", which on the phone looks like a session + whose tools are all broken. `files::resolve_blocking` is what the llama + driver resolves it with at launch, on the machine that will serve the + session. + - **A transcript outlives the enum.** Removing `Event::TaskNote` hours after adding it made every transcript that had recorded one unreadable, so `launch` failed for those sessions and `SessionManager::new` skipped them — diff --git a/server/src/config.rs b/server/src/config.rs index d02ff3b..3fe6a88 100644 --- a/server/src/config.rs +++ b/server/src/config.rs @@ -777,7 +777,11 @@ impl Config { pub fn load(path: &Path) -> Result { match std::fs::read_to_string(path) { Ok(text) => format::parse(&text) - .with_context(|| format!("{} is not valid config RON", path.display())), + .with_context(|| format!("{} is not valid config RON", path.display())) + .map(|mut config| { + Self::adopt_mcp_defaults(&mut config); + config + }), // A first run has no config -- the normal starting state; a token is // generated and saved on that first start. Err(err) if err.kind() == std::io::ErrorKind::NotFound => { @@ -788,6 +792,29 @@ impl Config { } } + /// Gives a provider written before `mcp_servers` existed the defaults a + /// probe would give it now. + /// + /// A migration, and a temporary one: the field arrived on 2026-09-19 and a + /// machine discovered before then has an empty list, which reads on screen + /// exactly like a machine somebody chose to give no MCP servers -- so a + /// llama session silently had no web search and nothing said why. Pressing + /// Rediscover fixes it, which is not something a person can be expected to + /// know. Delete this once every config here has been through it. + /// + /// Empty rather than absent is the condition, because there is no way to + /// remove one from the phone: nothing can have chosen the empty list yet. + fn adopt_mcp_defaults(&mut self) { + for provider in self + .machines + .iter_mut() + .flat_map(|machine| machine.providers.iter_mut()) + .filter(|provider| provider.mcp_servers.is_empty()) + { + provider.mcp_servers = crate::machines::mcp_defaults(provider.kind); + } + } + /// Writes the config, owner-readable only. /// /// The token hashes here are verifiers rather than secrets, but the file diff --git a/server/src/files.rs b/server/src/files.rs index 67283a0..59a9e3a 100644 --- a/server/src/files.rs +++ b/server/src/files.rs @@ -176,6 +176,34 @@ pub async fn list(transport: &Transport, path: &str) -> Result { }) } +/// The absolute path `path` names on that machine, with a leading `~` expanded +/// *there*. +/// +/// What [`list`] answers as its `path`, asked on its own: a caller that has to +/// hand a directory to something with no shell in front of it needs the +/// resolved form and nothing else. `llama-server` is the one such caller -- +/// its tools take a working directory as a request header and `chdir` to it +/// literally, so the `~` that every other path in this server carries through +/// to the far side's shell arrived there as a directory called `~`, and every +/// tool that uses one failed with "failed to spawn process". +/// +/// Blocking because a driver's launch is, and this is a question for the +/// machine that will serve the session rather than for this one: a remote +/// `~` is the remote home, and expanding it here would name a directory on +/// the wrong machine -- which is also why the answer is not cached anywhere +/// but on the driver that asked. +pub fn resolve_blocking(transport: &Transport, path: &str) -> Result { + let script = format!("{PATH_PRELUDE}cd -- \"$p\" && pwd -P"); + let out = transport.capture_blocking(&launch(script, path, None))?; + // Only the newline `pwd` ends with: a directory name may legitimately end + // in a space, and trimming whitespace would rename it. + let resolved = out.trim_end_matches('\n'); + if resolved.is_empty() { + anyhow::bail!("the machine did not say what {path} resolves to"); + } + Ok(resolved.to_string()) +} + /// The `find` output above, as rows. A record without all five fields is /// dropped rather than guessed at: it can only come from a `find` that printed /// something else, and half a row is worse than no row. @@ -436,6 +464,30 @@ mod tests { assert_eq!(names, ["binary.bin", "hello.txt", "it's a file", "sub"]); } + /// The regression `resolve_blocking` exists for: a working directory typed + /// as `~/…` reaches `llama-server` as a header it `chdir`s to, so it has to + /// arrive absolute or every tool that uses one fails to spawn. + #[test] + fn a_tilde_resolves_to_that_machine_s_home() { + let Some(home) = std::env::home_dir() else { + return; + }; + let resolved = resolve_blocking(&Transport::Here, "~").unwrap(); + assert!(resolved.starts_with('/'), "{resolved}"); + assert_eq!( + std::fs::canonicalize(&resolved).unwrap(), + std::fs::canonicalize(&home).unwrap(), + ); + let dir = tree(); + // An absolute path is answered as itself, resolved. + let full = dir.path().to_string_lossy().into_owned(); + assert_eq!( + resolve_blocking(&Transport::Here, &full).unwrap(), + std::fs::canonicalize(&full).unwrap().to_string_lossy(), + ); + assert!(resolve_blocking(&Transport::Here, &at(&dir, "nope")).is_err()); + } + #[tokio::test] async fn a_missing_directory_fails_with_the_machine_s_own_message() { let dir = tree(); diff --git a/server/src/machines.rs b/server/src/machines.rs index d6cd172..8e913f8 100644 --- a/server/src/machines.rs +++ b/server/src/machines.rs @@ -127,7 +127,7 @@ impl OfferedModel { /// Only llama.cpp, because only a driver that runs its own agent loop can use /// one -- the coding CLIs configure MCP themselves and a second answer here /// would quietly disagree with theirs. -fn mcp_defaults(kind: DriverKind) -> Vec { +pub fn mcp_defaults(kind: DriverKind) -> Vec { match kind { DriverKind::LlamaCpp => vec![crate::config::McpServerConfig { name: "exa".to_string(), @@ -270,41 +270,19 @@ pub fn id_from(label: &str) -> String { } } -/// Normalises what a phone keyboard produced: trims, drops blanks, and -/// expands a leading `~` the way a shell would. +/// Normalises what a phone keyboard produced: trims, and reads a field left +/// blank as absent rather than as an empty answer. +/// +/// It deliberately does **not** touch a leading `~`. A path is stored as it was +/// typed, because `~` and `/home/someone` are not two spellings of one path -- +/// the second is a snapshot of where the first pointed, and it is the snapshot +/// that breaks when an account is renamed or the value is read on another +/// machine. Expansion belongs where the path is used, against the machine it +/// belongs to: `ssh::quote_path` and `files::PATH_PRELUDE` for a remote one, +/// `ssh::expand_home` for one on this machine. pub fn tidy(value: &str) -> Option { let value = value.trim(); - if value.is_empty() { - return None; - } - Some(match value.strip_prefix("~/") { - Some(rest) => match std::env::home_dir() { - Some(home) => home.join(rest).to_string_lossy().into_owned(), - None => value.to_string(), - }, - None => value.to_string(), - }) -} - -/// The inverse of [`tidy`]'s expansion: an absolute path under this machine's -/// home, written back as `~/…`, so that a working directory reads on a phone the -/// way it is written by hand. -/// -/// Applied only to paths on **this** machine. `$HOME` here says nothing about -/// the home directory of a machine reached over ssh, so a remote path is stored -/// exactly as it was typed and the remote shell is what expands it. -pub fn shorten_home(path: &str) -> String { - let Some(home) = std::env::home_dir() else { - return path.to_string(); - }; - let home = home.to_string_lossy(); - // The separator has to be part of the match, or `/home/bobby` would be read - // as a path inside `/home/bob`. - match path.strip_prefix(home.as_ref()) { - Some("") => "~".to_string(), - Some(rest) if rest.starts_with('/') => format!("~{rest}"), - _ => path.to_string(), - } + (!value.is_empty()).then(|| value.to_string()) } /// Runs a launch to completion and returns its stdout as text. @@ -326,25 +304,16 @@ impl Transport { mod tests { use super::*; - /// The two halves of a home-relative path, which have to be inverses: what - /// is stored is what the phone draws, and what the phone sends back is what - /// a process is started in. + /// A path survives the boundary unchanged, tilde included -- the one thing + /// `tidy` must not do is decide where `~` is. #[test] - fn a_home_path_shortens_and_expands_back() { - let Some(home) = std::env::home_dir() else { - return; - }; - let full = home.join("repos/ai-app-2"); - let full = full.to_string_lossy(); - assert_eq!(shorten_home(&full), "~/repos/ai-app-2"); - assert_eq!(shorten_home(&home.to_string_lossy()), "~"); - assert_eq!(tidy("~/repos/ai-app-2").as_deref(), Some(full.as_ref())); - - // Not a prefix match on the characters: a sibling directory whose name - // merely starts with the home directory's is not inside it. - let sibling = format!("{}-backup/notes", home.to_string_lossy()); - assert_eq!(shorten_home(&sibling), sibling); - assert_eq!(shorten_home("/etc/hosts"), "/etc/hosts"); + fn a_typed_path_is_stored_as_typed() { + assert_eq!( + tidy(" ~/repos/ai-app-2 ").as_deref(), + Some("~/repos/ai-app-2") + ); + assert_eq!(tidy("/etc/hosts").as_deref(), Some("/etc/hosts")); + assert_eq!(tidy(" "), None); } #[test] diff --git a/server/src/routes.rs b/server/src/routes.rs index 05de7e6..4f2034d 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -418,40 +418,27 @@ struct SshRequest { } impl SshRequest { - /// Tidied at the boundary rather than stored as typed -- this came from a - /// phone keyboard, so it may have a stray space or a `~`. + /// Tidied at the boundary -- this came from a phone keyboard, so a field + /// may carry a stray space or have been left blank. Nothing here rewrites + /// a path: a `~` is stored as typed and expanded where it is used, by the + /// machine it belongs to. See [`crate::machines::tidy`]. fn into_config(self) -> Result { - let address = crate::machines::tidy(&self.address) + let tidy = crate::machines::tidy; + let address = tidy(&self.address) .ok_or_else(|| ApiError::BadRequest("a machine needs an address".to_string()))?; + let path = |value: Option| { + value + .as_deref() + .and_then(tidy) + .map(std::path::PathBuf::from) + }; Ok(crate::config::SshConfig { address, port: self.port, - identity_file: self - .identity_file - .as_deref() - .and_then(crate::machines::tidy) - .map(std::path::PathBuf::from), - options: self - .options - .iter() - .filter_map(|o| crate::machines::tidy(o)) - .collect(), - // Not `tidy`: that expands `~` to *this* machine's home, and this - // path is on the other one. The remote shell expands it there. - attachments_dir: self - .attachments_dir - .as_deref() - .map(str::trim) - .filter(|dir| !dir.is_empty()) - .map(std::path::PathBuf::from), - // The same rule, and for the same reason: this directory is - // on the other machine, so a `~` in it is that machine's home. - models_dir: self - .models_dir - .as_deref() - .map(str::trim) - .filter(|dir| !dir.is_empty()) - .map(std::path::PathBuf::from), + identity_file: path(self.identity_file), + options: self.options.iter().filter_map(|o| tidy(o)).collect(), + attachments_dir: path(self.attachments_dir), + models_dir: path(self.models_dir), }) } } @@ -1373,6 +1360,33 @@ async fn spawn(manager: &Arc, body: SpawnRequest) -> Result None, }; + // The same question `set_cwd` asks, and for the same reason: a typed + // directory that is not there becomes a session whose process cannot start, + // reported long after the typo and pointing at nothing. Stored as typed -- + // a `~` is the serving machine's home and stays one. Blank normalised to + // absent for the reason the title below is: a form whose directory field + // was left alone sends `""`, which is `Some`. + let typed_cwd = match body + .cwd + .as_ref() + .map(|cwd| cwd.to_string_lossy()) + .filter(|cwd| !cwd.trim().is_empty()) + { + Some(cwd) => { + let cwd = crate::files::check_path(&cwd).map_err(bad_request)?; + let machine = machine_by_id(manager, &body.machine)?; + let transport = crate::session::transport::Transport::for_machine(&machine); + if !crate::session::import::directory_exists(&transport, &cwd).await { + return Err(ApiError::BadRequest(format!( + "{} has no directory {cwd}", + machine.name + ))); + } + Some(PathBuf::from(cwd)) + } + None => None, + }; + let spec = SpawnSpec { machine: body.machine, provider: body.provider, @@ -1391,7 +1405,7 @@ async fn spawn(manager: &Arc, body: SpawnRequest) -> Result>, - /// The session's working directory, which is where its tools act. `None` - /// leaves that to `llama-server`, which is the honest answer rather than a - /// guess at one. + /// The session's working directory, which is where its tools act, resolved + /// on the machine serving this session. Absolute, because it is handed to + /// `llama-server` as a header it `chdir`s to with no shell to expand a + /// `~`. `None` leaves the directory to `llama-server`, which is the honest + /// answer rather than a guess at one. cwd: Option, /// Set by [`Driver::interrupt`]; the streaming loop and the tool loop both /// check it, leaving what was produced in the transcript. @@ -474,16 +476,30 @@ impl LlamaDriver { let sampling = sampling_from(&meta.params); + // Resolved on the machine that will serve this session, because it is + // the one path here that reaches something with no shell in front of + // it: `llama-server` `chdir`s to the header it is given. See + // `files::resolve_blocking`. A failure is the launch's, for the same + // reason a CLI session whose directory has gone fails at its own `cd` + // -- tools running somewhere other than where they were told to is + // worse than not starting. + let cwd = meta + .cwd + .as_ref() + .map(|path| { + crate::files::resolve_blocking(transport, &path.to_string_lossy()).with_context( + || format!("this session's working directory, {}", path.display()), + ) + }) + .transpose()?; + let driver = Self { shared: Arc::new(Shared { sink, transcript: transcript.to_path_buf(), session_dir: session_dir.to_path_buf(), sampling: Mutex::new(sampling), - cwd: meta - .cwd - .as_ref() - .map(|path| path.to_string_lossy().into_owned()), + cwd, cancel: AtomicBool::new(false), turns: Mutex::new(Turns::default()), serving: Mutex::new(Serving::Loading), diff --git a/server/src/session/llama/tools.rs b/server/src/session/llama/tools.rs index 6448f49..8b811e9 100644 --- a/server/src/session/llama/tools.rs +++ b/server/src/session/llama/tools.rs @@ -172,6 +172,12 @@ impl Tools { /// say they use one. A session with no working directory sends none, and /// `llama-server` falls back to its own -- which is the honest outcome: /// this server has no better answer for where "here" is. + /// + /// It must already be **absolute**: the header is passed to `chdir` with + /// no shell in the way, so a leading `~` is a directory of that name and + /// every tool using one fails with "failed to spawn process". The driver + /// resolves it on the serving machine at launch; see + /// `files::resolve_blocking`. pub fn execute( &self, name: &str, diff --git a/server/src/ssh.rs b/server/src/ssh.rs index ea4a17d..dce30f7 100644 --- a/server/src/ssh.rs +++ b/server/src/ssh.rs @@ -107,7 +107,11 @@ pub fn command( command.args(["-p", &port.to_string()]); } if let Some(identity) = &ssh.identity_file { - command.arg("-i").arg(identity); + // Expanded here rather than when it was typed: this one *is* a path on + // this machine, but storing what `~` resolved to on the day it was + // entered is what breaks when the account is renamed. There is no shell + // between here and `ssh`, so nothing else would expand it. + command.arg("-i").arg(expand_home(identity)); // Without this, ssh may offer an agent key first and authenticate as // somebody else entirely -- silently, and with different permissions. command.args(["-o", "IdentitiesOnly=yes"]);