refactor(maestro): route every client through the shared DaemonClient
ci / rust (pull_request) Successful in 3m0s
ci / rust (pull_request) Successful in 3m0s
The CLI, TUI and end-to-end harness each carried their own client: three reqwest builders with three timeout policies, three copies of the success-or-body check, and route strings at every call site. Two of them discarded the daemon's structured error and reported only a status code. Deletes `maestro-cli/src/http.rs`, both `ws_base` copies in the TUI, the CLI's partial `DaemonInfo` (it dropped `version` and `presets`), and the e2e harness's `check_ok`. `maestro-web` gains a protocol dependency and stops building the error envelope with a format string and reading it back as untyped JSON; the daemon and the proxy now register routes from the same constants. `maestro_web::router` returns `Result` — an unusable daemon URL fails at startup instead of at first connect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
01a1073943
commit
02356f84ab
@@ -17,10 +17,8 @@ dirs.workspace = true
|
||||
futures-util.workspace = true
|
||||
libc.workspace = true
|
||||
maestro-config.workspace = true
|
||||
maestro-protocol.workspace = true
|
||||
maestro-protocol = { workspace = true, features = ["client"] }
|
||||
nix.workspace = true
|
||||
reqwest.workspace = true
|
||||
serde.workspace = true
|
||||
serde_json.workspace = true
|
||||
thiserror.workspace = true
|
||||
tokio = { workspace = true, features = ["macros", "rt", "sync", "time", "io-util", "io-std", "signal"] }
|
||||
|
||||
@@ -13,7 +13,6 @@ use maestro_protocol::ClientControl;
|
||||
use nix::sys::termios::{cfmakeraw, tcgetattr, tcsetattr, SetArg, Termios};
|
||||
use tokio::io::{AsyncReadExt, AsyncWriteExt};
|
||||
use tokio_tungstenite::tungstenite::Message;
|
||||
use uuid::Uuid;
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
#[allow(clippy::result_large_err)]
|
||||
@@ -24,8 +23,8 @@ pub enum AttachError {
|
||||
Io(#[from] io::Error),
|
||||
#[error("termios: {0}")]
|
||||
Termios(#[from] nix::Error),
|
||||
#[error("invalid daemon_url: {0}")]
|
||||
InvalidDaemonUrl(String),
|
||||
#[error(transparent)]
|
||||
Url(#[from] maestro_protocol::routes::UrlError),
|
||||
#[error("invalid detach key: {0}")]
|
||||
InvalidDetachKey(String),
|
||||
#[error("stdin must be a TTY for attach")]
|
||||
@@ -63,26 +62,6 @@ pub fn parse_detach_key(spec: &str) -> Result<u8, AttachError> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Build the attach WS URL from the daemon's HTTP base.
|
||||
///
|
||||
/// The endpoint (`WS /api/attach/:id`) lives on the daemon's `bind_addr`, so we
|
||||
/// take the HTTP base (`http://127.0.0.1:7070`), swap the scheme to its WS
|
||||
/// equivalent, and append the route.
|
||||
#[allow(clippy::result_large_err)]
|
||||
pub fn attach_url(daemon_http_base: &str, session_id: Uuid) -> Result<String, AttachError> {
|
||||
let base = daemon_http_base.trim_end_matches('/');
|
||||
let ws_base = if let Some(rest) = base.strip_prefix("https://") {
|
||||
format!("wss://{rest}")
|
||||
} else if let Some(rest) = base.strip_prefix("http://") {
|
||||
format!("ws://{rest}")
|
||||
} else if base.starts_with("ws://") || base.starts_with("wss://") {
|
||||
base.to_string()
|
||||
} else {
|
||||
return Err(AttachError::InvalidDaemonUrl(daemon_http_base.to_string()));
|
||||
};
|
||||
Ok(format!("{ws_base}/api/attach/{session_id}"))
|
||||
}
|
||||
|
||||
/// RAII guard that puts the terminal into raw mode and restores on drop.
|
||||
#[derive(Debug)]
|
||||
struct RawTty {
|
||||
@@ -234,7 +213,6 @@ pub async fn run_attach_url(url: &str, detach_byte: u8) -> Result<(), AttachErro
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use uuid::Uuid;
|
||||
|
||||
#[test]
|
||||
fn parse_detach_key_handles_caret_form() {
|
||||
@@ -255,30 +233,4 @@ mod tests {
|
||||
Err(AttachError::InvalidDetachKey(_))
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn attach_url_swaps_scheme_and_appends_route() {
|
||||
let id = Uuid::from_u128(0xabcd);
|
||||
assert_eq!(
|
||||
attach_url("http://127.0.0.1:7070", id).unwrap(),
|
||||
format!("ws://127.0.0.1:7070/api/attach/{id}")
|
||||
);
|
||||
assert_eq!(
|
||||
attach_url("https://laptop.lan:7070/", id).unwrap(),
|
||||
format!("wss://laptop.lan:7070/api/attach/{id}")
|
||||
);
|
||||
// already-ws bases pass through
|
||||
assert_eq!(
|
||||
attach_url("ws://10.0.0.1:7070", id).unwrap(),
|
||||
format!("ws://10.0.0.1:7070/api/attach/{id}")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn attach_url_rejects_unknown_scheme() {
|
||||
assert!(matches!(
|
||||
attach_url("ftp://x", Uuid::nil()),
|
||||
Err(AttachError::InvalidDaemonUrl(_))
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,10 +6,11 @@
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use crate::attach::{attach_url, parse_detach_key, run_attach_url};
|
||||
use maestro_protocol::DaemonClient;
|
||||
|
||||
use crate::attach::{parse_detach_key, run_attach_url};
|
||||
use crate::cli::{AttachArgs, Command, KillArgs, ListArgs, SpawnArgs};
|
||||
use crate::config::{default_config_path, CliConfig, ConfigError};
|
||||
use crate::http::DaemonClient;
|
||||
use crate::sessions::{run_kill, run_list, run_spawn};
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
@@ -18,7 +19,7 @@ pub enum CliError {
|
||||
#[error("config: {0}")]
|
||||
Config(#[from] ConfigError),
|
||||
#[error("{0}")]
|
||||
Http(#[from] crate::http::HttpError),
|
||||
Client(#[from] maestro_protocol::ClientError),
|
||||
#[error("{0}")]
|
||||
Attach(#[from] crate::attach::AttachError),
|
||||
#[error("{0}")]
|
||||
@@ -73,7 +74,7 @@ async fn spawn(
|
||||
println!("spawned {}", session.id);
|
||||
|
||||
if attach_after {
|
||||
let url = attach_url(daemon.base(), session.id)?;
|
||||
let url = daemon.attach_url(session.id)?;
|
||||
run_attach_url(&url, detach_key).await?;
|
||||
}
|
||||
Ok(())
|
||||
@@ -87,7 +88,7 @@ async fn attach(
|
||||
) -> Result<(), CliError> {
|
||||
let daemon = client(config_path, daemon_url_override)?;
|
||||
let detach = parse_detach_key(&args.detach_key)?;
|
||||
let url = attach_url(daemon.base(), args.session_id)?;
|
||||
let url = daemon.attach_url(args.session_id)?;
|
||||
run_attach_url(&url, detach).await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1,161 +0,0 @@
|
||||
//! Thin HTTP client wrapper around `reqwest` for the daemon's `/api/*` surface.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
use reqwest::{Method, Response, StatusCode};
|
||||
use serde::de::DeserializeOwned;
|
||||
use serde::Serialize;
|
||||
|
||||
use crate::config::ConfigError;
|
||||
|
||||
/// Time budget for establishing a TCP connection to the daemon.
|
||||
const CONNECT_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
/// Total time budget for a single request (connect + send + receive). Bounds
|
||||
/// the case where the peer is TCP-alive but never makes forward progress.
|
||||
const REQUEST_TIMEOUT: Duration = Duration::from_secs(30);
|
||||
|
||||
/// Build a `reqwest::Client` with the CLI's standard timeouts and user-agent.
|
||||
///
|
||||
/// Centralizing construction here guarantees every client the CLI uses gets a
|
||||
/// connect *and* request timeout, so `maestro` never hangs forever on an
|
||||
/// unhealthy daemon (NAP-262).
|
||||
pub fn build_client() -> reqwest::Client {
|
||||
build_client_with_timeouts(CONNECT_TIMEOUT, REQUEST_TIMEOUT)
|
||||
}
|
||||
|
||||
fn build_client_with_timeouts(connect: Duration, request: Duration) -> reqwest::Client {
|
||||
reqwest::Client::builder()
|
||||
.user_agent(concat!("maestro-cli/", env!("CARGO_PKG_VERSION")))
|
||||
.connect_timeout(connect)
|
||||
.timeout(request)
|
||||
.build()
|
||||
.expect("build reqwest client")
|
||||
}
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum HttpError {
|
||||
#[error("http: {0}")]
|
||||
Reqwest(#[from] reqwest::Error),
|
||||
#[error("server returned {status}: {body}")]
|
||||
ServerError { status: StatusCode, body: String },
|
||||
#[error("decode: {0}")]
|
||||
Json(#[from] serde_json::Error),
|
||||
#[error("config: {0}")]
|
||||
Config(#[from] ConfigError),
|
||||
}
|
||||
|
||||
impl HttpError {
|
||||
/// True when the error is a transport-level failure to reach the host
|
||||
/// (connection refused / connect error) rather than an HTTP-level
|
||||
/// rejection.
|
||||
pub fn is_unreachable(&self) -> bool {
|
||||
matches!(self, HttpError::Reqwest(e) if e.is_connect())
|
||||
}
|
||||
}
|
||||
|
||||
/// HTTP client bound to one daemon's `/api/*` base URL. The daemon's surface
|
||||
/// needs no auth — reaching its bind address is the authorization.
|
||||
#[derive(Debug)]
|
||||
pub struct DaemonClient {
|
||||
base: String,
|
||||
inner: reqwest::Client,
|
||||
}
|
||||
|
||||
impl DaemonClient {
|
||||
pub fn new(base: String) -> Self {
|
||||
Self {
|
||||
base: base.trim_end_matches('/').to_string(),
|
||||
inner: build_client(),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn base(&self) -> &str {
|
||||
&self.base
|
||||
}
|
||||
|
||||
async fn send(
|
||||
&self,
|
||||
method: Method,
|
||||
path: &str,
|
||||
body: Option<serde_json::Value>,
|
||||
) -> Result<Response, HttpError> {
|
||||
let url = format!("{}{}", self.base, path);
|
||||
let mut req = self.inner.request(method, &url);
|
||||
if let Some(b) = body {
|
||||
req = req.json(&b);
|
||||
}
|
||||
Ok(req.send().await?)
|
||||
}
|
||||
|
||||
pub async fn get_json<T: DeserializeOwned>(&self, path: &str) -> Result<T, HttpError> {
|
||||
let resp = self.send(Method::GET, path, None).await?;
|
||||
ok_or_body(resp)
|
||||
.await?
|
||||
.json::<T>()
|
||||
.await
|
||||
.map_err(Into::into)
|
||||
}
|
||||
|
||||
pub async fn post_json<B: Serialize, T: DeserializeOwned>(
|
||||
&self,
|
||||
path: &str,
|
||||
body: &B,
|
||||
) -> Result<T, HttpError> {
|
||||
let v = serde_json::to_value(body)?;
|
||||
let resp = self.send(Method::POST, path, Some(v)).await?;
|
||||
ok_or_body(resp)
|
||||
.await?
|
||||
.json::<T>()
|
||||
.await
|
||||
.map_err(Into::into)
|
||||
}
|
||||
|
||||
pub async fn delete(&self, path: &str) -> Result<(), HttpError> {
|
||||
let resp = self.send(Method::DELETE, path, None).await?;
|
||||
ok_or_body(resp).await?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
async fn ok_or_body(resp: Response) -> Result<Response, HttpError> {
|
||||
let status = resp.status();
|
||||
if status.is_success() {
|
||||
return Ok(resp);
|
||||
}
|
||||
let body = resp.text().await.unwrap_or_default();
|
||||
Err(HttpError::ServerError { status, body })
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// A peer that accepts the TCP connection but never sends a response must
|
||||
/// not hang the client forever: the request timeout has to fire (NAP-262).
|
||||
#[tokio::test]
|
||||
async fn request_timeout_fires_when_peer_never_responds() {
|
||||
use std::io::Read;
|
||||
use std::net::TcpListener;
|
||||
|
||||
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
let addr = listener.local_addr().unwrap();
|
||||
// Accept one connection and sit on it without ever writing a response.
|
||||
let handle = std::thread::spawn(move || {
|
||||
if let Ok((mut sock, _)) = listener.accept() {
|
||||
let mut buf = [0u8; 1024];
|
||||
let _ = sock.read(&mut buf);
|
||||
std::thread::sleep(Duration::from_secs(2));
|
||||
}
|
||||
});
|
||||
|
||||
let client = build_client_with_timeouts(Duration::from_secs(5), Duration::from_millis(300));
|
||||
let err = client
|
||||
.get(format!("http://{addr}/"))
|
||||
.send()
|
||||
.await
|
||||
.expect_err("request should time out, not hang");
|
||||
assert!(err.is_timeout(), "expected timeout error, got: {err}");
|
||||
|
||||
let _ = handle.join();
|
||||
}
|
||||
}
|
||||
@@ -6,7 +6,6 @@ pub mod attach;
|
||||
pub mod cli;
|
||||
pub mod config;
|
||||
pub mod dispatch;
|
||||
pub mod http;
|
||||
pub mod sessions;
|
||||
|
||||
pub use dispatch::{dispatch, CliError};
|
||||
|
||||
@@ -3,31 +3,22 @@
|
||||
use std::collections::HashMap;
|
||||
use std::io::Write;
|
||||
|
||||
use maestro_protocol::{ActivityState, CreateSessionRequest, Session, SessionStatus, WorktreeSpec};
|
||||
use serde::Deserialize;
|
||||
use maestro_protocol::{
|
||||
ActivityState, ClientError, CreateSessionRequest, DaemonClient, DaemonInfo, Session,
|
||||
SessionStatus, WorktreeSpec,
|
||||
};
|
||||
use serde_json::Value;
|
||||
|
||||
use crate::cli::{KillArgs, ListArgs, SpawnArgs};
|
||||
use crate::http::{DaemonClient, HttpError};
|
||||
|
||||
/// The bits of `GET /api/info` the CLI renders. Deserialized structurally so
|
||||
/// the CLI doesn't depend on the daemon crate.
|
||||
#[derive(Debug, Clone, Default, Deserialize)]
|
||||
pub struct DaemonInfo {
|
||||
#[serde(default)]
|
||||
pub hostname: String,
|
||||
#[serde(default)]
|
||||
pub home_dir: String,
|
||||
}
|
||||
|
||||
/// `maestro list`
|
||||
pub async fn run_list(
|
||||
client: &DaemonClient,
|
||||
args: &ListArgs,
|
||||
out: &mut impl Write,
|
||||
) -> Result<(), HttpError> {
|
||||
let sessions: Vec<Session> = client.get_json("/api/sessions").await?;
|
||||
let info: DaemonInfo = client.get_json("/api/info").await.unwrap_or_default();
|
||||
) -> Result<(), ClientError> {
|
||||
let sessions = client.list_sessions().await?;
|
||||
let info = client.daemon_info().await.unwrap_or_default();
|
||||
|
||||
if args.json {
|
||||
let body = serde_json::json!({ "sessions": sessions });
|
||||
@@ -40,7 +31,7 @@ pub async fn run_list(
|
||||
}
|
||||
|
||||
/// `maestro spawn`
|
||||
pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result<Session, HttpError> {
|
||||
pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result<Session, ClientError> {
|
||||
let mut env: HashMap<String, String> = HashMap::new();
|
||||
for (k, v) in args.env.iter() {
|
||||
env.insert(k.clone(), v.clone());
|
||||
@@ -64,15 +55,12 @@ pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result<Session
|
||||
metadata,
|
||||
worktree,
|
||||
};
|
||||
let session: Session = client.post_json("/api/sessions", &body).await?;
|
||||
Ok(session)
|
||||
client.create_session(&body).await
|
||||
}
|
||||
|
||||
/// `maestro kill`
|
||||
pub async fn run_kill(client: &DaemonClient, args: &KillArgs) -> Result<(), HttpError> {
|
||||
client
|
||||
.delete(&format!("/api/sessions/{}", args.session_id))
|
||||
.await
|
||||
pub async fn run_kill(client: &DaemonClient, args: &KillArgs) -> Result<(), ClientError> {
|
||||
client.kill_session(args.session_id).await
|
||||
}
|
||||
|
||||
#[allow(clippy::write_literal)]
|
||||
@@ -167,6 +155,7 @@ mod tests {
|
||||
DaemonInfo {
|
||||
hostname: "naps-laptop".into(),
|
||||
home_dir: home.into(),
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user