From 99e57627e86d4c57eb6102e0b41d5758bacc8b25 Mon Sep 17 00:00:00 2001 From: gamnaansong Date: Mon, 1 Jun 2026 08:07:47 +0000 Subject: [PATCH] fix(cli): info/logs honor --profile and BOXLITE_PROFILE (POL-30/31) info.rs and logs.rs called create_runtime_with_options(), which always builds a local runtime regardless of --profile / BOXLITE_PROFILE / --url. Every other command uses create_runtime(), which applies the URL precedence ladder and routes to REST when a profile carries one. Switching info and logs to create_runtime() makes them route correctly: info renders the REST URL in place of homeDir + a "remote" sentinel for virtualization and skips the images count until REST image endpoints land (POL-32); logs has no REST equivalent yet, so it errors cleanly ("not yet supported over REST") instead of silently looking up the box in the wrong environment. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/boxlite/src/runtime/core.rs | 13 ++ src/cli/src/cli.rs | 132 ++++++++++++++++++++ src/cli/src/commands/info.rs | 46 +++++-- src/cli/src/commands/logs.rs | 19 ++- src/cli/tests/profile_routing.rs | 208 +++++++++++++++++++++++++++++++ 5 files changed, 406 insertions(+), 12 deletions(-) create mode 100644 src/cli/tests/profile_routing.rs diff --git a/src/boxlite/src/runtime/core.rs b/src/boxlite/src/runtime/core.rs index b9a42f3e9..516fc6eae 100644 --- a/src/boxlite/src/runtime/core.rs +++ b/src/boxlite/src/runtime/core.rs @@ -455,6 +455,19 @@ impl BoxliteRuntime { )), } } + + /// True iff this runtime is REST-backed (vs. a local VM backend). + /// + /// CLI commands that mix capabilities only meaningful on one side + /// (e.g. `info` showing host home_dir + virtualization vs. a remote + /// URL, or `logs` reading a host file that doesn't exist over REST) + /// need to render different output by backend kind. Keying on + /// `auth_backend` is stable across the planned image-over-REST + /// expansion — REST is identified by "has a remote identity," not + /// by "lacks image ops." + pub fn is_rest(&self) -> bool { + self.auth_backend.is_some() + } } // ============================================================================ diff --git a/src/cli/src/cli.rs b/src/cli/src/cli.rs index 999edd019..41eb33a72 100644 --- a/src/cli/src/cli.rs +++ b/src/cli/src/cli.rs @@ -193,6 +193,22 @@ impl GlobalFlags { .to_string() } + /// REST URL the runtime would talk to, applying the same precedence as + /// `create_runtime`: `--url` / `BOXLITE_REST_URL` > stored profile's + /// URL > none. Returns `None` when no REST destination is configured + /// (the local runtime case). Used by commands like `info` to render + /// "where am I pointing at" without rebuilding the precedence ladder. + pub fn resolved_url(&self) -> Option { + if let Some(u) = self.url.as_deref().filter(|s| !s.is_empty()) { + return Some(u.to_string()); + } + crate::credentials::load_named(&self.resolved_profile()) + .ok() + .flatten() + .map(|p| p.url) + .filter(|u| !u.is_empty()) + } + /// Resolve runtime options from config file and CLI overrides (--home, --registry). pub fn resolve_runtime_options(&self) -> anyhow::Result { let mut options = if let Some(config_path) = &self.config { @@ -1142,4 +1158,120 @@ mod tests { }; assert!(login.api_key_stdin); } + + /// Lay down a credentials file readable by `credentials::load_named` + /// rooted at `home`, and point `BOXLITE_HOME` at it for the duration of + /// the test. Returns a guard that restores the previous `BOXLITE_HOME` + /// (and releases the cross-test lock) on drop. + fn install_creds(home: &std::path::Path, body: &str) -> EnvGuard { + std::fs::create_dir_all(home).unwrap(); + std::fs::write(home.join("credentials.toml"), body).unwrap(); + EnvGuard::set("BOXLITE_HOME", home.to_str().unwrap()) + } + + /// All tests that mutate `BOXLITE_HOME` must serialize through this + /// mutex — `cargo test` runs tests in the same process by default and + /// env vars are process-global, so a parallel test would see another + /// test's override and pick up the wrong credentials.toml. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + struct EnvGuard { + key: String, + prev: Option, + _lock: std::sync::MutexGuard<'static, ()>, + } + impl EnvGuard { + fn set(key: &str, val: &str) -> Self { + let lock = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + let prev = std::env::var_os(key); + // SAFETY: the mutex above guarantees no other test in this + // module is touching env vars; the lock outlives the set/remove + // calls below via `_lock` held in this struct. + unsafe { std::env::set_var(key, val) }; + Self { + key: key.to_string(), + prev, + _lock: lock, + } + } + } + impl Drop for EnvGuard { + fn drop(&mut self) { + unsafe { + match &self.prev { + Some(v) => std::env::set_var(&self.key, v), + None => std::env::remove_var(&self.key), + } + } + } + } + + fn flags_with_profile(profile: Option<&str>, url: Option<&str>) -> GlobalFlags { + GlobalFlags { + debug: false, + home: None, + registry: vec![], + config: None, + url: url.map(str::to_string), + profile: profile.map(str::to_string), + path_prefix: None, + } + } + + /// `--url` / `BOXLITE_REST_URL` beats whatever the profile carries — + /// same precedence as `create_runtime`, exercised at the helper boundary + /// so `info` / `logs` can render "where am I pointing at" without + /// re-deriving the ladder. + #[test] + fn resolved_url_prefers_explicit_url_over_profile() { + let tmp = TempDir::new().unwrap(); + let _g = install_creds( + tmp.path(), + "[profiles.p1]\nurl = \"http://from-profile:1\"\nauth_method = \"api_key\"\n", + ); + let flags = flags_with_profile(Some("p1"), Some("http://from-flag:2")); + assert_eq!(flags.resolved_url().as_deref(), Some("http://from-flag:2")); + } + + /// With no `--url`, the stored profile's URL wins — the path that POL-30 + /// / POL-31 broke for `info` and `logs` while every other command + /// already honored it. + #[test] + fn resolved_url_falls_back_to_profile_url() { + let tmp = TempDir::new().unwrap(); + let _g = install_creds( + tmp.path(), + "[profiles.p1]\nurl = \"http://from-profile:1\"\nauth_method = \"api_key\"\n", + ); + let flags = flags_with_profile(Some("p1"), None); + assert_eq!( + flags.resolved_url().as_deref(), + Some("http://from-profile:1") + ); + } + + /// No `--url`, no matching profile → `None`. Caller (e.g. `info`) + /// then knows it's looking at a local runtime and can render local + /// fields (`home_dir`, virtualization probe). + #[test] + fn resolved_url_is_none_when_profile_missing() { + let tmp = TempDir::new().unwrap(); + let _g = install_creds(tmp.path(), ""); // empty credentials file + let flags = flags_with_profile(Some("missing"), None); + assert!(flags.resolved_url().is_none()); + } + + /// An explicitly empty URL in the profile is treated as "no URL" — + /// otherwise `info --profile p1` would silently produce an + /// unreachable connect on a malformed profile. + #[test] + fn resolved_url_treats_empty_string_as_none() { + let tmp = TempDir::new().unwrap(); + let _g = install_creds( + tmp.path(), + "[profiles.p1]\nurl = \"\"\nauth_method = \"api_key\"\n", + ); + let flags = flags_with_profile(Some("p1"), None); + assert!(flags.resolved_url().is_none()); + } } diff --git a/src/cli/src/commands/info.rs b/src/cli/src/commands/info.rs index fee3333bf..1f4d1c61b 100644 --- a/src/cli/src/commands/info.rs +++ b/src/cli/src/commands/info.rs @@ -1,11 +1,18 @@ use crate::cli::GlobalFlags; use crate::formatter; -use boxlite::BoxStatus; +use boxlite::{BoxStatus, BoxliteError}; use clap::Args; use clap::ValueEnum; use serde::Serialize; /// System-wide runtime information (CLI output shape). +/// +/// `homeDir` / `virtualization` are populated for the local backend; for +/// REST they become the URL string and a `"remote"` sentinel so the user +/// can see at a glance which environment the count fields describe. +/// `imagesCount` is omitted (set to `None`) when the backend doesn't +/// expose image listing — currently the REST backend, until image +/// endpoints land. #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] struct SystemInfo { @@ -18,7 +25,8 @@ struct SystemInfo { boxes_running: u32, boxes_stopped: u32, boxes_configured: u32, - images_count: u32, + #[serde(skip_serializing_if = "Option::is_none")] + images_count: Option, } /// Display system-wide runtime information (default: YAML). @@ -37,14 +45,26 @@ pub enum InfoFormat { } pub async fn execute(args: InfoArgs, global: &GlobalFlags) -> anyhow::Result<()> { - let options = global.resolve_runtime_options()?; - let home_dir = options.home_dir.to_string_lossy().to_string(); + let rt = global.create_runtime()?; + let is_rest = rt.is_rest(); + + let (home_dir, virtualization) = if is_rest { + // Local-only fields don't describe the environment the box/image + // counts come from; render the URL we're talking to instead. + let url = global + .resolved_url() + .unwrap_or_else(|| "(remote)".to_string()); + (url, "remote".to_string()) + } else { + let options = global.resolve_runtime_options()?; + let home = options.home_dir.to_string_lossy().to_string(); + let virt = boxlite::system_check::SystemCheck::run() + .map(|_| "available".to_string()) + .unwrap_or_else(|e| format!("unavailable: {}", e)); + (home, virt) + }; - let rt = global.create_runtime_with_options(options)?; let version = boxlite::VERSION.to_string(); - let virtualization = boxlite::system_check::SystemCheck::run() - .map(|_| "available".to_string()) - .unwrap_or_else(|e| format!("unavailable: {}", e)); let os = std::env::consts::OS.to_string(); let arch = std::env::consts::ARCH.to_string(); @@ -60,7 +80,15 @@ pub async fn execute(args: InfoArgs, global: &GlobalFlags) -> anyhow::Result<()> .filter(|b| b.status == BoxStatus::Configured) .count() as u32; - let images_count = rt.images()?.list().await?.len() as u32; + // Image listing is local-only today; surface it when the backend + // supports it and omit the field otherwise (rather than misreporting + // 0). When REST image endpoints land, removing this branch keeps the + // count visible. + let images_count = match rt.images() { + Ok(handle) => Some(handle.list().await?.len() as u32), + Err(BoxliteError::Unsupported(_)) => None, + Err(e) => return Err(e.into()), + }; let info = SystemInfo { version, diff --git a/src/cli/src/commands/logs.rs b/src/cli/src/commands/logs.rs index 2acac08df..b0c869df5 100644 --- a/src/cli/src/commands/logs.rs +++ b/src/cli/src/commands/logs.rs @@ -24,9 +24,22 @@ pub struct LogsArgs { } pub async fn execute(args: LogsArgs, global: &GlobalFlags) -> anyhow::Result<()> { - let options = global.resolve_runtime_options()?; - let home_dir = options.home_dir.clone(); - let rt = global.create_runtime_with_options(options)?; + let rt = global.create_runtime()?; + + // `logs` reads the box's console.log file directly from the local + // host's $BOXLITE_HOME — there is no REST endpoint for log streaming + // yet. Falling back to local would silently misdirect a `--profile` + // user to the wrong environment (POL-30/31). Error cleanly instead. + if rt.is_rest() { + return Err(anyhow::anyhow!( + "`logs` is not yet supported over REST — re-run without --profile / \ + BOXLITE_PROFILE / --url to read logs from the local runtime" + )); + } + + // Re-resolve options to learn the local home_dir. (We can't get it + // off the runtime — only the construction-time options had it.) + let home_dir = global.resolve_runtime_options()?.home_dir; let litebox = rt .get(&args.target) diff --git a/src/cli/tests/profile_routing.rs b/src/cli/tests/profile_routing.rs new file mode 100644 index 000000000..c8609ae98 --- /dev/null +++ b/src/cli/tests/profile_routing.rs @@ -0,0 +1,208 @@ +//! Integration tests for POL-30 / POL-31: `info` and `logs` must honor +//! `--profile` / `BOXLITE_PROFILE` instead of silently falling back to +//! the local runtime. Each test stands up a tiny HTTP stub on an +//! ephemeral port, writes a `credentials.toml` pointing at it, and runs +//! the CLI through `assert_cmd`. + +use std::io::{BufRead, BufReader, Write}; +use std::net::TcpListener; +use std::sync::Arc; +use std::time::Duration; + +use assert_cmd::Command; +use predicates::prelude::*; +use tempfile::TempDir; + +/// Minimal HTTP/1.1 stub. `handler(method, path) -> (status, json)`. +/// One request per connection (`Connection: close`); a daemon thread +/// serves sequential connections for the test's lifetime. +struct Stub { + port: u16, +} + +impl Stub { + fn start(handler: H) -> Self + where + H: Fn(&str, &str) -> (u16, String) + Send + Sync + 'static, + { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind ephemeral port"); + let port = listener.local_addr().unwrap().port(); + let handler = Arc::new(handler); + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + let Ok(peek) = stream.try_clone() else { + continue; + }; + let mut reader = BufReader::new(peek); + + let mut request_line = String::new(); + if reader.read_line(&mut request_line).is_err() { + continue; + } + loop { + let mut line = String::new(); + match reader.read_line(&mut line) { + Ok(0) => break, + Ok(_) if line == "\r\n" || line == "\n" => break, + Ok(_) => continue, + Err(_) => break, + } + } + + let mut parts = request_line.split_whitespace(); + let method = parts.next().unwrap_or(""); + let raw_path = parts.next().unwrap_or(""); + let path = raw_path.split('?').next().unwrap_or(raw_path); + + let (status, body) = handler(method, path); + let reason = match status { + 200 => "OK", + 400 => "Bad Request", + 401 => "Unauthorized", + 404 => "Not Found", + _ => "OK", + }; + let _ = write!( + stream, + "HTTP/1.1 {status} {reason}\r\nContent-Type: application/json\r\n\ + Content-Length: {len}\r\nConnection: close\r\n\r\n{body}", + len = body.len(), + ); + let _ = stream.flush(); + } + }); + Self { port } + } + + fn url(&self) -> String { + format!("http://127.0.0.1:{}", self.port) + } +} + +/// `boxlite …` with a hermetic env: BOXLITE_HOME points at `home`, +/// inherited BOXLITE_API_KEY / BOXLITE_REST_URL are scrubbed so they +/// can't leak in and decide the routing. +fn cli(home: &TempDir) -> Command { + let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("boxlite")); + cmd.env("BOXLITE_HOME", home.path()) + .env_remove("BOXLITE_API_KEY") + .env_remove("BOXLITE_REST_URL") + .env_remove("BOXLITE_PROFILE") + .timeout(Duration::from_secs(30)); + cmd +} + +/// Write a `credentials.toml` containing a profile named `p1` whose +/// URL points at `stub_url`. Returns the path for assertions. +fn write_p1_creds(home: &TempDir, stub_url: &str) -> std::path::PathBuf { + let path = home.path().join("credentials.toml"); + let body = format!( + "[profiles.p1]\nurl = \"{stub_url}\"\napi_key = \"k_test\"\nauth_method = \"api_key\"\n" + ); + std::fs::write(&path, body).unwrap(); + path +} + +/// POL-30: `info --profile p1` must call the stub server and surface +/// remote state, not the local runtime. The pre-fix code wired `info` +/// through `create_runtime_with_options(...)` — which is local-only — +/// so the URL never got dialed and the user saw the host's home_dir + +/// local box/image counts even though `--profile` named a REST target. +/// +/// Post-fix the URL is honored: the stub receives `GET /v1/boxes` and +/// the rendered output shows the stub URL in place of `homeDir` and +/// `remote` for `virtualization`. We assert on the URL substring (a +/// genuine REST-side signal that wouldn't appear in the pre-fix local +/// output) rather than just absence of `/home/`, so the test would +/// also catch a regression that prints a wrong URL. +#[test] +fn info_with_profile_routes_to_rest() { + let stub = Stub::start(|_m, path| { + if path.starts_with("/v1/boxes") { + (200, r#"{"boxes":[]}"#.to_string()) + } else { + ( + 404, + r#"{"error":{"message":"nope","type":"NotFoundError"}}"#.to_string(), + ) + } + }); + let home = TempDir::new().unwrap(); + write_p1_creds(&home, &stub.url()); + + cli(&home) + .args(["--profile", "p1", "info"]) + .assert() + .success() + .stdout( + predicate::str::contains(stub.url()) + .and(predicate::str::contains("virtualization: remote")), + ); +} + +/// POL-31: same bug, env-var entry point. Clap maps `BOXLITE_PROFILE` +/// into `GlobalFlags::profile`, so a regression that re-introduced the +/// local-only call site would surface here too. Separate test so a CI +/// failure tells you which entry point is broken at a glance. +#[test] +fn info_with_boxlite_profile_env_routes_to_rest() { + let stub = Stub::start(|_m, path| { + if path.starts_with("/v1/boxes") { + (200, r#"{"boxes":[]}"#.to_string()) + } else { + ( + 404, + r#"{"error":{"message":"nope","type":"NotFoundError"}}"#.to_string(), + ) + } + }); + let home = TempDir::new().unwrap(); + write_p1_creds(&home, &stub.url()); + + cli(&home) + .env("BOXLITE_PROFILE", "p1") + .args(["info"]) + .assert() + .success() + .stdout(predicate::str::contains(stub.url())); +} + +/// With no `--profile`, no `BOXLITE_PROFILE`, and no `default` profile in +/// the (otherwise empty) credentials file, `info` keeps its local +/// behavior — homeDir is the BOXLITE_HOME we set, virtualization runs +/// the local system check. Guards against a fix that over-corrects and +/// breaks the unauthenticated default path. +#[test] +fn info_without_profile_stays_local() { + let home = TempDir::new().unwrap(); + std::fs::write(home.path().join("credentials.toml"), "").unwrap(); + + cli(&home) + .args(["info"]) + .assert() + .success() + .stdout(predicate::str::contains(format!( + "homeDir: {}", + home.path().display() + ))); +} + +/// POL-30 (logs side): `logs` reads the box's console.log file from the +/// host's BOXLITE_HOME — there is no REST endpoint for log streaming +/// yet. The pre-fix code silently dropped the profile and looked up the +/// box in the *local* runtime, which is either wrong-data or a confusing +/// "No such box" depending on host state. Post-fix we error cleanly so a +/// scripted caller can tell the difference. +#[test] +fn logs_with_profile_errors_clearly_over_rest() { + let stub = Stub::start(|_m, _path| (200, r#"{"boxes":[]}"#.to_string())); + let home = TempDir::new().unwrap(); + write_p1_creds(&home, &stub.url()); + + cli(&home) + .args(["--profile", "p1", "logs", "anybox"]) + .assert() + .failure() + .stderr(predicate::str::contains("not yet supported over REST")); +}