fix: stderr/output hygiene rollup

.changeset/stderr-output-hygiene.md

@@ -0,0 +1,10 @@
+---
+"@googleworkspace/cli": patch
+---
+
+Stderr/output hygiene rollup: route diagnostics to stderr, add colored error labels, propagate auth errors.
+
+- **triage.rs**: "No messages found" sent to stderr so stdout stays valid JSON for pipes
+- **modelarmor.rs**: response body printed only on success; error message now includes body for diagnostics
+- **error.rs**: colored `error[variant]:` labels on stderr (respects `NO_COLOR` env var), `hint:` prefix for accessNotConfigured guidance
+- **calendar, chat, docs, drive, script, sheets**: auth failures now propagate as `GwsError::Auth` instead of silently proceeding unauthenticated (dry-run still works without auth)

src/credential_store.rs

@@ -14,6 +14,8 @@
 
 use std::path::PathBuf;
 
+use crate::error::sanitize_for_terminal;
+
 use aes_gcm::aead::{Aead, KeyInit, OsRng};
 use aes_gcm::{AeadCore, Aes256Gcm, Nonce};
 
@@ -31,7 +33,10 @@ fn ensure_key_dir(path: &std::path::Path) -> std::io::Result<()> {
             use std::os::unix::fs::PermissionsExt;
             if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700))
             {
-                eprintln!("Warning: failed to set secure permissions on key directory: {e}");
+                eprintln!(
+                    "Warning: failed to set secure permissions on key directory: {}",
+                    sanitize_for_terminal(&e.to_string())
+                );
             }
         }
     }
@@ -251,7 +256,10 @@ fn resolve_key(
                 }
             }
             Err(e) => {
-                eprintln!("Warning: keyring access failed, falling back to file storage: {e}");
+                eprintln!(
+                    "Warning: keyring access failed, falling back to file storage: {}",
+                    sanitize_for_terminal(&e.to_string())
+                );
             }
         }
     }

src/error.rs

@@ -148,34 +148,89 @@ impl GwsError {
     }
 }
 
+/// Returns true when stderr is connected to an interactive terminal,
+/// meaning ANSI color codes will be visible to the user.
+fn stderr_supports_color() -> bool {
+    use std::io::IsTerminal;
+    std::io::stderr().is_terminal() && std::env::var_os("NO_COLOR").is_none()
+}
+
+/// Wrap `text` in ANSI bold + the given color code, resetting afterwards.
+/// Returns the plain text unchanged when stderr is not a TTY.
+fn colorize(text: &str, ansi_color: &str) -> String {
+    if stderr_supports_color() {
+        format!("\x1b[1;{ansi_color}m{text}\x1b[0m")
+    } else {
+        text.to_string()
+    }
+}
+
+/// Strip terminal control characters from `text` to prevent escape-sequence
+/// injection when printing untrusted content (API responses, user input) to
+/// stderr.  Preserves newlines and tabs for readability.
+pub(crate) fn sanitize_for_terminal(text: &str) -> String {
+    text.chars()
+        .filter(|&c| {
+            if c == '\n' || c == '\t' {
+                return true;
+            }
+            !c.is_control()
+        })
+        .collect()
+}
+
+/// Format a colored error label for the given error variant.
+fn error_label(err: &GwsError) -> String {
+    match err {
+        GwsError::Api { .. } => colorize("error[api]:", "31"), // red
+        GwsError::Auth(_) => colorize("error[auth]:", "31"),   // red
+        GwsError::Validation(_) => colorize("error[validation]:", "33"), // yellow
+        GwsError::Discovery(_) => colorize("error[discovery]:", "31"), // red
+        GwsError::Other(_) => colorize("error:", "31"),        // red
+    }
+}
+
 /// Formats any error as a JSON object and prints to stdout.
 ///
