diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index ef475abb5..fd6ff057a 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -556,6 +556,12 @@ tauri-plugin-updater = "=2.9.0" [dev-dependencies] tempfile = "3.13" + +# `test-util` only for the test build: it is what lets a test drive Tokio's +# clock (`#[tokio::test(start_paused = true)]`) rather than really sleeping out +# a multi-second timeout. Deliberately absent from the production features. +tokio = { workspace = true, features = ["test-util"] } + insta = { version = "1", features = ["yaml", "redactions"] } assert_matches = "1" # HTTP mock server for sync adapter integration tests. diff --git a/src-tauri/crates/agent-cli/src/managed_config/file_io.rs b/src-tauri/crates/agent-cli/src/managed_config/file_io.rs index cb36fc378..553faec74 100644 --- a/src-tauri/crates/agent-cli/src/managed_config/file_io.rs +++ b/src-tauri/crates/agent-cli/src/managed_config/file_io.rs @@ -46,6 +46,33 @@ fn unique_temp_path(path: &Path) -> PathBuf { )) } +/// Create the staging file for an atomic write, owner-only. +/// +/// The mode has to be set at creation because the temp file holds the payload +/// before the caller gets a chance to tighten permissions on the destination: +/// with a 0002 umask a plain create lands at 0664, so a credential would be +/// group-readable for the whole write, and would stay that way in a temp file +/// left behind by a crash. +/// +/// There is no loose variant because nothing here wants one — every +/// destination in this module ends up 0600 anyway, via +/// [`app_paths::set_sensitive_file_permissions`]. +fn create_staging_file(path: &Path) -> std::io::Result { + let mut options = std::fs::OpenOptions::new(); + // The name carries pid + nanos, so `create_new` never collides in + // practice; it does guarantee we never inherit the mode of a file somebody + // else left at this path. + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + options.open(path) +} + +/// Crash-safe replace: write an owner-only sibling temp file, fsync, then +/// rename over the target. pub(super) fn write_file_atomic(path: &Path, bytes: &[u8]) -> Result<(), String> { if let Some(dir) = path.parent() { std::fs::create_dir_all(dir) @@ -54,7 +81,7 @@ pub(super) fn write_file_atomic(path: &Path, bytes: &[u8]) -> Result<(), String> let tmp = unique_temp_path(path); let result = (|| { - let mut file = std::fs::File::create(&tmp) + let mut file = create_staging_file(&tmp) .map_err(|err| format!("Failed to create {}: {err}", tmp.display()))?; use std::io::Write; file.write_all(bytes) diff --git a/src-tauri/crates/agent-cli/src/managed_config/generators.rs b/src-tauri/crates/agent-cli/src/managed_config/generators.rs index bf757b027..5eb904c82 100644 --- a/src-tauri/crates/agent-cli/src/managed_config/generators.rs +++ b/src-tauri/crates/agent-cli/src/managed_config/generators.rs @@ -65,11 +65,67 @@ pub(super) fn generate_codex_managed_config( "wire_api".to_string(), toml::Value::String("responses".to_string()), ); + orgii.insert( + "supports_websockets".to_string(), + toml::Value::Boolean(false), + ); + orgii.insert( + "request_max_retries".to_string(), + toml::Value::Integer(super::CODEX_REQUEST_MAX_RETRIES), + ); + orgii.insert( + "stream_max_retries".to_string(), + toml::Value::Integer(super::CODEX_STREAM_MAX_RETRIES), + ); providers.insert(ORGII_PROVIDER_ID.to_string(), toml::Value::Table(orgii)); toml::to_string_pretty(&config).map_err(|err| format!("TOML serialize error: {err}")) } +pub(super) fn generate_codex_hosted_profile(proxy_url: &str) -> Result { + let proxy_url = proxy_url.trim(); + if proxy_url.is_empty() { + return Err("Codex hosted profile requires a proxy URL".to_string()); + } + + let base_url = format!("{}/v1", proxy_url.trim_end_matches('/')); + let mut provider = toml::map::Map::new(); + provider.insert("name".to_string(), toml::Value::String("Proxy".to_string())); + provider.insert("base_url".to_string(), toml::Value::String(base_url)); + provider.insert( + "env_key".to_string(), + toml::Value::String("PROXY_TOKEN".to_string()), + ); + provider.insert( + "requires_openai_auth".to_string(), + toml::Value::Boolean(false), + ); + provider.insert( + "wire_api".to_string(), + toml::Value::String("responses".to_string()), + ); + provider.insert( + "supports_websockets".to_string(), + toml::Value::Boolean(false), + ); + provider.insert( + "request_max_retries".to_string(), + toml::Value::Integer(super::CODEX_REQUEST_MAX_RETRIES), + ); + provider.insert( + "stream_max_retries".to_string(), + toml::Value::Integer(super::CODEX_STREAM_MAX_RETRIES), + ); + + let mut providers = toml::map::Map::new(); + providers.insert("proxy".to_string(), toml::Value::Table(provider)); + let mut root = toml::map::Map::new(); + root.insert("model_providers".to_string(), toml::Value::Table(providers)); + + toml::to_string_pretty(&toml::Value::Table(root)) + .map_err(|err| format!("TOML serialize error: {err}")) +} + pub(super) fn selected_model_or_default(selected_model: Option<&str>) -> &str { selected_model .map(str::trim) diff --git a/src-tauri/crates/agent-cli/src/managed_config/mod.rs b/src-tauri/crates/agent-cli/src/managed_config/mod.rs index 3a4a30daa..5fe1dc10d 100644 --- a/src-tauri/crates/agent-cli/src/managed_config/mod.rs +++ b/src-tauri/crates/agent-cli/src/managed_config/mod.rs @@ -34,6 +34,38 @@ use operations::{ use registry::{supported_agent, unavailable_agent_message, MANAGED_CONFIG_ADAPTERS}; use transaction::recover_pending_transaction_unlocked; +/// Keep a small amount of Codex-native recovery without combining it with +/// ORGII's whole-process overload replay. +pub const CODEX_REQUEST_MAX_RETRIES: i64 = 2; +pub const CODEX_STREAM_MAX_RETRIES: i64 = 2; + +/// Write the complete, ORGII-owned Codex profile used by one hosted session. +/// +/// This deliberately does not merge with the user's global Codex config: the +/// caller points `CODEX_HOME` at a session-scoped directory, eliminating +/// cross-session and cross-instance last-writer-wins routing. +pub fn write_codex_hosted_profile( + profile_dir: &std::path::Path, + proxy_url: &str, +) -> Result<(), String> { + let content = generators::generate_codex_hosted_profile(proxy_url)?; + let config_path = profile_dir.join("config.toml"); + if std::fs::read_to_string(&config_path).ok().as_deref() == Some(content.as_str()) { + return Ok(()); + } + file_io::write_sensitive_file_atomic(&config_path, content.as_bytes()) +} + +/// Crash-safe replace of a CLI profile file: write an owner-only sibling temp +/// file, fsync, then rename over the target. The payload is never on disk +/// group- or world-readable, so callers holding credentials only need +/// [`app_paths::set_sensitive_file_permissions`] to pin the destination's +/// permissions (and to cover Windows ACLs) — and get to decide for themselves +/// whether a failure there is fatal. +pub fn write_cli_profile_file_atomic(path: &std::path::Path, bytes: &[u8]) -> Result<(), String> { + file_io::write_file_atomic(path, bytes) +} + pub use dto::{ CliConfigManagedStatus, CliConfigMode, CliConfigProfileManifest, CliConfigShutdownRestoreReport, CliConfigTargetFileManifest, CliConfigTargetFileStatus, diff --git a/src-tauri/crates/agent-cli/src/managed_config/tests.rs b/src-tauri/crates/agent-cli/src/managed_config/tests.rs index 1369216f3..b362bea86 100644 --- a/src-tauri/crates/agent-cli/src/managed_config/tests.rs +++ b/src-tauri/crates/agent-cli/src/managed_config/tests.rs @@ -133,6 +133,18 @@ shell_tool = true parsed["model_providers"]["orgii"]["requires_openai_auth"].as_bool(), Some(false) ); + assert_eq!( + parsed["model_providers"]["orgii"]["supports_websockets"].as_bool(), + Some(false) + ); + assert_eq!( + parsed["model_providers"]["orgii"]["request_max_retries"].as_integer(), + Some(CODEX_REQUEST_MAX_RETRIES) + ); + assert_eq!( + parsed["model_providers"]["orgii"]["stream_max_retries"].as_integer(), + Some(CODEX_STREAM_MAX_RETRIES) + ); } #[test] @@ -863,3 +875,64 @@ fn missing_managed_mode_backup_is_never_recreated_from_active_config() { assert!(result.is_err()); } + +#[test] +fn hosted_codex_profile_is_owned_valid_toml_and_uses_bounded_internal_retries() { + let temp = tempfile::tempdir().unwrap(); + + write_codex_hosted_profile(temp.path(), "http://127.0.0.1:43123/").unwrap(); + + let content = std::fs::read_to_string(temp.path().join("config.toml")).unwrap(); + let config: toml::Value = toml::from_str(&content).unwrap(); + let proxy = &config["model_providers"]["proxy"]; + assert_eq!( + proxy["base_url"].as_str(), + Some("http://127.0.0.1:43123/v1") + ); + assert_eq!(proxy["env_key"].as_str(), Some("PROXY_TOKEN")); + assert_eq!(proxy["requires_openai_auth"].as_bool(), Some(false)); + assert_eq!(proxy["wire_api"].as_str(), Some("responses")); + assert_eq!(proxy["supports_websockets"].as_bool(), Some(false)); + assert_eq!( + proxy["request_max_retries"].as_integer(), + Some(CODEX_REQUEST_MAX_RETRIES) + ); + assert_eq!( + proxy["stream_max_retries"].as_integer(), + Some(CODEX_STREAM_MAX_RETRIES) + ); +} + +#[cfg(unix)] +#[test] +fn credential_profile_writes_are_owner_only_from_the_first_byte() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("auth.json"); + + write_cli_profile_file_atomic(&path, b"{\"token\":\"secret\"}").unwrap(); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "credential file must land owner-only without a separate chmod" + ); + + // Rewriting goes through a fresh temp file; the replacement must be just + // as private as the original, and no staging file may survive. + write_cli_profile_file_atomic(&path, b"{\"token\":\"rotated\"}").unwrap(); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + let leftovers: Vec<_> = std::fs::read_dir(temp.path()) + .unwrap() + .filter_map(Result::ok) + .map(|entry| entry.file_name()) + .filter(|name| name.to_string_lossy().ends_with(".tmp")) + .collect(); + assert!( + leftovers.is_empty(), + "staging files left behind: {leftovers:?}" + ); +} diff --git a/src-tauri/crates/agent-core/src/core/providers/anthropic_native/client.rs b/src-tauri/crates/agent-core/src/core/providers/anthropic_native/client.rs index 5dd6fd454..b237140e0 100644 --- a/src-tauri/crates/agent-core/src/core/providers/anthropic_native/client.rs +++ b/src-tauri/crates/agent-core/src/core/providers/anthropic_native/client.rs @@ -151,7 +151,14 @@ impl AnthropicClient { let refreshed = key_vault::key_store::KEY_SERVICE .refresh_claude_code_oauth_key(&refresh_config.key_id, &rejected_access_token) .await - .map_err(ProviderError::AuthError)?; + .map_err(ProviderError::AuthError)? + .into_key() + .ok_or_else(|| { + ProviderError::AuthError(format!( + "Key {} is not a native Claude OAuth account", + refresh_config.key_id + )) + })?; let access_token = refreshed .session_token diff --git a/src-tauri/crates/agent-core/src/core/providers/codex_native/client.rs b/src-tauri/crates/agent-core/src/core/providers/codex_native/client.rs index 6ac60855f..464e9ef1e 100644 --- a/src-tauri/crates/agent-core/src/core/providers/codex_native/client.rs +++ b/src-tauri/crates/agent-core/src/core/providers/codex_native/client.rs @@ -209,7 +209,14 @@ impl CodexNativeClient { let refreshed = key_vault::key_store::KEY_SERVICE .refresh_codex_oauth_key(&refresh_config.key_id, &rejected_access_token) .await - .map_err(ProviderError::AuthError)?; + .map_err(ProviderError::AuthError)? + .into_key() + .ok_or_else(|| { + ProviderError::AuthError(format!( + "Key {} is not a native Codex OAuth account", + refresh_config.key_id + )) + })?; let access_token = refreshed .session_token diff --git a/src-tauri/crates/app-paths/src/lib.rs b/src-tauri/crates/app-paths/src/lib.rs index 04cfcd3a0..49b7ed8bd 100644 --- a/src-tauri/crates/app-paths/src/lib.rs +++ b/src-tauri/crates/app-paths/src/lib.rs @@ -1077,6 +1077,16 @@ pub fn codex_cli_profile_dir(account_id: &str) -> PathBuf { codex_cli_profile_root().join(sanitize_path_segment(account_id)) } +/// Session-scoped Codex CLI profile root for hosted-key sessions. +pub fn codex_hosted_cli_profile_root() -> PathBuf { + orgii_root().join("codex-hosted-cli-profiles") +} + +/// Session-scoped Codex CLI profile dir for one hosted-key session. +pub fn codex_hosted_cli_profile_dir(session_id: &str) -> PathBuf { + codex_hosted_cli_profile_root().join(sanitize_path_segment(session_id)) +} + /// Account-scoped Kiro CLI profile root: `~/.orgii/kiro-cli-profiles/`. pub fn kiro_cli_profile_root() -> PathBuf { orgii_root().join("kiro-cli-profiles") diff --git a/src-tauri/crates/key-vault/src/commands/registry/data/cli_agents.rs b/src-tauri/crates/key-vault/src/commands/registry/data/cli_agents.rs index cfc023ebf..132907029 100644 --- a/src-tauri/crates/key-vault/src/commands/registry/data/cli_agents.rs +++ b/src-tauri/crates/key-vault/src/commands/registry/data/cli_agents.rs @@ -125,20 +125,11 @@ pub(crate) fn cli_agent_registry() -> Vec { brand_color: "#10A37F", docs_url: "https://developers.openai.com/codex/config-basic", has_subscription_plan: true, - compatible_api_providers: &[ - "openai_api", - "atlascloud_api", - "openrouter_api", - "azure_openai_api", - "deepseek_api", - "groq_api", - "xai_api", - "dashscope_api", - "moonshot_api", - "zenmux_api", - "longcat_api", - "vllm_api", - ], + // Codex requires the Responses wire API. Generic OpenAI Chat + // compatibility is insufficient; keep this list limited to + // providers whose `/responses` route is explicitly supported or + // has been verified by ORGII. + compatible_api_providers: &["openai_api", "zenmux_api"], config_files: vec![home_config("config", "Config", ".codex/config.toml", CliConfigFormat::Toml, false)], is_complex_setup: false, default_setup_method: None, diff --git a/src-tauri/crates/key-vault/src/commands/registry/mod.rs b/src-tauri/crates/key-vault/src/commands/registry/mod.rs index a28f5a146..62f2d4e87 100644 --- a/src-tauri/crates/key-vault/src/commands/registry/mod.rs +++ b/src-tauri/crates/key-vault/src/commands/registry/mod.rs @@ -159,12 +159,15 @@ mod compatibility_tests { #[test] fn compatibility_comes_from_the_central_cli_registry() { - assert!(is_cli_provider_compatible("codex", "atlascloud_api")); + assert!(is_cli_provider_compatible("codex", "zenmux_api")); assert!(is_cli_provider_compatible("opencode", "atlascloud_api")); assert!(is_cli_provider_compatible("claude_code", "atlascloud_api")); assert!(is_cli_provider_compatible("codex", "openai_api")); assert!(is_cli_provider_compatible("claude_code", "anthropic_api")); assert!(!is_cli_provider_compatible("unknown", "openai_api")); + assert!(!is_cli_provider_compatible("codex", "deepseek_api")); + assert!(!is_cli_provider_compatible("codex", "atlascloud_api")); + assert!(!is_cli_provider_compatible("codex", "zhipu_api")); assert_eq!(cli_agent_display_name("opencode"), Some("OpenCode")); assert_eq!(cli_agent_display_name("unknown"), None); } diff --git a/src-tauri/crates/key-vault/src/commands/validate.rs b/src-tauri/crates/key-vault/src/commands/validate.rs index d3e290437..aaa7e50cc 100644 --- a/src-tauri/crates/key-vault/src/commands/validate.rs +++ b/src-tauri/crates/key-vault/src/commands/validate.rs @@ -1191,7 +1191,7 @@ async fn refresh_oauth_key_for_quota( key: &crate::key_store::ModelKey, ) -> Result { let rejected_access_token = key.session_token.clone().unwrap_or_default(); - match key.model_type { + let outcome = match key.model_type { ModelType::ClaudeCode => { KEY_SERVICE .refresh_claude_code_oauth_key(&key.id, &rejected_access_token) @@ -1206,7 +1206,10 @@ async fn refresh_oauth_key_for_quota( "OAuth quota refresh is not supported for {}", other.as_str() )), - } + }?; + outcome + .into_key() + .ok_or_else(|| format!("Key {} is not a native OAuth account", key.id)) } fn is_unauthorized_quota_error(error_message: &str) -> bool { diff --git a/src-tauri/crates/key-vault/src/key_store/agent_env_builder.rs b/src-tauri/crates/key-vault/src/key_store/agent_env_builder.rs index 0b06edf35..4f124790e 100644 --- a/src-tauri/crates/key-vault/src/key_store/agent_env_builder.rs +++ b/src-tauri/crates/key-vault/src/key_store/agent_env_builder.rs @@ -483,8 +483,9 @@ impl KeyService { env.insert("OPENAI_API_KEY".to_string(), proxy_token.to_string()); env.insert("PROXY_TOKEN".to_string(), proxy_token.to_string()); // Note: OPENAI_BASE_URL is NOT set for proxy mode. - // The base URL is configured in ~/.codex/config.toml under - // [model_providers.proxy], and selected via `-c model_provider="proxy"`. + // The base URL is configured in the hosted session's isolated + // CODEX_HOME under [model_providers.proxy], and selected via + // `-c model_provider="proxy"`. // This matches the market-worker's approach. } ModelType::Copilot => { diff --git a/src-tauri/crates/key-vault/src/key_store/service/claude_oauth.rs b/src-tauri/crates/key-vault/src/key_store/service/claude_oauth.rs index 21f407b51..7ab47067d 100644 --- a/src-tauri/crates/key-vault/src/key_store/service/claude_oauth.rs +++ b/src-tauri/crates/key-vault/src/key_store/service/claude_oauth.rs @@ -4,7 +4,7 @@ use chrono::{Duration as ChronoDuration, Utc}; use serde::{Deserialize, Serialize}; -use super::super::types::{AuthMethod, ModelKey, ModelType}; +use super::super::types::{AuthMethod, ModelKey, ModelType, OAuthRefreshOutcome}; use super::{KeyService, OAUTH_REFRESH_EXPIRY_SKEW_SECONDS, OAUTH_REFRESH_REQUEST_TIMEOUT}; const CLAUDE_CODE_TOKEN_URL: &str = "https://platform.claude.com/v1/oauth/token"; @@ -95,7 +95,9 @@ impl KeyService { let rejected_access_token = key.session_token.clone().unwrap_or_default(); self.refresh_claude_code_oauth_key(key_id, &rejected_access_token) - .await + .await? + .into_key() + .ok_or_else(|| format!("Key {} is not a native Claude OAuth account", key_id)) } /// Refresh a Claude Code OAuth key and persist the fresh access token. @@ -103,7 +105,21 @@ impl KeyService { &self, key_id: &str, rejected_access_token: &str, - ) -> Result { + ) -> Result { + let key = self + .get_key_by_id(key_id) + .ok_or_else(|| format!("Key not found: {}", key_id))?; + + if !key.is_native_oauth_for(&ModelType::ClaudeCode) { + tracing::info!( + "[key-vault] Claude Code OAuth refresh skipped key={} reason=not_claude_oauth type={:?} auth={:?}", + key_id, + key.model_type, + key.auth_method + ); + return Ok(OAuthRefreshOutcome::NotApplicable); + } + crate::e2e_guard::ensure_oauth_refresh_allowed()?; let refresh_lock = self.oauth_refresh_lock_for_key(key_id)?; @@ -131,14 +147,14 @@ impl KeyService { key.oauth_refresh_failure_count ); - if key.model_type != ModelType::ClaudeCode || key.auth_method != AuthMethod::Oauth { + if !key.is_native_oauth_for(&ModelType::ClaudeCode) { tracing::info!( "[key-vault] Claude Code OAuth refresh skipped key={} reason=not_claude_oauth type={:?} auth={:?}", key_id, key.model_type, key.auth_method ); - return Ok(key); + return Ok(OAuthRefreshOutcome::NotApplicable); } if key @@ -150,7 +166,7 @@ impl KeyService { "[key-vault] Claude Code OAuth refresh skipped key={} reason=access_token_already_rotated", key_id ); - return Ok(key); + return Ok(OAuthRefreshOutcome::AlreadyRotated(Box::new(key))); } let refresh_token = key @@ -166,8 +182,10 @@ impl KeyService { client_id: CLAUDE_CODE_CLIENT_ID, }; - let token_url = std::env::var(CLAUDE_CODE_REFRESH_TOKEN_URL_OVERRIDE_ENV) - .unwrap_or_else(|_| CLAUDE_CODE_TOKEN_URL.to_string()); + let token_url_override = std::env::var(CLAUDE_CODE_REFRESH_TOKEN_URL_OVERRIDE_ENV).ok(); + let token_url = token_url_override + .clone() + .unwrap_or_else(|| CLAUDE_CODE_TOKEN_URL.to_string()); tracing::info!( "[key-vault] Claude Code OAuth refresh request start key={} endpoint_override={} refresh_len={} access_len={}", key_id, @@ -176,8 +194,14 @@ impl KeyService { rejected_access_token.len() ); - let response = match reqwest::Client::builder() - .timeout(OAUTH_REFRESH_REQUEST_TIMEOUT) + let mut client_builder = reqwest::Client::builder().timeout(OAUTH_REFRESH_REQUEST_TIMEOUT); + if token_url_override.is_some() { + // Test/diagnostic override endpoints are commonly loopback. Do not + // let inherited HTTP(S)_PROXY route 127.0.0.1 away from the local + // one-shot server. + client_builder = client_builder.no_proxy(); + } + let response = match client_builder .build() .map_err(|err| format!("Claude Code OAuth refresh client build failed: {}", err))? .post(token_url) @@ -312,6 +336,6 @@ impl KeyService { .is_some_and(|token| !token.trim().is_empty()) ); - Ok(saved) + Ok(OAuthRefreshOutcome::Refreshed(Box::new(saved))) } } diff --git a/src-tauri/crates/key-vault/src/key_store/service/codex_oauth.rs b/src-tauri/crates/key-vault/src/key_store/service/codex_oauth.rs index 44dccfdd3..6e2e4c3a5 100644 --- a/src-tauri/crates/key-vault/src/key_store/service/codex_oauth.rs +++ b/src-tauri/crates/key-vault/src/key_store/service/codex_oauth.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use core_types::providers::{CODEX_ID_TOKEN_ENV_KEY, CODEX_REFRESH_TOKEN_ENV_KEY}; -use super::super::types::{AuthMethod, ModelKey, ModelType}; +use super::super::types::{AuthMethod, ModelKey, ModelType, OAuthRefreshOutcome}; use super::{KeyService, OAUTH_REFRESH_EXPIRY_SKEW_SECONDS, OAUTH_REFRESH_REQUEST_TIMEOUT}; const CODEX_TOKEN_URL: &str = "https://auth.openai.com/oauth/token"; @@ -88,14 +88,24 @@ impl KeyService { let rejected_access_token = key.session_token.clone().unwrap_or_default(); self.refresh_codex_oauth_key(key_id, &rejected_access_token) - .await + .await? + .into_key() + .ok_or_else(|| format!("Key {} is not a native Codex OAuth account", key_id)) } pub async fn refresh_codex_oauth_key( &self, key_id: &str, rejected_access_token: &str, - ) -> Result { + ) -> Result { + let key = self + .get_key_by_id(key_id) + .ok_or_else(|| format!("Key not found: {}", key_id))?; + + if !key.is_native_oauth_for(&ModelType::Codex) { + return Ok(OAuthRefreshOutcome::NotApplicable); + } + crate::e2e_guard::ensure_oauth_refresh_allowed()?; let refresh_lock = self.oauth_refresh_lock_for_key(key_id)?; @@ -105,8 +115,8 @@ impl KeyService { .get_key_by_id(key_id) .ok_or_else(|| format!("Key not found: {}", key_id))?; - if key.model_type != ModelType::Codex || key.auth_method != AuthMethod::Oauth { - return Ok(key); + if !key.is_native_oauth_for(&ModelType::Codex) { + return Ok(OAuthRefreshOutcome::NotApplicable); } if key @@ -114,7 +124,7 @@ impl KeyService { .as_deref() .is_some_and(|token| !token.is_empty() && token != rejected_access_token) { - return Ok(key); + return Ok(OAuthRefreshOutcome::AlreadyRotated(Box::new(key))); } let refresh_token = key @@ -130,11 +140,16 @@ impl KeyService { refresh_token: &refresh_token, }; - let token_url = std::env::var(CODEX_REFRESH_TOKEN_URL_OVERRIDE_ENV) - .unwrap_or_else(|_| CODEX_TOKEN_URL.to_string()); + let token_url_override = std::env::var(CODEX_REFRESH_TOKEN_URL_OVERRIDE_ENV).ok(); + let token_url = token_url_override + .clone() + .unwrap_or_else(|| CODEX_TOKEN_URL.to_string()); - let response = match reqwest::Client::builder() - .timeout(OAUTH_REFRESH_REQUEST_TIMEOUT) + let mut client_builder = reqwest::Client::builder().timeout(OAUTH_REFRESH_REQUEST_TIMEOUT); + if token_url_override.is_some() { + client_builder = client_builder.no_proxy(); + } + let response = match client_builder .build() .map_err(|err| format!("Codex OAuth refresh client build failed: {}", err))? .post(token_url) @@ -218,7 +233,7 @@ impl KeyService { Ok(entry.clone()) })?; - saved + saved.map(|key| OAuthRefreshOutcome::Refreshed(Box::new(key))) } } diff --git a/src-tauri/crates/key-vault/src/key_store/service/oauth_health.rs b/src-tauri/crates/key-vault/src/key_store/service/oauth_health.rs index 3dddddc3b..bdbf6dd19 100644 --- a/src-tauri/crates/key-vault/src/key_store/service/oauth_health.rs +++ b/src-tauri/crates/key-vault/src/key_store/service/oauth_health.rs @@ -38,7 +38,15 @@ impl KeyService { error_message: &str, ) -> Result, String> { self.update_store(|store| { - let entry = store.keys.get_mut(key_id)?; + let Some(entry) = store.keys.get_mut(key_id) else { + return Ok(None); + }; + if !entry.is_refreshable_native_oauth() { + return Err(format!( + "Key {} ({:?}, {:?}) is not a native OAuth account", + key_id, entry.model_type, entry.auth_method + )); + } let count = entry.oauth_refresh_failure_count.saturating_add(1); entry.oauth_refresh_failure_count = count; entry.last_oauth_refresh_failed_at = Some(Utc::now()); @@ -72,8 +80,8 @@ impl KeyService { .map(|dt| dt.to_rfc3339()), error_message ); - Some(entry.clone()) - }) + Ok(Some(entry.clone())) + })? } pub fn mark_claude_oauth_upstream_health( diff --git a/src-tauri/crates/key-vault/src/key_store/tests/tests.rs b/src-tauri/crates/key-vault/src/key_store/tests/tests.rs index 9fc922235..411970044 100644 --- a/src-tauri/crates/key-vault/src/key_store/tests/tests.rs +++ b/src-tauri/crates/key-vault/src/key_store/tests/tests.rs @@ -1,11 +1,102 @@ use crate::key_store::{ AuthMethod, CliOAuthTokenSync, CliOAuthTokenSyncOutcome, HealthStatus, KeyService, KeyStore, - ModelKey, ModelType, KEY_SERVICE, + ModelKey, ModelType, OAuthRefreshOutcome, KEY_SERVICE, }; use chrono::{TimeZone, Utc}; use std::collections::HashMap; use tempfile::tempdir; +#[test] +fn api_key_rejects_oauth_refresh_failure_bookkeeping_without_mutation() { + let temp_dir = tempdir().unwrap(); + let service = KeyService::new(Some(temp_dir.path().to_path_buf())); + + for model_type in [ + ModelType::Codex, + ModelType::ClaudeCode, + ModelType::ZhipuApi, + ModelType::ZenmuxApi, + ] { + let mut key = ModelKey::new(model_type); + key.api_key = Some("provider-test-key".to_string()); + let key_id = key.id.clone(); + service.save_key(key).unwrap(); + + assert!(service + .record_oauth_refresh_failure(&key_id, "401 Unauthorized: invalid API key") + .is_err()); + + let stored = service.get_key_by_id(&key_id).unwrap(); + assert_eq!(stored.oauth_refresh_failure_count, 0); + assert!(stored.last_oauth_refresh_failed_at.is_none()); + assert!(stored.temporary_unavailable_until.is_none()); + assert_eq!(stored.health_status, HealthStatus::Unknown); + assert!(stored.enabled); + } +} + +#[tokio::test] +async fn api_key_and_cross_provider_refreshes_are_not_applicable() { + let temp_dir = tempdir().unwrap(); + let service = KeyService::new(Some(temp_dir.path().to_path_buf())); + + let mut codex_api_key = ModelKey::new(ModelType::Codex); + codex_api_key.api_key = Some("codex-api-key".to_string()); + let codex_api_key_id = codex_api_key.id.clone(); + service.save_key(codex_api_key).unwrap(); + + let mut claude_api_key = ModelKey::new(ModelType::ClaudeCode); + claude_api_key.api_key = Some("claude-api-key".to_string()); + let claude_api_key_id = claude_api_key.id.clone(); + service.save_key(claude_api_key).unwrap(); + + let mut zhipu_key = ModelKey::new(ModelType::ZhipuApi); + zhipu_key.api_key = Some("zhipu-api-key".to_string()); + let zhipu_key_id = zhipu_key.id.clone(); + service.save_key(zhipu_key).unwrap(); + + let mut zenmux_key = ModelKey::new(ModelType::ZenmuxApi); + zenmux_key.api_key = Some("zenmux-api-key".to_string()); + let zenmux_key_id = zenmux_key.id.clone(); + service.save_key(zenmux_key).unwrap(); + + let mut atlas_key = ModelKey::new(ModelType::AtlascloudApi); + atlas_key.api_key = Some("atlas-api-key".to_string()); + let atlas_key_id = atlas_key.id.clone(); + service.save_key(atlas_key).unwrap(); + + assert!(matches!( + service + .refresh_codex_oauth_key(&codex_api_key_id, "codex-api-key") + .await + .unwrap(), + OAuthRefreshOutcome::NotApplicable + )); + assert!(matches!( + service + .refresh_claude_code_oauth_key(&claude_api_key_id, "claude-api-key") + .await + .unwrap(), + OAuthRefreshOutcome::NotApplicable + )); + for cross_provider_id in [&zhipu_key_id, &zenmux_key_id, &atlas_key_id] { + assert!(matches!( + service + .refresh_codex_oauth_key(cross_provider_id, "provider-api-key") + .await + .unwrap(), + OAuthRefreshOutcome::NotApplicable + )); + assert!(matches!( + service + .refresh_claude_code_oauth_key(cross_provider_id, "provider-api-key") + .await + .unwrap(), + OAuthRefreshOutcome::NotApplicable + )); + } +} + #[test] fn test_agent_type_conversion() { assert_eq!( @@ -884,6 +975,18 @@ async fn test_claude_concurrent_refreshes_once_without_consuming_rotating_token_ let first = first.unwrap(); let second = second.unwrap(); + assert!(matches!( + (&first, &second), + ( + OAuthRefreshOutcome::Refreshed(_), + OAuthRefreshOutcome::AlreadyRotated(_) + ) | ( + OAuthRefreshOutcome::AlreadyRotated(_), + OAuthRefreshOutcome::Refreshed(_) + ) + )); + let first = first.into_key().unwrap(); + let second = second.into_key().unwrap(); assert_eq!(request_count.load(Ordering::SeqCst), 1); assert_eq!(first.session_token.as_deref(), Some("fresh-claude-access")); assert_eq!(second.session_token.as_deref(), Some("fresh-claude-access")); @@ -966,6 +1069,18 @@ async fn test_codex_refresh_uses_form_body_and_concurrent_refreshes_once() { let first = first.unwrap(); let second = second.unwrap(); + assert!(matches!( + (&first, &second), + ( + OAuthRefreshOutcome::Refreshed(_), + OAuthRefreshOutcome::AlreadyRotated(_) + ) | ( + OAuthRefreshOutcome::AlreadyRotated(_), + OAuthRefreshOutcome::Refreshed(_) + ) + )); + let first = first.into_key().unwrap(); + let second = second.into_key().unwrap(); assert_eq!(request_count.load(Ordering::SeqCst), 1); assert_eq!(first.session_token.as_deref(), Some("fresh-codex-access")); assert_eq!(second.session_token.as_deref(), Some("fresh-codex-access")); diff --git a/src-tauri/crates/key-vault/src/key_store/types.rs b/src-tauri/crates/key-vault/src/key_store/types.rs index 84806c308..6995db1da 100644 --- a/src-tauri/crates/key-vault/src/key_store/types.rs +++ b/src-tauri/crates/key-vault/src/key_store/types.rs @@ -544,6 +544,22 @@ impl ModelKey { } } + /// Whether this credential is a native OAuth account for the target CLI. + /// Cross-provider keys and native API keys must never enter OAuth retry, + /// token rotation, or OAuth health bookkeeping. + pub fn is_native_oauth_for(&self, target: &ModelType) -> bool { + self.auth_method == AuthMethod::Oauth + && &self.model_type == target + && matches!(target, ModelType::Codex | ModelType::ClaudeCode) + } + + /// Whether this credential is handled by one of KeyService's OAuth + /// refresh implementations. + pub fn is_refreshable_native_oauth(&self) -> bool { + self.auth_method == AuthMethod::Oauth + && matches!(self.model_type, ModelType::Codex | ModelType::ClaudeCode) + } + /// Mask sensitive data for display pub fn mask_api_key(&self) -> Option { self.api_key.as_ref().map(|key| { @@ -579,6 +595,28 @@ impl ModelKey { } } +/// Result of an explicit OAuth refresh attempt. +/// +/// `AlreadyRotated` means another concurrent caller refreshed the access +/// token while this caller waited for the per-key lock. `NotApplicable` +/// makes API-key and cross-provider credentials impossible to mistake for a +/// successful refresh. +#[derive(Debug, Clone)] +pub enum OAuthRefreshOutcome { + Refreshed(Box), + AlreadyRotated(Box), + NotApplicable, +} + +impl OAuthRefreshOutcome { + pub fn into_key(self) -> Option { + match self { + Self::Refreshed(key) | Self::AlreadyRotated(key) => Some(*key), + Self::NotApplicable => None, + } + } +} + /// Whether the account talks to an official Anthropic endpoint. A configured /// `base_url` means the traffic goes through a third-party relay/mirror whose /// gateway may reject Anthropic-native request features (`output_config`/ diff --git a/src-tauri/crates/orgtrack-core/src/sources/codex/app/index.rs b/src-tauri/crates/orgtrack-core/src/sources/codex/app/index.rs index 167bc8696..b380be59b 100644 --- a/src-tauri/crates/orgtrack-core/src/sources/codex/app/index.rs +++ b/src-tauri/crates/orgtrack-core/src/sources/codex/app/index.rs @@ -663,16 +663,30 @@ fn resolve_codex_session_path(conn: &Connection, file_stem: &str) -> Result Result, String> { let home = app_paths::external_history_home_dir(); let mut dirs = codex_sessions_dir_candidates(&home); - // ORGII-managed own-key Codex runs redirect CODEX_HOME into per-account - // profile dirs; native-transcript mode reads those rollouts back here. - // (Hosted-key Codex keeps the system CODEX_HOME and is covered above.) + // ORGII-managed Codex runs redirect CODEX_HOME into isolated profile + // directories; native-transcript mode reads those rollouts back here. + dirs.extend(codex_managed_sessions_dirs( + &app_paths::codex_cli_profile_root(), + &app_paths::codex_hosted_cli_profile_root(), + )); + Ok(dirs) +} + +pub(crate) fn codex_managed_sessions_dirs( + account_profiles_root: &Path, + hosted_profiles_root: &Path, +) -> Vec { + let mut dirs = crate::sources::imported_history::managed_roots::profile_root_children( + account_profiles_root, + &["sessions"], + ); dirs.extend( crate::sources::imported_history::managed_roots::profile_root_children( - &app_paths::codex_cli_profile_root(), + hosted_profiles_root, &["sessions"], ), ); - Ok(dirs) + dirs } pub(crate) fn codex_sessions_dir_candidates(home: &Path) -> Vec { diff --git a/src-tauri/crates/orgtrack-core/src/sources/codex/app/mod.rs b/src-tauri/crates/orgtrack-core/src/sources/codex/app/mod.rs index a79f17f69..468d845dc 100644 --- a/src-tauri/crates/orgtrack-core/src/sources/codex/app/mod.rs +++ b/src-tauri/crates/orgtrack-core/src/sources/codex/app/mod.rs @@ -41,7 +41,7 @@ pub(crate) use crate::sources::imported_history::{ strip_orgii_exec_mode_bridge, }; #[cfg(test)] -pub(crate) use index::codex_sessions_dir_candidates; +pub(crate) use index::{codex_managed_sessions_dirs, codex_sessions_dir_candidates}; #[cfg(test)] pub(crate) use meta::{parse_codex_session_meta, parse_codex_session_meta_incremental}; #[cfg(test)] diff --git a/src-tauri/crates/orgtrack-core/src/sources/codex/app/transcript.rs b/src-tauri/crates/orgtrack-core/src/sources/codex/app/transcript.rs index 14c17fb76..bedaa5d7c 100644 --- a/src-tauri/crates/orgtrack-core/src/sources/codex/app/transcript.rs +++ b/src-tauri/crates/orgtrack-core/src/sources/codex/app/transcript.rs @@ -1210,9 +1210,27 @@ fn load_codex_app_from_path_with_mode<'a>( } } "task_complete" => { + let task_error_message = codex_task_error_message(&parsed.payload); + if let Some(error_message) = task_error_message.as_deref() { + let mut error_chunk = ActivityChunk::new(session_id, "error", "error"); + error_chunk.chunk_id = format!("codex-error-{sequence}"); + error_chunk.created_at = created_at.clone(); + error_chunk.result = json!({ + "error": error_message, + "observation": error_message, + "success": false, + }); + collector.current.push(error_chunk); + sequence += 1; + } if let Some(turn_id) = lifecycle_turn_id(&parsed.payload, active_task_turn_id.as_deref()) { + let lifecycle_action = if task_error_message.is_some() { + imported_history::ACTION_TYPE_TASK_FAILED + } else { + imported_history::ACTION_TYPE_TASK_COMPLETED + }; collector .current .push(imported_history::task_lifecycle_chunk( @@ -1220,7 +1238,7 @@ fn load_codex_app_from_path_with_mode<'a>( CODEX_PROVIDER_SLUG, sequence, &created_at, - imported_history::ACTION_TYPE_TASK_COMPLETED, + lifecycle_action, turn_id, )); sequence += 1; @@ -1330,6 +1348,26 @@ fn lifecycle_turn_id<'a>(payload: &'a Value, active_turn_id: Option<&'a str>) -> .or(active_turn_id) } +fn codex_task_error_message(payload: &Value) -> Option { + let error = payload.get("error")?; + if error.is_null() { + return None; + } + + let message = error + .as_str() + .or_else(|| error.get("message").and_then(Value::as_str)) + .map(str::trim) + .filter(|message| !message.is_empty()); + Some(match message { + Some(message) => message.to_string(), + None if error.as_object().is_some_and(|object| object.is_empty()) => { + "Codex task failed".to_string() + } + None => format!("Codex task failed: {error}"), + }) +} + #[allow(clippy::too_many_arguments)] fn resolve_codex_tool_outputs( transcript_session_id: &str, diff --git a/src-tauri/crates/orgtrack-core/src/sources/codex/app_tests.rs b/src-tauri/crates/orgtrack-core/src/sources/codex/app_tests.rs index 7263c6f0e..2405ed030 100644 --- a/src-tauri/crates/orgtrack-core/src/sources/codex/app_tests.rs +++ b/src-tauri/crates/orgtrack-core/src/sources/codex/app_tests.rs @@ -32,6 +32,35 @@ fn includes_codex_session_dir_candidates() { } } +#[test] +fn includes_account_and_hosted_managed_codex_rollouts() { + struct TempRoot(std::path::PathBuf); + impl Drop for TempRoot { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + let unique = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let temp = TempRoot(std::env::temp_dir().join(format!( + "orgtrack-managed-codex-{}-{unique}", + std::process::id() + ))); + let account_root = temp.0.join("accounts"); + let hosted_root = temp.0.join("hosted"); + let account_sessions = account_root.join("account-1").join("sessions"); + let hosted_sessions = hosted_root.join("session-1").join("sessions"); + std::fs::create_dir_all(&account_sessions).unwrap(); + std::fs::create_dir_all(&hosted_sessions).unwrap(); + + let dirs = codex_managed_sessions_dirs(&account_root, &hosted_root); + + assert!(dirs.contains(&account_sessions)); + assert!(dirs.contains(&hosted_sessions)); +} + #[test] fn normalizes_codex_collaboration_calls_without_exposing_encrypted_messages() { let spawn_calls = normalize_codex_tool_calls( @@ -581,6 +610,60 @@ fn codex_task_lifecycle_projects_only_finished_turns_as_completed() { crate::projectors::turn_metadata::project_activity_chunks(&completed_chunks); assert_eq!(completed_rounds[0].status, "completed"); + let failed_content = format!( + "{active_content}{}\n", + json!({ + "timestamp": "2026-07-21T01:00:02.000Z", + "type": "event_msg", + "payload": { + "type": "task_complete", + "turn_id": "turn-1", + "error": { + "message": "unexpected status 402 Payment Required: subscription quota exhausted" + } + } + }) + ); + std::fs::write(&path, failed_content).expect("write failed fixture"); + let failed_chunks = + load_codex_app_from_path("codexapp-lifecycle", &path).expect("parse failed turn"); + let error_chunk = failed_chunks + .iter() + .find(|chunk| chunk.action_type == "error") + .expect("failed task_complete should retain its error message"); + assert_eq!( + error_chunk.result.get("error").and_then(Value::as_str), + Some("unexpected status 402 Payment Required: subscription quota exhausted") + ); + assert_eq!( + failed_chunks.last().map(|chunk| chunk.action_type.as_str()), + Some(imported_history::ACTION_TYPE_TASK_FAILED) + ); + let failed_rounds = crate::projectors::turn_metadata::project_activity_chunks(&failed_chunks); + assert_eq!(failed_rounds[0].status, "failed"); + + let structured_error_content = format!( + "{active_content}{}\n", + json!({ + "timestamp": "2026-07-21T01:00:02.000Z", + "type": "event_msg", + "payload": { + "type": "task_complete", + "turn_id": "turn-1", + "error": { "code": "quota_exhausted" } + } + }) + ); + std::fs::write(&path, structured_error_content).expect("write structured error fixture"); + let structured_error_chunks = + load_codex_app_from_path("codexapp-lifecycle", &path).expect("parse structured error turn"); + assert_eq!( + structured_error_chunks + .last() + .map(|chunk| chunk.action_type.as_str()), + Some(imported_history::ACTION_TYPE_TASK_FAILED) + ); + std::fs::remove_file(&path).expect("remove fixture"); std::fs::remove_dir(&temp_dir).expect("remove temp dir"); } diff --git a/src-tauri/src/agent_sessions/cli/parsers/codex.rs b/src-tauri/src/agent_sessions/cli/parsers/codex.rs index b8118c430..8df054c1f 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/codex.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/codex.rs @@ -6,7 +6,9 @@ use serde_json::Value; -use super::CliAgentParser; +use super::{ + canonicalize_cli_error_message, is_codex_retry_notice, BoundedCliErrorDeduper, CliAgentParser, +}; use crate::agent_sessions::cli::parsers::normalizer::{normalize_tool_name, unwrap_codex_command}; use crate::agent_sessions::cli::parsers::types::{CliAgentType, TokenUsage}; use core_types::activity::ActivityChunk; @@ -16,6 +18,16 @@ pub struct CodexParser { thread_id: Option, usage: Option, got_turn_completed: bool, + error_deduper: BoundedCliErrorDeduper, + /// A non-retryable `error` is provisional until Codex emits the matching + /// terminal event. Keeping it here prevents one failed turn from becoming + /// both an error card and a failed session-end card. + pending_error_message: Option, + /// Last retry notice, kept only as a body of last resort. Codex recovers + /// from these, so it must never be rendered on its own — but when the + /// terminal event carries no message it is the only description of what + /// went wrong, and `Turn failed` tells the user nothing. + last_retry_notice: Option, } impl CodexParser { @@ -25,6 +37,9 @@ impl CodexParser { thread_id: None, usage: None, got_turn_completed: false, + error_deduper: BoundedCliErrorDeduper::default(), + pending_error_message: None, + last_retry_notice: None, } } @@ -139,6 +154,17 @@ impl CodexParser { let item_type = item.get("type").and_then(|v| v.as_str()).unwrap_or(""); let cursor_name = normalize_tool_name(CliAgentType::Codex, item_type); + // Rendering the fallback-metadata notice as an error card is + // misleading; Codex keeps running after it. + if item_type == "error" + && item + .get("message") + .and_then(|value| value.as_str()) + .is_some_and(super::is_codex_fallback_metadata_notice) + { + return vec![]; + } + // Reasoning items: extract summary text and emit as thinking if item_type == "reasoning" { let thought = Self::extract_reasoning_text(item); @@ -239,6 +265,8 @@ impl CodexParser { "turn.completed" => { self.got_turn_completed = true; + self.pending_error_message = None; + self.last_retry_notice = None; // Extract usage if let Some(usage) = data.get("usage") { @@ -277,22 +305,34 @@ impl CodexParser { .get("message") .and_then(|v| v.as_str()) .unwrap_or("Unknown error"); - let mut chunk = ActivityChunk::new(&self.session_id, "error", "error"); - chunk.result = serde_json::json!({ - "observation": message, "error": message, "success": false, - }); - vec![chunk] + if is_codex_retry_notice(message) { + self.last_retry_notice = Some(canonicalize_cli_error_message(message)); + return vec![]; + } + let message = canonicalize_cli_error_message(message); + let Some(message) = self.error_deduper.admit(message) else { + return vec![]; + }; + self.pending_error_message = Some(message); + vec![] } "turn.started" => vec![], // Informational, no chunk needed "turn.failed" => { self.got_turn_completed = true; // Prevent duplicate session_end - let error_msg = data + let terminal_error = data .get("error") .and_then(|e| e.get("message")) .and_then(|v| v.as_str()) - .unwrap_or("Turn failed"); + .map(canonicalize_cli_error_message) + .filter(|message| !message.is_empty()); + let error_msg = terminal_error + .or_else(|| self.pending_error_message.take()) + .or_else(|| self.last_retry_notice.take()) + .unwrap_or_else(|| "Turn failed".to_string()); + self.pending_error_message = None; + self.last_retry_notice = None; let mut chunk = ActivityChunk::new(&self.session_id, "session_end", "session_end"); chunk.result = serde_json::json!({ "success": false, @@ -370,10 +410,24 @@ impl CliAgentParser for CodexParser { return vec![]; } let mut chunk = ActivityChunk::new(&self.session_id, "session_end", "session_end"); - chunk.result = serde_json::json!({ - "success": exit_code == 0, - "exit_code": exit_code, - }); + // A retry notice only describes a failure if the process actually died; + // on a clean exit Codex recovered and there is nothing to report. + let failure_message = self + .pending_error_message + .take() + .or_else(|| self.last_retry_notice.take().filter(|_| exit_code != 0)); + if let Some(message) = failure_message { + chunk.result = serde_json::json!({ + "success": false, + "exit_code": exit_code, + "error_message": message, + }); + } else { + chunk.result = serde_json::json!({ + "success": exit_code == 0, + "exit_code": exit_code, + }); + } vec![chunk] } diff --git a/src-tauri/src/agent_sessions/cli/parsers/codex_app_server.rs b/src-tauri/src/agent_sessions/cli/parsers/codex_app_server.rs index 27379ba53..983b09a10 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/codex_app_server.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/codex_app_server.rs @@ -63,6 +63,7 @@ use tokio::sync::mpsc; use core_types::activity::ActivityChunk; +use super::canonicalize_cli_error_message; use super::normalizer::{normalize_tool_name, unwrap_codex_command}; use super::types::{CliAgentType, TokenUsage}; use crate::agent_sessions::cli::session_runner::launch_profiles::CliPermissionMode; @@ -215,6 +216,14 @@ pub(crate) struct CodexAppServerEventParser { turn_status: Option, turn_error: Option, session_start_emitted: bool, + error_deduper: super::BoundedCliErrorDeduper, + /// Held until `turn/completed`, whose error body is authoritative. + pending_error_message: Option, + /// Last `willRetry` error, kept only as a body of last resort. Codex + /// retries past these, so it must never be rendered on its own — but a + /// `turn/completed` that reports failure without an error body leaves the + /// turn with no message at all, and this is the only thing left to say. + last_retry_notice: Option, } impl CodexAppServerEventParser { @@ -227,6 +236,9 @@ impl CodexAppServerEventParser { turn_status: None, turn_error: None, session_start_emitted: false, + error_deduper: super::BoundedCliErrorDeduper::default(), + pending_error_message: None, + last_retry_notice: None, } } @@ -382,13 +394,26 @@ impl CodexAppServerEventParser { .and_then(|v| v.as_str()) .unwrap_or("completed") .to_string(); - let error_message = params + let terminal_error_message = params .get("turn") .and_then(|t| t.get("error")) .and_then(|e| e.get("message")) .and_then(|v| v.as_str()) - .map(|s| s.to_string()); + .map(canonicalize_cli_error_message) + .filter(|message| !message.is_empty()); let success = status == "completed"; + // The retry notice is the weakest signal there is: only a turn + // that actually failed may borrow it, an interrupted one has a + // better reason of its own — and it never outlives the turn. + let retry_fallback = self.last_retry_notice.take().filter(|_| status == "failed"); + let error_message = if success { + self.pending_error_message = None; + None + } else { + terminal_error_message + .or_else(|| self.pending_error_message.take()) + .or(retry_fallback) + }; let mut chunk = ActivityChunk::new(&self.session_id, "session_end", "session_end"); let mut result = serde_json::json!({ "success": success, @@ -414,13 +439,15 @@ impl CodexAppServerEventParser { .unwrap_or(false); if will_retry { tracing::warn!("[CodexAppServer] Retryable error: {}", message); + self.last_retry_notice = Some(canonicalize_cli_error_message(message)); return vec![]; } - let mut chunk = ActivityChunk::new(&self.session_id, "error", "error"); - chunk.result = serde_json::json!({ - "observation": message, "error": message, "success": false, - }); - vec![chunk] + let message = canonicalize_cli_error_message(message); + let Some(message) = self.error_deduper.admit(message) else { + return vec![]; + }; + self.pending_error_message = Some(message); + vec![] } other => { tracing::debug!("[CodexAppServer] Ignoring notification: {}", other); diff --git a/src-tauri/src/agent_sessions/cli/parsers/mod.rs b/src-tauri/src/agent_sessions/cli/parsers/mod.rs index ccc36758a..2c3aaa434 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/mod.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/mod.rs @@ -47,9 +47,89 @@ pub mod opencode; #[path = "tests/parser_integration_tests.rs"] mod parser_integration_tests; +use std::collections::{HashSet, VecDeque}; + use core_types::activity::ActivityChunk; use types::TokenUsage; +const CODEX_RECONNECTING_PREFIX: &str = "Reconnecting..."; +const MAX_CLI_ERROR_IDENTITIES_PER_TURN: usize = 32; + +#[derive(Default)] +pub(super) struct BoundedCliErrorDeduper { + seen: HashSet, + order: VecDeque, +} + +impl BoundedCliErrorDeduper { + pub(super) fn admit(&mut self, message: String) -> Option { + if self.seen.contains(&message) { + return None; + } + + while self.seen.len() >= MAX_CLI_ERROR_IDENTITIES_PER_TURN { + let Some(oldest) = self.order.pop_front() else { + self.seen.clear(); + break; + }; + self.seen.remove(&oldest); + } + + self.seen.insert(message.clone()); + self.order.push_back(message.clone()); + Some(message) + } +} + +/// Codex emits one top-level `error` event for every transport retry before +/// emitting the final failure. Retry notices are progress, not independent +/// user-visible errors. +pub(crate) fn is_codex_retry_notice(message: &str) -> bool { + message + .trim_start() + .get(..CODEX_RECONNECTING_PREFIX.len()) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case(CODEX_RECONNECTING_PREFIX)) +} + +/// Unknown custom models produce this non-fatal notice before the request +/// starts. Codex explicitly falls back and keeps running, so it is never the +/// reason a session failed — both the structured parser and the stderr +/// fallback summariser must drop it, or the same benign line reappears as the +/// persisted failure message. +pub(crate) fn is_codex_fallback_metadata_notice(message: &str) -> bool { + message.contains("Model metadata for") && message.contains("Defaulting to fallback metadata") +} + +/// Remove retry wrappers and volatile request identifiers so the same upstream +/// failure has a stable identity across attempts. +pub(crate) fn canonicalize_cli_error_message(message: &str) -> String { + let mut normalized = message.trim(); + if is_codex_retry_notice(normalized) { + if let Some(open_paren) = normalized.find('(') { + normalized = normalized[open_paren + 1..] + .strip_suffix(')') + .unwrap_or(&normalized[open_paren + 1..]); + } + } + + let lower = normalized.to_ascii_lowercase(); + if let Some(cutoff) = [ + ", cf-ray:", + ", request id:", + ", request-id:", + ", request_id:", + ", x-request-id:", + ] + .iter() + .filter_map(|marker| lower.find(marker)) + .min() + { + normalized = &normalized[..cutoff]; + } + + normalized.trim().to_string() +} + /// Trait for parsing a CLI agent's stdout line by line. pub trait CliAgentParser: Send { /// Parse a single line from the CLI's stdout. @@ -69,3 +149,22 @@ pub trait CliAgentParser: Send { None } } + +#[cfg(test)] +mod bounded_error_deduper_tests { + use super::{BoundedCliErrorDeduper, MAX_CLI_ERROR_IDENTITIES_PER_TURN}; + + #[test] + fn error_identity_retention_evicts_oldest_without_dropping_new_errors() { + let mut deduper = BoundedCliErrorDeduper::default(); + for index in 0..MAX_CLI_ERROR_IDENTITIES_PER_TURN { + assert!(deduper.admit(format!("error-{index}")).is_some()); + } + + assert!(deduper.admit("error-0".to_string()).is_none()); + assert!(deduper.admit("one-too-many".to_string()).is_some()); + assert!(deduper.admit("error-0".to_string()).is_some()); + assert_eq!(deduper.seen.len(), MAX_CLI_ERROR_IDENTITIES_PER_TURN); + assert_eq!(deduper.order.len(), MAX_CLI_ERROR_IDENTITIES_PER_TURN); + } +} diff --git a/src-tauri/src/agent_sessions/cli/parsers/tests/codex_app_server_tests.rs b/src-tauri/src/agent_sessions/cli/parsers/tests/codex_app_server_tests.rs index adb22bd6a..59c9096d4 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/tests/codex_app_server_tests.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/tests/codex_app_server_tests.rs @@ -371,16 +371,21 @@ fn interrupted_turn_records_status() { } #[test] -fn fatal_error_notification_maps_to_error_chunk_retryable_is_swallowed() { +fn fatal_error_is_coalesced_into_authoritative_failed_turn() { let mut p = parser(); let fatal = notif( &mut p, "error", json!({"threadId": "t", "turnId": "u", "error": {"message": "boom"}, "willRetry": false}), ); - assert_eq!(fatal.len(), 1); - assert_eq!(fatal[0].action_type, "error"); - assert_eq!(fatal[0].result["error"], "boom"); + assert!(fatal.is_empty()); + + let duplicate_fatal = notif( + &mut p, + "error", + json!({"threadId": "t", "turnId": "u", "error": {"message": "boom, request-id: second"}, "willRetry": false}), + ); + assert!(duplicate_fatal.is_empty()); let retryable = notif( &mut p, @@ -388,6 +393,92 @@ fn fatal_error_notification_maps_to_error_chunk_retryable_is_swallowed() { json!({"threadId": "t", "turnId": "u", "error": {"message": "transient"}, "willRetry": true}), ); assert!(retryable.is_empty()); + + let terminal = notif( + &mut p, + "turn/completed", + json!({"threadId": "t", "turn": { + "id": "u", "items": [], "status": "failed", + "error": {"message": "authoritative upstream failure"}, + }}), + ); + assert_eq!(terminal.len(), 1); + assert_eq!(terminal[0].action_type, "session_end"); + assert_eq!(terminal[0].result["success"], false); + assert_eq!( + terminal[0].result["error_message"], + "authoritative upstream failure" + ); + assert_eq!(p.turn_error(), Some("authoritative upstream failure")); +} + +#[test] +fn failed_turn_without_a_body_falls_back_to_the_last_retry_notice() { + let mut p = parser(); + let retryable = notif( + &mut p, + "error", + json!({"threadId": "t", "turnId": "u", "error": {"message": "stream disconnected"}, "willRetry": true}), + ); + assert!(retryable.is_empty(), "a retry is progress, not an error"); + + // codex reports the failure but attaches no error object — without the + // fallback the turn ends with no explanation at all. + let terminal = notif( + &mut p, + "turn/completed", + json!({"threadId": "t", "turn": {"id": "u", "items": [], "status": "failed"}}), + ); + assert_eq!(terminal[0].result["success"], false); + assert_eq!(terminal[0].result["error_message"], "stream disconnected"); + assert_eq!(p.turn_error(), Some("stream disconnected")); +} + +#[test] +fn retry_notice_never_outlives_its_turn() { + let mut p = parser(); + notif( + &mut p, + "error", + json!({"threadId": "t", "turnId": "u", "error": {"message": "stream disconnected"}, "willRetry": true}), + ); + + // Codex retried and got through: the notice describes nothing. + let ok = notif( + &mut p, + "turn/completed", + json!({"threadId": "t", "turn": {"id": "u", "items": [], "status": "completed"}}), + ); + assert_eq!(ok[0].result["success"], true); + assert!(ok[0].result.get("error_message").is_none()); + + // And it must not be waiting to attach itself to the next turn either. + let next = notif( + &mut p, + "turn/completed", + json!({"threadId": "t", "turn": {"id": "v", "items": [], "status": "failed"}}), + ); + assert!(next[0].result.get("error_message").is_none()); + assert_eq!(p.turn_error(), None); +} + +#[test] +fn interrupted_turn_does_not_borrow_a_retry_notice() { + let mut p = parser(); + notif( + &mut p, + "error", + json!({"threadId": "t", "turnId": "u", "error": {"message": "stream disconnected"}, "willRetry": true}), + ); + + // The user cancelled; "stream disconnected" is not why this turn ended. + let interrupted = notif( + &mut p, + "turn/completed", + json!({"threadId": "t", "turn": {"id": "u", "items": [], "status": "interrupted"}}), + ); + assert_eq!(interrupted[0].result["stop_reason"], "interrupted"); + assert!(interrupted[0].result.get("error_message").is_none()); } #[test] diff --git a/src-tauri/src/agent_sessions/cli/parsers/tests/parser_integration_tests.rs b/src-tauri/src/agent_sessions/cli/parsers/tests/parser_integration_tests.rs index 2026b781c..b15c588e6 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/tests/parser_integration_tests.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/tests/parser_integration_tests.rs @@ -27,23 +27,46 @@ mod tests { } #[test] - fn test_codex_error_event() { + fn test_codex_error_event_is_deferred_until_terminal_exit() { let mut parser = CodexParser::new("test-session"); + let metadata_fallback = parser.parse_line( + r#"{"type":"item.completed","item":{"id":"item_0","type":"error","message":"Model metadata for `z-ai/glm-5.2` not found. Defaulting to fallback metadata; this can degrade performance and cause issues."}}"#, + ); + assert!(metadata_fallback.is_empty()); + + let retry = parser.parse_line( + r#"{"type":"error","message":"Reconnecting... 1/5 (unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses, cf-ray: first)"}"#, + ); + assert!(retry.is_empty()); + let chunks = parser.parse_line( - r#"{"type":"error","message":"Quota exceeded. Check your plan and billing details."}"#, + r#"{"type":"error","message":"unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses, cf-ray: final"}"#, ); + assert!(chunks.is_empty()); - assert_eq!(chunks.len(), 1); - assert_eq!(chunks[0].action_type, "error"); - assert_eq!(chunks[0].function, "error"); - let result = &chunks[0].result; - assert_eq!(result["success"], false); - assert!(result["error"].as_str().unwrap().contains("Quota exceeded")); + let duplicate = parser.parse_line( + r#"{"type":"error","message":"unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses, cf-ray: another"}"#, + ); + assert!(duplicate.is_empty()); + + let terminal = parser.on_exit(1); + assert_eq!(terminal.len(), 1); + assert_eq!(terminal[0].action_type, "session_end"); + assert_eq!(terminal[0].result["success"], false); + assert_eq!( + terminal[0].result["error_message"], + "unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses" + ); } #[test] fn test_codex_turn_failed() { let mut parser = CodexParser::new("test-session"); + let provisional = parser.parse_line( + r#"{"type":"error","message":"earlier transport error, request-id: provisional"}"#, + ); + assert!(provisional.is_empty()); + let chunks = parser.parse_line( r#"{"type":"turn.failed","error":{"message":"unexpected status 401 Unauthorized: "}}"#, ); @@ -55,12 +78,53 @@ mod tests { .as_str() .unwrap() .contains("401 Unauthorized")); + assert_ne!(chunks[0].result["error_message"], "earlier transport error"); // on_exit should not produce another session_end after turn.failed let exit_chunks = parser.on_exit(1); assert!(exit_chunks.is_empty()); } + #[test] + fn test_codex_turn_failed_without_body_falls_back_to_the_retry_notice() { + let mut parser = CodexParser::new("test-session"); + let retry = parser.parse_line( + r#"{"type":"error","message":"Reconnecting... (upstream 503 Service Unavailable)"}"#, + ); + assert!(retry.is_empty(), "a retry notice is progress, not an error"); + + let chunks = parser.parse_line(r#"{"type":"turn.failed"}"#); + + // Without this fallback the only thing the user sees is "Turn failed". + assert_eq!(chunks.len(), 1); + assert_eq!(chunks[0].result["success"], false); + assert_eq!( + chunks[0].result["error_message"], + "upstream 503 Service Unavailable" + ); + } + + #[test] + fn test_codex_retry_notice_is_dropped_once_the_turn_recovers() { + let mut parser = CodexParser::new("test-session"); + parser.parse_line(r#"{"type":"error","message":"Reconnecting... (upstream 503)"}"#); + parser.parse_line(r#"{"type":"turn.completed"}"#); + + let exit_chunks = parser.on_exit(0); + assert!( + exit_chunks.is_empty(), + "a recovered turn must not resurface the notice" + ); + + // A later turn that dies without ever reconnecting keeps the generic + // exit reporting rather than inheriting the previous turn's notice. + let mut parser = CodexParser::new("test-session"); + parser.parse_line(r#"{"type":"error","message":"Reconnecting... (upstream 503)"}"#); + let exit_chunks = parser.on_exit(0); + assert_eq!(exit_chunks[0].result["success"], true); + assert!(exit_chunks[0].result.get("error_message").is_none()); + } + #[test] fn test_codex_item_completed_command() { let mut parser = CodexParser::new("test-session"); diff --git a/src-tauri/src/agent_sessions/cli/persistence/session_crud.rs b/src-tauri/src/agent_sessions/cli/persistence/session_crud.rs index b9db51d85..e3b53ed78 100644 --- a/src-tauri/src/agent_sessions/cli/persistence/session_crud.rs +++ b/src-tauri/src/agent_sessions/cli/persistence/session_crud.rs @@ -907,6 +907,17 @@ pub fn delete_session(session_id: &str) -> SqliteResult { { tracing::warn!(session_id, error = %err, "[cli-persistence] orgtrack delete mirror failed"); } + let hosted_codex_profile = app_paths::codex_hosted_cli_profile_dir(session_id); + if hosted_codex_profile.exists() { + if let Err(err) = std::fs::remove_dir_all(&hosted_codex_profile) { + tracing::warn!( + session_id, + path = %hosted_codex_profile.display(), + error = %err, + "[cli-persistence] hosted Codex profile delete failed" + ); + } + } } Ok(affected > 0) } diff --git a/src-tauri/src/agent_sessions/cli/session_runner/env_setup.rs b/src-tauri/src/agent_sessions/cli/session_runner/env_setup.rs index f1c4fe2b6..5605e066f 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/env_setup.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/env_setup.rs @@ -6,14 +6,9 @@ //! the OpenCode SSE sanitizer. Extracted from `session::run_session` so the //! runner reads as an orchestration of named phases. +use key_vault::key_store::{ModelKey, ModelType}; use std::collections::HashMap; use std::path::Path; -use std::process::Stdio; - -use tokio::process::Command; - -use integrations::cli_binary_resolver::{resolve_cli_binary_command, CliBinaryId}; -use key_vault::key_store::{ModelKey, ModelType}; use super::super::persistence::CodeSession; use super::super::types::{proxy_env, KeySource}; @@ -23,7 +18,7 @@ const OPENCODE_ZENMUX_PROVIDER_ID: &str = "zenmux"; const OPENCODE_ZENMUX_BASE_URL: &str = "https://zenmux.ai/api/v1"; const OPENCODE_DEFAULT_ZENMUX_MODEL: &str = "deepseek/deepseek-chat"; const ATLASCLOUD_PROVIDER_ID: &str = "atlascloud"; -const ATLASCLOUD_CODEX_PROVIDER_ID: &str = "atlas_coding_plan"; +const CODEX_COMPATIBLE_PROVIDER_ID: &str = "orgii_compatible"; const ATLASCLOUD_BASE_URL: &str = "https://api.atlascloud.ai/v1"; const ATLASCLOUD_DEFAULT_MODEL: &str = "zai-org/glm-5.1"; const OPENCODE_ZENMUX_MODEL_IDS: &[&str] = &[ @@ -200,32 +195,146 @@ pub(super) fn setup_opencode_atlascloud_profile( Ok(()) } -pub(super) fn setup_codex_atlascloud_profile( - profile_home: &Path, - selected_key: &ModelKey, +fn codex_compatible_model_id( session_model: Option<&str>, -) -> Result<(), String> { - let api_key = selected_key - .api_key - .as_deref() + selected_key: &ModelKey, +) -> Result { + session_model .filter(|value| !value.trim().is_empty()) - .ok_or_else(|| "Codex Atlas Cloud session requires an API key".to_string())?; - let model_id = atlascloud_model_id(session_model, selected_key); - let base_url = selected_key + .or_else(|| selected_key.enabled_models.first().map(String::as_str)) + .or_else(|| selected_key.available_models.first().map(String::as_str)) + .map(|model| normalize_codex_provider_model_id(model, &selected_key.model_type)) + .ok_or_else(|| { + format!( + "Codex provider {} requires an explicit Responses-compatible model", + selected_key.model_type.as_str() + ) + }) +} + +pub(super) fn normalize_codex_provider_model_id(model: &str, provider: &ModelType) -> String { + let model = model.trim(); + match provider { + // Apply the equivalent namespace removal for direct OpenAI keys. + ModelType::OpenaiApi => model.strip_prefix("openai/").unwrap_or(model).to_string(), + // ZenMux requires provider/model slugs, so preserve them exactly. + // Zhipu and Atlas Cloud are rejected by the compatibility gate because + // their coding endpoints expose Chat Completions but not Responses. + _ => model.to_string(), + } +} + +fn codex_compatible_base_url(selected_key: &ModelKey) -> Result { + selected_key .base_url .as_deref() .filter(|value| !value.trim().is_empty()) - .unwrap_or(ATLASCLOUD_BASE_URL); + .map(str::to_string) + .or_else(|| { + key_vault::provider_config::get_provider_config(selected_key.model_type.as_str()) + .default_base_url + }) + .ok_or_else(|| { + format!( + "Codex compatible provider {} requires a base URL", + selected_key.model_type.as_str() + ) + }) +} + +/// Direct OpenAI keys must keep Codex's built-in `openai` provider, which +/// already targets the official endpoint over Responses with native OpenAI +/// auth, WebSocket support and Codex's own retry defaults. Routing them through +/// the synthetic compatible-provider table downgrades all four for no benefit. +/// A custom endpoint override is the one case that still needs the table. +pub(super) fn codex_needs_compatible_profile(selected_key: &ModelKey) -> bool { + if selected_key.model_type != ModelType::OpenaiApi { + return true; + } + + let Some(base_url) = selected_key + .base_url + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + else { + return false; + }; + + key_vault::provider_config::get_provider_config(ModelType::OpenaiApi.as_str()) + .default_base_url + .as_deref() + .is_none_or(|official| !codex_endpoints_match(base_url, official)) +} + +fn codex_endpoints_match(left: &str, right: &str) -> bool { + left.trim_end_matches('/') == right.trim_end_matches('/') +} + +/// Drop a profile an earlier session wrote for this key. Without this a key +/// that had a custom endpoint and then had it cleared would keep routing +/// through the stale `orgii_compatible` table forever. Only ORGII-authored +/// profiles are removed, so anything Codex persisted itself survives. +pub(super) fn clear_codex_compatible_profile(profile_home: &Path) -> Result<(), String> { + let config_path = profile_home.join("config.toml"); + let Ok(existing) = std::fs::read_to_string(&config_path) else { + return Ok(()); + }; + if !existing.contains(CODEX_COMPATIBLE_PROVIDER_ID) { + return Ok(()); + } + + match std::fs::remove_file(&config_path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(format!("Failed to clear stale Codex config: {}", err)), + } +} + +pub(super) fn validate_codex_own_key_provider(selected_key: &ModelKey) -> Result<(), String> { + let provider = selected_key.model_type.as_str(); + if selected_key.model_type == ModelType::Codex + || key_vault::is_cli_provider_compatible("codex", provider) + { + return Ok(()); + } + + Err(format!( + "Provider {provider} is not registered as Responses-compatible with Codex CLI" + )) +} + +/// Write the Codex custom-provider profile used by every cross-provider +/// own-key session. Codex no longer supports the legacy `wire_api = "chat"` +/// setting: OpenAI-compatible providers must use Responses, and providers that +/// do not explicitly advertise WebSocket support must stay on HTTP/SSE. +pub(super) fn setup_codex_compatible_profile( + profile_home: &Path, + selected_key: &ModelKey, + session_model: Option<&str>, + env_vars: &mut HashMap, +) -> Result<(), String> { + let base_url = codex_compatible_base_url(selected_key)?; + let quoted_base_url = serde_json::to_string(&base_url).map_err(|err| err.to_string())?; + let provider_name = format!("ORGII {}", selected_key.model_type.as_str()); + let quoted_provider_name = + serde_json::to_string(&provider_name).map_err(|err| err.to_string())?; + let model_id = codex_compatible_model_id(session_model, selected_key)?; let quoted_model = serde_json::to_string(&model_id).map_err(|err| err.to_string())?; - let quoted_base_url = serde_json::to_string(base_url).map_err(|err| err.to_string())?; + let request_max_retries = agent_cli::managed_config::CODEX_REQUEST_MAX_RETRIES; + let stream_max_retries = agent_cli::managed_config::CODEX_STREAM_MAX_RETRIES; let config = format!( - "model_provider = \"{ATLASCLOUD_CODEX_PROVIDER_ID}\"\n\ + "model_provider = \"{CODEX_COMPATIBLE_PROVIDER_ID}\"\n\ model = {quoted_model}\n\n\ - [model_providers.{ATLASCLOUD_CODEX_PROVIDER_ID}]\n\ - name = \"atlascloud\"\n\ + [model_providers.{CODEX_COMPATIBLE_PROVIDER_ID}]\n\ + name = {quoted_provider_name}\n\ base_url = {quoted_base_url}\n\ - wire_api = \"chat\"\n\ - requires_openai_auth = true\n" + env_key = \"OPENAI_API_KEY\"\n\ + wire_api = \"responses\"\n\ + requires_openai_auth = false\n\ + supports_websockets = false\n\ + request_max_retries = {request_max_retries}\n\ + stream_max_retries = {stream_max_retries}\n" ); std::fs::create_dir_all(profile_home) @@ -233,12 +342,9 @@ pub(super) fn setup_codex_atlascloud_profile( std::fs::write(profile_home.join("config.toml"), config) .map_err(|err| format!("Failed to write Codex config: {}", err))?; - let auth_bytes = serde_json::to_vec_pretty(&serde_json::json!({ - "OPENAI_API_KEY": api_key - })) - .map_err(|err| err.to_string())?; - std::fs::write(profile_home.join("auth.json"), auth_bytes) - .map_err(|err| format!("Failed to write Codex auth: {}", err))?; + // The custom provider table is the single source of truth for routing; + // do not leave a second endpoint override for the child to interpret. + env_vars.remove("OPENAI_BASE_URL"); Ok(()) } @@ -286,6 +392,37 @@ pub(super) async fn start_session_mitm_proxy( /// Set up per-agent config/home directories and auth-profile state on the /// child's environment (Cursor, Claude Code, Codex own-key, OpenCode ZenMux, /// Kiro). Also clears any stale Kiro session lock for a resumed conversation. +pub(super) fn setup_codex_hosted_profile( + session_id: &str, + proxy_url: Option<&str>, + env_vars: &mut HashMap, +) -> Result<(), String> { + let proxy_url = proxy_url + .map(str::trim) + .filter(|value| !value.is_empty()) + .ok_or_else(|| "Codex hosted session requires a proxy URL".to_string())?; + if env_vars + .get("PROXY_TOKEN") + .map(|value| value.trim().is_empty()) + .unwrap_or(true) + { + return Err("Codex hosted session requires PROXY_TOKEN".to_string()); + } + + let codex_home = app_paths::codex_hosted_cli_profile_dir(session_id); + agent_cli::managed_config::write_codex_hosted_profile(&codex_home, proxy_url) + .map_err(|err| format!("Failed to setup hosted Codex profile: {err}"))?; + env_vars.insert( + "CODEX_HOME".to_string(), + codex_home.to_string_lossy().to_string(), + ); + tracing::info!( + "[CodeSession] Hosted Codex CODEX_HOME={}", + codex_home.display() + ); + Ok(()) +} + #[allow(clippy::too_many_arguments)] pub(super) fn configure_agent_profile( agent: &ModelType, @@ -353,15 +490,29 @@ pub(super) fn configure_agent_profile( "CODEX_HOME".to_string(), codex_home.to_string_lossy().to_string(), ); - write_codex_cli_auth_file(account_id, env_vars); - if selected_key.is_some_and(|key| key.model_type == ModelType::AtlascloudApi) { - let selected_key = selected_key - .ok_or_else(|| "Codex Atlas Cloud session requires a selected key".to_string())?; - setup_codex_atlascloud_profile(&codex_home, selected_key, session.model.as_deref()) - .map_err(|err| format!("Failed to setup Codex Atlas Cloud profile: {}", err))?; + let selected_key = selected_key + .ok_or_else(|| "Codex CLI own-key session requires a selected key".to_string())?; + validate_codex_own_key_provider(selected_key)?; + write_codex_cli_auth_file(account_id, selected_key, env_vars)?; + if selected_key.model_type.is_api_key_provider() + && codex_needs_compatible_profile(selected_key) + { + setup_codex_compatible_profile( + &codex_home, + selected_key, + session.model.as_deref(), + env_vars, + ) + .map_err(|err| format!("Failed to setup Codex compatible provider profile: {err}"))?; + } else if selected_key.model_type.is_api_key_provider() { + clear_codex_compatible_profile(&codex_home)?; } } + if matches!(agent, ModelType::Codex) && session.key_source == KeySource::HostedKey { + setup_codex_hosted_profile(session_id, session.proxy_url.as_deref(), env_vars)?; + } + if matches!(agent, ModelType::OpenCode) && session.key_source == KeySource::OwnKey && selected_key.is_some_and(|key| { @@ -511,134 +662,6 @@ pub(super) fn apply_system_proxy_passthrough(env_vars: &mut HashMap { - tracing::info!( - "[CodeSession] Wrote codex proxy config to {:?}", - config_file - ) - } - Err(err) => { - tracing::warn!("[CodeSession] Failed to write codex config.toml: {}", err) - } - } - } - } - } -} - -/// For a Codex hosted-key session: ensure `~/.codex/config.toml` has the proxy -/// `model_providers.proxy` section, then run `codex login --with-api-key` with -/// the proxy token. No-op for any other agent/key-source. Failures are logged -/// and the session continues. -pub(super) async fn setup_codex_hosted_proxy( - agent: &ModelType, - session: &CodeSession, - env_vars: &HashMap, -) { - if !(matches!(agent, ModelType::Codex) && session.key_source == KeySource::HostedKey) { - return; - } - - let proxy_url = session.proxy_url.clone().unwrap_or_default(); - if let Err(err) = tokio::task::spawn_blocking(move || { - ensure_codex_hosted_proxy_config(&proxy_url); - }) - .await - { - tracing::warn!("[CodeSession] Codex proxy config task failed: {err}"); - } - - let api_key_val = session.proxy_token.as_deref().unwrap_or(""); - if !api_key_val.is_empty() { - let codex_bin = resolve_cli_binary_command(CliBinaryId::Codex); - tracing::info!( - "[CodeSession] Running codex login --with-api-key via {}...", - codex_bin - ); - let mut login_cmd = Command::new(&codex_bin); - login_cmd - .arg("login") - .arg("--with-api-key") - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .envs(env_vars); - // Windows: don't flash a console window for `codex login`. - #[cfg(windows)] - login_cmd.creation_flags(app_platform::CREATE_NO_WINDOW); - match login_cmd.spawn() { - Ok(mut login_child) => { - if let Some(mut stdin) = login_child.stdin.take() { - use tokio::io::AsyncWriteExt; - let _ = stdin.write_all(api_key_val.as_bytes()).await; - drop(stdin); - } - match login_child.wait().await { - Ok(status) if status.success() => { - tracing::info!("[CodeSession] codex login succeeded"); - } - Ok(status) => { - tracing::warn!( - "[CodeSession] codex login failed (exit {:?}) — continuing anyway", - status.code() - ); - } - Err(err) => { - tracing::warn!( - "[CodeSession] codex login wait error: {} — continuing anyway", - err - ); - } - } - } - Err(err) => { - tracing::warn!( - "[CodeSession] Failed to spawn codex login: {} — continuing anyway", - err - ); - } - } - } -} - /// For an OpenCode session with an Anthropic `baseURL` configured, start the /// local SSE sanitizer proxy and repoint `ANTHROPIC_BASE_URL` at it. No-op for /// any other agent. Failures fall back to a direct connection. diff --git a/src-tauri/src/agent_sessions/cli/session_runner/finalize.rs b/src-tauri/src/agent_sessions/cli/session_runner/finalize.rs index 71d418624..a4fcb3542 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/finalize.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/finalize.rs @@ -7,15 +7,16 @@ //! proxy / proxy token / synced skill files. Extracted from //! `session::run_session`. -use std::collections::VecDeque; +use std::collections::{HashSet, VecDeque}; use std::sync::Arc; use tokio::sync::Mutex; use key_vault::key_store::{ModelType, KEY_SERVICE}; +use super::super::parsers::{canonicalize_cli_error_message, is_codex_fallback_metadata_notice}; use super::super::persistence::{self, CodeSession}; -use super::super::types::{KeySource, SessionStatus}; +use super::super::types::SessionStatus; use super::cursor_usage::fetch_cursor_usage_for_session; use super::helpers::{clear_live_status, flush_and_broadcast}; use super::oauth_setup::is_cli_oauth_failure_message; @@ -31,10 +32,96 @@ pub(super) struct SessionRunOutcome { /// App-server transport: whether the turn reached a non-failed /// `turn/completed`. pub codex_app_server_turn_ok: bool, - pub suppressed_oauth_error: Option, + /// OAuth error that is terminal after refresh failed or the one retry was + /// exhausted. A successfully refreshed first-attempt error is never kept. + pub terminal_oauth_error: Option, + /// Structured error emitted by the CLI transport. This is authoritative + /// over stderr, which may contain only launch/progress notices. + pub terminal_error_message: Option, pub stderr_lines: Arc>>, } +fn is_meaningful_stderr_line(line: &str) -> bool { + // Keep this fallback in step with what the structured parsers suppress. + // `not found` below would otherwise re-promote a notice the parser + // deliberately dropped into the persisted failure message. + if is_codex_fallback_metadata_notice(line) { + return false; + } + + let lower = line.to_lowercase(); + lower.contains("error") + || lower.contains("fatal") + || lower.contains("panic") + || lower.contains("fail") + || lower.contains("exception") + || lower.contains("timed out") + || lower.contains("timeout") + || lower.contains("refused") + || lower.contains("denied") + || lower.contains("not found") + || lower.contains("refresh token") + || lower.contains("access token") + || lower.contains("oauth") + || lower.contains("unauthorized") + || lower.contains("not authenticated") + || lower.contains("authentication") + || lower.contains("login required") + || lower.contains("please log in") + || lower.contains("please login") + || lower.contains("revoked") + || lower.contains("invalid_grant") +} + +/// Collapse retry diagnostics into one persisted error string. Structured CLI +/// logs prefix every retry with a fresh timestamp, so exact-line dedup alone +/// still rendered the same failure many times. +pub(super) fn summarize_cli_stderr(stderr_lines: &VecDeque) -> Option { + let mut seen = HashSet::new(); + let mut meaningful = Vec::new(); + + for line in stderr_lines + .iter() + .map(|line| line.trim()) + .filter(|line| !line.is_empty() && is_meaningful_stderr_line(line)) + { + let diagnostic = [" ERROR ", " WARN ", " FATAL "] + .iter() + .find_map(|marker| line.find(marker).map(|index| &line[index + marker.len()..])) + .unwrap_or(line) + .trim(); + let diagnostic = canonicalize_cli_error_message(diagnostic); + if seen.insert(diagnostic.clone()) { + meaningful.push(diagnostic); + } + } + + if meaningful.is_empty() { + // Nothing looked like a failure. Fall back to the last real line, but + // still never to a notice the parser deliberately suppressed — + // otherwise the filter above only holds while some *other* line + // happens to match, which is not a property worth having. + stderr_lines + .iter() + .rev() + .map(|line| line.trim()) + .find(|line| !line.is_empty() && !is_codex_fallback_metadata_notice(line)) + .map(str::to_string) + } else { + Some(meaningful.join("\n")) + } +} + +pub(super) fn resolve_cli_failure_message( + terminal_oauth_error: Option, + terminal_error_message: Option, + stderr_lines: &VecDeque, +) -> Option { + terminal_oauth_error + .or(terminal_error_message) + .or_else(|| summarize_cli_stderr(stderr_lines)) +} + /// Finalize a completed (or timed-out) session run: derive and persist the /// terminal status, surface an error message, run the terminal-transition /// side effects, and release per-session resources. @@ -42,6 +129,7 @@ pub(super) struct SessionRunOutcome { pub(super) async fn finalize_session_run( session: &CodeSession, agent: &ModelType, + oauth_retry_eligible: bool, env_vars: &std::collections::HashMap, run_started_at: chrono::DateTime, needs_mitm: bool, @@ -56,21 +144,21 @@ pub(super) async fn finalize_session_run( cli_session_id_out, cli_plan_approval_gate_reached, codex_app_server_turn_ok, - suppressed_oauth_error, + terminal_oauth_error, + terminal_error_message, stderr_lines, } = outcome; let session_id = session.session_id.as_str(); let account_id = session.account_id.as_deref(); - let setup_is_codex_own_key = - *agent == ModelType::Codex && session.key_source == KeySource::OwnKey; + let setup_is_codex_oauth = *agent == ModelType::Codex && oauth_retry_eligible; let setup_access_token = env_vars.get("OPENAI_API_KEY").cloned(); let setup_account_id = account_id.map(str::to_string); let setup_session_id = session_id.to_string(); let setup_cli_session_id = cli_session_id_out.clone(); let _ = tokio::task::spawn_blocking(move || { - if setup_is_codex_own_key { + if setup_is_codex_oauth { if let Err(err) = sync_codex_cli_auth_to_key_vault( setup_account_id.as_deref(), setup_access_token.as_deref(), @@ -136,50 +224,14 @@ pub(super) async fn finalize_session_run( }; let error_message: Option = if final_status == SessionStatus::Failed { - if let Some(message) = suppressed_oauth_error.clone() { - Some(message) - } else { - let buf = stderr_lines.lock().await; - let meaningful: Vec<&str> = buf - .iter() - .map(|s| s.as_str()) - .filter(|line| { - let lower = line.to_lowercase(); - lower.contains("error") - || lower.contains("fatal") - || lower.contains("panic") - || lower.contains("fail") - || lower.contains("exception") - || lower.contains("timed out") - || lower.contains("timeout") - || lower.contains("refused") - || lower.contains("denied") - || lower.contains("not found") - || lower.contains("refresh token") - || lower.contains("access token") - || lower.contains("oauth") - || lower.contains("unauthorized") - || lower.contains("not authenticated") - || lower.contains("authentication") - || lower.contains("login required") - || lower.contains("please log in") - || lower.contains("please login") - || lower.contains("revoked") - || lower.contains("invalid_grant") - }) - .collect(); - if meaningful.is_empty() { - buf.back().map(|s| s.to_string()) - } else { - Some(meaningful.join("\n")) - } - } + let buf = stderr_lines.lock().await; + resolve_cli_failure_message(terminal_oauth_error.clone(), terminal_error_message, &buf) } else { None }; let should_record_oauth_failure = *agent == ModelType::Codex - && session.key_source == KeySource::OwnKey + && oauth_retry_eligible && error_message .as_deref() .is_some_and(is_cli_oauth_failure_message); diff --git a/src-tauri/src/agent_sessions/cli/session_runner/oauth_setup.rs b/src-tauri/src/agent_sessions/cli/session_runner/oauth_setup.rs index 42c7c86a7..a2e569995 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/oauth_setup.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/oauth_setup.rs @@ -6,7 +6,7 @@ use std::collections::HashMap; use chrono::{SecondsFormat, Utc}; use core_types::activity::ActivityChunk; use core_types::providers::{CODEX_ID_TOKEN_ENV_KEY, CODEX_REFRESH_TOKEN_ENV_KEY}; -use key_vault::key_store::{ModelType, KEY_SERVICE}; +use key_vault::key_store::{ModelKey, ModelType, KEY_SERVICE}; use super::super::types::KeySource; @@ -72,28 +72,28 @@ pub(super) fn is_retryable_overloaded_chunk(chunk: &ActivityChunk) -> Option bool { - matches!(agent, ModelType::Codex | ModelType::ClaudeCode) +pub(super) fn is_cli_oauth_retry_eligible( + agent: &ModelType, + key_source: KeySource, + selected_key: Option<&ModelKey>, +) -> bool { + key_source == KeySource::OwnKey + && selected_key.is_some_and(|key| key.is_native_oauth_for(agent)) } pub(super) fn is_cli_oauth_stderr_retry_candidate( - agent: &ModelType, - key_source: KeySource, + oauth_retry_eligible: bool, exit_code: i32, replay_unsafe_output_seen: bool, ) -> bool { - key_source == KeySource::OwnKey - && exit_code != 0 - && !replay_unsafe_output_seen - && is_cli_oauth_retry_agent(agent) + oauth_retry_eligible && exit_code != 0 && !replay_unsafe_output_seen } pub(super) fn is_retryable_cli_oauth_failure_chunk( - agent: &ModelType, - key_source: KeySource, + oauth_retry_eligible: bool, chunk: &ActivityChunk, ) -> Option { - if key_source != KeySource::OwnKey || !is_cli_oauth_retry_agent(agent) { + if !oauth_retry_eligible { return None; } let message = chunk_error_message(chunk)?; @@ -141,49 +141,106 @@ pub(super) fn sanitize_cli_oauth_env_for_child( // ── Auth file writers ───────────────────────────────────────────────────────── -pub(super) fn write_codex_cli_auth_file(account_id: &str, env_vars: &HashMap) { - let codex_home = app_paths::codex_cli_profile_dir(account_id); - if let Err(err) = std::fs::create_dir_all(&codex_home) { - tracing::warn!("[CodeSession] Failed to create Codex home: {}", err); - return; +pub(super) fn codex_cli_auth_payload( + selected_key: &ModelKey, + env_vars: &HashMap, +) -> Result { + if !selected_key.is_native_oauth_for(&ModelType::Codex) { + let api_key = selected_key + .api_key + .as_deref() + .or_else(|| env_vars.get("OPENAI_API_KEY").map(String::as_str)) + .filter(|value| !value.trim().is_empty()) + .ok_or_else(|| format!("Codex API-key profile {} has no API key", selected_key.id))?; + return Ok(serde_json::json!({ "OPENAI_API_KEY": api_key })); } - let home_path = codex_home.to_string_lossy().to_string(); - tracing::info!("[CodeSession] CODEX_HOME={}", home_path); - - let auth_path = codex_home.join("auth.json"); - let access_token = env_vars.get("OPENAI_API_KEY").cloned().unwrap_or_default(); - let refresh_token = env_vars.get(CODEX_REFRESH_TOKEN_ENV_KEY).cloned(); - let id_token = env_vars.get(CODEX_ID_TOKEN_ENV_KEY).cloned(); - let account_id_from_token = id_token.as_deref().and_then(|token| { + let access_token = selected_key + .session_token + .as_deref() + .or_else(|| env_vars.get("OPENAI_API_KEY").map(String::as_str)) + .filter(|value| !value.trim().is_empty()) + .ok_or_else(|| { + format!( + "Codex OAuth profile {} has no access token", + selected_key.id + ) + })?; + let refresh_token = selected_key + .env_vars + .get(CODEX_REFRESH_TOKEN_ENV_KEY) + .map(String::as_str) + .or_else(|| { + env_vars + .get(CODEX_REFRESH_TOKEN_ENV_KEY) + .map(String::as_str) + }) + .filter(|value| !value.trim().is_empty()); + let id_token = selected_key + .env_vars + .get(CODEX_ID_TOKEN_ENV_KEY) + .map(String::as_str) + .or_else(|| env_vars.get(CODEX_ID_TOKEN_ENV_KEY).map(String::as_str)) + .filter(|value| !value.trim().is_empty()); + let account_id_from_token = id_token.and_then(|token| { agent_core::core::providers::codex_native::extract_account_id_from_id_token(token) }); - if access_token.trim().is_empty() { - return; + let mut tokens = serde_json::Map::new(); + tokens.insert( + "access_token".to_string(), + serde_json::Value::String(access_token.to_string()), + ); + if let Some(refresh_token) = refresh_token { + tokens.insert( + "refresh_token".to_string(), + serde_json::Value::String(refresh_token.to_string()), + ); + } + if let Some(id_token) = id_token { + tokens.insert( + "id_token".to_string(), + serde_json::Value::String(id_token.to_string()), + ); + } + if let Some(account_id) = account_id_from_token { + tokens.insert( + "account_id".to_string(), + serde_json::Value::String(account_id), + ); } - let last_refresh = Utc::now().to_rfc3339_opts(SecondsFormat::Micros, true); - let auth_json = serde_json::json!({ + Ok(serde_json::json!({ "OPENAI_API_KEY": serde_json::Value::Null, - "tokens": { - "access_token": access_token, - "refresh_token": refresh_token, - "id_token": id_token, - "account_id": account_id_from_token, - }, - "last_refresh": last_refresh, - }); - let write_result = serde_json::to_vec_pretty(&auth_json) - .map_err(|err| err.to_string()) - .and_then(|bytes| std::fs::write(&auth_path, bytes).map_err(|err| err.to_string())); - match write_result { - Ok(()) => tracing::info!( - "[CodeSession] Wrote fresh Codex auth.json to {:?}", - auth_path - ), - Err(err) => tracing::warn!("[CodeSession] Failed to write Codex auth.json: {}", err), - } + "tokens": tokens, + "last_refresh": Utc::now().to_rfc3339_opts(SecondsFormat::Micros, true), + })) +} + +pub(super) fn write_codex_cli_auth_file( + account_id: &str, + selected_key: &ModelKey, + env_vars: &HashMap, +) -> Result<(), String> { + let codex_home = app_paths::codex_cli_profile_dir(account_id); + std::fs::create_dir_all(&codex_home) + .map_err(|err| format!("Failed to create Codex home: {err}"))?; + + let home_path = codex_home.to_string_lossy().to_string(); + tracing::info!("[CodeSession] CODEX_HOME={}", home_path); + + let auth_path = codex_home.join("auth.json"); + let auth_json = codex_cli_auth_payload(selected_key, env_vars)?; + let bytes = serde_json::to_vec_pretty(&auth_json).map_err(|err| err.to_string())?; + // Atomic replace: a crash mid-write must not leave a truncated auth.json + // that silently downgrades the next launch to unauthenticated. The chmod + // stays fatal here — this file holds the credential itself. + agent_cli::managed_config::write_cli_profile_file_atomic(&auth_path, &bytes) + .map_err(|err| format!("Failed to write Codex auth.json: {err}"))?; + app_paths::set_sensitive_file_permissions(&auth_path) + .map_err(|err| format!("Failed to secure Codex auth.json: {err}"))?; + tracing::info!("[CodeSession] Wrote Codex auth.json to {:?}", auth_path); + Ok(()) } // ── OAuth refresh for retry ─────────────────────────────────────────────────── @@ -197,8 +254,8 @@ pub(super) async fn refresh_cli_oauth_for_retry( return Ok(false); }; - let refreshed = match agent { - ModelType::Codex => Some( + let outcome = match agent { + ModelType::Codex => { KEY_SERVICE .refresh_codex_oauth_key( account_id, @@ -207,9 +264,9 @@ pub(super) async fn refresh_cli_oauth_for_retry( .map(String::as_str) .unwrap_or(""), ) - .await?, - ), - ModelType::ClaudeCode => Some( + .await? + } + ModelType::ClaudeCode => { KEY_SERVICE .refresh_claude_code_oauth_key( account_id, @@ -218,14 +275,14 @@ pub(super) async fn refresh_cli_oauth_for_retry( .map(String::as_str) .unwrap_or(""), ) - .await?, - ), - _ => None, + .await? + } + _ => return Ok(false), }; - if refreshed.is_none() { + let Some(refreshed_key) = outcome.into_key() else { return Ok(false); - } + }; let refreshed_env = KEY_SERVICE.get_env_for_agent(agent, Some(account_id)); for (key, value) in refreshed_env { @@ -235,10 +292,10 @@ pub(super) async fn refresh_cli_oauth_for_retry( let auth_account_id = account_id.to_string(); let auth_env = env_vars.clone(); tokio::task::spawn_blocking(move || { - write_codex_cli_auth_file(&auth_account_id, &auth_env); + write_codex_cli_auth_file(&auth_account_id, &refreshed_key, &auth_env) }) .await - .map_err(|err| format!("Codex auth persistence task failed: {err}"))?; + .map_err(|err| format!("Codex auth persistence task failed: {err}"))??; } sanitize_cli_oauth_env_for_child(agent, env_vars); Ok(true) diff --git a/src-tauri/src/agent_sessions/cli/session_runner/session.rs b/src-tauri/src/agent_sessions/cli/session_runner/session.rs index 6081874dd..c3e6a2216 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/session.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/session.rs @@ -25,8 +25,10 @@ use super::super::types::KeySource; use super::command::{ build_command_with_launch_profile, launch_profile_env, CliCommandBuildRequest, }; -use super::helpers::{persist_attached_images, strip_ide_context}; -use super::oauth_setup::{refresh_cli_oauth_for_retry, sanitize_cli_oauth_env_for_child}; +use super::helpers::{emit_chunk, persist_attached_images, strip_ide_context}; +use super::oauth_setup::{ + is_cli_oauth_retry_eligible, refresh_cli_oauth_for_retry, sanitize_cli_oauth_env_for_child, +}; mod skills_resolve; mod spawn_retry; @@ -37,16 +39,171 @@ mod transport_standard; use skills_resolve::resolve_sde_skills; use spawn_retry::{is_transient_spawn_error, SPAWN_RETRY_ATTEMPTS, SPAWN_RETRY_BASE_DELAY_MS}; -fn resolve_session_model<'a>( +const MAX_OVERLOAD_RETRIES: u32 = 3; +const OVERLOAD_RETRY_BASE_DELAY_SECS: u64 = 2; + +const MAX_STDERR_LINES: usize = 20; + +/// How long to keep waiting for the stderr reader once the child is gone. A +/// CLI that hands its stderr to a surviving grandchild keeps the pipe open +/// forever, and no diagnostic is worth hanging the turn on. +const STDERR_DRAIN_TIMEOUT: tokio::time::Duration = tokio::time::Duration::from_secs(3); + +/// The child's stderr, collected by a background reader. +/// +/// The reader must be drained before the buffer is read. A child exiting only +/// closes the write end of the pipe; it says nothing about whether the reader +/// task has been scheduled to pick up what is still sitting in it. Reading +/// straight after `wait()` is how a session that failed loudly on stderr ends +/// up reporting no reason at all. +struct CliStderrCollector { + lines: Arc>>, + reader: Option>, +} + +impl CliStderrCollector { + fn new() -> Self { + Self { + lines: Arc::new(Mutex::new(VecDeque::with_capacity(MAX_STDERR_LINES))), + reader: None, + } + } + + /// The buffer itself, for readers that have already drained (or that run + /// after the turn, once draining is guaranteed). + fn lines(&self) -> Arc>> { + Arc::clone(&self.lines) + } + + fn attach(&mut self, stderr: tokio::process::ChildStderr, session_id: String) { + let sink = Arc::clone(&self.lines); + self.reader = Some(tokio::spawn(async move { + use tokio::io::AsyncBufReadExt; + let mut reader = BufReader::new(stderr).lines(); + while let Ok(Some(line)) = reader.next_line().await { + tracing::warn!("[CodeSession][stderr][{}] {}", session_id, line); + let mut buf = sink.lock().await; + if buf.len() >= MAX_STDERR_LINES { + buf.pop_front(); + } + buf.push_back(line); + } + })); + } + + /// Wait for the reader to hit EOF so the buffer holds everything the child + /// wrote. Idempotent — every consumer may call it, and only the first one + /// pays. Whatever was collected before the deadline stays in the buffer. + /// + /// On timeout the reader is aborted rather than abandoned: dropping a + /// `JoinHandle` only detaches the task. A reader parked on a pipe that a + /// surviving grandchild still holds would then keep itself, the + /// `ChildStderr` fd and a buffer handle alive for as long as that + /// grandchild lives — once per attempt, for the life of the process. + async fn drain(&mut self) { + let Some(mut reader) = self.reader.take() else { + return; + }; + if tokio::time::timeout(STDERR_DRAIN_TIMEOUT, &mut reader) + .await + .is_err() + { + tracing::warn!( + "[CodeSession] stderr still open {}s after the child exited; using what was collected so far", + STDERR_DRAIN_TIMEOUT.as_secs() + ); + reader.abort(); + // Awaited so the fd is closed by the time the caller reads the + // buffer, not whenever the cancelled task happens to be dropped. + let _ = reader.await; + } + } +} + +fn terminal_cli_error_from_chunk(chunk: &core_types::activity::ActivityChunk) -> Option { + let is_error = chunk.action_type == "error" || chunk.function == "error"; + let is_failed_session_end = (chunk.action_type == "session_end" + || chunk.function == "session_end") + && chunk + .result + .get("success") + .and_then(serde_json::Value::as_bool) + == Some(false); + if !is_error && !is_failed_session_end { + return None; + } + + ["error", "error_message", "observation"] + .iter() + .find_map(|field| { + let value = chunk.result.get(*field)?; + value + .as_str() + .or_else(|| value.get("message").and_then(serde_json::Value::as_str)) + }) + .map(super::super::parsers::canonicalize_cli_error_message) + .filter(|message| !message.is_empty()) +} + +fn record_terminal_cli_error( + current: &mut Option, + chunk: &core_types::activity::ActivityChunk, +) { + let Some(message) = terminal_cli_error_from_chunk(chunk) else { + return; + }; + let is_failed_session_end = (chunk.action_type == "session_end" + || chunk.function == "session_end") + && chunk + .result + .get("success") + .and_then(serde_json::Value::as_bool) + == Some(false); + if current.is_none() || is_failed_session_end { + *current = Some(message); + } +} + +fn is_app_overload_retry_eligible(agent: &ModelType) -> bool { + !matches!(agent, ModelType::Codex) +} + +fn exhausted_overload_error_chunk( + session_id: &str, + overload_retry_count: u32, + message: &str, +) -> Option<(String, core_types::activity::ActivityChunk)> { + if overload_retry_count < MAX_OVERLOAD_RETRIES { + return None; + } + + let message = super::super::parsers::canonicalize_cli_error_message(message); + let mut chunk = core_types::activity::ActivityChunk::new(session_id, "error", "error"); + chunk.result = serde_json::json!({ + "observation": message, + "error": message, + "success": false, + }); + Some((message, chunk)) +} + +fn resolve_session_model( agent: &ModelType, key_model_type: Option<&ModelType>, - session_model: Option<&'a str>, -) -> Option<&'a str> { + session_model: Option<&str>, +) -> Option { let is_cross_type_key = key_model_type.is_some_and(|key_type| key_type != agent); if is_cross_type_key && matches!(agent, ModelType::ClaudeCode) { None } else { - session_model + session_model.map(|model| { + if matches!(agent, ModelType::Codex) { + if let Some(provider) = key_model_type { + return super::env_setup::normalize_codex_provider_model_id(model, provider); + } + } + model.to_string() + }) } } @@ -101,6 +258,11 @@ pub async fn run_session( } } let key_model_type = selected_key.as_ref().map(|key| key.model_type.clone()); + let oauth_retry_eligible = + is_cli_oauth_retry_eligible(&agent, session.key_source, selected_key.as_ref()); + // Codex custom providers retain bounded request/stream retries inside the + // same process. Do not multiply those by replaying the entire turn here. + let overload_retry_eligible = is_app_overload_retry_eligible(&agent); let model = resolve_session_model(&agent, key_model_type.as_ref(), session.model.as_deref()); let repo_path = session.repo_path.as_deref(); let account_id = session.account_id.as_deref(); @@ -223,7 +385,7 @@ pub async fn run_session( let mut cmd_parts = build_command_with_launch_profile(CliCommandBuildRequest { agent: &agent, launch_profile: &launch_profile, - model, + model: model.as_deref(), task: &effective_input, resume_id: cli_resume_id.as_deref(), api_key: api_key_for_cli, @@ -379,8 +541,6 @@ pub async fn run_session( tracing::info!("[CodeSession] env {}={}", key, display_val); } - super::env_setup::setup_codex_hosted_proxy(&agent, &session, &env_vars).await; - super::env_setup::setup_opencode_sse_sanitizer(&agent, &mut env_vars).await; // ── Spawn subprocess ── @@ -389,14 +549,12 @@ pub async fn run_session( ModelType::Copilot | ModelType::Kiro | ModelType::OpenCode ); - const MAX_STDERR_LINES: usize = 20; let mut stderr_lines: Arc>>; let mut exit_code: i32; let mut oauth_retry_used = false; - let mut suppressed_oauth_error: Option = None; + let mut terminal_oauth_error: Option = None; + let mut terminal_error_message: Option = None; let mut overload_retry_count: u32 = 0; - const MAX_OVERLOAD_RETRIES: u32 = 3; - const OVERLOAD_RETRY_BASE_DELAY_SECS: u64 = 2; let base_sequence: i64 = persistence::max_chunk_sequence(&session_id).unwrap_or(-1) + 1; @@ -461,9 +619,8 @@ pub async fn run_session( let session_timeout = tokio::time::Duration::from_secs(4 * 60 * 60); loop { - let attempt_stderr_lines: Arc>> = - Arc::new(Mutex::new(VecDeque::with_capacity(MAX_STDERR_LINES))); - stderr_lines = Arc::clone(&attempt_stderr_lines); + let mut attempt_stderr = CliStderrCollector::new(); + stderr_lines = attempt_stderr.lines(); let mut spawn_cmd = Command::new(program); spawn_cmd .args(args) @@ -520,21 +677,10 @@ pub async fn run_session( } } - let stderr = child.stderr.take().expect("stderr was piped"); - let stderr_session_id = session_id.clone(); - let stderr_lines_writer = Arc::clone(&attempt_stderr_lines); - tokio::spawn(async move { - use tokio::io::AsyncBufReadExt; - let mut reader = BufReader::new(stderr).lines(); - while let Ok(Some(line)) = reader.next_line().await { - tracing::warn!("[CodeSession][stderr][{}] {}", stderr_session_id, line); - let mut buf = stderr_lines_writer.lock().await; - if buf.len() >= MAX_STDERR_LINES { - buf.pop_front(); - } - buf.push_back(line); - } - }); + attempt_stderr.attach( + child.stderr.take().expect("stderr was piped"), + session_id.clone(), + ); let retryable_oauth_message: Option; let retryable_overload_message: Option; @@ -544,10 +690,11 @@ pub async fn run_session( child, session_id.clone(), account_id, + oauth_retry_eligible, effective_input.clone(), working_dir, cli_resume_id.clone(), - model, + model.as_deref(), &launch_profile, image_paths.clone(), session_timeout, @@ -556,13 +703,15 @@ pub async fn run_session( cli_session_id_out, &mut sequence, codex_app_server_turn_ok, + &mut attempt_stderr, ) .await?; exit_code = outcome.exit_code; timed_out = outcome.timed_out; cli_session_id_out = outcome.cli_session_id_out; codex_app_server_turn_ok = outcome.codex_app_server_turn_ok; - retryable_oauth_message = None; + terminal_error_message = outcome.terminal_error_message; + retryable_oauth_message = outcome.retryable_oauth_message; retryable_overload_message = None; } else if is_acp_agent { let outcome = transport_acp::run_acp_branch( @@ -590,27 +739,33 @@ pub async fn run_session( let outcome = transport_standard::run_standard_branch( child, session_id.clone(), - &session, + oauth_retry_eligible, + overload_retry_eligible, agent.clone(), mode, account_id, - model, + model.as_deref(), session_timeout, pre_message_snapshot_id.clone(), snapshot_working_dir.clone(), cli_session_id_out, &mut sequence, - Arc::clone(&attempt_stderr_lines), + &mut attempt_stderr, ) .await; exit_code = outcome.exit_code; timed_out = outcome.timed_out; cli_session_id_out = outcome.cli_session_id_out; cli_plan_approval_gate_reached = outcome.cli_plan_approval_gate_reached; + terminal_error_message = outcome.terminal_error_message; retryable_oauth_message = outcome.retryable_oauth_message; retryable_overload_message = outcome.retryable_overload_message; } + // Whatever the transport did or did not read, the finalizer reads this + // buffer for every agent. A no-op if the transport already drained. + attempt_stderr.drain().await; + if timed_out { tracing::error!( "[CodeSession] Session {} timed out after 4 hours", @@ -621,28 +776,29 @@ pub async fn run_session( if let Some(message) = retryable_oauth_message { if oauth_retry_used { - suppressed_oauth_error = Some(message); + terminal_oauth_error = Some(message); break; } oauth_retry_used = true; - suppressed_oauth_error = Some(message.clone()); tracing::warn!( "[CodeSession] {} OAuth failed before replay-unsafe output; refreshing and retrying once", agent.as_str() ); match refresh_cli_oauth_for_retry(&agent, account_id, &mut env_vars).await { Ok(true) => { + // The first failure was recovered. Do not let it outrank a + // different terminal error produced by the retried turn. continue; } Ok(false) => { - suppressed_oauth_error = Some( + terminal_oauth_error = Some( "This account needs to be signed in again before the agent can continue." .to_string(), ); break; } Err(err) => { - suppressed_oauth_error = Some(format!( + terminal_oauth_error = Some(format!( "Automatic account refresh failed. Please sign in again. {}", err )); @@ -651,14 +807,18 @@ pub async fn run_session( } } - if let Some(ref message) = retryable_overload_message { - if overload_retry_count >= MAX_OVERLOAD_RETRIES { + if let Some(message) = retryable_overload_message { + if let Some((terminal_message, chunk)) = + exhausted_overload_error_chunk(&session_id, overload_retry_count, &message) + { tracing::warn!( "[CodeSession] {} API overloaded after {} retries; giving up: {}", agent.as_str(), MAX_OVERLOAD_RETRIES, - message, + terminal_message, ); + terminal_error_message = Some(terminal_message); + emit_chunk(&chunk, &session_id, &mut sequence).await; break; } let delay_secs = OVERLOAD_RETRY_BASE_DELAY_SECS * (1u64 << overload_retry_count); @@ -685,6 +845,7 @@ pub async fn run_session( super::finalize::finalize_session_run( &session, &agent, + oauth_retry_eligible, &env_vars, run_started_at, needs_mitm, @@ -697,7 +858,8 @@ pub async fn run_session( cli_session_id_out, cli_plan_approval_gate_reached, codex_app_server_turn_ok, - suppressed_oauth_error, + terminal_oauth_error, + terminal_error_message, stderr_lines, }, ) diff --git a/src-tauri/src/agent_sessions/cli/session_runner/session/tests.rs b/src-tauri/src/agent_sessions/cli/session_runner/session/tests.rs index 932fb619f..3aa4f4d18 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/session/tests.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/session/tests.rs @@ -1,6 +1,8 @@ use super::super::env_setup::{ - atlascloud_model_id, opencode_zenmux_model_id, setup_codex_atlascloud_profile, + atlascloud_model_id, clear_codex_compatible_profile, codex_needs_compatible_profile, + opencode_zenmux_model_id, setup_codex_compatible_profile, setup_codex_hosted_profile, setup_opencode_atlascloud_profile, setup_opencode_zenmux_profile, + validate_codex_own_key_provider, }; use super::super::input_assembly::cli_exec_mode_bridge; use super::super::oauth_setup::{is_api_overloaded_message, is_retryable_overloaded_chunk}; @@ -11,9 +13,9 @@ use super::super::plan_approval::{ use super::*; use core_types::activity::ActivityChunk; use core_types::providers::{CODEX_ID_TOKEN_ENV_KEY, CODEX_REFRESH_TOKEN_ENV_KEY}; -use key_vault::key_store::ModelKey; +use key_vault::key_store::{AuthMethod, ModelKey}; use serde_json::Value; -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::path::Path; use std::sync::Mutex as StdMutex; @@ -108,24 +110,481 @@ fn atlascloud_model_id_prefers_session_model() { } #[test] -fn setup_codex_atlascloud_profile_writes_provider_model_and_auth() { +fn setup_codex_compatible_profile_writes_responses_provider_without_websocket() { let temp_dir = tempfile::tempdir().expect("temp Codex profile"); - let mut key = ModelKey::new(ModelType::AtlascloudApi); - key.api_key = Some("atlas-test-key".to_string()); - key.enabled_models = vec!["zai-org/glm-5.1".to_string()]; + let mut key = ModelKey::new(ModelType::ZenmuxApi); + key.api_key = Some("zenmux-test-key".to_string()); + key.enabled_models = vec!["z-ai/glm-5.2".to_string()]; + let mut env = HashMap::from([ + ("OPENAI_API_KEY".to_string(), "zenmux-test-key".to_string()), + ( + "OPENAI_BASE_URL".to_string(), + "https://stale.example/v1".to_string(), + ), + ]); - setup_codex_atlascloud_profile(temp_dir.path(), &key, None).expect("setup profile"); + setup_codex_compatible_profile(temp_dir.path(), &key, None, &mut env).expect("setup profile"); let config = std::fs::read_to_string(temp_dir.path().join("config.toml")).expect("read config"); - assert!(config.contains("model_provider = \"atlas_coding_plan\"")); - assert!(config.contains("model = \"zai-org/glm-5.1\"")); - assert!(config.contains("[model_providers.atlas_coding_plan]")); - assert!(config.contains("base_url = \"https://api.atlascloud.ai/v1\"")); - assert!(config.contains("wire_api = \"chat\"")); - assert!(config.contains("requires_openai_auth = true")); + assert!(config.contains("model_provider = \"orgii_compatible\"")); + assert!(config.contains("model = \"z-ai/glm-5.2\"")); + assert!(config.contains("[model_providers.orgii_compatible]")); + assert!(config.contains("base_url = \"https://zenmux.ai/api/v1\"")); + assert!(config.contains("env_key = \"OPENAI_API_KEY\"")); + assert!(config.contains("wire_api = \"responses\"")); + assert!(config.contains("requires_openai_auth = false")); + assert!(config.contains("supports_websockets = false")); + assert!(config.contains("request_max_retries = 2")); + assert!(config.contains("stream_max_retries = 2")); + assert!(!config.contains("zenmux-test-key")); + assert!(!env.contains_key("OPENAI_BASE_URL")); + assert_eq!( + env.get("OPENAI_API_KEY").map(String::as_str), + Some("zenmux-test-key") + ); +} + +#[test] +fn codex_compatible_profile_uses_zenmux_default_base_url_and_namespaced_model() { + let temp_dir = tempfile::tempdir().expect("temp Codex profile"); + let key = ModelKey::new(ModelType::ZenmuxApi); + let mut env = HashMap::new(); + + setup_codex_compatible_profile(temp_dir.path(), &key, Some("z-ai/glm-5.2"), &mut env) + .expect("setup compatible profile"); + + let config = std::fs::read_to_string(temp_dir.path().join("config.toml")).expect("read config"); + assert!(config.contains("base_url = \"https://zenmux.ai/api/v1\"")); + assert!(config.contains("model = \"z-ai/glm-5.2\"")); +} + +#[test] +fn codex_own_key_provider_gate_uses_the_central_compatibility_contract() { + for model_type in [ModelType::Codex, ModelType::OpenaiApi, ModelType::ZenmuxApi] { + let key = ModelKey::new(model_type.clone()); + assert!( + validate_codex_own_key_provider(&key).is_ok(), + "expected {} to be compatible with Codex", + model_type.as_str() + ); + } + + for model_type in [ + ModelType::AnthropicApi, + ModelType::GeminiApi, + ModelType::ClaudeCode, + ModelType::AtlascloudApi, + ModelType::ZhipuApi, + ] { + let key = ModelKey::new(model_type.clone()); + assert!( + validate_codex_own_key_provider(&key).is_err(), + "expected {} to be rejected for Codex", + model_type.as_str() + ); + } +} - let auth = read_json(&temp_dir.path().join("auth.json")); - assert_eq!(auth["OPENAI_API_KEY"].as_str(), Some("atlas-test-key")); +#[test] +fn direct_openai_keys_keep_the_builtin_codex_provider() { + // No override and the official endpoint both mean "just use Codex's own + // `openai` provider" — no synthetic table, no capability downgrade. + assert!(!codex_needs_compatible_profile(&ModelKey::new( + ModelType::OpenaiApi + ))); + + let mut official = ModelKey::new(ModelType::OpenaiApi); + official.base_url = Some("https://api.openai.com/v1/".to_string()); + assert!(!codex_needs_compatible_profile(&official)); + + let mut blank = ModelKey::new(ModelType::OpenaiApi); + blank.base_url = Some(" ".to_string()); + assert!(!codex_needs_compatible_profile(&blank)); + + // A gateway/proxy endpoint still needs the custom provider table. + let mut gateway = ModelKey::new(ModelType::OpenaiApi); + gateway.base_url = Some("https://gateway.internal/v1".to_string()); + assert!(codex_needs_compatible_profile(&gateway)); + + // Third parties always need it. + assert!(codex_needs_compatible_profile(&ModelKey::new( + ModelType::ZenmuxApi + ))); +} + +#[test] +fn clearing_a_stale_compatible_profile_spares_codex_authored_config() { + let temp_dir = tempfile::tempdir().expect("temp Codex profile"); + let config_path = temp_dir.path().join("config.toml"); + + clear_codex_compatible_profile(temp_dir.path()).expect("absent profile is not an error"); + + std::fs::write(&config_path, "model_provider = \"orgii_compatible\"\n").expect("seed profile"); + clear_codex_compatible_profile(temp_dir.path()).expect("stale ORGII profile removed"); + assert!( + !config_path.exists(), + "an endpoint override that was later cleared must not keep routing" + ); + + std::fs::write(&config_path, "model = \"gpt-5.1-codex\"\n").expect("seed foreign config"); + clear_codex_compatible_profile(temp_dir.path()).expect("foreign config preserved"); + assert!(config_path.exists()); +} + +#[test] +fn codex_compatible_profile_rejects_an_unresolved_model() { + let temp_dir = tempfile::tempdir().expect("temp Codex profile"); + let key = ModelKey::new(ModelType::ZenmuxApi); + let mut env = HashMap::new(); + + let error = setup_codex_compatible_profile(temp_dir.path(), &key, None, &mut env) + .expect_err("missing model must fail closed"); + + assert!(error.contains("explicit Responses-compatible model")); + assert!(!temp_dir.path().join("config.toml").exists()); +} + +#[test] +fn hosted_codex_profile_is_session_scoped_and_fails_closed() { + with_temp_orgii_home(|_| { + let mut env = HashMap::from([("PROXY_TOKEN".to_string(), "hosted-test-token".to_string())]); + + setup_codex_hosted_profile("hosted-session-1", Some("http://127.0.0.1:43123"), &mut env) + .expect("setup hosted profile"); + + let profile = app_paths::codex_hosted_cli_profile_dir("hosted-session-1"); + assert_eq!( + env.get("CODEX_HOME").map(String::as_str), + Some(profile.to_string_lossy().as_ref()) + ); + let config = std::fs::read_to_string(profile.join("config.toml")).unwrap(); + assert!(config.contains("base_url = \"http://127.0.0.1:43123/v1\"")); + assert!(config.contains("supports_websockets = false")); + assert!(config.contains("request_max_retries = 2")); + assert!(config.contains("stream_max_retries = 2")); + + let mut missing_token = HashMap::new(); + assert!(setup_codex_hosted_profile( + "hosted-session-2", + Some("http://127.0.0.1:43123"), + &mut missing_token, + ) + .is_err()); + assert!(!missing_token.contains_key("CODEX_HOME")); + assert!(!app_paths::codex_hosted_cli_profile_dir("hosted-session-2").exists()); + + let blocked_profile = app_paths::codex_hosted_cli_profile_dir("hosted-session-3"); + std::fs::create_dir_all(blocked_profile.parent().unwrap()).unwrap(); + std::fs::write(&blocked_profile, b"not a directory").unwrap(); + let mut blocked_env = + HashMap::from([("PROXY_TOKEN".to_string(), "hosted-test-token".to_string())]); + assert!(setup_codex_hosted_profile( + "hosted-session-3", + Some("http://127.0.0.1:43123"), + &mut blocked_env, + ) + .is_err()); + assert!(!blocked_env.contains_key("CODEX_HOME")); + }); +} + +#[test] +fn codex_api_key_profile_must_not_be_written_as_oauth_tokens() { + with_temp_orgii_home(|_| { + let account_id = "zhipu-api-key-shape"; + let mut key = ModelKey::new(ModelType::ZhipuApi); + key.api_key = Some("zhipu-test-key".to_string()); + let mut env = HashMap::new(); + env.insert("OPENAI_API_KEY".to_string(), "zhipu-test-key".to_string()); + + super::super::oauth_setup::write_codex_cli_auth_file(account_id, &key, &env) + .expect("write Codex API-key auth profile"); + + let auth = read_json(&app_paths::codex_cli_profile_dir(account_id).join("auth.json")); + assert_eq!(auth["OPENAI_API_KEY"].as_str(), Some("zhipu-test-key")); + assert!(auth.get("tokens").is_none()); + }); +} + +#[test] +fn codex_auth_payload_matches_credential_type_matrix() { + use super::super::oauth_setup::codex_cli_auth_payload; + + for model_type in [ + ModelType::Codex, + ModelType::ZhipuApi, + ModelType::ZenmuxApi, + ModelType::AtlascloudApi, + ] { + let mut key = ModelKey::new(model_type); + key.api_key = Some("provider-api-key".to_string()); + let payload = codex_cli_auth_payload(&key, &HashMap::new()).unwrap(); + assert_eq!(payload["OPENAI_API_KEY"].as_str(), Some("provider-api-key")); + assert!(payload.get("tokens").is_none()); + } + + let mut oauth_key = ModelKey::new(ModelType::Codex); + oauth_key.auth_method = AuthMethod::Oauth; + oauth_key.session_token = Some("oauth-access".to_string()); + oauth_key.env_vars.insert( + CODEX_REFRESH_TOKEN_ENV_KEY.to_string(), + "oauth-refresh".to_string(), + ); + oauth_key + .env_vars + .insert(CODEX_ID_TOKEN_ENV_KEY.to_string(), "oauth-id".to_string()); + let payload = codex_cli_auth_payload(&oauth_key, &HashMap::new()).unwrap(); + assert!(payload["OPENAI_API_KEY"].is_null()); + assert_eq!( + payload["tokens"]["access_token"].as_str(), + Some("oauth-access") + ); + assert_eq!( + payload["tokens"]["refresh_token"].as_str(), + Some("oauth-refresh") + ); + assert_eq!(payload["tokens"]["id_token"].as_str(), Some("oauth-id")); +} + +#[test] +fn zenmux_auth_json_stays_api_key_shaped_when_profile_is_rewritten() { + use super::super::oauth_setup::write_codex_cli_auth_file; + + with_temp_orgii_home(|_| { + let account_id = "zenmux-rewrite-shape"; + let profile_dir = app_paths::codex_cli_profile_dir(account_id); + let mut key = ModelKey::new(ModelType::ZenmuxApi); + key.api_key = Some("zenmux-test-key".to_string()); + key.enabled_models = vec!["z-ai/glm-5.2".to_string()]; + let mut env = HashMap::new(); + env.insert("OPENAI_API_KEY".to_string(), "zenmux-test-key".to_string()); + setup_codex_compatible_profile(&profile_dir, &key, None, &mut env).unwrap(); + write_codex_cli_auth_file(account_id, &key, &env).unwrap(); + + let auth = read_json(&profile_dir.join("auth.json")); + assert_eq!(auth["OPENAI_API_KEY"].as_str(), Some("zenmux-test-key")); + assert!(auth.get("tokens").is_none()); + }); +} + +#[test] +fn oauth_retry_eligibility_requires_matching_native_oauth_credential() { + use super::super::oauth_setup::is_cli_oauth_retry_eligible; + + let mut codex_oauth = ModelKey::new(ModelType::Codex); + codex_oauth.auth_method = AuthMethod::Oauth; + let codex_api_key = ModelKey::new(ModelType::Codex); + let mut claude_oauth = ModelKey::new(ModelType::ClaudeCode); + claude_oauth.auth_method = AuthMethod::Oauth; + let claude_api_key = ModelKey::new(ModelType::ClaudeCode); + let zhipu_api_key = ModelKey::new(ModelType::ZhipuApi); + let zenmux_api_key = ModelKey::new(ModelType::ZenmuxApi); + + assert!(is_cli_oauth_retry_eligible( + &ModelType::Codex, + KeySource::OwnKey, + Some(&codex_oauth) + )); + assert!(is_cli_oauth_retry_eligible( + &ModelType::ClaudeCode, + KeySource::OwnKey, + Some(&claude_oauth) + )); + + for (target, key) in [ + (&ModelType::Codex, &codex_api_key), + (&ModelType::Codex, &zhipu_api_key), + (&ModelType::Codex, &zenmux_api_key), + (&ModelType::Codex, &claude_oauth), + (&ModelType::ClaudeCode, &claude_api_key), + (&ModelType::ClaudeCode, &zhipu_api_key), + (&ModelType::ClaudeCode, &zenmux_api_key), + (&ModelType::ClaudeCode, &codex_oauth), + ] { + assert!(!is_cli_oauth_retry_eligible( + target, + KeySource::OwnKey, + Some(key) + )); + } + assert!(!is_cli_oauth_retry_eligible( + &ModelType::Codex, + KeySource::HostedKey, + Some(&codex_oauth) + )); +} + +#[test] +fn app_server_auth_error_chunk_uses_the_native_oauth_retry_gate() { + use super::super::oauth_setup::is_retryable_cli_oauth_failure_chunk; + + let mut chunk = ActivityChunk::new("session-1", "error", "error"); + chunk.result = serde_json::json!({ + "observation": "401 Unauthorized: OAuth access token expired", + "error": "401 Unauthorized: OAuth access token expired", + "success": false, + }); + + assert!(is_retryable_cli_oauth_failure_chunk(true, &chunk).is_some()); + assert!(is_retryable_cli_oauth_failure_chunk(false, &chunk).is_none()); +} + +#[test] +fn stderr_summary_collapses_timestamped_retries_but_keeps_distinct_failures() { + let lines = VecDeque::from([ + "2026-08-03T07:23:34Z ERROR Reconnecting... 1/5 (unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses, cf-ray: first)".to_string(), + "2026-08-03T07:23:39Z ERROR Reconnecting... 2/5 (unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses, cf-ray: second)".to_string(), + "2026-08-03T07:23:42Z ERROR unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses, cf-ray: final".to_string(), + "2026-08-03T07:23:43Z ERROR codex_api: 401 Unauthorized".to_string(), + ]); + + assert_eq!( + super::super::finalize::summarize_cli_stderr(&lines).as_deref(), + Some( + "unexpected status 402 Payment Required, url: https://zenmux.ai/api/v1/responses\ncodex_api: 401 Unauthorized" + ) + ); +} + +#[test] +fn stderr_summary_drops_the_notice_the_parser_already_suppressed() { + let notice = "2026-08-03T07:23:30Z WARN Model metadata for `z-ai/glm-5.2` not found. Defaulting to fallback metadata; this can degrade performance and cause issues."; + let lines = VecDeque::from([ + notice.to_string(), + "2026-08-03T07:23:43Z ERROR codex_api: 401 Unauthorized".to_string(), + ]); + + // `not found` would otherwise re-promote a notice Codex recovers from into + // the persisted failure message. + assert_eq!( + super::super::finalize::summarize_cli_stderr(&lines).as_deref(), + Some("codex_api: 401 Unauthorized") + ); + + // On its own it is not a failure reason either. The last-line fallback + // must not resurrect it — a session that only logged this notice has no + // stderr-derived failure message at all. + let only_notice = VecDeque::from([notice.to_string()]); + assert_eq!( + super::super::finalize::summarize_cli_stderr(&only_notice), + None + ); + + // A real line behind the notice is still reachable through that fallback. + let notice_then_plain = VecDeque::from([ + "2026-08-03T07:23:44Z INFO codex_core: exiting".to_string(), + notice.to_string(), + ]); + assert_eq!( + super::super::finalize::summarize_cli_stderr(¬ice_then_plain).as_deref(), + Some("2026-08-03T07:23:44Z INFO codex_core: exiting") + ); +} + +#[test] +fn stderr_summary_falls_back_to_last_nonempty_line() { + let lines = VecDeque::from([ + "Reading additional input from stdin...".to_string(), + "".to_string(), + ]); + assert_eq!( + super::super::finalize::summarize_cli_stderr(&lines).as_deref(), + Some("Reading additional input from stdin...") + ); +} + +#[test] +fn structured_cli_error_wins_over_non_diagnostic_stderr() { + let lines = VecDeque::from(["Reading additional input from stdin...".to_string()]); + + assert_eq!( + super::super::finalize::resolve_cli_failure_message( + None, + Some("unexpected status 402 Payment Required".to_string()), + &lines, + ) + .as_deref(), + Some("unexpected status 402 Payment Required") + ); +} + +#[test] +fn terminal_cli_error_is_extracted_from_error_and_failed_session_end_chunks() { + let mut error_chunk = ActivityChunk::new("session-1", "error", "error"); + error_chunk.result = serde_json::json!({ + "error": "unexpected status 402 Payment Required, cf-ray: volatile-id", + "success": false, + }); + assert_eq!( + super::terminal_cli_error_from_chunk(&error_chunk).as_deref(), + Some("unexpected status 402 Payment Required") + ); + + let mut end_chunk = ActivityChunk::new("session-1", "session_end", "session_end"); + end_chunk.result = serde_json::json!({ + "success": false, + "error_message": "provider rejected the request", + }); + assert_eq!( + super::terminal_cli_error_from_chunk(&end_chunk).as_deref(), + Some("provider rejected the request") + ); +} + +#[test] +fn failed_session_end_replaces_an_earlier_provisional_error() { + let mut terminal_error = None; + let mut error_chunk = ActivityChunk::new("session-1", "error", "error"); + error_chunk.result = serde_json::json!({ + "error": "earlier transport error", + "success": false, + }); + super::record_terminal_cli_error(&mut terminal_error, &error_chunk); + assert_eq!(terminal_error.as_deref(), Some("earlier transport error")); + + let mut end_chunk = ActivityChunk::new("session-1", "session_end", "session_end"); + end_chunk.result = serde_json::json!({ + "success": false, + "error_message": "authoritative upstream failure", + }); + super::record_terminal_cli_error(&mut terminal_error, &end_chunk); + assert_eq!( + terminal_error.as_deref(), + Some("authoritative upstream failure") + ); +} + +#[test] +fn codex_uses_native_overload_retries_without_whole_turn_replay() { + assert!(!super::is_app_overload_retry_eligible(&ModelType::Codex)); + assert!(super::is_app_overload_retry_eligible( + &ModelType::ClaudeCode + )); +} + +#[test] +fn exhausted_overload_builds_one_visible_terminal_error_chunk() { + assert!(exhausted_overload_error_chunk( + "session-1", + MAX_OVERLOAD_RETRIES - 1, + "429 Too Many Requests", + ) + .is_none()); + + let (message, chunk) = exhausted_overload_error_chunk( + "session-1", + MAX_OVERLOAD_RETRIES, + "429 Too Many Requests, request-id: volatile", + ) + .expect("exhausted overload should produce a terminal error"); + + assert_eq!(message, "429 Too Many Requests"); + assert_eq!(chunk.action_type, "error"); + assert_eq!(chunk.function, "error"); + assert_eq!( + terminal_cli_error_from_chunk(&chunk).as_deref(), + Some(message.as_str()) + ); } #[test] @@ -159,15 +618,22 @@ fn setup_opencode_atlascloud_profile_writes_config_and_auth() { } #[test] -fn cross_type_atlascloud_model_is_preserved_for_codex() { +fn atlas_model_string_is_preserved_before_the_codex_provider_gate_rejects_it() { assert_eq!( resolve_session_model( &ModelType::Codex, Some(&ModelType::AtlascloudApi), Some("zai-org/glm-5.1"), - ), + ) + .as_deref(), Some("zai-org/glm-5.1") ); + assert!( + super::super::env_setup::validate_codex_own_key_provider(&ModelKey::new( + ModelType::AtlascloudApi, + )) + .is_err() + ); assert_eq!( resolve_session_model( &ModelType::ClaudeCode, @@ -178,6 +644,23 @@ fn cross_type_atlascloud_model_is_preserved_for_codex() { ); } +#[test] +fn codex_rejects_chat_only_providers_and_zenmux_preserves_aggregator_namespace() { + for provider in [ModelType::ZhipuApi, ModelType::AtlascloudApi] { + let key = ModelKey::new(provider); + let error = super::super::env_setup::validate_codex_own_key_provider(&key) + .expect_err("Chat Completions must not be treated as Codex Responses"); + assert!(error.contains("Responses-compatible")); + } + assert_eq!( + super::super::env_setup::normalize_codex_provider_model_id( + "z-ai/glm-5.2", + &ModelType::ZenmuxApi, + ), + "z-ai/glm-5.2" + ); +} + #[test] fn cli_plan_mode_bridge_preserves_side_chat_semantics() { let bridge = cli_exec_mode_bridge(Some("plan")).expect("plan bridge"); @@ -320,3 +803,144 @@ fn overloaded_chunk_detection() { })); assert!(is_retryable_overloaded_chunk(&no_error).is_none()); } + +/// The whole point of the collector: a child can exit with its stderr still +/// sitting unread in the pipe, and every consumer of the buffer runs after +/// `wait()`. Reading without draining is how a session that failed loudly +/// reports nothing at all. +/// +/// The writer emits its 200 lines in one `printf` rather than a loop on +/// purpose: a slow writer gives the reader task a poll between every line and +/// it keeps up trivially. Dumped in a single write, the reader is still +/// working through the pipe when the child is already reaped — without the +/// drain this buffer ends around line 127, and the last line, the one a real +/// CLI puts its error on, is exactly what gets lost. +/// +/// Hence the line numbers come from Rust and not from `seq` (which is not +/// POSIX): passing them as `"$@"` keeps it a single `printf`, and the test +/// stands on that shape. A `while` loop around `printf` costs 200 writes and +/// passes whether or not `drain` exists. +#[cfg(unix)] +#[tokio::test] +async fn stderr_collector_has_the_whole_output_once_drained() { + let numbers: Vec = (1..=200).map(|n| n.to_string()).collect(); + let mut child = tokio::process::Command::new("sh") + .arg("-c") + .arg(r#"exec >&2; printf 'line %s\n' "$@""#) + // `sh -c` assigns the first operand to $0, so the numbers start at $1. + .arg("stderr-writer") + .args(&numbers) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawn stderr writer"); + + let mut collector = CliStderrCollector::new(); + collector.attach( + child.stderr.take().expect("stderr was piped"), + "test-session".to_string(), + ); + + let status = child.wait().await.expect("wait for stderr writer"); + assert!(status.success()); + + collector.drain().await; + + let lines = collector.lines(); + let buf = lines.lock().await; + // The buffer is a bounded ring, so the last line written is the assertion + // that matters: it can only be there if the reader ran to EOF. + assert_eq!(buf.len(), MAX_STDERR_LINES); + assert_eq!(buf.back().map(String::as_str), Some("line 200")); + assert_eq!(buf.front().map(String::as_str), Some("line 181")); +} + +/// A CLI that leaves its stderr with a process outliving it keeps the pipe open +/// after the child itself is reaped, so `drain` gives up on a deadline — and has +/// to abort the reader when it does. Dropping the handle would only detach the +/// task, leaving it, the pipe fd and a buffer handle alive for as long as that +/// process lives, once per attempt. +/// +/// `start_paused` makes the deadline free: with the reader parked on the pipe +/// the runtime has nothing left to run, so the clock jumps to the timeout. The +/// reader's own `Arc` clone is the proof it stopped — the count can only drop +/// back once the task's future has been dropped. +#[cfg(unix)] +#[tokio::test(start_paused = true)] +async fn a_reader_the_grandchild_holds_open_is_aborted_not_detached() { + // The backgrounded sleep inherits stderr and outlives the shell, so the + // write end is still open once the child is gone. Its own process group is + // the only handle on it afterwards — the shell's exit orphans it. + let mut child = tokio::process::Command::new("sh") + .arg("-c") + .arg("exec >&2; echo dying; sleep 300 &") + .stderr(std::process::Stdio::piped()) + .process_group(0) + .spawn() + .expect("spawn stderr writer"); + let group = child.id().expect("child pid before wait") as libc::pid_t; + + let mut collector = CliStderrCollector::new(); + collector.attach( + child.stderr.take().expect("stderr was piped"), + "test-session".to_string(), + ); + assert!(child.wait().await.expect("wait for stderr writer").success()); + + let lines = collector.lines(); + assert_eq!( + Arc::strong_count(&lines), + 3, + "the collector, the reader task and this handle" + ); + + collector.drain().await; + + assert_eq!( + Arc::strong_count(&lines), + 2, + "a reader that outlived the deadline must be aborted, not detached" + ); + + // SAFETY: signalling a process group this test created. + unsafe { libc::kill(-group, libc::SIGKILL) }; +} + +#[cfg(unix)] +#[tokio::test] +async fn draining_the_stderr_collector_twice_is_a_no_op() { + let mut child = tokio::process::Command::new("sh") + .arg("-c") + .arg("echo boom >&2") + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawn stderr writer"); + + let mut collector = CliStderrCollector::new(); + collector.attach( + child.stderr.take().expect("stderr was piped"), + "test-session".to_string(), + ); + let _ = child.wait().await; + + // Both transports drain, and the run loop drains again for the finalizer. + collector.drain().await; + collector.drain().await; + + let lines = collector.lines(); + assert_eq!( + lines.lock().await.back().map(String::as_str), + Some("boom"), + "the second drain must not discard what the first collected" + ); +} + +/// A collector that was never attached to a child (spawn failed before the +/// pipe was taken) must not make the caller wait out the drain timeout. +#[tokio::test] +async fn draining_an_unattached_stderr_collector_returns_immediately() { + let mut collector = CliStderrCollector::new(); + tokio::time::timeout(tokio::time::Duration::from_secs(1), collector.drain()) + .await + .expect("drain of an unattached collector must not block"); + assert!(collector.lines().lock().await.is_empty()); +} diff --git a/src-tauri/src/agent_sessions/cli/session_runner/session/transport_app_server.rs b/src-tauri/src/agent_sessions/cli/session_runner/session/transport_app_server.rs index 604a75d3e..f0461580e 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/session/transport_app_server.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/session/transport_app_server.rs @@ -10,12 +10,17 @@ use crate::api::websocket_handler; use super::super::super::persistence; use super::super::helpers::{emit_chunk, snapshot_cli_file_edit}; use super::super::launch_profiles::ResolvedCliLaunchProfile; +use super::super::oauth_setup::{ + is_cli_chunk_replay_unsafe, is_cli_oauth_failure_message, is_retryable_cli_oauth_failure_chunk, +}; pub(super) struct AppServerOutcome { pub(super) exit_code: i32, pub(super) timed_out: bool, pub(super) cli_session_id_out: Option, pub(super) codex_app_server_turn_ok: bool, + pub(super) retryable_oauth_message: Option, + pub(super) terminal_error_message: Option, } #[allow(clippy::too_many_arguments)] @@ -23,6 +28,7 @@ pub(super) async fn run_codex_app_server_branch( mut child: Child, session_id: String, account_id: Option<&str>, + oauth_retry_eligible: bool, effective_input: String, working_dir: &str, cli_resume_id: Option, @@ -35,6 +41,7 @@ pub(super) async fn run_codex_app_server_branch( mut cli_session_id_out: Option, sequence: &mut i64, mut codex_app_server_turn_ok: bool, + attempt_stderr: &mut super::CliStderrCollector, ) -> Result { // ── Codex app-server: long-lived JSON-RPC over stdio ── // (experimental; gate = launch-profile transport="app-server"). @@ -60,8 +67,24 @@ pub(super) async fn run_codex_app_server_branch( codex_app_server::run_app_server_turn(stdin, stdout, turn, chunk_tx).await }); + let mut retryable_oauth_message = None; + let mut replay_unsafe_output_seen = false; + let mut terminal_error_message = None; let timeout_result = tokio::time::timeout(session_timeout, async { while let Some(chunk) = chunk_rx.recv().await { + if retryable_oauth_message.is_none() && !replay_unsafe_output_seen { + retryable_oauth_message = + is_retryable_cli_oauth_failure_chunk(oauth_retry_eligible, &chunk); + } + if retryable_oauth_message.is_some() { + // Suppress the failed attempt. The outer runner will refresh + // and replay only when no assistant/tool output was emitted. + continue; + } + super::record_terminal_cli_error(&mut terminal_error_message, &chunk); + if is_cli_chunk_replay_unsafe(&chunk) { + replay_unsafe_output_seen = true; + } // Bind the rollout-compatible thread id as soon as the // session_start chunk carries it (mirrors the parser // early-binding in the exec branch below): native @@ -125,10 +148,20 @@ pub(super) async fn run_codex_app_server_branch( } } Ok(Err(err)) if !timed_out => { - tracing::error!("[CodeSession] app-server protocol error: {}", err); + if oauth_retry_eligible + && !replay_unsafe_output_seen + && is_cli_oauth_failure_message(&err) + { + retryable_oauth_message = Some(err); + } else { + tracing::error!("[CodeSession] app-server protocol error: {}", err); + terminal_error_message = + Some(super::super::super::parsers::canonicalize_cli_error_message(&err)); + } } Err(join_err) => { tracing::error!("[CodeSession] app-server task panicked: {}", join_err); + terminal_error_message = Some(format!("Codex app-server task failed: {join_err}")); } _ => {} } @@ -146,10 +179,30 @@ pub(super) async fn run_codex_app_server_branch( .map_err(|err| format!("Wait error: {}", err))?; let exit_code = status.code().unwrap_or(-1); + // The child is gone; collect the rest of its stderr before the OAuth probe + // below reads it. + attempt_stderr.drain().await; + + if retryable_oauth_message.is_none() + && oauth_retry_eligible + && !timed_out + && !codex_app_server_turn_ok + && !replay_unsafe_output_seen + { + let stderr = attempt_stderr.lines(); + let stderr = stderr.lock().await; + retryable_oauth_message = stderr + .iter() + .find(|line| is_cli_oauth_failure_message(line)) + .cloned(); + } + Ok(AppServerOutcome { exit_code, timed_out, cli_session_id_out, codex_app_server_turn_ok, + retryable_oauth_message, + terminal_error_message, }) } diff --git a/src-tauri/src/agent_sessions/cli/session_runner/session/transport_standard.rs b/src-tauri/src/agent_sessions/cli/session_runner/session/transport_standard.rs index e3b412887..6da30b38b 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/session/transport_standard.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/session/transport_standard.rs @@ -2,19 +2,15 @@ //! `CliAgentParser`, handling plan-approval gating and oauth/overload retry //! signal detection. -use std::collections::VecDeque; use std::path::{Path, PathBuf}; -use std::sync::Arc; use tokio::io::BufReader; use tokio::process::Child; -use tokio::sync::Mutex; use crate::api::websocket_handler; use key_vault::key_store::ModelType; use super::super::super::persistence; -use super::super::super::persistence::CodeSession; use super::super::super::types::SessionStatus; use super::super::command::create_parser; use super::super::helpers::{ @@ -38,13 +34,15 @@ pub(super) struct StandardOutcome { pub(super) cli_session_id_out: Option, pub(super) retryable_oauth_message: Option, pub(super) retryable_overload_message: Option, + pub(super) terminal_error_message: Option, } #[allow(clippy::too_many_arguments)] pub(super) async fn run_standard_branch( mut child: Child, session_id: String, - session: &CodeSession, + oauth_retry_eligible: bool, + overload_retry_eligible: bool, agent: ModelType, mode: Option<&str>, account_id: Option<&str>, @@ -54,10 +52,11 @@ pub(super) async fn run_standard_branch( snapshot_working_dir: String, mut cli_session_id_out: Option, sequence: &mut i64, - attempt_stderr_lines: Arc>>, + attempt_stderr: &mut super::CliStderrCollector, ) -> StandardOutcome { let mut retryable_oauth_message: Option = None; let mut retryable_overload_message: Option = None; + let mut terminal_error_message: Option = None; let mut replay_unsafe_output_seen = false; // ── Standard agents: read stdout line by line through CliAgentParser ── @@ -140,10 +139,16 @@ pub(super) async fn run_standard_branch( if cli_plan_approval_gate_triggered { continue; } + + // Retain the structured terminal body before deciding + // whether this attempt is safe to retry. Intermediate + // attempts overwrite this outcome on the next pass; + // the last exhausted attempt must not lose its body. + super::record_terminal_cli_error(&mut terminal_error_message, &chunk); + if !replay_unsafe_output_seen { if let Some(message) = is_retryable_cli_oauth_failure_chunk( - &agent, - session.key_source, + oauth_retry_eligible, &chunk, ) { retryable_oauth_message = Some(message); @@ -151,9 +156,11 @@ pub(super) async fn run_standard_branch( } } - if let Some(message) = is_retryable_overloaded_chunk(&chunk) { - retryable_overload_message = Some(message); - break; + if overload_retry_eligible { + if let Some(message) = is_retryable_overloaded_chunk(&chunk) { + retryable_overload_message = Some(message); + break; + } } if is_cli_chunk_replay_unsafe(&chunk) { @@ -385,15 +392,19 @@ pub(super) async fn run_standard_branch( .and_then(|status| status.code()) .unwrap_or(-1); + // The child is gone; collect the rest of its stderr before anything reads + // it. The OAuth probe below is the whole reason the retry exists. + attempt_stderr.drain().await; + if retryable_oauth_message.is_none() && is_cli_oauth_stderr_retry_candidate( - &agent, - session.key_source, + oauth_retry_eligible, exit_code, replay_unsafe_output_seen, ) { - let buf = attempt_stderr_lines.lock().await; + let buf = attempt_stderr.lines(); + let buf = buf.lock().await; retryable_oauth_message = buf .iter() .find(|line| is_cli_oauth_failure_message(line)) @@ -406,17 +417,20 @@ pub(super) async fn run_standard_branch( { let exit_chunks = parser.on_exit(exit_code); for chunk in &exit_chunks { + super::record_terminal_cli_error(&mut terminal_error_message, chunk); if !replay_unsafe_output_seen { if let Some(message) = - is_retryable_cli_oauth_failure_chunk(&agent, session.key_source, chunk) + is_retryable_cli_oauth_failure_chunk(oauth_retry_eligible, chunk) { retryable_oauth_message = Some(message); break; } } - if let Some(message) = is_retryable_overloaded_chunk(chunk) { - retryable_overload_message = Some(message); - break; + if overload_retry_eligible { + if let Some(message) = is_retryable_overloaded_chunk(chunk) { + retryable_overload_message = Some(message); + break; + } } if let Some(snap_id) = &pre_message_snapshot_id { snapshot_cli_file_edit(&session_id, snap_id, chunk, &snapshot_working_dir).await; @@ -462,5 +476,6 @@ pub(super) async fn run_standard_branch( cli_session_id_out, retryable_oauth_message, retryable_overload_message, + terminal_error_message, } } diff --git a/src-tauri/src/agent_sessions/event_pipeline/ingestion/normalizer.rs b/src-tauri/src/agent_sessions/event_pipeline/ingestion/normalizer.rs index 070143dc4..aa09914c8 100644 --- a/src-tauri/src/agent_sessions/event_pipeline/ingestion/normalizer.rs +++ b/src-tauri/src/agent_sessions/event_pipeline/ingestion/normalizer.rs @@ -144,6 +144,20 @@ fn infer_display_variant( function_name: &str, result: &serde_json::Value, ) -> EventDisplayVariant { + let is_failed_session_end = (action_type == "session_end" || function_name == "session_end") + && result.get("success").and_then(|value| value.as_bool()) == Some(false) + && ["error", "error_message", "observation"] + .iter() + .any(|field| { + result + .get(*field) + .and_then(|value| value.as_str()) + .is_some_and(|message| !message.trim().is_empty()) + }); + if is_failed_session_end { + return EventDisplayVariant::Error; + } + // User messages if (action_type == "raw" || action_type == "raw_event") && raw_message_text(result).is_some() { return EventDisplayVariant::Message; @@ -497,7 +511,13 @@ fn infer_display_text( { "Session completed".to_string() } else { - "Session ended".to_string() + result_obj + .and_then(|o| { + str_field(o, "error") + .or_else(|| str_field(o, "error_message")) + .or_else(|| str_field(o, "observation")) + }) + .unwrap_or_else(|| "Session ended".to_string()) } } diff --git a/src-tauri/src/agent_sessions/event_pipeline/ingestion/tests/normalizer_tests.rs b/src-tauri/src/agent_sessions/event_pipeline/ingestion/tests/normalizer_tests.rs index d6f2d1a3b..da8efb238 100644 --- a/src-tauri/src/agent_sessions/event_pipeline/ingestion/tests/normalizer_tests.rs +++ b/src-tauri/src/agent_sessions/event_pipeline/ingestion/tests/normalizer_tests.rs @@ -97,6 +97,21 @@ fn test_normalize_assistant_message() { assert_eq!(event.source, EventSource::Assistant); } +#[test] +fn failed_session_end_with_error_body_is_normalized_as_visible_error() { + let mut chunk = make_chunk("session_end", "session_end"); + chunk.result = Some(serde_json::json!({ + "success": false, + "error_message": "unexpected status 402 Payment Required", + })); + + let event = normalize_chunk(&chunk, "sess-1"); + + assert_eq!(event.display_variant, EventDisplayVariant::Error); + assert_eq!(event.display_status, EventDisplayStatus::Failed); + assert_eq!(event.display_text, "unexpected status 402 Payment Required"); +} + #[test] fn test_normalize_user_message() { let chunk = RawActivityChunk { diff --git a/src-tauri/src/infrastructure/housekeeping.rs b/src-tauri/src/infrastructure/housekeeping.rs index 401344394..9e3d8fd2b 100644 --- a/src-tauri/src/infrastructure/housekeeping.rs +++ b/src-tauri/src/infrastructure/housekeeping.rs @@ -14,7 +14,7 @@ //! - Plan-mode plan file TTL prune (30 days, mtime-based, recursive) //! - Merkle snapshot TTL prune (30 days, mtime-based — stale snapshots //! auto-rebuild on next access) -//! - Orphan `cursor-config//`, hosted Claude Code profile, +//! - Orphan `cursor-config//`, hosted Claude Code/Codex profiles, //! and Kiro proxy home eviction (session no longer present //! in `agent_sessions` DB) //! - Orphan `agent-worktrees///` eviction (session @@ -97,6 +97,10 @@ pub struct HousekeepingStats { /// `~/.orgii/claude-code-cli-profiles/` because their owning CLI /// session was gone. Account-scoped BYOK profiles are retained. pub claude_code_session_profiles_evicted: usize, + /// Hosted per-session Codex profile dirs removed from + /// `~/.orgii/codex-hosted-cli-profiles/` because their owning CLI + /// session was gone. + pub codex_hosted_session_profiles_evicted: usize, /// Hosted Kiro proxy HOME dirs removed from `/tmp/orgii-{uid}/kiro-proxy/`. pub kiro_proxy_homes_evicted: usize, /// Screenshot files removed from `~/.orgii/screenshots/` via TTL sweep. @@ -175,6 +179,13 @@ pub fn run_deferred_cleanup() -> HousekeepingStats { err ), } + match evict_orphan_session_dirs(paths::codex_hosted_cli_profile_root(), &known) { + Ok(n) => stats.codex_hosted_session_profiles_evicted = n, + Err(err) => tracing::warn!( + "[housekeeping] codex hosted profile orphan sweep failed: {}", + err + ), + } match evict_orphan_kiro_proxy_homes(&known) { Ok(n) => stats.kiro_proxy_homes_evicted = n, Err(err) => tracing::warn!( @@ -253,7 +264,7 @@ pub fn run_deferred_cleanup() -> HousekeepingStats { } tracing::info!( - "[housekeeping] pass finished: file_history(sessions={}, rows={}), capped(sessions={}, manifests={}, blobs={}), logs_removed={}, cursor_configs_evicted={}, claude_code_session_profiles_evicted={}, kiro_proxy_homes_evicted={}, agent_worktrees_evicted={}, scratchpads_evicted={}, screenshots_removed={}, tool_results_removed={}, plans_removed={}, merkle_snapshots_removed={}, session_images_evicted={}, gateway_bindings_evicted={}, session_cache_rows_evicted={}", + "[housekeeping] pass finished: file_history(sessions={}, rows={}), capped(sessions={}, manifests={}, blobs={}), logs_removed={}, cursor_configs_evicted={}, claude_code_session_profiles_evicted={}, codex_hosted_session_profiles_evicted={}, kiro_proxy_homes_evicted={}, agent_worktrees_evicted={}, scratchpads_evicted={}, screenshots_removed={}, tool_results_removed={}, plans_removed={}, merkle_snapshots_removed={}, session_images_evicted={}, gateway_bindings_evicted={}, session_cache_rows_evicted={}", stats.file_history.sessions_removed, stats.file_history.db_rows_removed, stats.sessions_capped, @@ -262,6 +273,7 @@ pub fn run_deferred_cleanup() -> HousekeepingStats { stats.log_files_removed, stats.cursor_configs_evicted, stats.claude_code_session_profiles_evicted, + stats.codex_hosted_session_profiles_evicted, stats.kiro_proxy_homes_evicted, stats.agent_worktrees_evicted, stats.scratchpads_evicted, @@ -473,6 +485,22 @@ mod tests { }); } + #[test] + fn evict_orphan_hosted_codex_profiles_keeps_live_session() { + with_sandbox(|_| { + let root = paths::codex_hosted_cli_profile_root(); + std::fs::create_dir_all(root.join("cliagent-live")).unwrap(); + std::fs::create_dir_all(root.join("cliagent-dead")).unwrap(); + + let known = std::collections::HashSet::from(["cliagent-live".to_string()]); + let removed = evict_orphan_session_dirs(root.clone(), &known).unwrap(); + + assert_eq!(removed, 1); + assert!(root.join("cliagent-live").exists()); + assert!(!root.join("cliagent-dead").exists()); + }); + } + #[test] fn evict_orphan_kiro_proxy_homes_prunes_unknown_sessions() { with_sandbox(|root| { diff --git a/src-tauri/src/infrastructure/storage_commands.rs b/src-tauri/src/infrastructure/storage_commands.rs index b40458f54..85d09e268 100644 --- a/src-tauri/src/infrastructure/storage_commands.rs +++ b/src-tauri/src/infrastructure/storage_commands.rs @@ -12,8 +12,8 @@ use std::path::{Path, PathBuf}; use app_paths::{ agent_worktrees_root, claude_code_cli_profile_root, codex_cli_profile_root, - cursor_cli_profile_root, cursor_config_root, diagnostics_dir, extensions_dir, - file_history_root, kiro_cli_profile_root, logs_dir, lsp_bin_dir, models_dir, + codex_hosted_cli_profile_root, cursor_cli_profile_root, cursor_config_root, diagnostics_dir, + extensions_dir, file_history_root, kiro_cli_profile_root, logs_dir, lsp_bin_dir, models_dir, opencode_cli_profile_root, orgii_root, personal_workspace, screenshots_dir, semantic_index_dir, session_images_dir, sessions_db, sidecar_bin_dir, tool_results_root, }; @@ -108,6 +108,11 @@ pub fn get_disk_usage() -> DiskUsageReport { "Codex CLI Profiles", codex_cli_profile_root(), ), + ( + "codexHostedCliProfiles", + "Hosted Codex Session Profiles", + codex_hosted_cli_profile_root(), + ), ( "kiroCliProfiles", "Kiro CLI Profiles", @@ -177,6 +182,7 @@ fn category_path(key: &str) -> Option { "cursorCliProfiles" => Some(cursor_cli_profile_root()), "claudeCodeCliProfiles" => Some(claude_code_cli_profile_root()), "codexCliProfiles" => Some(codex_cli_profile_root()), + "codexHostedCliProfiles" => Some(codex_hosted_cli_profile_root()), "kiroCliProfiles" => Some(kiro_cli_profile_root()), "opencodeCliProfiles" => Some(opencode_cli_profile_root()), "extensions" => Some(extensions_dir()), diff --git a/src-tauri/src/orgtrack/session_provenance.rs b/src-tauri/src/orgtrack/session_provenance.rs index d8238247c..b9a1df427 100644 --- a/src-tauri/src/orgtrack/session_provenance.rs +++ b/src-tauri/src/orgtrack/session_provenance.rs @@ -493,6 +493,8 @@ mod tests { fn codex_lifecycle_maps_actor_to_independently_loadable_transcript() { let conn = Connection::open_in_memory().expect("in-memory SQLite"); SqliteRecordStore::init_tables(&conn).expect("initialize orgtrack schema"); + SqliteRecordStore::init_source_cache_tables(&conn) + .expect("initialize imported-history schema"); let store = SqliteRecordStore::new(&conn); let temp = tempfile::tempdir().expect("Codex session root"); let sessions_dir = temp diff --git a/src/engines/ChatPanel/ChatHistory/ActivityRouter.test.ts b/src/engines/ChatPanel/ChatHistory/ActivityRouter.test.ts index c438a7390..c6a9387f2 100644 --- a/src/engines/ChatPanel/ChatHistory/ActivityRouter.test.ts +++ b/src/engines/ChatPanel/ChatHistory/ActivityRouter.test.ts @@ -1,11 +1,16 @@ import { createElement } from "react"; import { renderToStaticMarkup } from "react-dom/server"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { makeSessionEvent } from "@src/engines/SessionCore/rendering/props/__tests__/fixtures"; import ActivityChatItem from "./ActivityRouter"; +vi.mock("../ChatItems/AgentErrorChatItem", () => ({ + default: ({ errorMessage }: { errorMessage: string }) => + `routed-agent-error:${errorMessage}`, +})); + describe("ActivityChatItem initial loading placeholder", () => { it("renders the shared block instead of synthetic loading text", () => { const event = makeSessionEvent({ @@ -23,3 +28,49 @@ describe("ActivityChatItem initial loading placeholder", () => { expect(markup).not.toContain("Loading..."); }); }); + +describe("ActivityChatItem error routing", () => { + it("renders a standard CLI error chunk with its error body", () => { + const event = makeSessionEvent({ + action_type: "error", + function: "error", + result: { + error: "unexpected status 402 Payment Required", + success: false, + }, + displayText: "unexpected status 402 Payment Required", + displayStatus: "failed", + displayVariant: "error", + }); + + const markup = renderToStaticMarkup( + createElement(ActivityChatItem, { event }) + ); + + expect(markup).toContain( + "routed-agent-error:unexpected status 402 Payment Required" + ); + }); + + it("renders a failed session_end with its terminal error body", () => { + const event = makeSessionEvent({ + action_type: "session_end", + function: "session_end", + result: { + error_message: "provider rejected the request", + success: false, + }, + displayText: "provider rejected the request", + displayStatus: "failed", + displayVariant: "error", + }); + + const markup = renderToStaticMarkup( + createElement(ActivityChatItem, { event }) + ); + + expect(markup).toContain( + "routed-agent-error:provider rejected the request" + ); + }); +}); diff --git a/src/engines/ChatPanel/ChatHistory/ActivityRouter.tsx b/src/engines/ChatPanel/ChatHistory/ActivityRouter.tsx index 21fd7de25..00791f937 100644 --- a/src/engines/ChatPanel/ChatHistory/ActivityRouter.tsx +++ b/src/engines/ChatPanel/ChatHistory/ActivityRouter.tsx @@ -33,7 +33,7 @@ import { import AgentChatItemDefault from "../ChatItems/AgentChatItemDefault"; import AgentErrorChatItem from "../ChatItems/AgentErrorChatItem"; import "./ActivityRouter.scss"; -import { isAgentErrorEvent } from "./chatItemPipeline/classifiers"; +import { getAgentErrorMessage } from "./chatItemPipeline/classifiers"; import UserMessageContent from "./components/UserMessageContent"; const log = createLogger("ActivityRouter"); @@ -59,6 +59,7 @@ const RESULT_COMPARE_KEYS = [ "observation", "success", "error", + "error_message", "images", "call_id", "output", @@ -270,10 +271,9 @@ const ActivityChatItem: React.FC = memo( const functionName = event.functionName; const eventType = getRegistryEventType(event); - if (isAgentErrorEvent(event) && event.result?.observation) { - return ( - - ); + const agentErrorMessage = getAgentErrorMessage(event); + if (agentErrorMessage) { + return ; } if ( diff --git a/src/engines/ChatPanel/ChatHistory/chatItemPipeline/classifiers.ts b/src/engines/ChatPanel/ChatHistory/chatItemPipeline/classifiers.ts index 060ce894f..d0d051b75 100644 --- a/src/engines/ChatPanel/ChatHistory/chatItemPipeline/classifiers.ts +++ b/src/engines/ChatPanel/ChatHistory/chatItemPipeline/classifiers.ts @@ -229,6 +229,8 @@ export const isManageTodoEvent = (event: SessionEvent): boolean => { * shape stamped by both producers: * - Rust `lifecycle::build_session_error_event` (id `session-error-…`) * - FE `makeErrorEvent` in sync/adapters/shared/eventFactories.ts + * - normalized CLI/native-transcript chunks (`actionType` / `functionName` + * / `displayVariant` = `error`) * * Mirrors Claude Code's `isApiErrorMessage` contract: every render, * filter, and collapse path must treat these as always-visible — a @@ -238,8 +240,26 @@ export const isManageTodoEvent = (event: SessionEvent): boolean => { */ export const isAgentErrorEvent = (event: SessionEvent): boolean => { return ( - event.functionName === "system" && - event.displayStatus === "failed" && - event.displayVariant === "message" + event.displayVariant === "error" || + event.actionType === "error" || + event.functionName === "error" || + (event.functionName === "system" && + event.displayStatus === "failed" && + event.displayVariant === "message") ); }; + +export const getAgentErrorMessage = (event: SessionEvent): string | null => { + if (!isAgentErrorEvent(event)) return null; + + for (const value of [ + event.result?.error, + event.result?.error_message, + event.result?.observation, + event.displayText, + ]) { + if (typeof value === "string" && value.trim()) return value; + } + + return null; +}; diff --git a/src/engines/ChatPanel/ChatHistory/hooks/__tests__/useChatGroups.test.ts b/src/engines/ChatPanel/ChatHistory/hooks/__tests__/useChatGroups.test.ts index 3e2fa79fa..42391b067 100644 --- a/src/engines/ChatPanel/ChatHistory/hooks/__tests__/useChatGroups.test.ts +++ b/src/engines/ChatPanel/ChatHistory/hooks/__tests__/useChatGroups.test.ts @@ -116,6 +116,20 @@ function errorItem(message: string): OptimizedChatItem { ); } +/** Shape emitted by normalized Codex CLI and native-transcript error chunks. */ +function cliErrorItem(message: string): OptimizedChatItem { + return item( + makeEvent({ + functionName: "error", + actionType: "error", + displayText: message, + displayStatus: "failed", + displayVariant: "error", + result: { error: message, success: false }, + }) + ); +} + /** Shape stamped by persistedMessageToSessionEvent for a compact-boundary row. */ function boundaryItem(summary: string): OptimizedChatItem { return item( @@ -274,6 +288,22 @@ describe("useChatGroups collapse — terminal error survival", () => { expect(result.flatItems.some((entry) => entry.structuralOnly)).toBe(false); }); + it("keeps a normalized CLI error when its historical turn is collapsed", () => { + const history = [ + userItem("first turn"), + toolItem(), + cliErrorItem("unexpected status 402 Payment Required"), + userItem("second turn"), + assistantItem("second reply"), + ]; + + const result = useChatGroups(history, { allTurnsCollapsed: true }); + + expect(flatTexts(result.flatItems)).toContain( + "unexpected status 402 Payment Required" + ); + }); + it("keeps both the final reply and the trailing error in a collapsed turn", () => { const history = [ userItem("first turn"),