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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xn8nHw1tw1R6PtiY1eEtw
This commit is contained in:
1 parent
ba6c770be3
commit
4cdcbd204a
3 files changed
+121
-25
No files matched your search
@@ -1,8 +1,11 @@
|
|||||||
//! The Claude Code driver: `claude -p` speaking stream-json on stdio,
|
//! The Claude Code driver: `claude -p` speaking stream-json on stdio,
|
||||||
//! translated into the common event model.
|
//! translated into the common event model.
|
||||||
//!
|
//!
|
||||||
//! This half owns the process -- spawning it (locally or through `ssh`),
|
//! This half owns the process -- asking a transport to start it, resuming
|
||||||
//! resuming it after a crash, writing lines to it, and shutting it down.
|
//! 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
|
//! 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.
|
//! 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 tokio::sync::{mpsc, oneshot};
|
||||||
|
|
||||||
use super::driver::{Driver, Event, EventSink, ImageRef, SessionStatus};
|
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};
|
use translate::{AnswerOutcome, Translator};
|
||||||
|
|
||||||
/// Where the driver remembers its CLI session id between backend runs --
|
/// Where the driver remembers its CLI session id between backend runs --
|
||||||
@@ -70,7 +74,7 @@ impl ClaudeDriver {
|
|||||||
pub fn spawn(
|
pub fn spawn(
|
||||||
meta: &SessionConfig,
|
meta: &SessionConfig,
|
||||||
provider: &ProviderConfig,
|
provider: &ProviderConfig,
|
||||||
host: Option<&HostConfig>,
|
transport: &Transport,
|
||||||
session_dir: &Path,
|
session_dir: &Path,
|
||||||
sink: EventSink,
|
sink: EventSink,
|
||||||
) -> Result<Self> {
|
) -> Result<Self> {
|
||||||
@@ -96,25 +100,13 @@ impl ClaudeDriver {
|
|||||||
args.push("--include-partial-messages".to_string());
|
args.push("--include-partial-messages".to_string());
|
||||||
|
|
||||||
let program = provider.command.as_deref().unwrap_or("claude");
|
let program = provider.command.as_deref().unwrap_or("claude");
|
||||||
let cwd = meta.cwd.as_deref();
|
let launch = Launch::new(program, args, meta.cwd.as_deref());
|
||||||
let where_it_runs = match host {
|
let mut child = transport.spawn(&launch)?;
|
||||||
Some(host) => format!("on {} ({})", host.name, host.address),
|
tracing::info!(
|
||||||
None => "on this machine".to_string(),
|
"session {} running {program} {}",
|
||||||
};
|
meta.id,
|
||||||
let mut child = crate::ssh::command(host, program, &args, cwd)
|
transport.describe()
|
||||||
.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 stdin = child.stdin.take().expect("piped stdin");
|
let stdin = child.stdin.take().expect("piped stdin");
|
||||||
let stdout = child.stdout.take().expect("piped stdout");
|
let stdout = child.stdout.take().expect("piped stdout");
|
||||||
@@ -168,7 +160,7 @@ impl ClaudeDriver {
|
|||||||
let (kill_tx, kill_rx) = oneshot::channel::<()>();
|
let (kill_tx, kill_rx) = oneshot::channel::<()>();
|
||||||
{
|
{
|
||||||
let sink = sink.clone();
|
let sink = sink.clone();
|
||||||
let label = format!("{} {where_it_runs}", provider.name);
|
let label = format!("{} {}", provider.name, transport.describe());
|
||||||
tokio::spawn(async move {
|
tokio::spawn(async move {
|
||||||
let status = tokio::select! {
|
let status = tokio::select! {
|
||||||
status = child.wait() => status.ok(),
|
status = child.wait() => status.ok(),
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ pub mod claude;
|
|||||||
pub mod driver;
|
pub mod driver;
|
||||||
pub mod echo;
|
pub mod echo;
|
||||||
pub mod transcript;
|
pub mod transcript;
|
||||||
|
pub mod transport;
|
||||||
|
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
@@ -28,6 +29,7 @@ use claude::ClaudeDriver;
|
|||||||
use driver::{Driver, Event, ImageRef, SessionStatus};
|
use driver::{Driver, Event, ImageRef, SessionStatus};
|
||||||
use echo::EchoDriver;
|
use echo::EchoDriver;
|
||||||
use transcript::{SeqEvent, Transcript};
|
use transcript::{SeqEvent, Transcript};
|
||||||
|
use transport::Transport;
|
||||||
|
|
||||||
/// Fan-out buffer per session. A subscriber that falls further behind than
|
/// Fan-out buffer per session. A subscriber that falls further behind than
|
||||||
/// this is caught up from the transcript file instead (see `routes`), so
|
/// this is caught up from the transcript file instead (see `routes`), so
|
||||||
@@ -472,7 +474,7 @@ fn launch(
|
|||||||
DriverKind::ClaudeCli => Box::new(ClaudeDriver::spawn(
|
DriverKind::ClaudeCli => Box::new(ClaudeDriver::spawn(
|
||||||
&meta,
|
&meta,
|
||||||
provider,
|
provider,
|
||||||
host,
|
&Transport::for_host(host),
|
||||||
&dir,
|
&dir,
|
||||||
sink.clone(),
|
sink.clone(),
|
||||||
)?),
|
)?),
|
||||||
|
|||||||
@@ -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<String>,
|
||||||
|
pub cwd: Option<PathBuf>,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl Launch {
|
||||||
|
pub fn new(program: impl Into<String>, args: Vec<String>, 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<Child> {
|
||||||
|
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),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user