fix: exit non-zero whenever a command reports an error

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<anyhow::Error> 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::<ReportedFailure>() {
+            return Self::failed(reported.kind);
+        }
         let code = error
             .downcast_ref::<CommandFailed>()
-            .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::<ReportedFailure>().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]

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<String, &'static str> {
     }
 }
 
-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<String, &'static str> {

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<bool>) {
+use crate::{
+    config::config, exit::ReportedFailure, models::config::UpdateConfig,
+    telemetry::event::ErrorKind, utils::output::ColorizeIfColoredOutput,
+};
+
+pub fn set_expand_refs_config(enabled: Option<bool>) -> 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<bool>) {
 
     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<bool>) {

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) {

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) {