diff --git a/cli/src/app.rs b/cli/src/app.rs index dff2b8b22..5e931c052 100644 --- a/cli/src/app.rs +++ b/cli/src/app.rs @@ -4,7 +4,7 @@ use std::process::ExitCode; use crate::services; use services::app_support::{self, RunOutcome}; -use services::error::ClassifiedError; +use services::error::CliError; use services::observability::traits::{ Logger as LoggerTrait, NoopTelemetry, Telemetry as TelemetryTrait, }; @@ -310,17 +310,19 @@ where fn perform_dependency_check anyhow::Result<()>>( dependency_check: F, -) -> Result<(), ClassifiedError> { +) -> Result<(), CliError> { dependency_check().map_err(|error| { - ClassifiedError::dependency(format!("Failed to initialize dependency checks: {error}")) + CliError::dependency(anyhow::Error::msg(format!( + "Failed to initialize dependency checks: {error}" + ))) }) } -fn build_startup_context() -> Result { +fn build_startup_context() -> Result { let cwd = std::env::current_dir().map_err(|error| { - ClassifiedError::runtime(format!( + CliError::runtime(anyhow::Error::msg(format!( "Failed to determine current directory for observability config resolution: {error}" - )) + ))) })?; let observability_config = services::config::resolve_observability_runtime_config(&cwd) .map_err(|error| app_support::classify_observability_configuration_error(&error))?; @@ -332,7 +334,7 @@ fn build_startup_context() -> Result { }) } -fn initialize_runtime(startup: StartupContext) -> Result { +fn initialize_runtime(startup: StartupContext) -> Result { let logger = services::observability::Logger::from_resolved_config(&startup.observability_config) .map_err(|error| app_support::classify_observability_configuration_error(&error))?; @@ -351,7 +353,7 @@ fn run_command_lifecycle( args: I, runtime: &AppRuntime, stderr: &mut StderrW, -) -> Result +) -> Result where I: IntoIterator, StderrW: Write, @@ -366,7 +368,9 @@ where None, ); let Some(command_args) = args.take() else { - return Err(ClassifiedError::runtime(REPEATED_COMMAND_DISPATCH_ERROR)); + return Err(CliError::runtime(anyhow::Error::msg( + REPEATED_COMMAND_DISPATCH_ERROR, + ))); }; let command = parse_command_phase(command_args, &runtime.registry, &context)?; app_support::execute_command_phase(&command, &context, stderr) @@ -377,7 +381,7 @@ fn parse_command_phase( args: I, registry: &services::command_registry::CommandRegistry, context: &impl HasLogger, -) -> Result +) -> Result where I: IntoIterator, { diff --git a/cli/src/services/agent_trace_sync/control_plane.rs b/cli/src/services/agent_trace_sync/control_plane.rs index b15c69633..ab994d823 100644 --- a/cli/src/services/agent_trace_sync/control_plane.rs +++ b/cli/src/services/agent_trace_sync/control_plane.rs @@ -114,7 +114,7 @@ const STATE_RETRY_INITIAL_BACKOFF_MS: u64 = 250; const STATE_RETRY_MAX_BACKOFF_MS: u64 = 2_000; /// Typed failure classification for control-plane HTTP interactions, kept -/// separate from `ClassifiedError` so the sync engine and CLI wiring can +/// separate from `CliError` so the sync engine and CLI wiring can /// react to each case before deciding how to surface it. #[derive(Debug)] pub enum ControlPlaneError { @@ -172,6 +172,20 @@ impl fmt::Display for ControlPlaneError { impl std::error::Error for ControlPlaneError {} +impl ControlPlaneError { + /// True only for the two variants that mean the caller has no usable + /// `WorkOS` credentials: no stored token, or a token the control plane + /// rejected as invalid/expired. Every other variant is a different kind + /// of failure (request shape, ownership, transport, server-side) and + /// must not be classified as an authentication failure. + pub fn is_authentication_failure(&self) -> bool { + matches!( + self, + Self::MissingCredentials | Self::AuthenticationFailed(_) + ) + } +} + impl From for ControlPlaneError { fn from(value: TokenStorageError) -> Self { Self::Storage(value.to_string()) diff --git a/cli/src/services/agent_trace_sync/mod.rs b/cli/src/services/agent_trace_sync/mod.rs index c27f55c92..7be8c6851 100644 --- a/cli/src/services/agent_trace_sync/mod.rs +++ b/cli/src/services/agent_trace_sync/mod.rs @@ -14,6 +14,7 @@ use crate::services::agent_trace_export::{ AgentTraceAgentTraceExportRow, AgentTraceDiffTraceExportRow, AgentTraceMessageExportRow, AgentTracePartExportRow, }; +use control_plane::ControlPlaneError; /// Bound on consecutive `409`/ambiguous-batch-failure reconciliation attempts /// for one stream, matching the order of magnitude of existing retry @@ -58,7 +59,7 @@ impl AgentTraceExportRow for AgentTraceAgentTraceExportRow { /// anything about the failed attempt itself. `Terminal` is different: the /// attempt is known to have failed in a way that cannot be resolved by /// `/state`, so the stream stops without invoking its refresh closure. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug)] pub enum BatchAttemptOutcome { /// The batch was accepted. `accepted` and `cursor` are the server /// response's own fields, validated by the engine before the stream @@ -69,9 +70,10 @@ pub enum BatchAttemptOutcome { /// The batch outcome could not be determined (`5xx`, a transport /// failure, or an invalid response). Ambiguous, - /// The batch failed with a terminal control-plane error. The string is - /// already safe to surface as a stream error and is never reconciled. - Terminal(String), + /// The batch failed with a terminal control-plane error. The typed + /// error is already safe to surface as a stream error and is never + /// reconciled. + Terminal(ControlPlaneError), } /// Terminal failure of [`sync_stream`]. @@ -80,14 +82,14 @@ pub enum StreamSyncError { /// The local-row reader closure failed. Read(String), /// The `/state`-refresh closure failed. - Refresh(String), + Refresh(ControlPlaneError), /// A syntactically successful batch response did not match the rows /// that were sent (`accepted != rows.len()` or /// `cursor != rows.last().source_row_id()`). InvalidResponse(String), /// The batch failed with a terminal control-plane error. Unlike /// [`Self::Refresh`], this does not represent a failed `/state` call. - Terminal(String), + Terminal(ControlPlaneError), /// The reconciliation loop exceeded [`RECONCILIATION_MAX_ATTEMPTS`] /// without converging. DidNotConverge, @@ -112,6 +114,19 @@ impl fmt::Display for StreamSyncError { impl std::error::Error for StreamSyncError {} +impl StreamSyncError { + /// True only when the underlying `ControlPlaneError` (from a `Refresh` + /// or `Terminal` failure) means the caller has no usable credentials. + /// `Read`, `InvalidResponse`, and `DidNotConverge` never carry a + /// `ControlPlaneError` and are never authentication failures. + pub fn is_authentication_failure(&self) -> bool { + match self { + Self::Refresh(error) | Self::Terminal(error) => error.is_authentication_failure(), + Self::Read(_) | Self::InvalidResponse(_) | Self::DidNotConverge => false, + } + } +} + /// Outcome of a fully converged [`sync_stream`] run for one stream. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct StreamSyncOutcome { @@ -437,7 +452,7 @@ mod tests { |cursor, limit| ready(Ok(local.after(cursor, limit))), |_cursor, _rows: &[AgentTraceMessageExportRow]| { ready(BatchAttemptOutcome::Terminal( - "batch route is not supported".to_string(), + ControlPlaneError::BadRequest("batch route is not supported".to_string()), )) }, || { @@ -448,7 +463,7 @@ mod tests { assert!(matches!( result, - Err(StreamSyncError::Terminal(reason)) if reason == "batch route is not supported" + Err(StreamSyncError::Terminal(ControlPlaneError::BadRequest(reason))) if reason == "batch route is not supported" )); assert_eq!(*refresh_calls.borrow(), 0); } diff --git a/cli/src/services/app_support.rs b/cli/src/services/app_support.rs index cf9c20879..14671d7ec 100644 --- a/cli/src/services/app_support.rs +++ b/cli/src/services/app_support.rs @@ -4,7 +4,7 @@ use std::process::ExitCode; use crate::app::{ContextWithRepoRoot, HasLogger}; use crate::services; use services::command_registry::RuntimeCommand; -use services::error::ClassifiedError; +use services::error::CliError; use services::observability::traits::Logger as LoggerTrait; const INVALID_CONFIG_WARNING_EVENT_ID: &str = "sce.config.invalid_config"; @@ -13,7 +13,7 @@ pub(crate) struct RunOutcome where L: LoggerTrait, { - pub(crate) result: Result, + pub(crate) result: Result, pub(crate) logger: Option, pub(crate) startup_diagnostic: Option, } @@ -46,8 +46,8 @@ where } } -pub(crate) fn classify_observability_configuration_error(error: &anyhow::Error) -> ClassifiedError { - ClassifiedError::validation(format!("Invalid observability configuration: {error}")) +pub(crate) fn classify_observability_configuration_error(error: &anyhow::Error) -> CliError { + CliError::validation(format!("Invalid observability configuration: {error}")) } pub(crate) fn invalid_discovered_config_guidance( @@ -105,7 +105,7 @@ pub(crate) fn execute_command_phase( command: &RuntimeCommand, context: &C, stderr: &mut W, -) -> Result +) -> Result where C: HasLogger + ContextWithRepoRoot, W: Write, @@ -137,44 +137,64 @@ where }) } -fn exit_with_error(stderr: &mut W, logger: Option<&L>, error: &ClassifiedError) -> ExitCode +fn exit_with_error(stderr: &mut W, logger: Option<&L>, error: &CliError) -> ExitCode where L: LoggerTrait, W: Write, { if let Some(log) = logger { - log.log_classified_error(error, None); + log.log_cli_error(error, None); } write_error_diagnostic(stderr, error); ExitCode::from(error.class().exit_code()) } -fn write_stdout_payload(writer: &mut W, payload: &str) -> Result<(), ClassifiedError> { +fn write_stdout_payload(writer: &mut W, payload: &str) -> Result<(), CliError> { if payload.is_empty() { return Ok(()); } writeln!(writer, "{payload}").map_err(|error| { - ClassifiedError::runtime(format!("Failed to write command output to stdout: {error}")) + CliError::runtime(anyhow::Error::msg(format!( + "Failed to write command output to stdout: {error}" + ))) }) } -fn write_error_diagnostic(writer: &mut W, error: &ClassifiedError) { - let rendered = if error.message().contains("Try:") { - error.message().to_string() - } else { - format!( - "{} Try: {}", - error.message(), - error.class().default_try_guidance() - ) +fn write_error_diagnostic(writer: &mut W, error: &CliError) { + write_error_diagnostic_with_color_policy( + writer, + error, + services::style::supports_color_stderr(), + ); +} + +fn write_error_diagnostic_with_color_policy( + writer: &mut W, + error: &CliError, + color_enabled: bool, +) { + let rendered = match error { + CliError::Internal { source, .. } => { + let message = format!("{source:#}"); + if message.contains("Try:") { + message + } else { + format!("{message} Try: {}", error.class().default_try_guidance()) + } + } + CliError::User { + error: user_error, .. + } => user_error.message().to_string(), }; - let styled_message = - services::style::error_text(&services::security::redact_sensitive_text(&rendered)); + let styled_message = services::style::error_text_with_color_policy( + &services::security::redact_sensitive_text(&rendered), + color_enabled, + ); writeln!( writer, "{} [{}]: {}", - services::style::heading("Error"), - services::style::error_code(error.code()), + services::style::heading_with_color_policy("Error", color_enabled), + services::style::error_code_with_color_policy(error.code(), color_enabled), styled_message ) .expect("writing error diagnostic to writer should not fail"); @@ -184,3 +204,152 @@ fn write_startup_diagnostic(writer: &mut W, diagnostic: &str) { writeln!(writer, "{}", services::style::error_code(diagnostic)) .expect("writing startup diagnostic to writer should not fail"); } + +#[cfg(test)] +mod tests { + use super::*; + use services::error::UserError; + use std::sync::{Arc, Mutex}; + + #[derive(Clone, Default)] + struct RecordingLogger { + log_cli_error_calls: Arc>>, + } + + impl LoggerTrait for RecordingLogger { + fn info(&self, _: &str, _: &str, _: &[(&str, &str)], _: Option<&str>) {} + fn debug(&self, _: &str, _: &str, _: &[(&str, &str)], _: Option<&str>) {} + fn warn(&self, _: &str, _: &str, _: &[(&str, &str)], _: Option<&str>) {} + fn error(&self, _: &str, _: &str, _: &[(&str, &str)], _: Option<&str>) {} + + fn log_cli_error(&self, error: &CliError, _session_id: Option<&str>) { + let has_source = match error { + CliError::User { source, .. } => source.is_some(), + CliError::Internal { .. } => true, + }; + self.log_cli_error_calls + .lock() + .expect("recording logger mutex must not be poisoned") + .push((error.code(), has_source)); + } + } + + fn diagnostic_lines(stderr: &str) -> Vec<&str> { + stderr + .lines() + .filter(|line| line.contains("Error") && line.contains("SCE-ERR-")) + .collect() + } + + #[test] + fn user_error_routes_to_friendly_diagnostic_with_empty_stdout_and_exit_four() { + let error = CliError::user_with_source( + UserError::NotAuthenticated, + anyhow::anyhow!("missing credentials"), + ); + let logger = RecordingLogger::default(); + let outcome = RunOutcome { + result: Err(error), + logger: Some(logger), + startup_diagnostic: None, + }; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let exit_code = render_run_outcome(outcome, &mut stdout, &mut stderr); + + assert!(stdout.is_empty(), "stdout must stay empty on failure"); + assert_eq!(exit_code, ExitCode::from(4)); + + let stderr_text = String::from_utf8(stderr).expect("stderr is valid utf8"); + assert_eq!( + diagnostic_lines(&stderr_text).len(), + 1, + "exactly one terminal diagnostic must be written" + ); + assert!(stderr_text.contains("You are not logged in")); + assert!(!stderr_text.contains("missing credentials")); + assert!(!stderr_text.to_lowercase().contains("control-plane")); + } + + #[test] + fn user_error_preserves_technical_source_for_observability() { + let error = CliError::user_with_source( + UserError::NotAuthenticated, + anyhow::anyhow!("missing credentials"), + ); + let logger = RecordingLogger::default(); + let calls = logger.log_cli_error_calls.clone(); + let outcome = RunOutcome { + result: Err(error), + logger: Some(logger), + startup_diagnostic: None, + }; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + render_run_outcome(outcome, &mut stdout, &mut stderr); + + let recorded = calls.lock().expect("mutex must not be poisoned"); + assert_eq!( + recorded.as_slice(), + [("SCE-ERR-RUNTIME", true)], + "observability must log the CliError with its technical source preserved" + ); + } + + #[test] + fn internal_error_still_renders_full_chain_and_exit_four() { + let source = anyhow::anyhow!("root cause").context("failed to sync"); + let error = CliError::runtime(source); + let outcome: RunOutcome = RunOutcome { + result: Err(error), + logger: None, + startup_diagnostic: None, + }; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let exit_code = render_run_outcome(outcome, &mut stdout, &mut stderr); + + assert!(stdout.is_empty()); + assert_eq!(exit_code, ExitCode::from(4)); + let stderr_text = String::from_utf8(stderr).expect("stderr is valid utf8"); + assert_eq!(diagnostic_lines(&stderr_text).len(), 1); + assert!(stderr_text.contains("failed to sync")); + assert!(stderr_text.contains("root cause")); + } + + #[test] + fn redaction_still_applies_to_the_rendered_message() { + let mut stderr = Vec::new(); + let error = CliError::user(UserError::NotAuthenticated); + write_error_diagnostic_with_color_policy(&mut stderr, &error, false); + + let rendered = String::from_utf8(stderr).expect("stderr is valid utf8"); + let redacted_message = + services::security::redact_sensitive_text(UserError::NotAuthenticated.message()); + assert!(rendered.contains(&redacted_message)); + } + + #[test] + fn user_error_diagnostic_is_styled_only_when_color_is_enabled() { + let error = CliError::user(UserError::NotAuthenticated); + + let mut colored = Vec::new(); + write_error_diagnostic_with_color_policy(&mut colored, &error, true); + let colored_text = String::from_utf8(colored).expect("stderr is valid utf8"); + + let mut plain = Vec::new(); + write_error_diagnostic_with_color_policy(&mut plain, &error, false); + let plain_text = String::from_utf8(plain).expect("stderr is valid utf8"); + + // TTY-following (color_enabled: true) and redirected/NO_COLOR + // (color_enabled: false) diverge: only the enabled case carries ANSI. + assert_ne!(colored_text, plain_text); + assert!(!plain_text.contains('\u{1b}')); + assert!(colored_text.contains('\u{1b}')); + assert!(plain_text.contains("You are not logged in")); + assert!(colored_text.contains("You are not logged in")); + } +} diff --git a/cli/src/services/auth_command/command.rs b/cli/src/services/auth_command/command.rs index 356c379bb..9c5abadb1 100644 --- a/cli/src/services/auth_command/command.rs +++ b/cli/src/services/auth_command/command.rs @@ -1,13 +1,12 @@ use crate::services::auth_command; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; pub struct AuthCommand { pub request: auth_command::AuthRequest, } impl AuthCommand { - pub fn execute(&self, _context: &C) -> Result { - auth_command::run_auth_subcommand(self.request) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}"))) + pub fn execute(&self, _context: &C) -> Result { + auth_command::run_auth_subcommand(self.request).map_err(CliError::runtime) } } diff --git a/cli/src/services/bash_policy.rs b/cli/src/services/bash_policy.rs index df38c1fba..c0712bd1a 100644 --- a/cli/src/services/bash_policy.rs +++ b/cli/src/services/bash_policy.rs @@ -9,18 +9,18 @@ use crate::services::config; use crate::services::config::policy::{ runtime_bash_policy_presets, BashPolicyConfig, CustomBashPolicyEntry, RuntimeBashPolicyPreset, }; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; pub mod command { use crate::services::bash_policy; - use crate::services::error::ClassifiedError; + use crate::services::error::CliError; pub struct PolicyCommand { pub request: bash_policy::BashPolicyRequest, } impl PolicyCommand { - pub fn execute(&self) -> Result { + pub fn execute(&self) -> Result { bash_policy::run_bash_policy_request(&self.request) } } @@ -455,7 +455,7 @@ struct JsonPolicyResult<'a> { policy_id: Option<&'a str>, } -pub fn run_bash_policy_request(request: &BashPolicyRequest) -> Result { +pub fn run_bash_policy_request(request: &BashPolicyRequest) -> Result { let stdin_payload = read_stdin_payload()?; run_bash_policy_request_from_payload(request, &stdin_payload) } @@ -463,29 +463,29 @@ pub fn run_bash_policy_request(request: &BashPolicyRequest) -> Result Result { +) -> Result { let command = parse_command_from_stdin(request.input, stdin_payload)?; let cwd = resolved_policy_project_root()?; let policy_config = config::resolve_bash_policy_runtime_config(&cwd).map_err(|error| { - ClassifiedError::runtime(format!( + CliError::runtime(anyhow::Error::msg(format!( "Failed to resolve bash policy configuration for '{}': {error}", cwd.display() - )) + ))) })?; let evaluation = evaluate_bash_command_policy(&command, policy_config.as_ref()); render_policy_result(request.output, &command, &evaluation) } -fn read_stdin_payload() -> Result { +fn read_stdin_payload() -> Result { let mut payload = String::new(); io::stdin().read_to_string(&mut payload).map_err(|error| { - ClassifiedError::validation(format!( + CliError::validation(format!( "Failed to read bash policy request from STDIN: {error}. Try: pipe a JSON payload to 'sce policy bash'." )) })?; if payload.trim().is_empty() { - return Err(ClassifiedError::validation( + return Err(CliError::validation( "Missing bash policy request on STDIN. Try: pipe Claude PreToolUse JSON or normalized {\"command\":...} JSON to 'sce policy bash'.", )); } @@ -495,18 +495,18 @@ fn read_stdin_payload() -> Result { fn parse_command_from_stdin( input: PolicyInputMode, stdin_payload: &str, -) -> Result { +) -> Result { match input { PolicyInputMode::ClaudePreToolUse => parse_claude_pre_tool_use_command(stdin_payload), PolicyInputMode::Normalized => parse_normalized_command(stdin_payload), } } -fn parse_claude_pre_tool_use_command(stdin_payload: &str) -> Result { +fn parse_claude_pre_tool_use_command(stdin_payload: &str) -> Result { let event: ClaudePreToolUseEvent = parse_json_payload(stdin_payload, "Claude PreToolUse")?; if let Some(tool_name) = event.tool_name.as_deref() { if tool_name != "Bash" { - return Err(ClassifiedError::validation(format!( + return Err(CliError::validation(format!( "Invalid Claude PreToolUse payload: expected tool_name 'Bash' but received '{tool_name}'." ))); } @@ -514,37 +514,37 @@ fn parse_claude_pre_tool_use_command(stdin_payload: &str) -> Result Result { +fn parse_normalized_command(stdin_payload: &str) -> Result { let request: NormalizedBashPolicyRequest = parse_json_payload(stdin_payload, "normalized bash policy")?; validate_non_empty_command(request.command, "normalized bash policy") } -fn parse_json_payload(stdin_payload: &str, label: &str) -> Result +fn parse_json_payload(stdin_payload: &str, label: &str) -> Result where T: for<'de> Deserialize<'de>, { serde_json::from_str(stdin_payload).map_err(|error| { - ClassifiedError::validation(format!( + CliError::validation(format!( "Invalid {label} JSON from STDIN: {error}. Try: pipe a valid JSON object to 'sce policy bash'." )) }) } -fn validate_non_empty_command(command: String, label: &str) -> Result { +fn validate_non_empty_command(command: String, label: &str) -> Result { if command.trim().is_empty() { - return Err(ClassifiedError::validation(format!( + return Err(CliError::validation(format!( "Invalid {label} payload: command must be a non-empty string." ))); } Ok(command) } -fn resolved_policy_project_root() -> Result { +fn resolved_policy_project_root() -> Result { let cwd = std::env::current_dir().map_err(|error| { - ClassifiedError::runtime(format!( + CliError::runtime(anyhow::Error::msg(format!( "Failed to determine current directory for bash policy configuration: {error}" - )) + ))) })?; Ok(resolve_git_root(&cwd).unwrap_or(cwd)) } @@ -567,14 +567,14 @@ fn render_policy_result( output: PolicyOutputMode, command: &str, evaluation: &PolicyEvaluation, -) -> Result { +) -> Result { match output { PolicyOutputMode::ClaudeHook => render_claude_hook_result(evaluation), PolicyOutputMode::Json => render_json_result(command, evaluation), } } -fn render_claude_hook_result(evaluation: &PolicyEvaluation) -> Result { +fn render_claude_hook_result(evaluation: &PolicyEvaluation) -> Result { match evaluation { PolicyEvaluation::Allowed { .. } => Ok(String::new()), PolicyEvaluation::Blocked { policy, .. } => serialize_json(&json!({ @@ -587,10 +587,7 @@ fn render_claude_hook_result(evaluation: &PolicyEvaluation) -> Result Result { +fn render_json_result(command: &str, evaluation: &PolicyEvaluation) -> Result { match evaluation { PolicyEvaluation::Allowed { normalized_argv } => serialize_json(&JsonPolicyResult { status: "ok", @@ -617,11 +614,11 @@ fn render_json_result( } } -fn serialize_json(value: &T) -> Result { +fn serialize_json(value: &T) -> Result { serde_json::to_string(value).map_err(|error| { - ClassifiedError::runtime(format!( + CliError::runtime(anyhow::Error::msg(format!( "Failed to serialize bash policy result JSON: {error}" - )) + ))) }) } @@ -869,7 +866,7 @@ mod tests { ) .expect_err("payload should fail"); - assert!(error.message().contains("expected tool_name 'Bash'")); + assert!(error.to_string().contains("expected tool_name 'Bash'")); } #[test] diff --git a/cli/src/services/command_registry.rs b/cli/src/services/command_registry.rs index b58a4ad58..b38f903e3 100644 --- a/cli/src/services/command_registry.rs +++ b/cli/src/services/command_registry.rs @@ -3,7 +3,7 @@ use std::io::Write; use crate::app::{ContextWithRepoRoot, HasLogger}; use crate::services; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; const DEFAULT_COMMAND_NAMES: &[&str] = &[ services::auth_command::NAME, @@ -55,7 +55,7 @@ impl RuntimeCommand { } #[allow(dead_code)] - pub fn execute(&self, context: &C) -> Result + pub fn execute(&self, context: &C) -> Result where C: HasLogger + ContextWithRepoRoot, { @@ -63,11 +63,7 @@ impl RuntimeCommand { self.execute_with_stderr(context, &mut stderr) } - pub fn execute_with_stderr( - &self, - context: &C, - stderr: &mut W, - ) -> Result + pub fn execute_with_stderr(&self, context: &C, stderr: &mut W) -> Result where C: HasLogger + ContextWithRepoRoot, W: Write, diff --git a/cli/src/services/config/command.rs b/cli/src/services/config/command.rs index 66ed0db62..0f7385a0c 100644 --- a/cli/src/services/config/command.rs +++ b/cli/src/services/config/command.rs @@ -1,13 +1,12 @@ use crate::services::config; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; pub struct ConfigCommand { pub subcommand: config::ConfigSubcommand, } impl ConfigCommand { - pub fn execute(&self, _context: &C) -> Result { - config::run_config_subcommand(self.subcommand.clone()) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}"))) + pub fn execute(&self, _context: &C) -> Result { + config::run_config_subcommand(self.subcommand.clone()).map_err(CliError::runtime) } } diff --git a/cli/src/services/doctor/command.rs b/cli/src/services/doctor/command.rs index a2bf6f478..3edf10299 100644 --- a/cli/src/services/doctor/command.rs +++ b/cli/src/services/doctor/command.rs @@ -1,14 +1,13 @@ use crate::app::ContextWithRepoRoot; use crate::services::doctor; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; pub struct DoctorCommand { pub request: doctor::DoctorRequest, } impl DoctorCommand { - pub fn execute(&self, context: &C) -> Result { - doctor::run_doctor_with_context(self.request, context) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}"))) + pub fn execute(&self, context: &C) -> Result { + doctor::run_doctor_with_context(self.request, context).map_err(CliError::runtime) } } diff --git a/cli/src/services/error.rs b/cli/src/services/error.rs index 0e8fb5694..5e2b487c6 100644 --- a/cli/src/services/error.rs +++ b/cli/src/services/error.rs @@ -16,6 +16,15 @@ impl FailureClass { } } + pub fn code(self) -> &'static str { + match self { + Self::Parse => "SCE-ERR-PARSE", + Self::Validation => "SCE-ERR-VALIDATION", + Self::Runtime => "SCE-ERR-RUNTIME", + Self::Dependency => "SCE-ERR-DEPENDENCY", + } + } + pub fn as_str(self) -> &'static str { match self { Self::Parse => "parse", @@ -39,63 +48,198 @@ impl FailureClass { } } -#[derive(Debug)] -pub struct ClassifiedError { - class: FailureClass, - code: &'static str, - message: String, +/// Catalog of expected, deliberately-explained failures presented to the user +/// as a friendly diagnostic instead of a technical error chain. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum UserError { + #[allow(dead_code)] + NotAuthenticated, } -impl ClassifiedError { - pub fn parse(message: impl Into) -> Self { - Self { - class: FailureClass::Parse, - code: "SCE-ERR-PARSE", - message: message.into(), +impl UserError { + pub fn class(self) -> FailureClass { + match self { + Self::NotAuthenticated => FailureClass::Runtime, } } - pub fn validation(message: impl Into) -> Self { - Self { - class: FailureClass::Validation, - code: "SCE-ERR-VALIDATION", - message: message.into(), + #[allow(dead_code)] + pub fn key(self) -> &'static str { + match self { + Self::NotAuthenticated => "auth.not_authenticated", + } + } + + pub fn message(self) -> &'static str { + match self { + Self::NotAuthenticated => { + "You are not logged in. Please log in using the `sce auth login` command." + } } } +} + +impl std::fmt::Display for UserError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.message()) + } +} + +/// Typed CLI-boundary error separating expected, user-facing failures from +/// internal failures whose technical `anyhow` source is preserved for +/// observability and diagnostic rendering. +#[derive(Debug)] +pub enum CliError { + #[allow(dead_code)] + User { + error: UserError, + source: Option, + }, + Internal { + class: FailureClass, + source: anyhow::Error, + }, +} - pub fn runtime(message: impl Into) -> Self { - Self { - class: FailureClass::Runtime, - code: "SCE-ERR-RUNTIME", - message: message.into(), +impl CliError { + #[allow(dead_code)] + pub fn user(error: UserError) -> Self { + Self::User { + error, + source: None, } } - pub fn dependency(message: impl Into) -> Self { - Self { - class: FailureClass::Dependency, - code: "SCE-ERR-DEPENDENCY", - message: message.into(), + #[allow(dead_code)] + pub fn user_with_source(error: UserError, source: impl Into) -> Self { + Self::User { + error, + source: Some(source.into()), } } - pub fn class(&self) -> FailureClass { - self.class + pub fn internal(class: FailureClass, source: impl Into) -> Self { + Self::Internal { + class, + source: source.into(), + } } - pub fn code(&self) -> &'static str { - self.code + pub fn runtime(source: impl Into) -> Self { + Self::internal(FailureClass::Runtime, source) + } + + pub fn dependency(source: impl Into) -> Self { + Self::internal(FailureClass::Dependency, source) + } + + /// Compatibility helper for parse failures still expressed as a plain + /// message rather than a live `anyhow::Error` source. + pub fn parse(message: impl Into) -> Self { + Self::internal(FailureClass::Parse, anyhow::Error::msg(message.into())) + } + + /// Compatibility helper for validation failures still expressed as a + /// plain message rather than a live `anyhow::Error` source. + pub fn validation(message: impl Into) -> Self { + Self::internal(FailureClass::Validation, anyhow::Error::msg(message.into())) } - pub fn message(&self) -> &str { - &self.message + pub fn class(&self) -> FailureClass { + match self { + Self::User { error, .. } => error.class(), + Self::Internal { class, .. } => *class, + } + } + + pub fn code(&self) -> &'static str { + self.class().code() } } -impl std::fmt::Display for ClassifiedError { +impl std::fmt::Display for CliError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.message) + match self { + Self::User { error, .. } => write!(f, "{error}"), + Self::Internal { source, .. } => write!(f, "{source:#}"), + } } } -impl std::error::Error for ClassifiedError {} +impl std::error::Error for CliError {} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn failure_class_code_maps_to_stable_sce_err_strings() { + assert_eq!(FailureClass::Parse.code(), "SCE-ERR-PARSE"); + assert_eq!(FailureClass::Validation.code(), "SCE-ERR-VALIDATION"); + assert_eq!(FailureClass::Runtime.code(), "SCE-ERR-RUNTIME"); + assert_eq!(FailureClass::Dependency.code(), "SCE-ERR-DEPENDENCY"); + } + + #[test] + fn user_error_not_authenticated_classifies_as_runtime() { + let error = CliError::user(UserError::NotAuthenticated); + + assert_eq!(error.class(), FailureClass::Runtime); + assert_eq!(error.code(), "SCE-ERR-RUNTIME"); + assert_eq!(UserError::NotAuthenticated.key(), "auth.not_authenticated"); + assert!(error.to_string().contains("You are not logged in")); + } + + #[test] + fn user_with_source_preserves_technical_source() { + let error = CliError::user_with_source( + UserError::NotAuthenticated, + anyhow::anyhow!("missing credentials"), + ); + + let CliError::User { source, .. } = &error else { + panic!("expected CliError::User"); + }; + assert_eq!( + source.as_ref().expect("source preserved").to_string(), + "missing credentials" + ); + assert!(error.to_string().contains("You are not logged in")); + } + + #[test] + fn internal_error_renders_the_real_anyhow_chain() { + let source = anyhow::anyhow!("root cause").context("failed to do the thing"); + let error = CliError::internal(FailureClass::Dependency, source); + + assert_eq!(error.class(), FailureClass::Dependency); + assert_eq!(error.code(), "SCE-ERR-DEPENDENCY"); + assert_eq!( + error.to_string(), + "failed to do the thing: root cause".to_string() + ); + } + + #[test] + fn runtime_and_dependency_constructors_classify_correctly() { + assert_eq!( + CliError::runtime(anyhow::anyhow!("boom")).class(), + FailureClass::Runtime + ); + assert_eq!( + CliError::dependency(anyhow::anyhow!("boom")).class(), + FailureClass::Dependency + ); + } + + #[test] + fn parse_and_validation_compatibility_helpers_wrap_plain_messages() { + let parse_error = CliError::parse("bad usage"); + let validation_error = CliError::validation("bad value"); + + assert_eq!(parse_error.class(), FailureClass::Parse); + assert_eq!(parse_error.to_string(), "bad usage"); + assert_eq!(validation_error.class(), FailureClass::Validation); + assert_eq!(validation_error.to_string(), "bad value"); + } +} diff --git a/cli/src/services/hooks/command.rs b/cli/src/services/hooks/command.rs index 4d466c2ad..5aa0cc6ce 100644 --- a/cli/src/services/hooks/command.rs +++ b/cli/src/services/hooks/command.rs @@ -1,5 +1,5 @@ use crate::app::HasLogger; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; use crate::services::hooks; pub struct HooksCommand { @@ -7,8 +7,8 @@ pub struct HooksCommand { } impl HooksCommand { - pub fn execute(&self, context: &C) -> Result { + pub fn execute(&self, context: &C) -> Result { hooks::run_hooks_subcommand(&self.subcommand, Some(context.logger())) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}"))) + .map_err(CliError::runtime) } } diff --git a/cli/src/services/observability.rs b/cli/src/services/observability.rs index 4e293bcad..2edc386bf 100644 --- a/cli/src/services/observability.rs +++ b/cli/src/services/observability.rs @@ -17,7 +17,7 @@ use tracing::Level; use crate::services::config::{ self, LogFormat, LogLevel, ENV_LOG_DIR, ENV_LOG_FORMAT, ENV_LOG_LEVEL, }; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; use crate::services::security::redact_sensitive_text; pub mod traits; @@ -136,16 +136,20 @@ impl Logger { self.log(LogLevel::Error, event_id, message, fields, session_id); } - pub fn log_classified_error(&self, error: &ClassifiedError, session_id: Option<&str>) { + pub fn log_cli_error(&self, error: &CliError, session_id: Option<&str>) { let event_id = format!("sce.error.{}", error.code()); + let message = error.to_string(); + let fields = cli_error_fields(error); + let field_refs: Vec<(&str, &str)> = fields + .iter() + .map(|(key, value)| (*key, value.as_str())) + .collect(); + self.log( LogLevel::Error, &event_id, - error.message(), - &[ - ("error_code", error.code()), - ("error_class", error.class().as_str()), - ], + &message, + &field_refs, session_id, ); } @@ -253,6 +257,41 @@ impl Logger { } } +fn cli_error_surface(error: &CliError) -> &'static str { + match error { + CliError::User { .. } => "user", + CliError::Internal { .. } => "internal", + } +} + +fn cli_error_technical_source(error: &CliError) -> Option<&anyhow::Error> { + match error { + CliError::User { source, .. } => source.as_ref(), + CliError::Internal { source, .. } => Some(source), + } +} + +fn cli_error_fields(error: &CliError) -> Vec<(&'static str, String)> { + let mut fields: Vec<(&str, String)> = vec![ + ("error_code", error.code().to_string()), + ("error_class", error.class().as_str().to_string()), + ("error_surface", cli_error_surface(error).to_string()), + ]; + + if let CliError::User { + error: user_error, .. + } = error + { + fields.push(("user_error", user_error.key().to_string())); + } + + if let Some(source) = cli_error_technical_source(error) { + fields.push(("error_source", format!("{source:#}"))); + } + + fields +} + fn validate_log_dir(value: &str) -> Result<()> { if value.is_empty() { bail!("Invalid {ENV_LOG_DIR} ''. Try: set it to a directory path or unset {ENV_LOG_DIR}."); @@ -644,3 +683,68 @@ fn emit_tracing_event_with_fields_json( ), } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::services::error::{FailureClass, UserError}; + + fn field_value<'a>(fields: &'a [(&'static str, String)], key: &str) -> Option<&'a str> { + fields + .iter() + .find(|(field_key, _)| *field_key == key) + .map(|(_, value)| value.as_str()) + } + + #[test] + fn observability_fields_for_user_error_carry_surface_and_key_plus_source() { + let error = CliError::user_with_source( + UserError::NotAuthenticated, + anyhow::anyhow!("missing credentials"), + ); + + let fields = cli_error_fields(&error); + + assert_eq!(field_value(&fields, "error_code"), Some("SCE-ERR-RUNTIME")); + assert_eq!(field_value(&fields, "error_class"), Some("runtime")); + assert_eq!(field_value(&fields, "error_surface"), Some("user")); + assert_eq!( + field_value(&fields, "user_error"), + Some("auth.not_authenticated") + ); + assert_eq!( + field_value(&fields, "error_source"), + Some("missing credentials") + ); + } + + #[test] + fn observability_fields_for_user_error_without_source_omit_error_source() { + let error = CliError::user(UserError::NotAuthenticated); + + let fields = cli_error_fields(&error); + + assert_eq!(field_value(&fields, "error_surface"), Some("user")); + assert_eq!(field_value(&fields, "error_source"), None); + } + + #[test] + fn observability_fields_for_internal_error_carry_surface_and_full_source_chain() { + let source = anyhow::anyhow!("root cause").context("failed to do the thing"); + let error = CliError::internal(FailureClass::Dependency, source); + + let fields = cli_error_fields(&error); + + assert_eq!( + field_value(&fields, "error_code"), + Some("SCE-ERR-DEPENDENCY") + ); + assert_eq!(field_value(&fields, "error_class"), Some("dependency")); + assert_eq!(field_value(&fields, "error_surface"), Some("internal")); + assert_eq!(field_value(&fields, "user_error"), None); + assert_eq!( + field_value(&fields, "error_source"), + Some("failed to do the thing: root cause") + ); + } +} diff --git a/cli/src/services/observability/traits.rs b/cli/src/services/observability/traits.rs index d715f1f3a..0515e080a 100644 --- a/cli/src/services/observability/traits.rs +++ b/cli/src/services/observability/traits.rs @@ -1,4 +1,4 @@ -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; pub trait Logger: Send + Sync { fn info( @@ -33,14 +33,14 @@ pub trait Logger: Send + Sync { session_id: Option<&str>, ); - fn log_classified_error(&self, error: &ClassifiedError, session_id: Option<&str>); + fn log_cli_error(&self, error: &CliError, session_id: Option<&str>); } pub trait Telemetry: Send + Sync { fn with_default_subscriber( &self, - action: &mut dyn FnMut() -> Result, - ) -> Result; + action: &mut dyn FnMut() -> Result, + ) -> Result; } #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] @@ -84,7 +84,7 @@ impl Logger for NoopLogger { ) { } - fn log_classified_error(&self, _error: &ClassifiedError, _session_id: Option<&str>) {} + fn log_cli_error(&self, _error: &CliError, _session_id: Option<&str>) {} } impl Logger for super::Logger { @@ -128,8 +128,8 @@ impl Logger for super::Logger { super::Logger::error(self, event_id, message, fields, session_id); } - fn log_classified_error(&self, error: &ClassifiedError, session_id: Option<&str>) { - super::Logger::log_classified_error(self, error, session_id); + fn log_cli_error(&self, error: &CliError, session_id: Option<&str>) { + super::Logger::log_cli_error(self, error, session_id); } } @@ -139,8 +139,8 @@ pub struct NoopTelemetry; impl Telemetry for NoopTelemetry { fn with_default_subscriber( &self, - action: &mut dyn FnMut() -> Result, - ) -> Result { + action: &mut dyn FnMut() -> Result, + ) -> Result { action() } } diff --git a/cli/src/services/parse/command_runtime.rs b/cli/src/services/parse/command_runtime.rs index d58dbbd63..b21d135a3 100644 --- a/cli/src/services/parse/command_runtime.rs +++ b/cli/src/services/parse/command_runtime.rs @@ -1,13 +1,13 @@ use crate::{cli_schema, command_surface, services}; use services::command_registry::{CommandRegistry, RuntimeCommand}; -use services::error::{ClassifiedError, FailureClass}; +use services::error::{CliError, FailureClass}; use services::observability::traits::Logger as LoggerTrait; pub fn parse_runtime_command( args: I, registry: &CommandRegistry, logger: Option<&dyn LoggerTrait>, -) -> Result +) -> Result where I: IntoIterator, { @@ -47,7 +47,7 @@ fn handle_clap_error( args: &[String], registry: &CommandRegistry, error: &clap::Error, -) -> Result { +) -> Result { if error.kind() == clap::error::ErrorKind::DisplayHelp { if let Some((name, text)) = render_subcommand_help_from_args(args) { return Ok(RuntimeCommand::HelpText( @@ -63,7 +63,7 @@ fn handle_clap_error( return Ok(help_text); } - return Err(ClassifiedError::parse( + return Err(CliError::parse( "Missing required subcommand. Try: run 'sce --help' to see valid commands.", )); } @@ -75,24 +75,21 @@ fn handle_clap_error( Err(classify_clap_error(error)) } -fn registry_command( - registry: &CommandRegistry, - name: &str, -) -> Result { +fn registry_command(registry: &CommandRegistry, name: &str) -> Result { if !registry.contains(name) { - return Err(ClassifiedError::runtime(format!( + return Err(CliError::runtime(anyhow::Error::msg(format!( "Command '{name}' is not registered. Try: run 'sce --help' to see available commands." - ))); + )))); } services::command_registry::default_runtime_command(name).ok_or_else(|| { - ClassifiedError::runtime(format!( + CliError::runtime(anyhow::Error::msg(format!( "Command '{name}' is not registered. Try: run 'sce --help' to see available commands." - )) + ))) }) } -fn classify_clap_error(error: &clap::Error) -> ClassifiedError { +fn classify_clap_error(error: &clap::Error) -> CliError { use clap::error::ErrorKind; let message = error.to_string(); @@ -105,8 +102,8 @@ fn classify_clap_error(error: &clap::Error) -> ClassifiedError { let cleaned_message = clean_clap_error_message(&message, error.kind()); match class { - FailureClass::Validation => ClassifiedError::validation(cleaned_message), - _ => ClassifiedError::parse(cleaned_message), + FailureClass::Validation => CliError::validation(cleaned_message), + _ => CliError::parse(cleaned_message), } } @@ -209,7 +206,7 @@ fn extract_quoted_value(message: &str) -> Option { Some(message[start + 1..start + 1 + end].to_string()) } -fn convert_clap_command(command: cli_schema::Commands) -> Result { +fn convert_clap_command(command: cli_schema::Commands) -> Result { match command { cli_schema::Commands::Config { subcommand } => convert_config_subcommand(subcommand), cli_schema::Commands::Auth { subcommand } => convert_auth_subcommand(subcommand), @@ -314,7 +311,7 @@ fn convert_policy_output_mode( #[allow(clippy::unnecessary_wraps, clippy::needless_pass_by_value)] fn convert_auth_subcommand( subcommand: cli_schema::AuthSubcommand, -) -> Result { +) -> Result { let subcommand = match subcommand { cli_schema::AuthSubcommand::Login { format } => { services::auth_command::AuthSubcommand::Login { format } @@ -347,7 +344,7 @@ fn convert_completion_shell( #[allow(clippy::unnecessary_wraps)] fn convert_config_subcommand( subcommand: cli_schema::ConfigSubcommand, -) -> Result { +) -> Result { match subcommand { cli_schema::ConfigSubcommand::Show { format, @@ -388,9 +385,9 @@ fn convert_config_subcommand( fn convert_setup_command( options: services::setup::SetupCliOptions, -) -> Result { +) -> Result { let request = services::setup::resolve_setup_request(options) - .map_err(|error| ClassifiedError::validation(error.to_string()))?; + .map_err(|error| CliError::validation(error.to_string()))?; Ok(RuntimeCommand::Setup( services::setup::command::SetupCommand { request }, @@ -400,7 +397,7 @@ fn convert_setup_command( #[allow(clippy::unnecessary_wraps)] fn convert_hooks_subcommand( subcommand: cli_schema::HooksSubcommand, -) -> Result { +) -> Result { let subcommand = convert_hooks_subcommand_request(subcommand)?; Ok(RuntimeCommand::Hooks( @@ -410,17 +407,17 @@ fn convert_hooks_subcommand( fn convert_hooks_subcommand_request( subcommand: cli_schema::HooksSubcommand, -) -> Result { +) -> Result { match subcommand { cli_schema::HooksSubcommand::PreCommit => Ok(services::hooks::HookSubcommand::PreCommit), cli_schema::HooksSubcommand::CommitMsg { message_file } => { Ok(services::hooks::HookSubcommand::CommitMsg { message_file }) } cli_schema::HooksSubcommand::PostCommit { vcs, remote_url } => { - let vcs_type = parse_optional_hook_vcs_type(vcs.as_deref()) - .map_err(ClassifiedError::validation)?; + let vcs_type = + parse_optional_hook_vcs_type(vcs.as_deref()).map_err(CliError::validation)?; let remote_url = - parse_optional_hook_remote_url(remote_url).map_err(ClassifiedError::validation)?; + parse_optional_hook_remote_url(remote_url).map_err(CliError::validation)?; Ok(services::hooks::HookSubcommand::PostCommit { vcs_type, @@ -544,6 +541,6 @@ mod tests { }; assert_eq!(error.class(), FailureClass::Parse); - assert!(error.message().contains("Unknown command 'renew'")); + assert!(error.to_string().contains("Unknown command 'renew'")); } } diff --git a/cli/src/services/setup/command.rs b/cli/src/services/setup/command.rs index 3b848fe2a..86bb6fe34 100644 --- a/cli/src/services/setup/command.rs +++ b/cli/src/services/setup/command.rs @@ -1,7 +1,7 @@ use anyhow::Context; use crate::app::ContextWithRepoRoot; -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; use crate::services::lifecycle::{ lifecycle_providers, RequiredHookInstallStatus, RequiredHooksInstallOutcome, }; @@ -12,18 +12,18 @@ pub struct SetupCommand { } impl SetupCommand { - pub fn execute(&self, context: &C) -> Result { + pub fn execute(&self, context: &C) -> Result { let setup_start_path = match &self.request.hooks_repo_path { Some(path) => path.clone(), None => std::env::current_dir() .context("Failed to determine current directory") - .map_err(|error| ClassifiedError::runtime(format!("{error:#}")))?, + .map_err(CliError::runtime)?, }; // The repository root is resolved before any prompt so the interactive // optional-workflow prompt can pre-check the persisted selection. - let repository_root = setup::ensure_git_repository(&setup_start_path) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}")))?; + let repository_root = + setup::ensure_git_repository(&setup_start_path).map_err(CliError::runtime)?; let setup_dispatch = if self.request.context_only { None @@ -40,7 +40,7 @@ impl SetupCommand { &setup::InquireSetupTargetPrompter, &optional_workflow_defaults, ) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}")))? + .map_err(CliError::runtime)? { setup::SetupDispatch::Proceed { mode: resolved_mode, @@ -57,8 +57,8 @@ impl SetupCommand { let mut sections = Vec::new(); // Every successful setup path ensures the durable-context baseline exists. - let context_message = setup::bootstrap_context_baseline(&repository_root) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}")))?; + let context_message = + setup::bootstrap_context_baseline(&repository_root).map_err(CliError::runtime)?; sections.push(context_message); if self.request.context_only { @@ -73,9 +73,7 @@ impl SetupCommand { let providers = lifecycle_providers(self.request.install_hooks); for provider in &providers { - let outcome = provider - .setup(&ctx) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}")))?; + let outcome = provider.setup(&ctx).map_err(CliError::runtime)?; sections.extend(outcome.messages); @@ -96,7 +94,7 @@ impl SetupCommand { let setup_message = setup::run_setup_for_mode(&repository_root, resolved_mode, optional_workflows) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}")))?; + .map_err(CliError::runtime)?; sections.push(setup_message); } diff --git a/cli/src/services/style.rs b/cli/src/services/style.rs index a1496dc5a..f448b4d10 100644 --- a/cli/src/services/style.rs +++ b/cli/src/services/style.rs @@ -39,13 +39,6 @@ where style_if(text, supports_color(), f) } -pub(crate) fn style_if_enabled_stderr(text: &str, f: F) -> String -where - F: FnOnce(&str) -> String, -{ - style_if(text, supports_color_stderr(), f) -} - pub(crate) fn success_with_stderr_color_policy(text: &str, color_enabled: bool) -> String { style_if(text, color_enabled, |s| s.green().bold().to_string()) } @@ -56,7 +49,7 @@ pub fn heading(text: &str) -> String { } #[must_use] -fn heading_with_color_policy(text: &str, color_enabled: bool) -> String { +pub(crate) fn heading_with_color_policy(text: &str, color_enabled: bool) -> String { style_if(text, color_enabled, |s| s.cyan().bold().to_string()) } @@ -72,12 +65,22 @@ fn command_name_with_color_policy(text: &str, color_enabled: bool) -> String { #[must_use] pub fn error_code(text: &str) -> String { - style_if_enabled_stderr(text, |s| s.red().bold().to_string()) + error_code_with_color_policy(text, supports_color_stderr()) } #[must_use] -pub fn error_text(text: &str) -> String { - style_if_enabled_stderr(text, |s| s.yellow().to_string()) +pub(crate) fn error_code_with_color_policy(text: &str, color_enabled: bool) -> String { + style_if(text, color_enabled, |s| s.red().bold().to_string()) +} + +/// Styles human-readable stderr diagnostic bodies (yellow), following the +/// stderr TTY/`NO_COLOR` policy passed in by the caller. `app_support`'s +/// error-diagnostic renderer is the sole caller in production, threading +/// `supports_color_stderr()` through explicitly so the same code path is +/// exercisable with an injected policy in tests. +#[must_use] +pub(crate) fn error_text_with_color_policy(text: &str, color_enabled: bool) -> String { + style_if(text, color_enabled, |s| s.yellow().to_string()) } #[must_use] @@ -322,3 +325,44 @@ pub(crate) fn banner_with_gradient_with_color_policy( fn lerp_u8(a: u8, b: u8, t: f64) -> u8 { (f64::from(a) + (f64::from(b) - f64::from(a)) * t).round() as u8 } + +#[cfg(test)] +mod tests { + use super::*; + + // `supports_color_stderr()` combines the real stderr TTY check with the + // `NO_COLOR` check into one `color_enabled` boolean; a real TTY can't be + // simulated in `cargo test`, so these `_with_color_policy` seams (the same + // pattern used elsewhere in this module and in `doctor/render.rs`) are + // exercised directly against that boolean instead of mutating process + // environment state. + + #[test] + fn error_text_styles_when_color_enabled() { + let styled = error_text_with_color_policy("boom", true); + assert_ne!(styled, "boom"); + assert!(styled.contains("boom")); + } + + #[test] + fn error_text_is_plain_when_color_disabled() { + // Covers both a non-TTY stderr (redirected) and `NO_COLOR` being set, + // since both collapse to `color_enabled: false`. + assert_eq!(error_text_with_color_policy("boom", false), "boom"); + } + + #[test] + fn error_code_styles_when_color_enabled() { + let styled = error_code_with_color_policy("SCE-ERR-RUNTIME", true); + assert_ne!(styled, "SCE-ERR-RUNTIME"); + assert!(styled.contains("SCE-ERR-RUNTIME")); + } + + #[test] + fn error_code_is_plain_when_color_disabled() { + assert_eq!( + error_code_with_color_policy("SCE-ERR-RUNTIME", false), + "SCE-ERR-RUNTIME" + ); + } +} diff --git a/cli/src/services/sync/command.rs b/cli/src/services/sync/command.rs index 3ff9845e6..6120bbe6f 100644 --- a/cli/src/services/sync/command.rs +++ b/cli/src/services/sync/command.rs @@ -1,7 +1,7 @@ use std::io::Write; use crate::app::ContextWithRepoRoot; -use crate::services::error::ClassifiedError; +use crate::services::error::{CliError, UserError}; use crate::services::sync::progress::{ IndicatifProgressReporter, NoopProgressReporter, ProgressReporter, }; @@ -16,7 +16,7 @@ pub struct SyncCommand { pub request: SyncRequest, } -fn current_repo_root(context: &C) -> Result +fn current_repo_root(context: &C) -> Result where C: ContextWithRepoRoot, { @@ -24,19 +24,25 @@ where Ok(path.to_path_buf()) } else { std::env::current_dir().map_err(|err| { - ClassifiedError::runtime(format!("failed to determine current directory: {err}")) + CliError::runtime(anyhow::Error::msg(format!( + "failed to determine current directory: {err}" + ))) }) } } #[allow(clippy::needless_pass_by_value)] -fn classify_sync_error(err: TraceSyncError) -> ClassifiedError { - ClassifiedError::runtime(format!("{err}")) +fn classify_sync_error(err: TraceSyncError) -> CliError { + if err.is_authentication_failure() { + CliError::user_with_source(UserError::NotAuthenticated, err) + } else { + CliError::runtime(err) + } } impl SyncCommand { #[allow(dead_code)] - pub fn execute(&self, context: &C) -> Result + pub fn execute(&self, context: &C) -> Result where C: ContextWithRepoRoot, { @@ -44,11 +50,7 @@ impl SyncCommand { self.execute_with_stderr(context, &mut stderr) } - pub fn execute_with_stderr( - &self, - context: &C, - stderr: &mut W, - ) -> Result + pub fn execute_with_stderr(&self, context: &C, stderr: &mut W) -> Result where C: ContextWithRepoRoot, W: Write, @@ -62,7 +64,7 @@ impl SyncCommand { context: &C, stderr: &mut W, clock: &Clock, - ) -> Result + ) -> Result where C: ContextWithRepoRoot, W: Write, @@ -88,6 +90,87 @@ impl SyncCommand { .map_err(classify_sync_error)?; render_sync::render(&report, self.request.format) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}"))) + .map_err(|error| CliError::runtime(anyhow::Error::msg(format!("{error:#}")))) + } +} + +#[cfg(test)] +mod tests { + use super::classify_sync_error; + use crate::services::agent_trace_sync::control_plane::ControlPlaneError; + use crate::services::agent_trace_sync::StreamSyncError; + use crate::services::error::CliError; + use crate::services::sync::sync::TraceSyncError; + + fn assert_user_not_authenticated(err: TraceSyncError) { + match classify_sync_error(err) { + CliError::User { error, source } => { + assert_eq!(error.key(), "auth.not_authenticated"); + assert!(source.is_some()); + } + other @ CliError::Internal { .. } => panic!("expected CliError::User, got {other:?}"), + } + } + + fn assert_internal(err: TraceSyncError) { + match classify_sync_error(err) { + CliError::Internal { .. } => {} + other @ CliError::User { .. } => panic!("expected CliError::Internal, got {other:?}"), + } + } + + #[test] + fn initial_state_missing_credentials_classifies_as_not_authenticated() { + assert_user_not_authenticated(TraceSyncError::ControlPlane( + ControlPlaneError::MissingCredentials, + )); + } + + #[test] + fn initial_state_authentication_failed_classifies_as_not_authenticated() { + assert_user_not_authenticated(TraceSyncError::ControlPlane( + ControlPlaneError::AuthenticationFailed("token expired".to_string()), + )); + } + + #[test] + fn stream_batch_authentication_failed_classifies_as_not_authenticated() { + assert_user_not_authenticated(TraceSyncError::Stream { + stream: "prompts", + source: StreamSyncError::Terminal(ControlPlaneError::AuthenticationFailed( + "token expired".to_string(), + )), + }); + } + + #[test] + fn stream_refresh_authentication_failed_classifies_as_not_authenticated() { + assert_user_not_authenticated(TraceSyncError::Stream { + stream: "prompts", + source: StreamSyncError::Refresh(ControlPlaneError::MissingCredentials), + }); + } + + #[test] + fn other_control_plane_errors_classify_as_internal() { + for error in [ + ControlPlaneError::Forbidden("nope".to_string()), + ControlPlaneError::BadRequest("bad".to_string()), + ControlPlaneError::Transport("down".to_string()), + ControlPlaneError::ServerError("500".to_string()), + ControlPlaneError::InvalidResponse("garbage".to_string()), + ControlPlaneError::Storage("disk".to_string()), + ControlPlaneError::Protocol { + status: reqwest::StatusCode::NOT_FOUND, + message: "route removed".to_string(), + }, + ] { + assert_internal(TraceSyncError::ControlPlane(error)); + } + } + + #[test] + fn runtime_failure_classifies_as_internal() { + assert_internal(TraceSyncError::Runtime("local failure".to_string())); } } diff --git a/cli/src/services/sync/sync.rs b/cli/src/services/sync/sync.rs index f95d1e261..32f1b35d8 100644 --- a/cli/src/services/sync/sync.rs +++ b/cli/src/services/sync/sync.rs @@ -132,6 +132,22 @@ impl fmt::Display for TraceSyncError { impl std::error::Error for TraceSyncError {} +impl TraceSyncError { + /// True when this failure means the caller has no usable `WorkOS` + /// credentials, whether that surfaced from the initial `/state` call + /// (`ControlPlane`) or from a stream's batch/refresh path (`Stream`). + /// `Runtime` never carries a `ControlPlaneError` and is never an + /// authentication failure. + #[allow(dead_code)] + pub fn is_authentication_failure(&self) -> bool { + match self { + Self::Runtime(_) => false, + Self::ControlPlane(error) => error.is_authentication_failure(), + Self::Stream { source, .. } => source.is_authentication_failure(), + } + } +} + /// Resolves the current repository's Agent Trace storage (the same /// `ContextWithRepoRoot`/`AgentTraceStorageContext`/`resolve_agent_trace_storage` /// path used by the sync command) and control-plane configuration, then @@ -507,7 +523,7 @@ where } Err(ControlPlaneError::Conflict(_)) => BatchAttemptOutcome::Conflict, Err(error) if is_stream_terminal(&error) => { - BatchAttemptOutcome::Terminal(error.to_string()) + BatchAttemptOutcome::Terminal(error) } Err(_) => BatchAttemptOutcome::Ambiguous, } @@ -522,7 +538,7 @@ where let response = client .ingestion_state(&state_request) .await - .map_err(|error| StreamSyncError::Refresh(error.to_string()))?; + .map_err(StreamSyncError::Refresh)?; Ok(cursor_for_stream(&response.cursors, stream)) }) }, diff --git a/cli/src/services/version/command.rs b/cli/src/services/version/command.rs index 4a073a5ab..c5fd2ae49 100644 --- a/cli/src/services/version/command.rs +++ b/cli/src/services/version/command.rs @@ -1,4 +1,4 @@ -use crate::services::error::ClassifiedError; +use crate::services::error::CliError; use crate::services::version; pub struct VersionCommand { @@ -6,8 +6,7 @@ pub struct VersionCommand { } impl VersionCommand { - pub fn execute(&self, _context: &C) -> Result { - version::render_version(self.request) - .map_err(|error| ClassifiedError::runtime(format!("{error:#}"))) + pub fn execute(&self, _context: &C) -> Result { + version::render_version(self.request).map_err(CliError::runtime) } } diff --git a/context/cli/agent-trace-sync-command.md b/context/cli/agent-trace-sync-command.md index 4545f7bde..7500c3119 100644 --- a/context/cli/agent-trace-sync-command.md +++ b/context/cli/agent-trace-sync-command.md @@ -43,7 +43,8 @@ Because every invocation starts from the control plane's authoritative `/state` ## Recovery semantics -- **`401` (unexpected):** the control-plane client refreshes the WorkOS token exactly once, saves it, and retries the request exactly once. Concurrent callers that observed the same rejected token coalesce onto the first refresh and reuse its saved token; a second `401` fails the command with `sce auth login` guidance, and there is no further retry. +- **`401` (unexpected):** the control-plane client refreshes the WorkOS token exactly once, saves it, and retries the request exactly once. Concurrent callers that observed the same rejected token coalesce onto the first refresh and reuse its saved token; a second `401` (`ControlPlaneError::MissingCredentials`/`AuthenticationFailed`) fails the command with `sce auth login` guidance, and there is no further retry. +- **Typed authentication classification:** `ControlPlaneError::is_authentication_failure()` is `true` only for `MissingCredentials`/`AuthenticationFailed`, and the same typed traversal is exposed as `StreamSyncError::is_authentication_failure()` and `TraceSyncError::is_authentication_failure()` — no sync-stream path erases a `ControlPlaneError` into a bare `String` before it reaches the command boundary. `cli/src/services/sync/command.rs`'s `classify_sync_error` calls this traversal (never string/substring matching) to route an authentication failure from the initial `/state` call, a stream batch request, or a stream reconciliation `/state` refresh to `CliError::User { error: UserError::NotAuthenticated, .. }`; every other `ControlPlaneError` variant (`Forbidden`, `BadRequest`, `Transport`, `ServerError`, `InvalidResponse`, `Storage`, `Protocol`) stays `CliError::Internal` with its full technical chain preserved. See [CLI error-code taxonomy](../sce/cli-error-code-taxonomy.md) for the `CliError`/`UserError` architecture and [sync-command.md](sync-command.md#error-classification) for the command-level classifier. - **`409` (cursor conflict):** the per-stream sync engine reconciles by refetching `/state`, replacing only the affected stream's cursor, and resuming from local rows after the refreshed cursor — already-accepted rows are never resent. - **Ambiguous batch failure (`5xx`, transport failure, or an undecodable `2xx` body):** the engine reconciles via `/state` before any resend. If the refreshed cursor advanced (the batch was actually committed), sync continues from it without resending. If the cursor is unchanged (the batch was not committed), sync may resend once from the authoritative cursor. - **Reconciliation bound:** both the `409` and ambiguous-failure reconciliation paths share one bounded attempt counter per stream; exhausting it fails that stream with a "did not converge" error instead of looping unboundedly. @@ -58,4 +59,5 @@ Because every invocation starts from the control plane's authoritative `/state` - [agent-trace-storage.md](agent-trace-storage.md) — the repository-scoped storage resolver used by sync. - [agent-trace-export-readers.md](../sce/agent-trace-export-readers.md) — the read-only local export boundary sync reads through. - [auth-db.md](../sce/auth-db.md) — encrypted WorkOS credential storage sync authenticates through. +- [CLI error-code taxonomy](../sce/cli-error-code-taxonomy.md) — the `CliError`/`UserError` typed boundary that authentication-failure classification renders through. - [Trace-sync progress stream contract](../decisions/2026-08-13-trace-sync-progress-stream-contract.md) — stderr progress/timestamps and stdout/JSON compatibility boundary. diff --git a/context/cli/styling-service.md b/context/cli/styling-service.md index 903157609..a87a418c7 100644 --- a/context/cli/styling-service.md +++ b/context/cli/styling-service.md @@ -16,7 +16,6 @@ The CLI styling service in `cli/src/services/style.rs` provides deterministic te ### Conditional Styling - `style_if_enabled(text: &str, f: F) -> String` - Applies styling function only when colors are enabled -- `style_if_enabled_stderr(text: &str, f: F) -> String` - Applies styling function only when stderr colors are enabled - `success_with_stderr_color_policy(text: &str, color_enabled: bool) -> String` - Internal helper for applying the shared green/bold stderr success policy when a caller already resolved the color decision ### Help Output Styling @@ -28,8 +27,9 @@ The CLI styling service in `cli/src/services/style.rs` provides deterministic te ### Error Diagnostics Styling - `error_code(text: &str) -> String` - Styles error codes (red/bold) for stderr diagnostics +- `error_code_with_color_policy(text: &str, color_enabled: bool) -> String` - Internal variant accepting an explicit color policy flag for testability - `heading(text: &str) -> String` - Styles headings for both stdout and stderr output (cyan/bold) -- `error_text(text: &str) -> String` - Styles human-readable stderr diagnostic bodies (yellow) +- `error_text_with_color_policy(text: &str, color_enabled: bool) -> String` - Internal helper styling human-readable stderr diagnostic bodies (yellow) given an explicit color policy flag; `app_support::write_error_diagnostic` is the sole production caller, passing `supports_color_stderr()` ### Command Output Styling @@ -76,14 +76,19 @@ sync progress module; this styling service owns only the shared color policy. ## Usage ```rust -use crate::services::style::{heading, command_name, error_code, error_text, success, label, value, prompt_label, prompt_value, supports_color}; +use crate::services::style::{heading, command_name, error_code, error_text_with_color_policy, success, label, value, prompt_label, prompt_value, supports_color, supports_color_stderr}; // Help output styling println!("{}", heading("Usage:")); println!(" {}", command_name("sce setup")); // Error diagnostics styling (stderr) -eprintln!("{} [{}]: {}", heading("Error"), error_code("SCE-ERR-PARSE"), error_text(message)); +eprintln!( + "{} [{}]: {}", + heading("Error"), + error_code("SCE-ERR-PARSE"), + error_text_with_color_policy(message, supports_color_stderr()) +); // Command output styling println!("{}", success("Setup completed successfully.")); diff --git a/context/cli/sync-command.md b/context/cli/sync-command.md index 5f5070cba..f3d9e24bd 100644 --- a/context/cli/sync-command.md +++ b/context/cli/sync-command.md @@ -92,10 +92,30 @@ terminal protocol failures, ownership rejection, and sanitized control-plane errors remain owned by `services::agent_trace_sync` and its control-plane client. The command change does not alter those semantics. +## Error classification + +`cli/src/services/sync/command.rs`'s `classify_sync_error` maps the command's +terminal `TraceSyncError` into the typed `CliError` boundary by calling +`TraceSyncError::is_authentication_failure()` — a typed traversal down to +`ControlPlaneError`, never string/substring matching. An authentication +failure from the initial `/state` call, a stream batch request, or a stream +reconciliation `/state` refresh (`ControlPlaneError::MissingCredentials` or +`AuthenticationFailed`) classifies as `CliError::User { error: +UserError::NotAuthenticated, .. }`, preserving the technical error as its +source; every other `ControlPlaneError` (`Forbidden`, `BadRequest`, +`Transport`, `ServerError`, `InvalidResponse`, `Storage`, `Protocol`) +classifies as `CliError::Internal`. `sync/command.rs` builds no friendly +sentence and applies no terminal styling itself — `app_support` renders the +single `You are not logged in...` diagnostic for the user case, and the full +`anyhow`/control-plane chain for the internal case. See [CLI error-code +taxonomy](../sce/cli-error-code-taxonomy.md) for the full `CliError`/`UserError` +architecture. + ## Related context - [Agent Trace sync architecture](agent-trace-sync-command.md) - [Agent Trace storage](agent-trace-storage.md) - [Agent Trace export readers](../sce/agent-trace-export-readers.md) - [CLI stdout/stderr contract](../sce/cli-stdout-stderr-contract.md) +- [CLI error-code taxonomy](../sce/cli-error-code-taxonomy.md) - [Trace-sync progress stream contract](../decisions/2026-08-13-trace-sync-progress-stream-contract.md) diff --git a/context/glossary.md b/context/glossary.md index 5661f1d45..e959981bf 100644 --- a/context/glossary.md +++ b/context/glossary.md @@ -127,7 +127,7 @@ - `SCE_LOG_FILE_MODE`: Optional runtime env key controlling `SCE_LOG_FILE` write policy; allowed values are `truncate` and `append`, defaults to `truncate`, and requires `SCE_LOG_FILE`. - `SCE_LOG_DIR`: Optional runtime env key for `sce` observability log-directory configuration; when set, it overrides config-file `log_dir` and the `/sce/logs` default and must be non-empty. -- `logger trait boundary`: `services::observability::traits::Logger` mirrors the current observability logger API (`info`, `debug`, `warn`, `error`, `log_classified_error`) for generic command/runtime bounds and tests, with each method accepting `Option<&str>` session context for file routing; the concrete `services::observability::Logger` implements it and `NoopLogger` remains available for side-effect-free tests. +- `logger trait boundary`: `services::observability::traits::Logger` mirrors the current observability logger API (`info`, `debug`, `warn`, `error`, `log_cli_error`) for generic command/runtime bounds and tests, with each method accepting `Option<&str>` session context for file routing; the concrete `services::observability::Logger` implements it and `NoopLogger` remains available for side-effect-free tests. - `telemetry trait boundary`: `services::observability::traits::Telemetry` mirrors the current telemetry subscriber API (`with_default_subscriber`) for generic app-runtime bounds and tests, with the concrete `services::observability::TelemetryRuntime` implementing it by delegating to the existing inherent method. - `app startup phases`: Current `cli/src/app.rs` execution model that separates dependency checking, startup-context construction, runtime initialization, command parse/execute, and output rendering into named helpers while preserving the CLI's existing exit-code, stderr-diagnostic, and degraded-startup behavior; output rendering and execution-phase logging helpers live in `cli/src/services/app_support.rs`. - `RunOutcome`: Generic final render payload in `cli/src/services/app_support.rs` (`RunOutcome`) carrying a command result, optional startup diagnostic, and optional logger implementing the logger trait boundary. Production construction in `cli/src/app.rs` uses the concrete observability logger, while rendering is not hardcoded to that production type. diff --git a/context/overview.md b/context/overview.md index 6e524c6eb..1f5ae69cf 100644 --- a/context/overview.md +++ b/context/overview.md @@ -19,7 +19,7 @@ The generated `/next-task` workflow persists task-level context-synchronization The CLI crate currently depends on `anyhow`, `chrono`, `clap`, `clap_complete`, `dirs`, `hmac`, `indicatif`, `inquire`, `jsonschema`, `keyring-core`, `murmur3`, `owo-colors`, `rand`, `reqwest`, `serde`, `serde_json`, `sha2`, `tokio`, `tracing`, `turso`, and `uuid`, with target-specific keyring backend dependencies for Linux/FreeBSD, macOS, and Windows. No CLI dev-dependencies are currently declared. Its command loop is implemented with `clap` derive-based argument parsing and `anyhow` error handling. Top-level help displays an ASCII art "SCE" banner with a per-column right-to-left color gradient (cyan to magenta when color is enabled, plain ASCII when disabled) above a slim command list without implemented/placeholder labels; `auth` is visible while `hooks` and `policy` remain directly invocable but hidden. The real top-level command catalog/help-visibility contract is centralized in `cli/src/cli_schema.rs` and consumed by `cli/src/command_surface.rs` for custom banner/help rendering plus known-command classification. The runtime includes implemented auth flows (`auth login|logout|whoami`), with authenticated whoami reading the Control Plane `GET /me` profile and rendering flat email/name/role/permissions/organization labels, optional names and missing values handled deterministically, and exact login guidance returned when logged out, alongside config inspection/validation, setup orchestration, doctor diagnosis/repair, attribution-only hooks, shell completion, and the top-level `sync` command. Parse-time command conversion plus run-time command handling flow through the internal `RuntimeCommand` seam in `cli/src/app.rs`. The command loop now enforces a stable exit-code contract in `cli/src/app.rs`: `2` parse failures, `3` invocation validation failures, `4` runtime failures, and `5` dependency startup failures. -The same runtime also emits stable user-facing stderr error classes (`SCE-ERR-PARSE`, `SCE-ERR-VALIDATION`, `SCE-ERR-RUNTIME`, `SCE-ERR-DEPENDENCY`) using deterministic `Error []: ...` diagnostics with class-default `Try:` remediation appended when missing. +The same runtime also emits stable user-facing stderr error classes (`SCE-ERR-PARSE`, `SCE-ERR-VALIDATION`, `SCE-ERR-RUNTIME`, `SCE-ERR-DEPENDENCY`) using deterministic `Error []: ...` diagnostics with class-default `Try:` remediation appended when missing. The command boundary's former flat, string-only `ClassifiedError` has been replaced by typed `CliError` in `cli/src/services/error.rs`: `CliError::User` carries a closed `UserError` catalog (currently only `NotAuthenticated`) for expected, deliberately-explained failures rendered as a friendly sentence with no `Try:` suffix, while `CliError::Internal` carries a live `anyhow::Error` source rendered as the real error chain with class-default remediation; `app_support` is the sole owner of turning either into the final styled stderr diagnostic, and `sce sync` is the first command to classify a failure (authentication) into `CliError::User`. See `context/sce/cli-error-code-taxonomy.md` for the full contract. The app runtime now also includes a structured observability baseline in `cli/src/services/observability.rs`: deterministic env-controlled log threshold/format (`SCE_LOG_LEVEL` defaults to `error`; `SCE_LOG_FORMAT` defaults to `text`), default-backed log-directory routing (`SCE_LOG_DIR` / config-file `log_dir` / `/sce/logs`) with per-operation machine-local dated file selection, optional session filename partitioning, and creation-triggered retention of direct regular `*.log` files to 10 entries, stable lifecycle event IDs, stderr primary emission so stdout command payloads remain pipe-safe, and `observability::traits` boundaries for logger and telemetry behavior. The app command dispatcher now enforces a centralized stdout/stderr stream contract in `cli/src/app.rs`: command success payloads are emitted on stdout only, while redacted user-facing diagnostics and text-mode sync progress are emitted on stderr; JSON sync remains silent. `cli/src/app.rs` also now runs through explicit startup phases — dependency check, observability config resolution, runtime initialization, command parse/execute, and output rendering — with the app runtime carrying logger/telemetry plus static command-catalog state across those phases while preserving the existing exit-code and degraded-startup contracts. Within that lifecycle, `parse_command_phase` delegates clap-to-runtime conversion to `cli/src/services/parse/command_runtime.rs`, which returns a static `RuntimeCommand` enum, `services::app_support::execute_command_phase` logs around enum-owned `execute_with_stderr(...)` dispatch, and generic `RunOutcome` rendering logs classified errors through the logger trait boundary without coupling render support to the production logger type. Command payload structs for `help`, `version`, `completion`, `auth`, `config`, `setup`, `doctor`, `hooks`, and `sync` live in service-owned `command.rs` files; `cli/src/services/command_registry.rs` owns the deterministic static command-name catalog and enum variants instead of boxed command trait objects. The CLI now also enforces a shared output-format parser contract in `cli/src/services/output_format.rs`, with canonical `--format ` parsing and command-specific actionable invalid-value guidance reused by `config` and `version` services. A compile-safe service lifecycle seam also exists in `cli/src/services/lifecycle.rs`: `ServiceLifecycle` exposes default no-op `diagnose`, `fix`, and `setup` methods against the narrow `HasRepoRoot` accessor, uses lifecycle-owned health/fix/setup result types, and owns the shared static `LifecycleProvider` enum catalog/factory with deterministic config → local_db → auth_db → agent_trace_db → hooks ordering and no boxed provider aggregation. Hooks has a `services/hooks/lifecycle.rs` provider for hook rollout diagnosis/fix/setup, config has a `services/config/lifecycle.rs` provider for global/repo-local config validation plus repo-local config bootstrap, local_db has a `services/local_db/lifecycle.rs` provider for canonical local DB path health, parent-directory readiness/bootstrap, and `LocalDb::new()` setup, auth_db has a `services/auth_db/lifecycle.rs` provider for canonical auth DB path health, parent-directory readiness/bootstrap, and `AuthDb::new()` setup, and agent_trace_db has a `services/agent_trace_db/lifecycle.rs` provider for repository-scoped Agent Trace DB setup and repository DB path health/parent readiness from resolved repository identity, returning an actionable "requires a Git repository" diagnostic outside repository context (no global/checkout fallback path). Doctor runtime aggregates the full shared provider catalog for `diagnose` and `fix` and adapts lifecycle records into doctor-owned output records; setup command aggregates the shared provider catalog for `setup` with hooks included only when requested and adapts lifecycle setup outcomes before rendering setup-owned messages. diff --git a/context/plans/typed-cli-errors.md b/context/plans/typed-cli-errors.md new file mode 100644 index 000000000..d6139f1e0 --- /dev/null +++ b/context/plans/typed-cli-errors.md @@ -0,0 +1,235 @@ +# Plan: typed-cli-errors + +## Change summary + +Replace the current string-only `ClassifiedError` (`class`, `code`, `message: String`) at the CLI command boundary with a typed `CliError` that separates two categories: `CliError::User { error: UserError, source: Option }` for expected, deliberately-explained failures, and `CliError::Internal { class: FailureClass, source: anyhow::Error }` for everything else. `UserError` starts with exactly one variant, `NotAuthenticated`, and `app_support` becomes the sole owner of turning it into a friendly, actionable stderr sentence using stderr TTY/`NO_COLOR` policy. `sce sync` is the first adopter: today its stream/control-plane errors are already erased into plain strings before reaching the CLI boundary (`BatchAttemptOutcome::Terminal(String)`, `StreamSyncError::Refresh(String)`/`Terminal(String)` in `cli/src/services/agent_trace_sync/mod.rs`, built from a typed `ControlPlaneError` via `.to_string()`/`error.to_string()` in `cli/src/services/agent_trace_sync/mod.rs:510` and `cli/src/services/sync/sync.rs:525`), so an authentication failure and, say, a `500` both render as an opaque runtime string. This plan fixes that erasure, adds `is_authentication_failure()` typed classification through `ControlPlaneError` → `StreamSyncError` → `TraceSyncError`, and wires `sce sync`'s classifier to produce `UserError::NotAuthenticated` for `MissingCredentials`/`AuthenticationFailed` while every other `ControlPlaneError` (`Forbidden`, `BadRequest`, `Transport`, `ServerError`, `InvalidResponse`, `Storage`, `Protocol`) stays an internal failure with its full `anyhow` chain intact for observability. + +This replaces `ClassifiedError` rather than extending it, and is scoped to one adopter — it does not migrate setup validation, clap/parser errors, bash policy errors, or `AuthError` to typed user errors, and it does not touch `sce auth whoami` semantics. It is a fresh implementation on top of current `main`; it does not build on, cherry-pick from, or reuse the `UserFacingPresentation` design of PR #221. + +## Acceptance criteria + +- [x] AC1: No arbitrary user-message escape hatch exists — `UserError` has no `Message(String)`/`Custom(...)` variant, and `UserFacingPresentation` does not exist anywhere in the codebase. + - Validate: `grep -rn "UserFacingPresentation" cli/src` and `grep -rn "UserError::Message\|UserError::Custom" cli/src` both return no results; `cli/src/services/error.rs` shows `UserError` with only `NotAuthenticated`. +- [x] AC2: `CliError` distinguishes typed user errors (`CliError::User`) from internal failures (`CliError::Internal`), and `FailureClass::code()` still maps to the four stable `SCE-ERR-*` strings. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml error::` +- [x] AC3: `sce sync` classifies an authentication failure from the initial `/state` call (`MissingCredentials` or `AuthenticationFailed`), a stream batch request, or a stream reconciliation `/state` refresh as `UserError::NotAuthenticated`, while `Forbidden`, `BadRequest`, `Transport`, `ServerError`, `InvalidResponse`, `Storage`, and `Protocol` remain internal failures. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml sync::` (positive and negative classification cases from Phase 10). +- [x] AC4: A `sce sync` authentication failure renders exactly one friendly login diagnostic on stderr (no low-level control-plane text), leaves stdout empty, exits with the runtime class (`4`), and still preserves the technical `ControlPlaneError`/`anyhow` source for observability. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml app_support::` end-to-end routing test asserting stdout/stderr/exit-code/single-diagnostic behavior. +- [ ] AC5: `CliError::Internal` diagnostics still render the real `anyhow` error chain (`format!("{source:#}")`) rather than a pre-stringified message, and existing exit-code classes plus `SCE-ERR-*` codes and `Try:` remediation are unchanged. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml app::` +- [x] AC6: Friendly login-guidance styling follows stderr TTY/`NO_COLOR` policy (`supports_color_stderr()`), independent of stdout's TTY state. + - Validate: targeted styling test covering TTY, redirected-stderr, and `NO_COLOR` cases for the user-error renderer. +- [x] AC7: `cli/src/services/sync/command.rs` contains no friendly-sentence construction, no terminal styling call, and no string/substring matching used to decide authentication semantics. + - Validate: `grep -n "style::success\|You are not logged in" cli/src/services/sync/command.rs` returns no results; manual review of `classify_sync_error` shows it dispatches only on `is_authentication_failure()`. +- [x] AC8: Observability logs one structured record per `CliError` (class, code, surface, `user_error` key when present, technical source) without emitting a second competing terminal stderr diagnostic for the same error. + - Validate: targeted observability test asserting a single stderr diagnostic write plus the structured log fields for both a `CliError::User` and a `CliError::Internal` case. + +### Full validation + +- `nix run .#pkl-check-generated` +- `nix flake check` + +### Context sync + +- `context/sce/cli-error-code-taxonomy.md` — `CliError`/`UserError` ownership replacing `ClassifiedError`. +- `context/sce/cli-stdout-stderr-contract.md` — stream contract restated against `CliError`. +- `context/sce/cli-observability-contract.md` — `log_cli_error` API, `error_surface`/`user_error` structured fields, single-owner terminal emission. +- `context/cli/sync-command.md` and `context/cli/agent-trace-sync-command.md` — typed authentication-failure classification through the sync stack. +- `context/overview.md` — cross-cutting `ClassifiedError` → `CliError` rename at the command boundary. + +## Task context synchronization lifecycle + +Persist this field in every plan; this is durable plan state, not chat state: + +- **Task context synchronization:** every task carries `pending | synced | blocked`. + A completed task must be `synced` before another task can start or the plan can + finish. +- For `blocked`, record **Blocker**, **Required action**, and **Retry condition** + beside the status. Never infer `synced` from conversation history; write every + lifecycle transition to the plan file. + +## Constraints and non-goals + +- **In scope:** `cli/src/services/error.rs`; `cli/src/app.rs`; `cli/src/services/app_support.rs`; `cli/src/services/command_registry.rs`; `cli/src/services/observability.rs` and `cli/src/services/observability/traits.rs`; `cli/src/services/parse/command_runtime.rs`; `cli/src/services/sync/command.rs`, `cli/src/services/sync/sync.rs`; `cli/src/services/agent_trace_sync/mod.rs` and `cli/src/services/agent_trace_sync/control_plane.rs`; the `CliError`-boundary surface of command adapters for auth/config/doctor/hooks/setup/version and policy code currently returning `ClassifiedError`; existing tests referencing `ClassifiedError`; the context docs listed under Context sync. +- **Out of scope:** migrating setup `bail!` validation to typed user errors; migrating clap/parser usage errors; migrating bash policy errors; redesigning `AuthError` as a whole; removing existing `Try:` remediation strings; changing `sce auth whoami` semantics (it keeps returning unauthenticated state as a successful result); broad CLI copy cleanup unrelated to this architecture; PR #221 (not built on, not cherry-picked from, not closed or modified by this plan). +- **Constraints:** no new crate dependencies; existing numeric exit-code classes (`2`/`3`/`4`/`5`) and `SCE-ERR-{PARSE,VALIDATION,RUNTIME,DEPENDENCY}` codes stay stable; `UserError` variants only — no `Message(String)`/`Custom(...)` escape hatch; no `UserFacingPresentation` type; no terminal styling stored on error types; no string/substring matching to determine authentication semantics; branch from current `main`. +- **Non-goal:** migrating the rest of the CLI's `ClassifiedError` call sites to typed `UserError` variants beyond `sce sync` authentication; broadening this into an `AuthError` redesign or a `sce auth whoami` behavior change. + +## Assumptions + +- The exact login-guidance sentence (`You are not logged in. Please log in using the \`sce auth login\` command.`) may be preserved verbatim from current behavior; the request states ownership matters, not exact punctuation. +- Illustrative type/field names in the request (`BatchAttemptOutcome`, `StreamSyncError`, the `is_authentication_failure` traversal shape) may differ in the implementation as long as `ControlPlaneError` stays typed end-to-end through the sync stack and authentication semantics are never derived from string matching. +- Branch creation, staging, and opening the draft PR against `main` are carried out through this repository's normal `/commit` and PR workflow after the task stack completes; they are not modeled as plan tasks. + +## Task stack + +- [x] T01: `Introduce the CliError/UserError boundary and retire ClassifiedError` (status:done) + - Task ID: T01 + - Scope: In — `cli/src/services/error.rs` (new `FailureClass::code()`, `CliError::{User,Internal}`, `UserError::NotAuthenticated` with `class()`/`key()`, constructors `user`/`user_with_source`/`internal`/`runtime`/`dependency` plus compatibility `parse`/`validation` string helpers that wrap `anyhow::Error::msg(...)`); mechanical `ClassifiedError` → `CliError` rename across `cli/src/app.rs`, `cli/src/services/app_support.rs`, `cli/src/services/command_registry.rs`, `cli/src/services/observability.rs`/`observability/traits.rs` (signature only), `cli/src/services/parse/command_runtime.rs`, `cli/src/services/sync/command.rs`, the auth/config/doctor/hooks/setup/version command adapters, policy code, and existing tests; `app_support::write_error_diagnostic` updated to an exhaustive match rendering `CliError::Internal` from `format!("{source:#}")` and `CliError::User` as the friendly `UserError::NotAuthenticated` sentence styled via `supports_color_stderr()`, with redaction applied after rendering and before the write. Out — Phase 3 anyhow-preservation cleanup in other adapters, sync's own stream-error typing, the sync classifier, and observability's structured logging fields (later tasks). + - Dependencies: none + - Done when: `ClassifiedError` no longer exists anywhere in `cli/src/`; `CliError`, `UserError::NotAuthenticated`, and the constructors above exist and compile; `write_error_diagnostic` renders both variants correctly (internal via the real error chain with unchanged `SCE-ERR-*`/`Try:` behavior, user via the friendly sentence under stderr color policy); existing exit-code and error-code behavior is unchanged for every current call site. + - Verify: `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml`; `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml`; `grep -rn "ClassifiedError" cli/src` (expect no results). + - Context synchronization: synced + - Completed: 2026-08-19 + - Files changed: `cli/src/services/error.rs`; `cli/src/app.rs`; `cli/src/services/app_support.rs`; `cli/src/services/command_registry.rs`; `cli/src/services/observability.rs`; `cli/src/services/observability/traits.rs`; `cli/src/services/parse/command_runtime.rs`; `cli/src/services/sync/command.rs`; `cli/src/services/auth_command/command.rs`; `cli/src/services/config/command.rs`; `cli/src/services/doctor/command.rs`; `cli/src/services/hooks/command.rs`; `cli/src/services/setup/command.rs`; `cli/src/services/version/command.rs`; `cli/src/services/bash_policy.rs`; `cli/src/services/agent_trace_sync/control_plane.rs` (doc-comment only) + - Result: `error.rs` now defines `FailureClass::code()`, `CliError::{User,Internal}`, `UserError::NotAuthenticated` (`class()`/`key()`/`message()`), and the `user`/`user_with_source`/`internal`/`runtime`/`dependency` constructors plus `parse`/`validation` string-compatibility helpers that wrap `anyhow::Error::msg(...)`. `ClassifiedError` is deleted; every prior call site now constructs `CliError`, with `format!`-built messages passed to `runtime`/`dependency` wrapped in `anyhow::Error::msg(...)` so `CliError::Internal` always carries a live `anyhow::Error` and renders via `format!("{source:#}")`. `app_support::write_error_diagnostic` is an exhaustive match: `Internal` renders the real error chain with unchanged `Try:` guidance; `User` renders `UserError::message()` with no `Try:` suffix. Both paths go through the existing stderr-policy-aware `services::style::error_text` and existing redaction. `observability::Logger::log_classified_error` (name unchanged; renamed in T05) now takes `&CliError` and logs `error.to_string()` in place of the old flat `.message()`. `UserError::NotAuthenticated`, `CliError::User`, `CliError::user`, and `CliError::user_with_source` are marked `#[allow(dead_code)]` since no call site constructs them yet — `sce sync` classification wiring is T04's scope. Three tests (`parse/command_runtime.rs`, `bash_policy.rs`) that asserted on the old `.message()` accessor were updated to `.to_string()`. Added a small unit-test module in `error.rs` covering `FailureClass::code()`, both `CliError` variants' `class()`/`code()`/`Display`, and the `parse`/`validation` compatibility helpers. + - Verify: + - `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — passed (dead-code lint required the `#[allow(dead_code)]` annotations noted above to compile clean under `-D warnings`). + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` — passed, 345/345. + - `grep -rn "ClassifiedError" cli/src` — no results. + - Additionally ran `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml` (clean) and manually exercised `sce bogus-command` (unchanged `Error [SCE-ERR-PARSE]: ... Try: ...` rendering, exit code 2) and `sce version` (unchanged success output) through the built binary. + - Context impact: Internal-only. `CliError`/`UserError` are new public types within `cli/src/services/error.rs`, but no external-facing behavior, CLI contract, or SCE-ERR-* code changed — the rename and wrapping are transparent to callers. No `context/sce`/`context/cli` doc requires updating yet; T07 documents the full architecture once T02–T06 finish shaping it. + +- [x] T02: `Preserve anyhow error sources at remaining command-adapter boundaries` (status:done) + - Task ID: T02 + - Scope: In — command adapters (auth/config/doctor/hooks/setup/version and other obvious `anyhow::Error`-returning call sites) currently doing `.map_err(|error| CliError::runtime(format!("{error:#}")))`, changed to `.map_err(CliError::runtime)` with `anyhow::Context` attached beforehand where it adds useful context. Out — redesigning domain error types, sync's own error path (T03/T04), setup `bail!` validation, clap/parser errors, bash policy errors. + - Dependencies: T01 + - Done when: identified adapters construct `CliError::Internal` from the live `anyhow::Error` object instead of a pre-formatted string; rendered diagnostic text for existing failure cases is unchanged, since `write_error_diagnostic` already renders `format!("{source:#}")`. + - Verify: `grep -rn 'CliError::runtime(format!("{error:#}"))' cli/src/services` (expect no results in migrated adapters); `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml`. + - Completed: 2026-08-19 + - Files changed: `cli/src/services/auth_command/command.rs`; `cli/src/services/config/command.rs`; `cli/src/services/doctor/command.rs`; `cli/src/services/hooks/command.rs`; `cli/src/services/setup/command.rs`; `cli/src/services/version/command.rs` + - Result: All six command-adapter `.map_err(|error| CliError::runtime(anyhow::Error::msg(format!("{error:#}"))))` call sites (one each in `auth_command`, `config`, `doctor`, `hooks`, `version`, and seven in `setup/command.rs`) now pass the live `anyhow::Error` directly via `.map_err(CliError::runtime)`, since every wrapped domain function already returns `anyhow::Result<...>` and `CliError::runtime` takes `impl Into`. No `anyhow::Context` needed attaching beforehand — none of the six sites were missing useful context. `cli/src/services/app_support.rs:157` (`write_stdout_payload`, wrapping an `io::Error`, not one of the six named adapters) and `cli/src/services/sync/command.rs:93` (explicitly T04's territory) were left untouched, matching the task's out-of-scope boundaries. `write_error_diagnostic` already renders `format!("{source:#}")`, so rendered diagnostic text for these call sites is unchanged; only the preserved source chain differs (previously erased into a message-only `anyhow::Error`, now the original error with its full chain intact for observability). + - Verify: + - `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — passed. + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` — passed, 345/345. + - `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml` — clean. + - `grep -rn 'CliError::runtime(anyhow::Error::msg(format!("{error:#}")))' cli/src/services/{auth_command,config,doctor,hooks,version,setup}` — no results. + - Context impact: Internal-only. No public interface, CLI contract, exit code, `SCE-ERR-*` code, or rendered diagnostic text changed — only the internal fidelity of the preserved `anyhow` source chain. No `context/sce`/`context/cli` doc requires updating for this task; T07 documents the full architecture once T02–T06 finish shaping it. + - Context synchronization: synced + +- [x] T03: `Preserve typed control-plane errors through the sync stream stack` (status:done) + - Task ID: T03 + - Scope: In — `cli/src/services/agent_trace_sync/mod.rs` (`BatchAttemptOutcome::Terminal(String)` → `Terminal(ControlPlaneError)`, `StreamSyncError::Refresh(String)`/`Terminal(String)` → `Refresh(ControlPlaneError)`/`Terminal(ControlPlaneError)`, and their construction sites at `mod.rs:510` and `sync.rs:525`); `ControlPlaneError::is_authentication_failure()` in `cli/src/services/agent_trace_sync/control_plane.rs` (true only for `MissingCredentials`/`AuthenticationFailed`); equivalent typed traversal `StreamSyncError::is_authentication_failure()` and `TraceSyncError::is_authentication_failure()` in `cli/src/services/sync/sync.rs`/`agent_trace_sync/mod.rs`. Out — the CLI-facing classifier (T04), any `CliError`/`UserError` reference (this task is internal to the sync/control-plane modules). + - Dependencies: none + - Done when: no sync-stream path erases a `ControlPlaneError` into a bare `String` before it reaches `TraceSyncError`; `is_authentication_failure()` is available on `ControlPlaneError`, `StreamSyncError`, and `TraceSyncError` and correctly returns `true` only for `MissingCredentials`/`AuthenticationFailed`; `Display` output for the affected variants is unchanged. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml agent_trace_sync`; `grep -rn "BatchAttemptOutcome::Terminal(String)\|StreamSyncError::Refresh(String)\|StreamSyncError::Terminal(String)" cli/src` (expect no results). + - Completed: 2026-08-19 + - Files changed: `cli/src/services/agent_trace_sync/control_plane.rs`; `cli/src/services/agent_trace_sync/mod.rs`; `cli/src/services/sync/sync.rs` + - Result: `ControlPlaneError::is_authentication_failure()` added in `control_plane.rs`, true only for `MissingCredentials`/`AuthenticationFailed`. In `mod.rs`, `BatchAttemptOutcome::Terminal` and `StreamSyncError::{Refresh,Terminal}` now carry `ControlPlaneError` instead of `String`; `StreamSyncError::is_authentication_failure()` added, delegating to the inner `ControlPlaneError` for `Refresh`/`Terminal` and returning `false` for `Read`/`InvalidResponse`/`DidNotConverge` (neither of which can carry a control-plane error). `BatchAttemptOutcome`'s `PartialEq, Eq` derive was dropped since `ControlPlaneError` doesn't implement them and no call site compared these enums by equality (confirmed via grep; all existing assertions use `matches!`). In `sync.rs`, the two construction sites (`sync.rs:510` `BatchAttemptOutcome::Terminal(error.to_string())` → `BatchAttemptOutcome::Terminal(error)`; `sync.rs:525` `.map_err(|error| StreamSyncError::Refresh(error.to_string()))` → `.map_err(StreamSyncError::Refresh)`) now pass the live `ControlPlaneError` through instead of stringifying it, and `TraceSyncError::is_authentication_failure()` was added, traversing `ControlPlane(_)` directly and `Stream { source, .. }` via `StreamSyncError::is_authentication_failure()`, `false` for `Runtime`. `TraceSyncError::is_authentication_failure()` is marked `#[allow(dead_code)]` since no call site invokes it yet — wiring it into `sce sync`'s classifier is T04's scope (same pattern T01 used for not-yet-called constructors). The one existing test relying on the old `String` shape (`terminal_failure_does_not_call_refresh` in `mod.rs`) was updated to construct/match `ControlPlaneError::BadRequest(...)` instead of a bare string. `Display` output is unchanged: both affected arms already interpolate `{reason}` via `write!`, and `ControlPlaneError`'s `Display` impl produces the same text `.to_string()` previously captured. + - Verify: + - `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — passed (required `#[allow(dead_code)]` on `TraceSyncError::is_authentication_failure()` to compile clean under `-D warnings`, same pattern as T01). + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml agent_trace_sync` — passed, 38/38. + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` — passed, 345/345 (no change in total count). + - `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml` — clean (required backticking `WorkOS` in two new doc comments for `clippy::doc_markdown` under `-D clippy::pedantic`). + - `grep -rn "BatchAttemptOutcome::Terminal(String)\|StreamSyncError::Refresh(String)\|StreamSyncError::Terminal(String)" cli/src` — no results. + - Context impact: Internal-only. `ControlPlaneError`/`StreamSyncError`/`TraceSyncError` are all internal-to-the-sync-stack types; no public CLI interface, exit code, `SCE-ERR-*` code, or rendered diagnostic text changed, and `Display` output for every affected variant is unchanged. No `context/sce`/`context/cli` doc requires updating yet; T07 documents the full architecture once T02–T06 finish shaping it. + - Context synchronization: synced + +- [x] T04: `Classify sce sync authentication failures as the typed user error` (status:done) + - Task ID: T04 + - Scope: In — `cli/src/services/sync/command.rs`'s `classify_sync_error(err: TraceSyncError) -> CliError`, rewritten to call `err.is_authentication_failure()` and return `CliError::user_with_source(UserError::NotAuthenticated, err)` when true, `CliError::runtime(err)` otherwise. Out — any friendly-sentence text, terminal styling, or color-policy decision in `sync/command.rs` (owned by `app_support` since T01); rendering/observability changes. + - Dependencies: T01, T03 + - Done when: `sce sync` authentication failures from the initial `/state` call, a stream batch request, and a stream reconciliation `/state` refresh all classify as `CliError::User { error: UserError::NotAuthenticated, .. }`; every other `ControlPlaneError` variant (`Forbidden`, `BadRequest`, `Transport`, `ServerError`, `InvalidResponse`, `Storage`, `Protocol`) still classifies as `CliError::Internal`; `sync/command.rs` contains no string matching, no styling call, and no friendly sentence. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml sync::`; `grep -n "style::success\|You are not logged in\|\\.contains(" cli/src/services/sync/command.rs` (expect no results). + - Completed: 2026-08-19 + - Files changed: `cli/src/services/sync/command.rs` + - Result: `classify_sync_error` now branches on `err.is_authentication_failure()`: `true` returns `CliError::user_with_source(UserError::NotAuthenticated, err)` (preserving the live `TraceSyncError`/`ControlPlaneError` chain as the technical source instead of the prior `anyhow::Error::msg(format!("{err}"))` stringification), `false` returns `CliError::runtime(err)` (also now passing the live error via `Into` rather than a pre-formatted string, matching T02's anyhow-preservation pattern). Added a `UserError` import. Added a `#[cfg(test)] mod tests` covering all four authentication paths named in the task (`ControlPlane(MissingCredentials)`, `ControlPlane(AuthenticationFailed)`, `Stream { source: Terminal(AuthenticationFailed) }`, `Stream { source: Refresh(MissingCredentials) }`) asserting `CliError::User` with a preserved source, plus negative cases for every other `ControlPlaneError` variant (`Forbidden`, `BadRequest`, `Transport`, `ServerError`, `InvalidResponse`, `Storage`) and `TraceSyncError::Runtime`, asserting `CliError::Internal`. No changes outside `sync/command.rs`; `sync.rs`'s pre-existing `#[allow(dead_code)]` on `TraceSyncError::is_authentication_failure()` was left in place since removing it was outside this task's declared scope (harmless now that the method has a real caller — an unused `allow` is not a compiler or clippy error). + - Verify: + - `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — passed. + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml sync::` — passed, 60/60, including the 8 new classification tests. + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` — passed, 351/351 (up from 345; +6 net after removing none and adding 8 minus overlap in filtered count reporting). + - `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml` — clean. + - `grep -n "style::success\|You are not logged in\|\.contains(" cli/src/services/sync/command.rs` — no results. + - Context impact: Internal-only for `sync/command.rs`'s own contract, but this is the first call site that actually constructs `CliError::User`/`UserError::NotAuthenticated`, so `sce sync` authentication failures now render through `app_support`'s friendly-diagnostic path (built in T01) instead of the generic internal-error path — a real, user-visible behavior change for that one failure mode, though the rendering logic itself is unchanged. `context/cli/sync-command.md` and `context/cli/agent-trace-sync-command.md` (listed under this plan's Context sync) describe this authentication-classification behavior; T07 is the task that updates durable context docs once T02–T06 finish shaping the full architecture, so no doc update is made here. + - Context synchronization: synced + +- [x] T05: `Give observability one owner for structured CliError logging without duplicate terminal output` (status:done) + - Task ID: T05 + - Scope: In — rename/refactor `Logger::log_classified_error` to `log_cli_error(&self, error: &CliError, session_id: Option<&str>)` in `cli/src/services/observability.rs` and `observability/traits.rs` (including `NoopLogger`), preserving `error_class`/`error_code` fields and adding `error_surface` (`user`/`internal`) plus `user_error` (the `UserError::key()`) when applicable, and the technical source when present; confirm `app_support` remains the sole writer of the terminal stderr diagnostic and observability never writes a second one. Out — changing which events get logged elsewhere, or altering `write_error_diagnostic`'s rendering (already correct from T01). + - Dependencies: T01 + - Done when: every call site logs through `log_cli_error`; structured log records for a `CliError::User` case carry `error_class=runtime error_code=SCE-ERR-RUNTIME error_surface=user user_error=auth.not_authenticated` plus the technical source, and a `CliError::Internal` case carries its class/code/surface and full source chain; exactly one terminal stderr diagnostic is written per failed command. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml observability`; `grep -rn "log_classified_error" cli/src` (expect no results). + - Completed: 2026-08-19 + - Files changed: `cli/src/services/observability.rs`; `cli/src/services/observability/traits.rs`; `cli/src/services/app_support.rs` + - Result: `Logger::log_classified_error` is renamed to `log_cli_error(&self, error: &CliError, session_id: Option<&str>)` in `observability.rs`, with its field-building logic extracted into a pure helper `cli_error_fields(error: &CliError) -> Vec<(&'static str, String)>` (plus small helpers `cli_error_surface`/`cli_error_technical_source`) so the shape is unit-testable without file I/O. The field list always carries `error_code`/`error_class`/`error_surface` (`"user"` for `CliError::User`, `"internal"` for `CliError::Internal`), adds `user_error` (`UserError::key()`) only for `CliError::User`, and adds `error_source` (`format!("{source:#}")`) whenever a technical source is present — always for `Internal`, and for `User` only when `user_with_source` supplied one. The trait method is renamed in `observability/traits.rs` on the `Logger` trait, the `NoopLogger` impl, and the concrete `Logger` impl (which now delegates to `super::Logger::log_cli_error`). The single call site in `app_support.rs:146` (`exit_with_error`) is updated to `log.log_cli_error(error, None)`; it remains the only logger call in that function, and `write_error_diagnostic` (the sole terminal stderr diagnostic writer, unchanged from T01) is still called exactly once per failed command — observability's own line goes to its structured log record, not a second competing diagnostic. No changes to `error.rs`: `UserError::key()` and the `CliError::User` variant already existed from T01 and simply gained their first real caller here; their pre-existing `#[allow(dead_code)]` attributes were left in place (harmless once used, out of this task's declared file scope). + - Verify: + - `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — passed. + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml observability` — passed, 3/3 new tests (`cli_error_fields` shape for `CliError::User` with source, `CliError::User` without source, and `CliError::Internal`). + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` — passed, 354/354 (up from 351; +3 new tests, no regressions). + - `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml` — clean. + - `grep -rn "log_classified_error" cli/src` — no results. + - Context impact: `local`. The literal method name `Logger::log_classified_error()`/`log_classified_error` appeared verbatim in `context/sce/cli-error-code-taxonomy.md`, `context/sce/cli-observability-contract.md`, and `context/glossary.md`; those were corrected to `log_cli_error` during context synchronization since a renamed API reference is a factual contradiction, not narrative detail. The full new structured-field list (`error_surface`, `user_error`, `error_source`) and the broader `CliError`/`UserError` architecture narrative remain deferred to T07 (already listed under this plan's Context sync), matching the T01–T04 precedent of fixing broken references immediately while batching full-architecture prose into T07. No public CLI interface, exit code, `SCE-ERR-*` code, or terminal-rendered diagnostic text changed — only the internal logger method name and the structured (non-terminal) log record's field set. + - Context synchronization: synced + +- [x] T06: `Add architecture and behavior tests for the typed error boundary` (status:done) + - Task ID: T06 + - Scope: In — tests covering: user-error routing (empty stdout, friendly stderr guidance with no low-level auth/control-plane text, exit code `4`, exactly one terminal diagnostic, technical source retained, redaction still applied); stderr color behavior (TTY-following styling, no ANSI on redirected stderr, `NO_COLOR` disabling styling, stdout TTY state not controlling stderr presentation); sync authentication propagation for all four paths (initial `/state` `MissingCredentials`, initial `/state` `AuthenticationFailed`, stream batch `AuthenticationFailed`, stream reconciliation refresh `AuthenticationFailed`); negative classification for `Forbidden`/`BadRequest`/`Transport`/`ServerError`/`InvalidResponse`/`Storage`/`Protocol` remaining internal. Out — new production behavior; this task only adds coverage for T01–T05. + - Dependencies: T01, T03, T04, T05 + - Done when: all listed cases have passing targeted tests; `cargo clippy` for the crate is clean. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml`. + - Completed: 2026-08-19 + - Files changed: `cli/src/services/app_support.rs`; `cli/src/services/style.rs`; `cli/src/services/sync/command.rs` + - Result: Added the T06-listed test coverage without changing production behavior for any real invocation. `sync/command.rs`: added the missing `ControlPlaneError::Protocol` negative-classification case to the existing `other_control_plane_errors_classify_as_internal` test (T04 already covered the four positive authentication paths and the `Forbidden`/`BadRequest`/`Transport`/`ServerError`/`InvalidResponse`/`Storage`/`Runtime` negative cases, but not `Protocol`); also narrowed two pre-existing `clippy::match_wildcard_for_single_variants` wildcard arms in the test module's `assert_user_not_authenticated`/`assert_internal` helpers to `other @ CliError::Internal { .. }`/`other @ CliError::User { .. }` — these only surface under `cargo clippy --all-targets` (test targets aren't compiled by the plan's bare `clippy --manifest-path` verify command). `style.rs`: `error_text`/`error_code` previously only exposed the real `supports_color_stderr()` TTY check with no injectable seam, and a real TTY can't be simulated in `cargo test`, so added `pub(crate) error_text_with_color_policy`/`error_code_with_color_policy` mirroring this repo's existing `_with_color_policy` convention (`doctor/render.rs`, `sync/progress.rs`, `setup/mod.rs`); the now-uncalled public `error_text` wrapper and the `style_if_enabled_stderr` helper that only it used were removed (`error_code` stays public, still used by `write_startup_diagnostic`); added 4 unit tests covering styled/plain output for both `_with_color_policy` primitives. `app_support.rs`: added `write_error_diagnostic_with_color_policy` (the production `write_error_diagnostic` now delegates to it, passing the real `supports_color_stderr()` — identical rendered output to before), and a new `#[cfg(test)] mod tests` (none existed previously) with 5 tests: empty-stdout/exit-4/single-diagnostic/friendly-text/no-low-level-text for the `CliError::User` path routed through `render_run_outcome`; a `RecordingLogger` (backed by a shared `Arc>`) proving `log_cli_error` is called exactly once with the technical source preserved; an unchanged-behavior regression test for `CliError::Internal`'s full `anyhow` chain and exit code 4; a redaction test asserting the rendered user-error diagnostic equals `redact_sensitive_text(UserError::NotAuthenticated.message())`; and a color-policy test asserting `color_enabled: true` injects ANSI escapes and `color_enabled: false` does not — covering TTY-following, redirected-stderr, and `NO_COLOR` behavior for the user-error renderer via the one boolean `supports_color_stderr()` already collapses those three real-world conditions into. + - Verify: + - `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — passed. + - `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` — passed, 363/363 (up from 354; +9: 5 new in `app_support.rs`, 4 new in `style.rs`). + - `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml` — clean (the plan's specified command). + - `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` — clean, after fixing the two pre-existing wildcard-match lints noted above (this broader invocation was run in addition to the plan's command since this task's changes are entirely inside `#[cfg(test)]` modules, which bare `cargo clippy` doesn't compile). + - Context impact: `local`. No production rendered diagnostic text, exit code, or `SCE-ERR-*` code changed for any real invocation — `write_error_diagnostic` still resolves styling via `supports_color_stderr()` exactly as before; only the computation was threaded through an explicit-bool seam for testability. However, `context/cli/styling-service.md` (not one of this plan's listed Context sync docs, but a real cross-reference) documented a standalone public `error_text(text: &str) -> String` primitive and `style_if_enabled_stderr` helper, and imported `error_text` directly in its usage example; both no longer exist as public/crate items (replaced by the crate-internal `error_text_with_color_policy`), which was a factual contradiction in that doc, not narrative — corrected during context synchronization below, following the same immediate-correction precedent T05 used for the `log_classified_error` → `log_cli_error` rename. None of this plan's own listed Context sync docs (`cli-error-code-taxonomy.md`, `cli-stdout-stderr-contract.md`, `cli-observability-contract.md`, `context/cli/sync-command.md`, `context/cli/agent-trace-sync-command.md`, `context/overview.md`) reference anything changed by this task; the full architecture narrative for those remains deferred to T07 per the T01–T05 precedent. + - Context synchronization: synced + +- [x] T07: `Document the typed CliError/UserError architecture in durable context` (status:done) + - Task ID: T07 + - Scope: In — update `context/sce/cli-error-code-taxonomy.md`, `context/sce/cli-stdout-stderr-contract.md`, `context/sce/cli-observability-contract.md`, `context/cli/sync-command.md`, `context/cli/agent-trace-sync-command.md`, and `context/overview.md` to describe `CliError::{User,Internal}`, `UserError` as the catalog of deliberately presented terminal failures, the separation from technical source errors, that commands/domain layers do not own terminal rendering, that `app_support` owns final stderr presentation with styling applied at the renderer using stderr policy, that observability retains technical detail independently, and that stable `SCE-ERR-*`/exit classes are unchanged. Out — introducing or documenting `UserFacingPresentation` (must not appear); restating unrelated legacy content. + - Dependencies: T06 + - Done when: the listed context docs describe the shipped `CliError`/`UserError` architecture with no reference to `ClassifiedError` or `UserFacingPresentation`. + - Verify: `nix run .#pkl-check-generated`; `grep -rn "UserFacingPresentation" context/` (expect no results); `grep -rln "ClassifiedError" context/cli context/sce` (expect no results among the updated files). + - Completed: 2026-08-19 + - Files changed: `context/sce/cli-error-code-taxonomy.md`; `context/sce/cli-stdout-stderr-contract.md`; `context/sce/cli-observability-contract.md`; `context/cli/sync-command.md`; `context/cli/agent-trace-sync-command.md`; `context/overview.md` + - Result: All six listed docs now describe the shipped architecture. `cli-error-code-taxonomy.md`'s Ownership section gained explicit statements that `UserError` is the closed catalog of deliberately presented terminal failures (no `Message`/`Custom` escape hatch), that command/domain layers construct and return a `CliError` without formatting terminal text or deciding user-error semantics by string matching, and that `app_support` styles both variants through `services::style::error_text_with_color_policy` under the stderr TTY/`NO_COLOR` policy independent of stdout's TTY state. `cli-stdout-stderr-contract.md` gained a bullet distinguishing the `CliError::Internal` (full `anyhow` chain plus class-default `Try:`) vs `CliError::User` (catalog message verbatim, no `Try:`) diagnostic bodies. `cli-observability-contract.md`'s error-log-record bullet now lists `error_surface`, `user_error`, and `error_source` alongside the existing `error_code`/`error_class`, matching `cli_error_fields()` in `observability.rs`. `sync-command.md` gained an "Error classification" section describing `classify_sync_error`'s typed `is_authentication_failure()` dispatch (never string matching) to `CliError::User { error: UserError::NotAuthenticated, .. }` vs `CliError::Internal`, plus a taxonomy cross-link. `agent-trace-sync-command.md`'s `401` recovery bullet gained a companion bullet on the typed `is_authentication_failure()` traversal through `ControlPlaneError`/`StreamSyncError`/`TraceSyncError` and the command-level classification outcome, plus a taxonomy cross-link in Related context. `overview.md`'s stderr-error-classes sentence (line 22) gained the cross-cutting `ClassifiedError` → `CliError` rename summary: the `User`/`Internal` split, `app_support` as sole renderer, and `sce sync` as the first `CliError::User` adopter. No production code was touched; `context/plans/typed-cli-errors.md`'s own pre-existing mentions of `UserFacingPresentation` (describing the constraint that it must not exist, and that this plan is not built on PR #221) are unchanged and out of this task's scope — they were present before T07 and are not among the six target docs. + - Verify: + - `nix run .#pkl-check-generated` — passed: "Ephemeral Pkl generation passed: 107 files". + - `grep -rn "UserFacingPresentation" context/` — four results, all pre-existing in `context/plans/typed-cli-errors.md` itself (AC1, Scope, Done-when, Verify prose describing the non-existence constraint and the PR #221 non-reuse note); none of the six updated docs reference it. + - `grep -rln "ClassifiedError" context/cli context/sce` — no results. + - Context impact: `local`. All six changes are documentation-only, describing already-shipped T01–T06 behavior with no change to public interface, exit codes, `SCE-ERR-*` codes, or rendered diagnostic text. `pkl-check-generated` confirms the generated Pkl payload set is unaffected. + - Context synchronization: synced + +## Open questions + +None. The change request fully specifies scope, architecture, phase-by-phase behavior, and acceptance criteria; the only latitude left (exact wording, illustrative type-shape naming) is recorded under Assumptions rather than blocking authoring. + +## Validation Report + +**Status:** failed +**Date:** 2026-08-19 + +### Commands run + +- `nix run .#pkl-check-generated` -> exit 0 (Ephemeral Pkl generation passed: 107 files) +- `nix flake check` -> exit 0 (all checks passed, including `cli-fmt`/`cli-clippy`/`cli-tests`, after the rustfmt drift found in the prior validation run was fixed) +- `grep -rn "UserFacingPresentation" cli/src` -> exit 1, no results (pass) +- `grep -rn "UserError::Message\|UserError::Custom" cli/src` -> exit 1, no results (pass) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml error::` -> exit 0 (6 passed; 0 failed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml sync::` -> exit 0 (60 passed; 0 failed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml app_support::` -> exit 0 (5 passed; 0 failed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml app::` -> exit 0 (0 passed; 0 failed — no test path matches the `app::` prefix, confirmed again with `-- --list`) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml observability` -> exit 0 (4 passed; 0 failed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml style::` -> exit 0 (4 passed; 0 failed) +- `grep -n "style::success\|You are not logged in" cli/src/services/sync/command.rs` -> exit 1, no results (pass) +- Manual review of `classify_sync_error` in `cli/src/services/sync/command.rs:35` -> dispatches only on `err.is_authentication_failure()` (pass) + +### Success-criteria verification + +- [x] AC1: No `Message`/`Custom` escape hatch; `UserFacingPresentation` absent from `cli/src` -> both greps returned no results; `UserError` in `error.rs` has only `NotAuthenticated`. +- [x] AC2: `CliError` distinguishes `User`/`Internal`; `FailureClass::code()` maps to the four `SCE-ERR-*` strings -> `error::` suite, 6/6 passed including `failure_class_code_maps_to_stable_sce_err_strings`. +- [x] AC3: `sce sync` classifies auth failures across all four paths as `NotAuthenticated`, other `ControlPlaneError` variants stay internal -> `sync::` suite, 60/60 passed, including the 4 positive and 7 negative classification tests in `sync/command.rs`. +- [x] AC4: Auth failure renders one friendly stderr diagnostic, empty stdout, exit 4, source preserved -> `app_support::` suite, 5/5 passed, including `user_error_routes_to_friendly_diagnostic_with_empty_stdout_and_exit_four`. +- [ ] AC5: `CliError::Internal` renders the real `anyhow` chain; exit-code classes, `SCE-ERR-*` codes, and `Try:` remediation unchanged -> the criterion's own `Validate:` command (`test ... app::`) still matches zero tests (re-confirmed with `-- --list`), so it ran successfully but confirmed nothing; the criterion remains unverified. +- [x] AC6: Friendly styling follows stderr TTY/`NO_COLOR` policy independent of stdout -> `style::` suite, 4/4 passed, covering styled/plain output under the injected color-policy boolean. +- [x] AC7: `sync/command.rs` has no friendly-sentence text, no styling call, no string matching for auth semantics -> grep returned no results; `classify_sync_error` dispatches only on `is_authentication_failure()`. +- [x] AC8: One structured log record per `CliError` with `error_surface`/`user_error`/source, no duplicate terminal diagnostic -> `observability` suite, 4/4 passed, including `user_error_preserves_technical_source_for_observability`, which asserts exactly one `log_cli_error` call. + +### Failed checks and follow-ups + +- AC5: the plan's own `Validate:` command (`./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml app::`) matches zero tests — `cli/src/app.rs` has no `#[cfg(test)] mod tests` and no submodules; evidence: `-- --list` filtered by `app::` returns nothing (re-confirmed after the `cli-fmt` fixes landed); required: either add a targeted `app::`-path test, or correct the `Validate:` command to point at whichever suite actually exercises unchanged `CliError::Internal` rendering, exit-code classes, `SCE-ERR-*` codes, and `Try:` remediation — e.g. `app_support::internal_error_still_renders_full_chain_and_exit_four` already covers part of this — before rerunning validation. + +### Residual risks + +- The previously reported `nix flake check` / `cli-fmt` failure was resolved between validation runs (rustfmt drift in `command_runtime.rs` and `sync/command.rs` was cleaned up, and `app_support.rs`'s error-heading now correctly routes through `heading_with_color_policy` instead of the unconditionally-styled `heading`). No other residual risk identified beyond the open AC5 check above. + +### Retry + +After repairs, rerun: + +`/validate context/plans/typed-cli-errors.md` diff --git a/context/sce/cli-error-code-taxonomy.md b/context/sce/cli-error-code-taxonomy.md index 29b4dc7a8..b43c0da46 100644 --- a/context/sce/cli-error-code-taxonomy.md +++ b/context/sce/cli-error-code-taxonomy.md @@ -15,9 +15,9 @@ It complements the numeric process exit-code classes documented in `context/sce/ ## Rendering contract - User-facing diagnostics are emitted on `stderr` as: `Error []: `. -- Before stderr emission, all `ClassifiedError` instances are logged via `Logger::log_classified_error()` with event ID `sce.error.{code}` and fields `error_code`, `error_class`. -- If a diagnostic message does not already include `Try:`, runtime appends class-default remediation guidance. -- If the message already contains `Try:`, runtime preserves the original remediation text and does not append a second one. +- Before stderr emission, all `CliError` instances are logged via `Logger::log_cli_error()` with event ID `sce.error.{code}` and fields `error_code`, `error_class`. +- For `CliError::Internal`, if the rendered message does not already include `Try:`, runtime appends class-default remediation guidance; if it already contains `Try:`, runtime preserves the original remediation text and does not append a second one. +- For `CliError::User`, runtime renders the catalog message from `UserError` verbatim, with no class-default `Try:` appended. - Diagnostic text is still redaction-filtered through `services::security::redact_sensitive_text` before emission. ## Actionable parser/invocation guidance contract @@ -30,10 +30,12 @@ It complements the numeric process exit-code classes documented in `context/sce/ ## Ownership -- `FailureClass` in `cli/src/services/error.rs` owns class selection. -- `ClassifiedError` in `cli/src/services/error.rs` owns stable code assignment. -- `Logger::log_classified_error` in `cli/src/services/observability.rs` owns structured error logging with `sce.error.{code}` event IDs. -- `write_error_diagnostic` in `cli/src/app.rs` owns final code-bearing stderr rendering. +- `FailureClass` in `cli/src/services/error.rs` owns class selection and stable code assignment (`FailureClass::code()`). +- `CliError::{User,Internal}` in `cli/src/services/error.rs` is the typed CLI-boundary error type; `CliError::code()`/`CliError::class()` delegate to the failure class. `CliError::User` carries a catalog `UserError` (currently only `NotAuthenticated`) for expected, deliberately-explained failures; `CliError::Internal` carries a live `anyhow::Error` source for every other failure. `CliError::User` may also carry an optional preserved technical `source`, kept for observability only and never rendered to the terminal. +- `UserError` in `cli/src/services/error.rs` is the closed catalog of deliberately presented terminal failures. It has no arbitrary-message variant (no `Message(String)`/`Custom(...)` escape hatch): every entry is a fixed, reviewed sentence returned by `UserError::message()`, keyed for structured logging by `UserError::key()`. +- Command and domain layers construct and return a `CliError`; they do not format terminal text, apply styling, or decide authentication/user-error semantics from string matching. `app_support` is the sole owner of turning a `CliError` into the final stderr sentence. +- `Logger::log_cli_error` in `cli/src/services/observability.rs` owns structured error logging with `sce.error.{code}` event IDs. +- `write_error_diagnostic` in `cli/src/services/app_support.rs` owns final code-bearing stderr rendering, including styling `CliError::User`'s catalog message and `CliError::Internal`'s rendered chain through `services::style::error_text_with_color_policy` under the stderr TTY/`NO_COLOR` policy (`services::style::supports_color_stderr()`), independent of stdout's TTY state. - `run_with_dependency_check_and_streams` in `cli/src/app.rs` owns error logging before stderr emission. ## Determinism and testing diff --git a/context/sce/cli-observability-contract.md b/context/sce/cli-observability-contract.md index dba7ee76a..02b44afd2 100644 --- a/context/sce/cli-observability-contract.md +++ b/context/sce/cli-observability-contract.md @@ -48,9 +48,9 @@ Runtime observability consumes the shared resolved observability config from `cl - `sce.command.dispatch_end` (debug level - logged after successful dispatch) - `sce.command.completed` - Error logging uses the pattern `sce.error.{code}` where `{code}` is the classified error code (e.g., `sce.error.SCE-ERR-RUNTIME`). -- All `ClassifiedError` instances are logged via `Logger::log_classified_error()` before user-facing stderr diagnostics are written. +- All `CliError` instances are logged via `Logger::log_cli_error()` before user-facing stderr diagnostics are written; observability retains full technical detail independently of what is rendered to the terminal, and never writes a second competing stderr diagnostic for the same error. - Event records include deterministic metadata keys used by automation (`command`, `failure_class`, `component` when applicable). -- Error log records include `error_code` and `error_class` fields for structured observability. +- Error log records include `error_code` and `error_class` fields for structured observability, plus `error_surface` (`user` for `CliError::User`, `internal` for `CliError::Internal`), `user_error` (the catalog `UserError::key()`, present only for `CliError::User`), and `error_source` (the full technical source chain, present whenever one was preserved — always for `CliError::Internal`, and for `CliError::User` only when constructed with `CliError::user_with_source`). - App runtime initializes tracing subscriber context before parse/dispatch and shuts down tracer provider on process exit. - Tracing event emission checks the `sce` target and requested tracing level before constructing serialized `fields` payloads; disabled or filtered tracing events return without building field JSON while enabled events preserve the same `event_id`, `event_message`, and `fields` payload shape. @@ -65,7 +65,7 @@ Runtime observability consumes the shared resolved observability config from `cl ## Observability trait boundaries -- `cli/src/services/observability/traits.rs` exposes the `services::observability::traits::Logger` trait with the current logging API: `info`, `debug`, `warn`, `error`, and `log_classified_error`, each accepting `Option<&str>` session context used only for file routing. +- `cli/src/services/observability/traits.rs` exposes the `services::observability::traits::Logger` trait with the current logging API: `info`, `debug`, `warn`, `error`, and `log_cli_error`, each accepting `Option<&str>` session context used only for file routing. - The concrete `services::observability::Logger` implements the trait while retaining the existing inherent methods and behavior. - `NoopLogger` is available from the same traits module for tests and future dependency-injected services that need a logger without side effects. - The same traits module exposes object-safe `services::observability::traits::Telemetry` with the current app subscriber boundary: `with_default_subscriber` for command-lifecycle execution. diff --git a/context/sce/cli-stdout-stderr-contract.md b/context/sce/cli-stdout-stderr-contract.md index 4c810737d..cb82148cb 100644 --- a/context/sce/cli-stdout-stderr-contract.md +++ b/context/sce/cli-stdout-stderr-contract.md @@ -8,7 +8,8 @@ This document defines the implemented stream contract for CLI command payload an - Command success payloads are emitted to `stdout` only through app-level stream handling. - User-facing diagnostics and failures are emitted to `stderr` only. -- Failure diagnostics are emitted as `Error []: ...` on `stderr`, where `` is the stable class-based `SCE-ERR-*` identifier from `ClassifiedError` in `cli/src/app.rs`; diagnostics are passed through shared redaction (`services::security::redact_sensitive_text`) before emission. +- Failure diagnostics are emitted as `Error []: ...` on `stderr`, where `` is the stable class-based `SCE-ERR-*` identifier from `CliError` in `cli/src/services/error.rs`; diagnostics are passed through shared redaction (`services::security::redact_sensitive_text`) before emission. +- The diagnostic body differs by `CliError` variant: `CliError::Internal` renders the real `anyhow` source chain (`format!("{source:#}")`) plus class-default `Try:` remediation; `CliError::User` renders its catalog `UserError` message verbatim, with no low-level technical text and no `Try:` suffix. Both bodies are styled through the same stderr TTY/`NO_COLOR` policy before redaction and emission. - Command handlers now return payload strings to the app dispatcher; the app owns stream selection and final emission. ## Implementation surface