Merge branch 'ci/openapi-shape-tests' into main

Adds tests/openapi_conformance.rs: the committed openapi.json is now the
authority for every wire shape src/client.rs carries (key sets, optionality
proven behaviourally, round-trip, endpoint wiring, the error-code enum, and the
untyped /config/apply body). Also adds a lib target so integration tests can
import the real types, moves the two inline #[cfg(test)] modules out to tests/,
and records three real drift findings as documented `unmodelled` allowlist
entries (PluginView, PluginInstallView, HookView).

Author: MattJackson

.github/workflows/ci.yml

@@ -23,6 +23,11 @@ jobs:
         run: cargo clippy --all-targets -- -D warnings
       - name: build
         run: cargo build --locked --verbose
+      # Includes tests/openapi_conformance.rs, the SHAPE gate: it loads the committed openapi.json
+      # and checks every serde type in src/client.rs against its schema (field set, required vs
+      # nullable, round trip, endpoint wiring). The spec-drift job below only compares a version
+      # STRING, so this is the check that fails when a struct drifts from the spec in the same
+      # commit. Keep `cargo test` in this workflow: it is the only thing running that gate.
       - name: test
         run: cargo test --locked --verbose
 
@@ -54,6 +59,8 @@ jobs:
 
   spec-drift:
     # The admin client is hand-rolled against openapi.json (committed at the repo root).
+    # This job answers "is the committed spec CURRENT?" only. Whether src/client.rs still MATCHES
+    # that spec is a separate question, answered by tests/openapi_conformance.rs in the build job.
     # Fail when the latest busbar release ships a NEWER spec version than the one committed,
     # so drift is visible instead of silent. Degrades to a warning if the GitHub API is
     # unavailable or rate-limited (keeps the check non-flaky).

README.md

