diff --git a/AGENTS.md b/AGENTS.md index 4d6590e..67e1d42 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,7 +11,8 @@ This is a Cargo workspace with two crates: The user-facing CLI surface. Contains all logic for local commands, wraps `clickhouse-cloud-api` for cloud. - Cloud handlers go through the `CloudClient` wrapper (`src/cloud/client.rs`), not `clickhouse_cloud_api::Client` directly. The wrapper handles credential precedence, error conversion, and response unwrapping. -- Cloud handlers always support `--json` output unless there is good reason not to. +- Cloud handlers always support `--json` output unless there is good reason not to. JSON is emitted automatically when `--json` is passed or a coding agent is detected (`is_ai_agent::detect()` via the `json_output()` helper in `main.rs`). +- `CloudError` carries a `kind: CloudErrorKind` (`Auth` for 401/403 and missing credentials, else `Generic`). It maps to `Error::AuthRequired` / `Error::Cloud` in `main.rs`, driving `gh`-style exit codes via `Error::exit_code()`: `0` success, `1` error, `2` cancelled, `4` auth required. Use `--help` to learn the current command surface. diff --git a/README.md b/README.md index 1f10b83..96f819f 100644 --- a/README.md +++ b/README.md @@ -709,6 +709,19 @@ clickhousectl cloud --json service list clickhousectl cloud --json service get ``` +`clickhousectl` auto-detects coding-agent contexts (Claude Code, Cursor, Codex, Gemini CLI, Goose, Devin, and any tool that sets the standard `AGENT` env var) and emits JSON to stdout automatically without setting `--json`. + +### Exit codes + +Follow `gh` conventions: + +| Code | Meaning | +| ---- | -------------------------------------------------------- | +| `0` | Success | +| `1` | Error (anything not classified below) | +| `2` | Cancelled (user aborted) | +| `4` | Auth required (no credentials, 401/403, OAuth-only writes) | + ## Skills Install the official ClickHouse Agent Skills from [ClickHouse/agent-skills](https://github.com/ClickHouse/agent-skills). diff --git a/crates/clickhousectl/src/cli.rs b/crates/clickhousectl/src/cli.rs index 5f5f58d..87e3592 100644 --- a/crates/clickhousectl/src/cli.rs +++ b/crates/clickhousectl/src/cli.rs @@ -56,7 +56,8 @@ CONTEXT FOR AGENTS: For write operations, authenticate with API keys: clickhousectl cloud auth login --api-key YOUR_KEY --api-secret YOUR_SECRET If the user doesn't have an account, suggest `clickhousectl cloud auth signup` first. - Add --json to any cloud command for machine-readable output. + JSON emitted automatically for known agents. + Exit codes follow gh conventions: 0 success, 1 error, 2 cancelled, 4 auth required. Typical workflow: `cloud auth login` → `cloud auth status` → `cloud org list` → `cloud service list`")] Cloud(Box), diff --git a/crates/clickhousectl/src/cloud/client.rs b/crates/clickhousectl/src/cloud/client.rs index 8900957..2e98704 100644 --- a/crates/clickhousectl/src/cloud/client.rs +++ b/crates/clickhousectl/src/cloud/client.rs @@ -4,9 +4,33 @@ use std::env; const DEFAULT_BASE_URL: &str = "https://api.clickhouse.cloud/v1"; +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum CloudErrorKind { + #[default] + Generic, + Auth, +} + #[derive(Debug)] pub struct CloudError { pub message: String, + pub kind: CloudErrorKind, +} + +impl CloudError { + pub fn new(message: impl Into) -> Self { + Self { + message: message.into(), + kind: CloudErrorKind::Generic, + } + } + + pub fn auth(message: impl Into) -> Self { + Self { + message: message.into(), + kind: CloudErrorKind::Auth, + } + } } impl std::fmt::Display for CloudError { @@ -133,12 +157,12 @@ fn resolve_auth_with_sources( }; if api_key.is_some() || api_secret.is_some() { - let key = api_key.map(String::from).ok_or_else(|| CloudError { - message: "API key required when --api-key or --api-secret is set".into(), - })?; - let secret = api_secret.map(String::from).ok_or_else(|| CloudError { - message: "API secret required when --api-key or --api-secret is set".into(), - })?; + let key = api_key + .map(String::from) + .ok_or_else(|| CloudError::auth("API key required when --api-key or --api-secret is set"))?; + let secret = api_secret + .map(String::from) + .ok_or_else(|| CloudError::auth("API secret required when --api-key or --api-secret is set"))?; return Ok(ResolvedAuth { creds: ResolvedCreds::Basic { key, secret }, source: AuthSource::CliFlags, @@ -181,9 +205,9 @@ fn resolve_auth_with_sources( }); } - Err(CloudError { - message: "No credentials found. Run `clickhousectl cloud auth login` (OAuth, read-only), `clickhousectl cloud auth login --api-key KEY --api-secret SECRET` (read/write), set CLICKHOUSE_CLOUD_API_KEY + CLICKHOUSE_CLOUD_API_SECRET (also picked up from a `.env` file in the current directory), or use --api-key/--api-secret.\n\nLearn how to create API keys: https://clickhouse.com/docs/cloud/manage/openapi?referrer=clickhousectl".into(), - }) + Err(CloudError::auth( + "No credentials found. Run `clickhousectl cloud auth login` (OAuth, read-only), `clickhousectl cloud auth login --api-key KEY --api-secret SECRET` (read/write), set CLICKHOUSE_CLOUD_API_KEY + CLICKHOUSE_CLOUD_API_SECRET (also picked up from a `.env` file in the current directory), or use --api-key/--api-secret.\n\nLearn how to create API keys: https://clickhouse.com/docs/cloud/manage/openapi?referrer=clickhousectl", + )) } /// Peek which credential source would win precedence right now without @@ -306,9 +330,7 @@ impl CloudClient { let http = reqwest::Client::builder() .user_agent(crate::user_agent::user_agent()) .build() - .map_err(|e| CloudError { - message: format!("Failed to create HTTP client: {}", e), - })?; + .map_err(|e| CloudError::new(format!("Failed to create HTTP client: {}", e)))?; let resolved = resolve_auth(api_key, api_secret, url_override)?; let lib_url = lib_base_url(&resolved.base_url); @@ -354,14 +376,14 @@ impl CloudClient { /// Unwrap an `ApiResponse` into `T`, returning an error if the result is empty. pub fn unwrap_response(response: clickhouse_cloud_api::models::ApiResponse) -> Result { - response.result.ok_or_else(|| CloudError { - message: "Empty response from API".into(), - }) + response + .result + .ok_or_else(|| CloudError::new("Empty response from API")) } /// Convert a library error into a `CloudError`, appending OAuth hints when relevant. pub fn convert_error(&self, err: clickhouse_cloud_api::Error) -> CloudError { - let message = match &err { + match &err { clickhouse_cloud_api::Error::Api { status, message } => { let mut msg = message.clone(); if *status == 403 && self.is_bearer_auth() { @@ -373,11 +395,14 @@ impl CloudClient { https://clickhouse.com/docs/cloud/manage/openapi?referrer=clickhousectl", ); } - msg + if matches!(*status, 401 | 403) { + CloudError::auth(msg) + } else { + CloudError::new(msg) + } } - other => other.to_string(), - }; - CloudError { message } + other => CloudError::new(other.to_string()), + } } // Organization endpoints (delegated to library client) @@ -1042,15 +1067,12 @@ impl CloudClient { pub async fn get_default_org_id(&self) -> Result { let orgs = self.list_organizations().await?; match orgs.len() { - 0 => Err(CloudError { - message: "No organization found for this API key".into(), - }), + 0 => Err(CloudError::new("No organization found for this API key")), 1 => Ok(orgs[0].id.to_string()), - _ => Err(CloudError { - message: "Multiple organizations found. Specify --org-id to choose one. \ - Use `clickhousectl cloud org list` to see your organizations." - .into(), - }), + _ => Err(CloudError::new( + "Multiple organizations found. Specify --org-id to choose one. \ + Use `clickhousectl cloud org list` to see your organizations.", + )), } } } @@ -1215,6 +1237,40 @@ mod tests { assert_eq!(err.message, "Forbidden"); } + #[test] + fn convert_error_flags_401_as_auth() { + let err = test_client().convert_error(clickhouse_cloud_api::Error::Api { + status: 401, + message: "Unauthorized".into(), + }); + assert_eq!(err.kind, CloudErrorKind::Auth); + } + + #[test] + fn convert_error_flags_403_as_auth() { + let err = test_client().convert_error(clickhouse_cloud_api::Error::Api { + status: 403, + message: "Forbidden".into(), + }); + assert_eq!(err.kind, CloudErrorKind::Auth); + } + + #[test] + fn convert_error_treats_other_status_as_generic() { + let err = test_client().convert_error(clickhouse_cloud_api::Error::Api { + status: 500, + message: "Internal Server Error".into(), + }); + assert_eq!(err.kind, CloudErrorKind::Generic); + } + + #[test] + fn convert_error_treats_non_api_error_as_generic() { + let err = + test_client().convert_error(clickhouse_cloud_api::Error::AuthMismatch("nope".into())); + assert_eq!(err.kind, CloudErrorKind::Generic); + } + // ── Dotenv resolver tests ────────────────────────────────────────────── // // Precedence is exercised by feeding `resolve_auth_with_sources` a diff --git a/crates/clickhousectl/src/cloud/mod.rs b/crates/clickhousectl/src/cloud/mod.rs index c55be7b..7a56449 100644 --- a/crates/clickhousectl/src/cloud/mod.rs +++ b/crates/clickhousectl/src/cloud/mod.rs @@ -11,5 +11,6 @@ pub mod types; mod types_test; pub use client::{ - AuthSource, CloudClient, dotenv_env_provenance, env_cred_presence, resolve_active_auth_source, + AuthSource, CloudClient, CloudError, CloudErrorKind, dotenv_env_provenance, env_cred_presence, + resolve_active_auth_source, }; diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index 63d1df1..83e13fd 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -55,15 +55,18 @@ pub enum Error { #[error("{0}")] Cloud(String), + #[error("{0}")] + AuthRequired(String), + + #[error("Cancelled")] + Cancelled, + #[error("{0}")] Skills(String), #[error("Invalid server name '{0}': must not contain path separators or '..'")] InvalidServerName(String), - #[error("--json and --foreground cannot be used together")] - JsonForegroundConflict, - #[error("Docker is not available: {0}")] DockerNotAvailable(String), @@ -73,3 +76,37 @@ pub enum Error { } pub type Result = std::result::Result; + +impl Error { + /// Process exit code following `gh` CLI conventions: + /// `0` success, `1` error, `2` cancelled, `4` auth required. + pub fn exit_code(&self) -> i32 { + match self { + Error::AuthRequired(_) => 4, + Error::Cancelled => 2, + _ => 1, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn auth_required_maps_to_4() { + assert_eq!(Error::AuthRequired("nope".into()).exit_code(), 4); + } + + #[test] + fn cancelled_maps_to_2() { + assert_eq!(Error::Cancelled.exit_code(), 2); + } + + #[test] + fn generic_errors_map_to_1() { + assert_eq!(Error::Cloud("boom".into()).exit_code(), 1); + assert_eq!(Error::NoVersionsInstalled.exit_code(), 1); + assert_eq!(Error::VersionNotFound("25.12".into()).exit_code(), 1); + } +} diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index fe0c08b..83d51fd 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -274,9 +274,8 @@ async fn start_server( args: Vec, json: bool, ) -> Result<()> { - if json && foreground { - return Err(Error::JsonForegroundConflict); - } + // `--foreground` streams the server's stdout/stderr and never emits a JSON + // summary, so it simply ignores `json` rather than erroring on `--json`. // Recover any orphaned servers so name resolution and collision checks // see processes that lost their metadata files. @@ -858,16 +857,6 @@ fn stop_all_servers_global(json: bool) -> Result<()> { mod tests { use super::*; - #[tokio::test] - async fn test_server_start_rejects_json_with_foreground() { - let result = start_server(None, None, None, None, true, vec![], true).await; - let err = result.unwrap_err(); - assert!( - matches!(err, Error::JsonForegroundConflict), - "expected JsonForegroundConflict, got: {err}" - ); - } - #[test] fn parse_postgres_install_spec_recognizes_at_and_colon() { assert_eq!(parse_postgres_install_spec("postgres@16"), Some("16")); diff --git a/crates/clickhousectl/src/main.rs b/crates/clickhousectl/src/main.rs index 5ab6393..f60c9cb 100644 --- a/crates/clickhousectl/src/main.rs +++ b/crates/clickhousectl/src/main.rs @@ -64,13 +64,21 @@ async fn main() { if let Err(e) = result { eprintln!("Error: {}", e); - std::process::exit(1); + std::process::exit(e.exit_code()); } } +/// Resolve whether to emit machine-readable JSON. True when `--json` was passed +/// or we're running under a known coding agent (same detection as the outbound +/// User-Agent in `user_agent.rs`). Pipes/redirects stay human-readable unless +/// `--json` is passed, matching `gh`/`kubectl` norms. +fn json_output(flag: bool) -> bool { + flag || is_ai_agent::detect().is_some() +} + async fn run(cmd: Commands) -> Result<()> { match cmd { - Commands::Local(args) => local::run(args.command, args.json).await, + Commands::Local(args) => local::run(args.command, json_output(args.json)).await, Commands::Skills(args) => run_skills(args).await, Commands::Cloud(args) => run_cloud(*args).await, Commands::Update(args) => run_update(args).await, @@ -113,10 +121,14 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { } else if api_key.is_some() || api_secret.is_some() { // Non-interactive API key login let key = api_key.ok_or_else(|| { - Error::Cloud("--api-key is required when --api-secret is provided".into()) + Error::AuthRequired( + "--api-key is required when --api-secret is provided".into(), + ) })?; let secret = api_secret.ok_or_else(|| { - Error::Cloud("--api-secret is required when --api-key is provided".into()) + Error::AuthRequired( + "--api-secret is required when --api-key is provided".into(), + ) })?; let mut creds = cloud::credentials::load_credentials().unwrap_or_default(); creds.api_key = Some(key); @@ -311,7 +323,7 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { } } - if args.json { + if json_output(args.json) { println!("{}", serde_json::to_string_pretty(&rows)?); } else { println!("{}", Table::new(rows).with(Style::markdown())); @@ -321,7 +333,9 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { }; } - // Refresh OAuth tokens if needed before creating the client + // Refresh OAuth tokens if needed. Errors here are filesystem failures + // (refresh-rpc failures are swallowed and tokens cleared), so this stays + // a generic error rather than `AuthRequired`. cloud::auth::ensure_fresh_tokens() .await .map_err(|e| Error::Cloud(e.to_string()))?; @@ -331,7 +345,7 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { args.api_secret.as_deref(), args.url.as_deref(), ) - .map_err(|e| Error::Cloud(e.to_string()))?; + .map_err(cloud_error_to_top_level)?; if args.debug { eprintln!("[debug] auth source: {}", client.auth_source().describe()); @@ -341,7 +355,7 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { // OAuth (Bearer) tokens are read-only. Block write commands early // to avoid fail loops where agents repeatedly hit 403 errors. if client.is_bearer_auth() && args.command.is_write_command() { - return Err(Error::Cloud( + return Err(Error::AuthRequired( "This command requires API key authentication. \ OAuth (browser login) provides read-only access.\n\n\ To authenticate with an API key:\n \ @@ -355,7 +369,7 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { )); } - let json = args.json; + let json = json_output(args.json); let result = match args.command { CloudCommands::Auth { .. } => unreachable!("handled above"), @@ -985,7 +999,24 @@ async fn run_cloud(args: CloudArgs) -> Result<()> { }, }; - result.map_err(|e| Error::Cloud(e.to_string())) + result.map_err(boxed_cloud_error_to_top_level) +} + +fn cloud_error_to_top_level(e: cloud::CloudError) -> Error { + match e.kind { + cloud::CloudErrorKind::Auth => Error::AuthRequired(e.message), + cloud::CloudErrorKind::Generic => Error::Cloud(e.message), + } +} + +// Cloud command fns return `Box`, so the `CloudError.kind` +// only survives via downcast — without it, auth-flagged errors silently fall back +// to `Error::Cloud` (exit 1) instead of `Error::AuthRequired` (exit 4). +fn boxed_cloud_error_to_top_level(e: Box) -> Error { + match e.downcast::() { + Ok(ce) => cloud_error_to_top_level(*ce), + Err(other) => Error::Cloud(other.to_string()), + } } async fn run_postgres( @@ -1200,3 +1231,49 @@ async fn run_postgres( } } } + +#[cfg(test)] +mod tests { + use super::*; + use cloud::{CloudError, CloudErrorKind}; + + #[test] + fn json_output_true_when_flag_set() { + assert!(json_output(true)); + } + + #[test] + fn cloud_error_kind_routes_to_top_level() { + assert!(matches!( + cloud_error_to_top_level(CloudError::auth("nope")), + Error::AuthRequired(_) + )); + assert!(matches!( + cloud_error_to_top_level(CloudError::new("boom")), + Error::Cloud(_) + )); + // Default kind is Generic. + assert_eq!(CloudError::new("x").kind, CloudErrorKind::Generic); + } + + #[test] + fn boxed_cloud_error_preserves_auth_kind_through_downcast() { + let boxed: Box = Box::new(CloudError::auth("nope")); + assert!(matches!( + boxed_cloud_error_to_top_level(boxed), + Error::AuthRequired(_) + )); + } + + #[test] + fn boxed_non_cloud_error_falls_back_to_generic() { + // Anything that isn't a CloudError must not downcast to AuthRequired — + // it falls back to Error::Cloud (exit 1). This pins the contract that a + // handler stringifying a CloudError before boxing silently loses exit 4. + let boxed: Box = "plain string error".into(); + assert!(matches!( + boxed_cloud_error_to_top_level(boxed), + Error::Cloud(_) + )); + } +}