From 5904f45ff32ee8abf4c6598e99000ee44475761c Mon Sep 17 00:00:00 2001 From: itsklimov Date: Fri, 24 Jul 2026 07:56:09 -0700 Subject: [PATCH 1/2] feat(session): carry current selections on CatalogUpdated (additive schema) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SessionEvent::CatalogUpdated gains optional current_model/current_mode/ current_effort so a catalog push can advertise the ACTIVE selection per axis, not only the option lists (the direct-CLI paths previously pushed current_value: null for any session the user had not interactively switched — the #609 regression against the #574 defaults). The fields are additive and #[serde(default)]: old persisted frames still deserialize (round-trip pinned by test), and this commit stamps None at every existing construction site so it builds standalone; the follow-up commit fills them. The structured config_option_rejected reconcile signal is routed PersistTier::Ephemeral here as well — an internal pump reconcile, never display history (its emitters land in the follow-up commit). --- crates/aionui-ai-agent/src/session_agent.rs | 13 +++++++ .../aionui-session/src/backend/claude_conn.rs | 7 ++++ .../aionui-session/src/backend/codex_conn.rs | 6 +++ crates/aionui-session/src/event.rs | 39 +++++++++++++++++++ 4 files changed, 65 insertions(+) diff --git a/crates/aionui-ai-agent/src/session_agent.rs b/crates/aionui-ai-agent/src/session_agent.rs index 1b4470b4e..25b128809 100644 --- a/crates/aionui-ai-agent/src/session_agent.rs +++ b/crates/aionui-ai-agent/src/session_agent.rs @@ -1837,6 +1837,10 @@ fn spawn_event_pump( models, modes, slash_commands, + // The additive current_model/current_mode/current_effort fields are + // consumed by the follow-up commit (highlight precedence); tolerated + // here so this commit builds standalone. + .. } = &env.event { let mut config_options: Vec = Vec::new(); @@ -4361,6 +4365,9 @@ mod pump_tests { description: None, }], slash_commands: Vec::new(), + current_model: None, + current_mode: None, + current_effort: None, })]; let frames = drain_script(script).await; let config = frames @@ -4424,6 +4431,9 @@ mod pump_tests { description: None, }, ], + current_model: None, + current_mode: None, + current_effort: None, })]; let frames = drain_script(script).await; let commands = frames @@ -4456,6 +4466,9 @@ mod pump_tests { }], modes: Vec::new(), slash_commands: Vec::new(), + current_model: None, + current_mode: None, + current_effort: None, })]; let frames = drain_script(script).await; assert!( diff --git a/crates/aionui-session/src/backend/claude_conn.rs b/crates/aionui-session/src/backend/claude_conn.rs index 7efed82a7..128b1776e 100644 --- a/crates/aionui-session/src/backend/claude_conn.rs +++ b/crates/aionui-session/src/backend/claude_conn.rs @@ -1776,6 +1776,12 @@ fn sniff_control_initialize( models: parsed_models, modes: crate::adapter::claude_permission_modes(), slash_commands: parsed_commands, + // The additive current_model/current_mode/current_effort fields are + // filled by the follow-up transport commit; None here keeps this + // commit standalone. + current_model: None, + current_mode: None, + current_effort: None, }, }); } @@ -4731,6 +4737,7 @@ mod tests { models, modes, slash_commands, + .. } = env.event { catalog = Some((models, modes, slash_commands)); diff --git a/crates/aionui-session/src/backend/codex_conn.rs b/crates/aionui-session/src/backend/codex_conn.rs index b77577e44..e76b1ba7c 100644 --- a/crates/aionui-session/src/backend/codex_conn.rs +++ b/crates/aionui-session/src/backend/codex_conn.rs @@ -1572,6 +1572,12 @@ async fn reader_task( // so the agent_metadata writeback + the frontend // AvailableCommands push see it (ELECTRON-3PX). slash_commands: builtin_slash_commands(), + // Filled by the follow-up transport + // commit; None keeps this commit + // standalone. + current_model: None, + current_mode: None, + current_effort: None, }, ); } diff --git a/crates/aionui-session/src/event.rs b/crates/aionui-session/src/event.rs index b7505a753..5534ce11d 100644 --- a/crates/aionui-session/src/event.rs +++ b/crates/aionui-session/src/event.rs @@ -318,6 +318,23 @@ pub enum SessionEvent { /// own mode semantics (claude permission-mode, etc.). modes: Vec, slash_commands: Vec, + /// The backend's CURRENT selections at emit time, so the (backend-Arc-free) + /// event pump can highlight the active model/mode/effort in the pushed + /// config-options frame without calling `capabilities()`. Before these fields + /// existed the push could only read the task-runtime's optimistic overrides — + /// `None` until the user interactively switched — so a catalog push always + /// carried `current_value: null` and the picker showed no active selection + /// (the #609 direct-path regression against the #574 defaults). `None` = the + /// backend does not know that axis's current (the pump's override-wins + /// precedence then falls through to nothing, exactly as before). + /// `#[serde(default)]` keeps OLD persisted frames (no current fields) + /// deserializable — the additive-variant guarantee this enum maintains. + #[serde(default)] + current_model: Option, + #[serde(default)] + current_mode: Option, + #[serde(default)] + current_effort: Option, }, /// ⭐ Subagent lifecycle normalization (Addendum 8 / §9.12-9.14 / U22). THE @@ -815,6 +832,11 @@ pub fn persist_tier(event: &SessionEvent) -> PersistTier { SubagentUpdate { .. } => PersistTier::DisplayAndState, // roster→display; resumable→Tier2.last_subagents Rewound { .. } => PersistTier::State, // turn-truncation anchor AdapterSpecific { tag, .. } if is_raw_timing(tag) => PersistTier::Ephemeral, + // Internal reconcile signal for the task-level pump (clear the optimistic + // effort highlight after a backend reject) — not a user-facing history + // entry; the user-visible half of the same reject is the Notice that + // rides alongside it (Display). + AdapterSpecific { tag, .. } if tag == "config_option_rejected" => PersistTier::Ephemeral, AdapterSpecific { .. } => PersistTier::Display, // Orchestration-lowered already handled above; this arm is unreachable // for them but keeps the match total over the enum. @@ -1601,6 +1623,23 @@ mod additive_tests { } other => panic!("expected Detached, got {other:?}"), } + // An OLD CatalogUpdated frame (pre-currents: no current_model/current_mode/ + // current_effort) must still deserialize → all three default to None. + let old_cat = r#"{"CatalogUpdated":{"models":[],"modes":[],"slash_commands":[]}}"#; + let ev: SessionEvent = serde_json::from_str(old_cat).expect("old CatalogUpdated deserializes"); + match ev { + SessionEvent::CatalogUpdated { + current_model, + current_mode, + current_effort, + .. + } => { + assert!(current_model.is_none(), "missing current_model → None"); + assert!(current_mode.is_none(), "missing current_mode → None"); + assert!(current_effort.is_none(), "missing current_effort → None"); + } + other => panic!("expected CatalogUpdated, got {other:?}"), + } // LC-8a: a Plan frame round-trips entries (status enum + optional priority). let plan = r#"{"Plan":{"entries":[{"content":"a","status":"InProgress","priority":"High"},{"content":"b","status":"Pending"}],"explanation":"why"}}"#; let ev: SessionEvent = serde_json::from_str(plan).expect("Plan deserializes"); From 5427f22f54febe55c00387eb18ec538da913dbc5 Mon Sep 17 00:00:00 2001 From: itsklimov Date: Fri, 24 Jul 2026 08:00:09 -0700 Subject: [PATCH 2/2] fix(session-port): seed thought-level defaults in direct CLI sessions Restores the thought-level startup semantics lost when Claude and Codex moved to the direct SessionAgentTask path in #609: the legacy ACP path's seed_startup_config_preferences (#574) was never ported, so a fresh chat with an assistant-default thinking level opened with no effort applied and a null current_value in the picker. - The initial effort resolves as persisted selection -> create-time config.thought_level (snapshot-wins, like mode/model). The persisted lookup accepts every legacy alias key the old ACP path stored raw (reasoning_effort/thought_level/thinking_budget/thinking; the canonical key wins), an EMPTY OR WHITESPACE-ONLY persisted value BLOCKS the default (legacy presence + trim parity), and the value is validated against the persisted-handshake preload catalog (drop-invalid; an unknown catalog stays permissive). - claude: the resolved seed rides the task runtime and is applied -- awaited -- by the FIRST send_message, strictly before the first prompt, exactly once; a manual switch invalidates a not-yet-delivered seed under a shared effort-ops gate. In-band control writes perform the turn-in-flight check and the write under the SAME stdin lock the prompt path uses (closes a TOCTOU that could land a control frame mid-turn). The optimistic effort tracker is PRE-STAMPED before the wire write and restored on a synchronous write error, so a fast rejection (a control_response arriving before dispatch returns) clears the tracker by-value instead of being overwritten by a post-write stamp. On a write error the latest-set tracker is restored to the PRIOR set it replaced (not cleared to None), so a late reject of an earlier still-pending set is still correlated and reconciled rather than lost. - codex: the seed is handed to the backend via SessionConfig.reasoning_effort and applied by a DETACHED post-open sequence (no first-turn gate): validated model reconcile (ACP clear_invalid_desired_* ported to the model/list catalog), a best-effort wait for the model settings response, then the effort settings write validated against the effective model. An invalid configured model is dropped with a corrected catalog push from BOTH the not-in-catalog and the empty/timeout branches (so a model/list arriving after the timeout cannot re-stamp the invalid seed). A failed effort settings write rolls the latest-set rpc id back to the prior value and removes its now-dead pending entry, mirroring claude so a late reject of the earlier set reconciles. - Catalog truth: CatalogUpdated carries the current model/mode/effort per the previous commit; claude always observes system/init's model; codex retains isDefault and defaultReasoningEffort from model/list and serves them as currents. A confirmed model switch (thread/settings/updated -> ConfigChanged) is followed by a re-emitted CatalogUpdated snapshot carrying the NEW model's OWN default effort, so the picker shows B's default rather than A's carried-over effort even when B also advertises it (the event pump resets the stale current on the switch). That follow-up snapshot omits the mode, so the pump PRESERVES the last-known current_mode when a CatalogUpdated carries current_mode: None (a missing field means "not carried", never "clear it"), keeping the mode confirmed by the preceding ConfigChanged. - Rejection reconcile: a backend error response for the LATEST effort set (ctl/rpc-id correlated; a superseded set's late reject is dropped whole) emits a structured config_option_rejected alongside the user-visible Notice. The event pump takes the SHARED effort-ops gate at the START of the reconcile -- the task holds that same gate across its optimistic dispatch+persist, so a reject racing the write reconciles strictly AFTER it -- then clears the optimistic highlight, removes the refused value from the persisted per-conversation selection, and re-pushes a corrected frame. Limitations (deliberate scope of this compact fix; a hardened transport-state-machine variant exists as a follow-up branch): - Config-option application is OPTIMISTIC: the set is recorded (and persisted per-conversation) immediately and corrected on an explicit backend rejection; a lost/absent response is not detected. - The codex effort seed is applied post-open with no first-turn gate -- the first codex turn may briefly run on launch defaults, matching the current upstream behaviour for model/mode. - The codex startup drain treats a JSON-RPC error on the model settings write the same as a success, so an explicit model-set error may still let the detached effort attempt run against the requested model; the backend stays authoritative and the reject signal corrects the effort. - A rejected interactive pick can briefly leave the GLOBAL cross-chat default (the assistant preference) pointing at the refused level until the next pick -- this matches the existing behaviour of the model and mode axes; the per-conversation selection is rolled back correctly. - A manual thinking-level pick racing the detached codex startup seed has no serialization gate on the wire -- last write wins (in practice the seed applies within milliseconds of open). - The write-error latest-set restore closes only the ORDERED case (set A written, set B write-errors synchronously, A rejected late). It does NOT disambiguate the case where a backend rejection is proven SIMULTANEOUSLY with that set's own synchronous write error -- which set the reject belongs to is genuinely ambiguous without a result-correlated two-phase send. That is left to the hardened transport-state-machine follow-up; here the backend stays authoritative and the next pick reconciles. --- crates/aionui-ai-agent/src/lib.rs | 1 + crates/aionui-ai-agent/src/session_agent.rs | 1792 ++++++++++++++--- crates/aionui-ai-agent/src/session_catalog.rs | 318 +++ .../aionui-session/src/backend/claude_conn.rs | 700 ++++++- .../aionui-session/src/backend/codex_conn.rs | 1036 +++++++++- crates/aionui-session/src/backend/mod.rs | 13 + 6 files changed, 3462 insertions(+), 398 deletions(-) create mode 100644 crates/aionui-ai-agent/src/session_catalog.rs diff --git a/crates/aionui-ai-agent/src/lib.rs b/crates/aionui-ai-agent/src/lib.rs index 78a8935ae..ea50f5377 100644 --- a/crates/aionui-ai-agent/src/lib.rs +++ b/crates/aionui-ai-agent/src/lib.rs @@ -25,6 +25,7 @@ pub(crate) mod runtime_status; pub mod runtime_token; pub(crate) mod services; pub mod session_agent; +pub(crate) mod session_catalog; pub mod session_context; pub mod shared_kernel; pub mod task_manager; diff --git a/crates/aionui-ai-agent/src/session_agent.rs b/crates/aionui-ai-agent/src/session_agent.rs index 25b128809..407965de5 100644 --- a/crates/aionui-ai-agent/src/session_agent.rs +++ b/crates/aionui-ai-agent/src/session_agent.rs @@ -36,6 +36,10 @@ use aionui_common::AgentType; use aionui_db::{IAcpSessionRepository, IMcpServerRepository, SaveRuntimeStateParams}; use aionui_realtime::EventBroadcaster; +use crate::session_catalog::{ + CatalogPreload, EFFORT_CONFIG_KEY, catalog_partial_from_caps, resolve_current_model_efforts, resolve_initial_effort, +}; + const EVENT_CHANNEL_CAPACITY: usize = 512; // Option ids for the generic tool-approval card. `confirm()` maps the incoming @@ -45,34 +49,6 @@ const PERM_ALLOW: &str = "allow"; const PERM_ALLOW_ALWAYS: &str = "allow_always"; const PERM_REJECT: &str = "reject"; -/// The `config_selections` key under which a claude session's chosen reasoning-effort -/// level is persisted. claude emits NO `ConfigChanged` for effort (only mode/model), so -/// `set_config_option` persists it here directly and `build_session_instance` re-applies -/// it after open (there is no spawn-time effort flag; it rides a post-open -/// control_request). The three accepted incoming option ids (`effort`/`reasoning_effort`/ -/// `thought_level`) all normalize to this one storage key. -const EFFORT_CONFIG_KEY: &str = "effort"; - -/// Resolve the reasoning-effort catalog to surface for the effort picker, mirroring the -/// backend's `effort_is_supported` current-model precedence: the efforts of the resolved -/// current model if it can be pinned, else the union across all advertised models (so we -/// don't hide a level some selectable model supports when the current model is ambiguous / -/// not-yet-known). Empty result = no effort axis → the caller omits the option entirely. -fn resolve_current_model_efforts(models: &[aionui_session::ModelInfo], current_model: Option<&str>) -> Vec { - if let Some(model) = current_model.and_then(|id| models.iter().find(|m| m.id == id)) { - return model.reasoning_efforts.clone(); - } - let mut union: Vec = Vec::new(); - for m in models { - for e in &m.reasoning_efforts { - if !union.contains(e) { - union.push(e.clone()); - } - } - } - union -} - /// Shared, cheaply-cloneable runtime state for a session task: the broadcast sender /// the translator writes and `subscribe()` reads, plus liveness bookkeeping. struct SessionRuntime { @@ -97,12 +73,30 @@ struct SessionRuntime { mode_override: std::sync::Mutex>, model_override: std::sync::Mutex>, /// Optimistic reasoning-effort ("thought level") selection, symmetric with - /// mode/model. claude emits NO `ConfigChanged`/echo for effort (unlike model/mode), - /// so the streaming catalog push — which runs in the backend-Arc-free event pump and - /// cannot read `capabilities().current_effort` — reads the highlight from here. REST - /// (`get_config_options`) prefers this over the (synchronously-seeded) caps value so - /// the observed re-read confirms the switch. `None` until the user picks a level. + /// mode/model. Neither backend emits a `ConfigChanged`/echo for effort (unlike + /// model/mode), so the streaming catalog push — which runs in the backend-Arc-free + /// event pump and cannot read `capabilities().current_effort` — reads the highlight + /// from here. REST (`get_config_options`) prefers this over the caps value so the + /// observed re-read confirms the switch. Seeded at build with the resolved initial + /// effort (persisted selection → assistant-default `thought_level`), overwritten by + /// interactive switches, and CLEARED by the pump's reconciles (catalog-arrival + /// invalidation / backend reject / failed initial dispatch) so it never advertises + /// a level the backend refused. effort_override: std::sync::Mutex>, + /// The resolved initial effort still WAITING to be applied to the backend. + /// Drained atomically by the FIRST `send_message` (startup barrier: the seed is + /// dispatched — and awaited — strictly before the first prompt, so the first + /// turn runs at the configured level; the old detached-spawn apply could lose + /// that race). INVALIDATED by a manual effort switch: `set_config_option` takes + /// it first, so a still-undelivered seed can never overwrite the user's pick. + /// `None` = nothing pending (no seed, already drained, or invalidated). + pending_startup_effort: std::sync::Mutex>, + /// Serializes every effort-affecting op: the startup-seed drain, a manual + /// switch's invalidate+persist, AND the pump's reject rollback RMW on the + /// persisted selection. Held by the RUNTIME (not the task) so the event pump + /// — which owns only an `Arc` — can reach it and cannot + /// clobber a concurrently persisted newer pick with a stale map. + effort_ops_gate: tokio::sync::Mutex<()>, } impl SessionRuntime { @@ -146,74 +140,29 @@ impl SessionRuntime { fn effort_override(&self) -> Option { self.effort_override.lock().ok().and_then(|g| g.clone()) } -} - -/// Cold-start catalog snapshot extracted from a persisted `agent_metadata` -/// handshake, in the SAME `aionui_session` shape the getters read off live -/// `capabilities()` — so serving the preload is a drop-in fallback with no shape -/// translation at read time. Empty vectors + `None` currents = nothing persisted. -#[derive(Default, Clone)] -struct CatalogPreload { - available_models: Vec, - current_model: Option, - available_modes: Vec, - current_mode: Option, -} - -impl CatalogPreload { - /// Parse the persisted handshake's `available_models` / `available_modes` - /// columns into the live-capabilities shape. Reuses the ACP path's - /// `extract_models_from_value` / `extract_modes_from_value` (the same - /// multi-shape parser that accepts both the `{available_models:[{id,label}]}` - /// column shape `spawn_catalog_writeback` persists AND a live-claude handshake), - /// so the two paths stay byte-compatible. `reasoning_efforts` is intentionally - /// dropped: the handshake catalog does not carry per-model efforts, and the - /// getters this feeds do not surface efforts. - fn from_handshake(handshake: &aionui_api_types::AgentHandshake) -> Self { - use crate::manager::acp::config_option_catalog::{extract_models_from_value, extract_modes_from_value}; - let (available_models, current_model) = handshake - .available_models - .as_ref() - .and_then(extract_models_from_value) - .map(|state| { - let models = state - .available_models - .iter() - .map(|m| aionui_session::ModelInfo { - id: m.model_id.to_string(), - name: m.name.clone(), - description: m.description.clone(), - reasoning_efforts: Vec::new(), - }) - .collect::>(); - let current = state.current_model_id.to_string(); - (models, (!current.is_empty()).then_some(current)) - }) - .unwrap_or_default(); - let (available_modes, current_mode) = handshake - .available_modes - .as_ref() - .and_then(extract_modes_from_value) - .map(|state| { - let modes = state - .available_modes - .iter() - .map(|m| aionui_session::ModeInfo { - id: m.id.to_string(), - name: m.name.clone(), - description: m.description.clone(), - }) - .collect::>(); - let current = state.current_mode_id.to_string(); - (modes, (!current.is_empty()).then_some(current)) - }) - .unwrap_or_default(); - Self { - available_models, - current_model, - available_modes, - current_mode, + /// Startup-barrier drain: take the pending seed (exactly-once). Called by + /// `send_message` before the first prompt. + fn take_startup_effort(&self) -> Option { + self.pending_startup_effort.lock().ok().and_then(|mut g| g.take()) + } + /// Manual-switch invalidation: a user pick supersedes an undelivered seed. + fn invalidate_startup_effort(&self) { + if let Ok(mut g) = self.pending_startup_effort.lock() { + *g = None; + } + } + /// Reconcile-on-reject / catalog-arrival reconcile: drop the optimistic effort + /// highlight IF it still holds `value` (a later switch may have overwritten it — + /// only a matching value is cleared). Returns whether a clear happened, so the + /// pump knows to re-push a corrected config-options frame. + fn clear_effort_override_if(&self, value: &str) -> bool { + if let Ok(mut g) = self.effort_override.lock() + && g.as_deref() == Some(value) + { + *g = None; + return true; } + false } } @@ -287,6 +236,13 @@ pub struct SessionAgentTask { /// paths with no persisted catalog (fresh agent, tests). Mirrors the ACP path's /// `preload_advertised_catalogs` "fill-when-empty, live-overwrites" semantics. catalog_preload: CatalogPreload, + /// Serializes every effort-affecting task operation — the startup-seed drain + /// (take + dispatch) in `send_message` and a manual effort switch + /// (invalidate + dispatch) in `set_config_option` — so their steps can never + /// interleave: without it, a first send could TAKE the seed while a manual + /// pick is mid-dispatch, landing the stale seed on the wire AFTER the pick + /// (wire ends on the seed, runtime/DB on the pick). The backend stdin lock + /// only orders individual writes; this gate orders the task-level intent. /// Command-id counter for `CommandMeta` (dispatch correlation). command_seq: AtomicI64, /// Resolved prompt-dump target (see [`SessionPromptDump`]). `None` when @@ -320,6 +276,8 @@ impl SessionAgentTask { session_repo, CatalogPreload::default(), None, + None, + None, ) } @@ -337,6 +295,8 @@ impl SessionAgentTask { session_repo: Option>, handshake: &aionui_api_types::AgentHandshake, prompt_dump: Option, + initial_effort: Option, + startup_effort: Option, ) -> Arc { Self::build( agent_type, @@ -346,9 +306,12 @@ impl SessionAgentTask { session_repo, CatalogPreload::from_handshake(handshake), prompt_dump, + initial_effort, + startup_effort, ) } + #[allow(clippy::too_many_arguments)] fn build( agent_type: AgentType, conversation_id: String, @@ -357,6 +320,8 @@ impl SessionAgentTask { session_repo: Option>, catalog_preload: CatalogPreload, prompt_dump: Option, + initial_effort: Option, + startup_effort: Option, ) -> Arc { let (tx, _rx) = broadcast::channel(EVENT_CHANNEL_CAPACITY); let runtime = Arc::new(SessionRuntime { @@ -366,7 +331,19 @@ impl SessionAgentTask { session_id: std::sync::Mutex::new(None), mode_override: std::sync::Mutex::new(None), model_override: std::sync::Mutex::new(None), - effort_override: std::sync::Mutex::new(None), + // Seeded with the resolved initial effort (persisted config_selections + // effort → else create-time config.thought_level) so REST + // `get_config_options` AND the CatalogUpdated push surface a non-null + // current_value from the first frame — without this the UI pill shows only + // the model until the user manually picks a level. + effort_override: std::sync::Mutex::new(initial_effort), + // The value queued for the startup-barrier apply (drained by the first + // send, invalidated by a manual switch). `None` when the BACKEND owns + // the startup apply (codex: the DETACHED post-open sequence applies the + // seed via `SessionConfig.reasoning_effort`, so a task-level re-apply + // would double-write). + pending_startup_effort: std::sync::Mutex::new(startup_effort), + effort_ops_gate: tokio::sync::Mutex::new(()), }); // Subscribe to the backend's event stream HERE (sync), then hand ONLY the // stream to the pump — never a backend Arc (see `spawn_event_pump` for why @@ -709,6 +686,19 @@ impl SessionAgentTask { // capabilities snapshot may simply not have the list yet). Only a NON-empty // catalog that omits `value` rejects. Other option ids (effort/thought_level) // are validated by the backend itself (claude effort catalog check). + // Effort aliases: take the effort-ops gate for the WHOLE manual switch and + // invalidate the pending startup seed BEFORE the first await point — the + // user's intent supersedes the seed the moment the switch is requested. The + // gate serializes this dispatch against the first send's seed drain, so the + // wire can never end on a stale seed written after the pick. + let is_effort_alias = matches!(option_id, "effort" | "reasoning_effort" | "thought_level"); + let _effort_gate = if is_effort_alias { + let gate = self.runtime.effort_ops_gate.lock().await; + self.runtime.invalidate_startup_effort(); + Some(gate) + } else { + None + }; let caps = self.backend.capabilities(); // A NON-empty catalog that omits `value` is the only rejection case (empty // catalog = permissive, per the comment above). `known` = catalog carries value. @@ -769,15 +759,19 @@ impl SessionAgentTask { "mode" => self.runtime.set_mode_override(value.to_string()), "model" => self.runtime.set_model_override(value.to_string()), "effort" | "reasoning_effort" | "thought_level" => { + // (The pending startup seed was already invalidated — before dispatch, + // under the effort-ops gate — at the top of this function.) // Optimistic highlight: claude emits no effort echo, so the streaming // catalog push reads the current level from this override. self.runtime.set_effort_override(value.to_string()); // Persist the chosen effort into `config_selections` so it survives a // respawn/resume. Unlike mode/model (persisted by the pump on // ConfigChanged), claude emits no ConfigChanged for effort, so this is - // the ONLY place the choice is durably recorded. Backend already accepted - // + validated it (dispatch above); best-effort persist (a DB failure must - // not fail the switch the CLI already applied). + // the ONLY place the choice is durably recorded. We optimistically record + // it after the adapter ACCEPTED the dispatched command (above) — not proof + // the CLI validated/applied the level; an explicit reject on the wire is + // compensated by the pump (reject → unpersist). Best-effort persist: a DB + // failure must not fail a switch the adapter already accepted. self.persist_effort(value).await; } _ => { @@ -791,7 +785,6 @@ impl SessionAgentTask { // Effort is emitted under the canonical id `reasoning_effort` (category // `thought_level`); a caller may address it via any of its aliases, so match by // category for the effort axis and by id otherwise. - let is_effort_alias = matches!(option_id, "effort" | "reasoning_effort" | "thought_level"); let observed = snapshot .config_options .iter() @@ -818,8 +811,10 @@ impl SessionAgentTask { /// [`EFFORT_CONFIG_KEY`]) so it survives a respawn/resume. Reads the existing /// selections first and MERGES (rather than overwriting the whole map) so any other /// future config key is preserved. Best-effort: a repo miss/failure is logged, not - /// propagated — the backend already applied the effort, and losing only the - /// persistence (not the live switch) is the safe degradation. No-op without a repo. + /// propagated — the record is written optimistically after the adapter accepted the + /// command (an explicit wire reject is compensated by the pump's unpersist), and + /// losing only the persistence (not the live switch) is the safe degradation. No-op + /// without a repo. async fn persist_effort(&self, value: &str) { let Some(repo) = self.session_repo.as_ref() else { return; @@ -924,6 +919,37 @@ impl IAgentTask for SessionAgentTask { // Command::Send. No-op / best-effort — never affects the dispatch. self.dump_session_cli_final_input(&content, Some(data.msg_id.as_str())); + // Startup barrier: apply the still-pending initial effort BEFORE the first + // prompt, awaited — so the first turn runs at the configured level instead of + // racing a detached apply. The take AND the dispatch happen under the + // effort-ops gate, serialized against a concurrent manual switch (which + // invalidates the seed under the same gate before its own dispatch) — the + // wire can therefore never end on a stale seed written after a user pick. + // The backend validates the value; a dispatch failure rolls the optimistic + // highlight back and never fails the send (the session is usable, only the + // seed is lost). + { + let _gate = self.runtime.effort_ops_gate.lock().await; + if let Some(effort) = self.runtime.take_startup_effort() { + match self + .backend + .dispatch(Command::SetConfigOption { + option_id: EFFORT_CONFIG_KEY.to_owned(), + value: effort.clone(), + }) + .await + { + Ok(_) => { + tracing::info!(conv_id = %self.conversation_id, effort = %effort, "session-port: applied initial reasoning effort before first prompt"); + } + Err(e) => { + self.runtime.clear_effort_override_if(&effort); + tracing::warn!(conv_id = %self.conversation_id, effort = %effort, error = %e, "session-port: applying initial effort failed (send proceeds, effort not applied)"); + } + } + } + } + let cmd = Command::Send { content, metadata: CommandMeta { @@ -1256,23 +1282,44 @@ pub async fn build_session_instance( } } - // #4 — the persisted reasoning-effort level (claude only). There is no spawn-time - // effort flag (effort rides a post-open control_request, NOT `--`args like - // model/mode), so it cannot go into `SessionConfig`; instead we re-apply it AFTER - // open. codex effort is not a standalone selection (it rides collaborationMode via - // SetMode), so this is claude-scoped. Read from the snapshot's config_selections - // (the map `set_config_option` persisted under EFFORT_CONFIG_KEY). - let persisted_effort = (backend_label == "claude") - .then(|| { - session_snapshot.and_then(|s| { - s.config_selections - .iter() - .find(|(k, _)| k.as_str() == EFFORT_CONFIG_KEY) - .map(|(_, v)| v.as_str().to_owned()) - }) - }) - .flatten() - .filter(|s| !s.is_empty()); + // #4 — the initial reasoning-effort level. There is no spawn-time effort flag, + // and the two backends take DIFFERENT seed paths: + // * claude has no `SessionConfig` effort slot — the seed rides a post-open + // control_request, dispatched by the task-level first-send drain (below). + // * codex DOES carry the seed on the session config: `SessionConfig.reasoning_effort` + // (set below, codex-only) is applied inside codex's detached post-open sequence + // via `thread/settings/update{effort}` — verified codex_conn.rs `SetConfigOption` + // arm against samples/codex-cli/0.137.0/schema-full/ClientRequest.json + // ThreadSettingsUpdateParams. + // Historically this read ONLY the + // persisted selection and ONLY for claude ("codex effort is not standalone" — + // stale: codex has had a first-class `thread/settings/update{effort}` wire since + // the port, see the dispatch arm), and `config.thought_level` was ignored + // entirely, so a fresh chat with an assistant-default thinking level opened with + // no effort applied and a null current_value in the picker (the #609 regression + // against the #574 defaults). + // + // The seed is validated against the best catalog knowledge available at open — + // the persisted-handshake preload (the live capabilities are empty until the + // async initialize/model-list lands) — mirroring the legacy path's + // `pending_startup_config` drop-invalid semantics. An unknown catalog is + // permissive; the pump's catalog-arrival + reject reconciles cover that window. + let catalog_preload = CatalogPreload::from_handshake(&metadata.handshake); + let known_efforts = resolve_current_model_efforts( + &catalog_preload.available_models, + session_snapshot + .and_then(|s| s.current_model_id.as_ref().map(|m| m.as_str().to_owned())) + .or_else(|| config.current_model_id.clone()) + .as_deref(), + ); + let initial_effort = resolve_initial_effort(session_snapshot, config, &known_efforts); + // codex applies the seed itself, inside its DETACHED post-open sequence + // (validated model reconcile → best-effort model-response wait → validated + // effort; NO first-turn gate) — hand it over via the session config. claude + // keeps the task-level first-send drain (stdin ordering is its contract). + if backend_label == "codex" { + session_config.reasoning_effort = initial_effort.clone(); + } // DEV (`--dump-prompts`): dump the resolved SessionConfig BEFORE it moves // into open_session. Best-effort — a failure only warns, never fails open. @@ -1327,25 +1374,6 @@ pub async fn build_session_instance( e => AgentError::bad_gateway(format!("open {backend_label} session: {e}")), })?; - // Re-apply the persisted effort now that the session is open. The backend validates - // it against the current model's advertised catalog (permissive until the catalog - // is discovered) and drops it if unsupported — the same clear_invalid_desired_* - // semantics as the codex model/mode reconcile. Best-effort: a dispatch failure must - // not fail the open (the session is usable; only the persisted effort is lost). - if let Some(effort) = persisted_effort { - if let Err(e) = backend - .dispatch(Command::SetConfigOption { - option_id: EFFORT_CONFIG_KEY.to_owned(), - value: effort.clone(), - }) - .await - { - tracing::warn!(conv_id = %conversation_id, effort = %effort, error = %e, "session-port: re-applying persisted effort failed (session usable, effort not restored)"); - } else { - tracing::info!(conv_id = %conversation_id, effort = %effort, "session-port: re-applied persisted reasoning effort after open"); - } - } - // GAP #7 (G5): project the backend's discovered catalog back into agent_metadata // so the cold-start picker stays fresh. Best-effort, detached, off the open path. if let Some((agent_id, catalog_tx)) = catalog_writeback { @@ -1367,7 +1395,32 @@ pub async fn build_session_instance( acp_session_repo, &metadata.handshake, prompt_dump, + // The runtime's optimistic effort highlight (REST and the CatalogUpdated + // push report one consistent non-null current_value from the first frame). + initial_effort.clone(), + // The task-level startup seed the first send drains — claude only: codex's + // seed was handed to the backend via `SessionConfig.reasoning_effort` and + // is applied inside its DETACHED post-open sequence, which a task-level + // re-apply would double-write. + if backend_label == "claude" { + initial_effort + } else { + None + }, ); + + // The first-send startup barrier is CLAUDE-ONLY (the seed above is `None` for + // codex). For claude, the resolved initial effort is NOT dispatched here: it rides + // the task runtime as a pending startup seed and is applied — awaited — by the + // FIRST `send_message`, strictly before the first prompt (the startup barrier). + // This guarantees claude's first turn runs at the seeded level (the old detached + // apply could lose that race) and lets a manual switch invalidate a not-yet-delivered + // seed. Codex takes the OTHER path entirely: its seed rode `SessionConfig` and is + // applied by the detached post-open bootstrap sequence — there is NO first-turn gate + // on codex, so its first prompt may race the seed apply (last wire write wins, + // reconciled by the reader's structured reject). Until the drain (claude) or the + // detached apply (codex), REST and the catalog push already highlight the seed via + // the runtime override. Ok(Some(crate::agent_task::AgentInstance::Session(task))) } @@ -1550,85 +1603,6 @@ pub fn spawn_catalog_writeback( }); } -/// Project a backend's discovered `Capabilities` (modes / models / slash commands) -/// into an `AgentHandshake` partial for the `agent_metadata` catalog. Verbatim port -/// of clean-slate `session_runtime::catalog_partial_from_caps`: emits both the ACP -/// `config_options[]` wire shape AND the top-level `available_modes`/`available_models` -/// columns directly (the shape-stable path that keeps the codex model picker from -/// going empty). -fn catalog_partial_from_caps(caps: &aionui_session::Capabilities) -> Option { - let mut config_options = Vec::new(); - if !caps.available_modes.is_empty() { - config_options.push(serde_json::json!({ - "id": "mode", - "category": "mode", - "type": "select", - "currentValue": caps.current_mode, - "options": caps.available_modes.iter().map(|m| serde_json::json!({ - "value": m.id, "name": m.name, "description": m.description, - })).collect::>(), - })); - } - if !caps.available_models.is_empty() { - config_options.push(serde_json::json!({ - "id": "model", - "category": "model", - "type": "select", - "currentValue": caps.current_model, - "options": caps.available_models.iter().map(|m| serde_json::json!({ - "value": m.id, "name": m.name, "description": m.description, - })).collect::>(), - })); - } - let available_commands = if caps.slash_commands.is_empty() { - None - } else { - Some(serde_json::json!( - caps.slash_commands - .iter() - .map(|c| serde_json::json!({ - "name": c.name, "description": c.description, - })) - .collect::>() - )) - }; - if config_options.is_empty() && available_commands.is_none() { - return None; - } - let config_options = if config_options.is_empty() { - None - } else { - Some(serde_json::Value::Array(config_options)) - }; - // Also project the top-level `available_modes`/`available_models` fields directly - // (shape: `{available_models:[{id,label}]}`), which `apply_handshake` persists to - // the catalog columns VERBATIM — the authoritative, shape-stable path (matches what - // a live claude handshake stores), so the codex model picker never goes empty. - let available_modes = (!caps.available_modes.is_empty()).then(|| { - serde_json::json!({ - "available_modes": caps.available_modes.iter().map(|m| serde_json::json!({ - "id": m.id, "name": m.name, "description": m.description, - })).collect::>(), - "current_mode_id": caps.current_mode, - }) - }); - let available_models = (!caps.available_models.is_empty()).then(|| { - serde_json::json!({ - "available_models": caps.available_models.iter().map(|m| serde_json::json!({ - "id": m.id, "label": m.name, - })).collect::>(), - "current_model_id": caps.current_model, - }) - }); - Some(aionui_api_types::AgentHandshake { - config_options, - available_modes, - available_models, - available_commands, - ..Default::default() - }) -} - /// Map a conversation's requested mode → the codex `thread/start.sandbox` string /// (`SandboxMode`: `read-only` / `workspace-write` / `danger-full-access`, verified /// `codex-cli/0.137.0/schema-full/ClientRequest.json` §SandboxMode), or `None` to keep @@ -1721,6 +1695,159 @@ fn session_event_name(e: &SessionEvent) -> &'static str { } /// Drain the backend's `events()` and re-broadcast each as an `AgentStreamEvent`. +/// The pump's retained copy of the latest `CatalogUpdated` payload: the option +/// lists plus the backend-reported currents that rode the event. Needed because +/// the pump deliberately holds NO backend Arc (see `spawn_event_pump`) — this is +/// its only material for rebuilding a config-options frame outside a catalog push +/// (the `config_option_rejected` reconcile). +struct LastCatalog { + models: Vec, + modes: Vec, + current_model: Option, + current_mode: Option, + current_effort: Option, +} + +/// Project a catalog (+ currents) into the `AcpConfigOption` push frame. Per axis +/// the current highlight is the task-runtime's optimistic override (the user's +/// latest interactive switch / the open-time effort seed) falling back to the +/// backend-reported current from the event — the same precedence +/// `get_config_options` (REST) applies over `capabilities()`, so push and REST +/// stay one consistent source of truth. `None` = nothing to push (both lists +/// empty; an empty-snapshot frame would only clobber the frontend's picker). +fn build_catalog_frame(runtime: &SessionRuntime, catalog: &LastCatalog) -> Option { + let mut config_options: Vec = Vec::new(); + if !catalog.modes.is_empty() { + config_options.push(aionui_api_types::AcpConfigOptionDto { + id: "mode".into(), + name: Some("Mode".into()), + label: None, + description: None, + category: Some("mode".into()), + option_type: "select".into(), + current_value: runtime.mode_override().or_else(|| catalog.current_mode.clone()), + options: catalog + .modes + .iter() + .map(|m| aionui_api_types::AcpConfigSelectOptionDto { + value: m.id.clone(), + name: Some(m.name.clone()), + label: None, + description: m.description.clone(), + }) + .collect(), + }); + } + if !catalog.models.is_empty() { + config_options.push(aionui_api_types::AcpConfigOptionDto { + id: "model".into(), + name: Some("Model".into()), + label: None, + description: None, + category: Some("model".into()), + option_type: "select".into(), + current_value: runtime.model_override().or_else(|| catalog.current_model.clone()), + options: catalog + .models + .iter() + .map(|m| aionui_api_types::AcpConfigSelectOptionDto { + value: m.id.clone(), + name: Some(m.name.clone()), + label: None, + description: m.description.clone(), + }) + .collect(), + }); + } + // Reasoning-effort axis (claude `supportedEffortLevels` / codex + // `supportedReasoningEfforts`). The frontend REPLACES its whole config-options + // snapshot on this frame, so effort MUST ride along or a late catalog push would + // wipe the effort option REST surfaced. Emitted only when the effective current + // model advertises efforts (union fallback when the current model is unknown). + let effective_model = runtime.model_override().or_else(|| catalog.current_model.clone()); + let efforts = resolve_current_model_efforts(&catalog.models, effective_model.as_deref()); + if !efforts.is_empty() { + config_options.push(aionui_api_types::AcpConfigOptionDto { + id: "reasoning_effort".into(), + name: Some("Thinking".into()), + label: None, + description: None, + category: Some("thought_level".into()), + option_type: "select".into(), + // The observed fallback is validated against the resolved efforts: a + // current the effective model does not advertise must not render (it + // would be its own lie — e.g. a stale current from a pre-switch model). + current_value: runtime + .effort_override() + .or_else(|| catalog.current_effort.clone()) + .filter(|e| efforts.iter().any(|x| x == e)), + options: efforts + .iter() + .map(|e| aionui_api_types::AcpConfigSelectOptionDto { + value: e.clone(), + name: Some(e.clone()), + label: None, + description: None, + }) + .collect(), + }); + } + if config_options.is_empty() { + return None; + } + serde_json::to_value(serde_json::json!({ "config_options": config_options })).ok() +} + +/// Round-11 (minimal-branch reject correctness): remove the REJECTED effort +/// value from the persisted `config_selections` so the next open does not +/// re-seed a level the backend refused. Mirrors `persist_effort` but retains +/// out any legacy-alias key holding the rejected value. Runs under the caller's +/// shared effort-ops gate so it cannot clobber a concurrently persisted newer +/// pick. Best-effort: a DB error only warns. +async fn unpersist_rejected_effort(repo: &dyn IAcpSessionRepository, conversation_id: &str, rejected: &str) { + let mut selections: std::collections::HashMap = match repo.load_runtime_state(conversation_id).await + { + Ok(Some(state)) => state + .config_selections_json + .as_deref() + .and_then(|raw| serde_json::from_str(raw).ok()) + .unwrap_or_default(), + Ok(None) => return, + Err(err) => { + tracing::warn!(conversation_id = %conversation_id, error = %err, "reject rollback: load_runtime_state failed"); + return; + } + }; + let before = selections.len(); + // Round-12 P1-3: compare TRIMMED — a persisted value with surrounding + // whitespace (` high `) is the same selection as the rejected `high` and + // must not survive the reject (resolve_initial_effort trims too, so an + // untrimmed survivor would re-seed the refused level on the next open). + let rejected_trimmed = rejected.trim(); + selections.retain(|key, value| { + !(crate::session_catalog::EFFORT_ALIAS_KEYS.contains(&key.as_str()) && value.trim() == rejected_trimmed) + }); + if selections.len() == before { + return; + } + let json = match serde_json::to_string(&selections) { + Ok(j) => j, + Err(err) => { + tracing::warn!(conversation_id = %conversation_id, error = %err, "reject rollback: encode config_selections failed"); + return; + } + }; + let params = SaveRuntimeStateParams { + config_selections_json: Some(Some(&json)), + ..Default::default() + }; + if let Err(err) = repo.save_runtime_state(conversation_id, ¶ms).await { + tracing::warn!(conversation_id = %conversation_id, error = %err, "reject rollback: save_runtime_state failed"); + } else { + tracing::info!(conversation_id = %conversation_id, effort = %rejected, "reject rollback: rejected effort removed from persisted config_selections"); + } +} + fn spawn_event_pump( mut events: BoxStream<'static, SessionEnvelope>, runtime: Arc, @@ -1790,6 +1917,11 @@ fn spawn_event_pump( // clean shutdown) would be misread as a mid-turn crash. Set on the terminal // TurnResult, reset on the next TurnStarted. let mut terminal_result_seen = false; + // The latest discovered catalog (+ the backend-reported currents that rode + // its CatalogUpdated), kept so a later `config_option_rejected` reconcile can + // re-push a corrected config-options frame — the pump owns no backend Arc, so + // this is its only source for rebuilding the snapshot. + let mut last_catalog: Option = None; while let Some(env) = events.next().await { runtime.touch(); tracing::debug!(conv_id = %conversation_id, event = session_event_name(&env.event), "session-pump: backend event"); @@ -1837,87 +1969,50 @@ fn spawn_event_pump( models, modes, slash_commands, - // The additive current_model/current_mode/current_effort fields are - // consumed by the follow-up commit (highlight precedence); tolerated - // here so this commit builds standalone. - .. + current_model, + current_mode, + current_effort, } = &env.event { - let mut config_options: Vec = Vec::new(); - if !modes.is_empty() { - config_options.push(aionui_api_types::AcpConfigOptionDto { - id: "mode".into(), - name: Some("Mode".into()), - label: None, - description: None, - category: Some("mode".into()), - option_type: "select".into(), - current_value: runtime.mode_override(), - options: modes - .iter() - .map(|m| aionui_api_types::AcpConfigSelectOptionDto { - value: m.id.clone(), - name: Some(m.name.clone()), - label: None, - description: m.description.clone(), - }) - .collect(), - }); - } - if !models.is_empty() { - config_options.push(aionui_api_types::AcpConfigOptionDto { - id: "model".into(), - name: Some("Model".into()), - label: None, - description: None, - category: Some("model".into()), - option_type: "select".into(), - current_value: runtime.model_override(), - options: models - .iter() - .map(|m| aionui_api_types::AcpConfigSelectOptionDto { - value: m.id.clone(), - name: Some(m.name.clone()), - label: None, - description: m.description.clone(), - }) - .collect(), - }); - } - // Reasoning-effort axis (claude per-model `supportedEffortLevels`). The - // frontend REPLACES its whole config-options snapshot on this frame, so we - // MUST re-emit effort here too — otherwise a late catalog push would wipe - // the effort option that `get_config_options` (REST) surfaced. The pump has - // no backend Arc, so the current model is resolved from the pushed catalog - // and the highlight comes from the runtime's optimistic effort override - // (claude emits no effort echo). Emitted only when the current model - // advertises efforts (union fallback when the current model is unknown). - let efforts = resolve_current_model_efforts(models, runtime.model_override().as_deref()); - if !efforts.is_empty() { - config_options.push(aionui_api_types::AcpConfigOptionDto { - id: "reasoning_effort".into(), - name: Some("Thinking".into()), - label: None, - description: None, - category: Some("thought_level".into()), - option_type: "select".into(), - current_value: runtime.effort_override(), - options: efforts - .iter() - .map(|e| aionui_api_types::AcpConfigSelectOptionDto { - value: e.clone(), - name: Some(e.clone()), - label: None, - description: None, - }) - .collect(), - }); - } - // No categories (both lists empty) → nothing to re-project; a spurious - // empty-snapshot frame would only clobber the frontend's picker. - if !config_options.is_empty() - && let Ok(v) = serde_json::to_value(serde_json::json!({ "config_options": config_options })) + // Catalog-arrival reconcile for the seeded effort highlight: the seed + // was validated against the (possibly stale/absent) persisted preload; + // the LIVE catalog is authoritative. A non-empty live effort list that + // omits the optimistic override falsifies it → clear, so this push and + // every later REST read fall back to the backend-reported current + // instead of advertising a level the model cannot run. + let effective_model = runtime.model_override().or_else(|| current_model.clone()); + let efforts = resolve_current_model_efforts(models, effective_model.as_deref()); + if let Some(override_effort) = runtime.effort_override() + && !efforts.is_empty() + && !efforts.iter().any(|e| e == &override_effort) + && runtime.clear_effort_override_if(&override_effort) { + tracing::warn!( + conv_id = %conversation_id, + effort = %override_effort, + "session-pump: seeded effort is not in the live catalog; highlight cleared" + ); + } + // Remember the latest catalog (+ its backend-reported currents) so a + // later `config_option_rejected` can re-push a corrected frame without + // a backend Arc. + // Round-12 P1-2: a CatalogUpdated that carries `current_mode: None` + // means "this frame has no mode info", NOT "clear the mode". The + // model-switch follow-up snapshot (and the discovery/invalid-model + // pushes) omit the mode, so REPLACING it with None would wipe the + // mode confirmed by the preceding ConfigChanged. Preserve the + // last-known mode when the event omits it. + let merged_mode = current_mode + .clone() + .or_else(|| last_catalog.as_ref().and_then(|c| c.current_mode.clone())); + last_catalog = Some(LastCatalog { + models: models.clone(), + modes: modes.clone(), + current_model: current_model.clone(), + current_mode: merged_mode, + current_effort: current_effort.clone(), + }); + if let Some(v) = build_catalog_frame(&runtime, last_catalog.as_ref().unwrap()) { let _ = runtime.tx.send(AgentStreamEvent::AcpConfigOption(v)); } // Slash-command catalog. claude advertises its command list in the @@ -1947,6 +2042,63 @@ fn spawn_event_pump( continue; } + // Reconcile-on-reject: the backend refused a config-option set (claude + // `sniff_set_config_reject` / the codex `thread/settings/update` error + // claim — both emit this structured tag alongside their user-facing + // Notice, and only for the LATEST set, never a superseded one). Clear + // every trace of the refused value — the optimistic runtime highlight AND + // the retained catalog's current (a codex catalog default can never BE + // the refused value, but scrubbing by value keeps the invariant local) — + // then re-push a corrected frame UNCONDITIONALLY: even when the runtime + // override was already cleared by an earlier catalog-arrival reconcile, + // the frontend may still be rendering the refused value from the last + // pushed frame, and only a fresh push corrects it. + if let SessionEvent::AdapterSpecific { tag, payload } = &env.event + && tag == "config_option_rejected" + { + let option_id = payload.get("option_id").and_then(serde_json::Value::as_str); + let value = payload.get("value").and_then(serde_json::Value::as_str); + if option_id == Some("effort") + && let Some(value) = value + { + // Round-11 (current-chat correctness): take the SHARED + // effort-ops gate at the VERY START of the reconcile, BEFORE + // clearing the runtime override / catalog / persisted + // selection. The task's `set_config_option` holds this same + // gate across its optimistic dispatch+persist, so a FAST + // reject that arrives mid-dispatch blocks HERE until the task + // has finished writing — our clear+unpersist then runs AFTER + // the optimistic write, leaving runtime/DB clean. Clearing + // before the gate would let the task re-set the override and + // re-persist the refused value after our scrub. + let _gate = runtime.effort_ops_gate.lock().await; + let cleared = runtime.clear_effort_override_if(value); + if let Some(catalog) = last_catalog.as_mut() + && catalog.current_effort.as_deref() == Some(value) + { + catalog.current_effort = None; + } + // The refused value must also leave the persisted selection, + // or the next open re-seeds it (resolve_initial_effort reads + // config_selections first). Same gate. + if let Some(repo) = session_repo.as_ref() { + unpersist_rejected_effort(repo.as_ref(), &conversation_id, value).await; + } + tracing::info!( + conv_id = %conversation_id, + effort = %value, + override_cleared = cleared, + "session-pump: backend rejected the effort set; highlight reconciled" + ); + if let Some(catalog) = last_catalog.as_ref() + && let Some(v) = build_catalog_frame(&runtime, catalog) + { + let _ = runtime.tx.send(AgentStreamEvent::AcpConfigOption(v)); + } + } + continue; + } + // Track in-flight workflow/subagent refs so a non-blocking Workflow's // intermediate `result` frame does not prematurely terminate the turn. // Mirrors `state::background_active`: a ref is in-flight while its status @@ -2069,6 +2221,52 @@ fn spawn_event_pump( if let Some(repo) = session_repo.as_ref() { persist_side_effects(repo.as_ref(), &conversation_id, &env.event).await; } + + // A confirmed mode/model switch (startup reconcile or interactive) updates + // the retained catalog's currents and re-pushes the config-options frame, + // so the picker highlight follows the switch without waiting for the next + // catalog discovery. This is what keeps a CONFIGURED codex session honest: + // the startup reconcile's SetModel lands as ConfigChanged after the first + // CatalogUpdated, and without this re-push the frontend would keep the + // pre-reconcile highlight. A model change also re-validates the effort + // highlight — a level the new model does not advertise is cleared (same + // catalog-arrival semantics as above). + if let SessionEvent::ConfigChanged { mode, model } = &env.event + && let Some(catalog) = last_catalog.as_mut() + { + if let Some(mode) = mode { + catalog.current_mode = Some(mode.clone()); + } + if let Some(model) = model { + catalog.current_model = Some(model.clone()); + // Round-11: the OBSERVED effort current belonged to the PREVIOUS + // model — reset it UNCONDITIONALLY on a model switch, even to a + // value the new model also supports (A default=medium -> B + // default=high would otherwise keep showing medium while codex + // runs B at high). The backend's follow-up catalog push + // re-supplies the NEW model's own default effort. + catalog.current_effort = None; + // A user's explicit override that the new model does not advertise + // is also cleared (an override the new model DOES support stays — + // it is the user's intent, re-applied by the picker). + let efforts = resolve_current_model_efforts(&catalog.models, Some(model.as_str())); + if !efforts.is_empty() + && let Some(override_effort) = runtime.effort_override() + && !efforts.iter().any(|e| e == &override_effort) + && runtime.clear_effort_override_if(&override_effort) + { + tracing::warn!( + conv_id = %conversation_id, + effort = %override_effort, + model = %model, + "session-pump: model switch invalidates the effort highlight; cleared" + ); + } + } + if let Some(v) = build_catalog_frame(&runtime, catalog) { + let _ = runtime.tx.send(AgentStreamEvent::AcpConfigOption(v)); + } + } for mut ev in translate_event(env.event, &conversation_id, terminal_result_seen) { // Keep the tool name alive across a call's multi-frame lifecycle (see // `stamp_tool_name`): the terminal ToolResult frame leaves the name @@ -2700,6 +2898,202 @@ mod build_mapping_tests { } } + fn snapshot_with_effort(effort: &str) -> PersistedSessionState { + use crate::shared_kernel::{ConfigKey, ConfigValue}; + let mut s = PersistedSessionState::default(); + s.config_selections + .insert(ConfigKey::new(EFFORT_CONFIG_KEY), ConfigValue::new(effort)); + s + } + + fn extra_with_thought_level(level: Option<&str>) -> AcpBuildExtra { + AcpBuildExtra { + thought_level: level.map(str::to_string), + ..Default::default() + } + } + + // ── resolve_initial_effort: the seed precedence + validation ──────────── + + // Fresh session (no snapshot): the assistant-default `config.thought_level` + // resolved at create time IS the initial effort — the #609 regression dropped + // it entirely (persisted-only, claude-only). + #[test] + fn initial_effort_falls_back_to_config_thought_level() { + let cfg = extra_with_thought_level(Some("high")); + assert_eq!(resolve_initial_effort(None, &cfg, &[]).as_deref(), Some("high")); + } + + // The interactive-switch-persisted selection wins over the create-time default — + // the same snapshot-wins precedence spec_mode_model applies to mode/model. + #[test] + fn initial_effort_persisted_selection_wins_over_default() { + let cfg = extra_with_thought_level(Some("high")); + let snap = snapshot_with_effort("low"); + assert_eq!(resolve_initial_effort(Some(&snap), &cfg, &[]).as_deref(), Some("low")); + } + + // Empty strings are noise (an unset column), never a seed. + #[test] + fn initial_effort_empty_values_are_filtered() { + assert_eq!( + resolve_initial_effort(None, &extra_with_thought_level(Some("")), &[]), + None + ); + assert_eq!(resolve_initial_effort(None, &extra_with_thought_level(None), &[]), None); + } + + // Legacy parity: `has_persisted_config_for_category` keys on the PRESENCE of a + // persisted selection, not its content — an explicitly-cleared (empty) level + // BLOCKS the create-time default instead of resurrecting it. + #[test] + fn initial_effort_empty_persisted_value_blocks_the_default() { + let snap = snapshot_with_effort(""); + assert_eq!( + resolve_initial_effort(Some(&snap), &extra_with_thought_level(Some("medium")), &[]), + None, + "an empty persisted selection means cleared, not fall-through" + ); + } + + // Upgrade compatibility: the legacy ACP path persisted the RAW wire option id — + // any effort alias must restore, with the canonical `effort` key winning when + // several coexist. + #[test] + fn initial_effort_restores_from_legacy_alias_keys() { + use crate::shared_kernel::{ConfigKey, ConfigValue}; + for alias in ["reasoning_effort", "thought_level", "thinking_budget", "thinking"] { + let mut snap = PersistedSessionState::default(); + snap.config_selections + .insert(ConfigKey::new(alias), ConfigValue::new("low")); + assert_eq!( + resolve_initial_effort(Some(&snap), &extra_with_thought_level(Some("high")), &[]).as_deref(), + Some("low"), + "legacy alias `{alias}` must restore and win over the default" + ); + } + // Canonical `effort` wins over a coexisting legacy alias (deterministic + // precedence, not HashMap iteration order). + let mut snap = snapshot_with_effort("medium"); + snap.config_selections.insert( + crate::shared_kernel::ConfigKey::new("thinking"), + crate::shared_kernel::ConfigValue::new("low"), + ); + assert_eq!( + resolve_initial_effort(Some(&snap), &extra_with_thought_level(None), &[]).as_deref(), + Some("medium"), + "the canonical effort key wins over legacy aliases" + ); + } + + // Round-11 legacy trim: a whitespace-only value is effectively absent — a + // blank persisted value BLOCKS the default (like empty), and a create-time + // value with surrounding whitespace is trimmed to its bare level. + #[test] + fn initial_effort_trims_whitespace_legacy_parity() { + // Whitespace-only persisted → cleared → blocks the default. + let snap = snapshot_with_effort(" "); + assert_eq!( + resolve_initial_effort(Some(&snap), &extra_with_thought_level(Some("high")), &[]), + None, + "a whitespace-only persisted value is cleared and blocks the default" + ); + // Padded create-time default → trimmed to the bare level. + assert_eq!( + resolve_initial_effort(None, &extra_with_thought_level(Some(" high ")), &[]).as_deref(), + Some("high"), + "a padded create-time value is trimmed to its bare level" + ); + // Whitespace-only create-time default → absent. + assert_eq!( + resolve_initial_effort(None, &extra_with_thought_level(Some(" ")), &[]), + None, + "a whitespace-only create-time value is absent" + ); + } + + // A known catalog that omits the value drops the seed (the legacy + // pending_startup_config ValueNotSelectable semantics — never highlight a level + // the model can't run); a known catalog that contains it passes it through. + #[test] + fn initial_effort_validated_against_known_catalog() { + let cfg = extra_with_thought_level(Some("ultra")); + let known: Vec = vec!["low".into(), "medium".into(), "high".into()]; + assert_eq!( + resolve_initial_effort(None, &cfg, &known), + None, + "an out-of-catalog value must be dropped" + ); + let cfg_ok = extra_with_thought_level(Some("medium")); + assert_eq!(resolve_initial_effort(None, &cfg_ok, &known).as_deref(), Some("medium")); + } + + // An EMPTY/unknown catalog is permissive (matches ACP is_*_valid: an absent + // catalog cannot invalidate; the backend re-validates on dispatch and the pump + // reconciles on catalog arrival / reject). + #[test] + fn initial_effort_unknown_catalog_is_permissive() { + let cfg = extra_with_thought_level(Some("anything")); + assert_eq!(resolve_initial_effort(None, &cfg, &[]).as_deref(), Some("anything")); + } + + // ── efforts round-trip: catalog_partial_from_caps → persisted handshake → + // CatalogPreload::from_handshake ──────────────────────────────────────── + + // The write-back projects per-model reasoning_efforts + a thought_level config + // option (with the current), and the preload parser restores the efforts — the + // full persistence round-trip that keeps the thinking picker (and seed + // validation) alive across a cold start. Before this the write-back dropped the + // efforts and the preload zeroed them. + #[test] + fn catalog_partial_and_preload_round_trip_reasoning_efforts() { + let caps = aionui_session::Capabilities { + available_models: vec![aionui_session::ModelInfo { + id: "opus".into(), + name: "Opus".into(), + description: None, + reasoning_efforts: vec!["low".into(), "high".into()], + }], + current_model: Some("opus".into()), + current_effort: Some("high".into()), + ..Default::default() + }; + let partial = catalog_partial_from_caps(&caps).expect("a catalog projects a partial"); + + // The thought axis rides config_options with its current. + let cfg = partial.config_options.as_ref().expect("config_options present"); + let thought = cfg + .as_array() + .unwrap() + .iter() + .find(|o| o["category"] == "thought_level") + .expect("a thought_level option must be projected"); + assert_eq!(thought["currentValue"], "high"); + assert_eq!( + thought["options"] + .as_array() + .unwrap() + .iter() + .map(|o| o["value"].as_str().unwrap()) + .collect::>(), + vec!["low", "high"] + ); + + // And the preload restores the per-model efforts from the persisted column. + let handshake = aionui_api_types::AgentHandshake { + available_models: partial.available_models.clone(), + ..Default::default() + }; + let preload = CatalogPreload::from_handshake(&handshake); + assert_eq!(preload.available_models.len(), 1); + assert_eq!( + preload.available_models[0].reasoning_efforts, + vec!["low".to_string(), "high".to_string()], + "reasoning_efforts must survive the persist/parse round-trip" + ); + assert_eq!(preload.current_model.as_deref(), Some("opus")); + } + #[test] fn assemble_spawn_env_orders_agent_overrides_before_runtime_context() { let agent_env = vec![ @@ -4479,11 +4873,789 @@ mod pump_tests { ); } - // send_message emits Start (before dispatch) stamped with the learned session id, - // and PromptAccepted does NOT double-emit a Start. + /// Extract each pushed AcpConfigOption frame's per-axis current_value + /// (`(mode, model, effort)`) in emit order. + fn config_frame_currents(frames: &[AgentStreamEvent]) -> Vec<(Option, Option, Option)> { + frames + .iter() + .filter_map(|f| match f { + AgentStreamEvent::AcpConfigOption(v) => { + let opts = v.get("config_options")?.as_array()?; + let cur = |cat: &str| { + opts.iter() + .find(|o| o.get("category").and_then(serde_json::Value::as_str) == Some(cat)) + .and_then(|o| o.get("current_value")) + .and_then(serde_json::Value::as_str) + .map(str::to_string) + }; + Some((cur("mode"), cur("model"), cur("thought_level"))) + } + _ => None, + }) + .collect() + } + + /// A one-model catalog whose event carries backend-reported currents. + fn catalog_with_currents(current_effort: Option<&str>) -> SessionEvent { + use aionui_session::{ModeInfo, ModelInfo}; + SessionEvent::CatalogUpdated { + models: vec![ModelInfo { + id: "opus".into(), + name: "Opus".into(), + description: None, + reasoning_efforts: vec!["low".into(), "medium".into(), "high".into()], + }], + modes: vec![ModeInfo { + id: "default".into(), + name: "Default".into(), + description: None, + }], + slash_commands: Vec::new(), + current_model: Some("opus".into()), + current_mode: Some("default".into()), + current_effort: current_effort.map(str::to_string), + } + } + + // The event's backend-reported currents light the pushed frame's highlight when + // the runtime holds no overrides — the fresh-session case: a seeded/default + // selection is visible from the FIRST catalog push, not only after an + // interactive switch (the #609-vs-#574 thought-level display regression fix). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn send_message_emits_single_leading_start_with_session_id() { - // Pre-seed the backend-bound id via a script event, then let the pump learn it. + async fn catalog_updated_event_currents_light_the_highlight() { + let frames = drain_script(vec![env(catalog_with_currents(Some("medium")))]).await; + let currents = config_frame_currents(&frames); + assert_eq!( + currents, + vec![( + Some("default".to_string()), + Some("opus".to_string()), + Some("medium".to_string()) + )], + "event currents must ride the pushed frame when no override is set" + ); + } + + // The runtime's optimistic overrides (user's interactive switch / open-time + // effort seed) WIN over the event's backend-reported currents — the same + // precedence `get_config_options` (REST) applies, so push and REST agree. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn catalog_updated_runtime_overrides_win_over_event_currents() { + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![env(catalog_with_currents(Some("medium")))], + gate: gate.clone(), + }); + let task = SessionAgentTask::new(AgentType::Acp, "conv-1".into(), "/w".into(), backend, None); + task.runtime.set_effort_override("high".into()); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + let mut frames = Vec::new(); + while let Ok(Ok(ev)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await { + frames.push(ev); + } + let currents = config_frame_currents(&frames); + assert_eq!( + currents.first().map(|c| c.2.clone()), + Some(Some("high".to_string())), + "the optimistic effort override must win over the event current" + ); + } + + // Catalog-arrival reconcile: a seeded effort the LIVE catalog does not advertise + // is falsified — the override is cleared and the pushed frame falls back to the + // backend-reported current (never highlight a level the model can't run; the + // legacy pending-seed path dropped such values before dispatch). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn catalog_arrival_clears_seeded_effort_not_in_catalog() { + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![env(catalog_with_currents(Some("medium")))], + gate: gate.clone(), + }); + let task = SessionAgentTask::new(AgentType::Acp, "conv-1".into(), "/w".into(), backend, None); + // Seeded from a stale preload: "ultra" is NOT in the live catalog's efforts. + task.runtime.set_effort_override("ultra".into()); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + let mut frames = Vec::new(); + while let Ok(Ok(ev)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await { + frames.push(ev); + } + let currents = config_frame_currents(&frames); + assert_eq!( + currents.first().map(|c| c.2.clone()), + Some(Some("medium".to_string())), + "an out-of-catalog seed must be cleared; the frame falls back to the event current" + ); + assert!( + task.runtime.effort_override().is_none(), + "the invalid override must be cleared, not just masked in the frame" + ); + } + + // Reconcile-on-reject: a backend `config_option_rejected` (claude control_response + // error / codex JSON-RPC error) clears the optimistic highlight AND re-pushes a + // corrected frame from the last-known catalog, so the picker reflects reality + // without waiting for the next catalog push or REST read. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn config_option_rejected_clears_override_and_repushes_frame() { + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![ + env(catalog_with_currents(None)), + env(SessionEvent::AdapterSpecific { + tag: "config_option_rejected".into(), + payload: serde_json::json!({ "option_id": "effort", "value": "high", "error": "nope" }), + }), + ], + gate: gate.clone(), + }); + let task = SessionAgentTask::new(AgentType::Acp, "conv-1".into(), "/w".into(), backend, None); + // The open-time seed the backend then rejects. IN the catalog (validation + // passed) — the reject is the backend's own veto. + task.runtime.set_effort_override("high".into()); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + let mut frames = Vec::new(); + while let Ok(Ok(ev)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await { + frames.push(ev); + } + let currents = config_frame_currents(&frames); + assert_eq!(currents.len(), 2, "the reject must re-push a corrected frame"); + assert_eq!( + currents[0].2.as_deref(), + Some("high"), + "the first push carries the (still-optimistic) seed" + ); + assert_eq!( + currents[1].2, None, + "the re-push after the reject must no longer highlight the refused level" + ); + assert!(task.runtime.effort_override().is_none(), "the override must be cleared"); + } + + /// A backend whose `dispatch(SetConfigOption{effort})` emits the + /// `config_option_rejected` signal INLINE — modelling a reject that is + /// causally AFTER the wire write (as a real reader would), so the pump's + /// reject reconcile genuinely races the task's optimistic override+persist. + struct RejectOnDispatchBackend { + tx: broadcast::Sender, + } + impl RejectOnDispatchBackend { + fn new() -> Self { + let (tx, _) = broadcast::channel(16); + Self { tx } + } + } + #[async_trait::async_trait] + impl SessionBackend for RejectOnDispatchBackend { + async fn dispatch(&self, c: Command) -> Result { + if let Command::SetConfigOption { option_id, value } = &c + && matches!(option_id.as_str(), "effort" | "reasoning_effort" | "thought_level") + { + let _ = self.tx.send(SessionEnvelope { + session_id: "conv-1".into(), + turn_gen: 1, + event: SessionEvent::AdapterSpecific { + tag: "config_option_rejected".into(), + payload: serde_json::json!({ "option_id": "effort", "value": value, "error": "nope" }), + }, + }); + } + Ok(CommandReceipt { + accepted: true, + admission: Admission::NoTurn, + turn_gen: 1, + }) + } + fn events(&self) -> BoxStream<'static, SessionEnvelope> { + use futures_util::StreamExt as _; + let rx = self.tx.subscribe(); + futures_util::stream::unfold(rx, |mut rx| async move { + loop { + match rx.recv().await { + Ok(env) => return Some((env, rx)), + Err(broadcast::error::RecvError::Lagged(_)) => continue, + Err(broadcast::error::RecvError::Closed) => return None, + } + } + }) + .boxed() + } + fn capabilities(&self) -> Capabilities { + Capabilities::default() + } + } + + /// Round-11 Fix A (deterministic fast-reject DURING dispatch): the reject is + /// emitted inline by `dispatch`, so the pump's reconcile races the task's + /// optimistic override+persist. Because the task holds the shared + /// effort-ops gate from BEFORE dispatch through persist, and the pump takes + /// the SAME gate at the start of its reject arm, the reconcile runs strictly + /// AFTER the optimistic write — final state is clean at EVERY layer: runtime + /// override cleared, config_selections free of the refused value, and a + /// rebuild does NOT re-seed it. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn fast_reject_during_dispatch_leaves_every_layer_clean() { + use aionui_db::{CreateAcpSessionParams, SqliteAcpSessionRepository, init_database_memory}; + let db = init_database_memory().await.unwrap(); + let repo: Arc = Arc::new(SqliteAcpSessionRepository::new(db.pool().clone())); + repo.create(&CreateAcpSessionParams { + conversation_id: "conv-1", + agent_source: "builtin", + agent_id: "claude", + }) + .await + .unwrap(); + let backend: Arc = Arc::new(RejectOnDispatchBackend::new()); + let task = SessionAgentTask::new( + AgentType::Acp, + "conv-1".into(), + "/w".into(), + backend, + Some(repo.clone()), + ); + // The user's optimistic pick — dispatch emits the reject inline; the task + // still sets its override + persists under the gate. + let _ = task.set_config_option("effort", "high").await; + // Let the pump's gated reconcile run (it blocked on the gate until the + // set_config_option above released it). + for _ in 0..80 { + let state = repo.load_runtime_state("conv-1").await.unwrap(); + let persisted_clean = state + .as_ref() + .and_then(|s| s.config_selections_json.as_deref()) + .map(|j| !j.contains("high")) + .unwrap_or(true); + if task.runtime.effort_override().is_none() && persisted_clean { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!( + task.runtime.effort_override().is_none(), + "the runtime override must end cleared after the reject reconcile" + ); + let state = repo.load_runtime_state("conv-1").await.unwrap().expect("state"); + assert!( + !state.config_selections_json.as_deref().unwrap_or("").contains("high"), + "the refused value must be removed from config_selections, got: {:?}", + state.config_selections_json + ); + // Rebuild parity: resolving the initial effort from the persisted state + // must NOT re-seed the refused level. + let persisted: std::collections::HashMap = state + .config_selections_json + .as_deref() + .and_then(|j| serde_json::from_str(j).ok()) + .unwrap_or_default(); + assert!( + !persisted.values().any(|v| v == "high"), + "a rebuild must not re-seed the refused effort" + ); + } + + /// Round-11 Fix A: an explicit backend reject must ALSO remove the refused + /// value from the persisted `config_selections` (real sqlite repo), or the + /// next open re-seeds a level the backend refused. The old code cleared only + /// the in-memory override; the persisted value survived. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn reject_unpersists_refused_effort_from_config_selections() { + use aionui_db::{CreateAcpSessionParams, SqliteAcpSessionRepository, init_database_memory}; + let db = init_database_memory().await.unwrap(); + let repo: Arc = Arc::new(SqliteAcpSessionRepository::new(db.pool().clone())); + repo.create(&CreateAcpSessionParams { + conversation_id: "conv-1", + agent_source: "builtin", + agent_id: "claude", + }) + .await + .unwrap(); + // A PERSISTED interactive selection the backend then rejects. + let selections = serde_json::json!({ "reasoning_effort": "high" }).to_string(); + repo.save_runtime_state( + "conv-1", + &SaveRuntimeStateParams { + config_selections_json: Some(Some(&selections)), + ..Default::default() + }, + ) + .await + .unwrap(); + + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![ + env(catalog_with_currents(None)), + env(SessionEvent::AdapterSpecific { + tag: "config_option_rejected".into(), + payload: serde_json::json!({ "option_id": "effort", "value": "high", "error": "nope" }), + }), + ], + gate: gate.clone(), + }); + let task = SessionAgentTask::new( + AgentType::Acp, + "conv-1".into(), + "/w".into(), + backend, + Some(repo.clone()), + ); + task.runtime.set_effort_override("high".into()); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + while let Ok(Ok(_)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await {} + assert!(task.runtime.effort_override().is_none(), "the override must be cleared"); + let state = repo.load_runtime_state("conv-1").await.unwrap().expect("state"); + assert!( + !state.config_selections_json.as_deref().unwrap_or("").contains("high"), + "the refused value must be REMOVED from the persisted selection, got: {:?}", + state.config_selections_json + ); + } + + /// Round-12 P1-3: a persisted selection with surrounding whitespace (` high `) + /// is the SAME selection as the rejected `high` and must be removed on reject + /// (unpersist compares trimmed) — otherwise resolve_initial_effort (which + /// trims) would re-seed the refused level on the next open. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn reject_unpersists_whitespace_padded_selection() { + use aionui_db::{CreateAcpSessionParams, SqliteAcpSessionRepository, init_database_memory}; + let db = init_database_memory().await.unwrap(); + let repo: Arc = Arc::new(SqliteAcpSessionRepository::new(db.pool().clone())); + repo.create(&CreateAcpSessionParams { + conversation_id: "conv-1", + agent_source: "builtin", + agent_id: "claude", + }) + .await + .unwrap(); + // Persisted with padding — the same level, differently stored. + let selections = serde_json::json!({ "reasoning_effort": " high " }).to_string(); + repo.save_runtime_state( + "conv-1", + &SaveRuntimeStateParams { + config_selections_json: Some(Some(&selections)), + ..Default::default() + }, + ) + .await + .unwrap(); + + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![ + env(catalog_with_currents(None)), + env(SessionEvent::AdapterSpecific { + tag: "config_option_rejected".into(), + payload: serde_json::json!({ "option_id": "effort", "value": "high", "error": "nope" }), + }), + ], + gate: gate.clone(), + }); + let task = SessionAgentTask::new( + AgentType::Acp, + "conv-1".into(), + "/w".into(), + backend, + Some(repo.clone()), + ); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + while let Ok(Ok(_)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await {} + let state = repo.load_runtime_state("conv-1").await.unwrap().expect("state"); + assert!( + !state.config_selections_json.as_deref().unwrap_or("").contains("high"), + "the whitespace-padded selection must be removed on reject (trim-symmetry), got: {:?}", + state.config_selections_json + ); + } + + /// Round-11 Fix C: on an A->B model switch the picker must end on B's OWN + /// default effort, NOT A's carried-over effort — even when B ALSO supports + /// the old effort (A default=medium -> B default=high while codex runs B at + /// high). The backend emits a corrected CatalogUpdated carrying B's default + /// AFTER the ConfigChanged; the pump resets the stale current on the switch + /// (no transient "B · medium" lie) and then reflects the backend's default_B. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn model_switch_lands_on_new_model_default_effort() { + use aionui_session::{ModeInfo, ModelInfo}; + let models = vec![ + ModelInfo { + id: "model-a".into(), + name: "A".into(), + description: None, + reasoning_efforts: vec!["medium".into(), "high".into()], + }, + ModelInfo { + id: "model-b".into(), + name: "B".into(), + description: None, + // SAME effort set as A, DIFFERENT default (the trap). + reasoning_efforts: vec!["medium".into(), "high".into()], + }, + ]; + let modes = vec![ModeInfo { + id: "default".into(), + name: "Default".into(), + description: None, + }]; + let catalog_a = SessionEvent::CatalogUpdated { + models: models.clone(), + modes: modes.clone(), + slash_commands: Vec::new(), + current_model: Some("model-a".into()), + current_mode: Some("default".into()), + current_effort: Some("medium".into()), + }; + // The backend's corrected snapshot that rides the switch (round-11 emit): + // model-b with ITS OWN default effort "high". + let catalog_b = SessionEvent::CatalogUpdated { + models, + modes, + slash_commands: Vec::new(), + current_model: Some("model-b".into()), + current_mode: None, + current_effort: Some("high".into()), + }; + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![ + env(catalog_a), + env(SessionEvent::ConfigChanged { + mode: None, + model: Some("model-b".into()), + }), + env(catalog_b), + ], + gate: gate.clone(), + }); + let task = SessionAgentTask::new(AgentType::Acp, "conv-1".into(), "/w".into(), backend, None); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + let mut frames = Vec::new(); + while let Ok(Ok(ev)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await { + frames.push(ev); + } + let currents = config_frame_currents(&frames); + // tuple = (mode, model, effort) + assert_eq!( + currents.last().map(|c| c.2.clone()), + Some(Some("high".to_string())), + "the picker must end on B's own default effort (high), not A's medium" + ); + assert_eq!( + currents.last().map(|c| c.1.clone()), + Some(Some("model-b".to_string())), + "the final catalog names the switched-to model B" + ); + // Round-12 P1-2: the follow-up snapshot carries current_mode:None but must + // NOT wipe the mode confirmed earlier — the final frame retains it. + assert_eq!( + currents.last().map(|c| c.0.clone()), + Some(Some("default".to_string())), + "the model-switch follow-up must not erase the retained mode" + ); + // And it must never transiently keep A's medium after the switch. + assert!( + !currents.iter().skip(1).any(|c| c.2.as_deref() == Some("medium")), + "no post-switch frame may keep the previous model's effort, got: {currents:?}" + ); + } + + /// Records every dispatched command (startup-barrier ordering assertions). + struct RecordingBackend { + commands: Arc>>, + } + + impl RecordingBackend { + fn new() -> (Arc, Arc>>) { + let commands = Arc::new(std::sync::Mutex::new(Vec::new())); + ( + Arc::new(Self { + commands: commands.clone(), + }), + commands, + ) + } + } + + #[async_trait::async_trait] + impl SessionBackend for RecordingBackend { + async fn dispatch(&self, c: Command) -> Result { + let label = match &c { + Command::Send { .. } => "send".to_string(), + Command::SetConfigOption { option_id, value } => format!("set:{option_id}={value}"), + other => format!("{other:?}"), + }; + self.commands.lock().unwrap_or_else(|e| e.into_inner()).push(label); + let admission = match c { + Command::Send { .. } => Admission::Started, + _ => Admission::NoTurn, + }; + Ok(CommandReceipt { + accepted: true, + admission, + turn_gen: 1, + }) + } + fn events(&self) -> BoxStream<'static, SessionEnvelope> { + use futures_util::StreamExt as _; + futures_util::stream::pending().boxed() + } + fn capabilities(&self) -> Capabilities { + Capabilities::default() + } + } + + fn task_with_seed(backend: Arc, seed: Option<&str>) -> Arc { + SessionAgentTask::new_with_preload( + AgentType::Acp, + "conv-1".into(), + "/w".into(), + backend, + None, + &aionui_api_types::AgentHandshake::default(), + None, + seed.map(str::to_string), + seed.map(str::to_string), + ) + } + + fn send_data(msg: &str) -> SendMessageData { + SendMessageData { + content: msg.into(), + msg_id: format!("m-{msg}"), + turn_id: None, + files: Vec::new(), + inject_skills: Vec::new(), + } + } + + // Startup barrier: the seeded effort is dispatched — and awaited — strictly + // BEFORE the first prompt, and exactly once (the second send must not re-apply). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn startup_seed_is_applied_before_the_first_prompt() { + let (backend, commands) = RecordingBackend::new(); + let task = task_with_seed(backend, Some("high")); + crate::agent_task::IAgentTask::send_message(task.as_ref(), send_data("one")) + .await + .unwrap(); + crate::agent_task::IAgentTask::send_message(task.as_ref(), send_data("two")) + .await + .unwrap(); + assert_eq!( + commands.lock().unwrap().as_slice(), + ["set:effort=high", "send", "send"], + "the seed must land before the first prompt and never re-apply" + ); + } + + // Manual-switch invalidation: a user pick BEFORE the first send supersedes the + // undelivered seed — the drain must dispatch nothing (the stale default may + // never overwrite the pick). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn manual_switch_invalidates_the_startup_seed() { + let (backend, commands) = RecordingBackend::new(); + let task = task_with_seed(backend, Some("low")); + task.set_config_option("effort", "high").await.unwrap(); + crate::agent_task::IAgentTask::send_message(task.as_ref(), send_data("one")) + .await + .unwrap(); + assert_eq!( + commands.lock().unwrap().as_slice(), + ["set:effort=high", "send"], + "the manual pick must be the only effort set; the seed is invalidated" + ); + assert_eq!( + task.runtime.effort_override().as_deref(), + Some("high"), + "the highlight follows the manual pick" + ); + } + + /// The reviewer's race, made deterministic: a manual pick whose dispatch is + /// STILL IN FLIGHT (holding the effort-ops gate) while the first send arrives. + /// The send's drain must serialize behind the gate and find the seed already + /// invalidated — the wire must end on the pick, with the stale seed never + /// written (previously: send could take the seed mid-manual and write + /// high → low → prompt while runtime/DB said high). + struct BlockingRecordingBackend { + commands: Arc>>, + /// Dispatches whose label equals `block_label` park here until notified. + block_label: String, + release: Arc, + blocked: Arc, + } + + #[async_trait::async_trait] + impl SessionBackend for BlockingRecordingBackend { + async fn dispatch(&self, c: Command) -> Result { + let label = match &c { + Command::Send { .. } => "send".to_string(), + Command::SetConfigOption { option_id, value } => format!("set:{option_id}={value}"), + other => format!("{other:?}"), + }; + if label == self.block_label { + self.blocked.notify_one(); + self.release.notified().await; + } + self.commands.lock().unwrap_or_else(|e| e.into_inner()).push(label); + let admission = match c { + Command::Send { .. } => Admission::Started, + _ => Admission::NoTurn, + }; + Ok(CommandReceipt { + accepted: true, + admission, + turn_gen: 1, + }) + } + fn events(&self) -> BoxStream<'static, SessionEnvelope> { + use futures_util::StreamExt as _; + futures_util::stream::pending().boxed() + } + fn capabilities(&self) -> Capabilities { + Capabilities::default() + } + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn concurrent_manual_pick_beats_in_flight_seed_drain() { + let commands = Arc::new(std::sync::Mutex::new(Vec::new())); + let release = Arc::new(tokio::sync::Notify::new()); + let blocked = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(BlockingRecordingBackend { + commands: commands.clone(), + block_label: "set:effort=high".into(), + release: release.clone(), + blocked: blocked.clone(), + }); + let task = task_with_seed(backend, Some("low")); + + // Manual pick: acquires the gate, invalidates the seed, then its dispatch + // parks mid-flight. + let manual = { + let task = task.clone(); + tokio::spawn(async move { task.set_config_option("effort", "high").await }) + }; + blocked.notified().await; // manual is now mid-dispatch, holding the gate + + // First send arrives while the manual is in flight: its drain must wait on + // the gate and then find NO seed. + let send = { + let task = task.clone(); + tokio::spawn( + async move { crate::agent_task::IAgentTask::send_message(task.as_ref(), send_data("one")).await }, + ) + }; + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + release.notify_one(); + manual.await.unwrap().unwrap(); + send.await.unwrap().unwrap(); + + assert_eq!( + commands.lock().unwrap().as_slice(), + ["set:effort=high", "send"], + "the pick must be the only effort write; the stale seed may never follow it" + ); + assert_eq!( + task.runtime.effort_override().as_deref(), + Some("high"), + "runtime highlight ends on the pick" + ); + } + + // No seed → no startup dispatch at all. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn no_seed_means_no_startup_dispatch() { + let (backend, commands) = RecordingBackend::new(); + let task = task_with_seed(backend, None); + crate::agent_task::IAgentTask::send_message(task.as_ref(), send_data("one")) + .await + .unwrap(); + assert_eq!(commands.lock().unwrap().as_slice(), ["send"]); + } + + // A confirmed model switch (startup reconcile / interactive) re-pushes the + // catalog frame with the NEW currents, and invalidates an effort highlight the + // new model does not advertise. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn config_changed_updates_currents_and_repushes_catalog() { + use aionui_session::{ModeInfo, ModelInfo}; + let catalog = SessionEvent::CatalogUpdated { + models: vec![ + ModelInfo { + id: "opus".into(), + name: "Opus".into(), + description: None, + reasoning_efforts: vec!["low".into(), "high".into()], + }, + ModelInfo { + id: "haiku".into(), + name: "Haiku".into(), + description: None, + reasoning_efforts: vec!["minimal".into()], + }, + ], + modes: vec![ModeInfo { + id: "default".into(), + name: "Default".into(), + description: None, + }], + slash_commands: Vec::new(), + current_model: Some("opus".into()), + current_mode: None, + current_effort: None, + }; + let gate = Arc::new(tokio::sync::Notify::new()); + let backend: Arc = Arc::new(GatedScriptBackend { + script: vec![ + env(catalog), + env(SessionEvent::ConfigChanged { + mode: Some("plan".into()), + model: Some("haiku".into()), + }), + ], + gate: gate.clone(), + }); + let task = SessionAgentTask::new(AgentType::Acp, "conv-1".into(), "/w".into(), backend, None); + // Seeded highlight valid for opus but NOT for haiku. + task.runtime.set_effort_override("high".into()); + let mut rx = crate::agent_task::IAgentTask::subscribe(task.as_ref()); + gate.notify_one(); + let mut frames = Vec::new(); + while let Ok(Ok(ev)) = tokio::time::timeout(std::time::Duration::from_millis(300), rx.recv()).await { + frames.push(ev); + } + let currents = config_frame_currents(&frames); + assert_eq!(currents.len(), 2, "the ConfigChanged must re-push the catalog frame"); + assert_eq!( + currents[1].0.as_deref(), + Some("plan"), + "the re-push carries the switched mode" + ); + assert_eq!( + currents[1].1.as_deref(), + Some("haiku"), + "the re-push carries the switched model" + ); + assert_eq!( + currents[1].2, None, + "an effort the new model does not advertise must no longer highlight" + ); + assert!( + task.runtime.effort_override().is_none(), + "the invalidated override must be cleared, not just masked" + ); + } + + // send_message emits Start (before dispatch) stamped with the learned session id, + // and PromptAccepted does NOT double-emit a Start. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn send_message_emits_single_leading_start_with_session_id() { + // Pre-seed the backend-bound id via a script event, then let the pump learn it. let backend: Arc = Arc::new(ScriptBackend(vec![env(SessionEvent::BackendBound { backend_session_id: Some("sid-abc".into()), })])); @@ -4541,6 +5713,8 @@ mod pump_tests { dir: tmp.path().to_path_buf(), backend: "claude", }), + None, + None, ); crate::agent_task::IAgentTask::send_message( task.as_ref(), @@ -4582,6 +5756,8 @@ mod pump_tests { dir: tmp.path().to_path_buf(), backend: "codex", }), + None, + None, ); // Inject an image directly onto the task's dump path via a content slice // containing an Image block. @@ -4616,6 +5792,8 @@ mod pump_tests { None, CatalogPreload::default(), None, + None, + None, ); crate::agent_task::IAgentTask::send_message( task.as_ref(), @@ -5116,6 +6294,8 @@ mod pump_tests { None, &handshake_with_catalog(), None, + None, + None, ); // get_model serves the preloaded catalog + persisted current model. @@ -5174,6 +6354,8 @@ mod pump_tests { None, &stale, None, + None, + None, ); let m = task.get_model().await.unwrap().model_info.expect("model_info"); assert_eq!( @@ -5228,6 +6410,8 @@ mod pump_tests { None, &handshake_with_catalog(), None, + None, + None, ); let m = task.get_model().await.unwrap().model_info.expect("model_info"); diff --git a/crates/aionui-ai-agent/src/session_catalog.rs b/crates/aionui-ai-agent/src/session_catalog.rs new file mode 100644 index 000000000..dd25acdeb --- /dev/null +++ b/crates/aionui-ai-agent/src/session_catalog.rs @@ -0,0 +1,318 @@ +//! Pure catalog / thought-level helpers for the direct-CLI session path +//! (`session_agent`): the initial-effort resolution (persisted selection wins, +//! legacy alias keys, drop-invalid validation), the per-model effort catalog +//! resolution, the persisted-handshake catalog preload, and the capabilities → +//! `agent_metadata` partial projection. Split out of `session_agent.rs` purely +//! for file-size hygiene — every item is a pure function / plain data holder +//! with no runtime or backend dependency. + +use crate::shared_kernel::PersistedSessionState; +use aionui_api_types::AcpBuildExtra; + +/// The `config_selections` key under which a session's chosen reasoning-effort +/// level is persisted. Neither backend emits a `ConfigChanged` for effort (only +/// mode/model), so `set_config_option` persists it here directly and +/// `build_session_instance` re-applies it after open — there is no spawn-time +/// effort flag; it rides a post-open control_request (claude) / +/// `thread/settings/update{effort}` (codex). The three accepted incoming option +/// ids (`effort`/`reasoning_effort`/`thought_level`) all normalize to this one +/// storage key. +pub(crate) const EFFORT_CONFIG_KEY: &str = "effort"; + +/// Resolve the reasoning-effort catalog to surface for the effort picker, mirroring the +/// backend's `effort_is_supported` current-model precedence: the efforts of the resolved +/// current model if it can be pinned, else the union across all advertised models (so we +/// don't hide a level some selectable model supports when the current model is ambiguous / +/// not-yet-known). Empty result = no effort axis → the caller omits the option entirely. +pub(crate) fn resolve_current_model_efforts( + models: &[aionui_session::ModelInfo], + current_model: Option<&str>, +) -> Vec { + if let Some(model) = current_model.and_then(|id| models.iter().find(|m| m.id == id)) { + return model.reasoning_efforts.clone(); + } + let mut union: Vec = Vec::new(); + for m in models { + for e in &m.reasoning_efforts { + if !union.contains(e) { + union.push(e.clone()); + } + } + } + union +} + +/// Every `config_selections` key an effort selection may be stored under, in +/// deterministic lookup precedence: the direct path's canonical +/// [`EFFORT_CONFIG_KEY`] first, then the raw option ids the LEGACY ACP path +/// persisted verbatim (`AcpSessionSyncService` writes the wire option id with no +/// canonicalization, and legacy agents advertised any of these — the same alias +/// set `config_option_aliases_for_category(ThoughtLevel)` matches on the legacy +/// read side). A conversation created before the session-model port can carry any +/// of them; reading only the canonical key would silently drop the user's choice +/// on upgrade. +pub(crate) const EFFORT_ALIAS_KEYS: [&str; 5] = [ + EFFORT_CONFIG_KEY, // "effort" + "reasoning_effort", + "thought_level", + "thinking_budget", + "thinking", +]; + +/// The persisted effort selection, if ANY alias key is present (first alias in +/// [`EFFORT_ALIAS_KEYS`] precedence wins when several coexist — e.g. a legacy +/// `thought_level` row later joined by a canonical `effort` write). +pub(crate) fn persisted_effort_selection(snapshot: &PersistedSessionState) -> Option { + EFFORT_ALIAS_KEYS.iter().find_map(|key| { + snapshot + .config_selections + .iter() + .find(|(k, _)| k.as_str() == *key) + .map(|(_, v)| v.as_str().to_owned()) + }) +} + +/// Resolve the initial reasoning-effort level for a session about to open: +/// the interactive-switch-persisted selection (any [`EFFORT_ALIAS_KEYS`] key) +/// wins over the create-time resolved default `config.thought_level` (assistant +/// fixed default / auto preference, written into the conversation's build extra +/// by the conversation service) — the same snapshot-wins precedence +/// `spec_mode_model` applies to mode/model. +/// +/// An EMPTY persisted value blocks the default entirely (returns `None`): the +/// legacy path's `has_persisted_config_for_category` keys on the PRESENCE of the +/// selection, not its content, so an explicitly-cleared level must not resurrect +/// the assistant default on the next open — presence parity with legacy. +/// +/// A resolved value is then validated against `known_efforts` (the best catalog +/// knowledge at open: live capabilities, else the persisted-handshake preload): +/// a NON-empty catalog that omits the value drops the seed (the legacy path's +/// `pending_startup_config` ValueNotSelectable semantics — never highlight a +/// level the model can't run); an EMPTY/unknown catalog is permissive (matches +/// ACP `is_*_valid`: an absent catalog cannot invalidate; the backend still +/// validates on dispatch and the pump reconciles on catalog arrival/reject). +pub(crate) fn resolve_initial_effort( + session_snapshot: Option<&PersistedSessionState>, + config: &AcpBuildExtra, + known_efforts: &[String], +) -> Option { + let effort = match session_snapshot.and_then(persisted_effort_selection) { + // Presence blocks the default (legacy parity): an empty OR whitespace-only + // persisted value means "cleared", not "fall back to the assistant + // default" (legacy trim — a blank string is effectively absent). + Some(persisted) if persisted.trim().is_empty() => return None, + Some(persisted) => persisted.trim().to_string(), + None => { + let level = config.thought_level.clone()?; + let trimmed = level.trim(); + if trimmed.is_empty() { + return None; + } + trimmed.to_string() + } + }; + if !known_efforts.is_empty() && !known_efforts.iter().any(|e| e == &effort) { + tracing::warn!( + effort = %effort, + ?known_efforts, + "session-port: initial thought level is not in the advertised effort catalog; dropping the seed" + ); + return None; + } + Some(effort) +} + +/// Cold-start catalog snapshot extracted from a persisted `agent_metadata` +/// handshake, in the SAME `aionui_session` shape the getters read off live +/// `capabilities()` — so serving the preload is a drop-in fallback with no shape +/// translation at read time. Empty vectors + `None` currents = nothing persisted. +#[derive(Default, Clone)] +pub(crate) struct CatalogPreload { + pub(crate) available_models: Vec, + pub(crate) current_model: Option, + pub(crate) available_modes: Vec, + pub(crate) current_mode: Option, +} + +impl CatalogPreload { + /// Parse the persisted handshake's `available_models` / `available_modes` + /// columns into the live-capabilities shape. Reuses the ACP path's + /// `extract_models_from_value` / `extract_modes_from_value` (the same + /// multi-shape parser that accepts both the `{available_models:[{id,label}]}` + /// column shape `spawn_catalog_writeback` persists AND a live-claude handshake), + /// so the two paths stay byte-compatible. Per-model `reasoning_efforts` are read + /// from the raw column JSON directly (`spawn_catalog_writeback` persists them + /// alongside id/label; the shared parser's state does not model efforts): without + /// them a cold-start `get_config_options` served zero efforts → the thinking + /// picker vanished (and the initial-effort seed could not be validated) until + /// the live catalog landed seconds later. + pub(crate) fn from_handshake(handshake: &aionui_api_types::AgentHandshake) -> Self { + use crate::manager::acp::config_option_catalog::{extract_models_from_value, extract_modes_from_value}; + // id → reasoning_efforts from the raw persisted entries (empty when the + // column predates efforts persistence or came from a live-claude handshake). + let efforts_by_id: std::collections::HashMap> = handshake + .available_models + .as_ref() + .and_then(|v| v.get("available_models")) + .and_then(serde_json::Value::as_array) + .map(|arr| { + arr.iter() + .filter_map(|m| { + let id = m.get("id").and_then(serde_json::Value::as_str)?.to_string(); + let efforts = m + .get("reasoning_efforts") + .and_then(serde_json::Value::as_array)? + .iter() + .filter_map(serde_json::Value::as_str) + .map(str::to_string) + .collect::>(); + Some((id, efforts)) + }) + .collect() + }) + .unwrap_or_default(); + let (available_models, current_model) = handshake + .available_models + .as_ref() + .and_then(extract_models_from_value) + .map(|state| { + let models = state + .available_models + .iter() + .map(|m| aionui_session::ModelInfo { + id: m.model_id.to_string(), + name: m.name.clone(), + description: m.description.clone(), + reasoning_efforts: efforts_by_id.get(&m.model_id.to_string()).cloned().unwrap_or_default(), + }) + .collect::>(); + let current = state.current_model_id.to_string(); + (models, (!current.is_empty()).then_some(current)) + }) + .unwrap_or_default(); + let (available_modes, current_mode) = handshake + .available_modes + .as_ref() + .and_then(extract_modes_from_value) + .map(|state| { + let modes = state + .available_modes + .iter() + .map(|m| aionui_session::ModeInfo { + id: m.id.to_string(), + name: m.name.clone(), + description: m.description.clone(), + }) + .collect::>(); + let current = state.current_mode_id.to_string(); + (modes, (!current.is_empty()).then_some(current)) + }) + .unwrap_or_default(); + Self { + available_models, + current_model, + available_modes, + current_mode, + } + } +} + +/// Project a backend's discovered `Capabilities` (modes / models / slash commands) +/// into an `AgentHandshake` partial for the `agent_metadata` catalog. Verbatim port +/// of clean-slate `session_runtime::catalog_partial_from_caps`: emits both the ACP +/// `config_options[]` wire shape AND the top-level `available_modes`/`available_models` +/// columns directly (the shape-stable path that keeps the codex model picker from +/// going empty). +pub(crate) fn catalog_partial_from_caps( + caps: &aionui_session::Capabilities, +) -> Option { + let mut config_options = Vec::new(); + if !caps.available_modes.is_empty() { + config_options.push(serde_json::json!({ + "id": "mode", + "category": "mode", + "type": "select", + "currentValue": caps.current_mode, + "options": caps.available_modes.iter().map(|m| serde_json::json!({ + "value": m.id, "name": m.name, "description": m.description, + })).collect::>(), + })); + } + if !caps.available_models.is_empty() { + config_options.push(serde_json::json!({ + "id": "model", + "category": "model", + "type": "select", + "currentValue": caps.current_model, + "options": caps.available_models.iter().map(|m| serde_json::json!({ + "value": m.id, "name": m.name, "description": m.description, + })).collect::>(), + })); + } + // Thought axis: project the discovered per-model efforts as the same + // `thought_level` option `get_config_options` serves live, so the persisted + // catalog keeps the thinking picker (and its current) across a cold start + // instead of silently dropping the axis (the pre-fix behavior). + let efforts = resolve_current_model_efforts(&caps.available_models, caps.current_model.as_deref()); + if !efforts.is_empty() { + config_options.push(serde_json::json!({ + "id": "reasoning_effort", + "category": "thought_level", + "type": "select", + "currentValue": caps.current_effort, + "options": efforts.iter().map(|e| serde_json::json!({ + "value": e, "name": e, + })).collect::>(), + })); + } + let available_commands = if caps.slash_commands.is_empty() { + None + } else { + Some(serde_json::json!( + caps.slash_commands + .iter() + .map(|c| serde_json::json!({ + "name": c.name, "description": c.description, + })) + .collect::>() + )) + }; + if config_options.is_empty() && available_commands.is_none() { + return None; + } + let config_options = if config_options.is_empty() { + None + } else { + Some(serde_json::Value::Array(config_options)) + }; + // Also project the top-level `available_modes`/`available_models` fields directly + // (shape: `{available_models:[{id,label}]}`), which `apply_handshake` persists to + // the catalog columns VERBATIM — the authoritative, shape-stable path (matches what + // a live claude handshake stores), so the codex model picker never goes empty. + let available_modes = (!caps.available_modes.is_empty()).then(|| { + serde_json::json!({ + "available_modes": caps.available_modes.iter().map(|m| serde_json::json!({ + "id": m.id, "name": m.name, "description": m.description, + })).collect::>(), + "current_mode_id": caps.current_mode, + }) + }); + let available_models = (!caps.available_models.is_empty()).then(|| { + serde_json::json!({ + // `reasoning_efforts` rides each entry so `CatalogPreload::from_handshake` + // can restore the effort axis on a cold start (additive to the + // `{id,label}` shape `extract_models_from_value` parses). + "available_models": caps.available_models.iter().map(|m| serde_json::json!({ + "id": m.id, "label": m.name, "reasoning_efforts": m.reasoning_efforts, + })).collect::>(), + "current_model_id": caps.current_model, + }) + }); + Some(aionui_api_types::AgentHandshake { + config_options, + available_modes, + available_models, + available_commands, + ..Default::default() + }) +} diff --git a/crates/aionui-session/src/backend/claude_conn.rs b/crates/aionui-session/src/backend/claude_conn.rs index 128b1776e..72f8b5a22 100644 --- a/crates/aionui-session/src/backend/claude_conn.rs +++ b/crates/aionui-session/src/backend/claude_conn.rs @@ -448,6 +448,26 @@ pub struct ClaudeSessionBackend { /// `std::sync::Mutex` (NOT tokio) so the sync reader `process_batch` closure can /// lock it without awaiting — mirrors `current_mode_override`. pending_set_config: Arc>>, + /// The ctl-id of the LATEST `set_config_option(effort)` dispatch. A rejection is + /// reconciled (clear `current_effort` + structured `config_option_rejected`) ONLY + /// when its request_id matches this — a late reject for a SUPERSEDED set (e.g. + /// high→low→high where the first high's reject arrives after the third set) must + /// not tear down the newer value it no longer speaks for. ctl ids are minted + /// monotonically, so equality against the latest is an exact "is this reject + /// about the current value" test. + latest_effort_ctl: Arc>>, + /// TEST SEAM (compiled out of production via `#[cfg(test)]`): when set, the + /// NEXT immediate control write in `write_or_queue_control_prepared` fails with + /// a synthetic transport error and resets the flag — exercises the effort + /// write-error rollback path deterministically. + #[cfg(test)] + fail_next_write: Arc, + /// TEST SEAM (compiled out of production via `#[cfg(test)]`): when armed, the + /// NEXT immediate control write PARKS on this notify BEFORE writing — lets a + /// test process a reject WHILE a dispatch is mid-write, mutation-proving the + /// pre-stamp (a post-write stamp would resurrect the refused level). + #[cfg(test)] + write_hold: Arc>>>, } /// One outstanding claude `can_use_tool` request, stored so `AnswerPermission` can @@ -508,7 +528,6 @@ struct ClaudeReaderState { discovered_model: Arc>>, /// #98/#101: shared catalog the reader fills from the initialize control_response. discovered_caps: Arc>, - want_init_model: bool, /// F-4 turn-active flag: set true on dispatch(Send), cleared by the reader at a /// turn terminal (TurnResult / Detached). The idle timer reads it so a streaming /// turn is never suspended mid-flight (see SuspendController::suspend_if_idle). @@ -524,6 +543,16 @@ struct ClaudeReaderState { /// `sniff_set_config_reject` can surface a rejection as a `Notice{Warning}` /// (shared Arc with `ClaudeSessionBackend.pending_set_config`). pending_set_config: Arc>>, + /// CP-1 mirror (shared Arc with `ClaudeSessionBackend.current_effort`): the + /// optimistically-tracked effort level. `sniff_set_config_reject` CLEARS it when + /// claude rejects the set — without the clear, `capabilities().current_effort` + /// kept advertising a level claude refused (the same lying-picker bug + /// `sniff_mode_reject` fixes for mode). Deliberately NOT stamped into + /// `CatalogUpdated` (it is optimistic, not observed — see + /// `sniff_control_initialize`). + current_effort: Arc>>, + /// Mirror of `ClaudeSessionBackend.latest_effort_ctl` (stale-reject guard). + latest_effort_ctl: Arc>>, } /// Spawn a claude stdout reader over `stdout`/`io` using the shared state. Used @@ -545,10 +574,11 @@ fn start_claude_reader( state.pending_perms, state.discovered_model, state.discovered_caps, - state.want_init_model, state.turn_in_flight, state.current_mode_override, state.pending_set_config, + state.current_effort, + state.latest_effort_ctl, ) .await; }) @@ -585,9 +615,16 @@ impl ClaudeSessionBackend { // #99: shared with the reader so a rejected set_config_option(effort) surfaces // a Notice instead of being silently dropped. let pending_set_config = Arc::new(std::sync::Mutex::new(std::collections::HashMap::new())); - // B-CLAUDE-INIT: only let the wire fill current_model when config did NOT - // supply one (config is authoritative; the init frame is the fallback). - let want_init_model = config.model.is_none(); + // CP-1: shared with the reader so it can clear the optimistic value on a + // rejected set (no lying picker). + let current_effort = Arc::new(std::sync::Mutex::new(None)); + // P: stale-reject guard — the reader only reconciles a reject whose ctl-id + // matches the LATEST effort set. + let latest_effort_ctl: Arc>> = Arc::new(std::sync::Mutex::new(None)); + #[cfg(test)] + let fail_next_write = Arc::new(std::sync::atomic::AtomicBool::new(false)); + #[cfg(test)] + let write_hold: Arc>>> = Arc::new(std::sync::Mutex::new(None)); let (event_tx, _) = broadcast::channel(1024); let stdio = io.take_stdio().await; @@ -603,10 +640,11 @@ impl ClaudeSessionBackend { pending_perms: pending_perms.clone(), discovered_model: discovered_model.clone(), discovered_caps: discovered_caps.clone(), - want_init_model, turn_in_flight: turn_in_flight.clone(), current_mode_override: current_mode_override.clone(), pending_set_config: pending_set_config.clone(), + current_effort: current_effort.clone(), + latest_effort_ctl: latest_effort_ctl.clone(), }; let reader = start_claude_reader(&reader_state, stdout, io.clone()); @@ -659,9 +697,14 @@ impl ClaudeSessionBackend { discovered_caps, pending_controls: Arc::new(Mutex::new(Vec::new())), control_seq: Arc::new(std::sync::atomic::AtomicU64::new(0)), - current_effort: Arc::new(std::sync::Mutex::new(None)), + current_effort, current_mode_override, pending_set_config, + latest_effort_ctl, + #[cfg(test)] + fail_next_write, + #[cfg(test)] + write_hold, } } @@ -801,23 +844,71 @@ impl ClaudeSessionBackend { efforts.is_empty() || efforts.contains(&value) } + /// Mint the next `ctl-N` request id. Split from the write so a caller can + /// register reader-side correlation state (pending maps, latest-set stamps) + /// BEFORE the frame is on the wire — registering after the write races a fast + /// control_response, which the reader would then find unclaimed and drop. + fn mint_ctl_id(&self) -> String { + use std::sync::atomic::Ordering; + format!("ctl-{}", self.control_seq.fetch_add(1, Ordering::SeqCst) + 1) + } + async fn write_or_queue_control(&self, request: serde_json::Value) -> Result { + let request_id = self.mint_ctl_id(); + self.write_or_queue_control_prepared(&request_id, request).await?; + Ok(request_id) + } + + /// Write (or mid-turn queue) a control_request under a PRE-MINTED id (see + /// [`mint_ctl_id`]). + async fn write_or_queue_control_prepared( + &self, + request_id: &str, + request: serde_json::Value, + ) -> Result<(), BackendError> { use std::sync::atomic::Ordering; - let request_id = format!("ctl-{}", self.control_seq.fetch_add(1, Ordering::SeqCst) + 1); let frame = serde_json::json!({ "type": "control_request", "request_id": request_id, "request": request, }); - if self.turn_in_flight.load(Ordering::SeqCst) { - let subtype = control_subtype(&frame); - let mut q = self.pending_controls.lock().await; - q.retain(|f| control_subtype(f) != subtype); - q.push(frame); - return Ok(request_id); + // TOCTOU guard: the in-flight check and the control write happen under the + // SAME stdin lock `dispatch(Send)` writes the prompt through, and Send sets + // `turn_in_flight` BEFORE acquiring that lock. So either this task observes + // in-flight=false while HOLDING the lock — the prompt cannot have been + // written yet, the control lands strictly before it (the ordering the + // pending-queue exists to guarantee) — or it observes true and queues for + // the next Send's drain. The old check-then-lock shape allowed a Send to + // complete entirely between the check and the write, landing the control + // MID-TURN (exactly what the queue is meant to prevent). + { + let mut guard = self.stdin.lock().await; + if !self.turn_in_flight.load(Ordering::SeqCst) { + #[cfg(test)] + { + if self.fail_next_write.swap(false, Ordering::SeqCst) { + return Err(BackendError::Transport("control write tripped (test seam)".into())); + } + let hold = self.write_hold.lock().unwrap_or_else(|e| e.into_inner()).clone(); + if let Some(notify) = hold { + notify.notified().await; + } + } + let stdin = guard + .as_mut() + .ok_or_else(|| BackendError::Transport("claude stdin unavailable".into()))?; + self.adapter + .write_control_response(stdin, &frame) + .await + .map_err(|e| BackendError::Transport(format!("write control_request: {e}")))?; + return Ok(()); + } } - self.write_control_frame(&frame).await?; - Ok(request_id) + let subtype = control_subtype(&frame); + let mut q = self.pending_controls.lock().await; + q.retain(|f| control_subtype(f) != subtype); + q.push(frame); + Ok(()) } /// G-A: interrupt the in-flight turn — write `control_request{subtype:"interrupt"}` @@ -1079,10 +1170,11 @@ async fn reader_task( pending_perms: Arc>>, discovered_model: Arc>>, discovered_caps: Arc>, - want_init_model: bool, turn_in_flight: Arc, current_mode_override: Arc>>, pending_set_config: Arc>>, + current_effort: Arc>>, + latest_effort_ctl: Arc>>, ) { use std::sync::atomic::Ordering; use tokio::io::AsyncReadExt; @@ -1146,13 +1238,21 @@ async fn reader_task( // parse_system drops). Done on the RAW frame so parse_chunk's // event stream stays zero-diff. Emits Provisioning per MCP // server (parity with codex mcpServerStatus→Provisioning). - sniff_init(v, want_init_model, &discovered_model, &event_tx, &session_id, cur_gen); + sniff_init(v, &discovered_model, &event_tx, &session_id, cur_gen); // #98/#101: sniff the `control_request{initialize}` RESPONSE for the // selectable model list + slash commands (claude's only catalog // channel — the data init frame above carries neither). Fills // discovered_caps; capabilities() merges it on read. Done on the RAW // frame (parse_chunk drops control frames to opaque). - sniff_control_initialize(v, &discovered_caps, &event_tx, &session_id, cur_gen); + sniff_control_initialize( + v, + &discovered_caps, + &discovered_model, + ¤t_mode_override, + &event_tx, + &session_id, + cur_gen, + ); // AUTHORITATIVE mode signal (design §9.10.1 option A / README #10): // claude stamps `permissionMode` on system/init AND system/status. This // single inbound path confirms EVERY mode change — user-driven (a @@ -1170,7 +1270,15 @@ async fn reader_task( // no handler before and was SILENTLY DROPPED. Routed by the ctl-id we // minted + registered in pending_set_config → surface a Notice{Warning}. // SUCCESS is silent (claude does not echo effort); the entry is just removed. - sniff_set_config_reject(v, &pending_set_config, &event_tx, &session_id, cur_gen); + sniff_set_config_reject( + v, + &pending_set_config, + &latest_effort_ctl, + ¤t_effort, + &event_tx, + &session_id, + cur_gen, + ); // NO set_model reader-side reconcile (design §9.10.1, Optimistic tier). // LIVE-PROBED (2.1.187, protocols/samples/claude-cli/2.1.187/_all_set_model.jsonl): // claude's set_model control_response is a BARE {subtype:"success"} — no @@ -1549,15 +1657,16 @@ fn register_or_clear_pending( } /// B-CLAUDE-INIT: sniff a raw `system/init` frame for discovery data the legacy -/// `parse_system` drops. Captures `model` into `discovered_model` (only when -/// `want_init_model`, i.e. config supplied none) and emits a `Provisioning` event +/// `parse_system` drops. ALWAYS captures `model` into `discovered_model` (the +/// authoritative observed current for every session — the old `want_init_model` +/// skip-when-configured gate is gone, see the call-site note) and emits a +/// `Provisioning` event /// per `mcp_servers[]` entry (connected→ToolsReady, failed→LoadFailed, /// needs-auth→Degraded) — parity with codex `mcpServerStatus→Provisioning`, so a /// failed/needs-auth MCP server is visible on the claude seam too. No-op for any /// non-init frame. Done on the raw frame (NOT parse_chunk) to keep zero-diff. fn sniff_init( frame: &serde_json::Value, - want_init_model: bool, discovered_model: &Arc>>, event_tx: &broadcast::Sender, session_id: &str, @@ -1569,7 +1678,14 @@ fn sniff_init( { return; } - if want_init_model && let Some(model) = frame.get("model").and_then(Value::as_str) { + // ALWAYS observe the init model — claude stamps the model it ACTUALLY runs + // (including a config-requested one, resolved) on system/init, so this is the + // authoritative observed current for CatalogUpdated. Config authority for + // `capabilities().current_model` is preserved separately by its None-only merge; + // the old `want_init_model` gate (skip when config supplied a model) left + // `discovered_model` empty for configured sessions and the catalog push then + // carried `current_model: None` — wiping the model highlight it should confirm. + if let Some(model) = frame.get("model").and_then(Value::as_str) { *discovered_model.lock().unwrap_or_else(|e| e.into_inner()) = Some(model.to_string()); } // Addendum 9 parity (codex thread/started, acp session/new|load): lower the @@ -1681,9 +1797,12 @@ fn sniff_mode( /// those keys is unambiguously the initialize reply. No-op for any other frame /// (can_use_tool success, set_model ack, etc. carry no `models`). Done on the RAW /// frame (parse_chunk drops control frames to opaque) — keeps the parse zero-diff. +#[allow(clippy::too_many_arguments)] fn sniff_control_initialize( frame: &serde_json::Value, discovered_caps: &Arc>, + discovered_model: &Arc>>, + current_mode_override: &Arc>>, event_tx: &broadcast::Sender, session_id: &str, turn_gen: u64, @@ -1769,6 +1888,17 @@ fn sniff_control_initialize( // selector stays disabled. Carry claude's fixed permission modes too: the // frontend replaces the WHOLE config_options snapshot on this frame, so omitting // modes would wipe the (synchronously-available) mode picker — a fresh regression. + // Stamp the OBSERVED currents only: `discovered_model` is the model claude + // itself reported on system/init (authoritative, config-requested sessions + // included); `current_mode_override` is reconciled to claude's own + // set_permission_mode ack / system-status track. `current_effort` is + // deliberately NOT stamped: claude never echoes effort, so our tracker is + // purely OPTIMISTIC — baking it into the event would let a later reject + // re-push resurrect the very value the backend refused (the pump would read + // it back out of its retained catalog). The effort highlight rides solely on + // the task-runtime override, which the reject reconcile clears. + let current_model = discovered_model.lock().unwrap_or_else(|e| e.into_inner()).clone(); + let current_mode = current_mode_override.lock().unwrap_or_else(|e| e.into_inner()).clone(); let _ = event_tx.send(SessionEnvelope { session_id: session_id.to_string(), turn_gen, @@ -1776,11 +1906,8 @@ fn sniff_control_initialize( models: parsed_models, modes: crate::adapter::claude_permission_modes(), slash_commands: parsed_commands, - // The additive current_model/current_mode/current_effort fields are - // filled by the follow-up transport commit; None here keeps this - // commit standalone. - current_model: None, - current_mode: None, + current_model, + current_mode, current_effort: None, }, }); @@ -1847,6 +1974,8 @@ fn sniff_mode_reject( fn sniff_set_config_reject( frame: &serde_json::Value, pending_set_config: &Arc>>, + latest_effort_ctl: &Arc>>, + current_effort: &Arc>>, event_tx: &broadcast::Sender, session_id: &str, turn_gen: u64, @@ -1873,12 +2002,49 @@ fn sniff_set_config_reject( // current_effort already reflects it. Just drop the pending entry (done above). return; } + // Stale-reject guard: only a rejection of the LATEST effort set reconciles + // state. A superseded set's late reject (high -> low -> high; the first high's + // error lands after the third set) speaks for a value that is no longer + // current — clearing on it would tear down the newer, possibly-accepted value. + // A stale reject is logged and dropped whole (its Notice would also mislead: + // the level it names may be active again via a newer accepted set). + let is_latest = latest_effort_ctl.lock().unwrap_or_else(|e| e.into_inner()).as_deref() == Some(request_id); let err = response.get("error").and_then(Value::as_str).unwrap_or("set rejected"); + if !is_latest { + tracing::info!( + session_id = %session_id, + set = %label, + "claude set_config_option(effort) rejected for a SUPERSEDED set; ignoring: {err}" + ); + return; + } tracing::error!( session_id = %session_id, set = %label, "claude set_config_option(effort) rejected: {err}" ); + // The set did not take → clear the optimistic tracker IF it still holds the + // rejected value, so `capabilities().current_effort` stops advertising a level + // claude refused (the mode analogue is `sniff_mode_reject`). A later successful + // set may already have overwritten it — only a matching value is cleared. + let rejected_value = label.split('\u{2192}').nth(1).unwrap_or("").to_string(); + { + let mut cur = current_effort.lock().unwrap_or_else(|e| e.into_inner()); + if cur.as_deref() == Some(rejected_value.as_str()) { + *cur = None; + } + } + // Structured reject signal for the task-level pump (it seeded/holds its own + // optimistic effort highlight and must clear it too — it cannot parse the + // free-text Notice below). Same envelope pattern as `mode_switch_rejected`. + let _ = event_tx.send(SessionEnvelope { + session_id: session_id.to_string(), + turn_gen, + event: SessionEvent::AdapterSpecific { + tag: "config_option_rejected".to_string(), + payload: serde_json::json!({ "option_id": "effort", "value": rejected_value, "error": err }), + }, + }); let _ = event_tx.send(SessionEnvelope { session_id: session_id.to_string(), turn_gen, @@ -2404,24 +2570,68 @@ impl SessionBackend for ClaudeSessionBackend { } else { serde_json::json!({ "effortLevel": value }) }; - let request_id = self - .write_or_queue_control(serde_json::json!({ - "subtype": "apply_flag_settings", - "settings": settings, - })) - .await?; - // #99: register the minted ctl-id so the reader surfaces a REJECTION - // (bad effort value → control_response{error}) as a Notice instead of - // silently dropping it. Success is silent (claude does not echo effort); - // the reader just removes the entry on a matching success. + // #99: register the minted ctl-id BEFORE the frame hits the wire — + // a fast control_response would otherwise find the pending map + // empty and the rejection would be silently dropped. The reader + // surfaces a REJECTION (bad effort value → control_response{error}) + // as a Notice + structured signal; success just removes the entry. + let request_id = self.mint_ctl_id(); self.pending_set_config .lock() .unwrap_or_else(|e| e.into_inner()) - .insert(request_id, format!("effort\u{2192}{value}")); - // CP-1: claude does not echo effort back, so remember it here → - // `capabilities().current_effort` highlights the active level for - // the picker (the frontend confirms by re-reading get_config_options). - *self.current_effort.lock().unwrap_or_else(|e| e.into_inner()) = Some(value.clone()); + .insert(request_id.clone(), format!("effort\u{2192}{value}")); + // Stale-reject guard: remember which ctl-id speaks for the CURRENT + // value, so the reader ignores a late reject of a superseded set. + // Capture the PRIOR latest so a synchronous write error can RESTORE + // it (round-12 P1-1): if a previous set A is still pending on the + // wire and this set B's write fails, clearing latest to None would + // strand A's late reject (its is-latest check would fail and it + // would never reconcile). Restoring A keeps it reconcilable. + let prior_latest_ctl = self + .latest_effort_ctl + .lock() + .unwrap_or_else(|e| e.into_inner()) + .replace(request_id.clone()); + // CP-1 fast-reject fix (round-11): PRE-STAMP the optimistic + // current BEFORE the write. claude does not echo effort, so the + // tracker is the picker's source. Stamping it AFTER the write + // let a FAST reject (control_response arriving before this arm + // returned) clear the tracker (by-value) and then be RE-SET by + // the post-write stamp — resurrecting the refused level. With + // the pre-stamp the reader's by-value clear is the last write, + // so a fast reject leaves the tracker cleared. The prior value + // is restored on a synchronous write error (the frame never + // left, so nothing is optimistically in effect). + let prior_effort = { + let mut cur = self.current_effort.lock().unwrap_or_else(|e| e.into_inner()); + (*cur).replace(value.clone()) + }; + if let Err(e) = self + .write_or_queue_control_prepared( + &request_id, + serde_json::json!({ + "subtype": "apply_flag_settings", + "settings": settings, + }), + ) + .await + { + // The frame never left: roll the correlation state back so the + // dead id can neither leak nor mask a later real reject, and + // restore the previous optimistic current. + self.pending_set_config + .lock() + .unwrap_or_else(|g| g.into_inner()) + .remove(&request_id); + { + let mut latest = self.latest_effort_ctl.lock().unwrap_or_else(|g| g.into_inner()); + if latest.as_deref() == Some(request_id.as_str()) { + *latest = prior_latest_ctl; + } + } + *self.current_effort.lock().unwrap_or_else(|g| g.into_inner()) = prior_effort; + return Err(e); + } let cur_gen = self.turn_gen.load(Ordering::SeqCst); Ok(CommandReceipt { accepted: true, @@ -3523,6 +3733,102 @@ mod tests { ); } + /// Startup ordering on the WIRE: an effort set dispatched before the first + /// Send (the task-level startup barrier drains the seed this way) must land on + /// stdin strictly BEFORE the prompt — the first turn runs at the seeded level. + #[tokio::test] + async fn effort_set_before_first_send_lands_before_the_prompt() { + let fake = FakeAgentIo::never_exits(Vec::new()); + let captured = fake.captured_stdin(); + let backend = ClaudeSessionBackend::build_with_io("s-order", Box::new(fake)).await; + + backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "high".into(), + }) + .await + .expect("effort set accepted"); + backend + .dispatch(Command::Send { + content: vec![ContentBlock::Text("hi".into())], + metadata: CommandMeta::default(), + }) + .await + .expect("send accepted"); + + // The capture drains asynchronously — poll until both frames landed. + let mut written = String::new(); + for _ in 0..40 { + written = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + if written.contains("apply_flag_settings") && written.contains("\"hi\"") { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + let effort_at = written + .find("apply_flag_settings") + .expect("the effort control_request must be written"); + let prompt_at = written.find("\"hi\"").expect("the prompt must be written"); + assert!( + effort_at < prompt_at, + "the effort control frame must precede the prompt on stdin, got: {written}" + ); + } + + /// TOCTOU shape: with a turn in flight, `write_or_queue_control` must QUEUE the + /// control (checked and written under the same stdin lock the prompt path + /// uses), never write it mid-turn; the queued frame drains at the head of the + /// NEXT Send, before its prompt. + #[tokio::test] + async fn effort_set_mid_turn_is_queued_and_drains_before_next_prompt() { + use std::sync::atomic::Ordering; + let fake = FakeAgentIo::never_exits(Vec::new()); + let captured = fake.captured_stdin(); + let backend = ClaudeSessionBackend::build_with_io("s-queue", Box::new(fake)).await; + + // A turn is in flight (dispatch(Send) sets this before taking the stdin lock). + backend.turn_in_flight.store(true, Ordering::SeqCst); + backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "high".into(), + }) + .await + .expect("effort set accepted (queued)"); + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + assert!( + !String::from_utf8_lossy(&captured.lock().await.clone()).contains("apply_flag_settings"), + "a mid-turn effort set must be queued, not written" + ); + + // Turn ends; the next Send drains the queue BEFORE its prompt. + backend.turn_in_flight.store(false, Ordering::SeqCst); + backend + .dispatch(Command::Send { + content: vec![ContentBlock::Text("next".into())], + metadata: CommandMeta::default(), + }) + .await + .expect("send accepted"); + let mut written = String::new(); + for _ in 0..40 { + written = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + if written.contains("apply_flag_settings") && written.contains("\"next\"") { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + let effort_at = written + .find("apply_flag_settings") + .expect("the queued effort control must drain"); + let prompt_at = written.find("\"next\"").expect("the prompt must be written"); + assert!( + effort_at < prompt_at, + "the drained control must precede the next prompt, got: {written}" + ); + } + /// §C5 HARD acceptance: claude parse ZERO-DIFF. The new ClaudeSessionBackend /// MUST surface exactly the SessionEvent sequence the legacy /// `ClaudeAdapter::parse_chunk` produces for the same bytes — the wrapping @@ -4095,6 +4401,159 @@ mod tests { assert!(matches!(err, BackendError::CommandNotSupported { command } if command == "set_config_option")); } + /// Round-12 P1-1 (claude): set A is written and pending; set B's write FAILS. + /// The rollback must RESTORE A as the latest effort ctl (not clear it to + /// None), so A's LATE reject still reconciles (clears the tracker + emits the + /// structured signal). Without the restore, A's reject would fail its + /// is-latest check and be silently dropped, leaving the refused level active. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn write_error_restores_prior_latest_so_late_reject_reconciles() { + use futures_util::StreamExt as _; + // A's late reject (ctl-1) is gated until after B's write fails. + let reject_a = concat!( + r#"{"type":"control_response","response":{"subtype":"error","request_id":"ctl-1","error":"unknown effort level: high"}}"#, + "\n", + ) + .as_bytes() + .to_vec(); + let fake = FakeAgentIo::never_exits(Vec::new()).with_gated_tail(reject_a); + let release = fake.stdout_releaser(); + let backend = ClaudeSessionBackend::build_with_io("s-p11", Box::new(fake)).await; + let mut events = backend.events(); + // Set A "high" → written (ctl-1), pending, latest=ctl-1, current=high. + backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "high".into(), + }) + .await + .expect("A accepted"); + // Set B "low" with the NEXT write tripped → the rollback must restore A. + backend.fail_next_write.store(true, std::sync::atomic::Ordering::SeqCst); + let b = backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "low".into(), + }) + .await; + assert!(b.is_err(), "B's tripped write must surface as Err, got: {b:?}"); + // A must still be the latest ctl (restored), and its pending entry intact. + assert_eq!( + backend + .latest_effort_ctl + .lock() + .unwrap_or_else(|e| e.into_inner()) + .as_deref(), + Some("ctl-1"), + "the failed B write must RESTORE A as the latest effort ctl" + ); + // Release A's late reject → it must reconcile (structured signal + clear). + release(); + let signalled = tokio::time::timeout(std::time::Duration::from_secs(2), async { + while let Some(env) = events.next().await { + if matches!(&env.event, SessionEvent::AdapterSpecific { tag, .. } if tag == "config_option_rejected") { + return true; + } + } + false + }) + .await + .unwrap_or(false); + assert!( + signalled, + "A's late reject must reconcile (never dropped as non-latest)" + ); + assert!( + backend + .current_effort + .lock() + .unwrap_or_else(|e| e.into_inner()) + .is_none(), + "the reconcile must clear the tracker" + ); + } + + /// Round-11/12 A2 (MUTATION-PROVING): the effort set PRE-STAMPS the + /// optimistic current BEFORE the wire write. This test parks the dispatch + /// mid-write (write_hold) and lets the reader process the reject WHILE the + /// dispatch is still writing — so a post-write stamp (the mutation) would + /// re-set the refused level AFTER the reader cleared it, leaving + /// current_effort = high. With the pre-stamp the reader's by-value clear is + /// the last write and current_effort ends None. Asserting None after the + /// dispatch returns therefore KILLS the post-stamp mutation. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn effort_reject_during_write_clears_tracker_prestamp_mutation_proving() { + // The effort dispatch mints ctl-1; its reject is gated until the dispatch + // has parked on the write hold (pending registered, current pre-stamped). + let reject = concat!( + r#"{"type":"control_response","response":{"subtype":"error","request_id":"ctl-1","error":"unknown effort level: high"}}"#, + "\n", + ) + .as_bytes() + .to_vec(); + let fake = FakeAgentIo::never_exits(Vec::new()).with_gated_tail(reject); + let release_reject = fake.stdout_releaser(); + let backend = std::sync::Arc::new(ClaudeSessionBackend::build_with_io("s-mp", Box::new(fake)).await); + // Arm the write hold: the dispatch will park mid-write. + let hold = std::sync::Arc::new(tokio::sync::Notify::new()); + *backend.write_hold.lock().unwrap() = Some(hold.clone()); + let dispatch = { + let backend = backend.clone(); + tokio::spawn(async move { + backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "high".into(), + }) + .await + }) + }; + // Wait until the dispatch has registered the pending correlation and + // pre-stamped the current (it is now parked on the write hold). + for _ in 0..80 { + let pending = backend + .pending_set_config + .lock() + .unwrap_or_else(|e| e.into_inner()) + .contains_key("ctl-1"); + let stamped = backend.capabilities().current_effort.as_deref() == Some("high"); + if pending && stamped { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!( + backend + .pending_set_config + .lock() + .unwrap_or_else(|e| e.into_inner()) + .contains_key("ctl-1"), + "the dispatch must have registered the pending before parking" + ); + // Release the reject → the reader clears the tracker WHILE the dispatch + // is still parked mid-write. + release_reject(); + for _ in 0..80 { + if backend.capabilities().current_effort.is_none() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!( + backend.capabilities().current_effort.is_none(), + "the reader must clear the tracker while the dispatch is mid-write" + ); + // Now let the write complete and the dispatch return. + hold.notify_one(); + dispatch.await.unwrap().expect("dispatch returns"); + // MUTATION KILL: a post-write stamp would have re-set high here. + assert!( + backend.capabilities().current_effort.is_none(), + "after the dispatch returns the tracker must STILL be cleared (no post-write stamp resurrects the refused level)" + ); + *backend.write_hold.lock().unwrap() = None; + } + /// #1 effort catalog validation (ACP `clear_invalid_desired_*` ported to effort). /// Once the initialize control_response has advertised a model with a bounded /// `supportedEffortLevels` set, a `SetConfigOption{effort}` for a level OUTSIDE that @@ -4644,12 +5103,12 @@ mod tests { } #[tokio::test] - async fn b_claude_init_does_not_override_config_model() { - // config model is authoritative: when build_with_io seeds a model (it does - // not — defaults None — so we test the inverse: when config HAS a model, the - // init wire model must NOT overwrite it). build_with_io uses default config - // (None), so here we assert the wire fills it; the config-wins path is - // covered by the want_init_model gate (config.model.is_none()). + async fn b_claude_init_populates_model_when_config_absent() { + // build_with_io uses the default config (model = None), so the init-wire model + // is the only source: assert the reader fills `capabilities().current_model` + // from the `system/init` frame. This test does NOT exercise the config-supplied + // path (that would need a configured build_with_io); it only pins the + // wire-fills-when-absent direction. let init = r#"{"type":"system","subtype":"init","session_id":"s","model":"wire-model","tools":[]}"#; let fake = FakeAgentIo::never_exits(format!("{init}\n").into_bytes()); let backend = ClaudeSessionBackend::build_with_io("s", Box::new(fake)).await; @@ -4760,6 +5219,52 @@ mod tests { assert_eq!(slash_commands[0].name, "verify"); } + /// The catalog push must carry the OBSERVED currents (system/init model), and + /// must NOT carry the optimistic effort tracker: claude never echoes effort, so + /// stamping our optimistic value into the event would let a later reject + /// re-push resurrect the refused level out of the pump's retained catalog. The + /// effort highlight rides solely on the task-runtime override. + #[tokio::test] + async fn control_initialize_catalog_updated_carries_observed_currents_only() { + use futures_util::StreamExt as _; + // A system/init (fills discovered_model) followed by the initialize response. + let init_frame = r#"{"type":"system","subtype":"init","model":"claude-opus-4-8","session_id":"s-cur"}"#; + let init_resp = r#"{"type":"control_response","response":{"subtype":"success","request_id":"ctl-1","response":{"models":[{"value":"claude-opus-4-8","displayName":"Opus","supportedEffortLevels":["low","high"]}]}}}"#; + let fake = + FakeAgentIo::never_exits(Vec::new()).with_gated_tail(format!("{init_frame}\n{init_resp}\n").into_bytes()); + let release = fake.stdout_releaser(); + let backend = ClaudeSessionBackend::build_with_io("s-cur", Box::new(fake)).await; + // Even with an optimistic effort tracked (live path: dispatch stored it), + // the event must not carry it. + *backend.current_effort.lock().unwrap_or_else(|e| e.into_inner()) = Some("high".to_string()); + let mut events = backend.events(); + release(); + + let mut found = None; + for _ in 0..80 { + if let Ok(Some(env)) = tokio::time::timeout(std::time::Duration::from_millis(200), events.next()).await + && let SessionEvent::CatalogUpdated { + current_model, + current_effort, + .. + } = env.event + { + found = Some((current_model, current_effort)); + break; + } + } + let (current_model, current_effort) = found.expect("a CatalogUpdated must be broadcast on initialize"); + assert_eq!( + current_model.as_deref(), + Some("claude-opus-4-8"), + "the system/init observed model rides the event" + ); + assert_eq!( + current_effort, None, + "the OPTIMISTIC effort tracker must NOT ride the event (observed-only)" + ); + } + /// A non-initialize success control_response (e.g. a set_model ack, which has no /// `models`/`commands`) must NOT clobber the catalog — the request_id-free sniff /// keys on the presence of `models`/`commands`, not on a correlation id. @@ -5264,16 +5769,25 @@ mod tests { let release = fake.stdout_releaser(); let backend = ClaudeSessionBackend::build_with_io("s-effort-err", Box::new(fake)).await; // Register the in-flight effort set keyed on the id we minted (live path: - // dispatch(SetConfigOption{effort}) does this). + // dispatch(SetConfigOption{effort}) does this) AND the optimistic current + // it stored — the reject must roll BOTH back. backend.set_pending_set_config_for_test("ctl-9", "effort\u{2192}ultra"); + *backend.current_effort.lock().unwrap_or_else(|e| e.into_inner()) = Some("ultra".to_string()); + // Live path: the dispatch that registered ctl-9 also marked it latest. + *backend.latest_effort_ctl.lock().unwrap_or_else(|e| e.into_inner()) = Some("ctl-9".to_string()); let mut events = backend.events(); release(); - let notice = tokio::time::timeout(std::time::Duration::from_secs(5), async { + let (rejected, notice) = tokio::time::timeout(std::time::Duration::from_secs(5), async { + let mut rejected = None; while let Some(env) = events.next().await { - if let SessionEvent::Notice { level, message } = env.event { - return Some((level, message)); + match env.event { + SessionEvent::AdapterSpecific { tag, payload } if tag == "config_option_rejected" => { + rejected = Some(payload); + } + SessionEvent::Notice { level, message } => return Some((rejected, (level, message))), + _ => {} } } None @@ -5287,6 +5801,24 @@ mod tests { "the Notice carries the label + claude's error message, got: {}", notice.1 ); + // The structured reject signal precedes the Notice so the task-level pump can + // clear its own optimistic highlight (it cannot parse the free-text Notice). + let payload = rejected.expect("a config_option_rejected must ride alongside the Notice"); + assert_eq!( + payload.get("value").and_then(serde_json::Value::as_str), + Some("ultra"), + "the rejected value rides the structured payload" + ); + // The optimistic current_effort was rolled back — capabilities() must no + // longer advertise a level claude refused (the mode analogue: sniff_mode_reject). + assert!( + backend + .current_effort + .lock() + .unwrap_or_else(|e| e.into_inner()) + .is_none(), + "the rejected optimistic current_effort must be cleared" + ); // The matching pending entry was claimed; the permission-mode error (ctl-1) // never had one, so it produced no effort Notice and left no leak. assert!( @@ -5299,6 +5831,66 @@ mod tests { ); } + /// P: a LATE reject of a SUPERSEDED effort set must reconcile NOTHING — the + /// high→low→high interleaving where the FIRST high's error control_response + /// arrives after the THIRD set: clearing on it would tear down the newer value + /// it no longer speaks for. Correlated by ctl-id against the latest set. + #[tokio::test] + async fn stale_effort_reject_is_ignored() { + let tail = concat!( + r#"{"type":"control_response","response":{"subtype":"error","request_id":"ctl-3","error":"unknown effort level: high"}}"#, + "\n", + ) + .as_bytes() + .to_vec(); + let fake = FakeAgentIo::never_exits(Vec::new()).with_gated_tail(tail); + let release = fake.stdout_releaser(); + let backend = ClaudeSessionBackend::build_with_io("s-stale", Box::new(fake)).await; + // The FIRST set (ctl-3, "high") is still pending, but two newer sets have + // happened since; the LATEST (ctl-7) re-selected "high" and succeeded. + backend.set_pending_set_config_for_test("ctl-3", "effort\u{2192}high"); + *backend.current_effort.lock().unwrap_or_else(|e| e.into_inner()) = Some("high".to_string()); + *backend.latest_effort_ctl.lock().unwrap_or_else(|e| e.into_inner()) = Some("ctl-7".to_string()); + + let mut events = backend.events(); + release(); + + // Drain briefly: NO Notice and NO config_option_rejected may surface. + let saw_reconcile = tokio::time::timeout(std::time::Duration::from_millis(600), async { + while let Some(env) = events.next().await { + match env.event { + SessionEvent::Notice { .. } => return true, + SessionEvent::AdapterSpecific { ref tag, .. } if tag == "config_option_rejected" => return true, + _ => {} + } + } + false + }) + .await + .unwrap_or(false); + assert!( + !saw_reconcile, + "a stale reject must emit neither Notice nor structured signal" + ); + assert_eq!( + backend + .current_effort + .lock() + .unwrap_or_else(|e| e.into_inner()) + .as_deref(), + Some("high"), + "the current value the stale reject no longer speaks for must survive" + ); + assert!( + backend + .pending_set_config + .lock() + .unwrap_or_else(|e| e.into_inner()) + .is_empty(), + "the stale pending entry is still claimed (no leak)" + ); + } + /// set_model is OPTIMISTIC (design §9.10.1). LIVE-PROBED (2.1.187, /// protocols/samples/claude-cli/2.1.187/_all_set_model.jsonl): claude's set_model /// control_response is a BARE {subtype:"success"} with NO model echo (and a bogus diff --git a/crates/aionui-session/src/backend/codex_conn.rs b/crates/aionui-session/src/backend/codex_conn.rs index e76b1ba7c..b06056166 100644 --- a/crates/aionui-session/src/backend/codex_conn.rs +++ b/crates/aionui-session/src/backend/codex_conn.rs @@ -181,9 +181,20 @@ impl BackendConnection for CodexConnection { // config → nothing to reconcile). The two are SEQUENCED (model first) only to keep // the two writes deterministic — SetMode no longer depends on current_model // (feature 012置换: permissions channel), but sequencing keeps the wire order stable. - if matches!(spec, SessionSpec::Fresh { .. }) && (config.model.is_some() || config.mode.is_some()) { + if matches!(spec, SessionSpec::Fresh { .. }) + && (config.model.is_some() || config.mode.is_some() || config.reasoning_effort.is_some()) + { let backend = Arc::new(backend); - spawn_codex_reconcile(backend.clone(), config.model.clone(), config.mode.clone()); + // Best-effort DETACHED startup apply (minimal branch): the seed is + // applied post-open with no gate on the first turn — a prompt racing + // these writes may briefly run on codex's launch defaults, matching + // current upstream behaviour for model/mode (see Limitations). + spawn_codex_bootstrap( + backend.clone(), + config.model.clone(), + config.mode.clone(), + config.reasoning_effort.clone(), + ); return Ok(backend); } @@ -218,20 +229,128 @@ const CODEX_RECONCILE_POLLS: u32 = 100; /// model+mode (it deliberately did not embed `config.model`, and codex has no /// `thread/start` mode param at all). This detached task applies the requested model /// then mode, each validated against its discovered catalog. The two are SEQUENCED — -/// model MUST settle first because `SetMode` builds a `collaborationMode` around the -/// tracked `current_model`; running them concurrently could fire `SetMode` while -/// `current_model` is still the (possibly-invalid) optimistic seed or already cleared. -fn spawn_codex_reconcile(backend: Arc, model: Option, mode: Option) { +/// a deterministic wire order (model settings write, drained, then mode) so the mode +/// apply is validated against the model codex actually kept rather than the optimistic +/// seed. We do NOT assert anything about codex's internal `SetMode`/`collaborationMode` +/// coupling — that is unverified; the ordering is our own conservative choice, not a +/// documented codex requirement. +fn spawn_codex_bootstrap( + backend: Arc, + model: Option, + mode: Option, + effort: Option, +) { tokio::spawn(async move { if let Some(model) = model { reconcile_codex_model(&backend, model).await; + // Barrier: wait for the settings ACK (the JSON-RPC response drains the + // pending_set entry) before the dependent effort apply — JSON-RPC gives + // no cross-request ordering guarantee, so "wrote model before effort" + // is not "applied model before effort". + // + // KNOWN LIMITATION (minimal branch): `await_pending_set_drained` + // treats ANY response — success OR JSON-RPC error — as "drained", so + // an explicit model-set ERROR still lets this detached sequence + // proceed to the effort write (validated against the REQUESTED model, + // not the one codex actually kept). The backend stays authoritative + // and the reader's structured reject reconcile corrects the effort; + // a result-correlated drain would need the hardened transport state + // machine, out of this compact fix's scope. Characterized by + // `model_settings_error_still_lets_effort_attempt_run` (fail-open). + await_pending_set_drained(&backend, "model\u{2192}").await; } if let Some(mode) = mode { reconcile_codex_mode(&backend, mode).await; } + if let Some(effort) = effort { + bootstrap_apply_effort(&backend, effort).await; + } }); } +/// Wait (bounded, same poll cadence as the reconcile) until no in-flight +/// `thread/settings/update` whose label starts with `prefix` remains unanswered — +/// the reader removes a `pending_set` entry when its JSON-RPC response arrives +/// (success OR error), so absence means "a terminal response was observed and the +/// entry drained" — NOT that the setting was accepted; the result is not classified +/// here. `false` = the bound elapsed with the request still unanswered (the bootstrap +/// proceeds — a wedged +/// settings response must not brick the session; the turn path stays usable). +async fn await_pending_set_drained(backend: &CodexSessionBackend, prefix: &str) -> bool { + for _ in 0..CODEX_RECONCILE_POLLS { + let pending = backend + .pending_set + .lock() + .await + .values() + .any(|label| label.starts_with(prefix)); + if !pending { + return true; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + tracing::warn!(prefix = %prefix, "codex bootstrap: settings ack never arrived within the poll bound"); + false +} + +/// Apply the seeded reasoning effort as the LAST step of the DETACHED startup +/// sequence: validate it against the EFFECTIVE model's advertised efforts (the +/// tracked current after the model reconcile, else the catalog default; an +/// unknown/empty list stays permissive, matching ACP `is_*_valid`), dispatch the +/// settings update, then best-effort wait for its response (see +/// `await_pending_set_drained` — the drain treats success and error alike, so +/// this is NOT a confirmed acknowledgement). An out-of-catalog seed is dropped +/// WITHOUT a wire write and surfaced as the same structured +/// `config_option_rejected` a backend reject emits, so the task runtime clears +/// its optimistic highlight instead of advertising a level the model cannot run. +async fn bootstrap_apply_effort(backend: &CodexSessionBackend, effort: String) { + let effective_model = { + let live = backend.current_model.lock().await.clone(); + let disc = backend.discovered.lock().unwrap_or_else(|e| e.into_inner()); + live.or_else(|| disc.default_model.clone()) + }; + let supported = { + let disc = backend.discovered.lock().unwrap_or_else(|e| e.into_inner()); + match effective_model + .as_deref() + .and_then(|id| disc.models.iter().find(|m| m.id == id)) + { + Some(model) if !model.reasoning_efforts.is_empty() => model.reasoning_efforts.contains(&effort), + // Unknown model / no advertised efforts → permissive (absent catalog + // cannot invalidate); codex itself still rejects an unsupported value. + _ => true, + } + }; + if !supported { + tracing::warn!( + effort = %effort, + model = ?effective_model, + "codex bootstrap: seeded effort not advertised by the effective model; dropping (structured reject emitted)" + ); + let _ = backend.event_tx.send(SessionEnvelope { + session_id: backend.session_id.clone(), + turn_gen: backend.turn_gen.load(std::sync::atomic::Ordering::SeqCst), + event: SessionEvent::AdapterSpecific { + tag: "config_option_rejected".to_string(), + payload: json!({ + "option_id": "effort", + "value": effort, + "error": "not supported by the effective model", + }), + }, + }); + return; + } + // Write via the shared helper directly (the detached seed has no dispatch + // gate on the minimal branch — a manual pick racing it just wins on the wire). + if let Err(e) = backend.write_effort_settings_update(&effort).await { + tracing::warn!(effort = %effort, error = %e, "codex bootstrap: effort write failed (session usable, seed not applied)"); + return; + } + await_pending_set_drained(backend, "effort\u{2192}").await; + tracing::info!(effort = %effort, "codex bootstrap: seeded reasoning effort settings written (response drained; not a confirmed ack)"); +} + /// Wait for a codex `*/list` catalog to populate `discovered`, returning the id list. /// Empty vec = never populated within the poll bound (cannot validate). async fn await_codex_catalog( @@ -251,6 +370,37 @@ async fn await_codex_catalog( Vec::new() } +/// Round-11 (minimal-branch correctness): re-push a CatalogUpdated carrying the +/// HONEST currents from `discovered` after the optimistic open-time model seed +/// was dropped — an earlier frame may already have advertised the requested +/// (invalid) model as current, and without this corrected push it stands +/// forever. current_model = the tracked live model (now cleared) or the +/// catalog default; current_effort = that model's defaultReasoningEffort. +async fn emit_corrected_codex_catalog(backend: &CodexSessionBackend) { + let live_model = backend.current_model.lock().await.clone(); + let (models, modes, current_model, current_effort) = { + let disc = backend.discovered.lock().unwrap_or_else(|e| e.into_inner()); + let effective = live_model.or_else(|| disc.default_model.clone()); + let current_effort = effective + .as_deref() + .and_then(|id| disc.default_efforts.get(id).cloned()); + (disc.models.clone(), disc.modes.clone(), effective, current_effort) + }; + emit( + &backend.event_tx, + &backend.session_id, + backend.turn_gen.load(std::sync::atomic::Ordering::SeqCst), + SessionEvent::CatalogUpdated { + models, + modes, + slash_commands: Vec::new(), + current_model, + current_mode: None, + current_effort, + }, + ); +} + /// Apply `requested` model the ACP way: wait for `model/list` to fill the catalog, then /// - if `requested` IS in the catalog → dispatch a `SetModel` (validated apply; /// success converges via `thread/settings/updated`); @@ -265,13 +415,21 @@ async fn reconcile_codex_model(backend: &CodexSessionBackend, requested: String) let catalog = await_codex_catalog(backend, |d| d.models.iter().map(|m| m.id.clone()).collect()).await; if catalog.is_empty() { - // Never learned the catalog → cannot validate. Leave codex on its launch - // default (the safe choice) rather than bind a possibly-invalid model. + // Never learned the catalog (reconcile timeout) → cannot validate. Leave + // codex on its launch default rather than bind a possibly-invalid model. + // Round-11: CLEAR the optimistic open-time seed and push corrected + // currents — otherwise a model/list arriving LATER (after this timeout) + // would run the discovery-emit path with the still-set requested seed as + // `current_model` and stamp the (possibly-invalid) model with no one left + // to correct it. With the seed cleared, that late emit serves the honest + // discovered default instead. tracing::warn!( requested_model = %requested, "codex model reconcile: model/list never populated; leaving thread on codex default \ (requested model NOT applied — cannot validate)" ); + *backend.current_model.lock().await = None; + emit_corrected_codex_catalog(backend).await; return; } @@ -286,6 +444,9 @@ async fn reconcile_codex_model(backend: &CodexSessionBackend, requested: String) (thread stays on codex default)" ); *backend.current_model.lock().await = None; + // Corrected push: re-advertise the honest default so a stale earlier + // frame that named the invalid model as current does not stand. + emit_corrected_codex_catalog(backend).await; return; } @@ -713,6 +874,19 @@ pub struct CodexSessionBackend { /// `map_notification` → ConfigChanged, live-verified), so emitting here too would /// duplicate the ConfigChanged. The codex analogue of acp_conn's `pending_set`. pending_set: Arc>>, + /// The rpc id of the LATEST `thread/settings/update{effort}` dispatch (0 = none + /// yet; real rpc ids start at 1). A rejection reconciles (structured + /// `config_option_rejected`) ONLY when its rpc id matches — a late reject of a + /// SUPERSEDED effort set must not tear down the newer value it no longer speaks + /// for. rpc ids are minted monotonically, so equality against the latest is an + /// exact "is this reject about the current value" test. + latest_effort_set_rpc: Arc, + /// TEST SEAM (compiled out of production via `#[cfg(test)]`): when set, the + /// NEXT `write_frame` fails with a synthetic transport error and resets the + /// flag — lets a test exercise the settings write-error rollback path + /// deterministically without a broken pipe. + #[cfg(test)] + fail_next_write: Arc, } /// One in-flight prompt-carrying client request (GAP-A correlation entry). @@ -753,6 +927,18 @@ struct Discovered { /// For codex this holds the fixed permission-tier mode enum mapped from /// `permissionProfile/list` (feature 012), NOT collaborationMode. modes: Vec, + /// The model `model/list` marks `isDefault: true` — codex's own launch default + /// (wire field verified against the calibrated capture fixture, + /// samples/codex-cli/0.137.0/appserver-methods/catalog.jsonl). Serves as + /// `capabilities().current_model` when config supplied no model: a fresh thread + /// runs on exactly this model (`thread/start` embeds no model; the reconcile only + /// overrides it when a model was requested). + default_model: Option, + /// Per-model `defaultReasoningEffort` from the same response (same capture) — + /// the effort codex runs a model at unless a `thread/settings/update{effort}` + /// overrides it. Keyed by model id; consulted for the CURRENT model to serve + /// `capabilities().current_effort` when no explicit effort was set. + default_efforts: std::collections::HashMap, } /// What `CodexSessionBackend::wake_handle` needs to re-spawn the codex app-server @@ -798,6 +984,13 @@ struct CodexReaderState { /// terminal (TurnResult / Detached). The idle timer reads it so a streaming turn /// is never suspended mid-flight. turn_in_flight: Arc, + /// Live current model (open-time config seed, then SetModel switches / the + /// reconcile's clear). The reader stamps it onto `CatalogUpdated.current_model` + /// so the push reflects the REQUESTED/current model, not the catalog default, + /// for a configured session. + current_model: Arc>>, + /// Mirror of `CodexSessionBackend.latest_effort_set_rpc` (stale-reject guard). + latest_effort_set_rpc: Arc, } /// Spawn a codex JSON-RPC reader over `stdout`/`io` using the shared state. Used @@ -827,6 +1020,8 @@ fn start_codex_reader( state.discovered, state.stdin, state.turn_in_flight, + state.current_model, + state.latest_effort_set_rpc, ) .await; }) @@ -954,6 +1149,9 @@ impl CodexSessionBackend { let resume_poison = Arc::new(Mutex::new(None)); let discovered = Arc::new(std::sync::Mutex::new(Discovered::default())); let turn_in_flight = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let latest_effort_set_rpc = Arc::new(std::sync::atomic::AtomicU64::new(0)); + #[cfg(test)] + let fail_next_write = Arc::new(std::sync::atomic::AtomicBool::new(false)); let (event_tx, _) = broadcast::channel(1024); let (stdin, stdout) = match io.take_stdio().await { @@ -978,6 +1176,8 @@ impl CodexSessionBackend { discovered: discovered.clone(), stdin: stdin.clone(), turn_in_flight: turn_in_flight.clone(), + current_model: current_model.clone(), + latest_effort_set_rpc: latest_effort_set_rpc.clone(), }; let reader = start_codex_reader(&reader_state, stdout, io.clone()); @@ -1032,11 +1232,18 @@ impl CodexSessionBackend { pending_resume, resume_poison, discovered, + latest_effort_set_rpc, + #[cfg(test)] + fail_next_write, } } /// Write one JSON-RPC frame (request or response) to stdin as a single line. async fn write_frame(&self, frame: Value) -> Result<(), BackendError> { + #[cfg(test)] + if self.fail_next_write.swap(false, std::sync::atomic::Ordering::SeqCst) { + return Err(BackendError::Transport("write frame tripped (test seam)".into())); + } let mut guard = self.stdin.lock().await; let stdin = guard .as_mut() @@ -1059,6 +1266,52 @@ impl CodexSessionBackend { self.rpc_id.fetch_add(1, Ordering::SeqCst) + 1 } + /// The shared `thread/settings/update{effort}` write (wake → bound thread → + /// pending_set registration + latest-set stamp → frame). Used by BOTH the + /// dispatch arm (a manual thinking-level pick) and the detached startup + /// bootstrap seed. Neither is serialized against the other on the wire — there + /// is no gate coupling them; a manual pick racing the seed is simply last wire + /// write wins, and the reader's latest-set stamp + structured reject reconcile + /// keep the tracked effort consistent with whichever write codex answered last. + async fn write_effort_settings_update(&self, value: &str) -> Result { + // F-4: between-turn config write → wake a suspended session first. + self.suspend + .ensure_awake(aionui_common::now_ms(), || self.wake_handle()) + .await?; + let tid = self.bound_thread().await?; + let id = self.next_rpc_id(); + // Register the rpc id so the reader claims the response: a JSON-RPC error + // (codex rejected the effort) surfaces as a Notice instead of being + // dropped (success converges via thread/settings/updated). + self.pending_set + .lock() + .await + .insert(id, format!("effort\u{2192}{value}")); + // Stale-reject guard: this rpc id now speaks for the current value. + // `swap` captures the PRIOR latest so a write error can RESTORE it + // (round-12 P1-1): if a previous set A is still pending and this set B's + // write fails, leaving latest at B would strand A's late reject (its + // is-latest check fails). Restore A and REMOVE the failed pending B + // (else it leaks and could mask a later real reject). + let prior_latest = self.latest_effort_set_rpc.swap(id, Ordering::SeqCst); + let frame = json!({ + "jsonrpc": "2.0", "id": id, "method": "thread/settings/update", + "params": { "threadId": tid, "effort": value } + }); + if let Err(e) = self.write_frame(frame).await { + self.pending_set.lock().await.remove(&id); + let _ = self + .latest_effort_set_rpc + .compare_exchange(id, prior_latest, Ordering::SeqCst, Ordering::SeqCst); + return Err(e); + } + Ok(CommandReceipt { + accepted: true, + admission: Admission::NoTurn, + turn_gen: self.turn_gen.load(Ordering::SeqCst), + }) + } + /// Resolve the bound backend threadId, waiting briefly for the async /// `thread/started` notification (Fresh sessions bind it on the wire; Resume /// pre-seeds it in open_session). Every `turn/*` + `thread/*` client request @@ -1239,6 +1492,8 @@ async fn reader_task( discovered: Arc>, stdin: Arc>>, turn_in_flight: Arc, + current_model: Arc>>, + latest_effort_set_rpc: Arc, ) { use tokio::io::{AsyncBufReadExt, BufReader}; @@ -1434,7 +1689,41 @@ async fn reader_task( continue; } for ev in map_notification(m, params) { - emit(&event_tx, &session_id, cur, ev); + // Round-11 (A->B default effort): a model-switch + // confirmation (ConfigChanged carrying a new model) is + // the ONLY signal codex sends — it does NOT follow with + // a fresh catalog. Re-emit a CatalogUpdated snapshot + // carrying the NEW model's OWN defaultReasoningEffort so + // the picker shows B's default, not A's carried-over + // effort (A default=medium -> B default=high must not + // keep showing medium). Emitted AFTER the ConfigChanged. + if let SessionEvent::ConfigChanged { + model: Some(new_model), .. + } = &ev + { + let (models, modes, current_effort) = { + let disc = discovered.lock().unwrap_or_else(|e| e.into_inner()); + let eff = disc.default_efforts.get(new_model).cloned(); + (disc.models.clone(), disc.modes.clone(), eff) + }; + let new_model = new_model.clone(); + emit(&event_tx, &session_id, cur, ev); + emit( + &event_tx, + &session_id, + cur, + SessionEvent::CatalogUpdated { + models, + modes, + slash_commands: Vec::new(), + current_model: Some(new_model), + current_mode: None, + current_effort, + }, + ); + } else { + emit(&event_tx, &session_id, cur, ev); + } } } _ => { @@ -1556,9 +1845,25 @@ async fn reader_task( // `config_options` on open, never re-fetches and the // selectors stay disabled. (codex's modes come from // permissionProfile/list — the fixed permission-tier enum.) - let (models, modes) = { + // Currents stamped for the EFFECTIVE model: the + // live tracked model (open-time config seed / + // SetModel switches — cleared by the reconcile if + // invalid) wins over the catalog's isDefault + // fallback, so a configured session's push + // confirms the REQUESTED model instead of + // resetting the highlight to codex's launch + // default. current_effort is that model's + // defaultReasoningEffort (codex-observed). + // Interactive switches still win at the pump + // (its runtime overrides take precedence). + let live_model = current_model.lock().await.clone(); + let (models, modes, current_model_now, current_effort_now) = { let disc = discovered.lock().unwrap_or_else(|e| e.into_inner()); - (disc.models.clone(), disc.modes.clone()) + let effective = live_model.or_else(|| disc.default_model.clone()); + let current_effort = effective + .as_deref() + .and_then(|id| disc.default_efforts.get(id).cloned()); + (disc.models.clone(), disc.modes.clone(), effective, current_effort) }; emit( &event_tx, @@ -1572,12 +1877,9 @@ async fn reader_task( // so the agent_metadata writeback + the frontend // AvailableCommands push see it (ELECTRON-3PX). slash_commands: builtin_slash_commands(), - // Filled by the follow-up transport - // commit; None keeps this commit - // standalone. - current_model: None, + current_model: current_model_now, current_mode: None, - current_effort: None, + current_effort: current_effort_now, }, ); } @@ -1637,6 +1939,36 @@ async fn reader_task( set = %label, "codex thread/settings/update (SetMode/SetModel/effort) rejected by agent: {message}" ); + // Stale-reject guard: only a rejection of the LATEST + // effort set reconciles state — a superseded set's late + // reject (high -> low -> high) speaks for a value that is + // no longer current; clearing on it would tear down the + // newer, possibly-accepted value. Stale effort rejects + // are logged (the error! above) and claim the pending + // entry, but emit neither the structured signal nor the + // Notice (the level they name may be active again). + let is_stale_effort = label.starts_with("effort\u{2192}") + && latest_effort_set_rpc.load(Ordering::SeqCst) != rid; + if is_stale_effort { + continue; + } + // Structured reject for an EFFORT set (label minted by + // dispatch as "effort→"): the task-level pump + // holds its own optimistic effort highlight (seeded from + // the assistant default) and must clear it so the picker + // stops advertising a level codex refused. Same tag the + // claude reader emits (sniff_set_config_reject). + if let Some(value) = label.strip_prefix("effort\u{2192}") { + emit( + &event_tx, + &session_id, + turn_gen.load(Ordering::SeqCst), + SessionEvent::AdapterSpecific { + tag: "config_option_rejected".to_string(), + payload: json!({ "option_id": "effort", "value": value, "error": message }), + }, + ); + } emit( &event_tx, &session_id, @@ -1894,11 +2226,24 @@ fn fill_discovery(kind: DiscoveryKind, result: &Value, discovered: &Arc { let arr = list("data", "models"); let present = arr.is_some(); + // Defaults ride the same response (capture: model item carries `isDefault` + // and `defaultReasoningEffort` alongside the efforts list). Collected here + // so `capabilities()` can serve current_model/current_effort for a session + // that never switched — before this the currents stayed None and the picker + // showed no active selection (the thought-level display gap). + let mut default_model: Option = None; + let mut default_efforts: std::collections::HashMap = std::collections::HashMap::new(); let models = arr .map(|arr| { arr.iter() .filter_map(|m| { let id = m.get("id").and_then(Value::as_str)?.to_string(); + if m.get("isDefault").and_then(Value::as_bool) == Some(true) && default_model.is_none() { + default_model = Some(id.clone()); + } + if let Some(effort) = m.get("defaultReasoningEffort").and_then(Value::as_str) { + default_efforts.insert(id.clone(), effort.to_string()); + } Some(ModelInfo { id, name: m.get("displayName").and_then(Value::as_str).unwrap_or("").to_string(), @@ -1927,7 +2272,10 @@ fn fill_discovery(kind: DiscoveryKind, result: &Value, discovered: &Arc { // codex's mode axis IS the permission axis. This is the DISCOVERY half of the @@ -3552,29 +3900,11 @@ impl SessionBackend for CodexSessionBackend { Command::SetConfigOption { option_id, value } if matches!(option_id.as_str(), "effort" | "reasoning_effort" | "thought_level") => { - // F-4: between-turn config write → wake a suspended session first. - self.suspend - .ensure_awake(aionui_common::now_ms(), || self.wake_handle()) - .await?; - let tid = self.bound_thread().await?; - let id = self.next_rpc_id(); - // Register the rpc id so the reader claims the response: a JSON-RPC error - // (codex rejected the effort) surfaces as a Notice instead of being - // dropped (success converges via thread/settings/updated). - self.pending_set - .lock() - .await - .insert(id, format!("effort\u{2192}{value}")); - let frame = json!({ - "jsonrpc": "2.0", "id": id, "method": "thread/settings/update", - "params": { "threadId": tid, "effort": value } - }); - self.write_frame(frame).await?; - Ok(CommandReceipt { - accepted: true, - admission: Admission::NoTurn, - turn_gen: self.turn_gen.load(Ordering::SeqCst), - }) + // NOTE (minimal branch): a manual pick racing the detached + // startup seed has no serialization gate — last wire write wins; + // the seed apply is near-instant post-open in practice (see + // Limitations). + self.write_effort_settings_update(&value).await } Command::SetConfigOption { .. } => Err(BackendError::CommandNotSupported { command: "set_config_option", @@ -3614,6 +3944,20 @@ impl SessionBackend for CodexSessionBackend { if !disc.modes.is_empty() { caps.available_modes = disc.modes.clone(); } + // Currents from the discovered defaults, None-only fill (an open-time + // `config.model` seed stays authoritative — the reconcile applies exactly it): + // a thread codex started without an explicit model runs on the `isDefault` + // model at its `defaultReasoningEffort`, so serving them as currents reflects + // what the session actually runs, not a guess. + if caps.current_model.is_none() { + caps.current_model = disc.default_model.clone(); + } + if caps.current_effort.is_none() { + caps.current_effort = caps + .current_model + .as_deref() + .and_then(|id| disc.default_efforts.get(id).cloned()); + } caps } @@ -6679,6 +7023,22 @@ mod tests { "permissionProfile/list built-ins → legacy bare tokens, got {:?}", caps.available_modes ); + // The wire's `isDefault:true` model IS what a thread codex started without an + // explicit model runs on, and its `defaultReasoningEffort` is the effort it + // runs at — both must surface as capabilities currents (the fixture always + // carried them; the parser used to drop them → currents stayed None → the + // picker showed no active model/thinking selection — the #609 direct-path + // regression against the #574 defaults). + assert_eq!( + caps.current_model.as_deref(), + Some("openai.gpt-5.5"), + "isDefault:true surfaces as current_model when config seeded none" + ); + assert_eq!( + caps.current_effort.as_deref(), + Some("medium"), + "the default model's defaultReasoningEffort surfaces as current_effort" + ); } /// The FIX (async catalog-arrival signal): each `model/list` / @@ -6729,6 +7089,602 @@ mod tests { ); } + /// Round-11 P2 (characterization of the KNOWN model-error fail-open): the + /// best-effort drain barrier treats a JSON-RPC ERROR on the model settings + /// write the SAME as a success (pending entry gone == "drained"), so the + /// detached seed still proceeds to the effort write despite the model set + /// having been rejected. This pins the documented limitation (the backend + /// stays authoritative; the reject reconcile corrects the effort) — a + /// result-correlated drain is out of the compact fix's scope. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn model_settings_error_still_lets_effort_attempt_run() { + let started = r#"{"jsonrpc":"2.0","method":"thread/started","params":{"thread":{"id":"th-fo"}}}"#; + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"model-a","displayName":"A","supportedReasoningEfforts":[{"reasoningEffort":"medium"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"medium","isDefault":true},{"id":"model-b","displayName":"B","supportedReasoningEfforts":[{"reasoningEffort":"medium"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"high","isDefault":false}],"nextCursor":null}}"#; + // The model settings write (rpc id 1) gets a JSON-RPC ERROR, released + // only after it is on the wire. + let err1 = "{\"jsonrpc\":\"2.0\",\"id\":1,\"error\":{\"code\":-32000,\"message\":\"model unavailable\"}}\n" + .as_bytes() + .to_vec(); + let fake = + FakeAgentIo::never_exits(format!("{started}\n{model_resp}\n").into_bytes()).with_gated_segments(vec![err1]); + let captured = fake.captured_stdin(); + let release = fake.segment_releaser(); + let backend = Arc::new(CodexSessionBackend::build_with_io("codex-fo", Box::new(fake)).await); + backend.pending_discovery.lock().await.insert(50, DiscoveryKind::Models); + let _events = backend.events(); + for _ in 0..80 { + let filled = !backend + .discovered + .lock() + .unwrap_or_else(|e| e.into_inner()) + .models + .is_empty(); + if filled && backend.thread_binding.lock().await.is_some() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + spawn_codex_bootstrap(backend.clone(), Some("model-b".into()), None, Some("high".into())); + // Model settings write appears. + let mut wire = String::new(); + for _ in 0..80 { + wire = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + if wire.contains("\"model\":\"model-b\"") { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!( + wire.contains("\"model\":\"model-b\""), + "model settings written, got: {wire}" + ); + // Release the ERROR response → the drain treats it as drained (fail-open) + // → the effort write STILL goes out (documented limitation). + release(); + let mut effort_seen = false; + for _ in 0..80 { + wire = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + if wire.contains("\"effort\":\"high\"") { + effort_seen = true; + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!( + effort_seen, + "documented fail-open: a model-set error still lets the effort attempt run, got: {wire}" + ); + } + + /// Round-11 Fix B (production discovery order): permissionProfile/list + /// arrives BEFORE model/list (codex answers modes first), the configured + /// model model-x is absent from the model/list, and the open-time optimistic + /// seed is model-x. Intermediate discovery pushes may still carry model-x, + /// but the LAST CatalogUpdated after the reconcile must be corrected: + /// current_model = the honest default model-a, current_effort = A's default, + /// the modes discovered earlier are RETAINED, model-x is no longer current, + /// and NO thread/settings/update{model:model-x} was ever written. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn permissions_before_model_invalid_configured_model_final_catalog_corrected() { + use futures_util::StreamExt as _; + // Permissions FIRST (id 52), then model/list (id 50) WITHOUT model-x. + let perm_resp = r#"{"jsonrpc":"2.0","id":52,"result":{"data":[{"id":":read-only","description":null},{"id":":workspace","description":null}],"nextCursor":null}}"#; + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"model-a","displayName":"A","supportedReasoningEfforts":[{"reasoningEffort":"medium"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"medium","isDefault":true}],"nextCursor":null}}"#; + let fake = FakeAgentIo::never_exits(format!("{perm_resp}\n{model_resp}\n").into_bytes()); + let captured = fake.captured_stdin(); + let backend = Arc::new(CodexSessionBackend::build_with_io("codex-pbm", Box::new(fake)).await); + { + let mut pd = backend.pending_discovery.lock().await; + pd.insert(52, DiscoveryKind::Permissions); + pd.insert(50, DiscoveryKind::Models); + } + // Open-time optimistic seed for the (invalid) configured model. + *backend.current_model.lock().await = Some("model-x".to_string()); + let mut events = backend.events(); + // Run the reconcile inline; it waits for the model/list, then drops + // model-x and emits the corrected push. Discovery pushes emitted by the + // reader (permissions, then models) buffer in the subscribed receiver. + reconcile_codex_model(&backend, "model-x".to_string()).await; + // Drain every buffered CatalogUpdated + a short tail; keep the LAST. + let mut last_catalog: Option<(Option, Option, Vec)> = None; + for _ in 0..40 { + match tokio::time::timeout(std::time::Duration::from_millis(100), events.next()).await { + Ok(Some(env)) => { + if let SessionEvent::CatalogUpdated { + current_model, + current_effort, + modes, + .. + } = env.event + { + last_catalog = Some(( + current_model, + current_effort, + modes.iter().map(|m| m.id.clone()).collect(), + )); + } + } + Ok(None) => break, + Err(_) => { + if last_catalog.is_some() { + break; + } + } + } + } + let (model, effort, modes) = last_catalog.expect("a corrected CatalogUpdated must be the last frame"); + assert_eq!( + model.as_deref(), + Some("model-a"), + "the final catalog must name the honest default, not model-x" + ); + assert_eq!( + effort.as_deref(), + Some("medium"), + "the final catalog carries model-a's default effort" + ); + assert!( + modes.contains(&"read-only".to_string()) && modes.contains(&"auto".to_string()), + "the modes discovered before the model must be retained, got: {modes:?}" + ); + let wire = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + assert!( + !wire.contains("\"model\":\"model-x\""), + "no thread/settings/update{{model:model-x}} may ever be written for the invalid model, got: {wire}" + ); + } + + /// Round-11 Fix B (empty/timeout branch): when the model/list never + /// populates (reconcile timeout), the optimistic open-time model seed must + /// be CLEARED and corrected currents pushed — otherwise a model/list + /// arriving LATER would run the discovery-emit path with the stale requested + /// seed as current_model and stamp a possibly-invalid model with no one left + /// to correct it. (Uses the real ~5s reconcile poll timeout.) + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn reconcile_timeout_clears_optimistic_model_seed() { + use futures_util::StreamExt as _; + // No model/list is ever fed → discovered stays empty → reconcile times out. + let fake = FakeAgentIo::never_exits(Vec::new()); + let backend = CodexSessionBackend::build_with_io("codex-to", Box::new(fake)).await; + // Open-time optimistic seed for a configured model we can never validate. + *backend.current_model.lock().await = Some("model-x".to_string()); + let mut events = backend.events(); + reconcile_codex_model(&backend, "model-x".to_string()).await; + assert!( + backend.current_model.lock().await.is_none(), + "a reconcile timeout must clear the unvalidated optimistic model seed" + ); + // A corrected CatalogUpdated was emitted (empty catalog → current_model None). + let mut saw_corrected = false; + for _ in 0..20 { + match tokio::time::timeout(std::time::Duration::from_millis(50), events.next()).await { + Ok(Some(env)) => { + if let SessionEvent::CatalogUpdated { current_model, .. } = env.event { + assert!( + current_model.is_none(), + "the corrected push must not name the invalid seed" + ); + saw_corrected = true; + break; + } + } + _ => break, + } + } + assert!(saw_corrected, "the timeout must emit a corrected catalog push"); + } + + /// Round-11 Fix C (backend): a model-switch confirmation + /// (thread/settings/updated{model:B}) must re-emit a CatalogUpdated snapshot + /// carrying B's OWN defaultReasoningEffort — codex sends only the + /// ConfigChanged, never a fresh catalog, so without this the picker keeps + /// A's effort. A/B share the same effort set but different defaults. + #[tokio::test] + async fn model_switch_reemits_catalog_with_new_model_default_effort() { + use futures_util::StreamExt as _; + // model-a default medium, model-b default high; SAME effort set. + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"model-a","displayName":"A","supportedReasoningEfforts":[{"reasoningEffort":"medium"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"medium","isDefault":true},{"id":"model-b","displayName":"B","supportedReasoningEfforts":[{"reasoningEffort":"medium"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"high","isDefault":false}],"nextCursor":null}}"#; + let settings = r#"{"jsonrpc":"2.0","method":"thread/settings/updated","params":{"threadId":"th1","threadSettings":{"model":"model-b","activePermissionProfile":null,"collaborationMode":{"mode":"default","settings":{"model":"model-b"}}}}}"#; + let fake = FakeAgentIo::never_exits(format!("{model_resp}\n{settings}\n").into_bytes()); + let backend = CodexSessionBackend::build_with_io("codex-switch", Box::new(fake)).await; + backend.pending_discovery.lock().await.insert(50, DiscoveryKind::Models); + let mut events = backend.events(); + // Collect until the post-ConfigChanged snapshot for model-b arrives. + let mut saw_config_changed = false; + let mut snapshot_effort = None; + for _ in 0..120 { + match tokio::time::timeout(std::time::Duration::from_millis(200), events.next()).await { + Ok(Some(env)) => match env.event { + SessionEvent::ConfigChanged { model: Some(m), .. } if m == "model-b" => { + saw_config_changed = true; + } + SessionEvent::CatalogUpdated { + current_model, + current_effort, + .. + } if saw_config_changed && current_model.as_deref() == Some("model-b") => { + snapshot_effort = Some(current_effort); + break; + } + _ => {} + }, + _ => break, + } + } + assert!(saw_config_changed, "the switch must emit ConfigChanged{{model-b}}"); + assert_eq!( + snapshot_effort.flatten().as_deref(), + Some("high"), + "the re-emitted snapshot must carry model-b's OWN default effort (high), not A's medium" + ); + } + + /// Round-11 Fix B: dropping an INVALID configured model must emit a CORRECTED + /// CatalogUpdated — an earlier frame may already have advertised the + /// requested-but-invalid model as current, and without the correction it + /// stands forever. The push carries the honest catalog default + its effort. + #[tokio::test] + async fn invalid_configured_model_drop_emits_corrected_catalog() { + use futures_util::StreamExt as _; + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"model-a","displayName":"A","supportedReasoningEfforts":[{"reasoningEffort":"medium"}],"defaultReasoningEffort":"medium","isDefault":true}],"nextCursor":null}}"#; + let fake = FakeAgentIo::never_exits(format!("{model_resp}\n").into_bytes()); + let backend = Arc::new(CodexSessionBackend::build_with_io("codex-drop", Box::new(fake)).await); + backend.pending_discovery.lock().await.insert(50, DiscoveryKind::Models); + // Open-time optimistic seed for the (invalid) configured model. + *backend.current_model.lock().await = Some("model-x".to_string()); + let mut events = backend.events(); + // Wait for the discovery-driven catalog to fill. + for _ in 0..80 { + if !backend + .discovered + .lock() + .unwrap_or_else(|e| e.into_inner()) + .models + .is_empty() + { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + reconcile_codex_model(&backend, "model-x".to_string()).await; + assert!( + backend.current_model.lock().await.is_none(), + "the invalid optimistic seed must be cleared" + ); + let mut corrected = None; + for _ in 0..80 { + match tokio::time::timeout(std::time::Duration::from_millis(200), events.next()).await { + Ok(Some(env)) => { + if let SessionEvent::CatalogUpdated { + current_model, + current_effort, + .. + } = env.event + && current_model.as_deref() == Some("model-a") + { + corrected = Some((current_model, current_effort)); + break; + } + } + _ => break, + } + } + let (_, current_effort) = corrected.expect("the drop must emit a corrected catalog push"); + assert_eq!( + current_effort.as_deref(), + Some("medium"), + "the corrected push carries the honest default currents" + ); + } + + /// The catalog push must also carry the discovered CURRENTS (isDefault model + + /// its defaultReasoningEffort): the task-level event pump holds no backend Arc, + /// so the pushed config-options frame can only highlight what rides the event — + /// before these fields the push always sent `current_value: null` for a session + /// the user had not switched, and the picker showed no active selection. + #[tokio::test] + async fn model_list_response_catalog_updated_carries_currents() { + use futures_util::StreamExt as _; + // Same calibrated capture shape as b_codex_model_list_response_fills_* above + // (samples/codex-cli/0.137.0/appserver-methods/catalog.jsonl). + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"openai.gpt-5.5","displayName":"GPT-5.5","supportedReasoningEfforts":[{"reasoningEffort":"low"},{"reasoningEffort":"medium"}],"defaultReasoningEffort":"medium","isDefault":true}],"nextCursor":null}}"#; + let fake = FakeAgentIo::never_exits(format!("{model_resp}\n").into_bytes()); + let backend = CodexSessionBackend::build_with_io("codex-cur", Box::new(fake)).await; + backend.pending_discovery.lock().await.insert(50, DiscoveryKind::Models); + let mut events = backend.events(); + let mut found = None; + for _ in 0..80 { + if let Ok(Some(env)) = tokio::time::timeout(std::time::Duration::from_millis(200), events.next()).await + && let SessionEvent::CatalogUpdated { + models, + current_model, + current_effort, + .. + } = env.event + && !models.is_empty() + { + found = Some((current_model, current_effort)); + break; + } + } + let (current_model, current_effort) = found.expect("a CatalogUpdated must be broadcast"); + assert_eq!( + current_model.as_deref(), + Some("openai.gpt-5.5"), + "isDefault rides the event" + ); + assert_eq!( + current_effort.as_deref(), + Some("medium"), + "the default model's defaultReasoningEffort rides the event" + ); + } + + /// A rejected `thread/settings/update{effort}` (JSON-RPC error response claimed + /// via `pending_set`) must emit the STRUCTURED `config_option_rejected` signal + /// (in addition to the free-text Notice) so the task-level pump can clear its + /// optimistic effort highlight — the Notice alone left the picker advertising a + /// level codex refused. + /// Round-12 P1-1 (codex): effort set A is written and pending (rpc 1); set + /// B's write FAILS. The rollback must REMOVE B's pending entry AND restore A + /// as the latest effort rpc, so A's LATE JSON-RPC error still reconciles. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn write_error_restores_prior_latest_rpc_so_late_reject_reconciles() { + use futures_util::StreamExt as _; + let started = r#"{"jsonrpc":"2.0","method":"thread/started","params":{"thread":{"id":"th-p11"}}}"#; + // A's late error is for rpc id 1 (the first effort write), gated until B fails. + let reject_a = r#"{"jsonrpc":"2.0","id":1,"error":{"code":-32602,"message":"unsupported reasoning effort"}}"#; + let fake = FakeAgentIo::never_exits(format!("{started}\n").into_bytes()) + .with_gated_tail(format!("{reject_a}\n").into_bytes()); + let release = fake.stdout_releaser(); + let backend = CodexSessionBackend::build_with_io("codex-p11", Box::new(fake)).await; + let mut events = backend.events(); + for _ in 0..80 { + if backend.thread_binding.lock().await.is_some() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + // Set A "high" → rpc 1, pending{1}, latest=1. + backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "high".into(), + }) + .await + .expect("A accepted"); + // Set B "low" with the next write tripped → rollback restores A. + backend.fail_next_write.store(true, std::sync::atomic::Ordering::SeqCst); + let b = backend + .dispatch(Command::SetConfigOption { + option_id: "effort".into(), + value: "low".into(), + }) + .await; + assert!(b.is_err(), "B's tripped write must surface as Err, got: {b:?}"); + assert_eq!( + backend.latest_effort_set_rpc.load(std::sync::atomic::Ordering::SeqCst), + 1, + "the failed B write must RESTORE A (rpc 1) as the latest effort rpc" + ); + assert!( + !backend.pending_set.lock().await.contains_key(&2), + "B's failed pending entry must be removed (no leak)" + ); + // Release A's late reject → it must reconcile (structured signal). + release(); + let signalled = tokio::time::timeout(std::time::Duration::from_secs(2), async { + while let Some(env) = events.next().await { + if matches!(&env.event, SessionEvent::AdapterSpecific { tag, .. } if tag == "config_option_rejected") { + return true; + } + } + false + }) + .await + .unwrap_or(false); + assert!( + signalled, + "A's late reject must reconcile (never dropped as non-latest)" + ); + } + + #[tokio::test] + async fn effort_set_rejection_emits_config_option_rejected() { + use futures_util::StreamExt as _; + let err_resp = r#"{"jsonrpc":"2.0","id":77,"error":{"code":-32602,"message":"unsupported reasoning effort"}}"#; + // Gated tail: the error frame is released only AFTER the pending/latest + // registration below, so the reader can never race past the setup. + let fake = FakeAgentIo::never_exits(Vec::new()).with_gated_tail(format!("{err_resp}\n").into_bytes()); + let release = fake.stdout_releaser(); + let backend = CodexSessionBackend::build_with_io("codex-rej", Box::new(fake)).await; + // Live path: dispatch(SetConfigOption{effort}) registers the rpc id → label + // AND marks it as the latest effort set (stale-reject guard). + backend + .latest_effort_set_rpc + .store(77, std::sync::atomic::Ordering::SeqCst); + backend + .pending_set + .lock() + .await + .insert(77, "effort\u{2192}xhigh".to_string()); + let mut events = backend.events(); + release(); + let mut rejected = None; + let mut saw_notice = false; + for _ in 0..80 { + match tokio::time::timeout(std::time::Duration::from_millis(200), events.next()).await { + Ok(Some(env)) => match env.event { + SessionEvent::AdapterSpecific { tag, payload } if tag == "config_option_rejected" => { + rejected = Some(payload); + } + SessionEvent::Notice { .. } => { + saw_notice = true; + break; + } + _ => {} + }, + _ => break, + } + } + let payload = rejected.expect("a rejected effort set must emit config_option_rejected"); + assert_eq!(payload.get("option_id").and_then(Value::as_str), Some("effort")); + assert_eq!(payload.get("value").and_then(Value::as_str), Some("xhigh")); + assert!(saw_notice, "the user-facing Notice still rides alongside"); + } + + /// P: a LATE reject of a SUPERSEDED effort set must emit neither the structured + /// signal nor the Notice — high→low→high where the first high's JSON-RPC error + /// arrives after the third set; correlated by rpc id against the latest set. + #[tokio::test] + async fn stale_effort_set_rejection_is_ignored() { + use futures_util::StreamExt as _; + let err_resp = r#"{"jsonrpc":"2.0","id":77,"error":{"code":-32602,"message":"unsupported reasoning effort"}}"#; + let fake = FakeAgentIo::never_exits(Vec::new()).with_gated_tail(format!("{err_resp}\n").into_bytes()); + let release = fake.stdout_releaser(); + let backend = CodexSessionBackend::build_with_io("codex-stale", Box::new(fake)).await; + backend + .pending_set + .lock() + .await + .insert(77, "effort\u{2192}high".to_string()); + // Two newer sets happened; rpc 93 is the latest. + backend + .latest_effort_set_rpc + .store(93, std::sync::atomic::Ordering::SeqCst); + let mut events = backend.events(); + release(); + let saw_reconcile = tokio::time::timeout(std::time::Duration::from_millis(600), async { + while let Some(env) = events.next().await { + match env.event { + SessionEvent::Notice { .. } => return true, + SessionEvent::AdapterSpecific { ref tag, .. } if tag == "config_option_rejected" => return true, + _ => {} + } + } + false + }) + .await + .unwrap_or(false); + assert!( + !saw_reconcile, + "a stale reject must emit neither Notice nor structured signal" + ); + assert!( + backend.pending_set.lock().await.is_empty(), + "the stale pending entry is still claimed (no leak)" + ); + } + + /// A CONFIGURED session's catalog push must confirm the REQUESTED model (the + /// live tracked current), not reset the highlight to the catalog's isDefault — + /// and the effort current must follow that model's defaultReasoningEffort. + #[tokio::test] + async fn model_list_catalog_updated_prefers_configured_model_over_default() { + use futures_util::StreamExt as _; + // Both models carry defaults (calibrated shape); isDefault is gpt-5.5, but + // the session was CONFIGURED for gpt-5.4. + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"openai.gpt-5.5","displayName":"GPT-5.5","supportedReasoningEfforts":[{"reasoningEffort":"low"},{"reasoningEffort":"medium"}],"defaultReasoningEffort":"medium","isDefault":true},{"id":"openai.gpt-5.4","displayName":"gpt-5.4","supportedReasoningEfforts":[{"reasoningEffort":"low"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"high","isDefault":false}],"nextCursor":null}}"#; + let fake = FakeAgentIo::never_exits(format!("{model_resp}\n").into_bytes()); + let backend = CodexSessionBackend::build_with_io("codex-cfg", Box::new(fake)).await; + // Live path: `spawn` seeds the tracked current from config.model. + *backend.current_model.lock().await = Some("openai.gpt-5.4".to_string()); + backend.pending_discovery.lock().await.insert(50, DiscoveryKind::Models); + let mut events = backend.events(); + let mut found = None; + for _ in 0..80 { + if let Ok(Some(env)) = tokio::time::timeout(std::time::Duration::from_millis(200), events.next()).await + && let SessionEvent::CatalogUpdated { + models, + current_model, + current_effort, + .. + } = env.event + && !models.is_empty() + { + found = Some((current_model, current_effort)); + break; + } + } + let (current_model, current_effort) = found.expect("a CatalogUpdated must be broadcast"); + assert_eq!( + current_model.as_deref(), + Some("openai.gpt-5.4"), + "the configured/tracked model wins over the catalog isDefault" + ); + assert_eq!( + current_effort.as_deref(), + Some("high"), + "the effort current follows the CONFIGURED model's defaultReasoningEffort" + ); + } + + /// The DETACHED startup seed on the wire (minimal branch — no first-turn + /// gate): configured non-default model B with a seeded effort. Expected + /// outbound order: settings{model B} → its ack consumed (the best-effort + /// drain barrier) → settings{effort}. The first turn is NOT gated on these + /// writes (see Limitations). + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn detached_seed_applies_model_then_effort_in_order() { + let started = r#"{"jsonrpc":"2.0","method":"thread/started","params":{"thread":{"id":"th-boot"}}}"#; + let model_resp = r#"{"jsonrpc":"2.0","id":50,"result":{"data":[{"id":"model-a","displayName":"A","supportedReasoningEfforts":[{"reasoningEffort":"low"},{"reasoningEffort":"medium"}],"defaultReasoningEffort":"medium","isDefault":true},{"id":"model-b","displayName":"B","supportedReasoningEfforts":[{"reasoningEffort":"low"},{"reasoningEffort":"high"}],"defaultReasoningEffort":"high","isDefault":false}],"nextCursor":null}}"#; + let ack1 = "{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":{\"ok\":true}}\n" + .as_bytes() + .to_vec(); + let fake = + FakeAgentIo::never_exits(format!("{started}\n{model_resp}\n").into_bytes()).with_gated_segments(vec![ack1]); + let captured = fake.captured_stdin(); + let release = fake.segment_releaser(); + let backend = Arc::new(CodexSessionBackend::build_with_io("codex-boot", Box::new(fake)).await); + backend.pending_discovery.lock().await.insert(50, DiscoveryKind::Models); + let _events = backend.events(); + for _ in 0..80 { + let filled = !backend + .discovered + .lock() + .unwrap_or_else(|e| e.into_inner()) + .models + .is_empty(); + if filled && backend.thread_binding.lock().await.is_some() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + spawn_codex_bootstrap(backend.clone(), Some("model-b".into()), None, Some("high".into())); + // Model settings write first; the effort write waits for the model ACK + // (best-effort drain barrier keeps the wire order deterministic). + let mut wire = String::new(); + for _ in 0..80 { + wire = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + if wire.contains("\"model\":\"model-b\"") { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!( + wire.contains("\"model\":\"model-b\""), + "model settings written, got: {wire}" + ); + assert!( + !wire.contains("\"effort\""), + "the effort write waits for the model ACK: {wire}" + ); + release(); + for _ in 0..80 { + wire = String::from_utf8_lossy(&captured.lock().await.clone()).to_string(); + if wire.contains("\"effort\":\"high\"") { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + let model_at = wire.find("\"model\":\"model-b\"").expect("model settings on wire"); + let effort_at = wire.find("\"effort\":\"high\"").expect("effort settings on wire"); + assert!( + model_at < effort_at, + "outbound order must be model → effort, got: {wire}" + ); + } + /// Cross-version fallback (README discipline #9): if a future codex renamed the /// model wrapper back to `models` or emitted bare-string reasoning efforts, the /// parser must still degrade gracefully (data-first, legacy-fallback) — so a rename diff --git a/crates/aionui-session/src/backend/mod.rs b/crates/aionui-session/src/backend/mod.rs index 7817e4b2e..0df172d07 100644 --- a/crates/aionui-session/src/backend/mod.rs +++ b/crates/aionui-session/src/backend/mod.rs @@ -251,4 +251,17 @@ pub struct SessionConfig { /// into the F-4 wake recipe (rides the cloned `config`) so a resume re-spawn /// uses the same binary (R16 continuity). pub cli_program: Option, + /// The resolved initial reasoning-effort ("thought level") for the session. + /// codex applies it inside its DETACHED post-open startup sequence — validated + /// model reconcile → best-effort wait for the model settings response → + /// validated effort (against the EFFECTIVE model's advertised efforts) — with + /// NO first-turn gate: codex's `thread/settings/update{effort}` only affects + /// SUBSEQUENT turns (ThreadSettingsUpdateParams, + /// samples/codex-cli/0.137.0/schema-full), so an effort applied after (or + /// racing) the first `turn/start` may briefly run that turn at the launch + /// default (a declared limitation of this compact fix). claude ignores this + /// field: its effort rides a post-open control_request the orchestration layer + /// sequences before the first prompt (stdin ordering is the claude contract). + /// `None` = no seed. + pub reasoning_effort: Option, }