From 4cdcbd204a772cddf6ba5f9084485c2b99ecc09e Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Fri, 28 Aug 2026 04:49:55 -0400 Subject: [PATCH] Put the transport above the drivers instead of inside one ClaudeDriver::spawn called ssh::command itself, so a translator whose job is a wire format also knew how sessions reach other machines, and every future driver would have had to remember the same. It now emits a `Launch` -- program, arguments, working directory -- and hands it to a `Transport` the manager chose from the session's host. This is the inversion Bryan asked for, and it pays for itself immediately in a place I had reported as a UI bug: "Run on" is offered for every provider but only the Claude driver honoured it, so choosing a host for an echo session silently ran it locally. With the transport above the driver that cannot be written -- EchoDriver builds no Launch, so there is nothing to wrap and nothing to misreport. The picker still needs to stop offering it, but the code no longer lies underneath. crate::ssh keeps the quoting, the forced options and the remote script, with its tests; transport.rs only decides which of the two it is. The two failure messages move with it, since they are transport-specific -- a missing ssh client here is a different thing to check than a program missing from a remote PATH. Noted in transport.rs rather than built, because nothing needs it yet: a remote llama-server is spawned as a process but spoken to over HTTP, so a transport eventually needs "reach this port" as well as "run this". Verified: cargo test (35), clippy, fmt. Nothing outside transport.rs mentions ssh now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017xn8nHw1tw1R6PtiY1eEtw --- server/src/session/claude.rs | 40 +++++-------- server/src/session/mod.rs | 4 +- server/src/session/transport.rs | 102 ++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 25 deletions(-) create mode 100644 server/src/session/transport.rs diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 1106f35..79c8268 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -1,8 +1,11 @@ //! The Claude Code driver: `claude -p` speaking stream-json on stdio, //! translated into the common event model. //! -//! This half owns the process -- spawning it (locally or through `ssh`), -//! resuming it after a crash, writing lines to it, and shutting it down. +//! This half owns the process -- asking a transport to start it, resuming +//! it after a crash, writing lines to it, and shutting it down. Where it +//! runs is `session::transport`'s business, not this file's: this one +//! emits a `Launch` and never learns whether it became a local child or an +//! ssh invocation. //! Turning a line into [`Event`]s is [`translate`], which changes when the //! CLI's wire format does rather than when any of the above does. //! @@ -40,7 +43,8 @@ use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::sync::{mpsc, oneshot}; use super::driver::{Driver, Event, EventSink, ImageRef, SessionStatus}; -use crate::config::{HostConfig, ProviderConfig, SessionConfig}; +use super::transport::{Launch, Transport}; +use crate::config::{ProviderConfig, SessionConfig}; use translate::{AnswerOutcome, Translator}; /// Where the driver remembers its CLI session id between backend runs -- @@ -70,7 +74,7 @@ impl ClaudeDriver { pub fn spawn( meta: &SessionConfig, provider: &ProviderConfig, - host: Option<&HostConfig>, + transport: &Transport, session_dir: &Path, sink: EventSink, ) -> Result { @@ -96,25 +100,13 @@ impl ClaudeDriver { args.push("--include-partial-messages".to_string()); let program = provider.command.as_deref().unwrap_or("claude"); - let cwd = meta.cwd.as_deref(); - let where_it_runs = match host { - Some(host) => format!("on {} ({})", host.name, host.address), - None => "on this machine".to_string(), - }; - let mut child = crate::ssh::command(host, program, &args, cwd) - .spawn() - .with_context(|| match host { - Some(host) => format!( - "couldn't start ssh to run \"{program}\" on {} -- is the ssh client \ - installed here?", - host.name - ), - None => format!( - "couldn't run \"{program}\" on this machine -- is it installed and on \ - PATH? If it lives on another machine, give the session a host to run on.", - ), - })?; - tracing::info!("session {} running {program} {where_it_runs}", meta.id); + let launch = Launch::new(program, args, meta.cwd.as_deref()); + let mut child = transport.spawn(&launch)?; + tracing::info!( + "session {} running {program} {}", + meta.id, + transport.describe() + ); let stdin = child.stdin.take().expect("piped stdin"); let stdout = child.stdout.take().expect("piped stdout"); @@ -168,7 +160,7 @@ impl ClaudeDriver { let (kill_tx, kill_rx) = oneshot::channel::<()>(); { let sink = sink.clone(); - let label = format!("{} {where_it_runs}", provider.name); + let label = format!("{} {}", provider.name, transport.describe()); tokio::spawn(async move { let status = tokio::select! { status = child.wait() => status.ok(), diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 2160e75..7ae466e 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -13,6 +13,7 @@ pub mod claude; pub mod driver; pub mod echo; pub mod transcript; +pub mod transport; use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -28,6 +29,7 @@ use claude::ClaudeDriver; use driver::{Driver, Event, ImageRef, SessionStatus}; use echo::EchoDriver; use transcript::{SeqEvent, Transcript}; +use transport::Transport; /// Fan-out buffer per session. A subscriber that falls further behind than /// this is caught up from the transcript file instead (see `routes`), so @@ -472,7 +474,7 @@ fn launch( DriverKind::ClaudeCli => Box::new(ClaudeDriver::spawn( &meta, provider, - host, + &Transport::for_host(host), &dir, sink.clone(), )?), diff --git a/server/src/session/transport.rs b/server/src/session/transport.rs new file mode 100644 index 0000000..79db4a6 --- /dev/null +++ b/server/src/session/transport.rs @@ -0,0 +1,102 @@ +//! Where a session's process runs, and the only place that knows how. +//! +//! A driver says *what* to run -- a [`Launch`] -- and hands it here. +//! Whether that becomes a child of this process or an `ssh host …` +//! invocation is settled in this module, so a driver carries no transport +//! knowledge and a second one cannot forget to handle the remote case. It +//! also means the wrapping is honest about drivers that run nothing at +//! all: `EchoDriver` builds no [`Launch`], so there is nothing to wrap and +//! no host for it to appear to honour. +//! +//! The quoting, the forced ssh options and the remote script are +//! `crate::ssh`'s, which this dispatches to. That split is deliberate: +//! this module decides *which* transport, that one knows what a correct +//! ssh invocation is. +//! +//! Known second operation, not built because nothing needs it yet: a +//! managed `llama-server` is spawned as a process but then spoken to over +//! HTTP, so a remote one needs a forwarded port (`ssh -L`) as well. A +//! transport is eventually "run this" plus "reach this port", where the +//! second is a no-op locally. See PLAN.md's SSH section. + +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use tokio::process::Child; + +use crate::config::HostConfig; + +/// What a driver needs run in order to exist as a process. +/// +/// Deliberately just the three things every transport can carry. Anything +/// a particular machine needs -- a port, a key, extra ssh options -- is +/// the transport's own configuration, not something a driver states. +pub struct Launch { + pub program: String, + pub args: Vec, + pub cwd: Option, +} + +impl Launch { + pub fn new(program: impl Into, args: Vec, cwd: Option<&Path>) -> Self { + Self { + program: program.into(), + args, + cwd: cwd.map(Path::to_path_buf), + } + } +} + +/// The machine a session's process runs on. +pub enum Transport { + /// The machine this server is running on. + Here, + /// Reached with the system `ssh` client. Owns its host entry rather + /// than borrowing it, so a session keeps working against the config it + /// was spawned with even if that entry is edited afterwards. + Ssh(HostConfig), +} + +impl Transport { + /// The transport a session's configured host names; absent is [`Self::Here`]. + pub fn for_host(host: Option<&HostConfig>) -> Self { + match host { + Some(host) => Self::Ssh(host.clone()), + None => Self::Here, + } + } + + /// Starts `launch`, with stdio piped and the child killed on drop. + /// + /// The failure names what to check, and the two transports fail for + /// genuinely different reasons -- a missing ssh client here versus a + /// program that is not on the remote PATH -- so each says its own + /// thing rather than one message hedging between them. + pub fn spawn(&self, launch: &Launch) -> Result { + let host = match self { + Self::Here => None, + Self::Ssh(host) => Some(host), + }; + crate::ssh::command(host, &launch.program, &launch.args, launch.cwd.as_deref()) + .spawn() + .with_context(|| match self { + Self::Ssh(host) => format!( + "couldn't start ssh to run \"{}\" on {} -- is the ssh client installed here?", + launch.program, host.name, + ), + Self::Here => format!( + "couldn't run \"{}\" on this machine -- is it installed and on PATH? If it \ + lives on another machine, give the session a host to run on.", + launch.program, + ), + }) + } + + /// How to say where this runs, for a log line a person reads. + pub fn describe(&self) -> String { + match self { + Self::Here => "on this machine".to_string(), + Self::Ssh(host) => format!("on {} ({})", host.name, host.address), + } + } +}