Drive the run terminal through the driver's PtySession

Fabro's TerminalSession trait, its DriverTerminalSession wrapper, and
TerminalSize were four method forwards and a size struct over the
driver's PtySession and PtySize. RunSandbox::open_terminal now returns
the driver's session, the server's websocket loop drives it directly and
renders its errors with display_for_log, and open_terminal_for_run sits
with the other reconnect helpers. terminal.rs is deleted.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

lib/apps/fabro-server/src/server/handler/sandbox.rs

@@ -6,8 +6,7 @@ use std::time::Duration;
 
 use axum::extract::ws::{Message as WsMessage, WebSocket, WebSocketUpgrade};
 use fabro_sandbox::{
-    FileKind, ProviderAccess, RunSandbox, TerminalSize, open_terminal_for_run,
-    reconnect_driver_for_run,
+    FileKind, ProviderAccess, PtySize, RunSandbox, open_terminal_for_run, reconnect_driver_for_run,
 };
 use fabro_types::{
     RunSandboxInstance, SandboxProviderKind, SandboxServiceDiscoverySource, SandboxServiceListMeta,
@@ -133,7 +132,7 @@ struct SandboxFileParams {
 
 #[derive(Debug, PartialEq, Eq)]
 enum TerminalClientMessage {
-    Resize(TerminalSize),
+    Resize(PtySize),
     Close,
 }
 
@@ -150,7 +149,7 @@ fn parse_terminal_control_message(text: &str) -> Result<TerminalClientMessage, &
     }
     match serde_json::from_str::<TerminalClientControl>(text) {
         Ok(TerminalClientControl::Resize { cols, rows }) if cols > 0 && rows > 0 => {
-            Ok(TerminalClientMessage::Resize(TerminalSize { cols, rows }))
+            Ok(TerminalClientMessage::Resize(PtySize { cols, rows }))
         }
         Ok(TerminalClientControl::Resize { .. }) => {
             Err("Terminal resize dimensions must be greater than zero.")
@@ -235,19 +234,19 @@ async fn terminal_websocket(mut socket: WebSocket, state: Arc<AppState>, id: Run
             return;
         }
     };
-    let session =
-        match open_terminal_for_run(&record, &access, Some(id), TerminalSize::default()).await {
-            Ok(session) => session,
-            Err(err) => {
-                let _ = socket
-                    .send(terminal_server_text(
-                        "error",
-                        Some(&err.display_with_causes()),
-                    ))
-                    .await;
-                return;
-            }
-        };
+    let session = match open_terminal_for_run(&record, &access, Some(id), PtySize::default()).await
+    {
+        Ok(session) => session,
+        Err(err) => {
+            let _ = socket
+                .send(terminal_server_text(
+                    "error",
+                    Some(&err.display_with_causes()),
+                ))
+                .await;
+            return;
+        }
+    };
 
     if socket
         .send(terminal_server_text("ready", None))
@@ -268,7 +267,7 @@ async fn terminal_websocket(mut socket: WebSocket, state: Arc<AppState>, id: Run
                     Ok(WsMessage::Binary(bytes)) => {
                         if let Err(err) = session.write_input(&bytes).await {
                             let _ = socket
-                                .send(terminal_server_text("error", Some(&err.display_with_causes())))
+                                .send(terminal_server_text("error", Some(&fabro_sandbox::display_for_log(&err))))
                                 .await;
                             break;
                         }
@@ -278,7 +277,7 @@ async fn terminal_websocket(mut socket: WebSocket, state: Arc<AppState>, id: Run
                             Ok(TerminalClientMessage::Resize(size)) => {
                                 if let Err(err) = session.resize(size).await {
                                     let _ = socket
-                                        .send(terminal_server_text("error", Some(&err.display_with_causes())))
+                                        .send(terminal_server_text("error", Some(&fabro_sandbox::display_for_log(&err))))
                                         .await;
                                     break;
                                 }
@@ -313,7 +312,7 @@ async fn terminal_websocket(mut socket: WebSocket, state: Arc<AppState>, id: Run
                     }
                     Err(err) => {
                         let _ = socket
-                            .send(terminal_server_text("error", Some(&err.display_with_causes())))
+                            .send(terminal_server_text("error", Some(&fabro_sandbox::display_for_log(&err))))
                             .await;
                         break;
                     }
@@ -322,7 +321,7 @@ async fn terminal_websocket(mut socket: WebSocket, state: Arc<AppState>, id: Run
         }
     }
     if let Err(err) = session.close().await {
-        tracing::warn!(error = %err.display_with_causes(), run_id = %id, "failed to close run terminal session");
+        tracing::warn!(error = %fabro_sandbox::display_for_log(&err), run_id = %id, "failed to close run terminal session");
     }
 }
 
@@ -938,7 +937,7 @@ mod tests {
     fn terminal_control_accepts_resize_and_close() {
         assert_eq!(
             parse_terminal_control_message(r#"{"type":"resize","cols":120,"rows":32}"#),
-            Ok(TerminalClientMessage::Resize(TerminalSize {
+            Ok(TerminalClientMessage::Resize(PtySize {
                 cols: 120,
                 rows: 32,
             }))

lib/components/fabro-sandbox/src/driver_sandbox.rs

@@ -24,9 +24,9 @@ use fabro_types::SandboxProviderKind;
 use fabro_util::workspace_glob::WorkspaceGlob;
 use sandbox_driver::{
     DirEntry, EventContext, ExecControls, ExecResult, ExecSpec, ExecStreamingResult, FileKind,
-    GrepMatch, GrepOptions, LifecycleTimers, PtyOptions, PtySize, Sandbox as DriverHandle,
-    SandboxProvider as DriverProvider, SandboxSource, SandboxSpec as DriverSpec, SandboxState,
-    Search as _, StdioProcess, WaitOptions, WalkOptions,
+    GrepMatch, GrepOptions, LifecycleTimers, PtyOptions, PtySession, PtySize,
+    Sandbox as DriverHandle, SandboxProvider as DriverProvider, SandboxSource,
+    SandboxSpec as DriverSpec, SandboxState, Search as _, StdioProcess, WaitOptions, WalkOptions,
 };
 use sandbox_driver_host::HostProvider;
 use tokio::fs;
@@ -36,7 +36,6 @@ use tokio_util::sync::CancellationToken;
 use crate::clone::{self, GitHubClone};
 use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason};
 use crate::push_credentials::{self, PushCredentialState};
-use crate::terminal::{DriverTerminalSession, TerminalSize};
 use crate::{GitRunInfo, GitSetupIntent, RefreshOutcome, RetryPlan};
 
 /// A sandbox on the worker host at `working_directory`, the fabro `local`
@@ -620,7 +619,7 @@ impl RunSandbox {
 
     /// Open an interactive shell in the sandbox's working directory over the
     /// driver's Pty facet.
-    pub async fn open_terminal(&self, size: TerminalSize) -> crate::Result<DriverTerminalSession> {
+    pub async fn open_terminal(&self, size: PtySize) -> crate::Result<Box<dyn PtySession>> {
         let handle = self.handle()?;
         let pty = handle.pty().ok_or_else(|| {
             crate::Error::message(format!(
@@ -629,16 +628,11 @@ impl RunSandbox {
             ))
         })?;
         let mut options = PtyOptions::default();
-        options.size = PtySize {
-            rows: size.rows,
-            cols: size.cols,
-        };
+        options.size = size;
         options.working_dir = Some(self.working_directory().to_string());
-        let session = pty
-            .open(&options)
+        pty.open(&options)
             .await
-            .map_err(|error| crate::Error::context("Failed to open sandbox terminal", error))?;
-        Ok(DriverTerminalSession::new(session))
+            .map_err(|error| crate::Error::context("Failed to open sandbox terminal", error))
     }
 
     /// Ask the sandbox for its platform once; `platform` and `os_version`

lib/components/fabro-sandbox/src/lib.rs

@@ -23,8 +23,6 @@ pub mod exec;
 
 pub mod reconnect;
 
-pub mod terminal;
-
 mod clone;
 pub mod docker;
 pub mod provider_sandbox;
@@ -61,7 +59,8 @@ pub use provider::{
 pub use provider_sandbox::{attach_provider_sandbox, provider_sandbox};
 pub use push_credentials::RefreshErrorKind;
 pub use reconnect::{
-    reconnect, reconnect_driver_for_run, reconnect_for_run, reconnect_for_run_with_events,
+    open_terminal_for_run, reconnect, reconnect_driver_for_run, reconnect_for_run,
+    reconnect_for_run_with_events,
 };
 pub use sandbox::{
     DEFAULT_EXEC_OUTPUT_TAIL_BYTES, GitRunInfo, GitSetupIntent, PushAttempt, PushError, PushReport,
@@ -74,8 +73,7 @@ pub use sandbox::{
 /// driver dependency.
 pub use sandbox_driver::{
     CaptureStats, DirEntry, ExecControls, ExecFailure, ExecResult, ExecSpec, ExecStreamingResult,
-    FileKind, GrepMatch, GrepOptions, NetworkPolicy, OutputSink, OutputStream, StderrTail,
-    StdioProcess, StdioProcessHandle, Termination, TransportError, WalkOptions,
+    FileKind, GrepMatch, GrepOptions, NetworkPolicy, OutputSink, OutputStream, PtySession, PtySize,
+    StderrTail, StdioProcess, StdioProcessHandle, Termination, TransportError, WalkOptions,
 };
 pub use sandbox_spec::{ProviderSandboxSpec, SandboxSpec};
-pub use terminal::{DriverTerminalSession, TerminalSession, TerminalSize, open_terminal_for_run};

lib/components/fabro-sandbox/src/reconnect.rs

@@ -2,7 +2,7 @@ use std::path::PathBuf;
 
 use anyhow::{Context, Result};
 use fabro_types::{BundledProvider, RunId, RunSandboxInstance};
-use sandbox_driver::EventContext;
+use sandbox_driver::{EventContext, PtySession, PtySize};
 
 use crate::driver::ProviderAccess;
 use crate::driver_sandbox::{RunSandbox, local_sandbox_with_events};
@@ -72,3 +72,24 @@ pub async fn reconnect_driver_for_run(
     };
     Ok(sandbox)
 }
+
+/// Opens an interactive shell in a run's sandbox over the driver's Pty
+/// facet, reconnecting from the run record first. The session is the
+/// driver's own; it is closed by the caller.
+pub async fn open_terminal_for_run(
+    record: &RunSandboxInstance,
+    access: &ProviderAccess,
+    run_id: Option<RunId>,
+    size: PtySize,
+) -> crate::Result<Box<dyn PtySession>> {
+    if record.provider.bundled() == Some(BundledProvider::Local) {
+        return Err(crate::Error::message(
+            "Local sandboxes do not support embedded terminals",
+        ));
+    }
+    let sandbox = reconnect_driver_for_run(record, access, run_id, None)
+        .await
+        .map_err(|err| crate::Error::context_anyhow("Failed to reconnect sandbox", err))?;
+    sandbox.activate().await?;
+    sandbox.open_terminal(size).await
+}

lib/components/fabro-sandbox/src/terminal.rs

@@ -1,92 +0,0 @@
-use async_trait::async_trait;
-use fabro_types::{BundledProvider, RunId, RunSandboxInstance};
-
-use crate::driver::ProviderAccess;
-use crate::reconnect;
-
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct TerminalSize {
-    pub cols: u16,
-    pub rows: u16,
-}
-
-impl Default for TerminalSize {
-    fn default() -> Self {
-        Self {
-            cols: 120,
-            rows: 32,
-        }
-    }
-}
-
-#[async_trait]
-pub trait TerminalSession: Send + Sync {
-    async fn write_input(&self, bytes: &[u8]) -> crate::Result<()>;
-    async fn read_output(&self) -> crate::Result<Option<Vec<u8>>>;
-    async fn resize(&self, size: TerminalSize) -> crate::Result<()>;
-    async fn close(&self) -> crate::Result<()>;
-}
-
-/// A terminal over the driver's Pty facet.
-pub struct DriverTerminalSession {
-    session: Box<dyn sandbox_driver::PtySession>,
-}
-
-impl DriverTerminalSession {
-    #[must_use]
-    pub fn new(session: Box<dyn sandbox_driver::PtySession>) -> Self {
-        Self { session }
-    }
-}
-
-#[async_trait]
-impl TerminalSession for DriverTerminalSession {
-    async fn write_input(&self, bytes: &[u8]) -> crate::Result<()> {
-        self.session
-            .write_input(bytes)
-            .await
-            .map_err(|err| crate::Error::context("Failed to write terminal input", err))
-    }
-
-    async fn read_output(&self) -> crate::Result<Option<Vec<u8>>> {
-        self.session
-            .read_output()
-            .await
-            .map_err(|err| crate::Error::context("Failed to read terminal output", err))
-    }
-
-    async fn resize(&self, size: TerminalSize) -> crate::Result<()> {
-        self.session
-            .resize(sandbox_driver::PtySize {
-                rows: size.rows,
-                cols: size.cols,
-            })
-            .await
-            .map_err(|err| crate::Error::context("Failed to resize terminal", err))
-    }
-
-    async fn close(&self) -> crate::Result<()> {
-        self.session
-            .close()
-            .await
-            .map_err(|err| crate::Error::context("Failed to close terminal", err))
-    }
-}
-
-pub async fn open_terminal_for_run(
-    record: &RunSandboxInstance,
-    access: &ProviderAccess,
-    run_id: Option<RunId>,
-    size: TerminalSize,
-) -> crate::Result<Box<dyn TerminalSession>> {
-    if record.provider.bundled() == Some(BundledProvider::Local) {
-        return Err(crate::Error::message(
-            "Local sandboxes do not support embedded terminals",
-        ));
-    }
-    let sandbox = reconnect::reconnect_driver_for_run(record, access, run_id, None)
-        .await
-        .map_err(|err| crate::Error::context_anyhow("Failed to reconnect sandbox", err))?;
-    sandbox.activate().await?;
-    Ok(Box::new(sandbox.open_terminal(size).await?))
-}