-/// For `accessNotConfigured` errors (HTTP 403, reason `accessNotConfigured`),
-/// additional human-readable guidance is printed to stderr explaining how to
-/// enable the API in GCP. The JSON output on stdout is unchanged (machine-readable).
+/// A human-readable colored label is printed to stderr when connected to a
+/// TTY. For `accessNotConfigured` errors (HTTP 403, reason
+/// `accessNotConfigured`), additional guidance is printed to stderr.
+/// The JSON output on stdout is unchanged (machine-readable).
 pub fn print_error_json(err: &GwsError) {
     let json = err.to_json();
     println!(
         "{}",
         serde_json::to_string_pretty(&json).unwrap_or_default()
     );
 
-    // Print actionable guidance to stderr for accessNotConfigured errors
+    // Print a colored summary to stderr. For accessNotConfigured errors,
+    // print specialized guidance instead of the generic message to avoid
+    // redundant output (the full API error already appears in the JSON).
     if let GwsError::Api {
         reason, enable_url, ..
     } = err
     {
         if reason == "accessNotConfigured" {
             eprintln!();
-            eprintln!("💡 API not enabled for your GCP project.");
+            let hint = colorize("hint:", "36"); // cyan
+            eprintln!(
+                "{} {hint} API not enabled for your GCP project.",
+                error_label(err)
+            );
             if let Some(url) = enable_url {
-                eprintln!("   Enable it at: {url}");
+                eprintln!("      Enable it at: {url}");
             } else {
-                eprintln!("   Visit the GCP Console → APIs & Services → Library to enable the required API.");
+                eprintln!("      Visit the GCP Console → APIs & Services → Library to enable the required API.");
             }
-            eprintln!("   After enabling, wait a few seconds and retry your command.");
+            eprintln!("      After enabling, wait a few seconds and retry your command.");
+            return;
         }
     }
+    eprintln!(
+        "{} {}",
+        error_label(err),
+        sanitize_for_terminal(&err.to_string())
+    );
 }
 
 #[cfg(test)]
@@ -327,4 +382,58 @@ mod tests {
         // enable_url key should not appear in JSON when None
         assert!(json["error"]["enable_url"].is_null());
     }
+
+    // --- colored output tests ---
+
+    #[test]
+    #[serial_test::serial]
+    fn test_colorize_respects_no_color_env() {
+        // NO_COLOR is the de-facto standard for disabling colors.
+        // When set, colorize() should return the plain text.
+        std::env::set_var("NO_COLOR", "1");
+        let result = colorize("hello", "31");
+        std::env::remove_var("NO_COLOR");
+        assert_eq!(result, "hello");
+    }
+
+    #[test]
+    fn test_error_label_contains_variant_name() {
+        let api_err = GwsError::Api {
+            code: 400,
+            message: "bad".to_string(),
+            reason: "r".to_string(),
+            enable_url: None,
+        };
+        let label = error_label(&api_err);
+        assert!(label.contains("error[api]:"));
+
+        let auth_err = GwsError::Auth("fail".to_string());
+        assert!(error_label(&auth_err).contains("error[auth]:"));
+
+        let val_err = GwsError::Validation("bad input".to_string());
+        assert!(error_label(&val_err).contains("error[validation]:"));
+
+        let disc_err = GwsError::Discovery("missing".to_string());
+        assert!(error_label(&disc_err).contains("error[discovery]:"));
+
+        let other_err = GwsError::Other(anyhow::anyhow!("oops"));
+        assert!(error_label(&other_err).contains("error:"));
+    }
+
+    #[test]
+    fn test_sanitize_for_terminal_strips_control_chars() {
+        // ANSI escape sequence should be stripped
+        let input = "normal \x1b[31mred text\x1b[0m end";
+        let sanitized = sanitize_for_terminal(input);
+        assert_eq!(sanitized, "normal [31mred text[0m end");
+        assert!(!sanitized.contains('\x1b'));
+
+        // Newlines and tabs preserved
+        let input2 = "line1\nline2\ttab";
+        assert_eq!(sanitize_for_terminal(input2), "line1\nline2\ttab");
+
+        // Other control characters stripped
+        let input3 = "hello\x07bell\x08backspace";
+        assert_eq!(sanitize_for_terminal(input3), "hellobellbackspace");
+    }
 }

src/executor.rs

