From 3639d3cb6e9d26dba24f5b60b60621419423e369 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radim=20H=C3=B6fer?= Date: Wed, 7 Oct 2026 16:23:36 +0200 Subject: [PATCH 1/2] fix: exit non-zero whenever a command reports an error Every command that printed an error still exited 0, so scripts and CI could not tell a failed pull, secrets request or config change from a successful one. A failed command now exits 1; a child process keeps its own exit code. Handlers that print their own error and used to return normally now return ReportedFailure, which exits 1 without printing the error twice: run and pull when no secrets are found (or the user declines to proceed after that error), run when fetching secrets fails, and the config setters, which returned nothing at all. The bare `agent hooks` invocation keeps exiting 0 on failure, since Claude Code, Codex and Cursor read its exit code. --- src/exit.rs | 99 ++++++++++++------ src/handlers/config/api_key.rs | 13 +-- src/handlers/config/expand_refs.rs | 19 ++-- src/handlers/config/output.rs | 14 ++- src/handlers/config/output_secrets.rs | 14 ++- src/handlers/config/profile.rs | 92 +++++++++++------ src/handlers/entry/config.rs | 10 +- src/handlers/entry/root.rs | 109 +++++++++----------- src/handlers/pull/entry.rs | 6 +- src/handlers/run/entry.rs | 15 +-- src/telemetry/event.rs | 14 ++- tests/exit_status_cli.rs | 139 +++++++++++++++++++++----- 12 files changed, 361 insertions(+), 183 deletions(-) diff --git a/src/exit.rs b/src/exit.rs index f2f7d17b..9dd23607 100644 --- a/src/exit.rs +++ b/src/exit.rs @@ -1,6 +1,7 @@ //! How a command ends: the exit code the shell sees, and whether the command -//! failed. They are separate on purpose, because many failures print an error -//! and still exit 0. +//! failed. A failed command exits 1 unless it says otherwise; the one +//! exception is the bare `agent hooks` invocation (see +//! [`Exit::zero_on_failure`]). //! //! `handle_cli` returns an [`Exit`], and `main` ends the process with //! [`Exit::terminate`], the only place that reports to telemetry and exits. @@ -38,10 +39,9 @@ impl Exit { } } - /// A failure that exits 0: the error was printed and the command returned - /// normally. Changing such a code is a product decision, not a refactor. + /// A failure whose error was already printed: exit code 1. pub fn failed(kind: ErrorKind) -> Self { - Self::failed_with(kind, 0) + Self::failed_with(kind, 1) } pub fn failed_with(kind: ErrorKind, code: i32) -> Self { @@ -51,13 +51,13 @@ impl Exit { } } - /// Gives a failure that would exit 0 exit code 1 instead. `agent` - /// commands use this so a profile that fails closed is not reported to - /// scripts as a success. Successes and explicit codes are left alone. - pub fn nonzero_on_failure(self) -> Self { + /// Keeps a failure at exit code 0, as every command did before failures + /// exited 1. Only for invocations other tools run and read the exit code + /// of, whose codes must not change; telemetry still sees the failure. + pub fn zero_on_failure(self) -> Self { match self.failure { - Some(_) if self.code == 0 => Self { code: 1, ..self }, - _ => self, + Some(_) => Self { code: 0, ..self }, + None => self, } } @@ -71,16 +71,47 @@ impl Exit { } impl From for Exit { - /// A failed command exits 0 unless the error says otherwise: a child - /// process that failed (`CommandFailed`) keeps its own exit code. + /// A failed command exits 1, except a child process that failed + /// (`CommandFailed`), which keeps its own exit code. fn from(error: anyhow::Error) -> Self { + if let Some(reported) = error.downcast_ref::() { + return Self::failed(reported.kind); + } let code = error .downcast_ref::() - .map_or(0, CommandFailed::exit_code); + .map_or(1, CommandFailed::exit_code); Self::failed_with(classify_error(&error), code) } } +/// An error a handler has already printed. Returning it fails the command +/// (exit 1) without printing anything more, for handlers that format their +/// own error output. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ReportedFailure { + pub kind: ErrorKind, +} + +impl ReportedFailure { + pub fn new(kind: ErrorKind) -> anyhow::Error { + anyhow::Error::new(Self { kind }) + } +} + +impl std::fmt::Display for ReportedFailure { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("command failed") + } +} + +impl std::error::Error for ReportedFailure {} + +/// Whether an error still has to be printed: one a handler already printed +/// (`ReportedFailure`) has not. +pub fn needs_printing(error: &anyhow::Error) -> bool { + error.downcast_ref::().is_none() +} + /// Lets the `Result<()>` most handlers return stand in for a command's /// outcome: success is `Exit::ok()`, an error is left for the caller to print. pub trait IntoExit { @@ -131,11 +162,11 @@ mod tests { } #[test] - fn failed_reports_a_failure_but_keeps_exit_code_0() { + fn failed_reports_a_failure_and_exits_1() { assert_eq!( Exit::failed(ErrorKind::Validation), Exit { - code: 0, + code: 1, failure: Some(ErrorKind::Validation) } ); @@ -153,24 +184,38 @@ mod tests { } #[test] - fn an_error_is_classified_by_type_and_exits_0() { + fn an_error_is_classified_by_type_and_exits_1() { let validation = anyhow::anyhow!(InputValidationError::MissingApiKey); assert_eq!( Exit::from(validation), Exit { - code: 0, + code: 1, failure: Some(ErrorKind::Validation) } ); assert_eq!( Exit::from(anyhow::anyhow!("anything else")), Exit { - code: 0, + code: 1, failure: Some(ErrorKind::Other) } ); } + #[test] + fn a_reported_failure_exits_1_with_its_kind_and_is_not_printed_again() { + let reported = ReportedFailure::new(ErrorKind::NotFound); + assert!(!needs_printing(&reported)); + assert_eq!( + Exit::from(reported), + Exit { + code: 1, + failure: Some(ErrorKind::NotFound) + } + ); + assert!(needs_printing(&anyhow::anyhow!("not printed yet"))); + } + #[cfg(unix)] #[test] fn a_failed_child_keeps_its_exit_code() { @@ -196,22 +241,16 @@ mod tests { } #[test] - fn nonzero_on_failure_only_replaces_a_failure_that_exits_0() { + fn zero_on_failure_only_changes_failures() { assert_eq!( - Exit::failed(ErrorKind::Validation).nonzero_on_failure(), + Exit::failed(ErrorKind::Validation).zero_on_failure(), Exit { - code: 1, + code: 0, failure: Some(ErrorKind::Validation) } ); - assert_eq!(Exit::ok().nonzero_on_failure(), Exit::ok()); - assert_eq!(Exit::code(3).nonzero_on_failure(), Exit::code(3)); - assert_eq!( - Exit::failed_with(ErrorKind::Other, 7) - .nonzero_on_failure() - .code, - 7 - ); + assert_eq!(Exit::ok().zero_on_failure(), Exit::ok()); + assert_eq!(Exit::code(3).zero_on_failure(), Exit::code(3)); } #[test] diff --git a/src/handlers/config/api_key.rs b/src/handlers/config/api_key.rs index 09e4ab5d..404ab90c 100644 --- a/src/handlers/config/api_key.rs +++ b/src/handlers/config/api_key.rs @@ -1,9 +1,9 @@ -// use anyhow::Result; - use std::io::{self, Read}; use crate::{ config::{config, secure_store}, + exit::ReportedFailure, + telemetry::event::ErrorKind, utils::{interaction::input_password, output::ColorizeIfColoredOutput}, }; @@ -21,12 +21,12 @@ pub fn read_api_key(read_from_stdin: bool) -> Result { } } -pub fn set_api_key(read_from_stdin: bool, profile: &str) { +pub fn set_api_key(read_from_stdin: bool, profile: &str) -> anyhow::Result<()> { let api_key_value = match read_api_key(read_from_stdin) { Ok(value) => value, Err(message) => { eprintln!("{}", message.red_if_tty_stderr()); - return; + return Err(ReportedFailure::new(ErrorKind::Validation)); } }; @@ -39,7 +39,7 @@ pub fn set_api_key(read_from_stdin: bool, profile: &str) { "Error:".red_if_tty_stderr(), store_err ); - return; + return Err(ReportedFailure::new(ErrorKind::Other)); } let fallback_res = config::update_config(crate::models::config::UpdateConfig { api_key: Some(api_key_value), @@ -49,7 +49,7 @@ pub fn set_api_key(read_from_stdin: bool, profile: &str) { if let Err(fallback_err) = fallback_res { eprintln!("{} {}", "Error:".red_if_tty_stderr(), fallback_err); - return; + return Err(ReportedFailure::new(ErrorKind::Other)); } eprintln!( @@ -65,6 +65,7 @@ pub fn set_api_key(read_from_stdin: bool, profile: &str) { } println!("API key set."); } + Ok(()) } fn read_api_key_from_stdin() -> Result { diff --git a/src/handlers/config/expand_refs.rs b/src/handlers/config/expand_refs.rs index e4dcd755..e157c439 100644 --- a/src/handlers/config/expand_refs.rs +++ b/src/handlers/config/expand_refs.rs @@ -1,13 +1,18 @@ -use crate::{config::config, models::config::UpdateConfig, utils::output::ColorizeIfColoredOutput}; +use anyhow::Result; -pub fn set_expand_refs_config(enabled: Option) { +use crate::{ + config::config, exit::ReportedFailure, models::config::UpdateConfig, + telemetry::event::ErrorKind, utils::output::ColorizeIfColoredOutput, +}; + +pub fn set_expand_refs_config(enabled: Option) -> Result<()> { if let None = enabled { eprintln!( "{} {}", "Error:".red_if_tty_stderr(), "No 'enabled' boolean value provided." ); - return; + return Err(ReportedFailure::new(ErrorKind::Validation)); } let enabled = enabled.unwrap(); @@ -20,10 +25,12 @@ pub fn set_expand_refs_config(enabled: Option) { if let Err(err) = res { eprintln!("{} {}", "Error:".red_if_tty_stderr(), err); - } else { - let msg = format!("Default expand-refs config set."); - println!("{}", msg); + return Err(ReportedFailure::new(ErrorKind::Other)); } + + let msg = format!("Default expand-refs config set."); + println!("{}", msg); + Ok(()) } pub fn print_expand_refs_config(enabled: &Option) { diff --git a/src/handlers/config/output.rs b/src/handlers/config/output.rs index ab75a471..50228c5f 100644 --- a/src/handlers/config/output.rs +++ b/src/handlers/config/output.rs @@ -1,11 +1,15 @@ +use anyhow::Result; + use crate::{ cmd::config::OutputFormat, config::config, + exit::ReportedFailure, models::config::{OutputFormatConfig, UpdateConfig}, + telemetry::event::ErrorKind, utils::output::ColorizeIfColoredOutput, }; -pub fn set_default_output_format(output_format: OutputFormat) { +pub fn set_default_output_format(output_format: OutputFormat) -> Result<()> { let res = config::update_config(UpdateConfig { api_key: None, expand_refs: None, @@ -17,10 +21,12 @@ pub fn set_default_output_format(output_format: OutputFormat) { if let Err(err) = res { eprintln!("{} {}", "Error:".red_if_tty_stderr(), err); - } else { - let msg = format!("{} {}", "✔".green_if_tty(), "Default output format set."); - println!("{}", msg); + return Err(ReportedFailure::new(ErrorKind::Other)); } + + let msg = format!("{} {}", "✔".green_if_tty(), "Default output format set."); + println!("{}", msg); + Ok(()) } pub fn print_default_output_format(output_format: &OutputFormat) { diff --git a/src/handlers/config/output_secrets.rs b/src/handlers/config/output_secrets.rs index 2c97d0b9..2a96aad5 100644 --- a/src/handlers/config/output_secrets.rs +++ b/src/handlers/config/output_secrets.rs @@ -1,11 +1,15 @@ +use anyhow::Result; + use crate::{ cmd::config::SecretsOutputFormat, config::config, + exit::ReportedFailure, models::config::{OutputFormatConfig, UpdateConfig}, + telemetry::event::ErrorKind, utils::output::ColorizeIfColoredOutput, }; -pub fn set_default_secrets_output_format(output_format: SecretsOutputFormat) { +pub fn set_default_secrets_output_format(output_format: SecretsOutputFormat) -> Result<()> { let res = config::update_config(UpdateConfig { api_key: None, expand_refs: None, @@ -17,10 +21,12 @@ pub fn set_default_secrets_output_format(output_format: SecretsOutputFormat) { if let Err(err) = res { eprintln!("{} {}", "Error:".red_if_tty_stderr(), err); - } else { - let msg = format!("Default secrets output format set."); - println!("{}", msg); + return Err(ReportedFailure::new(ErrorKind::Other)); } + + let msg = format!("Default secrets output format set."); + println!("{}", msg); + Ok(()) } pub fn print_default_secrets_output_format(output_format: &SecretsOutputFormat) { diff --git a/src/handlers/config/profile.rs b/src/handlers/config/profile.rs index 1c86367b..3731bd15 100644 --- a/src/handlers/config/profile.rs +++ b/src/handlers/config/profile.rs @@ -1,29 +1,42 @@ +use anyhow::Result; + use crate::{ cmd::config::{AddProfile, ProfileSubcommand}, config::{config, secure_store}, + exit::ReportedFailure, handlers::config::api_key, models::config::Config, + telemetry::event::ErrorKind, utils::output::{get_formatted_json_string, ColorizeIfColoredOutput}, }; use serde::Serialize; -pub fn handle_profile_command(command: ProfileSubcommand, config_data: &Config, json_output: bool) { +pub fn handle_profile_command( + command: ProfileSubcommand, + config_data: &Config, + json_output: bool, +) -> Result<()> { match command { ProfileSubcommand::Add(args) => add_profile(args, config_data), ProfileSubcommand::List => list_profiles(config_data, json_output), ProfileSubcommand::Current => match config::resolve_profile_name(config_data) { Ok(profile) if json_output => print_json(&CurrentProfileOutput { profile }), - Ok(profile) => println!("{}", profile.blue_bold_if_tty()), - Err(error) => eprintln!("{} {}", "Error:".red_if_tty_stderr(), error), + Ok(profile) => { + println!("{}", profile.blue_bold_if_tty()); + Ok(()) + } + Err(error) => report_error(error), }, ProfileSubcommand::Use(args) => match config::set_default_profile(&args.name) { - Ok(()) => println!("Default profile set to '{}'.", args.name), - Err(error) => eprintln!("{} {}", "Error:".red_if_tty_stderr(), error), + Ok(()) => { + println!("Default profile set to '{}'.", args.name); + Ok(()) + } + Err(error) => report_error(error), }, ProfileSubcommand::Remove(args) => { if let Err(error) = ensure_profile_can_be_removed(config_data, &args.name) { - eprintln!("{} {}", "Error:".red_if_tty_stderr(), error); - return; + return report_error(error); } if let Err(error) = secure_store::delete_api_key_for_profile(&args.name) { eprintln!( @@ -32,20 +45,32 @@ pub fn handle_profile_command(command: ProfileSubcommand, config_data: &Config, args.name, error ); - return; + return Err(ReportedFailure::new(ErrorKind::Other)); } match config::remove_profile(&args.name) { - Ok(()) => println!("Profile '{}' removed.", args.name), - Err(error) => eprintln!( - "{} The secure-store key was removed, but profile metadata could not be removed: {}", - "Warning:".yellow_if_tty_stderr(), - error - ), + Ok(()) => { + println!("Profile '{}' removed.", args.name); + Ok(()) + } + Err(error) => { + eprintln!( + "{} The secure-store key was removed, but profile metadata could not be removed: {}", + "Warning:".yellow_if_tty_stderr(), + error + ); + Err(ReportedFailure::new(ErrorKind::Other)) + } } } } } +/// Prints `error` the way every profile command does and fails the command. +fn report_error(error: impl std::fmt::Display) -> Result<()> { + eprintln!("{} {}", "Error:".red_if_tty_stderr(), error); + Err(ReportedFailure::new(ErrorKind::Validation)) +} + fn ensure_profile_can_be_removed(config_data: &Config, name: &str) -> anyhow::Result<()> { config::validate_profile_name(name)?; if name == config::DEFAULT_PROFILE { @@ -63,23 +88,20 @@ fn ensure_profile_can_be_removed(config_data: &Config, name: &str) -> anyhow::Re Ok(()) } -fn add_profile(args: AddProfile, config_data: &Config) { +fn add_profile(args: AddProfile, config_data: &Config) -> Result<()> { if let Err(error) = config::validate_profile_name(&args.name) { - eprintln!("{} {}", "Error:".red_if_tty_stderr(), error); - return; + return report_error(error); } if args.name == config::DEFAULT_PROFILE { - eprintln!( - "{} 'default' is the implicit profile; use 'config api-key set' to manage its key.", - "Error:".red_if_tty_stderr() + return report_error( + "'default' is the implicit profile; use 'config api-key set' to manage its key.", ); - return; } let api_key = match api_key::read_api_key(args.stdin) { Ok(key) => key, Err(error) => { eprintln!("{}", error.red_if_tty_stderr()); - return; + return Err(ReportedFailure::new(ErrorKind::Validation)); } }; if let Err(error) = secure_store::set_api_key_for_profile(&args.name, &api_key) { @@ -88,24 +110,28 @@ fn add_profile(args: AddProfile, config_data: &Config) { "Error:".red_if_tty_stderr(), error ); - return; + return Err(ReportedFailure::new(ErrorKind::Other)); } let profile_already_exists = config_data .profiles .as_ref() .is_some_and(|profiles| profiles.contains_key(&args.name)); match config::add_profile(&args.name, args.workspace) { - Ok(()) => println!("Profile '{}' saved.", args.name), + Ok(()) => { + println!("Profile '{}' saved.", args.name); + Ok(()) + } Err(error) => { if !profile_already_exists { let _ = secure_store::delete_api_key_for_profile(&args.name); } eprintln!("{} {}", "Error:".red_if_tty_stderr(), error); + Err(ReportedFailure::new(ErrorKind::Other)) } } } -fn list_profiles(config_data: &Config, json_output: bool) { +fn list_profiles(config_data: &Config, json_output: bool) -> Result<()> { let default = config_data .default_profile .as_deref() @@ -128,8 +154,7 @@ fn list_profiles(config_data: &Config, json_output: bool) { }), ); } - print_json(&ProfileListOutput { profiles }); - return; + return print_json(&ProfileListOutput { profiles }); } print_profile_row( config::DEFAULT_PROFILE, @@ -144,6 +169,7 @@ fn list_profiles(config_data: &Config, json_output: bool) { print_profile_row(name, profile.workspace.as_deref(), name == default); } } + Ok(()) } #[derive(Serialize)] @@ -164,10 +190,16 @@ struct ProfileListItem { is_default: bool, } -fn print_json(value: &T) { +fn print_json(value: &T) -> Result<()> { match get_formatted_json_string(value, true) { - Ok(json) => println!("{json}"), - Err(error) => eprintln!("{} {}", "Error:".red_if_tty_stderr(), error), + Ok(json) => { + println!("{json}"); + Ok(()) + } + Err(error) => { + eprintln!("{} {}", "Error:".red_if_tty_stderr(), error); + Err(ReportedFailure::new(ErrorKind::Other)) + } } } diff --git a/src/handlers/entry/config.rs b/src/handlers/entry/config.rs index eeb01166..495a846f 100644 --- a/src/handlers/entry/config.rs +++ b/src/handlers/entry/config.rs @@ -30,7 +30,7 @@ pub fn handle_config_commands( ConfigSubcommand::ApiKey(k) => match k.subcommand { ApiKeySubcommand::Set(s) => { let profile = crate::config::config::resolve_profile_name(config)?; - api_key::set_api_key(s.stdin, &profile); + api_key::set_api_key(s.stdin, &profile)?; } ApiKeySubcommand::Print(_) => { let profile = crate::config::config::resolve_profile_name(config)?; @@ -49,11 +49,11 @@ pub fn handle_config_commands( } }, ConfigSubcommand::Profile(command) => { - handle_profile_command(command.subcommand, config, json_output) + handle_profile_command(command.subcommand, config, json_output)? } ConfigSubcommand::Output(o) => match o.subcommand { OutputSubcommand::Set(s) => { - set_default_output_format(s.format); + set_default_output_format(s.format)?; } OutputSubcommand::Print => { if let Some(config) = &config.ouput_format { @@ -69,7 +69,7 @@ pub fn handle_config_commands( }, ConfigSubcommand::OutputSecrets(s) => match s.subcommand { SecretsOutputSubcommand::Set(s) => { - set_default_secrets_output_format(s.format); + set_default_secrets_output_format(s.format)?; } SecretsOutputSubcommand::Print => { if let Some(config) = &config.ouput_format { @@ -131,7 +131,7 @@ pub fn handle_config_commands( } ConfigSubcommand::ExpandRefs(r) => match r.subcommand { ExpandRefsSubcommand::Set(args) => { - set_expand_refs_config(args.enabled); + set_expand_refs_config(args.enabled)?; } ExpandRefsSubcommand::Print => print_expand_refs_config(&config.expand_refs), }, diff --git a/src/handlers/entry/root.rs b/src/handlers/entry/root.rs index 8ff45bbc..24049322 100644 --- a/src/handlers/entry/root.rs +++ b/src/handlers/entry/root.rs @@ -23,7 +23,7 @@ use crate::{ root::{Cli, EntityType, WhoamiCommand, WhoamiOutputFormat}, }, config::{config, secure_store}, - exit::{Exit, IntoExit}, + exit::{needs_printing, Exit, IntoExit}, handlers::{ agent::{ doctor::handle_agent_doctor_command, @@ -407,6 +407,7 @@ pub async fn handle_cli(args: Cli) -> Exit { }; } + let bare_agent_hook = is_bare_agent_hook(&args.entity_type); let config = config::get_config(); if let Ok(config) = config { if let EntityType::Config(cmd) = args.entity_type { @@ -419,7 +420,7 @@ pub async fn handle_cli(args: Cli) -> Exit { Ok(profile) => profile, Err(error) => { eprintln!("{}", error); - return Exit::failed(ErrorKind::Validation); + return exit_for(Exit::failed(ErrorKind::Validation), bare_agent_hook); } }; let secure_store_api_key = match secure_store::get_api_key_for_profile(&profile_name) { @@ -491,7 +492,6 @@ pub async fn handle_cli(args: Cli) -> Exit { // Local commands such as `agent logs` do not need Stashbase authentication. let api_key = api_key.unwrap_or_default(); - let agent_failures_exit_1 = agent_failures_exit_1(&args.entity_type); let result: anyhow::Result = match args.entity_type { EntityType::Whoami(WhoamiCommand { format }) => { @@ -1484,17 +1484,14 @@ pub async fn handle_cli(args: Cli) -> Exit { eprintln!("{}", "Request aborted".red_if_tty_stderr()); return Exit::code(130); } - eprintln!("{:?}", err); + if needs_printing(&err) { + eprintln!("{:?}", err); + } Exit::from(err) } }; - if agent_failures_exit_1 { - exit.nonzero_on_failure() - } else { - exit - } + exit_for(exit, bare_agent_hook) } else { - let agent_failures_exit_1 = agent_failures_exit_1(&args.entity_type); if let EntityType::Config(cmd) = args.entity_type { if let ConfigSubcommand::Reset(_) = cmd.subcommand { return print_error(handle_config_commands(cmd, &Config::new(), args.raw)); @@ -1506,25 +1503,21 @@ pub async fn handle_cli(args: Cli) -> Exit { eprintln!("{}", "Request aborted".red_if_tty_stderr()); return Exit::code(130); } - // An unreadable or malformed config file: printed, and the command - // still exits 0. + // An unreadable or malformed config file. eprintln!("{:?}", err); - let exit = Exit::failed(ErrorKind::Validation); - if agent_failures_exit_1 { - exit.nonzero_on_failure() - } else { - exit - } + exit_for(Exit::failed(ErrorKind::Validation), bare_agent_hook) } } -/// Prints an error the way every handler does, and exits 0 as they always -/// have. A failure that is only printed is still a failed command. +/// Prints an error the way every handler does, unless the handler already +/// printed it, and fails the command. fn print_error(result: anyhow::Result<()>) -> Exit { match result { Ok(()) => Exit::ok(), Err(error) => { - eprintln!("{:?}", error); + if needs_printing(&error) { + eprintln!("{:?}", error); + } Exit::from(error) } } @@ -1540,16 +1533,22 @@ fn check_exit(failed: bool) -> Exit { } } -/// `agent` commands exit 1 when they fail, except the bare `agent hooks` -/// invocation: Claude Code, Codex and Cursor run it before tool calls and -/// read its exit code, so its codes stay as they were. -fn agent_failures_exit_1(entity_type: &EntityType) -> bool { - match entity_type { +/// The bare `agent hooks` invocation: Claude Code, Codex and Cursor run it +/// before tool calls and read its exit code, so its failures keep exiting 0. +fn is_bare_agent_hook(entity_type: &EntityType) -> bool { + matches!( + entity_type, EntityType::Agent(crate::cmd::agent::AgentCommand { subcommand: AgentSubcommand::Hooks(command), - }) => command.subcommand.is_some(), - EntityType::Agent(_) => true, - _ => false, + }) if command.subcommand.is_none() + ) +} + +fn exit_for(exit: Exit, bare_agent_hook: bool) -> Exit { + if bare_agent_hook { + exit.zero_on_failure() + } else { + exit } } @@ -2350,17 +2349,20 @@ fn spawn_remote_session_rotation( #[cfg(test)] mod tests { use super::{ - agent_failures_exit_1, audit_binding_sources, codex_mcp_binding_header_overrides, - configured_host_matches, dependency_hooks_enabled, directory_profile_git_warning, - ensure_replacement_session_is_compatible, infer_remote_agent_type, remote_bindings, - remote_codex_command_with_mcp_binding_headers, remote_session_rotation_delay_for, - remote_session_transport_identity, remote_source_env_names, secret_child_name, - summarize_audit_events, uses_local_dependency_hook_broker_mode, + audit_binding_sources, codex_mcp_binding_header_overrides, configured_host_matches, + dependency_hooks_enabled, directory_profile_git_warning, + ensure_replacement_session_is_compatible, exit_for, infer_remote_agent_type, + is_bare_agent_hook, remote_bindings, remote_codex_command_with_mcp_binding_headers, + remote_session_rotation_delay_for, remote_session_transport_identity, + remote_source_env_names, secret_child_name, summarize_audit_events, + uses_local_dependency_hook_broker_mode, }; use crate::api::remote_proxy::{RemoteBinding, RemoteBindingSource}; use crate::cmd::root::Cli; + use crate::exit::Exit; use crate::handlers::run::proxy::ProxyAuditLogEvent; use crate::models::agent::{AgentBindingProfile, AgentProfile, AgentSecretsProfile}; + use crate::telemetry::event::ErrorKind; use clap::Parser; use std::{ collections::HashMap, @@ -2431,32 +2433,21 @@ mod tests { } #[test] - fn agent_failures_exit_1_except_the_bare_hook_invocation() { + fn only_the_bare_hook_invocation_keeps_exit_0_on_failure() { let parse = |args: &[&str]| Cli::try_parse_from(args).unwrap().entity_type; - assert!(!agent_failures_exit_1(&parse(&[ - "stashbase", - "agent", - "hooks" - ]))); - assert!(agent_failures_exit_1(&parse(&[ - "stashbase", - "agent", - "hooks", - "deps", - "install", - "codex" - ]))); - assert!(agent_failures_exit_1(&parse(&[ - "stashbase", - "agent", - "run", - "--profile", - "p", - "--", - "true" - ]))); - assert!(!agent_failures_exit_1(&parse(&["stashbase", "pull"]))); + assert!(is_bare_agent_hook(&parse(&["stashbase", "agent", "hooks"]))); + for args in [ + &["stashbase", "agent", "hooks", "deps", "install", "codex"][..], + &["stashbase", "agent", "run", "--profile", "p", "--", "true"], + &["stashbase", "pull"], + ] { + assert!(!is_bare_agent_hook(&parse(args)), "{args:?}"); + } + + let failed = Exit::failed(ErrorKind::Validation); + assert_eq!(exit_for(failed, true).code, 0); + assert_eq!(exit_for(failed, false).code, 1); } #[test] diff --git a/src/handlers/pull/entry.rs b/src/handlers/pull/entry.rs index 2cd6de63..f4969165 100644 --- a/src/handlers/pull/entry.rs +++ b/src/handlers/pull/entry.rs @@ -11,6 +11,7 @@ use log::debug; use crate::{ api::secrets, cmd::{config::SecretsOutputFormat, pull::PullFormat}, + exit::ReportedFailure, handlers::run::entry::{ get_set_name_comment_pairs, get_set_name_value_pairs, validate_no_duplicate_set_names, }, @@ -24,6 +25,7 @@ use crate::{ PushPullInputValidationError, YamlEnvConfigError, }, }, + telemetry::event::ErrorKind, utils::{ interaction::{self, select}, output::{get_formatted_json_string, ColorizeIfColoredOutput}, @@ -525,7 +527,7 @@ pub async fn handle_pull(args: HandlePullArgs) -> Result<()> { eprintln!("{}", msg); } - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::NotFound)); } if only_len > 0 && secrets.len() < only_len { @@ -653,7 +655,7 @@ pub async fn handle_pull(args: HandlePullArgs) -> Result<()> { } } } else { - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::NotFound)); } } else { if !setted_secrets.is_empty() { diff --git a/src/handlers/run/entry.rs b/src/handlers/run/entry.rs index db8d542b..6f40c072 100644 --- a/src/handlers/run/entry.rs +++ b/src/handlers/run/entry.rs @@ -8,6 +8,7 @@ use tabled::Tabled; use crate::{ api::secrets, cmd::secrets::SecretsFileFormat, + exit::ReportedFailure, handlers::run::subprocess, models::{ api_client::{GetRequestApiResponse, OutputError}, @@ -19,6 +20,7 @@ use crate::{ SecretsInputValidationError, }, }, + telemetry::event::{classify_output_error, ErrorKind}, utils::{ env, interaction::{self}, @@ -1024,7 +1026,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { ); } - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::NotFound)); } if only_len > 0 && secrets.len() < only_len { @@ -1055,7 +1057,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { }; if confirmation != Some(true) { - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::NotFound)); } } @@ -1143,7 +1145,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { } else if !silent { eprintln!("{formatted_err}"); } - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::Validation)); } let res = secrets::pull( @@ -1159,6 +1161,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { if let Err(err) = res { debug!("Error: {:#?}", &err); + let kind = classify_output_error(&err); let formatted_err = err.format_error_output(json_format)?; if let Some(mut spinner) = spinner { @@ -1167,7 +1170,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { eprintln!("{}", formatted_err); } - return Ok(()); + return Err(ReportedFailure::new(kind)); } match res { @@ -1214,7 +1217,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { } } - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::NotFound)); } if only_len > 0 && secrets.len() < only_len { @@ -1287,7 +1290,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { ) .await?; } else { - return Ok(()); + return Err(ReportedFailure::new(ErrorKind::NotFound)); } } else { if !setted_secrets.is_empty() { diff --git a/src/telemetry/event.rs b/src/telemetry/event.rs index c718dc63..362e44bc 100644 --- a/src/telemetry/event.rs +++ b/src/telemetry/event.rs @@ -183,15 +183,19 @@ pub fn classify_error(error: &anyhow::Error) -> ErrorKind { return ErrorKind::Validation; } if let Some(output) = error.downcast_ref::() { - return match output.get_status() { - Some(401) | Some(403) => ErrorKind::Auth, - Some(404) => ErrorKind::NotFound, - _ => ErrorKind::Other, - }; + return classify_output_error(output); } ErrorKind::Other } +pub fn classify_output_error(error: &OutputError) -> ErrorKind { + match error.get_status() { + Some(401) | Some(403) => ErrorKind::Auth, + Some(404) => ErrorKind::NotFound, + _ => ErrorKind::Other, + } +} + pub struct Invocation { pub command: TrackedCommand, pub duration_ms: u64, diff --git a/tests/exit_status_cli.rs b/tests/exit_status_cli.rs index acb5eea7..ca7ed782 100644 --- a/tests/exit_status_cli.rs +++ b/tests/exit_status_cli.rs @@ -3,11 +3,11 @@ //! reports. They run the real binary with an isolated `HOME`, a scrubbed //! environment and an API URL nothing listens on. //! -//! They pin today's behaviour, including paths that print an error and still -//! exit 0. Changing one of those exit codes is a product decision, so a test -//! here must not change as a side effect of refactoring how the CLI exits. -//! `agent` commands are the exception: every failure exits non-zero, so a -//! profile that fails closed is not reported to scripts as a success. +//! A command that reports an error exits non-zero, so scripts and CI see the +//! failure. The one exception is the bare `agent hooks` invocation, which +//! coding agents run before tool calls and whose exit code they read: its +//! failures still exit 0. Changing an exit code is a product decision, so a +//! test here must not change as a side effect of refactoring how the CLI exits. //! //! The tests that apply the macOS sandbox or bind a loopback port skip //! themselves when the OS refuses (for example inside another sandbox). @@ -197,7 +197,7 @@ fn a_malformed_config_toml_fails_every_command_that_reads_it() { check( &args.join(" "), &project.run(args), - 0, + 1, message, Reported::Nothing, ); @@ -207,14 +207,14 @@ fn a_malformed_config_toml_fails_every_command_that_reads_it() { check( "pull", &project.run(&["pull", "--api-key", "k"]), - 0, + 1, message, failed, ); check( "setup", &project.run(&["setup"]), - 0, + 1, message, Reported::Event("error", Some("validation")), ); @@ -240,7 +240,7 @@ fn config_reset_still_works_when_the_config_is_malformed() { } #[test] -fn an_unknown_profile_environment_variable_fails_with_exit_0() { +fn an_unknown_profile_environment_variable_fails_with_exit_1() { let project = Project::new(); let out = project.run_with( @@ -251,19 +251,34 @@ fn an_unknown_profile_environment_variable_fails_with_exit_0() { check( "unknown profile", &out, - 0, + 1, "no-such-profile", Reported::Event("error", Some("validation")), ); } #[test] -fn a_config_command_on_an_unknown_profile_prints_the_error_and_exits_0() { +fn a_config_command_on_an_unknown_profile_prints_the_error_and_exits_1() { let project = Project::new(); let out = project.run(&["config", "profile", "use", "nosuch"]); - check("config", &out, 0, "was not found", Reported::Nothing); + check("config", &out, 1, "was not found", Reported::Nothing); +} + +#[test] +fn a_config_setter_that_fails_exits_1() { + let project = Project::new(); + + let out = project.run(&["config", "profile", "add", "default"]); + + check( + "config profile add default", + &out, + 1, + "'default' is the implicit profile", + Reported::Nothing, + ); } // ---- API key and project file -------------------------------------------- @@ -320,7 +335,7 @@ fn a_missing_api_key_in_json_mode_prints_a_json_error_and_exits_1() { } #[test] -fn a_missing_stashbase_yaml_prints_the_error_and_exits_0() { +fn a_missing_stashbase_yaml_prints_the_error_and_exits_1() { let project = Project::new(); let failed = || Reported::Event("error", Some("validation")); @@ -328,18 +343,15 @@ fn a_missing_stashbase_yaml_prints_the_error_and_exits_0() { check( command, &project.run(&[command, "--api-key", "k"]), - 0, + 1, "No 'stashbase.yaml' file found", failed(), ); } } -/// Today these print the error and return normally, and telemetry reports the -/// command as a success: a tracked failure that goes unreported. Only the exit -/// code and the message are pinned. #[test] -fn conflicting_scope_flags_print_the_error_and_exit_0() { +fn conflicting_scope_flags_print_the_error_and_exit_1() { let project = Project::new(); for (args, message) in [ @@ -369,17 +381,68 @@ fn conflicting_scope_flags_print_the_error_and_exit_0() { check( args[0], &project.run(args), - 0, + 1, message, Reported::NotAsserted, ); } } +#[test] +fn an_api_request_that_fails_exits_1() { + let project = Project::new(); + + let out = project.run(&[ + "secrets", + "list", + "-p", + "myproj", + "-e", + "dev", + "--api-key", + "k", + ]); + + check( + "secrets list", + &out, + 1, + "Could not connect to the API", + Reported::Nothing, + ); +} + +#[test] +fn run_without_any_secrets_does_not_start_the_command_and_exits_1() { + let project = Project::new(); + std::fs::write(project.cwd.join("empty.env"), "").unwrap(); + + let out = project.run(&[ + "run", + "--file", + "empty.env", + "--api-key", + "k", + "--", + "sh", + "-c", + "echo child-ran", + ]); + + check( + "run", + &out, + 1, + "No secrets found", + Reported::Event("error", Some("not_found")), + ); + assert!(!text(&out.stdout).contains("child-ran")); +} + // ---- setup, doctor, generate --------------------------------------------- #[test] -fn setup_without_a_terminal_prints_the_error_and_exits_0() { +fn setup_without_a_terminal_prints_the_error_and_exits_1() { let project = Project::new(); let out = project.run(&["setup"]); @@ -387,7 +450,7 @@ fn setup_without_a_terminal_prints_the_error_and_exits_0() { check( "setup", &out, - 0, + 1, "not a terminal", Reported::Event("error", Some("other")), ); @@ -418,7 +481,7 @@ fn doctor_exits_1_when_a_check_fails_and_0_otherwise() { } #[test] -fn generate_prints_its_error_and_exits_0() { +fn generate_prints_its_error_and_exits_1() { let project = Project::new(); // ssh-keygen is not on PATH, so key generation fails inside the handler. @@ -427,7 +490,7 @@ fn generate_prints_its_error_and_exits_0() { check( "generate", &out, - 0, + 1, "Failed to execute ssh-keygen", Reported::Nothing, ); @@ -457,6 +520,30 @@ fn agent_init_refuses_to_overwrite_and_exits_1() { ); } +/// Coding agents run the bare `agent hooks` before tool calls and read its +/// exit code, so its failures keep exiting 0. +#[test] +fn the_bare_agent_hook_still_exits_0_when_it_fails() { + use std::io::Write; + + let project = Project::new(); + let mut child = project + .command(&["agent", "hooks", "--api-key", "k"], DEAD_API) + .stdin(Stdio::piped()) + .spawn() + .unwrap(); + child.stdin.take().unwrap().write_all(b"not json").unwrap(); + let out = child.wait_with_output().unwrap(); + + check( + "agent hooks", + &out, + 0, + "Agent hook input was not valid JSON", + Reported::NotAsserted, + ); +} + // ---- agent run: failures before launch ------------------------------------- #[test] @@ -677,8 +764,8 @@ fn ctrl_c_during_a_request_exits_130_and_reports_aborted() { ); } -/// The same failures as `conflicting_scope_flags_print_the_error_and_exit_0`, -/// seen from telemetry: the exit code stays 0, but they are failed commands. +/// The same failures as `conflicting_scope_flags_print_the_error_and_exit_1`, +/// seen from telemetry: they are reported as validation failures. #[test] fn conflicting_scope_flags_are_reported_as_validation_failures() { let project = Project::new(); @@ -700,7 +787,7 @@ fn conflicting_scope_flags_are_reported_as_validation_failures() { ] { let out = project.run(args); let all = text(&out.stderr); - assert_eq!(out.status.code(), Some(0), "{}: {all}", args[0]); + assert_eq!(out.status.code(), Some(1), "{}: {all}", args[0]); let events = events(&out); assert_eq!(events.len(), 1, "{}: {all}", args[0]); assert_eq!(events[0]["outcome"], "error", "{}: {}", args[0], events[0]); From 28dbe433f6d800ee60b9aa2465675c86105b804a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radim=20H=C3=B6fer?= Date: Thu, 8 Oct 2026 09:18:44 +0200 Subject: [PATCH 2/2] fix(run): report a missing API key in agent run as an auth error `agent run` skips the root API-key guard so file-only profiles can run, and meets a missing key later, when it fetches remote secrets. That path recorded the failure as a validation error, while the root guard and the JSON output call it an authentication error. It now reports `auth`. Adds a telemetry test for that path, and corrects the handle_load_env_run doc comment, which still said its printed errors return Ok(()). --- src/handlers/run/entry.rs | 12 +++++++----- tests/telemetry_cli.rs | 28 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/handlers/run/entry.rs b/src/handlers/run/entry.rs index 6f40c072..ac084b5e 100644 --- a/src/handlers/run/entry.rs +++ b/src/handlers/run/entry.rs @@ -549,10 +549,11 @@ pub struct HandleRunArgs { pub local_session: Option, } -/// Several paths below print an error and return `Ok(())` (missing secrets, -/// API errors, a declined prompt). `handle_cli` sees that as success, so -/// telemetry reports `run` as `ok` there; for `agent run`, -/// `telemetry::never_launched_error` still catches a run that never launched. +/// Errors this prints itself (missing secrets, API errors, a declined prompt) +/// return `ReportedFailure`, so the command exits 1 without printing them twice. +/// The one early `Ok(())` is cancelling the `stashbase.yaml` config selection, +/// which exits 0; for `agent run`, `telemetry::never_launched_error` still +/// catches a run that never launched. pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { let HandleRunArgs { api_key, @@ -1145,7 +1146,8 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { } else if !silent { eprintln!("{formatted_err}"); } - return Err(ReportedFailure::new(ErrorKind::Validation)); + // Matches the root API-key guard: the CLI calls this an authentication error. + return Err(ReportedFailure::new(ErrorKind::Auth)); } let res = secrets::pull( diff --git a/tests/telemetry_cli.rs b/tests/telemetry_cli.rs index 204fb50f..e7a7afdb 100644 --- a/tests/telemetry_cli.rs +++ b/tests/telemetry_cli.rs @@ -536,6 +536,34 @@ fn a_missing_api_key_is_reported_as_an_auth_error() { assert_eq!(events[0]["error_kind"], "auth", "{}", events[0]); } +/// `agent run` skips the root API-key guard so file-only profiles can run, and +/// meets the missing key later, when it fetches the remote secrets. +#[test] +fn a_missing_api_key_in_agent_run_is_reported_as_an_auth_error() { + let sandbox = Sandbox::new(); + sandbox.enable(); + sandbox.write_profile( + "p", + "[secrets]\nproject = \"project\"\nenvironment = \"environment\"\n\n\ + [secrets.GH_TOKEN]\n\n\ + [[secrets.GH_TOKEN.rules]]\neffect = \"allow\"\nhosts = [\"api.github.com\"]\n\ + methods = [\"GET\"]\npaths = [\"*\"]\n", + ); + + let out = sandbox.run(&["agent", "run", "--profile", "p", "--", "sh", "-c", "exit 0"]); + // The proxy binds a loopback port before the key is needed. + if stderr(&out).contains("Operation not permitted") { + eprintln!("skipping: loopback ports are refused inside another sandbox"); + return; + } + + assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); + let events = events(&out); + assert_eq!(events.len(), 1, "{}", stderr(&out)); + assert_eq!(events[0]["outcome"], "error"); + assert_eq!(events[0]["error_kind"], "auth", "{}", events[0]); +} + /// `handle_cli` prints these two failures and returns normally (exit 0), so /// telemetry must record them itself or it would report a success. #[test]