diff --git a/crates/jcode-app-core/src/agent.rs b/crates/jcode-app-core/src/agent.rs index 4a6d539a01..943b774fb7 100644 --- a/crates/jcode-app-core/src/agent.rs +++ b/crates/jcode-app-core/src/agent.rs @@ -19,7 +19,7 @@ mod streaming; #[cfg(test)] mod swarm_reminder_tests; mod tool_markup_recovery; -mod tools; +pub(crate) mod tools; mod turn_execution; mod turn_loops; mod turn_streaming_mpsc; diff --git a/crates/jcode-app-core/src/agent/prompting.rs b/crates/jcode-app-core/src/agent/prompting.rs index cfce180b9e..bd6f7f81aa 100644 --- a/crates/jcode-app-core/src/agent/prompting.rs +++ b/crates/jcode-app-core/src/agent/prompting.rs @@ -156,14 +156,20 @@ impl Agent { .as_ref() .map(std::path::PathBuf::from); - let (split, _context_info) = crate::prompt::build_system_prompt_split_with_agents_md( - skill_prompt.as_deref(), - available_skills, - self.session.is_canary, - memory_prompt, - working_dir.as_deref(), - self.agents_md_snapshot.clone(), - ); + let (split, _context_info) = + crate::prompt::build_system_prompt_split_with_capabilities_and_agents_md( + skill_prompt.as_deref(), + available_skills, + self.session.is_canary, + memory_prompt, + working_dir.as_deref(), + { + let mut capabilities = crate::prompt::PromptCapabilities::current(); + capabilities.text_only_model = !self.provider.supports_image_input(); + capabilities + }, + self.agents_md_snapshot.clone(), + ); split } diff --git a/crates/jcode-app-core/src/agent_tests.rs b/crates/jcode-app-core/src/agent_tests.rs index 19184f854e..f10a21d166 100644 --- a/crates/jcode-app-core/src/agent_tests.rs +++ b/crates/jcode-app-core/src/agent_tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::agent::environment::EnvSnapshotDetail; +use crate::agent::tools::tool_output_to_content_blocks; use crate::message::{Message, StreamEvent, ToolDefinition}; use crate::provider::{EventStream, Provider}; use crate::tool::Registry; @@ -667,6 +668,115 @@ fn tool_output_to_content_blocks_preserves_labeled_images() { } } +/// Text-only stub (Provider default `supports_image_input` = false) used to +/// repro the save/reload image-loss regression: tool results with images are +/// recorded through the same session-persistence path production turns use. +struct TextOnlyToolResultProvider; + +#[async_trait] +impl Provider for TextOnlyToolResultProvider { + async fn complete( + &self, + _messages: &[Message], + _tools: &[ToolDefinition], + _system: &str, + _resume_session_id: Option<&str>, + ) -> Result { + unreachable!("TextOnlyToolResultProvider does not complete requests") + } + + fn name(&self) -> &str { + "text-only-tool-result-stub" + } + + fn fork(&self) -> Arc { + Arc::new(Self) + } +} + +/// A text-only provider must not strip images from persisted history: the +/// tool-result blocks written to the session (and reloaded later for the user +/// and for history rendering) keep the real `ContentBlock::Image`. Only the +/// outgoing provider request is filtered, at the `MultiProvider` request +/// chokepoint (`image_clamp::filter_unsupported_outbound_images`), which has +/// its own tests in jcode-base. +#[tokio::test] +async fn text_only_provider_persists_tool_images_for_history() { + struct RestoreHome(Option); + impl Drop for RestoreHome { + fn drop(&mut self) { + if let Some(home) = &self.0 { + crate::env::set_var("JCODE_HOME", home); + } else { + crate::env::remove_var("JCODE_HOME"); + } + crate::config::Config::invalidate_cache(); + } + } + + let _lock = crate::storage::lock_test_env(); + let home = tempfile::tempdir().unwrap(); + let _restore = RestoreHome(std::env::var_os("JCODE_HOME")); + crate::env::set_var("JCODE_HOME", home.path()); + crate::config::Config::invalidate_cache(); + + // Provider trait default: supports_image_input() == false (text-only). + let provider: Arc = Arc::new(TextOnlyToolResultProvider); + let registry = Registry::new(provider.clone()).await; + let mut agent = Agent::new(provider, registry); + assert!( + !agent.provider.supports_image_input(), + "test provider must be text-only for the regression repro" + ); + + // Streaming save/reload check: a turn that executes an image-producing + // tool must leave the image in the saved session, so reloads and + // reconnects can rebuild it from history. + agent + .add_manual_tool_use( + "call_1".to_string(), + "read".to_string(), + serde_json::json!({}), + ) + .expect("record tool use"); + let output = ToolOutput::new("Image ready").with_labeled_image( + "image/png", + "ZmFrZQ==", + "screenshots/example.png", + ); + agent + .add_manual_tool_result("call_1".to_string(), output, 10) + .expect("record tool result"); + + let persisted = crate::session::Session::load(agent.session_id()).expect("load saved session"); + let tool_message = persisted + .messages + .iter() + .find(|message| { + message + .content + .iter() + .any(|block| matches!(block, ContentBlock::ToolResult { .. })) + }) + .expect("tool result message must be saved"); + let tool_blocks = &tool_message.content; + assert!( + tool_blocks.iter().any( + |block| matches!(block, ContentBlock::Image { media_type, data } + if media_type == "image/png" && data == "ZmFrZQ==") + ), + "a text-only provider must still persist tool images into history so \ + reloads can render them (greptile P1: they vanish on reload otherwise)" + ); + assert!( + !tool_blocks.iter().any(|block| { + matches!(block, ContentBlock::Text { text, .. } + if text.contains("Image output omitted")) + }), + "the omission note must not replace the image in saved history" + ); +} + #[tokio::test] async fn queued_soft_interrupt_images_are_injected_as_image_blocks() { let provider: Arc = Arc::new(NativeAutoCompactionProvider); diff --git a/crates/jcode-app-core/src/replay/tests.rs b/crates/jcode-app-core/src/replay/tests.rs index e6364608fa..ab73462772 100644 --- a/crates/jcode-app-core/src/replay/tests.rs +++ b/crates/jcode-app-core/src/replay/tests.rs @@ -5,7 +5,7 @@ use crate::session::{StoredReplayEvent, StoredReplayEventKind}; use chrono::{Duration, Utc}; use std::ffi::OsString; -fn lock_env() -> std::sync::MutexGuard<'static, ()> { +fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-app-core/src/restart_snapshot_tests.rs b/crates/jcode-app-core/src/restart_snapshot_tests.rs index 2260667f29..4ea263a181 100644 --- a/crates/jcode-app-core/src/restart_snapshot_tests.rs +++ b/crates/jcode-app-core/src/restart_snapshot_tests.rs @@ -9,7 +9,7 @@ use std::ffi::OsString; struct TestEnvGuard { prev_home: Option, _temp_home: tempfile::TempDir, - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, } impl TestEnvGuard { diff --git a/crates/jcode-app-core/src/server/client_lightweight_control.rs b/crates/jcode-app-core/src/server/client_lightweight_control.rs index d20936ab44..7bcc027874 100644 --- a/crates/jcode-app-core/src/server/client_lightweight_control.rs +++ b/crates/jcode-app-core/src/server/client_lightweight_control.rs @@ -1033,7 +1033,7 @@ mod lightweight_swarm_member_tests { struct HomeGuard { prev: Option, _temp: tempfile::TempDir, - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, } impl Drop for HomeGuard { diff --git a/crates/jcode-app-core/src/server/comm_control_tests.rs b/crates/jcode-app-core/src/server/comm_control_tests.rs index 57a94e45a2..1c02212aa0 100644 --- a/crates/jcode-app-core/src/server/comm_control_tests.rs +++ b/crates/jcode-app-core/src/server/comm_control_tests.rs @@ -20,7 +20,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use tokio::sync::{Mutex, RwLock, broadcast, mpsc}; struct RuntimeEnvGuard { - _guard: std::sync::MutexGuard<'static, ()>, + _guard: crate::storage::TestEnvGuard, prev_runtime: Option, } diff --git a/crates/jcode-app-core/src/server/comm_plan_tests.rs b/crates/jcode-app-core/src/server/comm_plan_tests.rs index b6444a039f..dbcf2ad52c 100644 --- a/crates/jcode-app-core/src/server/comm_plan_tests.rs +++ b/crates/jcode-app-core/src/server/comm_plan_tests.rs @@ -16,7 +16,7 @@ use std::time::Instant; use tokio::sync::{RwLock, broadcast, mpsc}; struct RuntimeEnvGuard { - _guard: std::sync::MutexGuard<'static, ()>, + _guard: crate::storage::TestEnvGuard, prev_runtime: Option, } diff --git a/crates/jcode-app-core/src/server/debug_command_exec.rs b/crates/jcode-app-core/src/server/debug_command_exec.rs index 9fa33cfb0b..47c5e56d7c 100644 --- a/crates/jcode-app-core/src/server/debug_command_exec.rs +++ b/crates/jcode-app-core/src/server/debug_command_exec.rs @@ -730,7 +730,7 @@ mod tests { /// guarding one global serialize nothing, which showed up as a rotating set /// of failures under `cargo test` that all passed with `--test-threads=1` /// (issue #593). Everything touching the environment must share one lock. - fn lock_env() -> std::sync::MutexGuard<'static, ()> { + fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-app-core/src/server/debug_swarm_write.rs b/crates/jcode-app-core/src/server/debug_swarm_write.rs index 82ebe91d9f..7a2d846678 100644 --- a/crates/jcode-app-core/src/server/debug_swarm_write.rs +++ b/crates/jcode-app-core/src/server/debug_swarm_write.rs @@ -533,7 +533,7 @@ mod tests { use std::time::Duration; struct EnvGuard { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, runtime: Option, } diff --git a/crates/jcode-app-core/src/server/debug_testers_tests.rs b/crates/jcode-app-core/src/server/debug_testers_tests.rs index 83b6fb798f..31de6b17fd 100644 --- a/crates/jcode-app-core/src/server/debug_testers_tests.rs +++ b/crates/jcode-app-core/src/server/debug_testers_tests.rs @@ -1,12 +1,12 @@ use super::{load_testers, save_testers}; use std::ffi::OsString; -fn lock_env() -> std::sync::MutexGuard<'static, ()> { +fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } struct TestHomeGuard { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, prev_home: Option, _temp_home: tempfile::TempDir, } diff --git a/crates/jcode-app-core/src/server/debug_tests.rs b/crates/jcode-app-core/src/server/debug_tests.rs index 6ee5d53e06..2dffd13e47 100644 --- a/crates/jcode-app-core/src/server/debug_tests.rs +++ b/crates/jcode-app-core/src/server/debug_tests.rs @@ -428,12 +428,12 @@ mod debug_execution_tests { use std::sync::Arc; use tokio::sync::{Mutex as AsyncMutex, RwLock}; - fn lock_env() -> std::sync::MutexGuard<'static, ()> { + fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } struct EnvVarGuard { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, key: &'static str, previous: Option, } diff --git a/crates/jcode-app-core/src/server/lifecycle.rs b/crates/jcode-app-core/src/server/lifecycle.rs index d2543b7ce5..fa0c961f4d 100644 --- a/crates/jcode-app-core/src/server/lifecycle.rs +++ b/crates/jcode-app-core/src/server/lifecycle.rs @@ -287,7 +287,7 @@ mod tests { use super::*; struct EnvGuard { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, entries: Vec<(&'static str, Option)>, } diff --git a/crates/jcode-app-core/src/server/provider_control_tests.rs b/crates/jcode-app-core/src/server/provider_control_tests.rs index dbb16c371e..90c1d7b58e 100644 --- a/crates/jcode-app-core/src/server/provider_control_tests.rs +++ b/crates/jcode-app-core/src/server/provider_control_tests.rs @@ -6,9 +6,9 @@ use crate::tool::Registry; use async_trait::async_trait; use std::collections::HashMap; use std::pin::Pin; +use std::sync::Mutex as StdMutex; use std::sync::RwLock as StdRwLock; use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::{Mutex as StdMutex, MutexGuard as StdMutexGuard}; #[tokio::test] async fn invalidate_openai_usage_acknowledges_after_clearing_pinned_daemon_cooldown() { @@ -381,14 +381,14 @@ impl Provider for AuthChangeMockProvider { /// Shared process-wide lock: env vars are global, so a private mutex here would /// race every other env-mutating test (issue #593). -fn lock_env() -> StdMutexGuard<'static, ()> { +fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } struct EnvGuard { saved: Vec<(&'static str, Option)>, _temp_home: tempfile::TempDir, - _lock: StdMutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, } impl EnvGuard { diff --git a/crates/jcode-app-core/src/server/swarm_mutation_state_tests.rs b/crates/jcode-app-core/src/server/swarm_mutation_state_tests.rs index 911fa24dec..0741d749fd 100644 --- a/crates/jcode-app-core/src/server/swarm_mutation_state_tests.rs +++ b/crates/jcode-app-core/src/server/swarm_mutation_state_tests.rs @@ -5,7 +5,7 @@ use super::{ use crate::protocol::ServerEvent; struct RuntimeEnvGuard { - _guard: std::sync::MutexGuard<'static, ()>, + _guard: crate::storage::TestEnvGuard, prev_runtime: Option, } diff --git a/crates/jcode-app-core/src/server/swarm_persistence_tests.rs b/crates/jcode-app-core/src/server/swarm_persistence_tests.rs index 904153189a..53a10044be 100644 --- a/crates/jcode-app-core/src/server/swarm_persistence_tests.rs +++ b/crates/jcode-app-core/src/server/swarm_persistence_tests.rs @@ -2,7 +2,7 @@ use super::*; use std::time::{Duration, Instant}; struct EnvGuard { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, runtime: Option, } diff --git a/crates/jcode-app-core/src/tool/browser_session_tests.rs b/crates/jcode-app-core/src/tool/browser_session_tests.rs index 2132d990ef..c81a09b02d 100644 --- a/crates/jcode-app-core/src/tool/browser_session_tests.rs +++ b/crates/jcode-app-core/src/tool/browser_session_tests.rs @@ -4,7 +4,7 @@ use std::os::unix::fs::PermissionsExt; use std::time::{Duration, Instant}; struct BrowserFixture { - _env_guard: std::sync::MutexGuard<'static, ()>, + _env_guard: crate::storage::TestEnvGuard, temp: tempfile::TempDir, previous_home: Option, previous_session: Option, diff --git a/crates/jcode-app-core/src/tool/browser_user_browser_tests.rs b/crates/jcode-app-core/src/tool/browser_user_browser_tests.rs index 0a1ac2f8cc..d7626d3d61 100644 --- a/crates/jcode-app-core/src/tool/browser_user_browser_tests.rs +++ b/crates/jcode-app-core/src/tool/browser_user_browser_tests.rs @@ -60,7 +60,7 @@ else: "#; struct Fixture { - _env_guard: std::sync::MutexGuard<'static, ()>, + _env_guard: crate::storage::TestEnvGuard, temp: tempfile::TempDir, previous_home: Option, previous_session: Option, diff --git a/crates/jcode-app-core/src/tool/conversation_search.rs b/crates/jcode-app-core/src/tool/conversation_search.rs index 476e36be88..2136744bcb 100644 --- a/crates/jcode-app-core/src/tool/conversation_search.rs +++ b/crates/jcode-app-core/src/tool/conversation_search.rs @@ -356,7 +356,7 @@ mod tests { ConversationSearchTool::new(manager) } - fn env_lock() -> std::sync::MutexGuard<'static, ()> { + fn env_lock() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-base/src/auth/lifecycle.rs b/crates/jcode-base/src/auth/lifecycle.rs index 4feab2a4db..5f711862af 100644 --- a/crates/jcode-base/src/auth/lifecycle.rs +++ b/crates/jcode-base/src/auth/lifecycle.rs @@ -1120,10 +1120,9 @@ pub fn model_switch_request_for_provider_id( #[cfg(test)] mod tests { use super::*; - use std::sync::MutexGuard; struct EnvGuard { - _lock: MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, saved: Vec<(&'static str, Option)>, } diff --git a/crates/jcode-base/src/auth/test_sandbox.rs b/crates/jcode-base/src/auth/test_sandbox.rs index bb455bdcfe..ee861f188a 100644 --- a/crates/jcode-base/src/auth/test_sandbox.rs +++ b/crates/jcode-base/src/auth/test_sandbox.rs @@ -1,10 +1,9 @@ use std::path::{Path, PathBuf}; -use std::sync::MutexGuard; use crate::provider_catalog::{OpenAiCompatibleProfile, openai_compatible_profiles}; pub struct AuthTestSandbox { - _lock: MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, temp: tempfile::TempDir, saved_env: Vec<(String, Option)>, } diff --git a/crates/jcode-base/src/copilot_usage.rs b/crates/jcode-base/src/copilot_usage.rs index c79c891800..2f94951162 100644 --- a/crates/jcode-base/src/copilot_usage.rs +++ b/crates/jcode-base/src/copilot_usage.rs @@ -106,7 +106,7 @@ mod tests { }; use std::ffi::OsString; - fn lock_env() -> std::sync::MutexGuard<'static, ()> { + fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-base/src/gateway/control.rs b/crates/jcode-base/src/gateway/control.rs index 795c7c536f..cea3fb9ae0 100644 --- a/crates/jcode-base/src/gateway/control.rs +++ b/crates/jcode-base/src/gateway/control.rs @@ -442,7 +442,7 @@ mod tests { /// left them racing the config and provider suites: a test here would swap /// `JCODE_HOME` out from under a provider test mid-assertion, failing it /// intermittently whenever the two modules happened to interleave. - fn lock_env() -> std::sync::MutexGuard<'static, ()> { + fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-base/src/mcp/oauth_tests.rs b/crates/jcode-base/src/mcp/oauth_tests.rs index 47ef470aab..c4eb22839b 100644 --- a/crates/jcode-base/src/mcp/oauth_tests.rs +++ b/crates/jcode-base/src/mcp/oauth_tests.rs @@ -6,7 +6,7 @@ use tokio::io::{AsyncReadExt, AsyncWriteExt}; // These tests serve real HTTP on ephemeral 127.0.0.1 sockets. No mocked client, // copied OAuth implementation, external provider, browser executable or secrets. struct Sandbox { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, temp: tempfile::TempDir, saved: Option, } diff --git a/crates/jcode-base/src/process_memory.rs b/crates/jcode-base/src/process_memory.rs index 5dde75da70..1c4ca29f82 100644 --- a/crates/jcode-base/src/process_memory.rs +++ b/crates/jcode-base/src/process_memory.rs @@ -177,7 +177,7 @@ pub fn snapshot_with_source(source: impl Into) -> ProcessMemorySnapshot snapshot } -#[cfg(not(target_os = "linux"))] +#[cfg(not(any(target_os = "linux", target_os = "macos")))] pub fn snapshot_with_source(source: impl Into) -> ProcessMemorySnapshot { let source = source.into(); logging::debug(&format!( @@ -188,6 +188,123 @@ pub fn snapshot_with_source(source: impl Into) -> ProcessMemorySnapshot snapshot } +#[cfg(target_os = "macos")] +pub fn snapshot_with_source(source: impl Into) -> ProcessMemorySnapshot { + let source = source.into(); + let snapshot = ProcessMemorySnapshot { + rss_bytes: mach_task_basic_info().map(|b| b.resident_size), + peak_rss_bytes: mach_task_basic_info().map(|b| b.resident_size_max), + virtual_bytes: mach_task_basic_info().map(|b| b.virtual_size), + thread_count: None, + main_stack_bytes: None, + os: mach_task_vm_info().map(|b| OsProcessMemoryInfo { + // phys_footprint is what the kernel charges for dirty + + // compressed anonymous memory, the closest analog of Linux + // Pss_Anon for the hibernation notice; `internal` is the + // resident dirty part of it (Linux RssAnon analog). + pss_anon_bytes: Some(b.phys_footprint), + rss_anon_bytes: Some(b.internal), + ..OsProcessMemoryInfo::default() + }), + allocator: allocator_info(), + }; + logging::debug(&format!( + "process memory snapshot source={source} rss={:?} private={:?} allocator={}", + snapshot.rss_bytes, + snapshot.os.as_ref().and_then(|os| os.pss_anon_bytes), + snapshot.allocator.name + )); + record_snapshot(source, snapshot.clone()); + snapshot +} + +/// `mach_task_basic_info_data_t`: filled through a raw call so the struct +/// never has to be spelled out against a specific SDK layout. +#[cfg(target_os = "macos")] +#[derive(Default)] +struct TaskBasicInfo { + virtual_size: u64, + resident_size: u64, + resident_size_max: u64, +} + +/// `task_vm_info` head: everything up to `phys_footprint` (rev1) plus the +/// two fields the snapshot reads. The full struct keeps growing with new +/// ledger tags, so pass a buffer big enough for rev2/rev3 and read only +/// these fields by offset. +#[cfg(target_os = "macos")] +#[derive(Default)] +struct TaskVmInfo { + internal: u64, + phys_footprint: u64, +} + +// task_info() count is in natural_t (u32) words, so a 1024-word buffer +// covers every task_vm_info revision shipped so far (~60 words). +#[cfg(target_os = "macos")] +const TASK_INFO_BUFFER_WORDS: usize = 1024; + +#[cfg(target_os = "macos")] +fn mach_task_basic_info() -> Option { + unsafe extern "C" { + fn mach_task_self() -> u32; + fn task_info(target: u32, flavor: u32, info: *mut u32, count: *mut u32) -> i32; + } + const MACH_TASK_BASIC_INFO: u32 = 20; + let mut buffer = [0u32; TASK_INFO_BUFFER_WORDS]; + let mut count = buffer.len() as u32; + let kr = unsafe { + task_info( + mach_task_self(), + MACH_TASK_BASIC_INFO, + buffer.as_mut_ptr(), + &mut count, + ) + }; + (kr == 0).then(|| TaskBasicInfo { + // mach_task_basic_info_data_t as the kernel packs it for flavor 20: + // virtual_size (u64@0), resident_size (u64@8), resident_size_max + // (u64@16). Verified against ps(1) rss/vsz on arm64. + virtual_size: read_u64(&buffer, 0), + resident_size: read_u64(&buffer, 8), + resident_size_max: read_u64(&buffer, 16), + }) +} + +#[cfg(target_os = "macos")] +fn mach_task_vm_info() -> Option { + unsafe extern "C" { + fn mach_task_self() -> u32; + fn task_info(target: u32, flavor: u32, info: *mut u32, count: *mut u32) -> i32; + } + const TASK_VM_INFO: u32 = 22; + let mut buffer = [0u32; TASK_INFO_BUFFER_WORDS]; + let mut count = buffer.len() as u32; + let kr = unsafe { + task_info( + mach_task_self(), + TASK_VM_INFO, + buffer.as_mut_ptr(), + &mut count, + ) + }; + (kr == 0).then(|| TaskVmInfo { + // task_vm_info head: virtual_size, region_count, page_size, + // resident, resident_peak, device, device_peak, then `internal` + // at word 12. phys_footprint is the rev1 tail at word 36. + internal: read_u64(&buffer, 48), + phys_footprint: read_u64(&buffer, 144), + }) +} + +/// Read a u64 from a packed buffer at a byte offset. +#[cfg(target_os = "macos")] +fn read_u64(words: &[u32; TASK_INFO_BUFFER_WORDS], offset: usize) -> u64 { + let lo = words[offset / 4] as u64; + let hi = words[offset / 4 + 1] as u64; + lo | (hi << 32) +} + pub fn history(limit: usize) -> Vec { let Ok(history) = memory_history().lock() else { logging::error("process memory history lock poisoned; returning empty history"); @@ -929,6 +1046,29 @@ fn parse_proc_value_bytes(status: &str, key: &str) -> Option { mod tests { use super::*; + #[cfg(target_os = "macos")] + #[test] + fn macos_snapshot_reports_rss_and_private_memory() { + let snapshot = snapshot(); + let rss = snapshot.rss_bytes.expect("task_info resident_size"); + assert!(rss > 0, "resident_size must be positive, got {rss}"); + let os = snapshot.os.expect("task_info TASK_VM_INFO"); + let footprint = os.pss_anon_bytes.expect("phys_footprint"); + assert!( + footprint > 0, + "phys_footprint must be positive, got {footprint}" + ); + // phys_footprint counts dirty + compressed pages; resident RSS counts + // resident pages incl. shared libraries. Neither bounds the other + // strictly, but both are in the same order of magnitude for a process + // this size. + let larger = rss.max(footprint); + let smaller = rss.min(footprint); + assert!( + smaller * 16 > larger, + "rss {rss} vs phys_footprint {footprint} beyond one order of magnitude apart" + ); + } #[test] fn estimate_json_bytes_matches_serialized_length_without_buffering_output() { let value = serde_json::json!({ diff --git a/crates/jcode-base/src/prompt.rs b/crates/jcode-base/src/prompt.rs index 74d1362467..c132b18100 100644 --- a/crates/jcode-base/src/prompt.rs +++ b/crates/jcode-base/src/prompt.rs @@ -34,22 +34,38 @@ pub fn load_base_system_prompt(working_dir: Option<&Path>) -> String { /// Prompt guidance for the optional Mermaid rendering capability. pub const MERMAID_PROMPT: &str = "# Mermaid\n\nRender fenced `mermaid` blocks inline."; +/// Prompt guidance injected when the active model cannot accept image input. +/// +/// Text-only models happily call screenshot/browser-capture tools because the +/// tool list does not say anything about modality; without this notice the +/// pixels get injected into the request and the turn fails. The notice is +/// part of the static prompt (model changes are rare and rebuild the split), +/// so the model knows up front that it must rely on textual alternatives. +pub const TEXT_ONLY_MODEL_PROMPT: &str = "# Model Image Capability\n\nThe model you are running does not support image input. Tool outputs that produce images (screenshots, browser captures, `read` of image files, generated images) will not carry their pixels into your context: jcode omits them and tells you so. Do not attempt visual inspection of images; rely on text-based alternatives (for example the `ocr` action for screen text, file contents, tool textual summaries) or ask the user to describe what is visible."; + /// Harness capabilities that conditionally contribute prompt modules. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PromptCapabilities { pub mermaid: bool, + /// True when the active provider/model cannot accept image input and the + /// prompt should say so. + pub text_only_model: bool, } impl Default for PromptCapabilities { fn default() -> Self { - Self { mermaid: true } + Self { + mermaid: true, + text_only_model: false, + } } } impl PromptCapabilities { - fn current() -> Self { + pub fn current() -> Self { Self { mermaid: crate::config::config().features.mermaid, + text_only_model: false, } } } @@ -62,6 +78,9 @@ fn base_system_prompt_parts( if capabilities.mermaid { parts.push(MERMAID_PROMPT.to_string()); } + if capabilities.text_only_model { + parts.push(TEXT_ONLY_MODEL_PROMPT.to_string()); + } parts } @@ -515,7 +534,30 @@ pub fn build_system_prompt_split_with_agents_md( ) } -fn build_system_prompt_split_with_capabilities_and_agents_md( +/// Build a split prompt with explicit harness capabilities and an already +/// captured AGENTS.md snapshot. Callers that know the active model's image +/// capability (the agent's provider) pass it via `capabilities.text_only_model`. +pub fn build_system_prompt_split_with_capabilities_and_agents_md( + skill_prompt: Option<&str>, + available_skills: &[SkillInfo], + is_selfdev: bool, + memory_prompt: Option<&str>, + working_dir: Option<&Path>, + capabilities: PromptCapabilities, + agents_md: (Option, ContextInfo), +) -> (SplitSystemPrompt, ContextInfo) { + build_system_prompt_split_with_capabilities_and_agents_md_inner( + skill_prompt, + available_skills, + is_selfdev, + memory_prompt, + working_dir, + capabilities, + agents_md, + ) +} + +fn build_system_prompt_split_with_capabilities_and_agents_md_inner( skill_prompt: Option<&str>, available_skills: &[SkillInfo], is_selfdev: bool, diff --git a/crates/jcode-base/src/prompt_tests.rs b/crates/jcode-base/src/prompt_tests.rs index 833e514682..e792786e79 100644 --- a/crates/jcode-base/src/prompt_tests.rs +++ b/crates/jcode-base/src/prompt_tests.rs @@ -24,7 +24,10 @@ fn mermaid_prompt_module_follows_capability() { false, None, None, - PromptCapabilities { mermaid: true }, + PromptCapabilities { + mermaid: true, + text_only_model: false, + }, ); assert!(enabled.static_part.contains(MERMAID_PROMPT)); @@ -34,12 +37,50 @@ fn mermaid_prompt_module_follows_capability() { false, None, None, - PromptCapabilities { mermaid: false }, + PromptCapabilities { + mermaid: false, + text_only_model: false, + }, ); assert!(!disabled.static_part.contains("Mermaid diagrams")); assert!(!disabled.static_part.contains("fenced `mermaid` code block")); } +#[test] +fn text_only_model_prompt_follows_capability() { + let (text_only, _) = build_system_prompt_split_with_capabilities( + None, + &[], + false, + None, + None, + PromptCapabilities { + mermaid: false, + text_only_model: true, + }, + ); + assert!(text_only.static_part.contains(TEXT_ONLY_MODEL_PROMPT)); + assert!( + text_only + .static_part + .contains("does not support image input") + ); + + let (vision, _) = build_system_prompt_split_with_capabilities( + None, + &[], + false, + None, + None, + PromptCapabilities { + mermaid: false, + text_only_model: false, + }, + ); + assert!(!vision.static_part.contains("Model Image Capability")); + assert!(!vision.static_part.contains("does not support image input")); +} + /// Verify skill prompts don't accidentally introduce "Claude Code" identity #[test] fn test_skill_prompt_integration() { @@ -199,6 +240,10 @@ fn agents_md_distinct_project_and_global_files_are_both_loaded() { #[test] fn captured_agents_md_keeps_split_prompt_stable_after_file_write() { + // Serialize against tests that mutate JCODE_HOME: the static prompt embeds + // global preferred-tools content, so a parallel env swap breaks the + // before/after equality below. + let _guard = crate::storage::lock_test_env(); let project_dir = tempfile::TempDir::new().unwrap(); let agents_md = project_dir.path().join("AGENTS.md"); std::fs::write(&agents_md, "original session instructions").unwrap(); diff --git a/crates/jcode-base/src/provider/accessors.rs b/crates/jcode-base/src/provider/accessors.rs index 0af913bbc8..29946d241b 100644 --- a/crates/jcode-base/src/provider/accessors.rs +++ b/crates/jcode-base/src/provider/accessors.rs @@ -58,6 +58,29 @@ impl MultiProvider { ProviderRegistry::new(self).active_openrouter_execution() } + /// The key runtime image-input capability records (rejections *and* + /// lookups) use for `provider`'s active runtime. + /// + /// Providers other than OpenRouter have one runtime per slot, so the + /// stable slot key identifies them. The OpenRouter slot multiplexes + /// several distinct endpoints (real OpenRouter, named OpenAI-compatible + /// profiles, direct endpoints) behind one `name()`, and two of those + /// endpoints can serve the same model name with *different* image + /// capabilities, so the record/lookup key must be the concrete runtime's + /// [`Provider::capability_scope`] (profile id or endpoint base) instead + /// of the bare slot key. Falls back to the slot key when no runtime is + /// installed or the runtime offers no scope. + pub(super) fn capability_key_for(&self, provider: ActiveProvider) -> String { + if provider == ActiveProvider::OpenRouter + && let Some(scope) = self + .active_openrouter_execution_provider() + .and_then(|runtime| runtime.capability_scope()) + { + return scope; + } + Self::provider_key(provider).to_string() + } + pub(super) fn clear_active_openai_compatible_profile(&self) { ProviderRegistry::new(self).clear_active_compatible_profile(); } diff --git a/crates/jcode-base/src/provider/catalog_routes.rs b/crates/jcode-base/src/provider/catalog_routes.rs index 10725bba17..d5ebb15872 100644 --- a/crates/jcode-base/src/provider/catalog_routes.rs +++ b/crates/jcode-base/src/provider/catalog_routes.rs @@ -1175,7 +1175,7 @@ mod tests { struct EnvGuard { vars: Vec<(&'static str, Option)>, _temp: tempfile::TempDir, - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, } impl EnvGuard { diff --git a/crates/jcode-base/src/provider/mod.rs b/crates/jcode-base/src/provider/mod.rs index aadf4ee248..5b25215602 100644 --- a/crates/jcode-base/src/provider/mod.rs +++ b/crates/jcode-base/src/provider/mod.rs @@ -18,6 +18,15 @@ mod failover; mod fingerprint; pub mod gemini; mod image_clamp; +mod image_capability { + //! Explicit re-exports from `jcode_provider_core` used by the + //! app-spine failover path in this crate (the OpenAI-compatible runtime + //! imports the remaining items directly from `jcode_provider_core`). + pub use jcode_provider_core::image_capability::{ + clear_image_input_overrides_for_provider, error_indicates_image_rejection, + image_input_rejected, record_image_input_rejection, + }; +} pub mod jcode; pub mod models; mod multi_provider; @@ -130,6 +139,16 @@ pub(crate) fn new_bedrock_provider() -> bedrock::BedrockProvider { provider } +/// Whether any message in the request carries an image block. Cheap pre-scan +/// for the modality-rejection retry path so non-image requests never pay for +/// the capability bookkeeping. +fn messages_contain_images(messages: &[Message]) -> bool { + messages + .iter() + .flat_map(|message| message.content.iter()) + .any(|block| matches!(block, crate::message::ContentBlock::Image { .. })) +} + /// Register the live agent provider so background helpers (memory sidecar) can /// reach whatever provider the user is actually running on. Safe to call more /// than once; the most recent registration wins. @@ -606,6 +625,19 @@ impl MultiProvider { let (estimated_input_chars, estimated_input_tokens) = Self::estimate_request_input(messages, tools, mode); + // A recorded runtime rejection (this endpoint/model rejected image + // input earlier, see `image_capability`) is authoritative over the + // static capability answer, so a failed request can be replayed once + // with every image block replaced by a text marker instead of + // repeating the doomed request. + let mut retry_with_images_filtered = false; + let mut messages: &[Message] = messages; + // Owns the filtered copy for the retry attempt; kept alive until the + // function returns so `messages` can borrow it. Deferred-init: the + // only read (`as_deref` below) is always preceded by the assignment + // in the same iteration. + let mut filtered_retry_messages: Option>; + for candidate in sequence { let label = Self::provider_label(candidate); let key = self.provider_runtime_cooldown_key(candidate); @@ -678,6 +710,12 @@ impl MultiProvider { continue; } + // One modality-retry per candidate: if the request carried image + // blocks and the endpoint answered "this model cannot accept + // images", replay the SAME candidate once with the images replaced + // by text markers. `continue` on the outer loop would silently + // fail over to a different provider, which is not the intent: the + // endpoint is healthy, only the image modality is unsupported. let attempt = match mode { CompletionMode::Unified { system } => { self.complete_on_provider(candidate, messages, tools, system, resume_session_id) @@ -698,7 +736,64 @@ impl MultiProvider { .await } }; - let attempt = self.await_account_stream_start(candidate, attempt).await; + let mut attempt = self.await_account_stream_start(candidate, attempt).await; + + if let Err(err) = &attempt + && !retry_with_images_filtered + && messages_contain_images(messages) + && image_capability::error_indicates_image_rejection(&err.to_string()) + { + // Record under the same capability-scoped key the lookups + // use (concrete runtime identity for the multiplexed + // OpenRouter slot), so a text-only endpoint's rejection never + // suppresses images for a vision-capable endpoint serving the + // same model name behind the same provider slot. + image_capability::record_image_input_rejection( + Some(self.capability_key_for(candidate).as_str()), + &self.candidate_model(candidate), + &err.to_string(), + ); + crate::logging::warn(&format!( + "Provider {} rejected image input for this model; \ + retrying once with images replaced by text markers", + label + )); + if let Some(filtered) = + image_clamp::filter_unsupported_outbound_images(messages, false) + { + retry_with_images_filtered = true; + filtered_retry_messages = Some(filtered); + // `as_deref` is `Some` here by construction; fall back to + // an empty slice rather than panicking on the invariant. + messages = filtered_retry_messages.as_deref().unwrap_or_default(); + attempt = match mode { + CompletionMode::Unified { system } => { + self.complete_on_provider( + candidate, + messages, + tools, + system, + resume_session_id, + ) + .await + } + CompletionMode::Split { + system_static, + system_dynamic, + } => { + self.complete_split_on_provider( + candidate, + messages, + tools, + system_static, + system_dynamic, + resume_session_id, + ) + .await + } + }; + } + } match attempt { Ok(stream) => { @@ -732,6 +827,7 @@ impl MultiProvider { summary.clone() }; let decision = Self::classify_failover_error(&err); + crate::logging::info(&format!( "Provider {} failed{}: {} (failover={} decision={})", label, @@ -768,6 +864,45 @@ impl MultiProvider { Err(self.no_provider_available_error(¬es)) } + /// Model id the candidate runtime would serve right now, for scoping + /// runtime image-capability records. + fn candidate_model(&self, candidate: ActiveProvider) -> String { + match candidate { + ActiveProvider::Claude => self + .anthropic_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::OpenAI => self + .openai_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::Copilot => self + .copilot_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::Antigravity => self + .antigravity_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::Gemini => self + .gemini_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::Cursor => self + .cursor_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::Bedrock => self + .bedrock_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + ActiveProvider::OpenRouter => self + .active_openrouter_execution_provider() + .map(|provider| provider.model()) + .unwrap_or_default(), + } + } + /// Record which login/credential just served a request in the /// cross-provider activity ledger (drives `/usage` recency sorting). /// Spawned off-thread: the ledger does file IO and a request was already @@ -1009,12 +1144,172 @@ impl MultiProvider { self.set_model_on_provider_with_credential_modes(provider, model, None, None) } - /// Bind the shared OpenAI-compatible slot to the managed jcode endpoint. - /// - /// Subscription model ids intentionally overlap with direct Anthropic and - /// OpenAI ids. A picker selection therefore cannot be reduced to a bare - /// model name: doing so lets `set_model`'s family heuristic spend the - /// user's unrelated provider credentials instead of their subscription. + /// Route a model switch to the concrete runtime it targets, without any + /// capability-record bookkeeping. [`Provider::set_model`] wraps this and + /// defers clearing image-input overrides until the switch succeeds. + fn set_model_route(&self, model: &str) -> Result<()> { + let requested_model = model.trim(); + if requested_model.is_empty() { + anyhow::bail!("Model cannot be empty"); + } + + if let Some((prefix, target_model)) = requested_model.split_once(':') + && let Some((method, label)) = prefix.split_once('@') + && matches!(method, "claude-oauth" | "openai-oauth") + { + return self.set_model_on_saved_account(method, label, target_model); + } + // Bare model changes keep the selected login. Explicit generic routes + // opt back into the normal active-account preference. + if let Some((prefix, _)) = requested_model.split_once(':') + && let Some(route) = + jcode_provider_core::AuthRoute::parse_explicit_credential_prefix(prefix) + { + let runtime = match route.provider { + jcode_provider_core::DualAuthProvider::Anthropic => self.anthropic_provider(), + jcode_provider_core::DualAuthProvider::OpenAI => self.openai_provider(), + }; + if let Some(runtime) = runtime { + runtime.set_oauth_account(None)?; + } + } + + if let Some(target_model) = requested_model.strip_prefix("grok-build:") { + let target_model = target_model.trim(); + if target_model.is_empty() { + anyhow::bail!("Grok Build model cannot be empty"); + } + let registry = ProviderRegistry::new(self); + let provider = registry + .compatible_profile(GROK_BUILD_PROFILE_ID) + .or_else(|| { + external::instantiate_expected_external_provider(external::GROK_BUILD_RUNTIME) + }) + .ok_or_else(|| anyhow!("Grok Build is not authenticated"))?; + provider.set_model(target_model)?; + registry.install_compatible_profile(GROK_BUILD_PROFILE_ID, provider); + registry.set_active_compatible_profile(GROK_BUILD_PROFILE_ID); + self.set_active_provider(ActiveProvider::OpenRouter); + return Ok(()); + } + + // `jcode:` is the explicit spec for the Jcode subscription + // runtime. Without this it fell through to the active provider and + // `--model jcode:gpt-5.5` silently stayed on the previous model. + if let Some(target_model) = jcode_subscription_model_prefix(requested_model) { + return self.set_model_on_jcode_subscription(target_model); + } + + if let Some((profile, target_model)) = Self::openai_compatible_model_prefix(requested_model) + { + return self.set_model_on_openai_compatible_profile(profile, target_model); + } + + // User-defined named provider profiles from config (`[providers.]`). + // The model picker emits `:` specs for their routes + // (issue #444), so the switch must bind that profile's runtime instead + // of falling through to global model-name heuristics. + if let Some((profile_name, target_model)) = + Self::named_provider_profile_model_prefix(requested_model) + { + return self.set_model_on_named_provider_profile(&profile_name, &target_model); + } + + // Provider-prefixed model names are explicit routing directives. They + // must never silently fall through to another provider when the target + // is unavailable. + if let Some((target, prefix, target_model)) = + explicit_model_provider_prefix(requested_model) + { + // The single canonical parser decides whether this prefix pins a + // dual-auth credential (and which provider/mode). Bare `claude:` / + // `openai:` prefixes route without pinning a credential. + let pinned = jcode_provider_core::AuthRoute::parse_explicit_credential_prefix(prefix); + let openai_credential_mode = pinned.and_then(|route| { + matches!( + route.provider, + jcode_provider_core::DualAuthProvider::OpenAI + ) + .then(|| match route.mode { + jcode_provider_core::AuthMode::ApiKey => openai::OpenAICredentialMode::ApiKey, + jcode_provider_core::AuthMode::Oauth => openai::OpenAICredentialMode::OAuth, + }) + }); + let anthropic_credential_mode = pinned.and_then(|route| { + matches!( + route.provider, + jcode_provider_core::DualAuthProvider::Anthropic + ) + .then(|| match route.mode { + jcode_provider_core::AuthMode::ApiKey => { + anthropic::AnthropicCredentialMode::ApiKey + } + jcode_provider_core::AuthMode::Oauth => { + anthropic::AnthropicCredentialMode::OAuth + } + }) + }); + if openai_credential_mode.is_some() || anthropic_credential_mode.is_some() { + return self.set_model_on_provider_with_credential_modes( + target, + target_model, + openai_credential_mode, + anthropic_credential_mode, + ); + } + return self.set_model_on_provider(target, target_model); + } + + // A custom OpenAI-compatible endpoint owns opaque, provider-local model + // IDs. Keep unprefixed names on that active endpoint, even when they + // resemble a globally known model family. Picker and slash-command + // route specs carry explicit prefixes, so the branch above can switch + // to any configured provider at any time. + if self.active_provider() == ActiveProvider::OpenRouter + && self + .active_openrouter_execution_provider() + .is_some_and(|provider| !provider.supports_provider_routing_features()) + { + return self.set_model_on_provider(ActiveProvider::OpenRouter, requested_model); + } + + // Normalize Copilot-style model names (dots -> hyphens) to canonical form. + // e.g. "claude-opus-4.6" -> "claude-opus-4-6" so Anthropic accepts it. + let model = if let Some(canonical) = normalize_copilot_model_name(requested_model) { + canonical + } else { + requested_model + }; + + if let Some((base_model, provider_pin)) = model.rsplit_once('@') + && !provider_pin.trim().is_empty() + && let Some(openrouter_model) = openrouter_catalog_model_id(base_model) + { + return self.set_model_on_provider( + ActiveProvider::OpenRouter, + &format!("{}@{}", openrouter_model, provider_pin), + ); + } + + // Detect which provider an unprefixed model belongs to. + let target_provider = provider_for_model(model); + if let Some(target_provider) = target_provider + && let Some(target) = provider_from_model_key(target_provider) + { + self.set_model_on_provider(target, model) + } else if let Some(profile) = self.openai_compatible_profile_owning_model(model) { + // Bare ids from an OpenAI-compatible catalog (`celeris-1`, + // `mimo-v2.5`, ...) match none of the built-in model-name + // heuristics. Without this, `/model ` fell through to + // whichever provider happened to be active and failed with a + // misleading "not supported by " error. + self.set_model_on_openai_compatible_profile(profile, model) + } else { + // Unknown model - try current provider. + self.set_model_on_provider(self.active_provider(), model) + } + } + fn set_model_on_jcode_subscription(&self, model: &str) -> Result<()> { let model = model.trim(); if model.is_empty() { @@ -1986,7 +2281,21 @@ impl Provider for MultiProvider { } fn supports_image_input(&self) -> bool { - match self.active_provider() { + // A recorded runtime rejection is authoritative: the endpoint told us + // this exact model cannot accept images, so both the outbound filter + // and the turn loops must treat it as text-only even when the static + // capability answer (default-true for direct OpenAI-compatible + // endpoints) disagrees. The lookup is scoped the same way the + // record is (concrete runtime identity for the multiplexed + // OpenRouter slot), so a rejection on one endpoint never suppresses + // images for another endpoint serving the same model name. + let active = self.active_provider(); + let key = self.capability_key_for(active); + if image_capability::image_input_rejected(&key, &self.candidate_model(active)) { + return false; + } + + match active { ActiveProvider::Claude => self .anthropic_provider() .map(|provider| provider.supports_image_input()) @@ -2044,166 +2353,18 @@ impl Provider for MultiProvider { // the catalog on next read instead of serving the memoized copy. self.invalidate_routes_memo(); - let requested_model = model.trim(); - if requested_model.is_empty() { - anyhow::bail!("Model cannot be empty"); - } - - if let Some((prefix, target_model)) = requested_model.split_once(':') - && let Some((method, label)) = prefix.split_once('@') - && matches!(method, "claude-oauth" | "openai-oauth") - { - return self.set_model_on_saved_account(method, label, target_model); - } - // Bare model changes keep the selected login. Explicit generic routes - // opt back into the normal active-account preference. - if let Some((prefix, _)) = requested_model.split_once(':') - && let Some(route) = - jcode_provider_core::AuthRoute::parse_explicit_credential_prefix(prefix) - { - let runtime = match route.provider { - jcode_provider_core::DualAuthProvider::Anthropic => self.anthropic_provider(), - jcode_provider_core::DualAuthProvider::OpenAI => self.openai_provider(), - }; - if let Some(runtime) = runtime { - runtime.set_oauth_account(None)?; - } - } - - if let Some(target_model) = requested_model.strip_prefix("grok-build:") { - let target_model = target_model.trim(); - if target_model.is_empty() { - anyhow::bail!("Grok Build model cannot be empty"); - } - let registry = ProviderRegistry::new(self); - let provider = registry - .compatible_profile(GROK_BUILD_PROFILE_ID) - .or_else(|| { - external::instantiate_expected_external_provider(external::GROK_BUILD_RUNTIME) - }) - .ok_or_else(|| anyhow!("Grok Build is not authenticated"))?; - provider.set_model(target_model)?; - registry.install_compatible_profile(GROK_BUILD_PROFILE_ID, provider); - registry.set_active_compatible_profile(GROK_BUILD_PROFILE_ID); - self.set_active_provider(ActiveProvider::OpenRouter); - return Ok(()); - } - - // `jcode:` is the explicit spec for the Jcode subscription - // runtime. Without this it fell through to the active provider and - // `--model jcode:gpt-5.5` silently stayed on the previous model. - if let Some(target_model) = jcode_subscription_model_prefix(requested_model) { - return self.set_model_on_jcode_subscription(target_model); - } - - if let Some((profile, target_model)) = Self::openai_compatible_model_prefix(requested_model) - { - return self.set_model_on_openai_compatible_profile(profile, target_model); - } - - // User-defined named provider profiles from config (`[providers.]`). - // The model picker emits `:` specs for their routes - // (issue #444), so the switch must bind that profile's runtime instead - // of falling through to global model-name heuristics. - if let Some((profile_name, target_model)) = - Self::named_provider_profile_model_prefix(requested_model) - { - return self.set_model_on_named_provider_profile(&profile_name, &target_model); - } - - // Provider-prefixed model names are explicit routing directives. They - // must never silently fall through to another provider when the target - // is unavailable. - if let Some((target, prefix, target_model)) = - explicit_model_provider_prefix(requested_model) - { - // The single canonical parser decides whether this prefix pins a - // dual-auth credential (and which provider/mode). Bare `claude:` / - // `openai:` prefixes route without pinning a credential. - let pinned = jcode_provider_core::AuthRoute::parse_explicit_credential_prefix(prefix); - let openai_credential_mode = pinned.and_then(|route| { - matches!( - route.provider, - jcode_provider_core::DualAuthProvider::OpenAI - ) - .then(|| match route.mode { - jcode_provider_core::AuthMode::ApiKey => openai::OpenAICredentialMode::ApiKey, - jcode_provider_core::AuthMode::Oauth => openai::OpenAICredentialMode::OAuth, - }) - }); - let anthropic_credential_mode = pinned.and_then(|route| { - matches!( - route.provider, - jcode_provider_core::DualAuthProvider::Anthropic - ) - .then(|| match route.mode { - jcode_provider_core::AuthMode::ApiKey => { - anthropic::AnthropicCredentialMode::ApiKey - } - jcode_provider_core::AuthMode::Oauth => { - anthropic::AnthropicCredentialMode::OAuth - } - }) - }); - if openai_credential_mode.is_some() || anthropic_credential_mode.is_some() { - return self.set_model_on_provider_with_credential_modes( - target, - target_model, - openai_credential_mode, - anthropic_credential_mode, - ); - } - return self.set_model_on_provider(target, target_model); - } - - // A custom OpenAI-compatible endpoint owns opaque, provider-local model - // IDs. Keep unprefixed names on that active endpoint, even when they - // resemble a globally known model family. Picker and slash-command - // route specs carry explicit prefixes, so the branch above can switch - // to any configured provider at any time. - if self.active_provider() == ActiveProvider::OpenRouter - && self - .active_openrouter_execution_provider() - .is_some_and(|provider| !provider.supports_provider_routing_features()) - { - return self.set_model_on_provider(ActiveProvider::OpenRouter, requested_model); - } - - // Normalize Copilot-style model names (dots -> hyphens) to canonical form. - // e.g. "claude-opus-4.6" -> "claude-opus-4-6" so Anthropic accepts it. - let model = if let Some(canonical) = normalize_copilot_model_name(requested_model) { - canonical - } else { - requested_model - }; - - if let Some((base_model, provider_pin)) = model.rsplit_once('@') - && !provider_pin.trim().is_empty() - && let Some(openrouter_model) = openrouter_catalog_model_id(base_model) - { - return self.set_model_on_provider( - ActiveProvider::OpenRouter, - &format!("{}@{}", openrouter_model, provider_pin), - ); - } - - // Detect which provider an unprefixed model belongs to. - let target_provider = provider_for_model(model); - if let Some(target_provider) = target_provider - && let Some(target) = provider_from_model_key(target_provider) - { - self.set_model_on_provider(target, model) - } else if let Some(profile) = self.openai_compatible_profile_owning_model(model) { - // Bare ids from an OpenAI-compatible catalog (`celeris-1`, - // `mimo-v2.5`, ...) match none of the built-in model-name - // heuristics. Without this, `/model ` fell through to - // whichever provider happened to be active and failed with a - // misleading "not supported by " error. - self.set_model_on_openai_compatible_profile(profile, model) - } else { - // Unknown model - try current provider. - self.set_model_on_provider(self.active_provider(), model) - } + // A runtime image-input rejection is scoped to the model that earned + // it: a successful switch gives the new model a fresh capability + // answer. The clear is deferred until the switch succeeds, because a + // FAILED switch leaves the previous model active, and wiping its + // capability restriction would let the very next request ship pixels + // to the endpoint that just rejected them. + let previous_capability_key = self.capability_key_for(self.active_provider()); + let result = self.set_model_route(model); + if result.is_ok() { + image_capability::clear_image_input_overrides_for_provider(&previous_capability_key); + } + result } fn set_route_selection(&self, selection: &RouteSelection) -> Result<()> { diff --git a/crates/jcode-base/src/provider/tests.rs b/crates/jcode-base/src/provider/tests.rs index 5fdd95e939..8a1a9c3974 100644 --- a/crates/jcode-base/src/provider/tests.rs +++ b/crates/jcode-base/src/provider/tests.rs @@ -785,6 +785,7 @@ struct StubExternalRuntime { model: std::sync::RwLock, credential_mode: std::sync::RwLock, oauth_account: std::sync::RwLock>, + supports_images: std::sync::atomic::AtomicBool, } impl StubExternalRuntime { @@ -802,9 +803,20 @@ impl StubExternalRuntime { model: std::sync::RwLock::new(models[0].to_string()), credential_mode: std::sync::RwLock::new(jcode_provider_core::CredentialMode::Auto), oauth_account: std::sync::RwLock::new(None), + supports_images: std::sync::atomic::AtomicBool::new(false), } } + /// Opt this stub into advertising image support so capability-override + /// tests can observe a recorded rejection flipping the answer to false. + /// Takes and returns the shared `Arc` so the same instance can be handed + /// to a `MultiProvider` while the test keeps a handle for assertions. + fn with_image_support(self: Arc) -> Arc { + self.supports_images + .store(true, std::sync::atomic::Ordering::SeqCst); + self + } + fn cursor() -> Self { Self::new("cursor", "Cursor", "cursor", cursor::AVAILABLE_MODELS) } @@ -919,6 +931,10 @@ impl Provider for StubExternalRuntime { .unwrap_or_else(|poisoned| poisoned.into_inner()) = mode; Ok(()) } + fn supports_image_input(&self) -> bool { + self.supports_images + .load(std::sync::atomic::Ordering::SeqCst) + } fn fork(&self) -> Arc { Arc::new(StubExternalRuntime::new( self.name, @@ -1333,3 +1349,243 @@ fn stale_credential_route_fallback_follows_runtime_pin_or_drops_to_auto() { assert!(stale_credential_route_fallback("openrouter:z-ai/glm-5.2@Novita").is_none()); }); } + +/// A runtime image-input rejection must flip `supports_image_input` to false +/// even when the underlying runtime advertises image support, and a model +/// switch must clear the override. +#[test] +fn recorded_rejection_overrides_supports_image_input() { + use jcode_provider_core::provider_key; + + with_clean_provider_test_env(|| { + let runtime = enter_test_runtime(); + let _enter = runtime.enter(); + + // A real-shaped anthropic runtime so a model switch can succeed and + // exercise the clear-on-success path, and a failure can be provoked + // with a model the runtime rejects. It advertises image support so a + // recorded rejection flipping the answer to false is observable. + let anthropic_stub = test_anthropic_runtime().with_image_support(); + let provider = MultiProvider { + anthropic: RwLock::new(Some(anthropic_stub.clone() as Arc)), + openai: RwLock::new(None), + copilot_api: RwLock::new(None), + antigravity: RwLock::new(None), + gemini: RwLock::new(None), + cursor: RwLock::new(None), + bedrock: RwLock::new(None), + openrouter: RwLock::new(None), + catalog_scope: next_catalog_scope(), + openai_compatible_profiles: RwLock::new(std::collections::HashMap::new()), + active_openai_compatible_profile: RwLock::new(None), + session_id: RwLock::new(None), + active: RwLock::new(ActiveProvider::Claude), + startup_notices: RwLock::new(Vec::new()), + initial_provider: Some(ActiveProvider::Claude), + routes_memo: std::sync::Mutex::new(None), + post_auth_refreshes_pending: Arc::new(std::sync::atomic::AtomicUsize::new(0)), + }; + provider.set_active_provider(ActiveProvider::Claude); + + let key = provider_key(ActiveProvider::Claude); + let model = "claude-opus-4-5"; + // The rejection lookup is scoped to the runtime's CURRENT model, so + // make the stub actually serve the model the test records against + // (its constructor defaults to AVAILABLE_MODELS[0]). + anthropic_stub + .set_model(model) + .expect("stub accepts its own catalog models"); + super::image_capability::clear_image_input_overrides_for_provider(key); + + // No rejection recorded: MultiProvider falls through to the runtime's + // static answer (stub opted into image support -> true). + assert!(provider.supports_image_input()); + + // Record a rejection for the exact (provider, model) pair; the + // recorded rejection must override the runtime's static answer. + super::image_capability::record_image_input_rejection( + Some(key), + model, + "model does not support image input", + ); + assert!(super::image_capability::image_input_rejected(key, model)); + assert!( + !provider.supports_image_input(), + "a recorded rejection must override the runtime's static image support" + ); + + // A FAILED switch must keep the override: the previous model stays + // active, and wiping its capability restriction would let the very + // next request ship pixels to the endpoint that just rejected them. + let switch_result = provider.set_model("not-a-real-model"); + assert!( + switch_result.is_err(), + "stub runtime must reject the unknown model so the switch fails" + ); + assert!( + !provider.supports_image_input(), + "a failed model switch must not clear the runtime image-input rejection" + ); + assert!( + super::image_capability::image_input_rejected(key, model), + "rejection record must survive a failed model switch" + ); + + // A model switch that SUCCEEDS clears the override so the new model + // gets a fresh capability answer. + provider + .set_model("claude-opus-5") + .expect("stub accepts its own model"); + assert!( + !super::image_capability::image_input_rejected(key, model), + "a successful model switch must clear the provider's image-input overrides" + ); + assert!( + provider.supports_image_input(), + "after the clear the runtime's static answer (true) applies again" + ); + }); +} + +/// A runtime whose first completion fails with an image-modality rejection and +/// whose subsequent completions succeed, so the retry path can be observed. +struct ImageRejectingRuntime { + calls: std::sync::atomic::AtomicUsize, +} + +#[async_trait::async_trait] +impl Provider for ImageRejectingRuntime { + async fn complete( + &self, + _messages: &[Message], + _tools: &[ToolDefinition], + _system: &str, + _resume_session_id: Option<&str>, + ) -> Result { + let calls = self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + if calls == 0 { + // Exact live shape observed from a corporate OpenAI-compatible + // gateway (HTTP 400, code `multimodal_processing_failed`), so this + // integration test exercises the same classification the live + // endpoint path takes, not only the older Anthropic-style text. + anyhow::bail!( + "Multimodal processing failed: Multimodal not supported for model: glm-5-2-nvfp4" + ); + } + let (tx, rx) = tokio::sync::mpsc::channel(1); + let _ = tx.try_send(Ok(crate::message::StreamEvent::TextDelta("ok".to_string()))); + Ok(Box::pin(futures::stream::unfold(rx, |mut rx| async move { + rx.recv().await.map(|event| (event, rx)) + }))) + } + fn name(&self) -> &'static str { + "image-rejecting-stub" + } + fn model(&self) -> String { + "stub-model".to_string() + } + // Mirror the real bug scenario: optimistic direct OpenAI-compatible + // endpoints advertise image support (trait default in their runtimes) + // while the model behind them is text-only. Without this the entry-time + // filter strips the image before attempt 1 and the retry path never runs. + fn supports_image_input(&self) -> bool { + true + } + fn fork(&self) -> Arc { + Arc::new(ImageRejectingRuntime { + calls: std::sync::atomic::AtomicUsize::new(0), + }) + } +} + +/// End-to-end: a request carrying an image block that hits an image-modality +/// rejection is replayed once with the images filtered to text markers, on the +/// same provider, without provider failover. +#[test] +fn image_modality_rejection_is_retried_with_filtered_messages() { + use jcode_message_types::{ContentBlock, Role}; + + with_clean_provider_test_env(|| { + let runtime = enter_test_runtime(); + let _enter = runtime.enter(); + + let stub = Arc::new(ImageRejectingRuntime { + calls: std::sync::atomic::AtomicUsize::new(0), + }); + let provider = MultiProvider { + anthropic: RwLock::new(None), + openai: RwLock::new(None), + copilot_api: RwLock::new(None), + antigravity: RwLock::new(None), + gemini: RwLock::new(None), + cursor: RwLock::new(None), + bedrock: RwLock::new(None), + openrouter: RwLock::new(None), + catalog_scope: next_catalog_scope(), + openai_compatible_profiles: RwLock::new(std::collections::HashMap::new()), + active_openai_compatible_profile: RwLock::new(None), + session_id: RwLock::new(None), + active: RwLock::new(ActiveProvider::Claude), + startup_notices: RwLock::new(Vec::new()), + initial_provider: Some(ActiveProvider::Claude), + routes_memo: std::sync::Mutex::new(None), + post_auth_refreshes_pending: Arc::new(std::sync::atomic::AtomicUsize::new(0)), + }; + *provider + .anthropic + .write() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = + Some(stub.clone() as Arc); + super::image_capability::clear_image_input_overrides_for_provider( + jcode_provider_core::provider_key(ActiveProvider::Claude), + ); + + let messages = vec![Message { + role: Role::User, + content: vec![ + ContentBlock::Text { + text: "what is in this picture?".to_string(), + cache_control: None, + }, + ContentBlock::Image { + media_type: "image/png".to_string(), + // A real 1x1 PNG: `clamp_outbound_images` replaces + // undecodable payloads with text markers before the + // request goes out, and the modality retry below only + // fires when the first attempt still carried the image. + data: "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==" + .to_string(), + }, + ], + timestamp: None, + tool_duration_ms: None, + }]; + + let stream = futures::executor::block_on(provider.complete(&messages, &[], "system", None)) + .expect("retry with filtered images must succeed"); + let text = futures::executor::block_on(async { + use futures::StreamExt; + let mut stream = stream; + let mut text = String::new(); + while let Some(event) = stream.next().await { + if let Ok(crate::message::StreamEvent::TextDelta(delta)) = event { + text.push_str(&delta); + } + } + text + }); + assert_eq!(text, "ok"); + assert_eq!( + stub.calls.load(std::sync::atomic::Ordering::SeqCst), + 2, + "first call rejects, filtered retry succeeds on the same provider" + ); + assert!( + super::image_capability::image_input_rejected( + jcode_provider_core::provider_key(ActiveProvider::Claude), + "stub-model" + ), + "rejection must be recorded so subsequent requests are pre-filtered" + ); + }); +} diff --git a/crates/jcode-base/src/provider_activity.rs b/crates/jcode-base/src/provider_activity.rs index c2cf90f42b..369f69fc65 100644 --- a/crates/jcode-base/src/provider_activity.rs +++ b/crates/jcode-base/src/provider_activity.rs @@ -392,7 +392,7 @@ mod tests { use super::*; use std::ffi::OsString; - fn lock_env() -> std::sync::MutexGuard<'static, ()> { + fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-base/src/session/prewarm.rs b/crates/jcode-base/src/session/prewarm.rs index 86df15e7ea..5cbbd798fc 100644 --- a/crates/jcode-base/src/session/prewarm.rs +++ b/crates/jcode-base/src/session/prewarm.rs @@ -281,7 +281,7 @@ mod tests { struct Home { _dir: tempfile::TempDir, prev: Option, - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, } impl Home { diff --git a/crates/jcode-base/src/session_tests/mod.rs b/crates/jcode-base/src/session_tests/mod.rs index 4829e4faff..f11d0d46bb 100644 --- a/crates/jcode-base/src/session_tests/mod.rs +++ b/crates/jcode-base/src/session_tests/mod.rs @@ -1,7 +1,7 @@ use super::*; use std::ffi::OsString; -fn lock_env() -> std::sync::MutexGuard<'static, ()> { +fn lock_env() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-base/src/sponsors/provenance.rs b/crates/jcode-base/src/sponsors/provenance.rs index 9f05795366..bddbab6592 100644 --- a/crates/jcode-base/src/sponsors/provenance.rs +++ b/crates/jcode-base/src/sponsors/provenance.rs @@ -224,7 +224,7 @@ pub(crate) fn drain_pending_for_tests() -> Vec { mod tests { use super::*; - fn enable_sponsors() -> (std::sync::MutexGuard<'static, ()>, tempfile::TempDir) { + fn enable_sponsors() -> (crate::storage::TestEnvGuard, tempfile::TempDir) { let guard = crate::storage::lock_test_env(); let temp = tempfile::tempdir().unwrap(); crate::env::set_var("JCODE_HOME", temp.path()); diff --git a/crates/jcode-base/src/storage.rs b/crates/jcode-base/src/storage.rs index 0633017768..5d6aa54761 100644 --- a/crates/jcode-base/src/storage.rs +++ b/crates/jcode-base/src/storage.rs @@ -31,10 +31,38 @@ pub fn test_env_lock() -> &'static Mutex<()> { } #[cfg(any(test, feature = "test-support"))] -pub fn lock_test_env() -> MutexGuard<'static, ()> { - test_env_lock() +thread_local! { + /// Whether this thread holds the env lock, so nested `lock_test_env` + /// calls on the same thread no-op instead of self-deadlocking. + static TEST_ENV_LOCK_HELD: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + +/// Reentrant guard for the shared test env lock. +/// +/// Same-thread nesting returns a no-op guard (`std::sync::Mutex` is not +/// reentrant); cross-thread callers still serialize on the same mutex. +#[cfg(any(test, feature = "test-support"))] +pub struct TestEnvGuard(Option>); + +#[cfg(any(test, feature = "test-support"))] +impl Drop for TestEnvGuard { + fn drop(&mut self) { + if self.0.take().is_some() { + TEST_ENV_LOCK_HELD.with(|held| held.set(false)); + } + } +} + +#[cfg(any(test, feature = "test-support"))] +pub fn lock_test_env() -> TestEnvGuard { + if TEST_ENV_LOCK_HELD.with(|held| held.get()) { + return TestEnvGuard(None); + } + let guard = test_env_lock() .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) + .unwrap_or_else(|poisoned| poisoned.into_inner()); + TEST_ENV_LOCK_HELD.with(|held| held.set(true)); + TestEnvGuard(Some(guard)) } #[cfg(test)] diff --git a/crates/jcode-base/src/storage/tests.rs b/crates/jcode-base/src/storage/tests.rs index 199ca758fe..a7031347ad 100644 --- a/crates/jcode-base/src/storage/tests.rs +++ b/crates/jcode-base/src/storage/tests.rs @@ -34,6 +34,24 @@ fn harden_secret_file_permissions_sets_owner_only_modes() { assert_eq!(file_mode, 0o600); } +#[test] +fn lock_test_env_reentrant_on_same_thread_and_serialized_across_threads() { + // Same-thread nesting must no-op instead of self-deadlocking. + let outer = lock_test_env(); + let nested = lock_test_env(); + drop(nested); + drop(outer); + + // Cross-thread acquisition still serializes: a second thread must not + // observe the lock as free while this thread holds it. + let held = lock_test_env(); + let second = std::thread::spawn(|| test_env_lock().try_lock().is_err()) + .join() + .expect("probe thread"); + assert!(second, "env lock must exclude other threads while held"); + drop(held); +} + #[test] fn user_home_path_uses_external_dir_under_jcode_home() { let _guard = lock_test_env(); diff --git a/crates/jcode-provider-core/src/image_capability.rs b/crates/jcode-provider-core/src/image_capability.rs new file mode 100644 index 0000000000..3d0b2aaa58 --- /dev/null +++ b/crates/jcode-provider-core/src/image_capability.rs @@ -0,0 +1,575 @@ +//! Runtime image-input capability overrides. +//! +//! Static capability knowledge (`Provider::supports_image_input`) can be wrong +//! for direct OpenAI-compatible endpoints: the default there is optimistic +//! (any `/v1/chat/completions` server "might" accept `image_url` parts), and a +//! text-only model behind such an endpoint advertises image support it does not +//! have. When that happens the request fails with a modality error only after +//! the image is already in the request, and every later request repeats the +//! failure because the stale `true` answer keeps the images flowing. +//! +//! This module records per `(runtime-scoped provider key, model)` that the +//! endpoint *actually* rejected image input, with a TTL. The key is the +//! concrete runtime identity (e.g. `openrouter::profile:`) where the +//! provider slot multiplexes several endpoints, so a rejection on one endpoint +//! never suppresses images for another endpoint serving the same model name. +//! `MultiProvider::supports_image_input` consults the override first, so: +//! +//! - `complete_with_failover` replaces image blocks with text markers for the +//! very next request (the existing #755 filter), and +//! - turn loops stop attaching new tool-result/generated images while the +//! override is fresh. +//! +//! The override is scoped to the model: switching to a vision-capable model on +//! the same endpoint clears the penalty (and a manual `/model` switch clears +//! every entry for the runtime anyway via `clear_image_input_overrides_for_provider`). +//! +//! Unlike `ACCOUNT_RUNTIME_UNAVAILABLE_PROVIDERS`, these entries never steer +//! provider failover: the endpoint is healthy, only the image modality is +//! unsupported, so we degrade the request instead of switching providers. +//! +//! The OpenAI-compatible runtime surfaces HTTP errors asynchronously inside the +//! response stream (the stream handle is returned before the request is sent), +//! so the streaming retry loop also consults this module: when a request that +//! carried `image_url` parts fails with a modality error, the retry loop can +//! rewrite the request JSON (`filter_image_url_parts_from_request`) and replay +//! the same request once instead of failing the turn. + +use std::collections::HashMap; +use std::sync::{LazyLock, RwLock}; +use std::time::{Duration, Instant, SystemTime}; + +/// How long a recorded "this model rejected images" observation stays +/// authoritative. Long enough to cover a long session, short enough that a +/// server-side upgrade (or a wrongly classified error) heals itself. +const IMAGE_INPUT_OVERRIDE_TTL: Duration = Duration::from_secs(30 * 60); + +#[derive(Debug, Clone)] +struct ImageInputRejection { + recorded_at: Instant, + #[allow(dead_code)] // kept for parity with other runtime records; useful in future dumps + observed_at: SystemTime, + #[allow(dead_code)] // debug aid when inspecting why a request was filtered + matched: String, +} + +static IMAGE_INPUT_REJECTIONS: LazyLock>> = + LazyLock::new(|| RwLock::new(HashMap::new())); + +/// Error fragments that mean "this model/endpoint cannot accept image input". +/// +/// Kept deliberately narrow: generic schema errors (e.g. a missing tool field) +/// must not be treated as a modality problem. All needles are matched +/// case-insensitively against the full error text. +const IMAGE_REJECTION_NEEDLES: &[&str] = &[ + // Anthropic-style + "does not support image", + "images are not supported", + "image input is not supported", + "unsupported content type: image", + "does not accept image", + "cannot process images", + "no image support", + // OpenAI / OpenAI-compatible style + "does not support image input", + "image input not supported", + "not support image_url", + "invalid content type: image_url", + "unsupported content type: image_url", + "image_url is not supported", + "image content is not supported", + "not multimodal", + "is not multimodal", + "does not support multimodal", + // Gateway-style (observed live on a corporate OpenAI-compatible gateway: + // `multimodal_processing_failed` / "Multimodal not supported for model: ..."). + // The word-boundary shapes are kept distinct from `modality not supported` + // so generic modality errors on other endpoints still need their own needle. + // The bare `multimodal processing failed` code is deliberately NOT a needle: + // vision-capable gateways reuse it for per-image failures (undecodable or + // over-limit pixels), so only an explicit capability statement such as + // "multimodal not supported" may disable image input for the model. + "multimodal not supported", + "multimodal is not supported", + "text-only model", + "only supports text", + "only supports text input", + "image is not a supported modality", + "modality not supported", + "unsupported modality", + "input modalities", +]; + +/// True when `error_text` looks like the endpoint rejected image input. +pub fn error_indicates_image_rejection(error_text: &str) -> bool { + let lower = error_text.to_ascii_lowercase(); + IMAGE_REJECTION_NEEDLES + .iter() + .any(|needle| lower.contains(needle)) +} + +/// Record that `model` on `provider_key` rejected image input. +/// +/// `provider_key` must already be runtime-scoped, because the same provider +/// slot can serve different accounts with different capabilities: pass the +/// concrete profile/endpoint identity (e.g. `openrouter::profile:` or +/// `openrouter::endpoint:`), not the bare slot key, when the caller knows +/// it. Two OpenAI-compatible profiles sharing a model name must not share a +/// rejection record: a text-only endpoint's rejection would otherwise make a +/// vision-capable endpoint omit valid images for the TTL window. Pass `None` +/// only when the caller genuinely cannot determine a concrete endpoint +/// identity; the record is then kept under the un-scoped key so lookups that +/// use the same key shape still find it. +pub fn record_image_input_rejection(provider_key: Option<&str>, model: &str, error_text: &str) { + // Trim whitespace but preserve the exact model id. Direct endpoints can + // serve distinct case-sensitive ids, so `model-a` must not disable images + // for a separate vision-capable `Model-A`. + let model = model.trim(); + if model.is_empty() { + return; + } + let Some(provider_key) = provider_key.map(str::trim).filter(|k| !k.is_empty()) else { + return; + }; + let matched = IMAGE_REJECTION_NEEDLES + .iter() + .find(|needle| error_text.to_ascii_lowercase().contains(*needle)) + .map(|needle| needle.to_string()) + .unwrap_or_else(|| "unclassified".to_string()); + let entry_key = format!("{provider_key}::{model}"); + if let Ok(mut rejections) = IMAGE_INPUT_REJECTIONS.write() { + rejections.insert( + entry_key, + ImageInputRejection { + recorded_at: Instant::now(), + observed_at: SystemTime::now(), + matched: matched.clone(), + }, + ); + } + jcode_logging::info(&format!( + "Recorded image-input rejection for {provider_key}/{model} (matched: {}). \ + Outbound images will be filtered to text markers until the model changes.", + matched + )); +} + +/// True when a fresh "rejected image input" record exists for `model` on +/// `provider_key`. Expired entries are dropped on read. +pub fn image_input_rejected(provider_key: &str, model: &str) -> bool { + let model = model.trim(); + if model.is_empty() || provider_key.trim().is_empty() { + return false; + } + let entry_key = format!("{provider_key}::{model}"); + let Ok(mut rejections) = IMAGE_INPUT_REJECTIONS.write() else { + return false; + }; + match rejections.get(&entry_key) { + Some(entry) if entry.recorded_at.elapsed() <= IMAGE_INPUT_OVERRIDE_TTL => true, + Some(_) => { + rejections.remove(&entry_key); + false + } + None => false, + } +} + +/// Drop every override for `provider_key` (called when the model is switched +/// on that runtime, so a fresh model gets a fresh capability answer). +pub fn clear_image_input_overrides_for_provider(provider_key: &str) { + let prefix = format!("{provider_key}::"); + // Collect first, then log and mutate outside the map so the shared lock is + // never held across a log write (a slow logger would block other sessions' + // image-capability checks). + let keys: Vec = match IMAGE_INPUT_REJECTIONS.read() { + Ok(rejections) => rejections + .keys() + .filter(|key| key.starts_with(&prefix)) + .cloned() + .collect(), + Err(_) => return, + }; + if keys.is_empty() { + return; + } + jcode_logging::info(&format!( + "Cleared image-input rejection overrides for {provider_key} ({} record(s): {})", + keys.len(), + keys.join(", ") + )); + if let Ok(mut rejections) = IMAGE_INPUT_REJECTIONS.write() { + // Remove only the exact keys observed above. A blanket prefix retain + // would also drop a rejection recorded by a concurrent image-bearing + // request between the read and this write, sending images back to a + // model that just rejected them. + for key in keys { + rejections.remove(&key); + } + } +} + +/// True when a serialized request body carries image content parts: the +/// chat-completions wire shape (`messages[].content[].type == "image_url"`) +/// or the Responses wire shape (`input[].content[].type == "input_image"`, +/// used by the Azure Responses route and the opencode-go Responses route). +pub fn request_contains_image_url_parts(request: &serde_json::Value) -> bool { + request + .get("messages") + .and_then(|m| m.as_array()) + .map(|messages| { + messages.iter().any(|message| { + message + .get("content") + .and_then(|c| c.as_array()) + .map(|parts| { + parts.iter().any(|part| { + part.get("type").and_then(|t| t.as_str()) == Some("image_url") + }) + }) + .unwrap_or(false) + }) + }) + .unwrap_or(false) + || request + .get("input") + .and_then(|i| i.as_array()) + .map(|items| { + items.iter().any(|item| { + item.get("content") + .and_then(|c| c.as_array()) + .map(|parts| { + parts.iter().any(|part| { + part.get("type").and_then(|t| t.as_str()) == Some("input_image") + }) + }) + .unwrap_or(false) + }) + }) + .unwrap_or(false) +} + +/// Replace every image content part in a serialized request body with a text +/// marker describing the omitted image. Handles both wire shapes: +/// chat-completions `messages[].content[].type == "image_url"` (nested +/// `image_url.url` data URL) and Responses `input[].content[].type == +/// "input_image"` (plain-string `image_url` data URL). Returns a new request +/// value; the input is untouched. Non-array message content (plain strings) +/// carries no images, so it is preserved as-is. +pub fn filter_image_url_parts_from_request(request: &serde_json::Value) -> serde_json::Value { + let mut filtered = request.clone(); + // Chat-completions shape. + if let Some(messages) = filtered.get_mut("messages").and_then(|m| m.as_array_mut()) { + for message in messages.iter_mut() { + let Some(parts) = message.get_mut("content").and_then(|c| c.as_array_mut()) else { + continue; + }; + for part in parts.iter_mut() { + if part.get("type").and_then(|t| t.as_str()) != Some("image_url") { + continue; + } + // Keep the media type in the marker so the model still knows what + // kind of image was omitted; the pixel data itself is dropped. + let media_type = part + .get("image_url") + .and_then(|iu| iu.get("url")) + .and_then(|u| u.as_str()) + .and_then(|url| url.strip_prefix("data:")) + .and_then(|rest| rest.split(';').next()) + .unwrap_or("unknown"); + *part = serde_json::json!({ + "type": "text", + "text": format!( + "[Image omitted: this model does not support image input; media_type={media_type}]" + ) + }); + } + } + } + // Responses shape (Azure Responses route, opencode-go Responses route): + // `input[].content[].type == "input_image"` with a plain-string + // `image_url` data URL. The replacement keeps the Responses content-part + // vocabulary (`input_text`) so the endpoint accepts the rewritten item. + if let Some(items) = filtered.get_mut("input").and_then(|i| i.as_array_mut()) { + for item in items.iter_mut() { + let Some(parts) = item.get_mut("content").and_then(|c| c.as_array_mut()) else { + continue; + }; + for part in parts.iter_mut() { + if part.get("type").and_then(|t| t.as_str()) != Some("input_image") { + continue; + } + let media_type = part + .get("image_url") + .and_then(|u| u.as_str()) + .and_then(|url| url.strip_prefix("data:")) + .and_then(|rest| rest.split(';').next()) + .unwrap_or("unknown"); + *part = serde_json::json!({ + "type": "input_text", + "text": format!( + "[Image omitted: this model does not support image input; media_type={media_type}]" + ) + }); + } + } + } + filtered +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn unique_provider() -> String { + format!( + "test-provider-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + ) + } + + #[test] + fn clear_preserves_rejections_recorded_concurrently() { + // A model switch collects the keys under the read lock, then logs, + // then takes the write lock. If an image-bearing request records a + // rejection in that window, the clear must not drop it: the blanket + // prefix retain used to delete it, sending images back to a model that + // just rejected them. + let provider = unique_provider(); + record_image_input_rejection( + Some(&provider), + "old-model", + "model does not support image input", + ); + + // Simulate the race window: snapshot what the clear would collect, + // have the concurrent request land, then run the clear's removal. + let prefix = format!("{provider}::"); + let keys: Vec = IMAGE_INPUT_REJECTIONS + .read() + .unwrap() + .keys() + .filter(|key| key.starts_with(&prefix)) + .cloned() + .collect(); + record_image_input_rejection( + Some(&provider), + "new-model", + "model does not support image input", + ); + if let Ok(mut rejections) = IMAGE_INPUT_REJECTIONS.write() { + for key in &keys { + rejections.remove(key); + } + } + + // The pre-switch record is gone, the concurrent record survives. + assert!(!image_input_rejected(&provider, "old-model")); + assert!(image_input_rejected(&provider, "new-model")); + clear_image_input_overrides_for_provider(&provider); + assert!(!image_input_rejected(&provider, "new-model")); + } + + #[test] + fn modality_errors_are_classified() { + assert!(error_indicates_image_rejection( + "Error: model does not support image input" + )); + assert!(error_indicates_image_rejection( + "400: this is a text-only model" + )); + assert!(error_indicates_image_rejection( + "Request failed: image_url is not supported by this model" + )); + assert!(error_indicates_image_rejection( + "Invalid request: unsupported modality: image" + )); + assert!(error_indicates_image_rejection( + "model is not multimodal, cannot accept image parts" + )); + assert!(error_indicates_image_rejection( + "model does not support multimodal requests" + )); + // Live shape observed from a corporate OpenAI-compatible gateway: + // HTTP 400, code `multimodal_processing_failed`, message + // "Multimodal processing failed: Multimodal not supported for model: ...". + // Must classify, otherwise the self-heal replay never fires there. + assert!(error_indicates_image_rejection( + "Multimodal processing failed: Multimodal not supported for model: revolut-ltd/glm-5-2-nvfp4" + )); + assert!(error_indicates_image_rejection( + "status: 400 Bad Request\n response: {\"error\":{\"type\":\"Bad Request\",\"code\":\"multimodal_processing_failed\",\"message\":\"Multimodal not supported for model: x\"}}" + )); + // A bare `multimodal_processing_failed` without a capability statement + // means THIS image could not be processed (decode failure, size limit), + // not that the model rejects images: a vision-capable gateway sends it + // for a single bad pixel blob, and suppressing images for the whole + // TTL window would drop every later valid image. + assert!(!error_indicates_image_rejection( + "Multimodal processing failed" + )); + assert!(!error_indicates_image_rejection( + "status: 400 Bad Request\n response: {\"error\":{\"type\":\"Bad Request\",\"code\":\"multimodal_processing_failed\",\"message\":\"Multimodal processing failed\"}}" + )); + } + + #[test] + fn unrelated_errors_are_not_classified() { + assert!(!error_indicates_image_rejection("rate limit exceeded")); + assert!(!error_indicates_image_rejection( + "tool_use ids were found without tool_result" + )); + assert!(!error_indicates_image_rejection( + "max_tokens must be greater than 0" + )); + // Positive-sounding multimodal text must not flip a request to the + // text-only path. + assert!(!error_indicates_image_rejection( + "gpt-4o is multimodal and can see images" + )); + assert!(!error_indicates_image_rejection("")); + } + + #[test] + fn rejection_roundtrip_and_clear() { + let provider = unique_provider(); + assert!(!image_input_rejected(&provider, "text-model")); + + record_image_input_rejection( + Some(&provider), + "text-model", + "does not support image input", + ); + assert!(image_input_rejected(&provider, "text-model")); + // Scoped per model. + assert!(!image_input_rejected(&provider, "other-model")); + + // Whitespace is trimmed, but model ids stay case-sensitive. + record_image_input_rejection( + Some(&provider), + " model-a ", + "does not support image input", + ); + assert!(image_input_rejected(&provider, "model-a")); + assert!(!image_input_rejected(&provider, "Model-A")); + + clear_image_input_overrides_for_provider(&provider); + assert!(!image_input_rejected(&provider, "text-model")); + assert!(!image_input_rejected(&provider, "model-a")); + } + + #[test] + fn empty_inputs_are_ignored() { + let provider = unique_provider(); + record_image_input_rejection(Some(&provider), " ", "does not support image input"); + record_image_input_rejection(None, "model", "does not support image input"); + record_image_input_rejection(Some(""), "model", "does not support image input"); + assert!(!image_input_rejected(&provider, " ")); + assert!(!image_input_rejected("", "model")); + } + + #[test] + fn request_image_url_detection_and_filtering() { + let request = json!({ + "model": "text-only-test-model", + "messages": [ + {"role": "system", "content": "you are helpful"}, + {"role": "user", "content": [ + {"type": "text", "text": "look at this"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,Zm9v"}} + ]}, + {"role": "tool", "tool_call_id": "c1", "content": "plain string content"} + ] + }); + assert!(request_contains_image_url_parts(&request)); + + let filtered = filter_image_url_parts_from_request(&request); + assert!(!request_contains_image_url_parts(&filtered)); + // Text parts preserved. + let user_parts = filtered["messages"][1]["content"].as_array().unwrap(); + assert_eq!(user_parts.len(), 2); + assert_eq!(user_parts[0]["type"], "text"); + assert_eq!(user_parts[0]["text"], "look at this"); + assert_eq!(user_parts[1]["type"], "text"); + let marker = user_parts[1]["text"].as_str().unwrap(); + assert!(marker.contains("[Image omitted:")); + assert!(marker.contains("media_type=image/png")); + // String-content messages untouched. + assert_eq!( + filtered["messages"][2]["content"], + json!("plain string content") + ); + // Original request not mutated. + assert!(request_contains_image_url_parts(&request)); + } + + #[test] + fn request_without_images_is_untouched() { + let request = json!({ + "model": "text-only-test-model", + "messages": [ + {"role": "user", "content": [{"type": "text", "text": "hello"}]}, + {"role": "user", "content": "plain string"} + ] + }); + assert!(!request_contains_image_url_parts(&request)); + let filtered = filter_image_url_parts_from_request(&request); + assert_eq!(filtered, request); + } + + #[test] + fn responses_input_image_detection_and_filtering() { + // Azure Responses / opencode-go Responses wire shape: images are + // `input[].content[].type == "input_image"` with a plain-string + // `image_url` data URL, not the chat-completions `image_url` object. + let request = json!({ + "model": "text-only-test-model", + "input": [ + {"type": "message", "role": "user", "content": [ + {"type": "input_text", "text": "look at this"}, + {"type": "input_image", "image_url": "data:image/jpeg;base64,Zm9v"} + ]}, + {"type": "function_call_output", "call_id": "c1", "output": "plain output"} + ], + "stream": true + }); + assert!(request_contains_image_url_parts(&request)); + + let filtered = filter_image_url_parts_from_request(&request); + assert!(!request_contains_image_url_parts(&filtered)); + // The rewritten part keeps the Responses vocabulary. + let parts = filtered["input"][0]["content"].as_array().unwrap(); + assert_eq!(parts.len(), 2); + assert_eq!(parts[0]["type"], "input_text"); + assert_eq!(parts[0]["text"], "look at this"); + assert_eq!(parts[1]["type"], "input_text"); + let marker = parts[1]["text"].as_str().unwrap(); + assert!(marker.contains("[Image omitted:")); + assert!(marker.contains("media_type=image/jpeg")); + // Non-message items untouched. + assert_eq!(filtered["input"][1]["type"], "function_call_output"); + assert_eq!(filtered["input"][1]["output"], "plain output"); + // Original request not mutated. + assert!(request_contains_image_url_parts(&request)); + } + + #[test] + fn responses_request_without_images_is_untouched() { + let request = json!({ + "model": "text-only-test-model", + "input": [ + {"type": "message", "role": "user", "content": [ + {"type": "input_text", "text": "hello"} + ]} + ] + }); + assert!(!request_contains_image_url_parts(&request)); + let filtered = filter_image_url_parts_from_request(&request); + assert_eq!(filtered, request); + } +} diff --git a/crates/jcode-provider-core/src/lib.rs b/crates/jcode-provider-core/src/lib.rs index 78638d3844..127e4dfec2 100644 --- a/crates/jcode-provider-core/src/lib.rs +++ b/crates/jcode-provider-core/src/lib.rs @@ -5,6 +5,7 @@ pub mod catalog_refresh; pub mod failover; pub mod fallback_pick; pub mod fingerprint; +pub mod image_capability; pub mod model_id; pub mod model_names; pub mod models; @@ -151,6 +152,18 @@ pub trait Provider: Send + Sync { "unknown".to_string() } + /// Stable identity of the concrete endpoint/profile this runtime instance + /// serves, used to scope runtime capability records (image-input rejections) + /// so two profiles that share the `name()` slot do not share capability + /// state. `None` falls back to `name()`. + /// + /// The value must be stable for the lifetime of the runtime instance and + /// identical across all writers (record + lookup paths); the OpenRouter + /// slot returns its profile id or api base. + fn capability_scope(&self) -> Option { + None + } + /// Human-readable description of the auth method the active provider will /// actually use for the next request (e.g. "OAuth" or "API key"), or `None` /// when there is no meaningful OAuth-vs-API-key distinction. UI surfaces use diff --git a/crates/jcode-provider-openai-runtime/src/openai_tests.rs b/crates/jcode-provider-openai-runtime/src/openai_tests.rs index 552d6523cd..1110e9bd53 100644 --- a/crates/jcode-provider-openai-runtime/src/openai_tests.rs +++ b/crates/jcode-provider-openai-runtime/src/openai_tests.rs @@ -8,7 +8,6 @@ use jcode_message_types::{ContentBlock, Role}; use std::collections::{HashMap, HashSet}; use std::ffi::OsString; use std::path::PathBuf; -use std::sync::MutexGuard; use std::time::{Duration, Instant}; const BRIGHT_PEARL_WRAPPED_TOOL_CALL_FIXTURE: &str = include_str!(concat!( env!("CARGO_MANIFEST_DIR"), @@ -164,7 +163,7 @@ async fn test_persistent_ws_state_with_ping_notify() -> ( } struct LiveOpenAITestEnv { - _lock: MutexGuard<'static, ()>, + _lock: jcode_base::storage::TestEnvGuard, _jcode_home: EnvVarGuard, _transport: EnvVarGuard, _temp: tempfile::TempDir, diff --git a/crates/jcode-provider-openrouter-runtime/src/azure_retry_tests.rs b/crates/jcode-provider-openrouter-runtime/src/azure_retry_tests.rs index d30226b355..db14c14e7c 100644 --- a/crates/jcode-provider-openrouter-runtime/src/azure_retry_tests.rs +++ b/crates/jcode-provider-openrouter-runtime/src/azure_retry_tests.rs @@ -115,6 +115,7 @@ fn run_azure(base: String) -> Vec> { Arc::new(Mutex::new(None)), "deployment".into(), false, + "openrouter::test-scope".to_string(), ), ) .await diff --git a/crates/jcode-provider-openrouter-runtime/src/openrouter_provider_impl.rs b/crates/jcode-provider-openrouter-runtime/src/openrouter_provider_impl.rs index e152c4a870..f80773a612 100644 --- a/crates/jcode-provider-openrouter-runtime/src/openrouter_provider_impl.rs +++ b/crates/jcode-provider-openrouter-runtime/src/openrouter_provider_impl.rs @@ -304,8 +304,20 @@ impl Provider for OpenRouterProvider { let send_openrouter_headers = self.send_openrouter_headers; let conversation_id = self.conversation_id.clone(); let request_for_retries = request; - let model_for_stream = model.clone(); + // The stream records image-modality rejections under this model + // string, and its consumers (runtime `supports_image_input` and the + // `OpenRouterStream` provider-pin matching) compare against the bare + // model id that `set_model` normalizes into state. The raw state + // string can transiently carry a session-routing + // `:` prefix (right after session restore, before + // `set_model` normalizes it, see #403); recording under that spelling + // would never match the stripped lookups, so hand the stream the + // same stripped form the lookups use. + let model_for_stream = self.strip_session_profile_prefix(&model).to_string(); let provider_pin = Arc::clone(&self.provider_pin); + // Same capability scope `supports_image_input` looks up under: the + // stream records rejections under the identical key the lookup uses. + let capability_scope_key = self.capability_scope_key(); tokio::spawn(async move { if tx @@ -329,6 +341,7 @@ impl Provider for OpenRouterProvider { provider_pin, model_for_stream, responses_api, + capability_scope_key, ) .await; }); @@ -340,6 +353,10 @@ impl Provider for OpenRouterProvider { "openrouter" } + fn capability_scope(&self) -> Option { + Some(self.capability_scope_key()) + } + fn display_name(&self) -> String { self.runtime_display_name() } @@ -357,6 +374,20 @@ impl Provider for OpenRouterProvider { .strip_session_profile_prefix(&raw_model) .trim() .to_ascii_lowercase(); + // A recorded runtime rejection (this endpoint/model answered "no + // image input" earlier, see `jcode_provider_core::image_capability`) + // is authoritative over the optimistic direct-endpoint default below, + // so follow-up requests filter images at build time. The lookup key is + // the same capability scope the streaming retry loop records under + // (profile id or endpoint base), so a rejection on one OpenAI-compatible + // profile never suppresses images for another profile serving the same + // model name. + if jcode_provider_core::image_capability::image_input_rejected( + &self.capability_scope_key(), + &model_id, + ) { + return false; + } if let Some(supports_images) = self.static_image_input_support.get(&model_id) { return *supports_images; } @@ -937,6 +968,7 @@ impl OpenRouterProvider { let api_base = self.api_base.clone(); let conversation_id = self.conversation_id.clone(); let provider_pin = Arc::clone(&self.provider_pin); + let capability_scope_key = self.capability_scope_key(); tokio::spawn(async move { if tx .send(Ok(StreamEvent::ConnectionType { @@ -959,12 +991,87 @@ impl OpenRouterProvider { provider_pin, model, false, + capability_scope_key, ) .await; }); Ok(Box::pin(ReceiverStream::new(rx))) } + /// Normalize an api base for the capability scope key: scheme and host + /// are case-insensitive URL parts and get lowercased, while the path is + /// case-sensitive (RFC 3986) and keeps its exact spelling so distinct + /// gateway routes (`/Text/v1` vs `/text/v1`) stay separate scopes. + fn normalized_endpoint_for_scope(api_base: &str) -> String { + let trimmed = api_base.trim(); + let (prefix, rest) = match trimmed.find("://") { + Some(idx) => { + let (scheme, rest) = trimmed.split_at(idx + 3); + (scheme, rest) + } + None => ("", trimmed), + }; + // Host runs until the first path char, query, or port colon; the port + // itself is also case-insensitive. + let host_end = rest.find(['/', '?', '#']).unwrap_or(rest.len()); + let (authority, tail) = rest.split_at(host_end); + format!("{prefix}{}{tail}", authority.to_ascii_lowercase()) + } + + /// Stable capability-record scope for this runtime instance. + /// + /// Image-input rejections are recorded per concrete endpoint, not per + /// provider slot: named profiles are distinguished by profile id *and* + /// api base (a profile's `base_url` can be edited while another session + /// still holds the old endpoint), and endpoint-only runtimes (explicit + /// `OPENROUTER_BASE_URL`, autodetected Ollama/LM Studio) by their api base + /// alone. Profile id and api base are both stable for the instance lifetime + /// and both are known at construction, so record and lookup paths derive + /// the identical key. Falls back to the bare slot key when neither is + /// present. + pub(crate) fn capability_scope_key(&self) -> String { + let profile_part = self + .profile_id + .as_deref() + .map(str::trim) + .filter(|id| !id.is_empty()); + let api_base = self.api_base.trim(); + if let Some(profile_id) = profile_part + && !api_base.is_empty() + { + return format!( + "{}::profile:{}::endpoint:{}", + jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ), + profile_id.to_ascii_lowercase(), + Self::normalized_endpoint_for_scope(api_base), + ); + } + if let Some(profile_id) = profile_part { + return format!( + "{}::profile:{}", + jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ), + profile_id.to_ascii_lowercase(), + ); + } + if !api_base.is_empty() { + return format!( + "{}::endpoint:{}", + jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ), + Self::normalized_endpoint_for_scope(api_base), + ); + } + jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ) + .to_string() + } + /// The disk-cache namespace this provider's *foreground* catalog reads and /// writes should use. /// diff --git a/crates/jcode-provider-openrouter-runtime/src/openrouter_sse_stream.rs b/crates/jcode-provider-openrouter-runtime/src/openrouter_sse_stream.rs index c861603a1a..3ee2c2e0c9 100644 --- a/crates/jcode-provider-openrouter-runtime/src/openrouter_sse_stream.rs +++ b/crates/jcode-provider-openrouter-runtime/src/openrouter_sse_stream.rs @@ -68,7 +68,24 @@ pub(super) async fn run_stream_with_retries( provider_pin: Arc>>, model: String, responses_api: bool, + capability_scope_key: String, ) { + let mut request = request; + // One modality retry: when the endpoint answers "this model cannot accept + // image input" for a request that carried `image_url` parts, rewrite the + // request with the images replaced by text markers and replay it once on + // the SAME profile. This is the streaming twin of the failover-layer retry + // in `MultiProvider::complete_with_failover`: HTTP errors surface here, + // inside the async stream, because the stream handle is handed back before + // the request is even sent. + let mut image_modality_retry_available = + jcode_provider_core::image_capability::request_contains_image_url_parts(&request); + // Set by the inner replay loop when an attempt fails for a non-modality + // (or after-partial-output) reason; consumed by the outer loop's + // transient-vs-deterministic classification. Deferred-init: the only + // read (`.take()` below) is always preceded by the assignment in the + // same outer-loop iteration. + let mut last_attempt_failure: Option<(anyhow::Error, bool, String)>; let mut last_error = None; let mut next_retry_delay = None; let config = jcode_base::config::config(); @@ -76,6 +93,11 @@ pub(super) async fn run_stream_with_retries( let retry_backoff_cap = std::time::Duration::from_secs(config.provider.retry_backoff_cap_secs.max(1)); + // Outer loop: one iteration per user-visible retry attempt (config + // max_retries). Replays happen inside the same `attempt` so the modality + // retry never consumes one of the user's `max_retries` (with + // `max_retries = 1` a `continue` would exit the loop and end the + // stream silently). for attempt in 0..max_retries { if attempt > 0 { let delay = jcode_provider_core::retry_after::retry_delay( @@ -93,84 +115,141 @@ pub(super) async fn run_stream_with_retries( )); } - jcode_base::logging::info(&format!( - "API stream attempt {}/{} over HTTPS transport (model: {}, endpoint: {}, auth: {})", - attempt + 1, - max_retries, - model, - api_base, - auth.label() - )); - - // Track whether this attempt streams replay-visible output so a - // mid-stream transport fault can roll the partial output back on the - // consumer before the retry replays the response from the top. - let (attempt_tx, attempt_guard) = - jcode_provider_core::attempt_tracker::track_attempt_output(tx.clone()); - - // Retries use a fresh unpooled client: the fault that broke attempt N - // (e.g. TLS BadRecordMac from a corrupting middlebox) may also have - // poisoned other idle pooled connections opened through the same path, - // so reusing the shared pool can fail identically. A fresh client - // guarantees a brand-new TCP+TLS connection. - let attempt_client = if attempt == 0 { - client.clone() - } else { - jcode_provider_core::fresh_transport_client() - }; + // Modality replay is an inner loop: a request that carried image + // parts may be rejected for the image modality and replayed once, + // rewritten with text markers, on the same attempt slot. Doing it + // with `continue` on the outer loop would silently swallow the + // failure when `max_retries == 1`: the loop would exit with + // `last_error` unset and the consumer would see an empty stream. + loop { + jcode_base::logging::info(&format!( + "API stream attempt {}/{} over HTTPS transport (model: {}, endpoint: {}, auth: {})", + attempt + 1, + max_retries, + model, + api_base, + auth.label() + )); - match stream_response( - attempt_client, - api_base.clone(), - auth.clone(), - send_openrouter_headers, - &conversation_id, - request.clone(), - azure_responses, - attempt_tx, - Arc::clone(&provider_pin), - model.clone(), - responses_api, - ) - .await - { - Ok(()) => { - let _ = attempt_guard.finish().await; - return; - } - Err(e) => { - let saw_output = attempt_guard.finish().await; - // Full anyhow chain ({:#}) so a `.context(...)`-wrapped transport - // cause (e.g. TLS BadRecordMac) is visible to the classifier. - if retryable_stream_failure(&e) && attempt + 1 < max_retries { - if saw_output { - // Partial output already reached the consumer; tell it - // to discard the partial attempt so the retried - // response replays cleanly instead of duplicating. + // Track whether this attempt streams replay-visible output so a + // mid-stream transport fault can roll the partial output back on the + // consumer before the retry replays the response from the top. + let (attempt_tx, attempt_guard) = + jcode_provider_core::attempt_tracker::track_attempt_output(tx.clone()); + + // Retries use a fresh unpooled client: the fault that broke attempt N + // (e.g. TLS BadRecordMac from a corrupting middlebox) may also have + // poisoned other idle pooled connections opened through the same path, + // so reusing the shared pool can fail identically. A fresh client + // guarantees a brand-new TCP+TLS connection. + let attempt_client = if attempt == 0 { + client.clone() + } else { + jcode_provider_core::fresh_transport_client() + }; + + match stream_response( + attempt_client, + api_base.clone(), + auth.clone(), + send_openrouter_headers, + &conversation_id, + request.clone(), + azure_responses, + attempt_tx, + Arc::clone(&provider_pin), + model.clone(), + responses_api, + ) + .await + { + Ok(()) => { + let _ = attempt_guard.finish().await; + return; + } + Err(e) => { + let saw_output = attempt_guard.finish().await; + // Full anyhow chain ({:#}) so a `.context(...)`-wrapped transport + // cause (e.g. TLS BadRecordMac) is visible to the classifier. + let error_str = format!("{e:#}").to_lowercase(); + // Image-modality rejection: the endpoint is healthy, only the + // `image_url` parts are unsupported by this model. Checked + // BEFORE the transient-error classifier, which correctly + // classifies the HTTP 400 as a deterministic client error: + // here that is true for the pixel data, not for the request + // shape. Replay the same request once with images replaced + // by text markers. Errors after partial output skip this path + // on purpose: the consumer already rendered the tokens, a + // silent rewrite would duplicate them. + if image_modality_retry_available + && !saw_output + && jcode_provider_core::image_capability::error_indicates_image_rejection( + &error_str, + ) + { + image_modality_retry_available = false; + // Record under the exact (capability-scope, model) + // pair that `MultiProvider::supports_image_input` and + // this runtime's own `supports_image_input` look up, so + // the NEXT request filters images at build time instead + // of paying another rejected call. The scope is the + // profile id or endpoint base, so a rejection on one + // OpenAI-compatible profile never suppresses images for + // another profile serving the same model name. The model + // here is the same raw string `Provider::model()` + // returns (the runtime's model id). + jcode_provider_core::image_capability::record_image_input_rejection( + Some(&capability_scope_key), + &model, + &error_str, + ); jcode_base::logging::warn(&format!( - "Transient API error after partial output; rolling back partial attempt and retrying: {}", - e - )); - let _ = tx - .send(Ok(StreamEvent::RetryRollback { - attempt: attempt + 2, - max: max_retries, - })) - .await; - } else { - jcode_base::logging::info(&format!( - "Transient API error, will retry: {}", - e + "Endpoint rejected image input for model {model}; \ + retrying once with images replaced by text markers" )); + request = jcode_provider_core::image_capability::filter_image_url_parts_from_request( + &request, + ); + // Replay inside the same attempt slot: no sleep (the + // failure is deterministic, not transient), no extra + // attempt consumed. + continue; } - next_retry_delay = jcode_provider_core::retry_after::retry_after_from_error(&e); - last_error = Some(e); - continue; + last_attempt_failure = Some((e, saw_output, error_str)); + break; } + } + } - let _ = tx.send(Err(e)).await; - return; + // The inner loop broke: either the failure was not a modality + // rejection, or it came after partial output. Classify transient + // vs deterministic here, on the last error observed. + if let Some((e, saw_output, _)) = last_attempt_failure.take() { + if retryable_stream_failure(&e) && attempt + 1 < max_retries { + if saw_output { + // Partial output already reached the consumer; tell it + // to discard the partial attempt so the retried + // response replays cleanly instead of duplicating. + jcode_base::logging::warn(&format!( + "Transient API error after partial output; rolling back partial attempt and retrying: {}", + e + )); + let _ = tx + .send(Ok(StreamEvent::RetryRollback { + attempt: attempt + 2, + max: max_retries, + })) + .await; + } else { + jcode_base::logging::info(&format!("Transient API error, will retry: {}", e)); + } + next_retry_delay = jcode_provider_core::retry_after::retry_after_from_error(&e); + last_error = Some(e); + continue; } + + let _ = tx.send(Err(e)).await; + return; } } @@ -367,6 +446,29 @@ async fn stream_response( if matches!(event, StreamEvent::MessageEnd { .. }) { saw_message_end = true; } + } else if let StreamEvent::Error { message, .. } = &event { + // A 200-status SSE `error` event saying the model rejects images + // must reach the attempt loop's modality recovery, not be + // forwarded and swallowed: forwarding made stream_response return + // Ok(()) with the rejection neither recorded nor retried, so the + // turn failed and every later turn repeated the same rejected + // request. Return it as an error so the outer loop classifies it + // (image rejection, no output yet) and replays once with images + // filtered. Any other in-stream error stays forwarded: the + // consumer surfaces it after the stream closes, as before. + if jcode_provider_core::image_capability::error_indicates_image_rejection(message) { + return Err(anyhow::anyhow!( + "OpenAI-compatible stream reported an image-modality rejection + endpoint: {} + model: {} + auth: {} + error: {}", + url, + model, + auth.label(), + message + )); + } } if tx.send(Ok(event)).await.is_err() { return Ok(()); diff --git a/crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs b/crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs index 8907b92042..b58825d467 100644 --- a/crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs +++ b/crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs @@ -102,7 +102,7 @@ impl SharedEnvLock { /// process env state, so a panic in one test must not cascade into a /// flood of unrelated `PoisonError` failures across every other test /// that takes this lock. - pub(crate) fn lock(&self) -> std::sync::MutexGuard<'static, ()> { + pub(crate) fn lock(&self) -> jcode_base::storage::TestEnvGuard { jcode_base::storage::lock_test_env() } } @@ -3156,6 +3156,7 @@ fn midstream_transport_fault_emits_retry_rollback_before_replay() { Arc::new(Mutex::new(None)), "test-model".to_string(), false, + "openrouter::test-scope".to_string(), ) .await; @@ -3548,3 +3549,693 @@ fn named_openai_compatible_provider_keeps_stable_name_and_profile_display_name() #[path = "openrouter_wire_tests.rs"] mod wire_tests; + +// ============================================================================ +// Stream-level image-modality retry (text-only model behind optimistic endpoint) +// ============================================================================ + +/// Fake SSE server for the streaming modality retry: the first request is +/// rejected with a 400 "does not support image input" error; the retry request +/// (which must no longer carry `image_url` parts) gets a clean SSE completion. +/// Both raw request bodies are captured for assertions. +fn spawn_image_rejecting_then_clean_server() -> ( + String, + std::sync::mpsc::Receiver, + std::sync::mpsc::Receiver, +) { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let addr = listener.local_addr().expect("addr"); + let (tx_first, rx_first) = std::sync::mpsc::channel::(); + let (tx_retry, rx_retry) = std::sync::mpsc::channel::(); + std::thread::spawn(move || { + // Connection 1: modality rejection. + { + let (mut stream, _) = listener.accept().expect("accept first"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("timeout"); + let mut buf = vec![0u8; 65536]; + let n = stream.read(&mut buf).unwrap_or(0); + let _ = tx_first.send(String::from_utf8_lossy(&buf[..n]).to_string()); + let body = r#"{"error": {"message": "This model does not support image input. The image_url content part is not supported: text-only model.", "type": "invalid_request_error", "code": "unsupported_modality"}}"#; + let response = format!( + "HTTP/1.1 400 Bad Request\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(response.as_bytes()); + } + // Connection 2 (modality retry): clean completion. + { + let (mut stream, _) = listener.accept().expect("accept retry"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("timeout"); + let mut buf = vec![0u8; 65536]; + let n = stream.read(&mut buf).unwrap_or(0); + let _ = tx_retry.send(String::from_utf8_lossy(&buf[..n]).to_string()); + let body = "data: {\"choices\":[{\"delta\":{\"content\":\"described marker only\"},\"finish_reason\":\"stop\"}]}\n\ndata: [DONE]\n\n"; + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(response.as_bytes()); + } + }); + (format!("http://{addr}/v1"), rx_first, rx_retry) +} + +/// The streaming twin of the failover-layer regression: HTTP errors surface +/// inside the async stream, so the modality retry must fire there. A request +/// carrying `image_url` parts that the endpoint rejects with a modality error +/// must be replayed once with the images replaced by a text marker, on the +/// same endpoint, and the final answer must still stream to the consumer. +#[test] +fn image_modality_rejection_is_retried_with_filtered_request() { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + + rt.block_on(async { + let (api_base, rx_first, rx_retry) = spawn_image_rejecting_then_clean_server(); + let client = reqwest::Client::new(); + let (tx, mut rx) = tokio::sync::mpsc::channel::>(64); + + let request = serde_json::json!({ + "model": "text-only-test-model", + "stream": true, + "messages": [ + {"role": "user", "content": [ + {"type": "text", "text": "describe red.png"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,aVZC"}} + ]} + ] + }); + + super::openrouter_sse_stream::run_stream_with_retries( + client, + api_base, + ProviderAuth::None { + label: "test".to_string(), + }, + false, + new_conversation_id(), + request, + false, + tx, + Arc::new(Mutex::new(None)), + "text-only-test-model".to_string(), + false, + "openrouter::test-scope".to_string(), + ) + .await; + + let mut final_text = String::new(); + let mut errored = false; + while let Some(item) = rx.recv().await { + match item { + Ok(StreamEvent::TextDelta(text)) => final_text.push_str(&text), + Err(_) => errored = true, + _ => {} + } + } + + assert!( + !errored, + "stream surfaced an error instead of replaying with filtered request" + ); + assert_eq!(final_text, "described marker only"); + + let first = rx_first + .recv_timeout(Duration::from_secs(5)) + .expect("first request captured"); + assert!( + first.contains(r#""type":"image_url""#), + "first request must carry the image_url part" + ); + + let retry = rx_retry + .recv_timeout(Duration::from_secs(5)) + .expect("retry request captured"); + assert!( + !retry.contains(r#""type":"image_url""#), + "modality retry must not carry image_url parts: {retry}" + ); + assert!( + retry.contains("[Image omitted: this model does not support image input"), + "modality retry must carry the text marker replacing the image: {retry}" + ); + }); +} + +/// Greptile follow-up: some gateways answer HTTP 200 and deliver the +/// image-modality rejection as an SSE `error` EVENT instead of a non-2xx +/// status. `stream_response` used to forward that event and return Ok(()), +/// so the attempt loop never saw a failure: the rejection was neither +/// recorded nor retried, the turn errored after the forwarded event, and +/// later turns repeated the identical rejected request. The SSE error event +/// must be converted into a stream failure so the same modality recovery +/// fires: one replay with images filtered, rejection recorded for later +/// turns, final answer delivered. +#[test] +fn sse_error_event_image_rejection_is_retried_with_filtered_request() { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + + rt.block_on(async { + let (api_base, rx_first, rx_retry) = spawn_sse_image_rejecting_then_clean_server(); + let client = reqwest::Client::new(); + let (tx, mut rx) = tokio::sync::mpsc::channel::>(64); + + let request = serde_json::json!({ + "model": "text-only-test-model", + "stream": true, + "messages": [ + {"role": "user", "content": [ + {"type": "text", "text": "describe red.png"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,aVZC"}} + ]} + ] + }); + + super::openrouter_sse_stream::run_stream_with_retries( + client, + api_base, + ProviderAuth::None { + label: "test".to_string(), + }, + false, + new_conversation_id(), + request, + false, + tx, + Arc::new(Mutex::new(None)), + "text-only-test-model".to_string(), + false, + "openrouter::test-scope".to_string(), + ) + .await; + + let mut final_text = String::new(); + let mut errored = false; + while let Some(item) = rx.recv().await { + match item { + Ok(StreamEvent::TextDelta(text)) => final_text.push_str(&text), + Err(_) => errored = true, + _ => {} + } + } + + assert!( + !errored, + "SSE error-event rejection must be replayed, not surfaced as a failure" + ); + assert_eq!(final_text, "described marker only"); + + let first = rx_first + .recv_timeout(Duration::from_secs(5)) + .expect("first request captured"); + assert!( + first.contains(r#""type":"image_url""#), + "first request must carry the image_url part" + ); + + let retry = rx_retry + .recv_timeout(Duration::from_secs(5)) + .expect("retry request captured"); + assert!( + !retry.contains(r#""type":"image_url""#), + "modality retry must not carry image_url parts: {retry}" + ); + assert!( + retry.contains("[Image omitted: this model does not support image input"), + "modality retry must carry the text marker replacing the image: {retry}" + ); + }); +} + +/// SSE twin of [`spawn_image_rejecting_then_clean_server`]: the rejection +/// arrives as a 200-status SSE `error` data event, not an HTTP 400 body. +fn spawn_sse_image_rejecting_then_clean_server() -> ( + String, + std::sync::mpsc::Receiver, + std::sync::mpsc::Receiver, +) { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let addr = listener.local_addr().expect("addr"); + let (tx_first, rx_first) = std::sync::mpsc::channel::(); + let (tx_retry, rx_retry) = std::sync::mpsc::channel::(); + std::thread::spawn(move || { + // Connection 1: 200 OK, then the rejection as an SSE error event. + { + let (mut stream, _) = listener.accept().expect("accept first"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("timeout"); + let mut buf = vec![0u8; 65536]; + let n = stream.read(&mut buf).unwrap_or(0); + let _ = tx_first.send(String::from_utf8_lossy(&buf[..n]).to_string()); + let body = "data: {\"error\": {\"message\": \"This model does not support image input. The image_url content part is not supported: text-only model.\", \"type\": \"invalid_request_error\", \"code\": \"unsupported_modality\"}}\n\n\n"; + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(response.as_bytes()); + } + // Connection 2 (modality retry): clean completion. + { + let (mut stream, _) = listener.accept().expect("accept retry"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("timeout"); + let mut buf = vec![0u8; 65536]; + let n = stream.read(&mut buf).unwrap_or(0); + let _ = tx_retry.send(String::from_utf8_lossy(&buf[..n]).to_string()); + let body = "data: {\"choices\":[{\"delta\":{\"content\":\"described marker only\"},\"finish_reason\":\"stop\"}]}\n\ndata: [DONE]\n\n"; + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(response.as_bytes()); + } + }); + (format!("http://{addr}/v1"), rx_first, rx_retry) +} + +/// The modality retry is one-shot and only fires for modality errors: a +/// request without `image_url` parts that fails with a 400 must surface the +/// error immediately, with no phantom retry attempt. +#[test] +fn non_image_requests_get_no_modality_retry() { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + + rt.block_on(async { + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let addr = listener.local_addr().expect("addr"); + let (tx_done, rx_done) = std::sync::mpsc::channel::(); + std::thread::spawn(move || { + use std::io::{Read, Write}; + let mut requests = 0usize; + while let Ok((mut stream, _)) = listener.accept() { + requests += 1; + let _ = tx_done.send(requests); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("timeout"); + let mut buf = vec![0u8; 65536]; + let _ = stream.read(&mut buf); + let body = r#"{"error": {"message": "Invalid request: max_tokens must be greater than 0", "type": "invalid_request_error"}}"#; + let response = format!( + "HTTP/1.1 400 Bad Request\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(response.as_bytes()); + } + }); + + let client = reqwest::Client::new(); + let (tx, mut rx) = tokio::sync::mpsc::channel::>(64); + let request = serde_json::json!({ + "model": "text-only-test-model", + "stream": true, + "messages": [{"role": "user", "content": "hi"}] + }); + + super::openrouter_sse_stream::run_stream_with_retries( + client, + format!("http://{addr}/v1"), + ProviderAuth::None { + label: "test".to_string(), + }, + false, + new_conversation_id(), + request, + false, + tx, + Arc::new(Mutex::new(None)), + "text-only-test-model".to_string(), + false, + "openrouter::test-scope".to_string(), + ) + .await; + + let mut errored = false; + while let Some(item) = rx.recv().await { + if item.is_err() { + errored = true; + } + } + assert!(errored, "deterministic 400 must surface as an error"); + let total_requests = rx_done.recv_timeout(Duration::from_secs(5)).expect("counted"); + assert_eq!( + total_requests, 1, + "non-modality 400 must not be replayed with a filtered request" + ); + }); +} + +/// The modality replay must not consume a user-visible retry attempt: with +/// `max_retries = 1` the first (and only) attempt is rejected for the image +/// modality, the replayed request must still run, stream its answer, and not +/// end the stream silently (regression: a `continue` on the outer loop used to +/// exit with no error and no output when `max_retries == 1`). +#[test] +fn image_modality_retry_survives_max_retries_one() { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + + rt.block_on(async { + let (api_base, rx_first, rx_retry) = spawn_image_rejecting_then_clean_server(); + let client = reqwest::Client::new(); + let (tx, mut rx) = tokio::sync::mpsc::channel::>(64); + + let request = serde_json::json!({ + "model": "text-only-test-model", + "stream": true, + "messages": [ + {"role": "user", "content": [ + {"type": "text", "text": "describe red.png"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,aVZC"}} + ]} + ] + }); + + let _env_guard = ENV_LOCK.lock(); + let _max_retries_guard = EnvVarGuard::set("JCODE_MAX_RETRIES", "1"); + super::openrouter_sse_stream::run_stream_with_retries( + client, + api_base, + ProviderAuth::None { + label: "test".to_string(), + }, + false, + new_conversation_id(), + request, + false, + tx, + Arc::new(Mutex::new(None)), + "text-only-test-model".to_string(), + false, + "openrouter::test-scope".to_string(), + ) + .await; + drop(_max_retries_guard); + drop(_env_guard); + + let mut final_text = String::new(); + let mut stream_ended_without_output = true; + while let Some(item) = rx.recv().await { + match item { + Ok(StreamEvent::TextDelta(text)) => { + stream_ended_without_output = false; + final_text.push_str(&text); + } + Err(_) => stream_ended_without_output = false, + _ => {} + } + } + assert!( + !stream_ended_without_output, + "with max_retries=1 the modality replay must still stream an answer, not end silently" + ); + assert_eq!(final_text, "described marker only"); + + let retry = rx_retry + .recv_timeout(Duration::from_secs(5)) + .expect("retry request captured even with max_retries=1"); + assert!( + !retry.contains(r#""type":"image_url""#), + "replayed request must carry the text marker, not pixels: {retry}" + ); + let _ = rx_first; + }); +} + +/// Regression for the record/lookup key mismatch in the streaming image +/// replay: `complete()` hands the stream the runtime's raw model state, but +/// the consumers of the recorded rejection (`supports_image_input` here, and +/// `MultiProvider::supports_image_input` via `candidate_model`) compare +/// against the *prefix-stripped* model id that `set_model` normally +/// normalizes into state. Right after session restore the state can still +/// carry the session-routing `:` prefix (#403), so a +/// rejection recorded as `ollama:llava` would never match the `llava` +/// lookup and the NEXT request would ship pixels again. +/// +/// Wire-level, through `complete()` on a real provider instance: the first +/// request carries the image and is rejected, the replay arrives pixel-free, +/// and afterwards the recorded rejection must be visible under the stripped +/// model key so `supports_image_input()` flips to false for follow-up turns. +#[test] +fn image_modality_rejection_records_under_stripped_model_key() { + let _lock = ENV_LOCK.lock(); + let key = jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ); + // Rejections are recorded under the runtime's capability scope (here + // profile id + api base, since `profile_id` is set), not the bare + // OpenRouter slot key. The exact scope is derived from the provider under + // test so this stays correct if the key shape ever changes again. + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(key); + + let (api_base, rx_first, rx_retry) = spawn_image_rejecting_then_clean_server(); + // Simulate the #403 transient: session restore left the raw state + // prefixed. `profile_id` matches the prefix so the strip logic applies. + let provider = OpenRouterProvider { + api_base: api_base.clone(), + model: Arc::new(RwLock::new("ollama:llava".to_string())), + profile_id: Some("ollama".to_string()), + supports_provider_features: false, + supports_model_catalog: false, + ..make_custom_compatible_provider() + }; + let scoped = provider.capability_scope_key(); + assert!( + scoped.starts_with(&format!("{key}::profile:ollama::endpoint:")), + "scope must be profile- and endpoint-scoped, got: {scoped}" + ); + + let messages = vec![Message { + role: Role::User, + content: vec![ + ContentBlock::Text { + text: "describe red.png".to_string(), + cache_control: None, + }, + ContentBlock::Image { + media_type: "image/png".to_string(), + data: "aVZC".to_string(), + }, + ], + timestamp: None, + tool_duration_ms: None, + }]; + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + rt.block_on(async { + // Before the rejection is known, the compat default is optimistic. + assert!( + provider.supports_image_input(), + "prefixed state must not break the pre-record capability answer" + ); + + let mut stream = provider + .complete(&messages, &[], "", None) + .await + .expect("complete returns a stream handle"); + let mut saw_text = false; + while let Some(event) = stream.next().await { + if let StreamEvent::TextDelta(_) = event.expect("stream event") { + saw_text = true; + } + } + assert!( + saw_text, + "the pixel-free replay must still stream an answer" + ); + + // The self-heal replay fired on the wire. + let retry = rx_retry + .recv_timeout(Duration::from_secs(5)) + .expect("retry request captured"); + assert!( + !retry.contains(r#""type":"image_url""#), + "replayed request must carry the text marker, not pixels: {retry}" + ); + let _ = rx_first; + + // Prevention layer: the rejection must be recorded under the stripped + // model id (`llava`), which is the key the lookups use. Without the + // fix this was recorded as `ollama:llava` and never matched. + assert!( + !provider.supports_image_input(), + "recorded rejection must flip supports_image_input to false under the stripped key" + ); + assert!(jcode_provider_core::image_capability::image_input_rejected( + &scoped, "llava" + )); + assert!( + !jcode_provider_core::image_capability::image_input_rejected(&scoped, "ollama:llava"), + "the record must not be stored under the raw prefixed spelling" + ); + assert!( + !jcode_provider_core::image_capability::image_input_rejected(key, "llava"), + "the record must be scoped to the profile, not the bare slot key" + ); + }); + + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(key); + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(&scoped); +} + +/// Same named profile, different `base_url`: a rejection observed on the old +/// endpoint must not suppress image input on the new endpoint. The capability +/// scope key has to include the concrete api base, not just the profile id, +/// because a profile's `base_url` can be edited while another session still +/// holds a provider instance built against the previous endpoint. +#[test] +fn image_rejection_does_not_leak_across_profile_endpoints() { + let key = jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ); + let old_endpoint_key = format!("{key}::profile:gateway::endpoint:http://old.example/v1"); + let new_endpoint_key = format!("{key}::profile:gateway::endpoint:http://new.example/v1"); + for scope in [&old_endpoint_key, &new_endpoint_key] { + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(scope); + } + + let old_provider = OpenRouterProvider { + api_base: "http://old.example/v1".to_string(), + profile_id: Some("gateway".to_string()), + ..make_custom_compatible_provider() + }; + let new_provider = OpenRouterProvider { + api_base: "http://new.example/v1".to_string(), + profile_id: Some("gateway".to_string()), + ..make_custom_compatible_provider() + }; + + assert_eq!( + old_provider.capability_scope_key(), + old_endpoint_key, + "profile-scoped key must include the concrete endpoint" + ); + assert_eq!( + new_provider.capability_scope_key(), + new_endpoint_key, + "same profile with a different base_url must produce a different scope key" + ); + assert_ne!( + old_provider.capability_scope_key(), + new_provider.capability_scope_key() + ); + + // A rejection recorded through the old endpoint's scope... + jcode_provider_core::image_capability::record_image_input_rejection( + Some(&old_endpoint_key), + "model-a", + "this model does not support image input", + ); + // ...must not be visible through the new endpoint's scope, so the new + // endpoint keeps its optimistic capability answer and keeps sending + // pixels instead of silently degrading to text markers. + assert!(jcode_provider_core::image_capability::image_input_rejected( + &old_endpoint_key, + "model-a" + )); + assert!( + !jcode_provider_core::image_capability::image_input_rejected(&new_endpoint_key, "model-a"), + "a rejection on one endpoint of a profile must not disable images on another" + ); + // Sanity: the scoped keys must not collapse onto the profile-only shape + // (the pre-fix bug) or the bare slot key. + let profile_only = format!("{key}::profile:gateway"); + assert!( + !jcode_provider_core::image_capability::image_input_rejected(&profile_only, "model-a"), + "records must live under endpoint-scoped keys, not the bare profile key" + ); + assert!( + !jcode_provider_core::image_capability::image_input_rejected(key, "model-a"), + "records must not leak onto the un-scoped slot key" + ); + + for scope in [&old_endpoint_key, &new_endpoint_key, &profile_only, key] { + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(scope); + } +} + +/// URL paths are case-sensitive (RFC 3986): a gateway serving distinct +/// backends at `/Text/v1` and `/text/v1` must get distinct capability scope +/// keys, so a rejection on the text-only route cannot disable images on the +/// vision route. Only scheme and host are case-insensitive and may be +/// normalized; the path keeps its exact spelling. +#[test] +fn image_rejection_does_not_leak_across_case_distinct_endpoint_paths() { + let key = jcode_provider_core::selection::provider_key( + jcode_provider_core::selection::ActiveProvider::OpenRouter, + ); + let text_key = format!("{key}::profile:gateway::endpoint:https://gateway.example/Text/v1"); + let vision_key = format!("{key}::profile:gateway::endpoint:https://gateway.example/text/v1"); + for scope in [&text_key, &vision_key] { + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(scope); + } + + let text_provider = OpenRouterProvider { + api_base: "https://GATEWAY.example/Text/v1".to_string(), + profile_id: Some("gateway".to_string()), + ..make_custom_compatible_provider() + }; + let vision_provider = OpenRouterProvider { + api_base: "https://gateway.example/text/v1".to_string(), + profile_id: Some("gateway".to_string()), + ..make_custom_compatible_provider() + }; + + // The host is normalized (case-insensitive), the path is preserved. + assert_eq!( + text_provider.capability_scope_key(), + text_key, + "host may be lowercased but the case-sensitive path must keep its spelling" + ); + assert_eq!(vision_provider.capability_scope_key(), vision_key); + assert_ne!( + text_provider.capability_scope_key(), + vision_provider.capability_scope_key(), + "distinct gateway routes must not share a rejection scope" + ); + + // A rejection recorded through the text-only route... + jcode_provider_core::image_capability::record_image_input_rejection( + Some(&text_key), + "model-a", + "this model does not support image input", + ); + // ...must not suppress image input on the vision route. + assert!(jcode_provider_core::image_capability::image_input_rejected( + &text_key, "model-a" + )); + assert!( + !jcode_provider_core::image_capability::image_input_rejected(&vision_key, "model-a"), + "a rejection on one gateway route must not disable images on a case-distinct route" + ); + + for scope in [&text_key, &vision_key] { + jcode_provider_core::image_capability::clear_image_input_overrides_for_provider(scope); + } +} diff --git a/crates/jcode-provider-openrouter-runtime/src/openrouter_wire_tests.rs b/crates/jcode-provider-openrouter-runtime/src/openrouter_wire_tests.rs index 0ca7c51d65..e8b1ef2bfd 100644 --- a/crates/jcode-provider-openrouter-runtime/src/openrouter_wire_tests.rs +++ b/crates/jcode-provider-openrouter-runtime/src/openrouter_wire_tests.rs @@ -101,6 +101,7 @@ fn captured_request_for_host(host: &str, conversation_id: &str) -> String { Arc::new(Mutex::new(None)), "m".to_string(), false, + "openrouter::test-scope".to_string(), ) .await; while events.recv().await.is_some() {} diff --git a/crates/jcode-tui-render/src/swarm_gallery_tests.rs b/crates/jcode-tui-render/src/swarm_gallery_tests.rs index 78c40976b9..168c5b622e 100644 --- a/crates/jcode-tui-render/src/swarm_gallery_tests.rs +++ b/crates/jcode-tui-render/src/swarm_gallery_tests.rs @@ -568,7 +568,10 @@ fn vertical_strip_unfocused_keeps_live_card_details_in_chat() { ); // The compact row names the active step in place of the spawn prompt, // but stays a single row: the full todo card is chat/focus-only. - assert!(all.contains("test token refresh flow"), "active step: {all}"); + assert!( + all.contains("test token refresh flow"), + "active step: {all}" + ); assert!( !all.contains("review authentication changes"), "spawn prompt should yield to the active todo: {all}" diff --git a/crates/jcode-tui/src/tui/app/commands_tests.rs b/crates/jcode-tui/src/tui/app/commands_tests.rs index caba5707f3..a3d3d02237 100644 --- a/crates/jcode-tui/src/tui/app/commands_tests.rs +++ b/crates/jcode-tui/src/tui/app/commands_tests.rs @@ -183,7 +183,7 @@ mod colors { /// still let them swap the config out from under another module's test /// mid-assertion, which is the race class that makes unrelated provider and /// header tests fail intermittently. - fn lock_shared_state() -> std::sync::MutexGuard<'static, ()> { + fn lock_shared_state() -> crate::storage::TestEnvGuard { crate::storage::lock_test_env() } diff --git a/crates/jcode-tui/src/tui/app/helpers_tests.rs b/crates/jcode-tui/src/tui/app/helpers_tests.rs index c9008e2b0a..63b9726fa1 100644 --- a/crates/jcode-tui/src/tui/app/helpers_tests.rs +++ b/crates/jcode-tui/src/tui/app/helpers_tests.rs @@ -270,18 +270,20 @@ fn resume_invocation_args_omits_blank_socket() { /// whether the developer machine has a published local build channel and of /// other tests mutating JCODE_HOME in parallel. Returns the guards that keep /// the environment pinned for the duration of the test. -fn pinned_resume_test_home() -> ( - std::sync::MutexGuard<'static, ()>, - tempfile::TempDir, - EnvVarGuard, -) { +/// +/// Field order matters: tuple fields drop in field order, so the env guard +/// must be field 0. If the lock were field 0 it would release before +/// JCODE_HOME was restored, and whichever test acquired the lock next would +/// run mid-restore with a foreign or missing home (that clobbered +/// gather_ambient_info_* in parallel runs). +fn pinned_resume_test_home() -> (EnvVarGuard, tempfile::TempDir, crate::storage::TestEnvGuard) { let env_lock = crate::storage::lock_test_env(); let temp = tempfile::tempdir().expect("tempdir"); let current = temp.path().join("builds").join("current"); std::fs::create_dir_all(¤t).expect("create builds/current"); std::fs::write(current.join("jcode"), b"#!/bin/sh\n").expect("write fake jcode binary"); let home = EnvVarGuard::set_path("JCODE_HOME", temp.path()); - (env_lock, temp, home) + (home, temp, env_lock) } #[test] @@ -414,7 +416,14 @@ fn gather_ambient_info_filters_to_session_reminders_when_ambient_disabled() { // Read synchronously while the temporary JCODE_HOME and its files are pinned: // an unrelated in-flight cache refresh can otherwise overwrite the cleared // process-global cache with data loaded under another test's JCODE_HOME. - let info = gather_ambient_info_inner(false).expect("ambient info"); + let info = gather_ambient_info_inner(false).unwrap_or_else(|| { + panic!( + "ambient info gathered no widget: JCODE_HOME={:?} (expected {:?}), queue.json={:?}", + std::env::var_os("JCODE_HOME"), + temp.path(), + std::fs::read_to_string(temp.path().join("ambient").join("queue.json")).ok() + ); + }); assert!(info.show_widget); assert_eq!(info.queue_count, 3); assert_eq!(info.reminder_count, 2); diff --git a/crates/jcode-tui/src/tui/ui.rs b/crates/jcode-tui/src/tui/ui.rs index 8285100645..0978592dcd 100644 --- a/crates/jcode-tui/src/tui/ui.rs +++ b/crates/jcode-tui/src/tui/ui.rs @@ -1572,12 +1572,19 @@ pub(crate) fn last_chat_frame() -> Option> { #[cfg(test)] pub(crate) fn render_state_test_lock() -> RenderStateTestGuard { static LOCK: std::sync::OnceLock> = std::sync::OnceLock::new(); + // Global lock order is env lock -> render lock: rendering tests touch env + // (config, JCODE_HOME) while holding the render lock, and env-taking tests + // render. Acquiring env first removes the AB-BA cycle that hung ui::tests. + let env_guard = crate::storage::lock_test_env(); let guard = LOCK .get_or_init(|| std::sync::Mutex::new(())) .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); RENDER_STATE_LOCK_HELD.with(|held| held.set(true)); - RenderStateTestGuard { _guard: guard } + RenderStateTestGuard { + _guard: guard, + _env_guard: env_guard, + } } /// Guard for [`render_state_test_lock`] that also records ownership on this @@ -1586,6 +1593,11 @@ pub(crate) fn render_state_test_lock() -> RenderStateTestGuard { #[cfg(test)] pub(crate) struct RenderStateTestGuard { _guard: std::sync::MutexGuard<'static, ()>, + // Held for the render guard's whole lifetime: releasing env earlier would + // let a sibling take env and wait on render while this thread still holds + // render, reopening the AB-BA window. Drops after `_guard` (fields drop in + // declaration order), so the LIFO env->render order holds on release too. + _env_guard: crate::storage::TestEnvGuard, } #[cfg(test)] diff --git a/crates/jcode-tui/src/tui/ui_tests/basic/idle_animation_partial.rs b/crates/jcode-tui/src/tui/ui_tests/basic/idle_animation_partial.rs index a46beae17a..c8d3a81e02 100644 --- a/crates/jcode-tui/src/tui/ui_tests/basic/idle_animation_partial.rs +++ b/crates/jcode-tui/src/tui/ui_tests/basic/idle_animation_partial.rs @@ -37,7 +37,7 @@ fn pin_full_tier() { /// guard *before* any render-state lock, matching the env-then-render order /// the app tests use. struct IdleAnimationEnvGuard { - _env: std::sync::MutexGuard<'static, ()>, + _env: crate::storage::TestEnvGuard, } impl IdleAnimationEnvGuard { diff --git a/scripts/code_size_budget.json b/scripts/code_size_budget.json index 02dc25faab..622b98fdde 100644 --- a/scripts/code_size_budget.json +++ b/scripts/code_size_budget.json @@ -1,7 +1,7 @@ { "threshold_loc": 1200, "tracked_files": { - "crates/jcode-app-core/src/agent.rs": 1332, + "crates/jcode-app-core/src/agent.rs": 1385, "crates/jcode-app-core/src/agent/response_recovery.rs": 1274, "crates/jcode-app-core/src/agent/turn_execution.rs": 1638, "crates/jcode-app-core/src/agent/turn_loops.rs": 1355, @@ -37,7 +37,7 @@ "crates/jcode-base/src/jev.rs": 1987, "crates/jcode-base/src/memory.rs": 1572, "crates/jcode-base/src/provider/catalog_routes.rs": 1596, - "crates/jcode-base/src/provider/mod.rs": 3228, + "crates/jcode-base/src/provider/mod.rs": 3389, "crates/jcode-base/src/provider/models.rs": 1363, "crates/jcode-base/src/provider_catalog.rs": 1353, "crates/jcode-base/src/session.rs": 1823, @@ -52,7 +52,7 @@ "crates/jcode-provider-anthropic/src/lib.rs": 1246, "crates/jcode-provider-bedrock/src/lib.rs": 1500, "crates/jcode-provider-copilot-runtime/src/lib.rs": 1201, - "crates/jcode-provider-core/src/lib.rs": 1618, + "crates/jcode-provider-core/src/lib.rs": 1631, "crates/jcode-provider-cursor-runtime/src/agent_transport.rs": 1873, "crates/jcode-provider-cursor-runtime/src/wire.rs": 1431, "crates/jcode-provider-doctor/src/lifecycle_driver.rs": 1980, diff --git a/scripts/swallowed_error_budget.json b/scripts/swallowed_error_budget.json index 8a506e5500..369ab3ad13 100644 --- a/scripts/swallowed_error_budget.json +++ b/scripts/swallowed_error_budget.json @@ -1,9 +1,9 @@ { - "total": 3820, + "total": 3829, "totals_by_pattern": { "dot_ok": 1497, "let_underscore": 1377, - "unwrap_or_default": 946 + "unwrap_or_default": 955 }, "tracked_files": { "crates/jcode-app-core/src/agent.rs": { @@ -1044,7 +1044,7 @@ "crates/jcode-base/src/provider/mod.rs": { "dot_ok": 3, "let_underscore": 1, - "unwrap_or_default": 14 + "unwrap_or_default": 23 }, "crates/jcode-base/src/provider/model_catalog_service.rs": { "dot_ok": 7, diff --git a/scripts/test_size_budget.json b/scripts/test_size_budget.json index dbc073382e..7a9dc2770c 100644 --- a/scripts/test_size_budget.json +++ b/scripts/test_size_budget.json @@ -1,7 +1,7 @@ { "threshold_loc": 1200, "tracked_files": { - "crates/jcode-app-core/src/agent_tests.rs": 3094, + "crates/jcode-app-core/src/agent_tests.rs": 3293, "crates/jcode-app-core/src/server/client_lifecycle_tests.rs": 1939, "crates/jcode-app-core/src/server/comm_session_tests.rs": 1326, "crates/jcode-app-core/src/server/provider_control_tests.rs": 1352, @@ -12,14 +12,14 @@ "crates/jcode-base/src/compaction_tests.rs": 1271, "crates/jcode-base/src/config_tests.rs": 1747, "crates/jcode-base/src/live_tests.rs": 3087, - "crates/jcode-base/src/provider/tests.rs": 1335, + "crates/jcode-base/src/provider/tests.rs": 1589, "crates/jcode-base/src/provider/tests/model_resolution.rs": 2248, "crates/jcode-base/src/provider_catalog_tests.rs": 1409, "crates/jcode-base/src/session_tests/cases.rs": 3267, "crates/jcode-base/src/usage/tests.rs": 1258, "crates/jcode-harness-api-server/src/translate_tests.rs": 4199, "crates/jcode-provider-anthropic-runtime/src/anthropic_tests.rs": 2029, - "crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs": 3550, + "crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs": 4241, "crates/jcode-sdk/tests/client_behavior.rs": 1432, "crates/jcode-telemetry-core/src/tests.rs": 1811, "crates/jcode-tui/src/tui/app/remote_tests.rs": 1391, diff --git a/src/cli/commands/restart_tests.rs b/src/cli/commands/restart_tests.rs index 5a1a361783..3f3ea22a53 100644 --- a/src/cli/commands/restart_tests.rs +++ b/src/cli/commands/restart_tests.rs @@ -8,7 +8,7 @@ use std::ffi::OsString; struct TestEnvGuard { prev_home: Option, _temp_home: tempfile::TempDir, - _lock: std::sync::MutexGuard<'static, ()>, + _lock: crate::storage::TestEnvGuard, } impl TestEnvGuard { diff --git a/src/cli/selfdev_tests.rs b/src/cli/selfdev_tests.rs index 6b588ba381..282b7c1c0c 100644 --- a/src/cli/selfdev_tests.rs +++ b/src/cli/selfdev_tests.rs @@ -5,7 +5,7 @@ use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; use std::sync::Arc; -fn lock_env() -> std::sync::MutexGuard<'static, ()> { +fn lock_env() -> storage::TestEnvGuard { storage::lock_test_env() } @@ -44,7 +44,7 @@ fn set_socket_test_env(socket_path: &Path, runtime_dir: &Path) -> EnvVarGuard { } struct TestEnvGuard { - _lock: std::sync::MutexGuard<'static, ()>, + _lock: storage::TestEnvGuard, _env: EnvVarGuard, _temp_home: tempfile::TempDir, } @@ -236,11 +236,7 @@ async fn test_wait_for_reloading_server_returns_true_for_live_listener() { assert!(wait_for_reloading_server().await); } -fn isolated_launcher_env() -> ( - std::sync::MutexGuard<'static, ()>, - EnvVarGuard, - tempfile::TempDir, -) { +fn isolated_launcher_env() -> (storage::TestEnvGuard, EnvVarGuard, tempfile::TempDir) { let lock = lock_env(); let temp = tempfile::tempdir().expect("tempdir"); let env = EnvVarGuard::capture(&["JCODE_INSTALL_DIR", "JCODE_HOME", "HOME", "USERPROFILE"]);