@@ -28,7 +28,7 @@ use serde_json::{json, Map, Value};
 use tokio::io::AsyncWriteExt;
 
 use crate::discovery::{RestDescription, RestMethod};
-use crate::error::GwsError;
+use crate::error::{sanitize_for_terminal, GwsError};
 
 /// Tracks what authentication method was used for the request.
 #[derive(Debug, Clone, PartialEq)]
@@ -261,7 +261,10 @@ async fn handle_json_response(
                     }
                 }
                 Err(e) => {
-                    eprintln!("⚠️  Model Armor sanitization failed: {e}");
+                    eprintln!(
+                        "⚠️  Model Armor sanitization failed: {}",
+                        sanitize_for_terminal(&e.to_string())
+                    );
                 }
             }
         }

src/generate_skills.rs

@@ -18,7 +18,7 @@
 
 use crate::commands;
 use crate::discovery;
-use crate::error::GwsError;
+use crate::error::{sanitize_for_terminal, GwsError};
 use crate::services;
 use clap::Command;
 use std::path::Path;
@@ -123,7 +123,10 @@ pub async fn handle_generate_skills(args: &[String]) -> Result<(), GwsError> {
             match discovery::fetch_discovery_document(entry.api_name, entry.version).await {
                 Ok(d) => d,
                 Err(e) => {
-                    eprintln!("  WARNING: Failed to fetch discovery doc for {alias}: {e}");
+                    eprintln!(
+                        "  WARNING: Failed to fetch discovery doc for {alias}: {}",
+                        sanitize_for_terminal(&e.to_string())
+                    );
                     continue;
                 }
             }

src/helpers/calendar.rs

@@ -167,7 +167,8 @@ TIPS:
                 let scopes_str: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
                 let (token, auth_method) = match auth::get_token(&scopes_str).await {
                     Ok(t) => (Some(t), executor::AuthMethod::OAuth),
-                    Err(_) => (None, executor::AuthMethod::None),
+                    Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
+                    Err(e) => return Err(GwsError::Auth(format!("Calendar auth failed: {e}"))),
                 };
 
                 let events_res = doc.resources.get("events").ok_or_else(|| {

src/helpers/chat.rs

@@ -80,7 +80,8 @@ TIPS:
                 let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
                 let (token, auth_method) = match auth::get_token(&scope_strs).await {
                     Ok(t) => (Some(t), executor::AuthMethod::OAuth),
-                    Err(_) => (None, executor::AuthMethod::None),
+                    Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
+                    Err(e) => return Err(GwsError::Auth(format!("Chat auth failed: {e}"))),
                 };
 
                 // Method: spaces.messages.create

src/helpers/docs.rs

@@ -72,7 +72,8 @@ TIPS:
                 let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
                 let (token, auth_method) = match auth::get_token(&scope_strs).await {
                     Ok(t) => (Some(t), executor::AuthMethod::OAuth),
-                    Err(_) => (None, executor::AuthMethod::None),
+                    Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
+                    Err(e) => return Err(GwsError::Auth(format!("Docs auth failed: {e}"))),
                 };
 
                 // Method: documents.batchUpdate

src/helpers/drive.rs

@@ -98,7 +98,8 @@ TIPS:
                 let scopes: Vec<&str> = create_method.scopes.iter().map(|s| s.as_str()).collect();
                 let (token, auth_method) = match auth::get_token(&scopes).await {
                     Ok(t) => (Some(t), executor::AuthMethod::OAuth),
-                    Err(_) => (None, executor::AuthMethod::None),
+                    Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
+                    Err(e) => return Err(GwsError::Auth(format!("Drive auth failed: {e}"))),
                 };
 
                 executor::execute_method(

src/helpers/events/subscribe.rs

@@ -1,5 +1,6 @@
 use super::*;
 use crate::auth::AccessTokenProvider;
+use crate::error::sanitize_for_terminal;
 use crate::helpers::PUBSUB_API_BASE;
 use std::path::PathBuf;
 
@@ -375,7 +376,11 @@ async fn pull_loop(
                     .unwrap_or(0);
                 let path = dir.join(format!("{ts}_{file_counter}.json"));
                 if let Err(e) = std::fs::write(&path, &json_str) {
-                    eprintln!("Warning: failed to write {}: {e}", path.display());
+                    eprintln!(
+                        "Warning: failed to write {}: {}",
+                        path.display(),
+                        sanitize_for_terminal(&e.to_string())
+                    );
                 } else {
                     eprintln!("Wrote {}", path.display());
                 }

src/helpers/gmail/mod.rs

@@ -26,6 +26,7 @@ use triage::handle_triage;
 use watch::handle_watch;
 
 pub(super) use crate::auth;
+use crate::error::sanitize_for_terminal;
 pub(super) use crate::error::GwsError;
 pub(super) use crate::executor;
 pub(super) use anyhow::Context;
@@ -223,11 +224,17 @@ fn extract_body_by_mime(payload: &Value, target_mime: &str) -> Option<String> {
                 Ok(decoded) => match String::from_utf8(decoded) {
                     Ok(s) => return Some(s),
                     Err(e) => {
-                        eprintln!("Warning: {target_mime} body is not valid UTF-8: {e}");
+                        eprintln!(
+                            "Warning: {target_mime} body is not valid UTF-8: {}",
+                            sanitize_for_terminal(&e.to_string())
+                        );
                     }
                 },
                 Err(e) => {
-                    eprintln!("Warning: {target_mime} body has invalid base64: {e}");
+                    eprintln!(
+                        "Warning: {target_mime} body has invalid base64: {}",
+                        sanitize_for_terminal(&e.to_string())
+                    );
                 }
             }
         }
@@ -389,7 +396,10 @@ pub(super) fn format_date_for_attribution(raw_date: &str) -> String {
     chrono::DateTime::parse_from_rfc2822(raw_date)
         .map(|dt| dt.format("%a, %b %-d, %Y at %-I:%M\u{202f}%p").to_string())
         .unwrap_or_else(|e| {
-            eprintln!("Note: could not parse date as RFC 2822 ({e}); using raw value.");
+            eprintln!(
+                "Note: could not parse date as RFC 2822 ({}); using raw value.",
+                sanitize_for_terminal(&e.to_string())
+            );
             html_escape(raw_date)
         })
 }

src/helpers/gmail/triage.rs

@@ -76,13 +76,13 @@ pub async fn handle_triage(matches: &ArgMatches) -> Result<(), GwsError> {
     let messages = match list_json.get("messages").and_then(|m| m.as_array()) {
         Some(m) => m,
         None => {
-            println!("No messages found matching query: {query}");
+            eprintln!("{}", no_messages_msg(query));
             return Ok(());
         }
     };
 
     if messages.is_empty() {
-        println!("No messages found matching query: {query}");
+        eprintln!("{}", no_messages_msg(query));
         return Ok(());
     }
 
@@ -178,8 +178,15 @@ pub async fn handle_triage(matches: &ArgMatches) -> Result<(), GwsError> {
     Ok(())
 }
 
+/// Returns the human-readable "no messages" diagnostic string.
+/// Extracted so the test can reference the exact same message without duplication.
+fn no_messages_msg(query: &str) -> String {
+    format!("No messages found matching query: {query}")
+}
+
 #[cfg(test)]
 mod tests {
+    use super::no_messages_msg;
     use clap::{Arg, ArgAction, Command};
 
     /// Build a clap command matching the +triage definition so we can
@@ -283,4 +290,13 @@ mod tests {
             .unwrap_or(crate::formatter::OutputFormat::Table);
         assert!(matches!(fmt, crate::formatter::OutputFormat::Json));
     }
+
+    #[test]
+    fn empty_result_message_is_not_json() {
+        // Verify that no_messages_msg() produces a human-readable string that
+        // belongs on stderr, not stdout. If it were valid JSON it could corrupt
+        // pipe workflows like `gws gmail +triage | jq`.
+        let msg = no_messages_msg("label:inbox");
+        assert!(serde_json::from_str::<serde_json::Value>(&msg).is_err());
+    }
 }