From 1b585504917cc233711f8749fd1d5fef3bc9ed6b Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 10:46:22 +0800 Subject: [PATCH 1/9] fix(key-vault): limit Codex own-key providers to Responses-capable ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex only speaks the Responses wire API — the installed codex-cli rejects `wire_api = "chat"` outright. The compatible-provider list nevertheless advertised every generic OpenAI-compatible provider, so selecting one produced a session that failed at request time instead of an upfront, actionable rejection in the picker. Narrow the list to providers whose `/responses` route is supported or has been verified end to end: `openai_api` and `zenmux_api`. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../src/commands/registry/data/cli_agents.rs | 19 +++++-------------- .../key-vault/src/commands/registry/mod.rs | 5 ++++- 2 files changed, 9 insertions(+), 15 deletions(-) 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); } From 9b518172a9f5a73fa9566ca974d21161bfc36871 Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 10:47:17 +0800 Subject: [PATCH 2/9] refactor(cli-session): isolate Codex CLI profiles per account/session Own-key and hosted Codex runs both pointed `CODEX_HOME` at shared state, so two concurrent sessions raced on one `config.toml` and the last writer decided everyone's routing. Give hosted sessions their own profile root alongside the existing per-account one, and generate the hosted profile from a single writer instead of merging into the user's global config. Housekeeping, storage reporting and native-transcript indexing all learn about the new root so hosted rollouts are still swept, sized and imported. Also expose `write_cli_profile_file_atomic` so callers that own credential files can replace them crash-safely without inheriting the warn-on-chmod-failure behaviour of the sensitive-file helper. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../src/managed_config/generators.rs | 56 +++++++++++++++++++ .../agent-cli/src/managed_config/mod.rs | 30 ++++++++++ .../agent-cli/src/managed_config/tests.rs | 39 +++++++++++++ src-tauri/crates/app-paths/src/lib.rs | 10 ++++ .../src/key_store/agent_env_builder.rs | 5 +- .../src/sources/codex/app/index.rs | 24 ++++++-- .../src/sources/codex/app/mod.rs | 2 +- .../cli/persistence/session_crud.rs | 11 ++++ src-tauri/src/infrastructure/housekeeping.rs | 32 ++++++++++- .../src/infrastructure/storage_commands.rs | 10 +++- src-tauri/src/orgtrack/session_provenance.rs | 2 + 11 files changed, 209 insertions(+), 12 deletions(-) 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..3f6635467 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,36 @@ 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 a sibling temp file, fsync, +/// then rename over the target. Callers holding credentials must still apply +/// [`app_paths::set_sensitive_file_permissions`] and 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..43b1783b4 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,30 @@ 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) + ); +} 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/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/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/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/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 From d9e89b043f5da8c7b49e91f71946a7b6893c0b03 Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 10:47:43 +0800 Subject: [PATCH 3/9] fix(cli-session): render Codex errors instead of dropping them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex app-server task failures carried their message in the task payload only, so a failed turn was replayed as a plain completion: no error chunk, `task_completed` lifecycle, and nothing for the UI to show. Emit the error chunk, mark the turn failed, and keep the same shape in the native transcript importer. The reverse problem existed too. `Model metadata for ... Defaulting to fallback metadata` is a non-fatal notice — Codex falls back and keeps running — but only the structured parser dropped it. Hoist the rule into one shared predicate so the stderr fallback cannot re-promote it into the persisted failure message of an unrelated error. On the frontend, error recognition no longer requires the `system`/`failed`/`message` triple: normalized CLI chunks whose action, function or display variant is `error` now route to the error card, and the message is taken from `error` / `error_message` / `observation` / display text rather than `observation` alone. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../src/sources/codex/app/transcript.rs | 40 +++++++- .../src/sources/codex/app_tests.rs | 83 ++++++++++++++++ .../src/agent_sessions/cli/parsers/codex.rs | 62 +++++++++--- .../cli/parsers/codex_app_server.rs | 28 ++++-- .../src/agent_sessions/cli/parsers/mod.rs | 99 +++++++++++++++++++ .../parsers/tests/codex_app_server_tests.rs | 30 +++++- .../parsers/tests/parser_integration_tests.rs | 40 ++++++-- .../event_pipeline/ingestion/normalizer.rs | 22 ++++- .../ingestion/tests/normalizer_tests.rs | 15 +++ .../ChatHistory/ActivityRouter.test.ts | 53 +++++++++- .../ChatPanel/ChatHistory/ActivityRouter.tsx | 10 +- .../chatItemPipeline/classifiers.ts | 26 ++++- .../hooks/__tests__/useChatGroups.test.ts | 30 ++++++ 13 files changed, 496 insertions(+), 42 deletions(-) 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..66751f790 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,11 @@ 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, } impl CodexParser { @@ -25,6 +32,8 @@ impl CodexParser { thread_id: None, usage: None, got_turn_completed: false, + error_deduper: BoundedCliErrorDeduper::default(), + pending_error_message: None, } } @@ -139,6 +148,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 +259,7 @@ impl CodexParser { "turn.completed" => { self.got_turn_completed = true; + self.pending_error_message = None; // Extract usage if let Some(usage) = data.get("usage") { @@ -277,22 +298,31 @@ 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) { + 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()) + .unwrap_or_else(|| "Turn failed".to_string()); + self.pending_error_message = None; let mut chunk = ActivityChunk::new(&self.session_id, "session_end", "session_end"); chunk.result = serde_json::json!({ "success": false, @@ -370,10 +400,18 @@ 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, - }); + if let Some(message) = self.pending_error_message.take() { + 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..a9ad4cf21 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,9 @@ 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, } impl CodexAppServerEventParser { @@ -227,6 +231,8 @@ impl CodexAppServerEventParser { turn_status: None, turn_error: None, session_start_emitted: false, + error_deduper: super::BoundedCliErrorDeduper::default(), + pending_error_message: None, } } @@ -382,13 +388,20 @@ 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"; + let error_message = if success { + self.pending_error_message = None; + None + } else { + terminal_error_message.or_else(|| self.pending_error_message.take()) + }; let mut chunk = ActivityChunk::new(&self.session_id, "session_end", "session_end"); let mut result = serde_json::json!({ "success": success, @@ -416,11 +429,12 @@ impl CodexAppServerEventParser { tracing::warn!("[CodexAppServer] Retryable error: {}", 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..7e1fe0a3f 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/mod.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/mod.rs @@ -47,9 +47,108 @@ 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() +} + +#[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); + } +} + /// Trait for parsing a CLI agent's stdout line by line. pub trait CliAgentParser: Send { /// Parse a single line from the CLI's stdout. 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..7b15dea31 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,23 @@ 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] 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..c8d6e6cf5 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,6 +78,7 @@ 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); 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/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"), From a1965c160135bfafcf26903f53de4fbcfd5b904b Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 10:49:19 +0800 Subject: [PATCH 4/9] fix(key-vault): gate OAuth refresh and CLI retry on native accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refresh_codex_oauth_key` / `refresh_claude_code_oauth_key` returned a `ModelKey` no matter what they were handed, so a cross-provider or API-key credential came back looking like a successful token rotation. Return an explicit `OAuthRefreshOutcome` (`Refreshed` / `AlreadyRotated` / `NotApplicable`) and make every caller — provider clients, quota validation, OAuth health bookkeeping, the CLI retry path — decide what to do with a non-OAuth key instead of guessing. OAuth health now errors on a key that is not a refreshable native OAuth account rather than recording failures against it. CLI OAuth retry eligibility keys off the selected credential rather than the agent alone, `auth.json` is built from that credential (API key vs token bundle) instead of scraped from the environment, and it is written atomically: a crash mid-write must not leave a truncated file that silently downgrades the next launch to unauthenticated. Direct `openai_api` keys now keep Codex's built-in `openai` provider — official endpoint, Responses, native OpenAI auth, WebSockets and Codex's own retry defaults — instead of being routed through the synthetic `orgii_compatible` table, which downgrades all four for no benefit. A custom `base_url` still gets the table. Any ORGII-written `config.toml` left by an earlier run is cleared first, so clearing a base URL override actually takes effect; profiles Codex wrote itself are left alone. Terminal transport errors are captured as they happen and take precedence over the stderr summary, so a failed session reports the CLI's own error rather than whichever line of launch noise matched a keyword. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../core/providers/anthropic_native/client.rs | 9 +- .../src/core/providers/codex_native/client.rs | 9 +- .../crates/key-vault/src/commands/validate.rs | 7 +- .../src/key_store/service/claude_oauth.rs | 46 +- .../src/key_store/service/codex_oauth.rs | 37 +- .../src/key_store/service/oauth_health.rs | 14 +- .../key-vault/src/key_store/tests/tests.rs | 117 +++- .../crates/key-vault/src/key_store/types.rs | 38 ++ .../cli/session_runner/env_setup.rs | 351 ++++++------ .../cli/session_runner/finalize.rs | 140 +++-- .../cli/session_runner/oauth_setup.rs | 175 ++++-- .../cli/session_runner/session.rs | 141 ++++- .../cli/session_runner/session/tests.rs | 508 +++++++++++++++++- .../session/transport_app_server.rs | 54 +- .../session/transport_standard.rs | 39 +- 15 files changed, 1330 insertions(+), 355 deletions(-) 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/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/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/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..ae5571f63 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,92 @@ 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() { + stderr_lines + .iter() + .rev() + .map(|line| line.trim()) + .find(|line| !line.is_empty()) + .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 +125,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 +140,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 +220,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..505f80359 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,93 @@ 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; + +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 +180,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 +307,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 +463,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 ── @@ -393,10 +475,9 @@ pub async fn run_session( 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; @@ -544,10 +625,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 +638,15 @@ pub async fn run_session( cli_session_id_out, &mut sequence, codex_app_server_turn_ok, + Arc::clone(&attempt_stderr_lines), ) .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,11 +674,12 @@ 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(), @@ -607,6 +692,7 @@ pub async fn run_session( 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; } @@ -621,28 +707,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 +738,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 +776,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 +789,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..fa49288f8 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,470 @@ 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() + ); + } +} + +#[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")); +} - let auth = read_json(&temp_dir.path().join("auth.json")); - assert_eq!(auth["OPENAI_API_KEY"].as_str(), Some("atlas-test-key")); +#[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 — fall through to the + // last-line fallback rather than reporting it as the error. + let only_notice = VecDeque::from([notice.to_string()]); + assert_eq!( + super::super::finalize::summarize_cli_stderr(&only_notice).as_deref(), + Some(notice) + ); +} + +#[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 +607,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 +633,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"); 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..c19248943 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 @@ -3,19 +3,28 @@ //! Experimental; gated by the launch-profile transport="app-server" setting //! (see `super::super::launch_profiles::uses_codex_app_server`). +use std::collections::VecDeque; +use std::sync::Arc; + use tokio::process::Child; +use tokio::sync::Mutex; 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 +32,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 +45,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_lines: Arc>>, ) -> Result { // ── Codex app-server: long-lived JSON-RPC over stdio ── // (experimental; gate = launch-profile transport="app-server"). @@ -60,8 +71,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 +152,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 +183,25 @@ pub(super) async fn run_codex_app_server_branch( .map_err(|err| format!("Wait error: {}", err))?; let exit_code = status.code().unwrap_or(-1); + 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.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..c77567ace 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 @@ -14,7 +14,6 @@ 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 +37,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>, @@ -58,6 +59,7 @@ pub(super) async fn run_standard_branch( ) -> 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 +142,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 +159,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) { @@ -387,8 +397,7 @@ pub(super) async fn run_standard_branch( 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, ) @@ -406,17 +415,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 +474,6 @@ pub(super) async fn run_standard_branch( cli_session_id_out, retryable_oauth_message, retryable_overload_message, + terminal_error_message, } } From 145dffdc5f3a9359ecef25999d07a5e9ca7ec0d1 Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 11:22:24 +0800 Subject: [PATCH 5/9] style(cli-session): move deduper tests below the parser trait `clippy::items_after_test_module` fires on the inline test module added in the previous commit: everything declared after a `#[cfg(test)] mod` is easy to miss. Move the module to the end of the file; no logic change. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../src/agent_sessions/cli/parsers/mod.rs | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src-tauri/src/agent_sessions/cli/parsers/mod.rs b/src-tauri/src/agent_sessions/cli/parsers/mod.rs index 7e1fe0a3f..2c3aaa434 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/mod.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/mod.rs @@ -130,25 +130,6 @@ pub(crate) fn canonicalize_cli_error_message(message: &str) -> String { normalized.trim().to_string() } -#[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); - } -} - /// Trait for parsing a CLI agent's stdout line by line. pub trait CliAgentParser: Send { /// Parse a single line from the CLI's stdout. @@ -168,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); + } +} From cd41a7e18e220006cac0bf8923e491bfbfba8e59 Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 13:02:31 +0800 Subject: [PATCH 6/9] fix(cli-session): drop suppressed notices from the stderr fallback too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `summarize_cli_stderr` filtered the fallback-metadata notice out of the keyword pass but not out of the last-line fallback, so a session whose stderr held only that notice still reported it as the failure reason — the exact behaviour the filter exists to prevent, surviving only because some other line usually matches first. The test asserted that outcome, which pinned the bug rather than the rule. Apply the same suppression in the fallback: a real line behind the notice is still reachable, and a session that logged nothing else now has no stderr-derived message at all, which beats blaming a notice Codex recovered from. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../cli/session_runner/finalize.rs | 6 +++++- .../cli/session_runner/session/tests.rs | 19 +++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) 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 ae5571f63..a4fcb3542 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/finalize.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/finalize.rs @@ -97,11 +97,15 @@ pub(super) fn summarize_cli_stderr(stderr_lines: &VecDeque) -> Option Date: Tue, 4 Aug 2026 13:03:51 +0800 Subject: [PATCH 7/9] fix(agent-cli): create credential staging files owner-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The atomic writer created its temp file with `File::create`, so with the common 0002 umask the payload sat at 0664 for the whole write and the destination stayed 0664 until the caller's `chmod` landed after the rename. For `auth.json` that payload is an access/refresh token pair, and the profile directories are 0775, so other local accounts could read it in that window — or indefinitely, from a staging file left behind by a crash, since cleanup only runs on the error path. This was narrowly a regression: the previous non-atomic write reused the existing 0600 file, so only first creation was exposed; routing every refresh through a fresh temp file reopened it on every write. Set the mode at creation for credential writes instead of correcting it afterwards. `set_sensitive_file_permissions` stays where it is — it still pins the destination and covers Windows ACLs — but it is no longer what stands between a token and a group-readable file. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../agent-cli/src/managed_config/file_io.rs | 29 +++++++++++++++- .../agent-cli/src/managed_config/mod.rs | 8 +++-- .../agent-cli/src/managed_config/tests.rs | 34 +++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) 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/mod.rs b/src-tauri/crates/agent-cli/src/managed_config/mod.rs index 3f6635467..5fe1dc10d 100644 --- a/src-tauri/crates/agent-cli/src/managed_config/mod.rs +++ b/src-tauri/crates/agent-cli/src/managed_config/mod.rs @@ -56,9 +56,11 @@ pub fn write_codex_hosted_profile( file_io::write_sensitive_file_atomic(&config_path, content.as_bytes()) } -/// Crash-safe replace of a CLI profile file: write a sibling temp file, fsync, -/// then rename over the target. Callers holding credentials must still apply -/// [`app_paths::set_sensitive_file_permissions`] and decide for themselves +/// 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) 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 43b1783b4..b362bea86 100644 --- a/src-tauri/crates/agent-cli/src/managed_config/tests.rs +++ b/src-tauri/crates/agent-cli/src/managed_config/tests.rs @@ -902,3 +902,37 @@ fn hosted_codex_profile_is_owned_valid_toml_and_uses_bounded_internal_retries() 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:?}" + ); +} From c876942fba029d0fd8eca037478f78a8bef47ca2 Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 13:19:57 +0800 Subject: [PATCH 8/9] fix(cli-session): keep the last retry notice as a failure fallback A `Reconnecting...` / `willRetry` notice is dropped on sight so a recovered retry never renders as an error. But when the terminal event reports failure with no error body, that notice was the only description of what went wrong, and the turn ended as a bare "Turn failed". Keep it as a last-resort body: used only after the terminal error and the pending fatal error, cleared on every success, gated on a non-zero exit in `on_exit`, and never lent to an interrupted turn. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../src/agent_sessions/cli/parsers/codex.rs | 18 ++++- .../cli/parsers/codex_app_server.rs | 15 +++- .../parsers/tests/codex_app_server_tests.rs | 69 +++++++++++++++++++ .../parsers/tests/parser_integration_tests.rs | 40 +++++++++++ 4 files changed, 140 insertions(+), 2 deletions(-) diff --git a/src-tauri/src/agent_sessions/cli/parsers/codex.rs b/src-tauri/src/agent_sessions/cli/parsers/codex.rs index 66751f790..8df054c1f 100644 --- a/src-tauri/src/agent_sessions/cli/parsers/codex.rs +++ b/src-tauri/src/agent_sessions/cli/parsers/codex.rs @@ -23,6 +23,11 @@ pub struct CodexParser { /// 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 { @@ -34,6 +39,7 @@ impl CodexParser { got_turn_completed: false, error_deduper: BoundedCliErrorDeduper::default(), pending_error_message: None, + last_retry_notice: None, } } @@ -260,6 +266,7 @@ 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") { @@ -299,6 +306,7 @@ impl CodexParser { .and_then(|v| v.as_str()) .unwrap_or("Unknown error"); 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); @@ -321,8 +329,10 @@ impl CodexParser { .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, @@ -400,7 +410,13 @@ impl CliAgentParser for CodexParser { return vec![]; } let mut chunk = ActivityChunk::new(&self.session_id, "session_end", "session_end"); - if let Some(message) = self.pending_error_message.take() { + // 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, 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 a9ad4cf21..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 @@ -219,6 +219,11 @@ pub(crate) struct CodexAppServerEventParser { 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 { @@ -233,6 +238,7 @@ impl CodexAppServerEventParser { session_start_emitted: false, error_deduper: super::BoundedCliErrorDeduper::default(), pending_error_message: None, + last_retry_notice: None, } } @@ -396,11 +402,17 @@ impl CodexAppServerEventParser { .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()) + 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!({ @@ -427,6 +439,7 @@ 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 message = canonicalize_cli_error_message(message); 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 7b15dea31..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 @@ -412,6 +412,75 @@ fn fatal_error_is_coalesced_into_authoritative_failed_turn() { 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] fn unknown_notifications_are_ignored() { let mut p = parser(); 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 c8d6e6cf5..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 @@ -85,6 +85,46 @@ mod tests { 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"); From 553894cbd15bac2f108372e6710c1263efd19c68 Mon Sep 17 00:00:00 2001 From: raymond <13162938362@163.com> Date: Tue, 4 Aug 2026 13:58:18 +0800 Subject: [PATCH 9/9] fix(cli-session): drain the child's stderr before reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stderr reader was a detached task and every consumer — the OAuth retry probe in both transports, and the finalizer's error summary — read the buffer straight after `child.wait()`. A child exiting only closes the write end of the pipe; it says nothing about whether the reader has picked up what is still in it. A CLI that dumps its diagnostics in one write loses the tail, which is where the error line is: a session that failed loudly on stderr reports no reason at all. Wrap the buffer and its JoinHandle in `CliStderrCollector` and drain before reading. The drain is idempotent, so both transports and the run loop can each ask for it, and bounded at 3s so a grandchild holding the pipe open cannot hang the turn. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- src-tauri/Cargo.toml | 6 + .../cli/session_runner/session.rs | 111 +++++++++++--- .../cli/session_runner/session/tests.rs | 141 ++++++++++++++++++ .../session/transport_app_server.rs | 13 +- .../session/transport_standard.rs | 12 +- 5 files changed, 251 insertions(+), 32 deletions(-) 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/src/agent_sessions/cli/session_runner/session.rs b/src-tauri/src/agent_sessions/cli/session_runner/session.rs index 505f80359..c3e6a2216 100644 --- a/src-tauri/src/agent_sessions/cli/session_runner/session.rs +++ b/src-tauri/src/agent_sessions/cli/session_runner/session.rs @@ -42,6 +42,84 @@ use spawn_retry::{is_transient_spawn_error, SPAWN_RETRY_ATTEMPTS, SPAWN_RETRY_BA 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" @@ -471,7 +549,6 @@ 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; @@ -542,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) @@ -601,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; @@ -638,7 +703,7 @@ pub async fn run_session( cli_session_id_out, &mut sequence, codex_app_server_turn_ok, - Arc::clone(&attempt_stderr_lines), + &mut attempt_stderr, ) .await?; exit_code = outcome.exit_code; @@ -685,7 +750,7 @@ pub async fn run_session( snapshot_working_dir.clone(), cli_session_id_out, &mut sequence, - Arc::clone(&attempt_stderr_lines), + &mut attempt_stderr, ) .await; exit_code = outcome.exit_code; @@ -697,6 +762,10 @@ pub async fn run_session( 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", 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 ea11f748b..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 @@ -803,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 c19248943..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 @@ -3,11 +3,7 @@ //! Experimental; gated by the launch-profile transport="app-server" setting //! (see `super::super::launch_profiles::uses_codex_app_server`). -use std::collections::VecDeque; -use std::sync::Arc; - use tokio::process::Child; -use tokio::sync::Mutex; use crate::api::websocket_handler; @@ -45,7 +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_lines: Arc>>, + attempt_stderr: &mut super::CliStderrCollector, ) -> Result { // ── Codex app-server: long-lived JSON-RPC over stdio ── // (experimental; gate = launch-profile transport="app-server"). @@ -183,13 +179,18 @@ 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.lock().await; + let stderr = attempt_stderr.lines(); + let stderr = stderr.lock().await; retryable_oauth_message = stderr .iter() .find(|line| is_cli_oauth_failure_message(line)) 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 c77567ace..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,13 +2,10 @@ //! `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; @@ -55,7 +52,7 @@ 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; @@ -395,6 +392,10 @@ 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( oauth_retry_eligible, @@ -402,7 +403,8 @@ pub(super) async fn run_standard_branch( 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))