From 567ad7fafd332c237e99f4d9eb69bf5dd0bb9842 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Mar 2026 17:46:25 -0700 Subject: [PATCH 1/7] Show spawned agent model and effort in TUI (#14273) - include the requested sub-agent model and reasoning effort in the spawn begin event\n- render that metadata next to the spawned agent name and role in the TUI transcript --------- Co-authored-by: Codex --- .../schema/json/EventMsg.json | 16 ++++ .../codex_app_server_protocol.schemas.json | 8 ++ .../codex_app_server_protocol.v2.schemas.json | 8 ++ .../typescript/CollabAgentSpawnBeginEvent.ts | 3 +- .../core/src/tools/handlers/multi_agents.rs | 2 + .../src/event_processor_with_human_output.rs | 1 + .../tests/event_processor_with_json_output.rs | 3 + codex-rs/protocol/src/protocol.rs | 2 + codex-rs/tui/src/chatwidget.rs | 25 ++++- codex-rs/tui/src/chatwidget/tests.rs | 46 +++++++++ codex-rs/tui/src/multi_agents.rs | 95 +++++++++++++++---- ...gents__tests__collab_agent_transcript.snap | 2 +- 12 files changed, 186 insertions(+), 25 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/EventMsg.json b/codex-rs/app-server-protocol/schema/json/EventMsg.json index 9de2021690..dbf9fc8e9f 100644 --- a/codex-rs/app-server-protocol/schema/json/EventMsg.json +++ b/codex-rs/app-server-protocol/schema/json/EventMsg.json @@ -3015,10 +3015,16 @@ "description": "Identifier for the collab tool call.", "type": "string" }, + "model": { + "type": "string" + }, "prompt": { "description": "Initial prompt sent to the agent. Can be empty to prevent CoT leaking at the beginning.", "type": "string" }, + "reasoning_effort": { + "$ref": "#/definitions/ReasoningEffort" + }, "sender_thread_id": { "allOf": [ { @@ -3037,7 +3043,9 @@ }, "required": [ "call_id", + "model", "prompt", + "reasoning_effort", "sender_thread_id", "type" ], @@ -9144,10 +9152,16 @@ "description": "Identifier for the collab tool call.", "type": "string" }, + "model": { + "type": "string" + }, "prompt": { "description": "Initial prompt sent to the agent. Can be empty to prevent CoT leaking at the beginning.", "type": "string" }, + "reasoning_effort": { + "$ref": "#/definitions/ReasoningEffort" + }, "sender_thread_id": { "allOf": [ { @@ -9166,7 +9180,9 @@ }, "required": [ "call_id", + "model", "prompt", + "reasoning_effort", "sender_thread_id", "type" ], diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6e40a6eb2a..0a8dd747f1 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -4378,10 +4378,16 @@ "description": "Identifier for the collab tool call.", "type": "string" }, + "model": { + "type": "string" + }, "prompt": { "description": "Initial prompt sent to the agent. Can be empty to prevent CoT leaking at the beginning.", "type": "string" }, + "reasoning_effort": { + "$ref": "#/definitions/v2/ReasoningEffort" + }, "sender_thread_id": { "allOf": [ { @@ -4400,7 +4406,9 @@ }, "required": [ "call_id", + "model", "prompt", + "reasoning_effort", "sender_thread_id", "type" ], diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 90c576612e..79add1f96e 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -6180,10 +6180,16 @@ "description": "Identifier for the collab tool call.", "type": "string" }, + "model": { + "type": "string" + }, "prompt": { "description": "Initial prompt sent to the agent. Can be empty to prevent CoT leaking at the beginning.", "type": "string" }, + "reasoning_effort": { + "$ref": "#/definitions/ReasoningEffort" + }, "sender_thread_id": { "allOf": [ { @@ -6202,7 +6208,9 @@ }, "required": [ "call_id", + "model", "prompt", + "reasoning_effort", "sender_thread_id", "type" ], diff --git a/codex-rs/app-server-protocol/schema/typescript/CollabAgentSpawnBeginEvent.ts b/codex-rs/app-server-protocol/schema/typescript/CollabAgentSpawnBeginEvent.ts index a86598e20c..5f86922442 100644 --- a/codex-rs/app-server-protocol/schema/typescript/CollabAgentSpawnBeginEvent.ts +++ b/codex-rs/app-server-protocol/schema/typescript/CollabAgentSpawnBeginEvent.ts @@ -1,6 +1,7 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { ReasoningEffort } from "./ReasoningEffort"; import type { ThreadId } from "./ThreadId"; export type CollabAgentSpawnBeginEvent = { @@ -16,4 +17,4 @@ sender_thread_id: ThreadId, * Initial prompt sent to the agent. Can be empty to prevent CoT leaking at the * beginning. */ -prompt: string, }; +prompt: string, model: string, reasoning_effort: ReasoningEffort, }; diff --git a/codex-rs/core/src/tools/handlers/multi_agents.rs b/codex-rs/core/src/tools/handlers/multi_agents.rs index abcf9de4cb..54e146518a 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents.rs @@ -157,6 +157,8 @@ mod spawn { call_id: call_id.clone(), sender_thread_id: session.conversation_id, prompt: prompt.clone(), + model: args.model.clone().unwrap_or_default(), + reasoning_effort: args.reasoning_effort.unwrap_or_default(), } .into(), ) diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 5c6cd1c474..79c0f1b695 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -698,6 +698,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { call_id, sender_thread_id: _, prompt, + .. }) => { ts_msg!( self, diff --git a/codex-rs/exec/tests/event_processor_with_json_output.rs b/codex-rs/exec/tests/event_processor_with_json_output.rs index a051b5bb00..e31da9dc67 100644 --- a/codex-rs/exec/tests/event_processor_with_json_output.rs +++ b/codex-rs/exec/tests/event_processor_with_json_output.rs @@ -34,6 +34,7 @@ use codex_protocol::ThreadId; use codex_protocol::config_types::ModeKind; use codex_protocol::mcp::CallToolResult; use codex_protocol::models::WebSearchAction; +use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; @@ -547,6 +548,8 @@ fn collab_spawn_begin_and_end_emit_item_events() { call_id: "call-10".to_string(), sender_thread_id, prompt: prompt.clone(), + model: "gpt-5".to_string(), + reasoning_effort: ReasoningEffortConfig::default(), }), ); let begin_events = ep.collect_thread_events(&begin); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index e76ae07643..2d7c63a753 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -3132,6 +3132,8 @@ pub struct CollabAgentSpawnBeginEvent { /// Initial prompt sent to the agent. Can be empty to prevent CoT leaking at the /// beginning. pub prompt: String, + pub model: String, + pub reasoning_effort: ReasoningEffortConfig, } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 7fdc294dac..70b524466c 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -101,6 +101,7 @@ use codex_protocol::protocol::AgentReasoningRawContentEvent; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::BackgroundEventEvent; use codex_protocol::protocol::CodexErrorInfo; +use codex_protocol::protocol::CollabAgentSpawnBeginEvent; use codex_protocol::protocol::CreditsSnapshot; use codex_protocol::protocol::DeprecationNoticeEvent; use codex_protocol::protocol::ErrorEvent; @@ -579,6 +580,7 @@ pub(crate) struct ChatWidget { // Latest completed user-visible Codex output that `/copy` should place on the clipboard. last_copyable_output: Option, running_commands: HashMap, + pending_collab_spawn_requests: HashMap, suppressed_exec_calls: HashSet, skills_all: Vec, skills_initial_state: Option>, @@ -3243,6 +3245,7 @@ impl ChatWidget { plan_stream_controller: None, last_copyable_output: None, running_commands: HashMap::new(), + pending_collab_spawn_requests: HashMap::new(), suppressed_exec_calls: HashSet::new(), last_unified_wait: None, unified_exec_wait_streak: None, @@ -3427,6 +3430,7 @@ impl ChatWidget { plan_stream_controller: None, last_copyable_output: None, running_commands: HashMap::new(), + pending_collab_spawn_requests: HashMap::new(), suppressed_exec_calls: HashSet::new(), last_unified_wait: None, unified_exec_wait_streak: None, @@ -3603,6 +3607,7 @@ impl ChatWidget { plan_stream_controller: None, last_copyable_output: None, running_commands: HashMap::new(), + pending_collab_spawn_requests: HashMap::new(), suppressed_exec_calls: HashSet::new(), last_unified_wait: None, unified_exec_wait_streak: None, @@ -4999,8 +5004,24 @@ impl ChatWidget { } EventMsg::ExitedReviewMode(review) => self.on_exited_review_mode(review), EventMsg::ContextCompacted(_) => self.on_agent_message("Context compacted".to_owned()), - EventMsg::CollabAgentSpawnBegin(_) => {} - EventMsg::CollabAgentSpawnEnd(ev) => self.on_collab_event(multi_agents::spawn_end(ev)), + EventMsg::CollabAgentSpawnBegin(CollabAgentSpawnBeginEvent { + call_id, + model, + reasoning_effort, + .. + }) => { + self.pending_collab_spawn_requests.insert( + call_id, + multi_agents::SpawnRequestSummary { + model, + reasoning_effort, + }, + ); + } + EventMsg::CollabAgentSpawnEnd(ev) => { + let spawn_request = self.pending_collab_spawn_requests.remove(&ev.call_id); + self.on_collab_event(multi_agents::spawn_end(ev, spawn_request.as_ref())); + } EventMsg::CollabAgentInteractionBegin(_) => {} EventMsg::CollabAgentInteractionEnd(ev) => { self.on_collab_event(multi_agents::interaction_end(ev)) diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 18f1f83d8f..c18d1070be 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -58,9 +58,12 @@ use codex_protocol::protocol::AgentMessageDeltaEvent; use codex_protocol::protocol::AgentMessageEvent; use codex_protocol::protocol::AgentReasoningDeltaEvent; use codex_protocol::protocol::AgentReasoningEvent; +use codex_protocol::protocol::AgentStatus; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::BackgroundEventEvent; use codex_protocol::protocol::CodexErrorInfo; +use codex_protocol::protocol::CollabAgentSpawnBeginEvent; +use codex_protocol::protocol::CollabAgentSpawnEndEvent; use codex_protocol::protocol::CreditsSnapshot; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -1838,6 +1841,7 @@ async fn make_chatwidget_manual( plan_stream_controller: None, last_copyable_output: None, running_commands: HashMap::new(), + pending_collab_spawn_requests: HashMap::new(), suppressed_exec_calls: HashSet::new(), skills_all: Vec::new(), skills_initial_state: None, @@ -2011,6 +2015,48 @@ fn lines_to_single_string(lines: &[ratatui::text::Line<'static>]) -> String { s } +#[tokio::test] +async fn collab_spawn_end_shows_requested_model_and_effort() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(None).await; + let sender_thread_id = ThreadId::new(); + let spawned_thread_id = ThreadId::new(); + + chat.handle_codex_event(Event { + id: "spawn-begin".into(), + msg: EventMsg::CollabAgentSpawnBegin(CollabAgentSpawnBeginEvent { + call_id: "call-spawn".to_string(), + sender_thread_id, + prompt: "Explore the repo".to_string(), + model: "gpt-5".to_string(), + reasoning_effort: ReasoningEffortConfig::High, + }), + }); + chat.handle_codex_event(Event { + id: "spawn-end".into(), + msg: EventMsg::CollabAgentSpawnEnd(CollabAgentSpawnEndEvent { + call_id: "call-spawn".to_string(), + sender_thread_id, + new_thread_id: Some(spawned_thread_id), + new_agent_nickname: Some("Robie".to_string()), + new_agent_role: Some("explorer".to_string()), + prompt: "Explore the repo".to_string(), + status: AgentStatus::PendingInit, + }), + }); + + let cells = drain_insert_history(&mut rx); + let rendered = cells + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::>() + .join("\n"); + + assert!( + rendered.contains("Spawned Robie [explorer] (gpt-5 high)"), + "expected spawn line to include agent metadata and requested model, got {rendered:?}" + ); +} + fn status_line_text(chat: &ChatWidget) -> Option { chat.status_line_text() } diff --git a/codex-rs/tui/src/multi_agents.rs b/codex-rs/tui/src/multi_agents.rs index 7161e8880d..3a90f77418 100644 --- a/codex-rs/tui/src/multi_agents.rs +++ b/codex-rs/tui/src/multi_agents.rs @@ -2,6 +2,7 @@ use crate::history_cell::PlainHistoryCell; use crate::render::line_utils::prefix_lines; use crate::text_formatting::truncate_text; use codex_protocol::ThreadId; +use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::protocol::AgentStatus; use codex_protocol::protocol::CollabAgentInteractionEndEvent; use codex_protocol::protocol::CollabAgentRef; @@ -36,6 +37,12 @@ struct AgentLabel<'a> { role: Option<&'a str>, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct SpawnRequestSummary { + pub(crate) model: String, + pub(crate) reasoning_effort: ReasoningEffortConfig, +} + pub(crate) fn agent_picker_status_dot_spans(is_closed: bool) -> Vec> { let dot = if is_closed { "•".into() @@ -74,7 +81,10 @@ pub(crate) fn sort_agent_picker_threads(agent_threads: &mut [(ThreadId, AgentPic }); } -pub(crate) fn spawn_end(ev: CollabAgentSpawnEndEvent) -> PlainHistoryCell { +pub(crate) fn spawn_end( + ev: CollabAgentSpawnEndEvent, + spawn_request: Option<&SpawnRequestSummary>, +) -> PlainHistoryCell { let CollabAgentSpawnEndEvent { call_id: _, sender_thread_id: _, @@ -93,6 +103,7 @@ pub(crate) fn spawn_end(ev: CollabAgentSpawnEndEvent) -> PlainHistoryCell { nickname: new_agent_nickname.as_deref(), role: new_agent_role.as_deref(), }, + spawn_request, ), None => title_text("Agent spawn failed"), }; @@ -122,6 +133,7 @@ pub(crate) fn interaction_end(ev: CollabAgentInteractionEndEvent) -> PlainHistor nickname: receiver_agent_nickname.as_deref(), role: receiver_agent_role.as_deref(), }, + None, ); let mut details = Vec::new(); @@ -141,7 +153,7 @@ pub(crate) fn waiting_begin(ev: CollabWaitingBeginEvent) -> PlainHistoryCell { let receiver_agents = merge_wait_receivers(&receiver_thread_ids, receiver_agents); let title = match receiver_agents.as_slice() { - [receiver] => title_with_agent("Waiting for", agent_label_from_ref(receiver)), + [receiver] => title_with_agent("Waiting for", agent_label_from_ref(receiver), None), [] => title_text("Waiting for agents"), _ => title_text(format!("Waiting for {} agents", receiver_agents.len())), }; @@ -187,6 +199,7 @@ pub(crate) fn close_end(ev: CollabCloseEndEvent) -> PlainHistoryCell { nickname: receiver_agent_nickname.as_deref(), role: receiver_agent_role.as_deref(), }, + None, ), Vec::new(), ) @@ -209,6 +222,7 @@ pub(crate) fn resume_begin(ev: CollabResumeBeginEvent) -> PlainHistoryCell { nickname: receiver_agent_nickname.as_deref(), role: receiver_agent_role.as_deref(), }, + None, ), Vec::new(), ) @@ -232,6 +246,7 @@ pub(crate) fn resume_end(ev: CollabResumeEndEvent) -> PlainHistoryCell { nickname: receiver_agent_nickname.as_deref(), role: receiver_agent_role.as_deref(), }, + None, ), vec![status_summary_line(&status)], ) @@ -249,9 +264,14 @@ fn title_text(title: impl Into) -> Line<'static> { title_spans_line(vec![Span::from(title.into()).bold()]) } -fn title_with_agent(prefix: &str, agent: AgentLabel<'_>) -> Line<'static> { +fn title_with_agent( + prefix: &str, + agent: AgentLabel<'_>, + spawn_request: Option<&SpawnRequestSummary>, +) -> Line<'static> { let mut spans = vec![Span::from(format!("{prefix} ")).bold()]; spans.extend(agent_label_spans(agent)); + spans.extend(spawn_request_spans(spawn_request)); title_spans_line(spans) } @@ -298,6 +318,25 @@ fn agent_label_spans(agent: AgentLabel<'_>) -> Vec> { spans } +fn spawn_request_spans(spawn_request: Option<&SpawnRequestSummary>) -> Vec> { + let Some(spawn_request) = spawn_request else { + return Vec::new(); + }; + + let model = spawn_request.model.trim(); + if model.is_empty() && spawn_request.reasoning_effort == ReasoningEffortConfig::default() { + return Vec::new(); + } + + let details = if model.is_empty() { + format!("({})", spawn_request.reasoning_effort) + } else { + format!("({model} {})", spawn_request.reasoning_effort) + }; + + vec![Span::from(" ").dim(), Span::from(details).magenta()] +} + fn prompt_line(prompt: &str) -> Option> { let trimmed = prompt.trim(); if trimmed.is_empty() { @@ -460,15 +499,21 @@ mod tests { let bob_id = ThreadId::from_string("00000000-0000-0000-0000-000000000003") .expect("valid bob thread id"); - let spawn = spawn_end(CollabAgentSpawnEndEvent { - call_id: "call-spawn".to_string(), - sender_thread_id, - new_thread_id: Some(robie_id), - new_agent_nickname: Some("Robie".to_string()), - new_agent_role: Some("explorer".to_string()), - prompt: "Compute 11! and reply with just the integer result.".to_string(), - status: AgentStatus::PendingInit, - }); + let spawn = spawn_end( + CollabAgentSpawnEndEvent { + call_id: "call-spawn".to_string(), + sender_thread_id, + new_thread_id: Some(robie_id), + new_agent_nickname: Some("Robie".to_string()), + new_agent_role: Some("explorer".to_string()), + prompt: "Compute 11! and reply with just the integer result.".to_string(), + status: AgentStatus::PendingInit, + }, + Some(&SpawnRequestSummary { + model: "gpt-5".to_string(), + reasoning_effort: ReasoningEffortConfig::High, + }), + ); let send = interaction_end(CollabAgentInteractionEndEvent { call_id: "call-send".to_string(), @@ -540,15 +585,21 @@ mod tests { .expect("valid sender thread id"); let robie_id = ThreadId::from_string("00000000-0000-0000-0000-000000000002") .expect("valid robie thread id"); - let cell = spawn_end(CollabAgentSpawnEndEvent { - call_id: "call-spawn".to_string(), - sender_thread_id, - new_thread_id: Some(robie_id), - new_agent_nickname: Some("Robie".to_string()), - new_agent_role: Some("explorer".to_string()), - prompt: String::new(), - status: AgentStatus::PendingInit, - }); + let cell = spawn_end( + CollabAgentSpawnEndEvent { + call_id: "call-spawn".to_string(), + sender_thread_id, + new_thread_id: Some(robie_id), + new_agent_nickname: Some("Robie".to_string()), + new_agent_role: Some("explorer".to_string()), + prompt: String::new(), + status: AgentStatus::PendingInit, + }, + Some(&SpawnRequestSummary { + model: "gpt-5".to_string(), + reasoning_effort: ReasoningEffortConfig::High, + }), + ); let lines = cell.display_lines(200); let title = &lines[0]; @@ -558,6 +609,8 @@ mod tests { assert_eq!(title.spans[4].content.as_ref(), "[explorer]"); assert_eq!(title.spans[4].style.fg, None); assert!(!title.spans[4].style.add_modifier.contains(Modifier::DIM)); + assert_eq!(title.spans[6].content.as_ref(), "(gpt-5 high)"); + assert_eq!(title.spans[6].style.fg, Some(Color::Magenta)); } fn cell_to_text(cell: &PlainHistoryCell) -> String { diff --git a/codex-rs/tui/src/snapshots/codex_tui__multi_agents__tests__collab_agent_transcript.snap b/codex-rs/tui/src/snapshots/codex_tui__multi_agents__tests__collab_agent_transcript.snap index 19001a70df..2bc6083fcd 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__multi_agents__tests__collab_agent_transcript.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__multi_agents__tests__collab_agent_transcript.snap @@ -2,7 +2,7 @@ source: tui/src/multi_agents.rs expression: snapshot --- -• Spawned Robie [explorer] +• Spawned Robie [explorer] (gpt-5 high) └ Compute 11! and reply with just the integer result. • Sent input to Robie [explorer] From 3f7cb030438c5f911b11fb66913e46ffa0570f50 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Mar 2026 17:59:41 -0700 Subject: [PATCH 2/7] Stabilize websocket response.failed error delivery (#14017) ## What changed - Drop failed websocket connections immediately after a terminal stream error instead of awaiting a graceful close handshake before forwarding the error to the caller. - Keep the success path and the closed-connection guard behavior unchanged. ## Why this fixes the flake - The failing integration test waits for the second websocket stream to surface the model error before issuing a follow-up request. - On slower runners, the old error path awaited `ws_stream.close().await` before sending the error downstream. If that close handshake stalled, the test kept waiting for an error that had already happened server-side and nextest timed it out. - Dropping the failed websocket immediately makes the terminal error observable right away and marks the session closed so the next request reconnects cleanly instead of depending on a best-effort close handshake. ## Code or test? - This is a production logic fix in `codex-api`. The existing websocket integration test already exercises the regression path. --- .../src/endpoint/responses_websocket.rs | 55 ++++++++--------- codex-rs/core/tests/common/responses.rs | 14 ++++- codex-rs/core/tests/suite/agent_websocket.rs | 1 + .../core/tests/suite/client_websockets.rs | 61 +++++++++++++++++++ codex-rs/core/tests/suite/turn_state.rs | 3 + 5 files changed, 102 insertions(+), 32 deletions(-) diff --git a/codex-rs/codex-api/src/endpoint/responses_websocket.rs b/codex-rs/codex-api/src/endpoint/responses_websocket.rs index 925f7d52d0..30af9783a9 100644 --- a/codex-rs/codex-api/src/endpoint/responses_websocket.rs +++ b/codex-rs/codex-api/src/endpoint/responses_websocket.rs @@ -53,9 +53,6 @@ enum WsCommand { message: Message, tx_result: oneshot::Sender>, }, - Close { - tx_result: oneshot::Sender>, - }, } impl WsStream { @@ -80,11 +77,6 @@ impl WsStream { break; } } - WsCommand::Close { tx_result } => { - let result = inner.close(None).await; - let _ = tx_result.send(result); - break; - } } } message = inner.next() => { @@ -144,11 +136,6 @@ impl WsStream { .await } - async fn close(&self) -> Result<(), WsError> { - self.request(|tx_result| WsCommand::Close { tx_result }) - .await - } - async fn next(&mut self) -> Option> { self.rx_message.recv().await } @@ -242,26 +229,32 @@ impl ResponsesWebsocketConnection { .await; } let mut guard = stream.lock().await; - let Some(ws_stream) = guard.as_mut() else { - let _ = tx_event - .send(Err(ApiError::Stream( - "websocket connection is closed".to_string(), - ))) - .await; - return; + let result = { + let Some(ws_stream) = guard.as_mut() else { + let _ = tx_event + .send(Err(ApiError::Stream( + "websocket connection is closed".to_string(), + ))) + .await; + return; + }; + + run_websocket_response_stream( + ws_stream, + tx_event.clone(), + request_body, + idle_timeout, + telemetry, + ) + .await }; - if let Err(err) = run_websocket_response_stream( - ws_stream, - tx_event.clone(), - request_body, - idle_timeout, - telemetry, - ) - .await - { - let _ = ws_stream.close().await; - *guard = None; + if let Err(err) = result { + // A terminal stream error should reach the caller immediately. Waiting for a + // graceful close handshake here can stall indefinitely and mask the error. + let failed_stream = guard.take(); + drop(guard); + drop(failed_stream); let _ = tx_event.send(Err(err)).await; } }); diff --git a/codex-rs/core/tests/common/responses.rs b/codex-rs/core/tests/common/responses.rs index d07b155f61..cf7c03f4df 100644 --- a/codex-rs/core/tests/common/responses.rs +++ b/codex-rs/core/tests/common/responses.rs @@ -416,6 +416,11 @@ pub struct WebSocketConnectionConfig { /// Tests use this to force websocket setup into an in-flight state so first-turn warmup paths /// can be exercised deterministically. pub accept_delay: Option, + /// Whether the server should send a websocket close frame after all scripted responses. + /// + /// Tests can disable this to simulate a peer that surfaces a terminal event but never + /// completes the close handshake. + pub close_after_requests: bool, } pub struct WebSocketTestServer { @@ -1168,6 +1173,7 @@ pub async fn start_websocket_server(connections: Vec>>) -> WebSoc requests, response_headers: Vec::new(), accept_delay: None, + close_after_requests: true, }) .collect(); start_websocket_server_with_headers(connections).await @@ -1261,6 +1267,7 @@ pub async fn start_websocket_server_with_headers( log.push(Vec::new()); log.len() - 1 }; + let close_after_requests = connection.close_after_requests; for request_events in connection.requests { let Some(Ok(message)) = ws_stream.next().await else { break; @@ -1324,7 +1331,12 @@ pub async fn start_websocket_server_with_headers( } } - let _ = ws_stream.close(None).await; + if close_after_requests { + let _ = ws_stream.close(None).await; + } else { + let _ = shutdown_rx.await; + return; + } if connections.lock().unwrap().is_empty() { return; diff --git a/codex-rs/core/tests/suite/agent_websocket.rs b/codex-rs/core/tests/suite/agent_websocket.rs index 5e81452a49..45752f1826 100644 --- a/codex-rs/core/tests/suite/agent_websocket.rs +++ b/codex-rs/core/tests/suite/agent_websocket.rs @@ -129,6 +129,7 @@ async fn websocket_first_turn_handles_handshake_delay_with_startup_prewarm() -> response_headers: Vec::new(), // Delay handshake so turn processing must tolerate websocket startup latency. accept_delay: Some(Duration::from_millis(150)), + close_after_requests: true, }]) .await; diff --git a/codex-rs/core/tests/suite/client_websockets.rs b/codex-rs/core/tests/suite/client_websockets.rs index cda634448c..0850f6e540 100755 --- a/codex-rs/core/tests/suite/client_websockets.rs +++ b/codex-rs/core/tests/suite/client_websockets.rs @@ -653,6 +653,7 @@ async fn responses_websocket_emits_reasoning_included_event() { requests: vec![vec![ev_response_created("resp-1"), ev_completed("resp-1")]], response_headers: vec![("X-Reasoning-Included".to_string(), "true".to_string())], accept_delay: None, + close_after_requests: true, }]) .await; @@ -725,6 +726,7 @@ async fn responses_websocket_emits_rate_limit_events() { ("X-Reasoning-Included".to_string(), "true".to_string()), ], accept_delay: None, + close_after_requests: true, }]) .await; @@ -1369,6 +1371,65 @@ async fn responses_websocket_v2_after_error_uses_full_create_without_previous_re server.shutdown().await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn responses_websocket_v2_surfaces_terminal_error_without_close_handshake() { + skip_if_no_network!(); + + let server = start_websocket_server_with_headers(vec![WebSocketConnectionConfig { + requests: vec![ + vec![ev_response_created("resp-1"), ev_completed("resp-1")], + vec![json!({ + "type": "response.failed", + "response": { + "error": { + "code": "invalid_prompt", + "message": "synthetic websocket failure" + } + } + })], + ], + response_headers: Vec::new(), + accept_delay: None, + close_after_requests: false, + }]) + .await; + + let harness = websocket_harness_with_v2(&server, true).await; + let mut session = harness.client.new_session(); + let prompt_one = prompt_with_input(vec![message_item("hello")]); + let prompt_two = prompt_with_input(vec![message_item("hello"), message_item("second")]); + + stream_until_complete(&mut session, &harness, &prompt_one).await; + + let mut second_stream = session + .stream( + &prompt_two, + &harness.model_info, + &harness.session_telemetry, + harness.effort, + harness.summary, + None, + None, + ) + .await + .expect("websocket stream failed"); + + let saw_error = tokio::time::timeout(Duration::from_secs(2), async { + while let Some(event) = second_stream.next().await { + if event.is_err() { + return true; + } + } + false + }) + .await + .expect("timed out waiting for terminal websocket error"); + + assert!(saw_error, "expected second websocket stream to error"); + + server.shutdown().await; +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn responses_websocket_v2_sets_openai_beta_header() { skip_if_no_network!(); diff --git a/codex-rs/core/tests/suite/turn_state.rs b/codex-rs/core/tests/suite/turn_state.rs index c068cfafb1..7a930af606 100644 --- a/codex-rs/core/tests/suite/turn_state.rs +++ b/codex-rs/core/tests/suite/turn_state.rs @@ -103,6 +103,7 @@ async fn websocket_turn_state_persists_within_turn_and_resets_after() -> Result< ]], response_headers: vec![(TURN_STATE_HEADER.to_string(), "ts-1".to_string())], accept_delay: None, + close_after_requests: true, }, WebSocketConnectionConfig { requests: vec![vec![ @@ -112,6 +113,7 @@ async fn websocket_turn_state_persists_within_turn_and_resets_after() -> Result< ]], response_headers: Vec::new(), accept_delay: None, + close_after_requests: true, }, WebSocketConnectionConfig { requests: vec![vec![ @@ -121,6 +123,7 @@ async fn websocket_turn_state_persists_within_turn_and_resets_after() -> Result< ]], response_headers: Vec::new(), accept_delay: None, + close_after_requests: true, }, ]) .await; From cec211cabc158532459b0c522a0cf855a891bd40 Mon Sep 17 00:00:00 2001 From: pash-openai Date: Tue, 10 Mar 2026 18:00:48 -0700 Subject: [PATCH 3/7] render local file links from target paths (#13857) Co-authored-by: Josh McKinney --- codex-rs/tui/src/chatwidget.rs | 17 +- codex-rs/tui/src/history_cell.rs | 76 +++- codex-rs/tui/src/markdown.rs | 23 +- codex-rs/tui/src/markdown_render.rs | 396 ++++++++++++++---- codex-rs/tui/src/markdown_render_tests.rs | 168 ++++++-- codex-rs/tui/src/markdown_stream.rs | 48 ++- ...s__markdown_render_file_link_snapshot.snap | 2 +- codex-rs/tui/src/streaming/controller.rs | 30 +- codex-rs/tui/src/streaming/mod.rs | 19 +- 9 files changed, 619 insertions(+), 160 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 70b524466c..cd926590ae 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1468,6 +1468,7 @@ impl ChatWidget { if self.plan_stream_controller.is_none() { self.plan_stream_controller = Some(PlanStreamController::new( self.last_rendered_width.get().map(|w| w.saturating_sub(4)), + &self.config.cwd, )); } if let Some(controller) = self.plan_stream_controller.as_mut() @@ -1506,7 +1507,7 @@ impl ChatWidget { // TODO: Replace streamed output with the final plan item text if plan streaming is // removed or if we need to reconcile mismatches between streamed and final content. } else if !plan_text.is_empty() { - self.add_to_history(history_cell::new_proposed_plan(plan_text)); + self.add_to_history(history_cell::new_proposed_plan(plan_text, &self.config.cwd)); } if should_restore_after_stream { self.pending_status_indicator_restore = true; @@ -1539,8 +1540,10 @@ impl ChatWidget { // At the end of a reasoning block, record transcript-only content. self.full_reasoning_buffer.push_str(&self.reasoning_buffer); if !self.full_reasoning_buffer.is_empty() { - let cell = - history_cell::new_reasoning_summary_block(self.full_reasoning_buffer.clone()); + let cell = history_cell::new_reasoning_summary_block( + self.full_reasoning_buffer.clone(), + &self.config.cwd, + ); self.add_boxed_history(cell); } self.reasoning_buffer.clear(); @@ -2780,6 +2783,7 @@ impl ChatWidget { } self.stream_controller = Some(StreamController::new( self.last_rendered_width.get().map(|w| w.saturating_sub(2)), + &self.config.cwd, )); } if let Some(controller) = self.stream_controller.as_mut() @@ -5156,7 +5160,12 @@ impl ChatWidget { } else { // Show explanation when there are no structured findings. let mut rendered: Vec> = vec!["".into()]; - append_markdown(&explanation, None, &mut rendered); + append_markdown( + &explanation, + None, + Some(self.config.cwd.as_path()), + &mut rendered, + ); let body_cell = AgentMessageCell::new(rendered, false); self.app_event_tx .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index e6feef2cfd..3ae4213b80 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -375,14 +375,19 @@ impl HistoryCell for UserHistoryCell { pub(crate) struct ReasoningSummaryCell { _header: String, content: String, + /// Session cwd used to render local file links inside the reasoning body. + cwd: PathBuf, transcript_only: bool, } impl ReasoningSummaryCell { - pub(crate) fn new(header: String, content: String, transcript_only: bool) -> Self { + /// Create a reasoning summary cell that will render local file links relative to the session + /// cwd active when the summary was recorded. + pub(crate) fn new(header: String, content: String, cwd: &Path, transcript_only: bool) -> Self { Self { _header: header, content, + cwd: cwd.to_path_buf(), transcript_only, } } @@ -392,6 +397,7 @@ impl ReasoningSummaryCell { append_markdown( &self.content, Some((width as usize).saturating_sub(2)), + Some(self.cwd.as_path()), &mut lines, ); let summary_style = Style::default().dim().italic(); @@ -997,11 +1003,15 @@ pub(crate) fn padded_emoji(emoji: &str) -> String { #[derive(Debug)] struct TooltipHistoryCell { tip: String, + cwd: PathBuf, } impl TooltipHistoryCell { - fn new(tip: String) -> Self { - Self { tip } + fn new(tip: String, cwd: &Path) -> Self { + Self { + tip, + cwd: cwd.to_path_buf(), + } } } @@ -1016,6 +1026,7 @@ impl HistoryCell for TooltipHistoryCell { append_markdown( &format!("**Tip:** {}", self.tip), Some(wrap_width), + Some(self.cwd.as_path()), &mut lines, ); @@ -1108,7 +1119,7 @@ pub(crate) fn new_session_info( matches!(config.service_tier, Some(ServiceTier::Fast)), ) }) - .map(TooltipHistoryCell::new) + .map(|tip| TooltipHistoryCell::new(tip, &config.cwd)) { parts.push(Box::new(tooltips)); } @@ -2046,8 +2057,12 @@ pub(crate) fn new_plan_update(update: UpdatePlanArgs) -> PlanUpdateCell { PlanUpdateCell { explanation, plan } } -pub(crate) fn new_proposed_plan(plan_markdown: String) -> ProposedPlanCell { - ProposedPlanCell { plan_markdown } +/// Create a proposed-plan cell that snapshots the session cwd for later markdown rendering. +pub(crate) fn new_proposed_plan(plan_markdown: String, cwd: &Path) -> ProposedPlanCell { + ProposedPlanCell { + plan_markdown, + cwd: cwd.to_path_buf(), + } } pub(crate) fn new_proposed_plan_stream( @@ -2063,6 +2078,8 @@ pub(crate) fn new_proposed_plan_stream( #[derive(Debug)] pub(crate) struct ProposedPlanCell { plan_markdown: String, + /// Session cwd used to keep local file-link display aligned with live streamed plan rendering. + cwd: PathBuf, } #[derive(Debug)] @@ -2081,7 +2098,12 @@ impl HistoryCell for ProposedPlanCell { let plan_style = proposed_plan_style(); let wrap_width = width.saturating_sub(4).max(1) as usize; let mut body: Vec> = Vec::new(); - append_markdown(&self.plan_markdown, Some(wrap_width), &mut body); + append_markdown( + &self.plan_markdown, + Some(wrap_width), + Some(self.cwd.as_path()), + &mut body, + ); if body.is_empty() { body.push(Line::from("(empty)".dim().italic())); } @@ -2231,7 +2253,15 @@ pub(crate) fn new_image_generation_call( PlainHistoryCell { lines } } -pub(crate) fn new_reasoning_summary_block(full_reasoning_buffer: String) -> Box { +/// Create the reasoning history cell emitted at the end of a reasoning block. +/// +/// The helper snapshots `cwd` into the returned cell so local file links render the same way they +/// did while the turn was live, even if rendering happens after other app state has advanced. +pub(crate) fn new_reasoning_summary_block( + full_reasoning_buffer: String, + cwd: &Path, +) -> Box { + let cwd = cwd.to_path_buf(); let full_reasoning_buffer = full_reasoning_buffer.trim(); if let Some(open) = full_reasoning_buffer.find("**") { let after_open = &full_reasoning_buffer[(open + 2)..]; @@ -2242,9 +2272,12 @@ pub(crate) fn new_reasoning_summary_block(full_reasoning_buffer: String) -> Box< if after_close_idx < full_reasoning_buffer.len() { let header_buffer = full_reasoning_buffer[..after_close_idx].to_string(); let summary_buffer = full_reasoning_buffer[after_close_idx..].to_string(); + // Preserve the session cwd so local file links render the same way in the + // collapsed reasoning block as they did while streaming live content. return Box::new(ReasoningSummaryCell::new( header_buffer, summary_buffer, + &cwd, false, )); } @@ -2253,6 +2286,7 @@ pub(crate) fn new_reasoning_summary_block(full_reasoning_buffer: String) -> Box< Box::new(ReasoningSummaryCell::new( "".to_string(), full_reasoning_buffer.to_string(), + &cwd, true, )) } @@ -2468,6 +2502,12 @@ mod tests { .expect("config") } + fn test_cwd() -> PathBuf { + // These tests only need a stable absolute cwd; using temp_dir() avoids baking Unix- or + // Windows-specific root semantics into the fixtures. + std::env::temp_dir() + } + fn render_lines(lines: &[Line<'static>]) -> Vec { lines .iter() @@ -3999,6 +4039,7 @@ mod tests { fn reasoning_summary_block() { let cell = new_reasoning_summary_block( "**High level reasoning**\n\nDetailed reasoning goes here.".to_string(), + &test_cwd(), ); let rendered_display = render_lines(&cell.display_lines(80)); @@ -4014,6 +4055,7 @@ mod tests { let cell: Box = Box::new(ReasoningSummaryCell::new( "High level reasoning".to_string(), summary.to_string(), + &test_cwd(), false, )); let width: u16 = 24; @@ -4054,7 +4096,8 @@ mod tests { #[test] fn reasoning_summary_block_returns_reasoning_cell_when_feature_disabled() { - let cell = new_reasoning_summary_block("Detailed reasoning goes here.".to_string()); + let cell = + new_reasoning_summary_block("Detailed reasoning goes here.".to_string(), &test_cwd()); let rendered = render_transcript(cell.as_ref()); assert_eq!(rendered, vec!["• Detailed reasoning goes here."]); @@ -4067,6 +4110,7 @@ mod tests { config.model_supports_reasoning_summaries = Some(true); let cell = new_reasoning_summary_block( "**High level reasoning**\n\nDetailed reasoning goes here.".to_string(), + &test_cwd(), ); let rendered_display = render_lines(&cell.display_lines(80)); @@ -4075,8 +4119,10 @@ mod tests { #[test] fn reasoning_summary_block_falls_back_when_header_is_missing() { - let cell = - new_reasoning_summary_block("**High level reasoning without closing".to_string()); + let cell = new_reasoning_summary_block( + "**High level reasoning without closing".to_string(), + &test_cwd(), + ); let rendered = render_transcript(cell.as_ref()); assert_eq!(rendered, vec!["• **High level reasoning without closing"]); @@ -4084,14 +4130,17 @@ mod tests { #[test] fn reasoning_summary_block_falls_back_when_summary_is_missing() { - let cell = - new_reasoning_summary_block("**High level reasoning without closing**".to_string()); + let cell = new_reasoning_summary_block( + "**High level reasoning without closing**".to_string(), + &test_cwd(), + ); let rendered = render_transcript(cell.as_ref()); assert_eq!(rendered, vec!["• High level reasoning without closing"]); let cell = new_reasoning_summary_block( "**High level reasoning without closing**\n\n ".to_string(), + &test_cwd(), ); let rendered = render_transcript(cell.as_ref()); @@ -4102,6 +4151,7 @@ mod tests { fn reasoning_summary_block_splits_header_and_summary_when_present() { let cell = new_reasoning_summary_block( "**High level plan**\n\nWe should fix the bug next.".to_string(), + &test_cwd(), ); let rendered_display = render_lines(&cell.display_lines(80)); diff --git a/codex-rs/tui/src/markdown.rs b/codex-rs/tui/src/markdown.rs index 2ea307066b..228febb854 100644 --- a/codex-rs/tui/src/markdown.rs +++ b/codex-rs/tui/src/markdown.rs @@ -1,10 +1,21 @@ use ratatui::text::Line; +use std::path::Path; + +/// Render markdown into `lines` while resolving local file-link display relative to `cwd`. +/// +/// Callers that already know the session working directory should pass it here so streamed and +/// non-streamed rendering show the same relative path text even if the process cwd differs. pub(crate) fn append_markdown( markdown_source: &str, width: Option, + cwd: Option<&Path>, lines: &mut Vec>, ) { - let rendered = crate::markdown_render::render_markdown_text_with_width(markdown_source, width); + let rendered = crate::markdown_render::render_markdown_text_with_width_and_cwd( + markdown_source, + width, + cwd, + ); crate::render::line_utils::push_owned_lines(&rendered.lines, lines); } @@ -30,7 +41,7 @@ mod tests { fn citations_render_as_plain_text() { let src = "Before 【F:/x.rs†L1】\nAfter 【F:/x.rs†L3】\n"; let mut out = Vec::new(); - append_markdown(src, None, &mut out); + append_markdown(src, None, None, &mut out); let rendered = lines_to_strings(&out); assert_eq!( rendered, @@ -46,7 +57,7 @@ mod tests { // Basic sanity: indented code with surrounding blank lines should produce the indented line. let src = "Before\n\n code 1\n\nAfter\n"; let mut out = Vec::new(); - append_markdown(src, None, &mut out); + append_markdown(src, None, None, &mut out); let lines = lines_to_strings(&out); assert_eq!(lines, vec!["Before", "", " code 1", "", "After"]); } @@ -55,7 +66,7 @@ mod tests { fn append_markdown_preserves_full_text_line() { let src = "Hi! How can I help with codex-rs today? Want me to explore the repo, run tests, or work on a specific change?\n"; let mut out = Vec::new(); - append_markdown(src, None, &mut out); + append_markdown(src, None, None, &mut out); assert_eq!( out.len(), 1, @@ -76,7 +87,7 @@ mod tests { #[test] fn append_markdown_matches_tui_markdown_for_ordered_item() { let mut out = Vec::new(); - append_markdown("1. Tight item\n", None, &mut out); + append_markdown("1. Tight item\n", None, None, &mut out); let lines = lines_to_strings(&out); assert_eq!(lines, vec!["1. Tight item".to_string()]); } @@ -85,7 +96,7 @@ mod tests { fn append_markdown_keeps_ordered_list_line_unsplit_in_context() { let src = "Loose vs. tight list items:\n1. Tight item\n"; let mut out = Vec::new(); - append_markdown(src, None, &mut out); + append_markdown(src, None, None, &mut out); let lines = lines_to_strings(&out); diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs index 2bbe19b6f7..1b09b84c37 100644 --- a/codex-rs/tui/src/markdown_render.rs +++ b/codex-rs/tui/src/markdown_render.rs @@ -1,8 +1,16 @@ +//! Markdown rendering for the TUI transcript. +//! +//! This renderer intentionally treats local file links differently from normal web links. For +//! local paths, the displayed text comes from the destination, not the markdown label, so +//! transcripts show the real file target (including normalized location suffixes) and can shorten +//! absolute paths relative to a known working directory. + use crate::render::highlight::highlight_code_to_lines; use crate::render::line_utils::line_to_static; use crate::wrapping::RtOptions; use crate::wrapping::adaptive_wrap_line; use codex_utils_string::normalize_markdown_hash_location_suffix; +use dirs::home_dir; use pulldown_cmark::CodeBlockKind; use pulldown_cmark::CowStr; use pulldown_cmark::Event; @@ -16,7 +24,10 @@ use ratatui::text::Line; use ratatui::text::Span; use ratatui::text::Text; use regex_lite::Regex; +use std::path::Path; +use std::path::PathBuf; use std::sync::LazyLock; +use url::Url; struct MarkdownStyles { h1: Style, @@ -79,11 +90,26 @@ pub fn render_markdown_text(input: &str) -> Text<'static> { render_markdown_text_with_width(input, None) } +/// Render markdown using the current process working directory for local file-link display. pub(crate) fn render_markdown_text_with_width(input: &str, width: Option) -> Text<'static> { + let cwd = std::env::current_dir().ok(); + render_markdown_text_with_width_and_cwd(input, width, cwd.as_deref()) +} + +/// Render markdown with an explicit working directory for local file links. +/// +/// The `cwd` parameter controls how absolute local targets are shortened before display. Passing +/// the session cwd keeps full renders, history cells, and streamed deltas visually aligned even +/// when rendering happens away from the process cwd. +pub(crate) fn render_markdown_text_with_width_and_cwd( + input: &str, + width: Option, + cwd: Option<&Path>, +) -> Text<'static> { let mut options = Options::empty(); options.insert(Options::ENABLE_STRIKETHROUGH); let parser = Parser::new_ext(input, options); - let mut w = Writer::new(parser, width); + let mut w = Writer::new(parser, width, cwd); w.run(); w.text } @@ -92,9 +118,11 @@ pub(crate) fn render_markdown_text_with_width(input: &str, width: Option) struct LinkState { destination: String, show_destination: bool, - hidden_location_suffix: Option, - label_start_span_idx: usize, - label_styled: bool, + /// Pre-rendered display text for local file links. + /// + /// When this is present, the markdown label is intentionally suppressed so the rendered + /// transcript always reflects the real target path. + local_target_display: Option, } fn should_render_link_destination(dest_url: &str) -> bool { @@ -116,20 +144,6 @@ static HASH_LOCATION_SUFFIX_RE: LazyLock = Err(error) => panic!("invalid hash location regex: {error}"), }); -fn is_local_path_like_link(dest_url: &str) -> bool { - dest_url.starts_with("file://") - || dest_url.starts_with('/') - || dest_url.starts_with("~/") - || dest_url.starts_with("./") - || dest_url.starts_with("../") - || dest_url.starts_with("\\\\") - || matches!( - dest_url.as_bytes(), - [drive, b':', separator, ..] - if drive.is_ascii_alphabetic() && matches!(separator, b'/' | b'\\') - ) -} - struct Writer<'a, I> where I: Iterator>, @@ -148,6 +162,9 @@ where code_block_lang: Option, code_block_buffer: String, wrap_width: Option, + cwd: Option, + line_ends_with_local_link_target: bool, + pending_local_link_soft_break: bool, current_line_content: Option>, current_initial_indent: Vec>, current_subsequent_indent: Vec>, @@ -159,7 +176,7 @@ impl<'a, I> Writer<'a, I> where I: Iterator>, { - fn new(iter: I, wrap_width: Option) -> Self { + fn new(iter: I, wrap_width: Option, cwd: Option<&Path>) -> Self { Self { iter, text: Text::default(), @@ -175,6 +192,9 @@ where code_block_lang: None, code_block_buffer: String::new(), wrap_width, + cwd: cwd.map(Path::to_path_buf), + line_ends_with_local_link_target: false, + pending_local_link_soft_break: false, current_line_content: None, current_initial_indent: Vec::new(), current_subsequent_indent: Vec::new(), @@ -191,6 +211,7 @@ where } fn handle_event(&mut self, event: Event<'a>) { + self.prepare_for_event(&event); match event { Event::Start(tag) => self.start_tag(tag), Event::End(tag) => self.end_tag(tag), @@ -213,6 +234,23 @@ where } } + fn prepare_for_event(&mut self, event: &Event<'a>) { + if !self.pending_local_link_soft_break { + return; + } + + // Local file links render from the destination at `TagEnd::Link`, so a Markdown soft break + // immediately before a descriptive `: ...` should stay inline instead of splitting the + // list item across two lines. + if matches!(event, Event::Text(text) if text.trim_start().starts_with(':')) { + self.pending_local_link_soft_break = false; + return; + } + + self.pending_local_link_soft_break = false; + self.push_line(Line::default()); + } + fn start_tag(&mut self, tag: Tag<'a>) { match tag { Tag::Paragraph => self.start_paragraph(), @@ -324,6 +362,10 @@ where } fn text(&mut self, text: CowStr<'a>) { + if self.suppressing_local_link_label() { + return; + } + self.line_ends_with_local_link_target = false; if self.pending_marker_line { self.push_line(Line::default()); } @@ -373,6 +415,10 @@ where } fn code(&mut self, code: CowStr<'a>) { + if self.suppressing_local_link_label() { + return; + } + self.line_ends_with_local_link_target = false; if self.pending_marker_line { self.push_line(Line::default()); self.pending_marker_line = false; @@ -382,6 +428,10 @@ where } fn html(&mut self, html: CowStr<'a>, inline: bool) { + if self.suppressing_local_link_label() { + return; + } + self.line_ends_with_local_link_target = false; self.pending_marker_line = false; for (i, line) in html.lines().enumerate() { if self.needs_newline { @@ -398,10 +448,23 @@ where } fn hard_break(&mut self) { + if self.suppressing_local_link_label() { + return; + } + self.line_ends_with_local_link_target = false; self.push_line(Line::default()); } fn soft_break(&mut self) { + if self.suppressing_local_link_label() { + return; + } + if self.line_ends_with_local_link_target { + self.pending_local_link_soft_break = true; + self.line_ends_with_local_link_target = false; + return; + } + self.line_ends_with_local_link_target = false; self.push_line(Line::default()); } @@ -513,36 +576,13 @@ where fn push_link(&mut self, dest_url: String) { let show_destination = should_render_link_destination(&dest_url); - let label_styled = !show_destination; - let label_start_span_idx = self - .current_line_content - .as_ref() - .map(|line| line.spans.len()) - .unwrap_or(0); - if label_styled { - self.push_inline_style(self.styles.code); - } self.link = Some(LinkState { show_destination, - hidden_location_suffix: if is_local_path_like_link(&dest_url) { - dest_url - .rsplit_once('#') - .and_then(|(_, fragment)| { - HASH_LOCATION_SUFFIX_RE - .is_match(fragment) - .then(|| format!("#{fragment}")) - }) - .and_then(|suffix| normalize_markdown_hash_location_suffix(&suffix)) - .or_else(|| { - COLON_LOCATION_SUFFIX_RE - .find(&dest_url) - .map(|m| m.as_str().to_string()) - }) + local_target_display: if is_local_path_like_link(&dest_url) { + render_local_link_target(&dest_url, self.cwd.as_deref()) } else { None }, - label_start_span_idx, - label_styled, destination: dest_url, }); } @@ -550,43 +590,34 @@ where fn pop_link(&mut self) { if let Some(link) = self.link.take() { if link.show_destination { - if link.label_styled { - self.pop_inline_style(); - } self.push_span(" (".into()); self.push_span(Span::styled(link.destination, self.styles.link)); self.push_span(")".into()); - } else if let Some(location_suffix) = link.hidden_location_suffix.as_deref() { - let label_text = self - .current_line_content - .as_ref() - .and_then(|line| { - line.spans.get(link.label_start_span_idx..).map(|spans| { - spans - .iter() - .map(|span| span.content.as_ref()) - .collect::() - }) - }) - .unwrap_or_default(); - if label_text - .rsplit_once('#') - .is_some_and(|(_, fragment)| HASH_LOCATION_SUFFIX_RE.is_match(fragment)) - || COLON_LOCATION_SUFFIX_RE.find(&label_text).is_some() - { - // The label already carries a location suffix; don't duplicate it. - } else { - self.push_span(Span::styled(location_suffix.to_string(), self.styles.code)); + } else if let Some(local_target_display) = link.local_target_display { + if self.pending_marker_line { + self.push_line(Line::default()); } - if link.label_styled { - self.pop_inline_style(); - } - } else if link.label_styled { - self.pop_inline_style(); + // Local file links are rendered as code-like path text so the transcript shows the + // resolved target instead of arbitrary caller-provided label text. + let style = self + .inline_styles + .last() + .copied() + .unwrap_or_default() + .patch(self.styles.code); + self.push_span(Span::styled(local_target_display, style)); + self.line_ends_with_local_link_target = true; } } } + fn suppressing_local_link_label(&self) -> bool { + self.link + .as_ref() + .and_then(|link| link.local_target_display.as_ref()) + .is_some() + } + fn flush_current_line(&mut self) { if let Some(line) = self.current_line_content.take() { let style = self.current_line_style; @@ -610,6 +641,7 @@ where self.current_initial_indent.clear(); self.current_subsequent_indent.clear(); self.current_line_in_code_block = false; + self.line_ends_with_local_link_target = false; } } @@ -631,6 +663,7 @@ where self.current_line_style = style; self.current_line_content = Some(line); self.current_line_in_code_block = self.in_code_block; + self.line_ends_with_local_link_target = false; self.pending_marker_line = false; } @@ -687,6 +720,223 @@ where } } +fn is_local_path_like_link(dest_url: &str) -> bool { + dest_url.starts_with("file://") + || dest_url.starts_with('/') + || dest_url.starts_with("~/") + || dest_url.starts_with("./") + || dest_url.starts_with("../") + || dest_url.starts_with("\\\\") + || matches!( + dest_url.as_bytes(), + [drive, b':', separator, ..] + if drive.is_ascii_alphabetic() && matches!(separator, b'/' | b'\\') + ) +} + +/// Parse a local link target into normalized path text plus an optional location suffix. +/// +/// This accepts the path shapes Codex emits today: `file://` URLs, absolute and relative paths, +/// `~/...`, Windows paths, and `#L..C..` or `:line:col` suffixes. +fn render_local_link_target(dest_url: &str, cwd: Option<&Path>) -> Option { + let (path_text, location_suffix) = parse_local_link_target(dest_url)?; + let mut rendered = display_local_link_path(&path_text, cwd); + if let Some(location_suffix) = location_suffix { + rendered.push_str(&location_suffix); + } + Some(rendered) +} + +/// Split a local-link destination into `(normalized_path_text, location_suffix)`. +/// +/// The returned path text never includes a trailing `#L..` or `:line[:col]` suffix. Path +/// normalization expands `~/...` when possible and rewrites path separators into display-stable +/// forward slashes. The suffix, when present, is returned separately in normalized markdown form. +/// +/// Returns `None` only when the destination looks like a `file://` URL but cannot be parsed into a +/// local path. Plain path-like inputs always return `Some(...)` even if they are relative. +fn parse_local_link_target(dest_url: &str) -> Option<(String, Option)> { + if dest_url.starts_with("file://") { + let url = Url::parse(dest_url).ok()?; + let path_text = file_url_to_local_path_text(&url)?; + let location_suffix = url + .fragment() + .and_then(normalize_hash_location_suffix_fragment); + return Some((path_text, location_suffix)); + } + + let mut path_text = dest_url; + let mut location_suffix = None; + // Prefer `#L..` style fragments when both forms are present so URLs like `path#L10` do not + // get misparsed as a plain path ending in `:10`. + if let Some((candidate_path, fragment)) = dest_url.rsplit_once('#') + && let Some(normalized) = normalize_hash_location_suffix_fragment(fragment) + { + path_text = candidate_path; + location_suffix = Some(normalized); + } + if location_suffix.is_none() + && let Some(suffix) = extract_colon_location_suffix(path_text) + { + let path_len = path_text.len().saturating_sub(suffix.len()); + path_text = &path_text[..path_len]; + location_suffix = Some(suffix); + } + + Some((expand_local_link_path(path_text), location_suffix)) +} + +/// Normalize a hash fragment like `L12` or `L12C3-L14C9` into the display suffix we render. +/// +/// Returns `None` for fragments that are not location references. This deliberately ignores other +/// `#...` fragments so non-location hashes stay part of the path text. +fn normalize_hash_location_suffix_fragment(fragment: &str) -> Option { + HASH_LOCATION_SUFFIX_RE + .is_match(fragment) + .then(|| format!("#{fragment}")) + .and_then(|suffix| normalize_markdown_hash_location_suffix(&suffix)) +} + +/// Extract a trailing `:line`, `:line:col`, or range suffix from a plain path-like string. +/// +/// The suffix must occur at the end of the input; embedded colons elsewhere in the path are left +/// alone. This is what keeps Windows drive letters like `C:/...` from being misread as locations. +fn extract_colon_location_suffix(path_text: &str) -> Option { + COLON_LOCATION_SUFFIX_RE + .find(path_text) + .filter(|matched| matched.end() == path_text.len()) + .map(|matched| matched.as_str().to_string()) +} + +/// Expand home-relative paths and normalize separators for display. +/// +/// If `~/...` cannot be expanded because the home directory is unavailable, the original text still +/// goes through separator normalization and is returned as-is otherwise. +fn expand_local_link_path(path_text: &str) -> String { + // Expand `~/...` eagerly so home-relative links can participate in the same normalization and + // cwd-relative shortening path as absolute links. + if let Some(rest) = path_text.strip_prefix("~/") + && let Some(home) = home_dir() + { + return normalize_local_link_path_text(&home.join(rest).to_string_lossy()); + } + + normalize_local_link_path_text(path_text) +} + +/// Convert a `file://` URL into the normalized local-path text used for transcript rendering. +/// +/// This prefers `Url::to_file_path()` for standard file URLs. When that rejects Windows-oriented +/// encodings, we reconstruct a display path from the host/path parts so UNC paths and drive-letter +/// URLs still render sensibly. +fn file_url_to_local_path_text(url: &Url) -> Option { + if let Ok(path) = url.to_file_path() { + return Some(normalize_local_link_path_text(&path.to_string_lossy())); + } + + // Fall back to string reconstruction for cases `to_file_path()` rejects, especially UNC-style + // hosts and Windows drive paths encoded in URL form. + let mut path_text = url.path().to_string(); + if let Some(host) = url.host_str() + && !host.is_empty() + && host != "localhost" + { + path_text = format!("//{host}{path_text}"); + } else if matches!( + path_text.as_bytes(), + [b'/', drive, b':', b'/', ..] if drive.is_ascii_alphabetic() + ) { + path_text.remove(0); + } + + Some(normalize_local_link_path_text(&path_text)) +} + +/// Normalize local-path text into the transcript display form. +/// +/// Display normalization is intentionally lexical: it does not touch the filesystem, resolve +/// symlinks, or collapse `.` / `..`. It only converts separators to forward slashes and rewrites +/// UNC-style `\\\\server\\share` inputs into `//server/share` so later prefix checks operate on a +/// stable representation. +fn normalize_local_link_path_text(path_text: &str) -> String { + // Render all local link paths with forward slashes so display and prefix stripping are stable + // across mixed Windows and Unix-style inputs. + if let Some(rest) = path_text.strip_prefix("\\\\") { + format!("//{}", rest.replace('\\', "/").trim_start_matches('/')) + } else { + path_text.replace('\\', "/") + } +} + +fn is_absolute_local_link_path(path_text: &str) -> bool { + path_text.starts_with('/') + || path_text.starts_with("//") + || matches!( + path_text.as_bytes(), + [drive, b':', b'/', ..] if drive.is_ascii_alphabetic() + ) +} + +/// Remove trailing separators from a local path without destroying root semantics. +/// +/// Roots like `/`, `//`, and `C:/` stay intact so callers can still distinguish "the root itself" +/// from "a path under the root". +fn trim_trailing_local_path_separator(path_text: &str) -> &str { + if path_text == "/" || path_text == "//" { + return path_text; + } + if matches!(path_text.as_bytes(), [drive, b':', b'/'] if drive.is_ascii_alphabetic()) { + return path_text; + } + path_text.trim_end_matches('/') +} + +/// Strip `cwd_text` from the start of `path_text` when `path_text` is strictly underneath it. +/// +/// Returns the relative remainder without a leading slash. If the path equals the cwd exactly, this +/// returns `None` so callers can keep rendering the full path instead of collapsing it to an empty +/// string. +fn strip_local_path_prefix<'a>(path_text: &'a str, cwd_text: &str) -> Option<&'a str> { + let path_text = trim_trailing_local_path_separator(path_text); + let cwd_text = trim_trailing_local_path_separator(cwd_text); + if path_text == cwd_text { + return None; + } + + // Treat filesystem roots specially so `/tmp/x` under `/` becomes `tmp/x` instead of being + // left unchanged by the generic prefix-stripping branch. + if cwd_text == "/" || cwd_text == "//" { + return path_text.strip_prefix('/'); + } + + path_text + .strip_prefix(cwd_text) + .and_then(|rest| rest.strip_prefix('/')) +} + +/// Choose the visible path text for a local link after normalization. +/// +/// Relative paths stay relative. Absolute paths are shortened against `cwd` only when they are +/// lexically underneath it; otherwise the absolute path is preserved. This is display logic only, +/// not filesystem canonicalization. +fn display_local_link_path(path_text: &str, cwd: Option<&Path>) -> String { + let path_text = normalize_local_link_path_text(path_text); + if !is_absolute_local_link_path(&path_text) { + return path_text; + } + + if let Some(cwd) = cwd { + // Only shorten absolute paths that are under the provided session cwd; otherwise preserve + // the original absolute target for clarity. + let cwd_text = normalize_local_link_path_text(&cwd.to_string_lossy()); + if let Some(stripped) = strip_local_path_prefix(&path_text, &cwd_text) { + return stripped.to_string(); + } + } + + path_text +} + #[cfg(test)] mod markdown_render_tests { include!("markdown_render_tests.rs"); diff --git a/codex-rs/tui/src/markdown_render_tests.rs b/codex-rs/tui/src/markdown_render_tests.rs index 9981246093..376b80f61d 100644 --- a/codex-rs/tui/src/markdown_render_tests.rs +++ b/codex-rs/tui/src/markdown_render_tests.rs @@ -3,12 +3,18 @@ use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; use ratatui::text::Text; +use std::path::Path; use crate::markdown_render::COLON_LOCATION_SUFFIX_RE; use crate::markdown_render::HASH_LOCATION_SUFFIX_RE; use crate::markdown_render::render_markdown_text; +use crate::markdown_render::render_markdown_text_with_width_and_cwd; use insta::assert_snapshot; +fn render_markdown_text_for_cwd(input: &str, cwd: &Path) -> Text<'static> { + render_markdown_text_with_width_and_cwd(input, None, Some(cwd)) +} + #[test] fn empty() { assert_eq!(render_markdown_text(""), Text::default()); @@ -661,8 +667,9 @@ fn load_location_suffix_regexes() { #[test] fn file_link_hides_destination() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "[codex-rs/tui/src/markdown_render.rs](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs)", + Path::new("/Users/example/code/codex"), ); let expected = Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs".cyan()])); assert_eq!(text, expected); @@ -670,97 +677,101 @@ fn file_link_hides_destination() { #[test] fn file_link_appends_line_number_when_label_lacks_it() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "[markdown_render.rs](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs:74)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter([ - "markdown_render.rs".cyan(), - ":74".cyan(), - ])); + let expected = Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74".cyan()])); assert_eq!(text, expected); } #[test] -fn file_link_uses_label_for_line_number() { - let text = render_markdown_text( - "[markdown_render.rs:74](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs:74)", +fn file_link_keeps_absolute_paths_outside_cwd() { + let text = render_markdown_text_for_cwd( + "[README.md:74](/Users/example/code/codex/README.md:74)", + Path::new("/Users/example/code/codex/codex-rs/tui"), ); - let expected = Text::from(Line::from_iter(["markdown_render.rs:74".cyan()])); + let expected = Text::from(Line::from_iter(["/Users/example/code/codex/README.md:74".cyan()])); assert_eq!(text, expected); } #[test] fn file_link_appends_hash_anchor_when_label_lacks_it() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "[markdown_render.rs](file:///Users/example/code/codex/codex-rs/tui/src/markdown_render.rs#L74C3)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter([ - "markdown_render.rs".cyan(), - ":74:3".cyan(), - ])); + let expected = + Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74:3".cyan()])); assert_eq!(text, expected); } #[test] -fn file_link_uses_label_for_hash_anchor() { - let text = render_markdown_text( +fn file_link_uses_target_path_for_hash_anchor() { + let text = render_markdown_text_for_cwd( "[markdown_render.rs#L74C3](file:///Users/example/code/codex/codex-rs/tui/src/markdown_render.rs#L74C3)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter(["markdown_render.rs#L74C3".cyan()])); + let expected = + Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74:3".cyan()])); assert_eq!(text, expected); } #[test] fn file_link_appends_range_when_label_lacks_it() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "[markdown_render.rs](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs:74:3-76:9)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter([ - "markdown_render.rs".cyan(), - ":74:3-76:9".cyan(), - ])); + let expected = + Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74:3-76:9".cyan()])); assert_eq!(text, expected); } #[test] -fn file_link_uses_label_for_range() { - let text = render_markdown_text( +fn file_link_uses_target_path_for_range() { + let text = render_markdown_text_for_cwd( "[markdown_render.rs:74:3-76:9](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs:74:3-76:9)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter(["markdown_render.rs:74:3-76:9".cyan()])); + let expected = + Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74:3-76:9".cyan()])); assert_eq!(text, expected); } #[test] fn file_link_appends_hash_range_when_label_lacks_it() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "[markdown_render.rs](file:///Users/example/code/codex/codex-rs/tui/src/markdown_render.rs#L74C3-L76C9)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter([ - "markdown_render.rs".cyan(), - ":74:3-76:9".cyan(), - ])); + let expected = + Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74:3-76:9".cyan()])); assert_eq!(text, expected); } #[test] fn multiline_file_link_label_after_styled_prefix_does_not_panic() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "**bold** plain [foo\nbar](file:///Users/example/code/codex/codex-rs/tui/src/markdown_render.rs#L74C3)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from_iter([ - Line::from_iter(["bold".bold(), " plain ".into(), "foo".cyan()]), - Line::from_iter(["bar".cyan(), ":74:3".cyan()]), - ]); + let expected = Text::from(Line::from_iter([ + "bold".bold(), + " plain ".into(), + "codex-rs/tui/src/markdown_render.rs:74:3".cyan(), + ])); assert_eq!(text, expected); } #[test] -fn file_link_uses_label_for_hash_range() { - let text = render_markdown_text( +fn file_link_uses_target_path_for_hash_range() { + let text = render_markdown_text_for_cwd( "[markdown_render.rs#L74C3-L76C9](file:///Users/example/code/codex/codex-rs/tui/src/markdown_render.rs#L74C3-L76C9)", + Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter(["markdown_render.rs#L74C3-L76C9".cyan()])); + let expected = + Text::from(Line::from_iter(["codex-rs/tui/src/markdown_render.rs:74:3-76:9".cyan()])); assert_eq!(text, expected); } @@ -778,8 +789,9 @@ fn url_link_shows_destination() { #[test] fn markdown_render_file_link_snapshot() { - let text = render_markdown_text( + let text = render_markdown_text_for_cwd( "See [markdown_render.rs:74](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs:74).", + Path::new("/Users/example/code/codex"), ); let rendered = text .lines @@ -796,6 +808,82 @@ fn markdown_render_file_link_snapshot() { assert_snapshot!(rendered); } +#[test] +fn unordered_list_local_file_link_stays_inline_with_following_text() { + let text = render_markdown_text_with_width_and_cwd( + "- [binary](/Users/example/code/codex/codex-rs/README.md:93): core is the agent/business logic, tui is the terminal UI, exec is the headless automation surface, and cli is the top-level multitool binary.", + Some(72), + Some(Path::new("/Users/example/code/codex")), + ); + let rendered = text + .lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>(); + assert_eq!( + rendered, + vec![ + "- codex-rs/README.md:93: core is the agent/business logic, tui is the", + " terminal UI, exec is the headless automation surface, and cli is the", + " top-level multitool binary.", + ] + ); +} + +#[test] +fn unordered_list_local_file_link_soft_break_before_colon_stays_inline() { + let text = render_markdown_text_with_width_and_cwd( + "- [binary](/Users/example/code/codex/codex-rs/README.md:93)\n : core is the agent/business logic.", + Some(72), + Some(Path::new("/Users/example/code/codex")), + ); + let rendered = text + .lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>(); + assert_eq!( + rendered, + vec!["- codex-rs/README.md:93: core is the agent/business logic.",] + ); +} + +#[test] +fn consecutive_unordered_list_local_file_links_do_not_detach_paths() { + let text = render_markdown_text_with_width_and_cwd( + "- [binary](/Users/example/code/codex/codex-rs/README.md:93)\n : cli is the top-level multitool binary.\n- [expectations](/Users/example/code/codex/codex-rs/core/README.md:1)\n : codex-core owns the real runtime behavior.", + Some(72), + Some(Path::new("/Users/example/code/codex")), + ); + let rendered = text + .lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>(); + assert_eq!( + rendered, + vec![ + "- codex-rs/README.md:93: cli is the top-level multitool binary.", + "- codex-rs/core/README.md:1: codex-core owns the real runtime behavior.", + ] + ); +} + #[test] fn code_block_known_lang_has_syntax_colors() { let text = render_markdown_text("```rust\nfn main() {}\n```\n"); diff --git a/codex-rs/tui/src/markdown_stream.rs b/codex-rs/tui/src/markdown_stream.rs index 6ac457eee2..a18457d6bc 100644 --- a/codex-rs/tui/src/markdown_stream.rs +++ b/codex-rs/tui/src/markdown_stream.rs @@ -1,4 +1,6 @@ use ratatui::text::Line; +use std::path::Path; +use std::path::PathBuf; use crate::markdown; @@ -8,14 +10,22 @@ pub(crate) struct MarkdownStreamCollector { buffer: String, committed_line_count: usize, width: Option, + cwd: PathBuf, } impl MarkdownStreamCollector { - pub fn new(width: Option) -> Self { + /// Create a collector that renders markdown using `cwd` for local file-link display. + /// + /// The collector snapshots `cwd` into owned state because stream commits can happen long after + /// construction. The same `cwd` should be reused for the entire stream lifecycle; mixing + /// different working directories within one stream would make the same link render with + /// different path prefixes across incremental commits. + pub fn new(width: Option, cwd: &Path) -> Self { Self { buffer: String::new(), committed_line_count: 0, width, + cwd: cwd.to_path_buf(), } } @@ -41,7 +51,7 @@ impl MarkdownStreamCollector { return Vec::new(); }; let mut rendered: Vec> = Vec::new(); - markdown::append_markdown(&source, self.width, &mut rendered); + markdown::append_markdown(&source, self.width, Some(self.cwd.as_path()), &mut rendered); let mut complete_line_count = rendered.len(); if complete_line_count > 0 && crate::render::line_utils::is_blank_line_spaces_only( @@ -82,7 +92,7 @@ impl MarkdownStreamCollector { tracing::trace!("markdown finalize (raw source):\n---\n{source}\n---"); let mut rendered: Vec> = Vec::new(); - markdown::append_markdown(&source, self.width, &mut rendered); + markdown::append_markdown(&source, self.width, Some(self.cwd.as_path()), &mut rendered); let out = if self.committed_line_count >= rendered.len() { Vec::new() @@ -96,12 +106,19 @@ impl MarkdownStreamCollector { } } +#[cfg(test)] +fn test_cwd() -> PathBuf { + // These tests only need a stable absolute cwd; using temp_dir() avoids baking Unix- or + // Windows-specific root semantics into the fixtures. + std::env::temp_dir() +} + #[cfg(test)] pub(crate) fn simulate_stream_markdown_for_tests( deltas: &[&str], finalize: bool, ) -> Vec> { - let mut collector = MarkdownStreamCollector::new(None); + let mut collector = MarkdownStreamCollector::new(None, &test_cwd()); let mut out = Vec::new(); for d in deltas { collector.push_delta(d); @@ -122,7 +139,7 @@ mod tests { #[tokio::test] async fn no_commit_until_newline() { - let mut c = super::MarkdownStreamCollector::new(None); + let mut c = super::MarkdownStreamCollector::new(None, &super::test_cwd()); c.push_delta("Hello, world"); let out = c.commit_complete_lines(); assert!(out.is_empty(), "should not commit without newline"); @@ -133,7 +150,7 @@ mod tests { #[tokio::test] async fn finalize_commits_partial_line() { - let mut c = super::MarkdownStreamCollector::new(None); + let mut c = super::MarkdownStreamCollector::new(None, &super::test_cwd()); c.push_delta("Line without newline"); let out = c.finalize_and_drain(); assert_eq!(out.len(), 1); @@ -253,7 +270,7 @@ mod tests { async fn heading_starts_on_new_line_when_following_paragraph() { // Stream a paragraph line, then a heading on the next line. // Expect two distinct rendered lines: "Hello." and "Heading". - let mut c = super::MarkdownStreamCollector::new(None); + let mut c = super::MarkdownStreamCollector::new(None, &super::test_cwd()); c.push_delta("Hello.\n"); let out1 = c.commit_complete_lines(); let s1: Vec = out1 @@ -309,7 +326,7 @@ mod tests { // Paragraph without trailing newline, then a chunk that starts with the newline // and the heading text, then a final newline. The collector should first commit // only the paragraph line, and later commit the heading as its own line. - let mut c = super::MarkdownStreamCollector::new(None); + let mut c = super::MarkdownStreamCollector::new(None, &super::test_cwd()); c.push_delta("Sounds good!"); // No commit yet assert!(c.commit_complete_lines().is_empty()); @@ -354,7 +371,8 @@ mod tests { // Sanity check raw markdown rendering for a simple line does not produce spurious extras. let mut rendered: Vec> = Vec::new(); - crate::markdown::append_markdown("Hello.\n", None, &mut rendered); + let test_cwd = super::test_cwd(); + crate::markdown::append_markdown("Hello.\n", None, Some(test_cwd.as_path()), &mut rendered); let rendered_strings: Vec = rendered .iter() .map(|l| { @@ -414,7 +432,8 @@ mod tests { let streamed_str = lines_to_plain_strings(&streamed); let mut rendered_all: Vec> = Vec::new(); - crate::markdown::append_markdown(input, None, &mut rendered_all); + let test_cwd = super::test_cwd(); + crate::markdown::append_markdown(input, None, Some(test_cwd.as_path()), &mut rendered_all); let rendered_all_str = lines_to_plain_strings(&rendered_all); assert_eq!( @@ -520,7 +539,8 @@ mod tests { let full: String = deltas.iter().copied().collect(); let mut rendered_all: Vec> = Vec::new(); - crate::markdown::append_markdown(&full, None, &mut rendered_all); + let test_cwd = super::test_cwd(); + crate::markdown::append_markdown(&full, None, Some(test_cwd.as_path()), &mut rendered_all); let rendered_all_strs = lines_to_plain_strings(&rendered_all); assert_eq!( @@ -608,7 +628,8 @@ mod tests { // Compute a full render for diagnostics only. let full: String = deltas.iter().copied().collect(); let mut rendered_all: Vec> = Vec::new(); - crate::markdown::append_markdown(&full, None, &mut rendered_all); + let test_cwd = super::test_cwd(); + crate::markdown::append_markdown(&full, None, Some(test_cwd.as_path()), &mut rendered_all); // Also assert exact expected plain strings for clarity. let expected = vec![ @@ -635,7 +656,8 @@ mod tests { let streamed_strs = lines_to_plain_strings(&streamed); let full: String = deltas.iter().copied().collect(); let mut rendered: Vec> = Vec::new(); - crate::markdown::append_markdown(&full, None, &mut rendered); + let test_cwd = super::test_cwd(); + crate::markdown::append_markdown(&full, None, Some(test_cwd.as_path()), &mut rendered); let rendered_strs = lines_to_plain_strings(&rendered); assert_eq!(streamed_strs, rendered_strs, "full:\n---\n{full}\n---"); } diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__markdown_render_file_link_snapshot.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__markdown_render_file_link_snapshot.snap index 1b1f1210f4..63c42564de 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__markdown_render_file_link_snapshot.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__markdown_render_file_link_snapshot.snap @@ -3,4 +3,4 @@ source: tui/src/markdown_render_tests.rs assertion_line: 714 expression: rendered --- -See markdown_render.rs:74. +See codex-rs/tui/src/markdown_render.rs:74. diff --git a/codex-rs/tui/src/streaming/controller.rs b/codex-rs/tui/src/streaming/controller.rs index 6117485adf..7f7346265e 100644 --- a/codex-rs/tui/src/streaming/controller.rs +++ b/codex-rs/tui/src/streaming/controller.rs @@ -4,6 +4,7 @@ use crate::render::line_utils::prefix_lines; use crate::style::proposed_plan_style; use ratatui::prelude::Stylize; use ratatui::text::Line; +use std::path::Path; use std::time::Duration; use std::time::Instant; @@ -18,9 +19,13 @@ pub(crate) struct StreamController { } impl StreamController { - pub(crate) fn new(width: Option) -> Self { + /// Create a controller whose markdown renderer shortens local file links relative to `cwd`. + /// + /// The controller snapshots the path into stream state so later commit ticks and finalization + /// render against the same session cwd that was active when streaming started. + pub(crate) fn new(width: Option, cwd: &Path) -> Self { Self { - state: StreamState::new(width), + state: StreamState::new(width, cwd), finishing_after_drain: false, header_emitted: false, } @@ -115,9 +120,14 @@ pub(crate) struct PlanStreamController { } impl PlanStreamController { - pub(crate) fn new(width: Option) -> Self { + /// Create a plan-stream controller whose markdown renderer shortens local file links relative + /// to `cwd`. + /// + /// The controller snapshots the path into stream state so later commit ticks and finalization + /// render against the same session cwd that was active when streaming started. + pub(crate) fn new(width: Option, cwd: &Path) -> Self { Self { - state: StreamState::new(width), + state: StreamState::new(width, cwd), header_emitted: false, top_padding_emitted: false, } @@ -232,6 +242,13 @@ impl PlanStreamController { #[cfg(test)] mod tests { use super::*; + use std::path::PathBuf; + + fn test_cwd() -> PathBuf { + // These tests only need a stable absolute cwd; using temp_dir() avoids baking Unix- or + // Windows-specific root semantics into the fixtures. + std::env::temp_dir() + } fn lines_to_plain_strings(lines: &[ratatui::text::Line<'_>]) -> Vec { lines @@ -248,7 +265,7 @@ mod tests { #[tokio::test] async fn controller_loose_vs_tight_with_commit_ticks_matches_full() { - let mut ctrl = StreamController::new(None); + let mut ctrl = StreamController::new(None, &test_cwd()); let mut lines = Vec::new(); // Exact deltas from the session log (section: Loose vs. tight list items) @@ -346,7 +363,8 @@ mod tests { // Full render of the same source let source: String = deltas.iter().copied().collect(); let mut rendered: Vec> = Vec::new(); - crate::markdown::append_markdown(&source, None, &mut rendered); + let test_cwd = test_cwd(); + crate::markdown::append_markdown(&source, None, Some(test_cwd.as_path()), &mut rendered); let rendered_strs = lines_to_plain_strings(&rendered); assert_eq!(streamed, rendered_strs); diff --git a/codex-rs/tui/src/streaming/mod.rs b/codex-rs/tui/src/streaming/mod.rs index c783f27ae9..e39b00e097 100644 --- a/codex-rs/tui/src/streaming/mod.rs +++ b/codex-rs/tui/src/streaming/mod.rs @@ -10,6 +10,7 @@ //! arrival timestamp so policy code can reason about oldest queued age without peeking into text. use std::collections::VecDeque; +use std::path::Path; use std::time::Duration; use std::time::Instant; @@ -33,10 +34,13 @@ pub(crate) struct StreamState { } impl StreamState { - /// Creates an empty stream state with an optional target wrap width. - pub(crate) fn new(width: Option) -> Self { + /// Create stream state whose markdown collector renders local file links relative to `cwd`. + /// + /// Controllers are expected to pass the session cwd here once and keep it stable for the + /// lifetime of the active stream. + pub(crate) fn new(width: Option, cwd: &Path) -> Self { Self { - collector: MarkdownStreamCollector::new(width), + collector: MarkdownStreamCollector::new(width, cwd), queued_lines: VecDeque::new(), has_seen_delta: false, } @@ -102,10 +106,17 @@ impl StreamState { mod tests { use super::*; use pretty_assertions::assert_eq; + use std::path::PathBuf; + + fn test_cwd() -> PathBuf { + // These tests only need a stable absolute cwd; using temp_dir() avoids baking Unix- or + // Windows-specific root semantics into the fixtures. + std::env::temp_dir() + } #[test] fn drain_n_clamps_to_available_lines() { - let mut state = StreamState::new(None); + let mut state = StreamState::new(None, &test_cwd()); state.enqueue(vec![Line::from("one")]); let drained = state.drain_n(8); From 24b8d443b82aad9f336b9c379aebbeab8667466a Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 10 Mar 2026 18:33:52 -0700 Subject: [PATCH 4/7] Prefix code mode output with success or failure message and include error stack (#14272) --- codex-rs/core/src/tools/code_mode.rs | 101 ++++++---- codex-rs/core/src/tools/code_mode_runner.cjs | 8 +- codex-rs/core/src/tools/handlers/code_mode.rs | 3 +- codex-rs/core/src/tools/spec.rs | 2 +- codex-rs/core/tests/suite/code_mode.rs | 186 +++++++++++++----- 5 files changed, 211 insertions(+), 89 deletions(-) diff --git a/codex-rs/core/src/tools/code_mode.rs b/codex-rs/core/src/tools/code_mode.rs index 1a885b1b2e..6e2c704b7d 100644 --- a/codex-rs/core/src/tools/code_mode.rs +++ b/codex-rs/core/src/tools/code_mode.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use std::process::ExitStatus; use std::sync::Arc; +use std::time::Duration; use crate::client_common::tools::ToolSpec; use crate::codex::Session; @@ -10,6 +10,7 @@ use crate::exec_env::create_env; use crate::features::Feature; use crate::function_tool::FunctionCallError; use crate::tools::ToolRouter; +use crate::tools::context::FunctionToolOutput; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::context::ToolPayload; use crate::tools::js_repl::resolve_compatible_node; @@ -81,6 +82,8 @@ enum NodeToHostMessage { content_items: Vec, stored_values: HashMap, #[serde(default)] + error_text: Option, + #[serde(default)] max_output_tokens_per_exec_call: Option, }, } @@ -105,7 +108,7 @@ pub(crate) fn instructions(config: &Config) -> Option { )); section.push_str("- Import nested tools from `tools.js`, for example `import { exec_command } from \"tools.js\"` or `import { tools } from \"tools.js\"`. Namespaced tools are also available from `tools/.js`; MCP tools use `tools/mcp/.js`, for example `import { append_notebook_logs_chart } from \"tools/mcp/ologs.js\"`. `tools[name]` and identifier wrappers like `await exec_command(args)` remain available for compatibility. Nested tool calls resolve to their code-mode result values.\n"); section.push_str(&format!( - "- Import `{{ output_text, output_image, set_max_output_tokens_per_exec_call, store, load }}` from `@openai/code_mode` (or `\"openai/code_mode\"`). `output_text(value)` surfaces text back to the model and stringifies non-string objects with `JSON.stringify(...)` when possible. `output_image(imageUrl)` appends an `input_image` content item for `http(s)` or `data:` URLs. `store(key, value)` persists JSON-serializable values across `{PUBLIC_TOOL_NAME}` calls in the current session, and `load(key)` returns a cloned stored value or `undefined`. `set_max_output_tokens_per_exec_call(value)` sets the token budget used to truncate the final Rust-side result of the current `{PUBLIC_TOOL_NAME}` execution; the default is `10000`. This guards the overall `{PUBLIC_TOOL_NAME}` output, not individual nested tool invocations. When truncation happens, the final text uses the unified-exec style `Original token count:` / `Output:` wrapper and the usual `…N tokens truncated…` marker.\n", + "- Import `{{ output_text, output_image, set_max_output_tokens_per_exec_call, store, load }}` from `@openai/code_mode` (or `\"openai/code_mode\"`). `output_text(value)` surfaces text back to the model and stringifies non-string objects with `JSON.stringify(...)` when possible. `output_image(imageUrl)` appends an `input_image` content item for `http(s)` or `data:` URLs. `store(key, value)` persists JSON-serializable values across `{PUBLIC_TOOL_NAME}` calls in the current session, and `load(key)` returns a cloned stored value or `undefined`. `set_max_output_tokens_per_exec_call(value)` sets the token budget used to truncate the final Rust-side result of the current `{PUBLIC_TOOL_NAME}` execution; the default is `10000`. This guards the overall `{PUBLIC_TOOL_NAME}` output, not individual nested tool invocations. The returned content starts with a separate `Script completed` or `Script failed` text item that includes wall time. When truncation happens, the final text may include `Total output lines:` and the usual `…N tokens truncated…` marker.\n", )); section.push_str( "- Function tools require JSON object arguments. Freeform tools require raw strings.\n", @@ -121,7 +124,7 @@ pub(crate) async fn execute( turn: Arc, tracker: SharedTurnDiffTracker, code: String, -) -> Result, FunctionCallError> { +) -> Result { let exec = ExecContext { session, turn, @@ -140,8 +143,9 @@ async fn execute_node( source: String, enabled_tools: Vec, stored_values: HashMap, -) -> Result, String> { +) -> Result { let node_path = resolve_compatible_node(exec.turn.config.js_repl_node_path.as_deref()).await?; + let started_at = std::time::Instant::now(); let env = create_env(&exec.turn.shell_environment_policy, None); let mut cmd = tokio::process::Command::new(&node_path); @@ -190,7 +194,7 @@ async fn execute_node( .await?; let mut stdout_lines = BufReader::new(stdout).lines(); - let mut final_content_items = None; + let mut pending_result = None; while let Some(line) = stdout_lines .next_line() .await @@ -213,6 +217,7 @@ async fn execute_node( NodeToHostMessage::Result { content_items, stored_values, + error_text, max_output_tokens_per_exec_call, } => { exec.session @@ -220,8 +225,9 @@ async fn execute_node( .code_mode_store .replace_stored_values(stored_values) .await; - final_content_items = Some(truncate_code_mode_result( + pending_result = Some(( output_content_items_from_json_values(content_items)?, + error_text, max_output_tokens_per_exec_call, )); break; @@ -238,20 +244,39 @@ async fn execute_node( let stderr = stderr_task .await .map_err(|err| format!("failed to collect {PUBLIC_TOOL_NAME} stderr: {err}"))?; + let wall_time = started_at.elapsed(); + let success = status.success(); - match final_content_items { - Some(content_items) if status.success() => Ok(content_items), - Some(_) => Err(format_runner_failure( - &format!("{PUBLIC_TOOL_NAME} execution failed"), - status, - &stderr, - )), - None => Err(format_runner_failure( - &format!("{PUBLIC_TOOL_NAME} runner exited without returning a result"), - status, - &stderr, - )), + let Some((mut content_items, error_text, max_output_tokens_per_exec_call)) = pending_result + else { + let message = if stderr.is_empty() { + format!("{PUBLIC_TOOL_NAME} runner exited without returning a result (status {status})") + } else { + stderr + }; + return Err(message); + }; + + if !success { + let error_text = error_text.unwrap_or_else(|| { + if stderr.is_empty() { + format!("Process exited with status {status}") + } else { + stderr + } + }); + content_items.push(FunctionCallOutputContentItem::InputText { + text: format!("Script error:\n{error_text}"), + }); } + + let mut content_items = + truncate_code_mode_result(content_items, max_output_tokens_per_exec_call); + prepend_script_status(&mut content_items, success, wall_time); + Ok(FunctionToolOutput::from_content( + content_items, + Some(success), + )) } async fn write_message( @@ -274,15 +299,21 @@ async fn write_message( .map_err(|err| format!("failed to flush {PUBLIC_TOOL_NAME} message: {err}")) } -fn append_stderr(message: String, stderr: &str) -> String { - if stderr.trim().is_empty() { - return message; - } - format!("{message}\n\nnode stderr:\n{stderr}") -} - -fn format_runner_failure(message: &str, status: ExitStatus, stderr: &str) -> String { - append_stderr(format!("{message} (status {status})"), stderr) +fn prepend_script_status( + content_items: &mut Vec, + success: bool, + wall_time: Duration, +) { + let wall_time_seconds = ((wall_time.as_secs_f32()) * 10.0).round() / 10.0; + let header = format!( + "{}\nWall time {wall_time_seconds:.1} seconds\nOutput:\n", + if success { + "Script completed" + } else { + "Script failed" + } + ); + content_items.insert(0, FunctionCallOutputContentItem::InputText { text: header }); } fn build_source(user_code: &str, enabled_tools: &[EnabledTool]) -> Result { @@ -301,25 +332,17 @@ fn truncate_code_mode_result( max_output_tokens_per_exec_call: Option, ) -> Vec { let max_output_tokens = resolve_max_tokens(max_output_tokens_per_exec_call); + let policy = TruncationPolicy::Tokens(max_output_tokens); if items .iter() .all(|item| matches!(item, FunctionCallOutputContentItem::InputText { .. })) { - let (mut truncated_items, original_token_count) = - formatted_truncate_text_content_items_with_policy( - &items, - TruncationPolicy::Tokens(max_output_tokens), - ); - if let Some(original_token_count) = original_token_count - && let Some(FunctionCallOutputContentItem::InputText { text }) = - truncated_items.first_mut() - { - *text = format!("Original token count: {original_token_count}\nOutput:\n{text}"); - } + let (truncated_items, _) = + formatted_truncate_text_content_items_with_policy(&items, policy); return truncated_items; } - truncate_function_output_items_with_policy(&items, TruncationPolicy::Tokens(max_output_tokens)) + truncate_function_output_items_with_policy(&items, policy) } async fn build_enabled_tools(exec: &ExecContext) -> Vec { diff --git a/codex-rs/core/src/tools/code_mode_runner.cjs b/codex-rs/core/src/tools/code_mode_runner.cjs index 00395c1df7..8e5cc9d38a 100644 --- a/codex-rs/core/src/tools/code_mode_runner.cjs +++ b/codex-rs/core/src/tools/code_mode_runner.cjs @@ -104,6 +104,10 @@ function readContentItems(context) { } } +function formatErrorText(error) { + return String(error && error.stack ? error.stack : error); +} + function isValidIdentifier(name) { return /^[A-Za-z_$][0-9A-Za-z_$]*$/.test(name); } @@ -378,11 +382,11 @@ async function main() { }); process.exit(0); } catch (error) { - process.stderr.write(`${String(error && error.stack ? error.stack : error)}\n`); await protocol.send({ type: 'result', content_items: readContentItems(context), stored_values: state.storedValues, + error_text: formatErrorText(error), max_output_tokens_per_exec_call: state.maxOutputTokensPerExecCall, }); process.exit(1); @@ -391,7 +395,7 @@ async function main() { void main().catch(async (error) => { try { - process.stderr.write(`${String(error && error.stack ? error.stack : error)}\n`); + process.stderr.write(`${formatErrorText(error)}\n`); } finally { process.exitCode = 1; } diff --git a/codex-rs/core/src/tools/handlers/code_mode.rs b/codex-rs/core/src/tools/handlers/code_mode.rs index 3637f61727..4763a69b46 100644 --- a/codex-rs/core/src/tools/handlers/code_mode.rs +++ b/codex-rs/core/src/tools/handlers/code_mode.rs @@ -48,7 +48,6 @@ impl ToolHandler for CodeModeHandler { } }; - let content_items = code_mode::execute(session, turn, tracker, code).await?; - Ok(FunctionToolOutput::from_content(content_items, Some(true))) + code_mode::execute(session, turn, tracker, code).await } } diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 2adebe78f1..321a5377fb 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -1621,7 +1621,7 @@ source: /[\s\S]+/ enabled_tool_names.join(", ") }; let description = format!( - "Runs JavaScript in a Node-backed `node:vm` context. This is a freeform tool: send raw JavaScript source text (no JSON/quotes/markdown fences). Direct tool calls remain available while `{PUBLIC_TOOL_NAME}` is enabled. Inside JavaScript, import nested tools from `tools.js`, for example `import {{ exec_command }} from \"tools.js\"` or `import {{ tools }} from \"tools.js\"`. Namespaced tools are also available from `tools/.js`; MCP tools use `tools/mcp/.js`, for example `import {{ append_notebook_logs_chart }} from \"tools/mcp/ologs.js\"`. `tools[name]` and identifier wrappers like `await shell(args)` remain available for compatibility when the tool name is a valid JS identifier. Nested tool calls resolve to their code-mode result values. Import `{{ output_text, output_image, set_max_output_tokens_per_exec_call, store, load }}` from `\"@openai/code_mode\"` (or `\"openai/code_mode\"`); `output_text(value)` surfaces text back to the model and stringifies non-string objects when possible, `output_image(imageUrl)` appends an `input_image` content item for `http(s)` or `data:` URLs, `store(key, value)` persists JSON-serializable values across `{PUBLIC_TOOL_NAME}` calls in the current session, `load(key)` returns a cloned stored value or `undefined`, and `set_max_output_tokens_per_exec_call(value)` sets the token budget used to truncate the final Rust-side result of the current `{PUBLIC_TOOL_NAME}` execution. The default is `10000`. This guards the overall `{PUBLIC_TOOL_NAME}` output, not individual nested tool invocations. When truncation happens, the final text uses the unified-exec style `Original token count:` / `Output:` wrapper and the usual `…N tokens truncated…` marker. Function tools require JSON object arguments. Freeform tools require raw strings. `add_content(value)` remains available for compatibility with a content item, content-item array, or string. Structured nested-tool results should be converted to text first, for example with `JSON.stringify(...)`. Only content passed to `output_text(...)`, `output_image(...)`, or `add_content(value)` is surfaced back to the model. Enabled nested tools: {enabled_list}." + "Runs JavaScript in a Node-backed `node:vm` context. This is a freeform tool: send raw JavaScript source text (no JSON/quotes/markdown fences). Direct tool calls remain available while `{PUBLIC_TOOL_NAME}` is enabled. Inside JavaScript, import nested tools from `tools.js`, for example `import {{ exec_command }} from \"tools.js\"` or `import {{ tools }} from \"tools.js\"`. Namespaced tools are also available from `tools/.js`; MCP tools use `tools/mcp/.js`, for example `import {{ append_notebook_logs_chart }} from \"tools/mcp/ologs.js\"`. `tools[name]` and identifier wrappers like `await shell(args)` remain available for compatibility when the tool name is a valid JS identifier. Nested tool calls resolve to their code-mode result values. Import `{{ output_text, output_image, set_max_output_tokens_per_exec_call, store, load }}` from `\"@openai/code_mode\"` (or `\"openai/code_mode\"`); `output_text(value)` surfaces text back to the model and stringifies non-string objects when possible, `output_image(imageUrl)` appends an `input_image` content item for `http(s)` or `data:` URLs, `store(key, value)` persists JSON-serializable values across `{PUBLIC_TOOL_NAME}` calls in the current session, `load(key)` returns a cloned stored value or `undefined`, and `set_max_output_tokens_per_exec_call(value)` sets the token budget used to truncate the final Rust-side result of the current `{PUBLIC_TOOL_NAME}` execution. The default is `10000`. This guards the overall `{PUBLIC_TOOL_NAME}` output, not individual nested tool invocations. The returned content starts with a separate `Script completed` or `Script failed` text item that includes wall time. When truncation happens, the final text may include `Total output lines:` and the usual `…N tokens truncated…` marker. Function tools require JSON object arguments. Freeform tools require raw strings. `add_content(value)` remains available for compatibility with a content item, content-item array, or string. Structured nested-tool results should be converted to text first, for example with `JSON.stringify(...)`. Only content passed to `output_text(...)`, `output_image(...)`, or `add_content(value)` is surfaced back to the model. Enabled nested tools: {enabled_list}." ); ToolSpec::Freeform(FreeformTool { diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index f341c23366..ecca32a336 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -24,14 +24,35 @@ use std::fs; use std::time::Duration; use wiremock::MockServer; -fn custom_tool_output_text_and_success( +fn custom_tool_output_items(req: &ResponsesRequest, call_id: &str) -> Vec { + req.custom_tool_call_output(call_id) + .get("output") + .and_then(Value::as_array) + .expect("custom tool output should be serialized as content items") + .clone() +} + +fn text_item(items: &[Value], index: usize) -> &str { + items[index] + .get("text") + .and_then(Value::as_str) + .expect("content item should be input_text") +} + +fn custom_tool_output_body_and_success( req: &ResponsesRequest, call_id: &str, ) -> (String, Option) { - let (output, success) = req + let (_, success) = req .custom_tool_call_output_content_and_success(call_id) .expect("custom tool output should be present"); - (output.unwrap_or_default(), success) + let items = custom_tool_output_items(req, call_id); + let output = items + .iter() + .skip(1) + .filter_map(|item| item.get("text").and_then(Value::as_str)) + .collect(); + (output, success) } async fn run_code_mode_turn( @@ -152,13 +173,16 @@ add_content(JSON.stringify(await exec_command({ cmd: "printf code_mode_exec_mark .await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); - assert_ne!( - success, - Some(false), - "exec call failed unexpectedly: {output}" + let items = custom_tool_output_items(&req, "call-1"); + assert_eq!(items.len(), 2); + assert_regex_match( + concat!( + r"(?s)\A", + r"Script completed\nWall time \d+\.\d seconds\nOutput:\n\z" + ), + text_item(&items, 0), ); - let parsed: Value = serde_json::from_str(&output)?; + let parsed: Value = serde_json::from_str(text_item(&items, 1))?; assert!( parsed .get("chunk_id") @@ -201,22 +225,66 @@ add_content(JSON.stringify(await exec_command({ .await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); - assert_ne!( - success, - Some(false), - "exec call failed unexpectedly: {output}" + let items = custom_tool_output_items(&req, "call-1"); + assert_eq!(items.len(), 2); + assert_regex_match( + concat!( + r"(?s)\A", + r"Script completed\nWall time \d+\.\d seconds\nOutput:\n\z" + ), + text_item(&items, 0), ); let expected_pattern = r#"(?sx) \A -Original\ token\ count:\ \d+\n -Output:\n Total\ output\ lines:\ 1\n \n -\{"chunk_id".*…\d+\ tokens\ truncated….* +.*…\d+\ tokens\ truncated….* \z "#; - assert_regex_match(expected_pattern, &output); + assert_regex_match(expected_pattern, text_item(&items, 1)); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_returns_accumulated_output_when_script_fails() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let (_test, second_mock) = run_code_mode_turn( + &server, + "use code_mode to surface script failures", + r#" +add_content("before crash"); +add_content("still before crash"); +throw new Error("boom"); +"#, + false, + ) + .await?; + + let req = second_mock.single_request(); + let items = custom_tool_output_items(&req, "call-1"); + assert_eq!(items.len(), 4); + assert_regex_match( + concat!( + r"(?s)\A", + r"Script failed\nWall time \d+\.\d seconds\nOutput:\n\z" + ), + text_item(&items, 0), + ); + assert_eq!(text_item(&items, 1), "before crash"); + assert_eq!(text_item(&items, 2), "still before crash"); + assert_regex_match( + r#"(?sx) +\A +Script\ error:\n +Error:\ boom\n +(?:\s+at\ .+\n?)+ +\z +"#, + text_item(&items, 3), + ); Ok(()) } @@ -239,7 +307,7 @@ output_text({ json: true }); .await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); assert_ne!( success, Some(false), @@ -270,14 +338,25 @@ output_text(circular); .await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let items = custom_tool_output_items(&req, "call-1"); + let (_, success) = req + .custom_tool_call_output_content_and_success("call-1") + .expect("custom tool output should be present"); assert_ne!( success, Some(true), "circular stringify unexpectedly succeeded" ); - assert!(output.contains("exec execution failed")); - assert!(output.contains("Converting circular structure to JSON")); + assert_eq!(items.len(), 2); + assert_regex_match( + concat!( + r"(?s)\A", + r"Script failed\nWall time \d+\.\d seconds\nOutput:\n\z" + ), + text_item(&items, 0), + ); + assert!(text_item(&items, 1).contains("Script error:")); + assert!(text_item(&items, 1).contains("Converting circular structure to JSON")); Ok(()) } @@ -301,28 +380,34 @@ output_image("data:image/png;base64,AAA"); .await?; let req = second_mock.single_request(); - let (_, success) = custom_tool_output_text_and_success(&req, "call-1"); + let items = custom_tool_output_items(&req, "call-1"); + let (_, success) = custom_tool_output_body_and_success(&req, "call-1"); assert_ne!( success, Some(false), "code_mode image output failed unexpectedly" ); + assert_eq!(items.len(), 3); + assert_regex_match( + concat!( + r"(?s)\A", + r"Script completed\nWall time \d+\.\d seconds\nOutput:\n\z" + ), + text_item(&items, 0), + ); assert_eq!( - req.custom_tool_call_output("call-1"), + items[1], serde_json::json!({ - "type": "custom_tool_call_output", - "call_id": "call-1", - "output": [ - { - "type": "input_image", - "image_url": "https://example.com/image.jpg" - }, - { - "type": "input_image", - "image_url": "data:image/png;base64,AAA" - } - ] - }) + "type": "input_image", + "image_url": "https://example.com/image.jpg" + }), + ); + assert_eq!( + items[2], + serde_json::json!({ + "type": "input_image", + "image_url": "data:image/png;base64,AAA" + }), ); Ok(()) @@ -345,11 +430,22 @@ async fn code_mode_can_apply_patch_via_nested_tool() -> Result<()> { run_code_mode_turn(&server, "use exec to run apply_patch", &code, true).await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let items = custom_tool_output_items(&req, "call-1"); + let (_, success) = req + .custom_tool_call_output_content_and_success("call-1") + .expect("custom tool output should be present"); assert_ne!( success, Some(false), - "exec apply_patch call failed unexpectedly: {output}" + "exec apply_patch call failed unexpectedly: {items:?}" + ); + assert_eq!(items.len(), 2); + assert_regex_match( + concat!( + r"(?s)\A", + r"Script completed\nWall time \d+\.\d seconds\nOutput:\n\z" + ), + text_item(&items, 0), ); let file_path = test.cwd_path().join(file_name); @@ -381,7 +477,7 @@ add_content( run_code_mode_turn_with_rmcp(&server, "use exec to run the rmcp echo tool", code).await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); assert_ne!( success, Some(false), @@ -420,7 +516,7 @@ add_content( run_code_mode_turn_with_rmcp(&server, "use exec to run the rmcp echo tool", code).await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); assert_ne!( success, Some(false), @@ -464,7 +560,7 @@ add_content( .await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); assert_ne!( success, Some(false), @@ -505,7 +601,7 @@ add_content( run_code_mode_turn_with_rmcp(&server, "use exec to call rmcp echo badly", code).await?; let req = second_mock.single_request(); - let (output, success) = custom_tool_output_text_and_success(&req, "call-1"); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); assert_ne!( success, Some(false), @@ -562,7 +658,7 @@ add_content("stored"); let first_request = first_follow_up.single_request(); let (first_output, first_success) = - custom_tool_output_text_and_success(&first_request, "call-1"); + custom_tool_output_body_and_success(&first_request, "call-1"); assert_ne!( first_success, Some(false), @@ -600,7 +696,7 @@ add_content(JSON.stringify(load("nb"))); let second_request = second_follow_up.single_request(); let (second_output, second_success) = - custom_tool_output_text_and_success(&second_request, "call-2"); + custom_tool_output_body_and_success(&second_request, "call-2"); assert_ne!( second_success, Some(false), From 566897d427a86ea5d4d95e596462614fe101df75 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 10 Mar 2026 18:38:39 -0700 Subject: [PATCH 5/7] Make unified exec session_id numeric (#14279) It's a number on the write_stdin input, make it a number on the output and also internally. --- codex-rs/core/src/tools/context.rs | 6 +- .../core/src/tools/handlers/unified_exec.rs | 8 +- codex-rs/core/src/tools/spec.rs | 2 +- .../core/src/unified_exec/async_watcher.rs | 4 +- codex-rs/core/src/unified_exec/errors.rs | 2 +- codex-rs/core/src/unified_exec/mod.rs | 50 ++---- .../core/src/unified_exec/process_manager.rs | 154 +++++++++--------- 7 files changed, 99 insertions(+), 127 deletions(-) diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index 36a328b37d..041de50f5e 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -159,7 +159,7 @@ pub struct ExecCommandToolOutput { /// Raw bytes returned for this unified exec call before any truncation. pub raw_output: Vec, pub max_output_tokens: Option, - pub process_id: Option, + pub process_id: Option, pub exit_code: Option, pub original_token_count: Option, pub session_command: Option>, @@ -194,7 +194,7 @@ impl ToolOutput for ExecCommandToolOutput { #[serde(skip_serializing_if = "Option::is_none")] exit_code: Option, #[serde(skip_serializing_if = "Option::is_none")] - session_id: Option, + session_id: Option, #[serde(skip_serializing_if = "Option::is_none")] original_token_count: Option, output: String, @@ -204,7 +204,7 @@ impl ToolOutput for ExecCommandToolOutput { chunk_id: (!self.chunk_id.is_empty()).then(|| self.chunk_id.clone()), wall_time_seconds: self.wall_time.as_secs_f64(), exit_code: self.exit_code, - session_id: self.process_id.clone(), + session_id: self.process_id, original_token_count: self.original_token_count, output: self.truncated_output(), }; diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 4eb7125e9f..edc6763ef2 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -190,7 +190,7 @@ impl ToolHandler for UnifiedExecHandler { ) { let approval_policy = context.turn.approval_policy.value(); - manager.release_process_id(&process_id).await; + manager.release_process_id(process_id).await; return Err(FunctionCallError::RespondToModel(format!( "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" ))); @@ -211,7 +211,7 @@ impl ToolHandler for UnifiedExecHandler { ) { Ok(normalized) => normalized, Err(err) => { - manager.release_process_id(&process_id).await; + manager.release_process_id(process_id).await; return Err(FunctionCallError::RespondToModel(err)); } }; @@ -228,7 +228,7 @@ impl ToolHandler for UnifiedExecHandler { ) .await? { - manager.release_process_id(&process_id).await; + manager.release_process_id(process_id).await; return Ok(ExecCommandToolOutput { event_call_id: String::new(), chunk_id: String::new(), @@ -271,7 +271,7 @@ impl ToolHandler for UnifiedExecHandler { let args: WriteStdinArgs = parse_arguments(&arguments)?; let response = manager .write_stdin(WriteStdinRequest { - process_id: &args.session_id.to_string(), + process_id: args.session_id, input: &args.chars, yield_time_ms: args.yield_time_ms, max_output_tokens: args.max_output_tokens, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 321a5377fb..8f7a25076e 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -59,7 +59,7 @@ fn unified_exec_output_schema() -> JsonValue { "description": "Process exit code when the command finished during this call." }, "session_id": { - "type": "string", + "type": "number", "description": "Session identifier to pass to write_stdin when the process is still running." }, "original_token_count": { diff --git a/codex-rs/core/src/unified_exec/async_watcher.rs b/codex-rs/core/src/unified_exec/async_watcher.rs index 1fbb1e8f6d..47543a00fc 100644 --- a/codex-rs/core/src/unified_exec/async_watcher.rs +++ b/codex-rs/core/src/unified_exec/async_watcher.rs @@ -110,7 +110,7 @@ pub(crate) fn spawn_exit_watcher( call_id: String, command: Vec, cwd: PathBuf, - process_id: String, + process_id: i32, transcript: Arc>, started_at: Instant, ) { @@ -129,7 +129,7 @@ pub(crate) fn spawn_exit_watcher( call_id, command, cwd, - Some(process_id), + Some(process_id.to_string()), transcript, String::new(), exit_code, diff --git a/codex-rs/core/src/unified_exec/errors.rs b/codex-rs/core/src/unified_exec/errors.rs index 284c7bca6d..966775eee2 100644 --- a/codex-rs/core/src/unified_exec/errors.rs +++ b/codex-rs/core/src/unified_exec/errors.rs @@ -7,7 +7,7 @@ pub(crate) enum UnifiedExecError { CreateProcess { message: String }, // The model is trained on `session_id`, but internally we track a `process_id`. #[error("Unknown process id {process_id}")] - UnknownProcessId { process_id: String }, + UnknownProcessId { process_id: i32 }, #[error("failed to write to stdin")] WriteToStdin, #[error( diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index c090346a28..91af47accd 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -86,7 +86,7 @@ impl UnifiedExecContext { #[derive(Debug)] pub(crate) struct ExecCommandRequest { pub command: Vec, - pub process_id: String, + pub process_id: i32, pub yield_time_ms: u64, pub max_output_tokens: Option, pub workdir: Option, @@ -101,7 +101,7 @@ pub(crate) struct ExecCommandRequest { #[derive(Debug)] pub(crate) struct WriteStdinRequest<'a> { - pub process_id: &'a str, + pub process_id: i32, pub input: &'a str, pub yield_time_ms: u64, pub max_output_tokens: Option, @@ -109,14 +109,14 @@ pub(crate) struct WriteStdinRequest<'a> { #[derive(Default)] pub(crate) struct ProcessStore { - processes: HashMap, - reserved_process_ids: HashSet, + processes: HashMap, + reserved_process_ids: HashSet, } impl ProcessStore { - fn remove(&mut self, process_id: &str) -> Option { - self.reserved_process_ids.remove(process_id); - self.processes.remove(process_id) + fn remove(&mut self, process_id: i32) -> Option { + self.reserved_process_ids.remove(&process_id); + self.processes.remove(&process_id) } } @@ -144,7 +144,7 @@ impl Default for UnifiedExecProcessManager { struct ProcessEntry { process: Arc, call_id: String, - process_id: String, + process_id: i32, command: Vec, tty: bool, network_approval_id: Option, @@ -238,7 +238,7 @@ mod tests { async fn write_stdin( session: &Arc, - process_id: &str, + process_id: i32, input: &str, yield_time_ms: u64, ) -> Result { @@ -294,11 +294,7 @@ mod tests { let (session, turn) = test_session_and_turn().await; let open_shell = exec_command(&session, &turn, "bash -i", 2_500).await?; - let process_id = open_shell - .process_id - .as_ref() - .expect("expected process_id") - .as_str(); + let process_id = open_shell.process_id.expect("expected process_id"); write_stdin( &session, @@ -330,15 +326,11 @@ mod tests { let (session, turn) = test_session_and_turn().await; let shell_a = exec_command(&session, &turn, "bash -i", 2_500).await?; - let session_a = shell_a - .process_id - .as_ref() - .expect("expected process id") - .clone(); + let session_a = shell_a.process_id.expect("expected process id"); write_stdin( &session, - session_a.as_str(), + session_a, "export CODEX_INTERACTIVE_SHELL_VAR=codex\n", 2_500, ) @@ -358,11 +350,7 @@ mod tests { let out_3 = write_stdin( &session, - shell_a - .process_id - .as_ref() - .expect("expected process id") - .as_str(), + shell_a.process_id.expect("expected process id"), "echo $CODEX_INTERACTIVE_SHELL_VAR\n", 2_500, ) @@ -384,11 +372,7 @@ mod tests { let (session, turn) = test_session_and_turn().await; let open_shell = exec_command(&session, &turn, "bash -i", 2_500).await?; - let process_id = open_shell - .process_id - .as_ref() - .expect("expected process id") - .as_str(); + let process_id = open_shell.process_id.expect("expected process id"); write_stdin( &session, @@ -501,11 +485,7 @@ mod tests { let (session, turn) = test_session_and_turn().await; let open_shell = exec_command(&session, &turn, "bash -i", 2_500).await?; - let process_id = open_shell - .process_id - .as_ref() - .expect("expected process id") - .as_str(); + let process_id = open_shell.process_id.expect("expected process id"); write_stdin(&session, process_id, "exit\n", 2_500).await?; diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index f50da1f71f..29311b1ff4 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -98,41 +98,39 @@ struct PreparedProcessHandles { cancellation_token: CancellationToken, pause_state: Option>, command: Vec, - process_id: String, + process_id: i32, tty: bool, } impl UnifiedExecProcessManager { - pub(crate) async fn allocate_process_id(&self) -> String { + pub(crate) async fn allocate_process_id(&self) -> i32 { loop { let mut store = self.process_store.lock().await; let process_id = if should_use_deterministic_process_ids() { // test or deterministic mode - let next = store + store .reserved_process_ids .iter() - .filter_map(|s| s.parse::().ok()) + .copied() .max() .map(|m| std::cmp::max(m, 999) + 1) - .unwrap_or(1000); - - next.to_string() + .unwrap_or(1000) } else { // production mode → random - rand::rng().random_range(1_000..100_000).to_string() + rand::rng().random_range(1_000..100_000) }; if store.reserved_process_ids.contains(&process_id) { continue; } - store.reserved_process_ids.insert(process_id.clone()); + store.reserved_process_ids.insert(process_id); return process_id; } } - pub(crate) async fn release_process_id(&self, process_id: &str) { + pub(crate) async fn release_process_id(&self, process_id: i32) { let removed = { let mut store = self.process_store.lock().await; store.remove(process_id) @@ -172,7 +170,7 @@ impl UnifiedExecProcessManager { (Arc::new(process), deferred_network_approval) } Err(err) => { - self.release_process_id(&request.process_id).await; + self.release_process_id(request.process_id).await; return Err(err); } }; @@ -188,7 +186,7 @@ impl UnifiedExecProcessManager { &request.command, cwd.clone(), ExecCommandSource::UnifiedExecStartup, - Some(request.process_id.clone()), + Some(request.process_id.to_string()), ); emitter.emit(event_ctx, ToolEventStage::Begin).await; @@ -227,7 +225,7 @@ impl UnifiedExecProcessManager { let exit_code = process.exit_code(); let has_exited = process.has_exited() || exit_code.is_some(); let chunk_id = generate_chunk_id(); - let process_id = request.process_id.clone(); + let process_id = request.process_id; if has_exited { // Short‑lived command: emit ExecCommandEnd immediately using the @@ -240,7 +238,7 @@ impl UnifiedExecProcessManager { context.call_id.clone(), request.command.clone(), cwd.clone(), - Some(process_id), + Some(process_id.to_string()), Arc::clone(&transcript), text.clone(), exit, @@ -248,7 +246,7 @@ impl UnifiedExecProcessManager { ) .await; - self.release_process_id(&request.process_id).await; + self.release_process_id(request.process_id).await; finish_deferred_network_approval( context.session.as_ref(), deferred_network_approval.take(), @@ -287,7 +285,7 @@ impl UnifiedExecProcessManager { process_id: if has_exited { None } else { - Some(request.process_id.clone()) + Some(request.process_id) }, exit_code, original_token_count: Some(original_token_count), @@ -301,7 +299,7 @@ impl UnifiedExecProcessManager { &self, request: WriteStdinRequest<'_>, ) -> Result { - let process_id = request.process_id.to_string(); + let process_id = request.process_id; let PreparedProcessHandles { writer_tx, @@ -315,7 +313,7 @@ impl UnifiedExecProcessManager { process_id, tty, .. - } = self.prepare_process_handles(process_id.as_str()).await?; + } = self.prepare_process_handles(process_id).await?; if !request.input.is_empty() { if !tty { @@ -359,7 +357,7 @@ impl UnifiedExecProcessManager { // still alive or has exited and been removed from the store; we thread // that through so the handler can tag TerminalInteraction with an // appropriate process_id and exit_code. - let status = self.refresh_process_state(process_id.as_str()).await; + let status = self.refresh_process_state(process_id).await; let (process_id, exit_code, event_call_id) = match status { ProcessStatus::Alive { exit_code, @@ -372,7 +370,7 @@ impl UnifiedExecProcessManager { } ProcessStatus::Unknown => { return Err(UnifiedExecError::UnknownProcessId { - process_id: request.process_id.to_string(), + process_id: request.process_id, }); } }; @@ -392,18 +390,18 @@ impl UnifiedExecProcessManager { Ok(response) } - async fn refresh_process_state(&self, process_id: &str) -> ProcessStatus { + async fn refresh_process_state(&self, process_id: i32) -> ProcessStatus { let status = { let mut store = self.process_store.lock().await; - let Some(entry) = store.processes.get(process_id) else { + let Some(entry) = store.processes.get(&process_id) else { return ProcessStatus::Unknown; }; let exit_code = entry.process.exit_code(); - let process_id = entry.process_id.clone(); + let process_id = entry.process_id; if entry.process.has_exited() { - let Some(entry) = store.remove(&process_id) else { + let Some(entry) = store.remove(process_id) else { return ProcessStatus::Unknown; }; ProcessStatus::Exited { @@ -426,16 +424,13 @@ impl UnifiedExecProcessManager { async fn prepare_process_handles( &self, - process_id: &str, + process_id: i32, ) -> Result { let mut store = self.process_store.lock().await; - let entry = - store - .processes - .get_mut(process_id) - .ok_or(UnifiedExecError::UnknownProcessId { - process_id: process_id.to_string(), - })?; + let entry = store + .processes + .get_mut(&process_id) + .ok_or(UnifiedExecError::UnknownProcessId { process_id })?; entry.last_used = Instant::now(); let OutputHandles { output_buffer, @@ -458,7 +453,7 @@ impl UnifiedExecProcessManager { cancellation_token, pause_state, command: entry.command.clone(), - process_id: entry.process_id.clone(), + process_id: entry.process_id, tty: entry.tty, }) } @@ -481,7 +476,7 @@ impl UnifiedExecProcessManager { command: &[String], cwd: PathBuf, started_at: Instant, - process_id: String, + process_id: i32, tty: bool, network_approval_id: Option, transcript: Arc>, @@ -489,7 +484,7 @@ impl UnifiedExecProcessManager { let entry = ProcessEntry { process: Arc::clone(&process), call_id: context.call_id.clone(), - process_id: process_id.clone(), + process_id, command: command.to_vec(), tty, network_approval_id, @@ -499,7 +494,7 @@ impl UnifiedExecProcessManager { let (number_processes, pruned_entry) = { let mut store = self.process_store.lock().await; let pruned_entry = Self::prune_processes_if_needed(&mut store); - store.processes.insert(process_id.clone(), entry); + store.processes.insert(process_id, entry); (store.processes.len(), pruned_entry) }; // prune_processes_if_needed runs while holding process_store; do async @@ -526,7 +521,7 @@ impl UnifiedExecProcessManager { context.call_id.clone(), command.to_vec(), cwd, - process_id.clone(), + process_id, transcript, started_at, ); @@ -759,31 +754,31 @@ impl UnifiedExecProcessManager { return None; } - let meta: Vec<(String, Instant, bool)> = store + let meta: Vec<(i32, Instant, bool)> = store .processes .iter() - .map(|(id, entry)| (id.clone(), entry.last_used, entry.process.has_exited())) + .map(|(id, entry)| (*id, entry.last_used, entry.process.has_exited())) .collect(); if let Some(process_id) = Self::process_id_to_prune_from_meta(&meta) { - return store.remove(&process_id); + return store.remove(process_id); } None } // Centralized pruning policy so we can easily swap strategies later. - fn process_id_to_prune_from_meta(meta: &[(String, Instant, bool)]) -> Option { + fn process_id_to_prune_from_meta(meta: &[(i32, Instant, bool)]) -> Option { if meta.is_empty() { return None; } let mut by_recency = meta.to_vec(); by_recency.sort_by_key(|(_, last_used, _)| Reverse(*last_used)); - let protected: HashSet = by_recency + let protected: HashSet = by_recency .iter() .take(8) - .map(|(process_id, _, _)| process_id.clone()) + .map(|(process_id, _, _)| *process_id) .collect(); let mut lru = meta.to_vec(); @@ -793,7 +788,7 @@ impl UnifiedExecProcessManager { .iter() .find(|(process_id, _, exited)| !protected.contains(process_id) && *exited) { - return Some(process_id.clone()); + return Some(*process_id); } lru.into_iter() @@ -824,7 +819,7 @@ enum ProcessStatus { Alive { exit_code: Option, call_id: String, - process_id: String, + process_id: i32, }, Exited { exit_code: Option, @@ -874,67 +869,64 @@ mod tests { #[test] fn pruning_prefers_exited_processes_outside_recently_used() { let now = Instant::now(); - let id = |n: i32| n.to_string(); let meta = vec![ - (id(1), now - Duration::from_secs(40), false), - (id(2), now - Duration::from_secs(30), true), - (id(3), now - Duration::from_secs(20), false), - (id(4), now - Duration::from_secs(19), false), - (id(5), now - Duration::from_secs(18), false), - (id(6), now - Duration::from_secs(17), false), - (id(7), now - Duration::from_secs(16), false), - (id(8), now - Duration::from_secs(15), false), - (id(9), now - Duration::from_secs(14), false), - (id(10), now - Duration::from_secs(13), false), + (1, now - Duration::from_secs(40), false), + (2, now - Duration::from_secs(30), true), + (3, now - Duration::from_secs(20), false), + (4, now - Duration::from_secs(19), false), + (5, now - Duration::from_secs(18), false), + (6, now - Duration::from_secs(17), false), + (7, now - Duration::from_secs(16), false), + (8, now - Duration::from_secs(15), false), + (9, now - Duration::from_secs(14), false), + (10, now - Duration::from_secs(13), false), ]; let candidate = UnifiedExecProcessManager::process_id_to_prune_from_meta(&meta); - assert_eq!(candidate, Some(id(2))); + assert_eq!(candidate, Some(2)); } #[test] fn pruning_falls_back_to_lru_when_no_exited() { let now = Instant::now(); - let id = |n: i32| n.to_string(); let meta = vec![ - (id(1), now - Duration::from_secs(40), false), - (id(2), now - Duration::from_secs(30), false), - (id(3), now - Duration::from_secs(20), false), - (id(4), now - Duration::from_secs(19), false), - (id(5), now - Duration::from_secs(18), false), - (id(6), now - Duration::from_secs(17), false), - (id(7), now - Duration::from_secs(16), false), - (id(8), now - Duration::from_secs(15), false), - (id(9), now - Duration::from_secs(14), false), - (id(10), now - Duration::from_secs(13), false), + (1, now - Duration::from_secs(40), false), + (2, now - Duration::from_secs(30), false), + (3, now - Duration::from_secs(20), false), + (4, now - Duration::from_secs(19), false), + (5, now - Duration::from_secs(18), false), + (6, now - Duration::from_secs(17), false), + (7, now - Duration::from_secs(16), false), + (8, now - Duration::from_secs(15), false), + (9, now - Duration::from_secs(14), false), + (10, now - Duration::from_secs(13), false), ]; let candidate = UnifiedExecProcessManager::process_id_to_prune_from_meta(&meta); - assert_eq!(candidate, Some(id(1))); + assert_eq!(candidate, Some(1)); } #[test] fn pruning_protects_recent_processes_even_if_exited() { let now = Instant::now(); - let id = |n: i32| n.to_string(); let meta = vec![ - (id(1), now - Duration::from_secs(40), false), - (id(2), now - Duration::from_secs(30), false), - (id(3), now - Duration::from_secs(20), true), - (id(4), now - Duration::from_secs(19), false), - (id(5), now - Duration::from_secs(18), false), - (id(6), now - Duration::from_secs(17), false), - (id(7), now - Duration::from_secs(16), false), - (id(8), now - Duration::from_secs(15), false), - (id(9), now - Duration::from_secs(14), false), - (id(10), now - Duration::from_secs(13), true), + (1, now - Duration::from_secs(40), false), + (2, now - Duration::from_secs(30), false), + (3, now - Duration::from_secs(20), true), + (4, now - Duration::from_secs(19), false), + (5, now - Duration::from_secs(18), false), + (6, now - Duration::from_secs(17), false), + (7, now - Duration::from_secs(16), false), + (8, now - Duration::from_secs(15), false), + (9, now - Duration::from_secs(14), false), + (10, now - Duration::from_secs(13), true), ]; let candidate = UnifiedExecProcessManager::process_id_to_prune_from_meta(&meta); // (10) is exited but among the last 8; we should drop the LRU outside that set. - assert_eq!(candidate, Some(id(1))); + assert_eq!(candidate, Some(1)); } } From 5b10b93ba299283b03b2a2d6142ce54356bc8b26 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Mar 2026 18:42:05 -0700 Subject: [PATCH 6/7] Add realtime start instructions config override (#14270) - add `realtime_start_instructions` config support - thread it into realtime context updates, schema, docs, and tests --- codex-rs/core/config.schema.json | 4 ++ codex-rs/core/src/config/config_tests.rs | 32 ++++++++++ codex-rs/core/src/config/mod.rs | 9 +++ codex-rs/core/src/context_manager/updates.rs | 12 +++- codex-rs/core/tests/suite/compact_remote.rs | 66 ++++++++++++++++++++ codex-rs/protocol/src/models.rs | 7 ++- docs/config.md | 7 +++ 7 files changed, 134 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 9c83c6a8f8..b77ed57193 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1788,6 +1788,10 @@ "experimental_compact_prompt_file": { "$ref": "#/definitions/AbsolutePathBuf" }, + "experimental_realtime_start_instructions": { + "description": "Experimental / do not use. Replaces the built-in realtime start instructions inserted into developer messages when realtime becomes active.", + "type": "string" + }, "experimental_realtime_ws_backend_prompt": { "description": "Experimental / do not use. Overrides only the realtime conversation websocket transport instructions (the `Op::RealtimeConversation` `/ws` session.update instructions) without changing normal prompts.", "type": "string" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index f15ec4dd50..707566eb1c 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -3965,6 +3965,7 @@ fn test_precedence_fixture_with_o3_profile() -> std::io::Result<()> { personality: Some(Personality::Pragmatic), chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), realtime_audio: RealtimeAudioConfig::default(), + experimental_realtime_start_instructions: None, experimental_realtime_ws_base_url: None, experimental_realtime_ws_model: None, experimental_realtime_ws_backend_prompt: None, @@ -4100,6 +4101,7 @@ fn test_precedence_fixture_with_gpt3_profile() -> std::io::Result<()> { personality: Some(Personality::Pragmatic), chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), realtime_audio: RealtimeAudioConfig::default(), + experimental_realtime_start_instructions: None, experimental_realtime_ws_base_url: None, experimental_realtime_ws_model: None, experimental_realtime_ws_backend_prompt: None, @@ -4233,6 +4235,7 @@ fn test_precedence_fixture_with_zdr_profile() -> std::io::Result<()> { personality: Some(Personality::Pragmatic), chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), realtime_audio: RealtimeAudioConfig::default(), + experimental_realtime_start_instructions: None, experimental_realtime_ws_base_url: None, experimental_realtime_ws_model: None, experimental_realtime_ws_backend_prompt: None, @@ -4352,6 +4355,7 @@ fn test_precedence_fixture_with_gpt5_profile() -> std::io::Result<()> { personality: Some(Personality::Pragmatic), chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), realtime_audio: RealtimeAudioConfig::default(), + experimental_realtime_start_instructions: None, experimental_realtime_ws_base_url: None, experimental_realtime_ws_model: None, experimental_realtime_ws_backend_prompt: None, @@ -5261,6 +5265,34 @@ async fn feature_requirements_reject_legacy_aliases() { ); } +#[test] +fn experimental_realtime_start_instructions_load_from_config_toml() -> std::io::Result<()> { + let cfg: ConfigToml = toml::from_str( + r#" +experimental_realtime_start_instructions = "start instructions from config" +"#, + ) + .expect("TOML deserialization should succeed"); + + assert_eq!( + cfg.experimental_realtime_start_instructions.as_deref(), + Some("start instructions from config") + ); + + let codex_home = TempDir::new()?; + let config = Config::load_from_base_config_with_overrides( + cfg, + ConfigOverrides::default(), + codex_home.path().to_path_buf(), + )?; + + assert_eq!( + config.experimental_realtime_start_instructions.as_deref(), + Some("start instructions from config") + ); + Ok(()) +} + #[test] fn experimental_realtime_ws_base_url_loads_from_config_toml() -> std::io::Result<()> { let cfg: ConfigToml = toml::from_str( diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 697f50d7c0..41eaeabb92 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -470,6 +470,10 @@ pub struct Config { /// context appended to websocket session instructions. An empty string /// disables startup context injection entirely. pub experimental_realtime_ws_startup_context: Option, + /// Experimental / do not use. Replaces the built-in realtime start + /// instructions inserted into developer messages when realtime becomes + /// active. + pub experimental_realtime_start_instructions: Option, /// When set, restricts ChatGPT login to a specific workspace identifier. pub forced_chatgpt_workspace_id: Option, @@ -1241,6 +1245,10 @@ pub struct ConfigToml { /// context appended to websocket session instructions. An empty string /// disables startup context injection entirely. pub experimental_realtime_ws_startup_context: Option, + /// Experimental / do not use. Replaces the built-in realtime start + /// instructions inserted into developer messages when realtime becomes + /// active. + pub experimental_realtime_start_instructions: Option, pub projects: Option>, /// Controls the web search tool mode: disabled, cached, or live. @@ -2426,6 +2434,7 @@ impl Config { experimental_realtime_ws_model: cfg.experimental_realtime_ws_model, experimental_realtime_ws_backend_prompt: cfg.experimental_realtime_ws_backend_prompt, experimental_realtime_ws_startup_context: cfg.experimental_realtime_ws_startup_context, + experimental_realtime_start_instructions: cfg.experimental_realtime_start_instructions, forced_chatgpt_workspace_id, forced_login_method, include_apply_patch_tool: include_apply_patch_tool_flag, diff --git a/codex-rs/core/src/context_manager/updates.rs b/codex-rs/core/src/context_manager/updates.rs index 63deb5c808..0d26c551ea 100644 --- a/codex-rs/core/src/context_manager/updates.rs +++ b/codex-rs/core/src/context_manager/updates.rs @@ -75,7 +75,17 @@ pub(crate) fn build_realtime_update_item( next.realtime_active, ) { (Some(true), false) => Some(DeveloperInstructions::realtime_end_message("inactive")), - (Some(false), true) | (None, true) => Some(DeveloperInstructions::realtime_start_message()), + (Some(false), true) | (None, true) => Some( + if let Some(instructions) = next + .config + .experimental_realtime_start_instructions + .as_deref() + { + DeveloperInstructions::realtime_start_message_with_instructions(instructions) + } else { + DeveloperInstructions::realtime_start_message() + }, + ), (Some(true), true) | (Some(false), false) => None, (None, false) => previous_turn_settings .and_then(|settings| settings.realtime_active) diff --git a/codex-rs/core/tests/suite/compact_remote.rs b/codex-rs/core/tests/suite/compact_remote.rs index 9bea953632..b0f28fecc1 100644 --- a/codex-rs/core/tests/suite/compact_remote.rs +++ b/codex-rs/core/tests/suite/compact_remote.rs @@ -161,6 +161,25 @@ fn assert_request_contains_realtime_start(request: &responses::ResponsesRequest) ); } +fn assert_request_contains_custom_realtime_start( + request: &responses::ResponsesRequest, + instructions: &str, +) { + let body = request.body_json().to_string(); + assert!( + body.contains(""), + "expected request to preserve the realtime wrapper" + ); + assert!( + body.contains(instructions), + "expected request to use custom realtime start instructions" + ); + assert!( + !body.contains("Realtime conversation started."), + "expected request to replace the default realtime start instructions" + ); +} + fn assert_request_contains_realtime_end(request: &responses::ResponsesRequest) { let body = request.body_json().to_string(); assert!( @@ -1518,6 +1537,53 @@ async fn snapshot_request_shape_remote_pre_turn_compaction_restates_realtime_sta Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_request_uses_custom_experimental_realtime_start_instructions() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = wiremock::MockServer::start().await; + let realtime_server = start_remote_realtime_server().await; + let custom_instructions = "custom realtime start instructions"; + let mut builder = remote_realtime_test_codex_builder(&realtime_server).with_config({ + let custom_instructions = custom_instructions.to_string(); + move |config| { + config.experimental_realtime_start_instructions = Some(custom_instructions); + } + }); + let test = builder.build(&server).await?; + + let responses_mock = responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_assistant_message("m1", "REMOTE_FIRST_REPLY"), + responses::ev_completed("r1"), + ]), + ) + .await; + + start_realtime_conversation(test.codex.as_ref()).await?; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "USER_ONE".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }) + .await?; + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + assert_request_contains_custom_realtime_start( + &responses_mock.single_request(), + custom_instructions, + ); + + close_realtime_conversation(test.codex.as_ref()).await?; + realtime_server.shutdown().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn snapshot_request_shape_remote_pre_turn_compaction_restates_realtime_end() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 0d50370eb1..fd353b12c4 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -520,9 +520,12 @@ impl DeveloperInstructions { } pub fn realtime_start_message() -> Self { + Self::realtime_start_message_with_instructions(REALTIME_START_INSTRUCTIONS.trim()) + } + + pub fn realtime_start_message_with_instructions(instructions: &str) -> Self { DeveloperInstructions::new(format!( - "{REALTIME_CONVERSATION_OPEN_TAG}\n{}\n{REALTIME_CONVERSATION_CLOSE_TAG}", - REALTIME_START_INSTRUCTIONS.trim() + "{REALTIME_CONVERSATION_OPEN_TAG}\n{instructions}\n{REALTIME_CONVERSATION_CLOSE_TAG}" )) } diff --git a/docs/config.md b/docs/config.md index fc9d62b8e8..a810262a40 100644 --- a/docs/config.md +++ b/docs/config.md @@ -49,4 +49,11 @@ Plan preset. The string value `none` means "no reasoning" (an explicit Plan override), not "inherit the global default". There is currently no separate config value for "follow the global default in Plan mode". +## Realtime start instructions + +`experimental_realtime_start_instructions` lets you replace the built-in +developer message Codex inserts when realtime becomes active. It only affects +the realtime start message in prompt history and does not change websocket +backend prompt settings or the realtime end/inactive message. + Ctrl+C/Ctrl+D quitting uses a ~1 second double-press hint (`ctrl + c again to quit`). From cc417c39a00f81b9c30d26ab45b0726a4887cb5e Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Mar 2026 18:42:50 -0700 Subject: [PATCH 7/7] Split spawn_csv from multi_agent (#14282) - make `spawn_csv` a standalone feature for CSV agent jobs - keep `spawn_csv -> multi_agent` one-way and preserve restricted subagent disable paths --- codex-rs/core/config.schema.json | 6 ++++ codex-rs/core/src/codex.rs | 1 + codex-rs/core/src/features.rs | 32 +++++++++++++++++++ codex-rs/core/src/guardian.rs | 1 + codex-rs/core/src/memories/phase2.rs | 1 + codex-rs/core/src/tasks/review.rs | 1 + .../core/src/tools/handlers/multi_agents.rs | 1 + codex-rs/core/src/tools/spec.rs | 27 ++++++++++++++-- codex-rs/core/tests/suite/agent_jobs.rs | 8 ++--- 9 files changed, 72 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index b77ed57193..338a34e591 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -459,6 +459,9 @@ "skill_mcp_dependency_install": { "type": "boolean" }, + "spawn_csv": { + "type": "boolean" + }, "sqlite": { "type": "boolean" }, @@ -1957,6 +1960,9 @@ "skill_mcp_dependency_install": { "type": "boolean" }, + "spawn_csv": { + "type": "boolean" + }, "sqlite": { "type": "boolean" }, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 4eb66dea64..a21833c8fa 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -385,6 +385,7 @@ impl Codex { if let SessionSource::SubAgent(SubAgentSource::ThreadSpawn { depth, .. }) = session_source && depth >= config.agent_max_depth { + let _ = config.features.disable(Feature::SpawnCsv); let _ = config.features.disable(Feature::Collab); } diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 107b99ad44..7de8b8c7ed 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -138,6 +138,8 @@ pub enum Feature { EnableRequestCompression, /// Enable collab tools. Collab, + /// Enable CSV-backed agent job tools. + SpawnCsv, /// Enable apps. Apps, /// Enable plugins. @@ -414,6 +416,9 @@ impl Features { } pub(crate) fn normalize_dependencies(&mut self) { + if self.enabled(Feature::SpawnCsv) && !self.enabled(Feature::Collab) { + self.enable(Feature::Collab); + } if self.enabled(Feature::JsReplToolsOnly) && !self.enabled(Feature::JsRepl) { tracing::warn!("js_repl_tools_only requires js_repl; disabling js_repl_tools_only"); self.disable(Feature::JsReplToolsOnly); @@ -693,6 +698,12 @@ pub const FEATURES: &[FeatureSpec] = &[ }, default_enabled: false, }, + FeatureSpec { + id: Feature::SpawnCsv, + key: "spawn_csv", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::Apps, key: "apps", @@ -997,6 +1008,27 @@ mod tests { assert_eq!(feature_for_key("collab"), Some(Feature::Collab)); } + #[test] + fn spawn_csv_is_under_development() { + assert_eq!(Feature::SpawnCsv.stage(), Stage::UnderDevelopment); + assert_eq!(Feature::SpawnCsv.default_enabled(), false); + } + + #[test] + fn spawn_csv_normalization_enables_multi_agent_one_way() { + let mut spawn_csv_features = Features::with_defaults(); + spawn_csv_features.enable(Feature::SpawnCsv); + spawn_csv_features.normalize_dependencies(); + assert_eq!(spawn_csv_features.enabled(Feature::SpawnCsv), true); + assert_eq!(spawn_csv_features.enabled(Feature::Collab), true); + + let mut collab_features = Features::with_defaults(); + collab_features.enable(Feature::Collab); + collab_features.normalize_dependencies(); + assert_eq!(collab_features.enabled(Feature::Collab), true); + assert_eq!(collab_features.enabled(Feature::SpawnCsv), false); + } + #[test] fn apps_require_feature_flag_and_chatgpt_auth() { let mut features = Features::with_defaults(); diff --git a/codex-rs/core/src/guardian.rs b/codex-rs/core/src/guardian.rs index d8c5d40e77..8db5af402b 100644 --- a/codex-rs/core/src/guardian.rs +++ b/codex-rs/core/src/guardian.rs @@ -687,6 +687,7 @@ fn build_guardian_subagent_config( )?); } for feature in [ + Feature::SpawnCsv, Feature::Collab, Feature::WebSearchRequest, Feature::WebSearchCached, diff --git a/codex-rs/core/src/memories/phase2.rs b/codex-rs/core/src/memories/phase2.rs index 1a31bb3358..75b29aeff2 100644 --- a/codex-rs/core/src/memories/phase2.rs +++ b/codex-rs/core/src/memories/phase2.rs @@ -270,6 +270,7 @@ mod agent { // Approval policy agent_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); // Consolidation runs as an internal sub-agent and must not recursively delegate. + let _ = agent_config.features.disable(Feature::SpawnCsv); let _ = agent_config.features.disable(Feature::Collab); // Sandbox policy diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index 1146be615d..0a72355b5b 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -100,6 +100,7 @@ async fn start_review_conversation( { panic!("by construction Constrained must always support Disabled: {err}"); } + let _ = sub_agent_config.features.disable(Feature::SpawnCsv); let _ = sub_agent_config.features.disable(Feature::Collab); // Set explicit review rubric for the sub-agent diff --git a/codex-rs/core/src/tools/handlers/multi_agents.rs b/codex-rs/core/src/tools/handlers/multi_agents.rs index 54e146518a..a2d4e39b99 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents.rs @@ -974,6 +974,7 @@ fn apply_spawn_agent_runtime_overrides( fn apply_spawn_agent_overrides(config: &mut Config, child_depth: i32) { if child_depth >= config.agent_max_depth { + let _ = config.features.disable(Feature::SpawnCsv); let _ = config.features.disable(Feature::Collab); } } diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 8f7a25076e..a34ca73151 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -135,6 +135,7 @@ impl ToolsConfig { let include_js_repl_tools_only = include_js_repl && features.enabled(Feature::JsReplToolsOnly); let include_collab_tools = features.enabled(Feature::Collab); + let include_agent_jobs = features.enabled(Feature::SpawnCsv); let include_request_user_input = !matches!(session_source, SessionSource::SubAgent(_)); let include_default_mode_request_user_input = include_request_user_input && features.enabled(Feature::DefaultModeRequestUserInput); @@ -143,7 +144,6 @@ impl ToolsConfig { features.enabled(Feature::Artifact) && codex_artifacts::can_manage_artifact_runtime(); let include_image_gen_tool = features.enabled(Feature::ImageGeneration) && supports_image_generation(model_info); - let include_agent_jobs = include_collab_tools; let request_permission_enabled = features.enabled(Feature::RequestPermissions); let request_permissions_tool_enabled = features.enabled(Feature::RequestPermissionsTool); let shell_command_backend = @@ -2631,6 +2631,28 @@ mod tests { session_source: SessionSource::Cli, }); let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); + assert_contains_tool_names( + &tools, + &["spawn_agent", "send_input", "wait", "close_agent"], + ); + assert_lacks_tool_name(&tools, "spawn_agents_on_csv"); + } + + #[test] + fn test_build_specs_spawn_csv_enables_agent_jobs_and_collab_tools() { + let config = test_config(); + let model_info = + ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); + let mut features = Features::with_defaults(); + features.enable(Feature::SpawnCsv); + features.normalize_dependencies(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + }); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert_contains_tool_names( &tools, &[ @@ -2668,7 +2690,8 @@ mod tests { let model_info = ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); let mut features = Features::with_defaults(); - features.enable(Feature::Collab); + features.enable(Feature::SpawnCsv); + features.normalize_dependencies(); features.enable(Feature::Sqlite); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, diff --git a/codex-rs/core/tests/suite/agent_jobs.rs b/codex-rs/core/tests/suite/agent_jobs.rs index 190302a3e1..443043c6f7 100644 --- a/codex-rs/core/tests/suite/agent_jobs.rs +++ b/codex-rs/core/tests/suite/agent_jobs.rs @@ -224,7 +224,7 @@ async fn report_agent_job_result_rejects_wrong_thread() -> Result<()> { let mut builder = test_codex().with_config(|config| { config .features - .enable(Feature::Collab) + .enable(Feature::SpawnCsv) .expect("test config should allow feature update"); config .features @@ -290,7 +290,7 @@ async fn spawn_agents_on_csv_runs_and_exports() -> Result<()> { let mut builder = test_codex().with_config(|config| { config .features - .enable(Feature::Collab) + .enable(Feature::SpawnCsv) .expect("test config should allow feature update"); config .features @@ -333,7 +333,7 @@ async fn spawn_agents_on_csv_dedupes_item_ids() -> Result<()> { let mut builder = test_codex().with_config(|config| { config .features - .enable(Feature::Collab) + .enable(Feature::SpawnCsv) .expect("test config should allow feature update"); config .features @@ -391,7 +391,7 @@ async fn spawn_agents_on_csv_stop_halts_future_items() -> Result<()> { let mut builder = test_codex().with_config(|config| { config .features - .enable(Feature::Collab) + .enable(Feature::SpawnCsv) .expect("test config should allow feature update"); config .features