From bb513c9273c0ab51fb9382652982d0d2fc369540 Mon Sep 17 00:00:00 2001 From: Alex Araujo Date: Fri, 30 Oct 2026 08:40:34 -0500 Subject: [PATCH 1/2] Build the packdb client binary The packdb-cli package now builds `packdb`, the command-line client. Its main parses the library's `Command` and exits with the status `run` returns; the library stays the shared API. `packdb` is a client only. Bare `packdb` prints help to stderr and exits 2, and no subcommand starts a server. The checks gate now fails if the package's dependency closure contains a PackDB package other than packdb-cli, packdb-client and packdb-otel. The process tests move from packdb-cli-test-harness to the real binary, unchanged apart from the executable they run, and gain cases for --version, help for every command, and bare `packdb` exiting promptly without creating files. The harness is removed. --- CHANGELOG.md | 8 +- Cargo.lock | 11 -- Cargo.toml | 2 +- README.md | 14 +- RELEASING.md | 9 +- crates/packdb-cli-test-harness/Cargo.toml | 30 ---- crates/packdb-cli/COMPATIBILITY.md | 15 +- crates/packdb-cli/Cargo.toml | 12 +- crates/packdb-cli/README.md | 13 +- crates/packdb-cli/src/lib.rs | 4 +- .../src/main.rs | 18 +- .../tests/process.rs | 154 ++++++++++++++---- scripts/oss-ci.sh | 34 ++-- 13 files changed, 203 insertions(+), 121 deletions(-) delete mode 100644 crates/packdb-cli-test-harness/Cargo.toml rename crates/{packdb-cli-test-harness => packdb-cli}/src/main.rs (70%) rename crates/{packdb-cli-test-harness => packdb-cli}/tests/process.rs (73%) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4945c9..3cd84c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,12 @@ Breaking. `packdb-query` is retired in favour of `packdb`, whose command library this repository now provides; its 0.2.x archives are unchanged. See `crates/packdb-cli/COMPATIBILITY.md` for the contract and the migration table. -- The package is `packdb-cli`, a library with no executable. `packdb query sql` - replaces `packdb-query`; the `packdb` binary is assembled with the server. +- The package is `packdb-cli`. It builds the `packdb` client binary, and its + library (`Command`, `run`, `Io`) stays the shared API. `packdb query sql` + replaces `packdb-query`. `packdb` never starts a server and depends on no + server package; the single-node server is `packdb-server`, built from the + server repository. The test-only `packdb-cli-test-harness` is removed: its + process tests now run the real binary. - Credentials never come from argument values: `--api-key-file`, `--token-file` and `--basic-auth-file`, or `PACKDB_API_KEY`, `PACKDB_TOKEN`, `PACKDB_BASIC_AUTH` and their `_FILE` forms. Two credentials are refused instead of the first diff --git a/Cargo.lock b/Cargo.lock index 3ed68b2..36fc3cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1654,17 +1654,6 @@ dependencies = [ "zstd", ] -[[package]] -name = "packdb-cli-test-harness" -version = "0.2.4" -dependencies = [ - "clap", - "packdb-cli", - "serde_json", - "tokio", - "wiremock", -] - [[package]] name = "packdb-client" version = "0.2.4" diff --git a/Cargo.toml b/Cargo.toml index 9b2421d..c4dd70f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ [workspace] resolver = "3" -members = ["crates/packdb-cli", "crates/packdb-cli-test-harness"] +members = ["crates/packdb-cli"] [workspace.package] edition = "2024" diff --git a/README.md b/README.md index 949b92c..293d7c5 100644 --- a/README.md +++ b/README.md @@ -1,18 +1,18 @@ # PackDB command library -`packdb-cli` is the command library behind `packdb`, PackDB's command-line -client. It implements `packdb query sql|promql|logql|traceql`, `packdb trace`, +`packdb-cli` builds `packdb`, PackDB's command-line client, and the command +library behind it. It implements `packdb query sql|promql|logql|traceql`, `packdb trace`, `packdb doctor` (limited diagnostics), `packdb smoke-test`, `packdb support-bundle` and `packdb usage-report`. | Package | Scope | | --- | --- | -| `packdb-cli` | The command library: arguments, credentials, output, completeness and exit codes | -| `packdb-cli-test-harness` | Test-only executable that runs the library as a process; never released | +| `packdb-cli` | The `packdb` binary and its command library: arguments, credentials, output, completeness and exit codes | -This repository ships no executable. The `packdb` binary that embeds the library -is assembled with the server. `packdb-query`, the binary of releases 0.2.0 to -0.2.4, is retired; its archives remain on +`packdb` is a client only. It never starts a server and depends on no server +package; the single-node server is `packdb-server`, built from the server +repository. `packdb-query`, the binary of releases 0.2.0 to 0.2.4, is retired; +its archives remain on [GitHub Releases](https://github.com/packdb-oss/packdb-cli/releases) unchanged. This workspace provides client-side commands. It does not implement the server diff --git a/RELEASING.md b/RELEASING.md index 0664e13..38b3ba3 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -10,9 +10,10 @@ GitHub releases and crates.io publication are separate decisions; crates.io is p cannot resolve from the registry, until `packdb-otel` and then `packdb-client` are on crates.io. Do not attempt them in the other order. -This repository releases source only. The `packdb` binary that embeds the library -is built and released with the server; `packdb-query` binary archives of 0.2.0 to -0.2.4 stay on their GitHub Releases unchanged. +This repository releases source. The `packdb` client binary builds from it; how +`packdb` and `packdb-server` binaries are distributed is decided separately. +`packdb-query` binary archives of 0.2.0 to 0.2.4 stay on their GitHub Releases +unchanged. ## Private candidates @@ -21,7 +22,7 @@ is built and released with the server; `packdb-query` binary archives of 0.2.0 t 3. Inspect `cargo package --list`. The package must include its README, LICENSE and COMPATIBILITY.md, and exclude private data and internal-only source. 4. Dispatch `oss-candidate`. It reruns every gate on the exact commit, including - the test harness's process-level contract tests, and records the source archive, + the `packdb` binary's process-level contract tests, and records the source archive, package and source inventories, dependency inventory, checksums and the SDK revision it was built against. 5. Review the source archive, dependency inventory, checksums and release record. A diff --git a/crates/packdb-cli-test-harness/Cargo.toml b/crates/packdb-cli-test-harness/Cargo.toml deleted file mode 100644 index 0d1bf4f..0000000 --- a/crates/packdb-cli-test-harness/Cargo.toml +++ /dev/null @@ -1,30 +0,0 @@ -[package] -name = "packdb-cli-test-harness" -version = "0.2.4" -edition = "2024" -rust-version = "1.94.1" -description = "Test-only executable that runs the packdb-cli command library as a process. Never released." -license = "Apache-2.0" -repository = "https://github.com/packdb-oss/packdb-cli" -publish = false - -[[bin]] -name = "packdb-cli-test-harness" -path = "src/main.rs" - -[dependencies] -packdb-cli = { version = "0.2.4", path = "../packdb-cli" } -clap = { version = "4.5", features = ["derive"] } -tokio = { version = "1.49", features = ["rt"] } - -[dev-dependencies] -serde_json = "1" -tokio = { version = "1.49", features = ["macros", "rt", "rt-multi-thread"] } -wiremock = "0.6" - -[features] -default = [] -flight-sql = ["packdb-cli/flight-sql"] - -[lints.rust] -missing_docs = "deny" diff --git a/crates/packdb-cli/COMPATIBILITY.md b/crates/packdb-cli/COMPATIBILITY.md index f26f82f..fe14628 100644 --- a/crates/packdb-cli/COMPATIBILITY.md +++ b/crates/packdb-cli/COMPATIBILITY.md @@ -1,9 +1,9 @@ # `packdb` command contract What the `packdb` client commands promise their callers. This library implements -them; the `packdb` binary that embeds it is assembled with the server. Until that -binary exists, the contract is tested through `packdb-cli-test-harness`, a -test-only executable that is never released. +them, and this crate's `packdb` binary runs them. `packdb` is a client only: the +single-node server is `packdb-server`, built from the server repository, and each +reports its own version. Read as: anything in **The contract** changes only with a deliberate, documented decision. Anything under **Not promised** may change without one. @@ -463,10 +463,11 @@ refuses an answer whose declarations differ from these: unavailable value reported as missing evidence and never as zero, a server without the route, answers with a wrong schema, scope, period, omitted or negative value, `401`, `403` and an unreachable server. -- `crates/packdb-cli-test-harness/tests/process.rs`: the same contract as a process - sees it, covering help, the refused secret argument, stdout/stderr separation, - exit codes, credential files, conflicts, a closed stdout (exit `8`), the PromQL, - trace and time-argument cases, `doctor` and `usage-report`. +- `crates/packdb-cli/tests/process.rs`: the same contract as the `packdb` binary + shows it, covering help for every command, `--version`, bare `packdb` exiting `2` + promptly without creating files, the refused secret argument, stdout/stderr + separation, exit codes, credential files, conflicts, a closed stdout (exit `8`), + the PromQL, trace and time-argument cases, `doctor` and `usage-report`. - `packdb-client`'s own tests cover the transport fallback rules, the query API requests and error classification. diff --git a/crates/packdb-cli/Cargo.toml b/crates/packdb-cli/Cargo.toml index 7094894..78cdeca 100644 --- a/crates/packdb-cli/Cargo.toml +++ b/crates/packdb-cli/Cargo.toml @@ -3,7 +3,7 @@ name = "packdb-cli" version = "0.2.4" edition = "2024" rust-version = "1.94.1" -description = "The command library behind the PackDB `packdb` command: queries and operator commands against a running server." +description = "PackDB's `packdb` command-line client and the command library behind it: queries and operator commands against a running server." license = "Apache-2.0" readme = "README.md" keywords = ["packdb", "cli", "sql", "query"] @@ -12,6 +12,12 @@ repository = "https://github.com/packdb-oss/packdb-cli" # a separate reviewed allowlist change and release authorization. publish = false +# The `packdb` client. The library stays the shared API: `Command`, `run` and +# `Io` are what the binary, and any other front end, call. +[[bin]] +name = "packdb" +path = "src/main.rs" + [dependencies] # Enable the `sql` feature so the SQL client (including Arrow result # formatting) is compiled in. Flight SQL transport is opt-in via the @@ -33,14 +39,14 @@ serde_json = { version = "1", features = ["preserve_order"] } # The support bundle's archive container and its temporary file. tar = { version = "0.4", default-features = false } tempfile = "3" -tokio = { version = "1.49", features = ["macros", "time"] } +tokio = { version = "1.49", features = ["macros", "rt", "time"] } # The `smoke-test` run marker and trace and span identifiers. uuid = { version = "1", features = ["v4"] } # Compresses the support bundle. zstd = { version = "0.14", default-features = false } [dev-dependencies] -tokio = { version = "1.49", features = ["macros", "rt"] } +tokio = { version = "1.49", features = ["macros", "rt", "rt-multi-thread"] } wiremock = "0.6" [features] diff --git a/crates/packdb-cli/README.md b/crates/packdb-cli/README.md index ed21cf5..0bf0021 100644 --- a/crates/packdb-cli/README.md +++ b/crates/packdb-cli/README.md @@ -1,12 +1,13 @@ # `packdb-cli` -The command library behind `packdb`: queries, trace lookup, limited `doctor` -diagnostics, `smoke-test`, `support-bundle` and `usage-report` against a running -PackDB server. +`packdb`, PackDB's command-line client, and the command library behind it: +queries, trace lookup, limited `doctor` diagnostics, `smoke-test`, +`support-bundle` and `usage-report` against a running PackDB server. -This crate ships no executable. The `packdb` binary that embeds it is assembled -with the server. `packdb-query`, the binary of releases 0.2.0 to 0.2.4, is -retired; its released archives are unchanged. +The crate builds the `packdb` binary and exports the library it runs (`Command`, +`run`, `Io`). `packdb` never starts a server; the single-node server is +`packdb-server`, built from the server repository. `packdb-query`, the binary of +releases 0.2.0 to 0.2.4, is retired; its released archives are unchanged. The supported surface — arguments, environment variables, output, completeness and exit codes — is written down in [`COMPATIBILITY.md`](./COMPATIBILITY.md), diff --git a/crates/packdb-cli/src/lib.rs b/crates/packdb-cli/src/lib.rs index fdc5275..aa1747b 100644 --- a/crates/packdb-cli/src/lib.rs +++ b/crates/packdb-cli/src/lib.rs @@ -15,8 +15,8 @@ //! The command library behind `packdb`. //! //! [`Command`] is a clap subcommand set; [`run`] executes one against a running -//! PackDB server and returns its [`Exit`] status. The library ships no -//! executable: the `packdb` binary that embeds it is assembled with the server. +//! PackDB server and returns its [`Exit`] status. This crate's `packdb` binary +//! runs it; the library is the shared API for any other front end. //! `COMPATIBILITY.md` is the supported contract: arguments, environment, //! output and exit codes. diff --git a/crates/packdb-cli-test-harness/src/main.rs b/crates/packdb-cli/src/main.rs similarity index 70% rename from crates/packdb-cli-test-harness/src/main.rs rename to crates/packdb-cli/src/main.rs index cfd2bdf..66e54ab 100644 --- a/crates/packdb-cli-test-harness/src/main.rs +++ b/crates/packdb-cli/src/main.rs @@ -12,9 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Runs the `packdb-cli` command library as a process, so tests can assert -//! real stdout, stderr and exit codes. Test-only: it is never released, and -//! it is not the `packdb` binary, which is assembled with the server. +//! `packdb`, PackDB's command-line client. It parses a [`packdb_cli::Command`] +//! and exits with the status [`packdb_cli::run`] returns; the commands +//! themselves live in the library. It never starts a server: the single-node +//! server is `packdb-server`, built from the server repository. + +#![deny(clippy::unwrap_used)] +#![deny(clippy::expect_used)] +#![deny(clippy::indexing_slicing)] +#![deny(clippy::panic)] +#![deny(clippy::unreachable)] use std::process::ExitCode; @@ -24,7 +31,7 @@ use clap::Parser; #[command( name = "packdb", version, - about = "PackDB client commands (test harness)", + about = "PackDB command-line client: queries and operator commands against a running PackDB server", arg_required_else_help = true, subcommand_required = true )] @@ -35,7 +42,8 @@ struct Cli { fn main() -> ExitCode { // clap prints help and exits 2 when no subcommand is given, exits 2 on any - // other usage error, and exits 0 for --help and --version. + // other usage error, and exits 0 for --help and --version. Parsing comes + // first, so none of those paths builds a runtime or opens a connection. let cli = Cli::parse(); let runtime = match tokio::runtime::Builder::new_current_thread() .enable_all() diff --git a/crates/packdb-cli-test-harness/tests/process.rs b/crates/packdb-cli/tests/process.rs similarity index 73% rename from crates/packdb-cli-test-harness/tests/process.rs rename to crates/packdb-cli/tests/process.rs index 45e7b89..a9f2d54 100644 --- a/crates/packdb-cli-test-harness/tests/process.rs +++ b/crates/packdb-cli/tests/process.rs @@ -24,7 +24,7 @@ use serde_json::{Value, json}; use wiremock::matchers::{header, method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; -const HARNESS: &str = env!("CARGO_BIN_EXE_packdb-cli-test-harness"); +const PACKDB: &str = env!("CARGO_BIN_EXE_packdb"); /// Every variable the commands read, removed so the runner's own environment /// cannot leak into a case. @@ -42,22 +42,18 @@ const VARIABLES: [&str; 11] = [ "PACKDB_URL", ]; -async fn harness(args: &[&str], env: &[(&str, &str)]) -> Output { +async fn packdb(args: &[&str], env: &[(&str, &str)]) -> Output { let args: Vec = args.iter().map(|arg| (*arg).to_string()).collect(); let env: Vec<(String, String)> = env .iter() .map(|(k, v)| ((*k).to_string(), (*v).to_string())) .collect(); tokio::task::spawn_blocking(move || { - let mut command = Command::new(HARNESS); + let mut command = Command::new(PACKDB); for variable in VARIABLES { command.env_remove(variable); } - command - .args(&args) - .envs(env) - .output() - .expect("run the harness") + command.args(&args).envs(env).output().expect("run packdb") }) .await .expect("join") @@ -102,7 +98,7 @@ async fn sql_server(response: ResponseTemplate) -> MockServer { #[tokio::test(flavor = "multi_thread")] async fn no_subcommand_prints_help_on_stderr_and_exits_2() { - let output = harness(&[], &[]).await; + let output = packdb(&[], &[]).await; assert_eq!(output.status.code(), Some(2)); assert!(output.stdout.is_empty()); assert!( @@ -114,14 +110,106 @@ async fn no_subcommand_prints_help_on_stderr_and_exits_2() { #[tokio::test(flavor = "multi_thread")] async fn help_exits_0() { - let output = harness(&["--help"], &[]).await; + let output = packdb(&["--help"], &[]).await; assert_eq!(output.status.code(), Some(0)); assert!(text(&output.stdout).contains("query")); } +#[tokio::test(flavor = "multi_thread")] +async fn version_prints_the_package_version_and_exits_0() { + let output = packdb(&["--version"], &[]).await; + assert_eq!(output.status.code(), Some(0)); + assert_eq!( + text(&output.stdout), + format!("packdb {}\n", env!("CARGO_PKG_VERSION")) + ); + assert!(output.stderr.is_empty(), "{}", text(&output.stderr)); +} + +#[tokio::test(flavor = "multi_thread")] +async fn every_command_has_help_that_exits_0() { + for command in [ + &["query", "sql"][..], + &["query", "promql"], + &["query", "logql"], + &["query", "traceql"], + &["trace"], + &["doctor"], + &["smoke-test"], + &["support-bundle"], + &["usage-report"], + ] { + let mut args = command.to_vec(); + args.push("--help"); + let output = packdb(&args, &[]).await; + let usage = format!("Usage: packdb {}", command.join(" ")); + assert_eq!(output.status.code(), Some(0), "{command:?}"); + assert!( + text(&output.stdout).contains(&usage), + "{command:?}: {}", + text(&output.stdout) + ); + } +} + +/// Bare `packdb` is a client asking for a command, never a server starting up: +/// it exits promptly with `2`, and leaves nothing behind in its working +/// directory or home. +#[tokio::test(flavor = "multi_thread")] +async fn bare_packdb_exits_2_promptly_and_creates_no_files() { + let cwd = tempfile::tempdir().expect("working directory"); + let home = tempfile::tempdir().expect("home"); + let (cwd_path, home_path) = (cwd.path().to_path_buf(), home.path().to_path_buf()); + let output = tokio::task::spawn_blocking(move || { + let mut command = Command::new(PACKDB); + for variable in VARIABLES { + command.env_remove(variable); + } + let mut child = command + .current_dir(&cwd_path) + .env("HOME", &home_path) + .env("XDG_CONFIG_HOME", home_path.join(".config")) + .env("XDG_DATA_HOME", home_path.join(".local/share")) + .env("XDG_STATE_HOME", home_path.join(".local/state")) + .env("XDG_CACHE_HOME", home_path.join(".cache")) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn packdb"); + // A process still running after this long is serving, not printing + // help. Stop it so the failure is reported rather than hanging the run. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + while child.try_wait().expect("poll packdb").is_none() { + if std::time::Instant::now() >= deadline { + child.kill().expect("stop packdb"); + panic!("bare packdb was still running after 30 s"); + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + child.wait_with_output().expect("collect packdb's output") + }) + .await + .expect("join"); + assert_eq!(output.status.code(), Some(2)); + assert!(output.stdout.is_empty(), "{}", text(&output.stdout)); + assert!( + text(&output.stderr).contains("Usage: packdb"), + "{}", + text(&output.stderr) + ); + for dir in [cwd.path(), home.path()] { + let entries: Vec<_> = std::fs::read_dir(dir) + .expect("list directory") + .map(|entry| entry.expect("entry").file_name()) + .collect(); + assert!(entries.is_empty(), "{}: {entries:?}", dir.display()); + } +} + #[tokio::test(flavor = "multi_thread")] async fn a_secret_cannot_be_passed_as_an_argument_value() { - let output = harness(&["query", "sql", "SELECT 1", "--api-key", "secret"], &[]).await; + let output = packdb(&["query", "sql", "SELECT 1", "--api-key", "secret"], &[]).await; assert_eq!(output.status.code(), Some(2)); assert!(!text(&output.stdout).contains("secret")); } @@ -134,7 +222,7 @@ async fn json_on_stdout_warnings_on_stderr_and_exit_3_for_a_partial_result() { .insert_header("x-packdb-partial-causes", "cold_tier_unavailable"), ) .await; - let output = harness( + let output = packdb( &[ "query", "sql", @@ -167,7 +255,7 @@ async fn each_failure_has_its_exit_code() { ]; for (status, body, code) in cases { let server = sql_server(ResponseTemplate::new(status).set_body_string(body)).await; - let output = harness( + let output = packdb( &["query", "sql", "SELECT 1", "--transport", "http"], &[("PACKDB_ENDPOINT", &server.uri())], ) @@ -182,7 +270,7 @@ async fn each_failure_has_its_exit_code() { async fn an_unreachable_server_exits_6() { let refusing = refusing_endpoint(); let endpoint = refusing.endpoint.clone(); - let output = harness( + let output = packdb( &[ "query", "sql", @@ -210,7 +298,7 @@ async fn a_credential_file_is_read_and_its_secret_is_never_printed() { .await; let file = std::env::temp_dir().join(format!("packdb-cli-token-{}", std::process::id())); std::fs::write(&file, "secret-token\n").expect("write token"); - let output = harness( + let output = packdb( &[ "query", "sql", @@ -239,7 +327,7 @@ async fn conflicting_credentials_exit_2_without_contacting_the_server() { .expect(0) .mount(&server) .await; - let output = harness( + let output = packdb( &["query", "sql", "SELECT 1"], &[ ("PACKDB_ENDPOINT", &server.uri()), @@ -257,8 +345,8 @@ async fn conflicting_credentials_exit_2_without_contacting_the_server() { /// A reader that goes away before the result is written: the write fails with /// a broken pipe, and the command reports it as exit 8 instead of succeeding. /// -/// The server holds its reply until the test has closed the harness's stdout, -/// so the pipe is closed before the harness can write, whatever the scheduling. +/// The server holds its reply until the test has closed packdb's stdout, +/// so the pipe is closed before packdb can write, whatever the scheduling. #[tokio::test(flavor = "multi_thread")] async fn a_closed_stdout_exits_8() { use std::io::{Read, Write}; @@ -282,7 +370,7 @@ async fn a_closed_stdout_exits_8() { .expect("answer the query"); }); let status = tokio::task::spawn_blocking(move || { - let mut command = Command::new(HARNESS); + let mut command = Command::new(PACKDB); for variable in VARIABLES { command.env_remove(variable); } @@ -299,10 +387,10 @@ async fn a_closed_stdout_exits_8() { .stdout(Stdio::piped()) .stderr(Stdio::null()) .spawn() - .expect("spawn the harness"); + .expect("spawn packdb"); drop(child.stdout.take()); closed.send(()).expect("release the reply"); - child.wait().expect("wait for the harness") + child.wait().expect("wait for packdb") }) .await .expect("join"); @@ -312,9 +400,9 @@ async fn a_closed_stdout_exits_8() { #[tokio::test(flavor = "multi_thread")] async fn help_lists_every_query_language_and_trace() { - let top = harness(&["--help"], &[]).await; + let top = packdb(&["--help"], &[]).await; assert!(text(&top.stdout).contains("trace"), "{}", text(&top.stdout)); - let query = harness(&["query", "--help"], &[]).await; + let query = packdb(&["query", "--help"], &[]).await; assert_eq!(query.status.code(), Some(0)); let help = text(&query.stdout); for language in ["sql", "promql", "logql", "traceql"] { @@ -333,7 +421,7 @@ async fn a_promql_answer_without_a_carrier_exits_3_as_unknown() { )) .mount(&server) .await; - let output = harness( + let output = packdb( &["query", "promql", "1", "--format", "json"], &[ ("PACKDB_ENDPOINT", &server.uri()), @@ -368,13 +456,13 @@ async fn a_found_trace_exits_0_and_a_missing_one_exits_1() { ) .mount(&server) .await; - let found = harness(&["trace", "ab12"], &[("PACKDB_ENDPOINT", &server.uri())]).await; + let found = packdb(&["trace", "ab12"], &[("PACKDB_ENDPOINT", &server.uri())]).await; assert_eq!(found.status.code(), Some(0), "{}", text(&found.stderr)); let envelope: Value = serde_json::from_slice(&found.stdout).expect("one JSON object"); assert_eq!(envelope["completeness"], "complete"); assert!(found.stderr.is_empty(), "{}", text(&found.stderr)); - let missing = harness(&["trace", "ff"], &[("PACKDB_ENDPOINT", &server.uri())]).await; + let missing = packdb(&["trace", "ff"], &[("PACKDB_ENDPOINT", &server.uri())]).await; assert_eq!(missing.status.code(), Some(1), "{}", text(&missing.stderr)); assert!( text(&missing.stderr).contains("trace not found: ff"), @@ -403,7 +491,7 @@ async fn conflicting_or_incomplete_time_arguments_exit_2() { &["trace", "ab12", "--start", "now-1h"][..], &["query", "traceql", "{}", "--start", "tomorrow"][..], ] { - let output = harness(args, &[("PACKDB_ENDPOINT", &refusing.endpoint)]).await; + let output = packdb(args, &[("PACKDB_ENDPOINT", &refusing.endpoint)]).await; assert_eq!(output.status.code(), Some(2), "{args:?}"); assert!(output.stdout.is_empty(), "{args:?}"); } @@ -411,17 +499,17 @@ async fn conflicting_or_incomplete_time_arguments_exit_2() { #[tokio::test(flavor = "multi_thread")] async fn doctor_is_listed_refuses_csv_and_exits_6_when_unreachable() { - let help = harness(&["--help"], &[]).await; + let help = packdb(&["--help"], &[]).await; assert!( text(&help.stdout).contains("doctor"), "{}", text(&help.stdout) ); - let csv = harness(&["doctor", "--format", "csv"], &[]).await; + let csv = packdb(&["doctor", "--format", "csv"], &[]).await; assert_eq!(csv.status.code(), Some(2)); assert!(csv.stdout.is_empty()); let refusing = refusing_endpoint(); - let output = harness(&["doctor"], &[("PACKDB_ENDPOINT", &refusing.endpoint)]).await; + let output = packdb(&["doctor"], &[("PACKDB_ENDPOINT", &refusing.endpoint)]).await; assert_eq!(output.status.code(), Some(6), "{}", text(&output.stderr)); assert!( text(&output.stdout).starts_with("packdb doctor (limited diagnostics): "), @@ -437,17 +525,17 @@ async fn doctor_is_listed_refuses_csv_and_exits_6_when_unreachable() { #[tokio::test] async fn usage_report_is_listed_refuses_csv_and_exits_6_when_unreachable() { - let help = harness(&["--help"], &[]).await; + let help = packdb(&["--help"], &[]).await; assert!( text(&help.stdout).contains("usage-report"), "{}", text(&help.stdout) ); - let csv = harness(&["usage-report", "--format", "csv"], &[]).await; + let csv = packdb(&["usage-report", "--format", "csv"], &[]).await; assert_eq!(csv.status.code(), Some(2)); assert!(csv.stdout.is_empty()); let refusing = refusing_endpoint(); - let output = harness( + let output = packdb( &["usage-report"], &[("PACKDB_ENDPOINT", &refusing.endpoint)], ) diff --git a/scripts/oss-ci.sh b/scripts/oss-ci.sh index f5d2cd5..fa9cf36 100755 --- a/scripts/oss-ci.sh +++ b/scripts/oss-ci.sh @@ -106,6 +106,21 @@ EOF log "sibling SDK checkout: $(cd "$sdk" && git rev-parse HEAD 2>/dev/null || echo 'not a git checkout')" } +# `packdb` is a client: it must never be able to start a server, so it may not +# depend on one. Every PackDB package in its normal and build dependency closure, +# with every feature enabled, must be this package or the SDK it builds on. A +# server package -- `packdb` or any other `packdb-*` crate -- fails the gate. +readonly CLIENT_PACKDB_PACKAGES="packdb-cli packdb-client packdb-otel" +check_client_dependencies() { + log "client dependency closure" + local closure unexpected + closure="$(cargo tree --locked -p packdb-cli --all-features -e normal,build \ + --prefix none --format '{p}')" || die "cargo tree failed for packdb-cli" + unexpected="$(awk '{print $1}' <<<"$closure" | sort -u | + grep -E '^packdb($|-)' | grep -vxF -f <(tr ' ' '\n' <<<"$CLIENT_PACKDB_PACKAGES") || true)" + [[ -z "$unexpected" ]] || die "packdb-cli depends on a server package: $unexpected" +} + check_policy() { log "policy files and contacts" local f @@ -164,9 +179,9 @@ PY for package in "${PACKAGES[@]}"; do listed="$(cargo package --list --locked --allow-dirty -p "$package" 2>/dev/null)" || die "cargo package --list failed for $package" - # A library crate -- and the contract must travel inside the archive, + # The library and the binary -- and the contract must travel inside the archive, # not merely exist in the repo. - for required in Cargo.toml README.md LICENSE COMPATIBILITY.md src/lib.rs; do + for required in Cargo.toml README.md LICENSE COMPATIBILITY.md src/lib.rs src/main.rs; do grep -qx "$required" <<<"$listed" || die "$package package is missing $required" done if grep -E '(^|/)(target|dist)/|\.profraw$' <<<"$listed"; then @@ -175,10 +190,10 @@ PY done } -# The workspace has one library, `packdb-cli`, and one test-only executable, -# `packdb-cli-test-harness`, whose tests run the library as a process to check -# stdout, stderr and exit codes. `--all-targets` runs both; `--doc` covers the -# library's documentation examples. +# The workspace has one package, `packdb-cli`: the command library and the +# `packdb` binary, whose process tests check stdout, stderr and exit codes. +# `--all-targets` runs both; `--doc` covers the library's documentation +# examples. feature_matrix() { local toolchain="$1" cargo "+$toolchain" test --locked --workspace --all-targets @@ -197,6 +212,7 @@ cmd_checks() { python3 scripts/check_action_pins.py --self-test python3 scripts/check_source_headers.py require_sdk_sibling + check_client_dependencies python3 scripts/export_clean.py python3 scripts/coverage_gate.py --self-test cargo fmt --all -- --check @@ -229,10 +245,8 @@ cmd_compatibility() { check_gate_input compatibility scope >/dev/null require_sdk_sibling - # The command contract is checked through the test harness: its tests run - # the library as a process and assert stdout, stderr and exit codes. This - # repository ships no executable; the `packdb` binary is assembled with the - # server. + # The command contract is checked through the `packdb` binary: its process + # tests assert stdout, stderr and exit codes. feature_matrix "$MSRV" } From 2e346c3957efea81f0599f5ad01d1b73e2203e22 Mon Sep 17 00:00:00 2001 From: Alex Araujo Date: Fri, 30 Oct 2026 09:06:08 -0500 Subject: [PATCH 2/2] Deliver help, version and usage errors with exit 8 on failure `Cli::parse()` let clap print help, the version and usage errors and exit by itself, so a closed or failing stdout or stderr still exited 0 or 2. `packdb` now parses with `try_parse`, writes what clap renders to the stream clap chose, flushes it, and exits 8 when that fails, as the contract promises for every output. The runtime-start error is written the same way. The process tests run help, `--version`, two usage errors and bare `packdb` against a stream whose pipe has no reader. Spawns and pipe closes in the suite now share a lock, so no concurrently forked child can keep such a pipe's reading end open. --- crates/packdb-cli/COMPATIBILITY.md | 1 + crates/packdb-cli/src/main.rs | 85 ++++++++++++++++++++---- crates/packdb-cli/tests/process.rs | 102 ++++++++++++++++++++++++++++- 3 files changed, 173 insertions(+), 15 deletions(-) diff --git a/crates/packdb-cli/COMPATIBILITY.md b/crates/packdb-cli/COMPATIBILITY.md index fe14628..36bc8d9 100644 --- a/crates/packdb-cli/COMPATIBILITY.md +++ b/crates/packdb-cli/COMPATIBILITY.md @@ -467,6 +467,7 @@ refuses an answer whose declarations differ from these: shows it, covering help for every command, `--version`, bare `packdb` exiting `2` promptly without creating files, the refused secret argument, stdout/stderr separation, exit codes, credential files, conflicts, a closed stdout (exit `8`), + help, `--version`, usage errors and bare `packdb` on an unwritable stream (exit `8`), the PromQL, trace and time-argument cases, `doctor` and `usage-report`. - `packdb-client`'s own tests cover the transport fallback rules, the query API requests and error classification. diff --git a/crates/packdb-cli/src/main.rs b/crates/packdb-cli/src/main.rs index 66e54ab..34afb95 100644 --- a/crates/packdb-cli/src/main.rs +++ b/crates/packdb-cli/src/main.rs @@ -23,9 +23,11 @@ #![deny(clippy::panic)] #![deny(clippy::unreachable)] +use std::io::{IsTerminal, Write}; use std::process::ExitCode; use clap::Parser; +use packdb_cli::Exit; #[derive(Parser)] #[command( @@ -41,29 +43,84 @@ struct Cli { } fn main() -> ExitCode { - // clap prints help and exits 2 when no subcommand is given, exits 2 on any - // other usage error, and exits 0 for --help and --version. Parsing comes - // first, so none of those paths builds a runtime or opens a connection. - let cli = Cli::parse(); + let mut stdout = std::io::stdout().lock(); + let mut stderr = std::io::stderr().lock(); + // Parsing comes first, so help, version and usage errors never build a + // runtime or open a connection. clap renders them; this process writes + // them, so a failed write is reported like any other output failure. + let exit = match Cli::try_parse() { + Ok(cli) => execute(cli.command, &mut stdout, &mut stderr), + Err(error) => deliver_parse_outcome(&error, &mut stdout, &mut stderr), + }; + ExitCode::from(exit.code()) +} + +fn execute(command: packdb_cli::Command, stdout: &mut dyn Write, stderr: &mut dyn Write) -> Exit { let runtime = match tokio::runtime::Builder::new_current_thread() .enable_all() .build() { Ok(runtime) => runtime, Err(error) => { - eprintln!("error: cannot start the runtime: {error}"); - return ExitCode::from(packdb_cli::Exit::Usage.code()); + return match writeln!(stderr, "error: cannot start the runtime: {error}") + .and_then(|()| stderr.flush()) + { + Ok(()) => Exit::Usage, + Err(_) => Exit::OutputFailed, + }; } }; - let mut stdout = std::io::stdout().lock(); - let mut stderr = std::io::stderr().lock(); - let exit = runtime.block_on(packdb_cli::run( - cli.command, + runtime.block_on(packdb_cli::run( + command, packdb_cli::Io { - stdout: &mut stdout, - stderr: &mut stderr, + stdout, + stderr, env: &packdb_cli::ProcessEnvironment, }, - )); - ExitCode::from(exit.code()) + )) +} + +/// Write what clap rendered: `--help` and `--version` on stdout with `0`; +/// bare `packdb`'s help and every usage error on stderr with `2`. Output that +/// cannot be written or flushed exits `8`, as the contract promises. +fn deliver_parse_outcome( + error: &clap::Error, + stdout: &mut dyn Write, + stderr: &mut dyn Write, +) -> Exit { + let exit = if error.exit_code() == 0 { + Exit::Success + } else { + Exit::Usage + }; + let on_stderr = error.use_stderr(); + let styled = if on_stderr { + std::io::stderr().is_terminal() + } else { + std::io::stdout().is_terminal() + }; + let rendered = error.render(); + let text = if styled { + rendered.ansi().to_string() + } else { + rendered.to_string() + }; + let stream: &mut dyn Write = if on_stderr { stderr } else { stdout }; + match stream + .write_all(text.as_bytes()) + .and_then(|()| stream.flush()) + { + Ok(()) => exit, + Err(failure) => { + if !on_stderr { + // Best effort: if this report cannot be written either, the + // exit code still says the output failed. + drop( + writeln!(stderr, "error: cannot write to stdout: {failure}") + .and_then(|()| stderr.flush()), + ); + } + Exit::OutputFailed + } + } } diff --git a/crates/packdb-cli/tests/process.rs b/crates/packdb-cli/tests/process.rs index a9f2d54..9446ab3 100644 --- a/crates/packdb-cli/tests/process.rs +++ b/crates/packdb-cli/tests/process.rs @@ -42,6 +42,30 @@ const VARIABLES: [&str; 11] = [ "PACKDB_URL", ]; +/// Held while a child is spawned and while a pipe's reading end is closed. +/// +/// A spawned child holds a copy of every descriptor this process has open +/// until it execs. A closed-pipe case is only deterministic if no such copy +/// of its reading end survives the close, so no spawn may be in flight then. +/// `spawn` returns after the child has exec'd, so holding this across each +/// spawn and each close is enough. +static SPAWN: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +fn spawn_lock() -> std::sync::MutexGuard<'static, ()> { + SPAWN + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + +/// Spawn `command` with the lock held, then wait for its output without it. +fn output(command: &mut Command) -> Output { + let child = { + let _spawning = spawn_lock(); + command.spawn().expect("spawn packdb") + }; + child.wait_with_output().expect("wait for packdb") +} + async fn packdb(args: &[&str], env: &[(&str, &str)]) -> Output { let args: Vec = args.iter().map(|arg| (*arg).to_string()).collect(); let env: Vec<(String, String)> = env @@ -53,7 +77,14 @@ async fn packdb(args: &[&str], env: &[(&str, &str)]) -> Output { for variable in VARIABLES { command.env_remove(variable); } - command.args(&args).envs(env).output().expect("run packdb") + output( + command + .args(&args) + .envs(env) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()), + ) }) .await .expect("join") @@ -165,6 +196,7 @@ async fn bare_packdb_exits_2_promptly_and_creates_no_files() { for variable in VARIABLES { command.env_remove(variable); } + let spawning = spawn_lock(); let mut child = command .current_dir(&cwd_path) .env("HOME", &home_path) @@ -177,6 +209,7 @@ async fn bare_packdb_exits_2_promptly_and_creates_no_files() { .stderr(Stdio::piped()) .spawn() .expect("spawn packdb"); + drop(spawning); // A process still running after this long is serving, not printing // help. Stop it so the failure is reported rather than hanging the run. let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); @@ -342,6 +375,70 @@ async fn conflicting_credentials_exit_2_without_contacting_the_server() { assert!(!stderr.contains("secret-"), "{stderr}"); } +/// Which of `packdb`'s streams a case makes unwritable. +#[derive(Debug, Clone, Copy)] +enum Closed { + Stdout, + Stderr, +} + +/// Run `packdb` with one stream a pipe whose reading end is already closed, +/// so every write to it fails with a broken pipe. (A read-only file would not +/// do: the standard library treats `EBADF` on stdout as a closed stdout and +/// discards the write.) The other stream is captured. +async fn with_closed(args: &[&str], closed: Closed) -> Output { + let args: Vec = args.iter().map(|arg| (*arg).to_string()).collect(); + tokio::task::spawn_blocking(move || { + let writer = { + let _closing = spawn_lock(); + let (reader, writer) = std::io::pipe().expect("pipe"); + drop(reader); + writer + }; + let mut command = Command::new(PACKDB); + for variable in VARIABLES { + command.env_remove(variable); + } + command.args(&args).stdin(Stdio::null()); + match closed { + Closed::Stdout => command.stdout(writer).stderr(Stdio::piped()), + Closed::Stderr => command.stdout(Stdio::piped()).stderr(writer), + }; + output(&mut command) + }) + .await + .expect("join") +} + +/// What clap renders -- help, the version, a usage error, bare `packdb`'s +/// help -- is output like any other: when its stream cannot be written, +/// `packdb` exits 8 instead of 0 or 2. +#[tokio::test(flavor = "multi_thread")] +async fn help_version_and_usage_errors_on_a_closed_stream_exit_8() { + for (args, closed) in [ + (&["--help"][..], Closed::Stdout), + (&["query", "sql", "--help"], Closed::Stdout), + (&["--version"], Closed::Stdout), + (&["query", "sql"], Closed::Stderr), + (&["no-such-command"], Closed::Stderr), + (&[], Closed::Stderr), + ] { + let output = with_closed(args, closed).await; + assert_eq!( + output.status.code(), + Some(8), + "{args:?} with {closed:?} unwritable" + ); + if let Closed::Stdout = closed { + assert!( + text(&output.stderr).contains("cannot write to stdout"), + "{args:?}: {}", + text(&output.stderr) + ); + } + } +} + /// A reader that goes away before the result is written: the write fails with /// a broken pipe, and the command reports it as exit 8 instead of succeeding. /// @@ -374,6 +471,7 @@ async fn a_closed_stdout_exits_8() { for variable in VARIABLES { command.env_remove(variable); } + let spawning = spawn_lock(); let mut child = command .args([ "query", @@ -388,7 +486,9 @@ async fn a_closed_stdout_exits_8() { .stderr(Stdio::null()) .spawn() .expect("spawn packdb"); + // Still holding the lock: no other spawn can copy the reading end. drop(child.stdout.take()); + drop(spawning); closed.send(()).expect("release the reply"); child.wait().expect("wait for packdb") })