@@ -4,9 +4,15 @@ A human-facing CLI for the [busbar](https://github.com/GetBusbar) gateway's **ad
 (`/api/v1/admin`). It speaks the frozen v1 contract over HTTP/HTTPS with a thin, hand-rolled
 client (no OpenAPI generator), so it's small and easy to extend.
 
-The contract it targets is committed at [`openapi.json`](openapi.json) (busbar **1.5.2**);
+The contract it targets is committed at [`openapi.json`](openapi.json) (busbar **1.5.3**);
 CI compares that spec's version against the latest busbar release so drift is visible.
 
+Because the client is hand-rolled, a version match is not a shape match:
+[`tests/openapi_conformance.rs`](tests/openapi_conformance.rs) loads that same committed
+`openapi.json` and checks every request/response type in `src/client.rs` against its schema, field
+set, required/nullable-ness, and endpoint wiring. Renaming a Rust field, making a required field
+optional, or resyncing a spec that grew a property all fail `cargo test`.
+
 ## Install
 
 From source (published later on crates.io / as a GitHub release + Homebrew tap):

src/argmap.rs

@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: Apache-2.0
+
+//! Pure CLI-argument mappings shared by the binary and its tests.
+//!
+//! These carry contract meaning (the tri-state `allowed_pools`, the first-`=`-wins label split),
+//! so they live in the library rather than in `main.rs`: a test that imports the REAL function
+//! catches a regression, a test against a copy of it cannot.
+
+use anyhow::Result;
+
+/// Resolve the three distinct `allowed_pools` states the server understands: omitted (`None`) =
+/// ALL pools; an explicit empty list (`--no-pools`) = NO pools; a non-empty list = exactly those.
+/// A shared function so `cmd_keys_create` and its test exercise the SAME mapping. Collapsing
+/// `--no-pools` into `None` would mint an all-pools key when no-pools was asked for (fail-open on
+/// privilege).
+pub fn resolve_allowed_pools(no_pools: bool, pools: &[String]) -> Option<Vec<String>> {
+    if no_pools {
+        Some(Vec::new())
+    } else if pools.is_empty() {
+        None
+    } else {
+        Some(pools.to_vec())
+    }
+}
+
+/// Parse repeated `--label KEY=VALUE` arguments into the map the mint body carries. The split is
+/// on the FIRST `=` only, so a value that itself contains `=` (a URL query string, base64 padding)
+/// survives intact.
+pub fn parse_labels(pairs: &[String]) -> Result<std::collections::BTreeMap<String, String>> {
+    pairs
+        .iter()
+        .map(|p| {
+            p.split_once('=')
+                .map(|(k, v)| (k.to_string(), v.to_string()))
+                .ok_or_else(|| anyhow::anyhow!("--label must be KEY=VALUE, got {p:?}"))
+        })
+        .collect()
+}

src/client.rs

@@ -297,15 +297,19 @@ pub struct ConfigApplyView {
 
 // ── Contract-mirroring request/response types ────────────────────────────────────────────────
 
-#[derive(Deserialize)]
-struct ErrorEnvelope {
-    error: ErrorDetail,
+/// The gateway's error envelope (`Error` in the spec): `{"error":{"code","message"}}`. Public so
+/// the conformance tests can assert the decoded shape against the committed schema; the CLI itself
+/// only renders `code`/`message` into a message.
+#[derive(Debug, Deserialize, Serialize)]
+pub struct ErrorEnvelope {
+    pub error: ErrorDetail,
 }
 
-#[derive(Deserialize)]
-struct ErrorDetail {
-    code: String,
-    message: String,
+/// One error detail: a `code` drawn from the spec's closed vocabulary plus a human `message`.
+#[derive(Debug, Deserialize, Serialize)]
+pub struct ErrorDetail {
+    pub code: String,
+    pub message: String,
 }
 
 /// `GET /info` — mirrors `contract::InfoView`.
@@ -610,151 +614,3 @@ pub struct HookPage {
     #[serde(default)]
     pub next_cursor: Option<String>,
 }
-
-#[cfg(test)]
-mod spec_shape_tests {
-    // These lock the 1.5.2 wire shapes this crate was realigned to (commit that repaired the
-    // 1.4.x drift). The spec-drift CI job only checks the committed openapi.json's VERSION string;
-    // it does NOT check that these structs still match the spec's schemas. Without these, a
-    // rebase reintroducing a 1.4.x field, or flipping `allowed_pools` back to a non-Option Vec,
-    // compiles clean and ships — exactly the regression class the realignment fixed.
-    use super::*;
-
-    #[test]
-    fn key_view_allowed_pools_null_is_all_pools() {
-        // null (or omitted) allowed_pools => None => "all pools". A regression to `Vec<String>`
-        // would fail to deserialize null, or silently decode it as an empty (== NO pools) list.
-        let k: KeyView = serde_json::from_str(
-            r#"{"id":"vk_1","name":"n","allowed_pools":null,"state":"active","enabled":true}"#,
-        )
-        .expect("null allowed_pools must decode");
-        assert_eq!(
-            k.allowed_pools, None,
-            "null must mean all-pools (None), not []"
-        );
-    }
-
-    #[test]
-    fn key_view_allowed_pools_empty_is_no_pools() {
-        let k: KeyView =
-            serde_json::from_str(r#"{"id":"vk_1","name":"n","allowed_pools":[]}"#).unwrap();
-        assert_eq!(
-            k.allowed_pools,
-            Some(Vec::new()),
-            "explicit [] must stay a distinct empty list (NO pools), never collapse to None"
-        );
-    }
-
-    #[test]
-    fn created_key_view_carries_signed_token_not_secret() {
-        // 1.5.0 credential is `token` (+ expires_at), NOT the 1.4.x `secret`. A revert to a
-        // required `secret` field would fail to decode this real-shaped response.
-        let c: CreatedKeyView = serde_json::from_str(
-            r#"{"id":"vk_1","name":"n","token":"bbk_abc","expires_at":1785772871,"state":"active"}"#,
-        )
-        .expect("token-shaped CreatedKeyView must decode");
-        assert_eq!(c.token, "bbk_abc");
-        assert_eq!(c.expires_at, 1785772871_u64);
-    }
-
-    #[test]
-    fn create_key_req_omits_none_fields_and_never_sends_legacy_budget() {
-        // deny_unknown_fields on the server rejects any stray field. This pins the exact minimal
-        // body and fails if a 1.4.x budget/rpm/tpm field is ever reintroduced onto the struct.
-        let req = CreateKeyReq {
-            name: "svc".into(),
-            allowed_pools: None,
-            group: None,
-            parent: None,
-            expires_in: None,
-            expires_at: None,
-            labels: Default::default(),
-            issue_aws_credential: false,
-        };
-        let v: serde_json::Value = serde_json::to_value(&req).unwrap();
-        let obj = v.as_object().unwrap();
-        assert_eq!(
-            obj.keys().collect::<Vec<_>>(),
-            vec!["name"],
-            "an all-defaults CreateKeyReq must serialize to exactly {{name}} — any extra key is \
-             either a leaked None or a reintroduced legacy field the server will 400 on"
-        );
-        for banned in [
-            "budget",
-            "budget_period",
-            "rpm_limit",
-            "tpm_limit",
-            "max_budget_cents",
-        ] {
-            assert!(
-                !obj.contains_key(banned),
-                "legacy field {banned} must never serialize"
-            );
-        }
-    }
-
-    #[test]
-    fn create_key_req_empty_allowed_pools_serializes_as_empty_array() {
-        // The --no-pools path sets Some(vec![]); it must reach the wire as `[]`, not be dropped.
-        let req = CreateKeyReq {
-            name: "svc".into(),
-            allowed_pools: Some(Vec::new()),
-            group: None,
-            parent: None,
-            expires_in: None,
-            expires_at: None,
-            labels: Default::default(),
-            issue_aws_credential: false,
-        };
-        let v: serde_json::Value = serde_json::to_value(&req).unwrap();
-        assert_eq!(v["allowed_pools"], serde_json::json!([]));
-    }
-
-    #[test]
-    fn rotated_key_view_token_and_secret_are_both_optional() {
-        // The doc invariant is "exactly one of token or secret"; the type models both as optional
-        // so a token-only rotate decodes without a phantom `secret`.
-        let r: RotatedKeyView =
-            serde_json::from_str(r#"{"id":"vk_1","name":"n","token":"bbk_new"}"#).unwrap();
-        assert_eq!(r.token.as_deref(), Some("bbk_new"));
-        assert_eq!(r.secret, None);
-    }
-
-    #[test]
-    fn inspect_view_carries_manifest_preview_shape() {
-        // 1.5.2 added POST /plugins/inspect — a stateless preview whose body is the PluginSchemaView
-        // shape plus version/kind. This pins the fields busbar-admin renders so a future spec resync
-        // that drops/renames one (e.g. `trust`, `schema_error`, or the new `version`) fails loudly.
-        let v: InspectView = serde_json::from_str(
-            r#"{"name":"acme-store","version":"1.2.3","kind":"store","trust":"unverified",
-                "source":"manifest","restart_required_default":true,
-                "schema":{"type":"object"},"schema_error":null}"#,
-        )
-        .expect("inspect preview must decode");
-        assert_eq!(v.name, "acme-store");
-        assert_eq!(v.version.as_deref(), Some("1.2.3"));
-        assert_eq!(v.kind.as_deref(), Some("store"));
-        assert_eq!(v.trust, "unverified");
-        assert_eq!(v.restart_required_default, Some(true));
-        assert!(v.schema.is_some());
-        assert_eq!(v.schema_error, None);
-    }
-
-    #[test]
-    fn inspect_view_tolerates_null_kind_and_absent_version() {
-        // An unresolvable candidate reports kind/version/restart_required_default as null; the CLI
-        // must still decode (never refuse) so it can render the `trust`/`schema_error` verdict.
-        let v: InspectView = serde_json::from_str(
-            r#"{"name":"bad","kind":null,"trust":"rejected","source":"manifest","schema":null,
-                "schema_error":"settings_schema is not valid JSON"}"#,
-        )
-        .expect("a rejected/unresolvable candidate must still decode");
-        assert_eq!(v.kind, None);
-        assert_eq!(v.version, None);
-        assert_eq!(v.trust, "rejected");
-        assert_eq!(
-            v.schema_error.as_deref(),
-            Some("settings_schema is not valid JSON")
-        );
-    }
-}

src/lib.rs

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: Apache-2.0
+#![forbid(unsafe_code)]
+
+//! The busbar-admin library surface.
+//!
+//! The binary (`src/main.rs`) is the clap surface + rendering; everything it needs to talk to a
+//! gateway, and every pure mapping it applies to CLI arguments, lives here so integration tests
+//! under `tests/` exercise the SAME code the binary runs (a `[[bin]]`-only crate cannot be
+//! imported by a test, which is why the wire types could previously only be tested from an
+//! inline `#[cfg(test)]` module).
+
+pub mod argmap;
+pub mod client;

src/main.rs

@@ -4,29 +4,15 @@
 //! busbar-admin — a human-facing CLI for the busbar gateway's admin API (`/api/v1/admin`).
 //!
 //! Config resolution is CLI flag > env var > a clear error. The thin admin client lives in
-//! [`client`]; this module is the clap surface + human/JSON rendering.
-
-mod client;
+//! `busbar_admin::client`; this module is the clap surface + human/JSON rendering.
 
 use anyhow::{Context, Result};
 use clap::{Args, Parser, Subcommand};
 
-use client::{Client, CreateKeyReq, InspectPluginReq, InstallPluginReq, KeyView, PluginView, Tls};
-
-/// Resolve the three distinct `allowed_pools` states the server understands: omitted (`None`) =
-/// ALL pools; an explicit empty list (`--no-pools`) = NO pools; a non-empty list = exactly those.
-/// A shared function so `cmd_keys_create` and its test exercise the SAME mapping — collapsing
-/// `--no-pools` into `None` would mint an all-pools key when no-pools was asked for (fail-open on
-/// privilege).
-fn resolve_allowed_pools(no_pools: bool, pools: &[String]) -> Option<Vec<String>> {
-    if no_pools {
-        Some(Vec::new())
-    } else if pools.is_empty() {
-        None
-    } else {
-        Some(pools.to_vec())
-    }
-}
+use busbar_admin::argmap::{parse_labels, resolve_allowed_pools};
+use busbar_admin::client::{
+    Client, CreateKeyReq, InspectPluginReq, InstallPluginReq, KeyView, PluginView, Tls,
+};
 
 /// busbar-admin — talk to a busbar gateway's admin API.
 #[derive(Parser)]
@@ -338,17 +324,6 @@ fn cmd_keys_list(c: &Client, json: bool) -> Result<()> {
     Ok(())
 }
 
-fn parse_labels(pairs: &[String]) -> Result<std::collections::BTreeMap<String, String>> {
-    pairs
-        .iter()
-        .map(|p| {
-            p.split_once('=')
-                .map(|(k, v)| (k.to_string(), v.to_string()))
-                .ok_or_else(|| anyhow::anyhow!("--label must be KEY=VALUE, got {p:?}"))
-        })
-        .collect()
-}
-
 fn pools_summary(pools: &Option<Vec<String>>) -> String {
     match pools {
         None => "(all)".into(),
@@ -737,51 +712,3 @@ fn human_duration(secs: u64) -> String {
     parts.push(format!("{s}s"));
     parts.join(" ")
 }
-
-#[cfg(test)]
-mod cli_logic_tests {
-    use super::*;
-
-    #[test]
-    fn parse_labels_splits_on_first_equals_only() {
-        // A value containing '=' (a URL query string, a base64 pad) must survive intact — the
-        // split is on the FIRST '=', not all of them.
-        let m = parse_labels(&["url=http://x?a=b".into(), "team=platform".into()]).unwrap();
-        assert_eq!(m.get("url").map(String::as_str), Some("http://x?a=b"));
-        assert_eq!(m.get("team").map(String::as_str), Some("platform"));
-    }
-
-    #[test]
-    fn parse_labels_rejects_a_pair_with_no_equals() {
-        assert!(parse_labels(&["novalue".into()]).is_err());
-    }
-
-    #[test]
-    fn parse_labels_allows_empty_value() {
-        let m = parse_labels(&["k=".into()]).unwrap();
-        assert_eq!(m.get("k").map(String::as_str), Some(""));
-    }
-
-    // Calls the REAL `resolve_allowed_pools` that `cmd_keys_create` uses — not a copy — so a
-    // regression in the actual fail-open-on-privilege mapping fails this test. (The round-1
-    // version tested a duplicated helper and was tautological: it passed even if the real code
-    // regressed.)
-    #[test]
-    fn allowed_pools_tristate_no_pools_is_empty_not_none() {
-        assert_eq!(
-            resolve_allowed_pools(true, &[]),
-            Some(Vec::new()),
-            "--no-pools => NO pools"
-        );
-        assert_eq!(
-            resolve_allowed_pools(false, &[]),
-            None,
-            "omitted => ALL pools"
-        );
-        assert_eq!(
-            resolve_allowed_pools(false, &["p1".into()]),
-            Some(vec!["p1".into()]),
-            "a list => exactly those pools"
-        );
-    }
-}