From 807ededaa3bb420333fa09327a702aa6004f9cfa Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 10 Jun 2026 17:22:07 -0700 Subject: [PATCH] Simplify goal file materialization --- codex-rs/app-server-client/src/lib.rs | 28 -- codex-rs/app-server-client/src/remote.rs | 90 ++--- codex-rs/tui/src/app/thread_goal_actions.rs | 2 +- codex-rs/tui/src/app_server_session.rs | 95 +---- codex-rs/tui/src/chatwidget/goal_menu.rs | 7 +- .../tui/src/chatwidget/tests/goal_menu.rs | 2 +- .../src/chatwidget/tests/goal_validation.rs | 58 +-- codex-rs/tui/src/goal_display.rs | 25 +- codex-rs/tui/src/goal_files.rs | 362 +++++++++--------- codex-rs/tui/src/goal_files_tests.rs | 187 ++------- 10 files changed, 253 insertions(+), 603 deletions(-) diff --git a/codex-rs/app-server-client/src/lib.rs b/codex-rs/app-server-client/src/lib.rs index f741617cfa..a0f3e60587 100644 --- a/codex-rs/app-server-client/src/lib.rs +++ b/codex-rs/app-server-client/src/lib.rs @@ -38,7 +38,6 @@ use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::InitializeCapabilities; use codex_app_server_protocol::InitializeParams; use codex_app_server_protocol::JSONRPCErrorError; -use codex_app_server_protocol::JSONRPCRequest; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::Result as JsonRpcResult; use codex_app_server_protocol::ServerNotification; @@ -848,16 +847,6 @@ impl AppServerClient { } } - pub async fn request_json_rpc(&self, request: JSONRPCRequest) -> IoResult { - match self { - Self::InProcess(_) => Err(IoError::new( - ErrorKind::InvalidInput, - "raw JSON-RPC requests are only supported by the remote app-server client", - )), - Self::Remote(client) => client.request_json_rpc(request).await, - } - } - pub async fn request_typed(&self, request: ClientRequest) -> Result where T: DeserializeOwned, @@ -911,20 +900,6 @@ impl AppServerClient { } } - pub fn remote_codex_home(&self) -> Option<&str> { - match self { - Self::InProcess(_) => None, - Self::Remote(client) => client.codex_home(), - } - } - - pub fn remote_platform_family(&self) -> Option<&str> { - match self { - Self::InProcess(_) => None, - Self::Remote(client) => client.platform_family(), - } - } - pub fn request_handle(&self) -> AppServerRequestHandle { match self { Self::InProcess(client) => AppServerRequestHandle::InProcess(client.request_handle()), @@ -1131,8 +1106,6 @@ mod tests { result: serde_json::json!({ "userAgent": "codex_cli_rs/9.8.7-test (Test OS; x86_64) rust", "codexHome": "/server/.codex", - "platformFamily": "unix", - "platformOs": "linux", }), }), ) @@ -1470,7 +1443,6 @@ mod tests { assert_eq!(client.server_version(), Some("9.8.7-test")); assert_eq!(client.codex_home(), Some("/server/.codex")); - assert_eq!(client.platform_family(), Some("unix")); let response: GetAccountResponse = client .request_typed(ClientRequest::GetAccount { request_id: RequestId::Integer(1), diff --git a/codex-rs/app-server-client/src/remote.rs b/codex-rs/app-server-client/src/remote.rs index b94cf47379..7f1b1d7091 100644 --- a/codex-rs/app-server-client/src/remote.rs +++ b/codex-rs/app-server-client/src/remote.rs @@ -152,7 +152,6 @@ pub struct RemoteAppServerClient { pending_events: VecDeque, server_version: Option, codex_home: Option, - platform_family: Option, worker_handle: tokio::task::JoinHandle<()>, } @@ -160,7 +159,6 @@ pub struct RemoteAppServerClient { struct RemoteInitializeInfo { server_version: Option, codex_home: Option, - platform_family: Option, } #[derive(Clone)] @@ -198,10 +196,6 @@ impl RemoteAppServerClient { self.codex_home.as_deref() } - pub fn platform_family(&self) -> Option<&str> { - self.platform_family.as_deref() - } - async fn connect_with_stream( channel_capacity: usize, endpoint: String, @@ -235,17 +229,36 @@ impl RemoteAppServerClient { }; match command { RemoteClientCommand::Request { request, response_tx } => { - if !write_remote_request( + let request_id = request.id.clone(); + if pending_requests.contains_key(&request_id) { + let _ = response_tx.send(Err(IoError::new( + ErrorKind::InvalidInput, + format!("duplicate remote app-server request id `{request_id}`"), + ))); + continue; + } + pending_requests.insert(request_id.clone(), response_tx); + if let Err(err) = write_jsonrpc_message( &mut stream, + JSONRPCMessage::Request(*request), &endpoint, - &mut pending_requests, - *request, - response_tx, - &event_tx, - &mut worker_exit_error, ) .await { + let err_message = err.to_string(); + let message = format!( + "remote app server at `{endpoint}` write failed: {err_message}" + ); + if let Some(response_tx) = pending_requests.remove(&request_id) { + let _ = response_tx.send(Err(err)); + } + let _ = deliver_event( + &event_tx, + AppServerEvent::Disconnected { + message: message.clone(), + }, + ); + worker_exit_error = Some((ErrorKind::BrokenPipe, message)); break; } } @@ -471,7 +484,6 @@ impl RemoteAppServerClient { pending_events: pending_events.into(), server_version: initialize_info.server_version, codex_home: initialize_info.codex_home, - platform_family: initialize_info.platform_family, worker_handle, }) } @@ -487,7 +499,7 @@ impl RemoteAppServerClient { .await } - pub async fn request_json_rpc(&self, request: JSONRPCRequest) -> IoResult { + async fn request_json_rpc(&self, request: JSONRPCRequest) -> IoResult { let (response_tx, response_rx) = oneshot::channel(); self.command_tx .send(RemoteClientCommand::Request { @@ -619,7 +631,6 @@ impl RemoteAppServerClient { pending_events: _pending_events, server_version: _server_version, codex_home: _codex_home, - platform_family: _platform_family, worker_handle, } = self; let mut worker_handle = worker_handle; @@ -648,7 +659,7 @@ impl RemoteAppServerRequestHandle { .await } - async fn request_json_rpc(&self, request: JSONRPCRequest) -> IoResult { + pub async fn request_json_rpc(&self, request: JSONRPCRequest) -> IoResult { let (response_tx, response_rx) = oneshot::channel(); self.command_tx .send(RemoteClientCommand::Request { @@ -856,11 +867,6 @@ where .and_then(serde_json::Value::as_str) .filter(|codex_home| !codex_home.is_empty()) .map(str::to_string); - initialize_info.platform_family = response - .result - .get("platformFamily") - .and_then(serde_json::Value::as_str) - .map(str::to_string); break Ok(()); } JSONRPCMessage::Error(error) if error.id == initialize_request_id => { @@ -974,47 +980,6 @@ fn deliver_event( }) } -async fn write_remote_request( - stream: &mut WebSocketStream, - endpoint: &str, - pending_requests: &mut HashMap>>, - request: JSONRPCRequest, - response_tx: oneshot::Sender>, - event_tx: &mpsc::UnboundedSender, - worker_exit_error: &mut Option<(ErrorKind, String)>, -) -> bool -where - S: AsyncRead + AsyncWrite + Unpin, -{ - let request_id = request.id.clone(); - if pending_requests.contains_key(&request_id) { - let _ = response_tx.send(Err(IoError::new( - ErrorKind::InvalidInput, - format!("duplicate remote app-server request id `{request_id}`"), - ))); - return true; - } - pending_requests.insert(request_id.clone(), response_tx); - if let Err(err) = - write_jsonrpc_message(stream, JSONRPCMessage::Request(request), endpoint).await - { - let err_message = err.to_string(); - let message = format!("remote app server at `{endpoint}` write failed: {err_message}"); - if let Some(response_tx) = pending_requests.remove(&request_id) { - let _ = response_tx.send(Err(err)); - } - let _ = deliver_event( - event_tx, - AppServerEvent::Disconnected { - message: message.clone(), - }, - ); - *worker_exit_error = Some((ErrorKind::BrokenPipe, message)); - return false; - } - true -} - fn jsonrpc_request_from_client_request(request: ClientRequest) -> JSONRPCRequest { let value = match serde_json::to_value(request) { Ok(value) => value, @@ -1085,7 +1050,6 @@ mod tests { pending_events: VecDeque::new(), server_version: None, codex_home: None, - platform_family: None, worker_handle, }; diff --git a/codex-rs/tui/src/app/thread_goal_actions.rs b/codex-rs/tui/src/app/thread_goal_actions.rs index a3383726ca..244bddc7f0 100644 --- a/codex-rs/tui/src/app/thread_goal_actions.rs +++ b/codex-rs/tui/src/app/thread_goal_actions.rs @@ -290,7 +290,7 @@ impl App { fn show_replace_thread_goal_confirmation(&mut self, thread_id: ThreadId, objective: String) { let replace_objective = objective.clone(); let subtitle = if let Some(path) = goal_files::objective_file_path(&objective) { - format!("New objective file: {}", path.display()) + format!("New objective file: {path}") } else { format!("New objective: {objective}") }; diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 0884adfffe..982ffaf1a1 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -12,8 +12,6 @@ use crate::session_state::ThreadSessionState; use crate::status::StatusAccountDisplay; use crate::status::plan_type_display_name; use crate::terminal_visualization_instructions::with_terminal_visualization_instructions; -use base64::Engine; -use base64::engine::general_purpose::STANDARD; use codex_app_server_client::AppServerClient; use codex_app_server_client::AppServerEvent; use codex_app_server_client::AppServerRequestHandle; @@ -29,17 +27,10 @@ use codex_app_server_protocol::ExternalAgentConfigDetectResponse; use codex_app_server_protocol::ExternalAgentConfigImportParams; use codex_app_server_protocol::ExternalAgentConfigImportResponse; use codex_app_server_protocol::ExternalAgentConfigMigrationItem; -use codex_app_server_protocol::FsCreateDirectoryParams; -use codex_app_server_protocol::FsCreateDirectoryResponse; -use codex_app_server_protocol::FsReadFileParams; -use codex_app_server_protocol::FsReadFileResponse; -use codex_app_server_protocol::FsWriteFileParams; -use codex_app_server_protocol::FsWriteFileResponse; use codex_app_server_protocol::GetAccountParams; use codex_app_server_protocol::GetAccountRateLimitsResponse; use codex_app_server_protocol::GetAccountResponse; use codex_app_server_protocol::JSONRPCErrorError; -use codex_app_server_protocol::JSONRPCRequest; use codex_app_server_protocol::LogoutAccountResponse; use codex_app_server_protocol::MemoryResetResponse; use codex_app_server_protocol::Model as ApiModel; @@ -135,7 +126,6 @@ use codex_utils_absolute_path::AbsolutePathBuf; use color_eyre::eyre::ContextCompat; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; -use serde::de::DeserializeOwned; use std::collections::HashMap; use std::path::PathBuf; use std::sync::atomic::AtomicBool; @@ -256,11 +246,10 @@ impl AppServerSession { } pub(crate) fn remote_codex_home(&self) -> Option<&str> { - self.client.remote_codex_home() - } - - pub(crate) fn remote_platform_family(&self) -> Option<&str> { - self.client.remote_platform_family() + let AppServerClient::Remote(client) = &self.client else { + return None; + }; + client.codex_home() } pub(crate) fn server_version(&self) -> Option<&str> { @@ -942,82 +931,6 @@ impl AppServerSession { .wrap_err("thread/goal/clear failed in TUI") } - pub(crate) async fn fs_read_file(&mut self, path: AbsolutePathBuf) -> Result> { - let request_id = self.next_request_id(); - let response: FsReadFileResponse = self - .client - .request_typed(ClientRequest::FsReadFile { - request_id, - params: FsReadFileParams { path }, - }) - .await - .wrap_err("fs/readFile failed in TUI")?; - STANDARD - .decode(response.data_base64) - .wrap_err("fs/readFile returned invalid base64 data") - } - - pub(crate) async fn fs_write_file( - &mut self, - path: AbsolutePathBuf, - bytes: Vec, - ) -> Result<()> { - let request_id = self.next_request_id(); - let _: FsWriteFileResponse = self - .client - .request_typed(ClientRequest::FsWriteFile { - request_id, - params: FsWriteFileParams { - path, - data_base64: STANDARD.encode(bytes), - }, - }) - .await - .wrap_err("fs/writeFile failed in TUI")?; - Ok(()) - } - - pub(crate) async fn fs_create_directory(&mut self, path: AbsolutePathBuf) -> Result<()> { - let request_id = self.next_request_id(); - let _: FsCreateDirectoryResponse = self - .client - .request_typed(ClientRequest::FsCreateDirectory { - request_id, - params: FsCreateDirectoryParams { - path, - recursive: Some(true), - }, - }) - .await - .wrap_err("fs/createDirectory failed in TUI")?; - Ok(()) - } - - pub(crate) async fn request_json_rpc_typed( - &mut self, - method: &str, - params: serde_json::Value, - ) -> Result - where - T: DeserializeOwned, - { - let request_id = self.next_request_id(); - let response = self - .client - .request_json_rpc(JSONRPCRequest { - id: request_id, - method: method.to_string(), - params: Some(params), - trace: None, - }) - .await - .wrap_err_with(|| format!("{method} failed in TUI"))?; - let result = response.map_err(|source| { - color_eyre::eyre::eyre!("{method} failed in TUI: {}", source.message) - })?; - serde_json::from_value(result).wrap_err_with(|| format!("{method} returned invalid data")) - } - pub(crate) async fn logout_account(&mut self) -> Result<()> { let request_id = self.next_request_id(); let _: LogoutAccountResponse = self diff --git a/codex-rs/tui/src/chatwidget/goal_menu.rs b/codex-rs/tui/src/chatwidget/goal_menu.rs index 102db1b705..5ddb805280 100644 --- a/codex-rs/tui/src/chatwidget/goal_menu.rs +++ b/codex-rs/tui/src/chatwidget/goal_menu.rs @@ -47,7 +47,7 @@ impl ChatWidget { objective: String, ) { let subtitle = if let Some(path) = goal_files::objective_file_path(&objective) { - format!("Goal file: {}", path.display()) + format!("Goal file: {path}") } else { format!("Goal: {objective}") }; @@ -131,10 +131,7 @@ fn goal_summary_lines(goal: &AppThreadGoal) -> Vec> { fn goal_objective_line(objective: &str) -> Line<'static> { if let Some(path) = goal_files::objective_file_path(objective) { - Line::from(vec![ - "Objective file: ".dim(), - path.display().to_string().into(), - ]) + Line::from(vec!["Objective file: ".dim(), path.into()]) } else { Line::from(vec!["Objective: ".dim(), objective.to_string().into()]) } diff --git a/codex-rs/tui/src/chatwidget/tests/goal_menu.rs b/codex-rs/tui/src/chatwidget/tests/goal_menu.rs index ceba892392..b391075d65 100644 --- a/codex-rs/tui/src/chatwidget/tests/goal_menu.rs +++ b/codex-rs/tui/src/chatwidget/tests/goal_menu.rs @@ -84,7 +84,7 @@ async fn goal_menu_managed_file_snapshot() { .join("attachments") .join("00000000-0000-4000-8000-000000000000") .join("goal-objective.md"); - let goal_path = crate::goal_files::GoalFilePath::from_local(&path); + let goal_path = path.display().to_string(); goal.objective = crate::goal_files::objective_file_reference(&goal_path) .expect("goal objective file reference"); diff --git a/codex-rs/tui/src/chatwidget/tests/goal_validation.rs b/codex-rs/tui/src/chatwidget/tests/goal_validation.rs index 5d2c1241f7..d4644fdf54 100644 --- a/codex-rs/tui/src/chatwidget/tests/goal_validation.rs +++ b/codex-rs/tui/src/chatwidget/tests/goal_validation.rs @@ -14,12 +14,6 @@ fn complete_turn_with_message(chat: &mut ChatWidget, turn_id: &str, message: Opt handle_turn_completed(chat, turn_id, /*duration_ms*/ None); } -fn submit_composer_text(chat: &mut ChatWidget, text: &str) { - chat.bottom_pane - .set_composer_text(text.to_string(), Vec::new(), Vec::new()); - submit_current_composer(chat); -} - fn submit_current_composer(chat: &mut ChatWidget) { chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); @@ -48,46 +42,6 @@ fn next_goal_draft( } } -#[test] -fn sentinel_like_objective_is_plain_text() { - let objective = concat!( - "Codex goal objective file: ", - "/tmp/attachments/00000000-0000-4000-8000-000000000000/goal-objective.md\n", - "Read that file before continuing." - ); - - assert_eq!(crate::goal_files::objective_file_path(objective), None); -} - -#[tokio::test] -async fn goal_slash_command_accepts_objective_at_limit() { - let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; - chat.set_feature_enabled(Feature::Goals, /*enabled*/ true); - let thread_id = ThreadId::new(); - chat.thread_id = Some(thread_id); - let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS); - let command = format!("/goal {objective}"); - - submit_composer_text(&mut chat, &command); - - assert_eq!(next_goal_draft(&mut rx, thread_id).objective, objective); - assert_no_submit_op(&mut op_rx); -} - -#[tokio::test] -async fn goal_slash_command_accepts_multiline_objective_after_blank_first_line() { - let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; - chat.set_feature_enabled(Feature::Goals, /*enabled*/ true); - let thread_id = ThreadId::new(); - chat.thread_id = Some(thread_id); - let objective = "follow these instructions\npreserve this detail"; - - submit_composer_text(&mut chat, &format!("/goal \n\n{objective}")); - - assert_eq!(next_goal_draft(&mut rx, thread_id).objective, objective); - assert_no_submit_op(&mut op_rx); -} - #[tokio::test] async fn goal_slash_command_emits_only_inserted_paste_text_element() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; @@ -154,16 +108,8 @@ async fn queued_goal_slash_command_emits_oversized_objective_and_stops_queue() { complete_turn_with_message(&mut chat, "turn-1", Some("done")); - let (actual_thread_id, actual_objective) = loop { - match rx.try_recv().expect("expected goal objective event") { - AppEvent::SetThreadGoalDraft { - thread_id, draft, .. - } => break (thread_id, draft.objective), - _ => continue, - } - }; - assert_eq!(actual_thread_id, thread_id); - assert_eq!(actual_objective, objective); + let draft = next_goal_draft(&mut rx, thread_id); + assert_eq!(draft.objective, objective); assert_eq!(chat.input_queue.queued_user_messages.len(), 1); assert_no_submit_op(&mut op_rx); } diff --git a/codex-rs/tui/src/goal_display.rs b/codex-rs/tui/src/goal_display.rs index 2f1a16d76b..7b77695e28 100644 --- a/codex-rs/tui/src/goal_display.rs +++ b/codex-rs/tui/src/goal_display.rs @@ -43,7 +43,7 @@ pub(crate) fn goal_status_label(status: ThreadGoalStatus) -> &'static str { pub(crate) fn goal_usage_summary(goal: &ThreadGoal) -> String { let objective = if let Some(path) = crate::goal_files::objective_file_path(&goal.objective) { - format!("Objective file: {}", path.display()) + format!("Objective file: {path}") } else { format!("Objective: {}", goal.objective) }; @@ -113,27 +113,4 @@ mod tests { "Objective: Complete the task described in ../gameboy-long-running-prompt5.txt Time: 2m. Tokens: 63.9K/50K." ); } - - #[test] - fn goal_usage_summary_formats_managed_file_objective() { - let temp_dir = tempfile::tempdir().expect("tempdir"); - let path = codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path_checked( - temp_dir - .path() - .join("attachments") - .join("00000000-0000-4000-8000-000000000000") - .join("goal-objective.md"), - ) - .expect("absolute path"); - let path_for_goal = crate::goal_files::GoalFilePath::from_local(&path); - let objective = crate::goal_files::objective_file_reference(&path_for_goal) - .expect("goal file reference"); - let mut goal = test_thread_goal(/*token_budget*/ None, /*tokens_used*/ 0); - goal.objective = objective; - - assert_eq!( - goal_usage_summary(&goal), - format!("Objective file: {} Time: 2m.", path.display()) - ); - } } diff --git a/codex-rs/tui/src/goal_files.rs b/codex-rs/tui/src/goal_files.rs index aecb6bab5b..17ddbb99fb 100644 --- a/codex-rs/tui/src/goal_files.rs +++ b/codex-rs/tui/src/goal_files.rs @@ -15,12 +15,20 @@ use anyhow::Result; use anyhow::bail; use base64::Engine; use base64::engine::general_purpose::STANDARD; +use codex_app_server_client::AppServerRequestHandle; +use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::FsCreateDirectoryParams; use codex_app_server_protocol::FsCreateDirectoryResponse; +use codex_app_server_protocol::FsReadFileParams; use codex_app_server_protocol::FsReadFileResponse; +use codex_app_server_protocol::FsWriteFileParams; use codex_app_server_protocol::FsWriteFileResponse; +use codex_app_server_protocol::JSONRPCRequest; +use codex_app_server_protocol::RequestId; use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS; use codex_protocol::user_input::TextElement; use codex_utils_absolute_path::AbsolutePathBuf; +use serde::de::DeserializeOwned; use serde_json::json; use uuid::Uuid; @@ -60,167 +68,166 @@ pub(crate) trait GoalFileStore { ) -> impl std::future::Future>> + Send; } -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) struct GoalFilePath { - raw: String, - separator: char, -} - -impl GoalFilePath { - pub(crate) fn from_local(path: &AbsolutePathBuf) -> Self { - Self { - raw: path.display().to_string(), - separator: std::path::MAIN_SEPARATOR, - } - } - - pub(crate) fn from_remote(path: &str, platform_family: Option<&str>) -> Self { - Self { - raw: path.to_string(), - separator: if platform_family == Some("windows") || is_windows_absolute_path(path) { - '\\' - } else { - '/' - }, - } - } - - pub(crate) fn as_str(&self) -> &str { - &self.raw - } - - pub(crate) fn display(&self) -> &str { - &self.raw - } - - fn join(&self, segment: impl AsRef) -> Self { - let segment = segment.as_ref(); - let trimmed = self.raw.trim_end_matches(['/', '\\']); - let mut raw = if trimmed.is_empty() { - self.separator.to_string() - } else { - trimmed.to_string() - }; - if !raw.ends_with(self.separator) { - raw.push(self.separator); - } - raw.push_str(segment); - Self { - raw, - separator: self.separator, - } - } - - fn from_managed_reference(raw: &str) -> Option { - let separator = if is_windows_absolute_path(raw) { - '\\' - } else if raw.starts_with('/') { - '/' - } else { - return None; - }; - let normalized = raw.replace('\\', "/"); - let parts = normalized - .split('/') - .filter(|part| !part.is_empty()) - .collect::>(); - if parts.len() < 3 { - return None; - } - let file_name = parts.last()?; - let attachment_id = parts.get(parts.len() - 2)?; - let attachment_dir = parts.get(parts.len() - 3)?; - if *file_name == GOAL_FILE_NAME - && *attachment_dir == GOAL_ATTACHMENT_DIR - && Uuid::parse_str(attachment_id).is_ok() - { - Some(Self { - raw: raw.to_string(), - separator, - }) - } else { - None - } - } -} +/// Path syntax for goal files has to match the app-server host, not the TUI +/// host, because remote fs APIs deserialize and resolve paths on the server. +pub(crate) type GoalFilePath = String; impl GoalFileStore for AppServerSession { async fn create_directory(&mut self, path: GoalFilePath) -> Result<()> { - if !self.uses_remote_workspace() { - return self - .fs_create_directory(local_goal_file_path(&path)?) - .await - .map_err(|err| anyhow::anyhow!("{err}")); - } - - let _: FsCreateDirectoryResponse = self - .request_json_rpc_typed( - "fs/createDirectory", - json!({ - "path": path.as_str(), - "recursive": true, - }), - ) - .await - .map_err(|err| anyhow::anyhow!("{err}"))?; + let _: FsCreateDirectoryResponse = request_goal_fs( + self, + "fs/createDirectory", + ClientRequest::FsCreateDirectory { + request_id: goal_request_id(), + params: FsCreateDirectoryParams { + path: local_goal_file_path(&path)?, + recursive: Some(true), + }, + }, + json!({ + "path": path, + "recursive": true, + }), + ) + .await?; Ok(()) } async fn write_file(&mut self, path: GoalFilePath, bytes: Vec) -> Result<()> { - if !self.uses_remote_workspace() { - return self - .fs_write_file(local_goal_file_path(&path)?, bytes) - .await - .map_err(|err| anyhow::anyhow!("{err}")); - } - - let _: FsWriteFileResponse = self - .request_json_rpc_typed( - "fs/writeFile", - json!({ - "path": path.as_str(), - "dataBase64": STANDARD.encode(bytes), - }), - ) - .await - .map_err(|err| anyhow::anyhow!("{err}"))?; + let data_base64 = STANDARD.encode(bytes); + let _: FsWriteFileResponse = request_goal_fs( + self, + "fs/writeFile", + ClientRequest::FsWriteFile { + request_id: goal_request_id(), + params: FsWriteFileParams { + path: local_goal_file_path(&path)?, + data_base64: data_base64.clone(), + }, + }, + json!({ + "path": path, + "dataBase64": data_base64, + }), + ) + .await?; Ok(()) } async fn read_file(&mut self, path: GoalFilePath) -> Result> { - if !self.uses_remote_workspace() { - return self - .fs_read_file(local_goal_file_path(&path)?) - .await - .map_err(|err| anyhow::anyhow!("{err}")); - } - - let response: FsReadFileResponse = self - .request_json_rpc_typed("fs/readFile", json!({ "path": path.as_str() })) - .await - .map_err(|err| anyhow::anyhow!("{err}"))?; + let response: FsReadFileResponse = request_goal_fs( + self, + "fs/readFile", + ClientRequest::FsReadFile { + request_id: goal_request_id(), + params: FsReadFileParams { + path: local_goal_file_path(&path)?, + }, + }, + json!({ "path": path }), + ) + .await?; STANDARD .decode(response.data_base64) .context("fs/readFile returned invalid base64 data") } } +async fn request_goal_fs( + app_server: &AppServerSession, + method: &str, + local_request: ClientRequest, + remote_params: serde_json::Value, +) -> Result +where + T: DeserializeOwned, +{ + if app_server.uses_remote_workspace() { + return remote_json_rpc_typed(app_server, method, remote_params).await; + } + app_server + .request_handle() + .request_typed(local_request) + .await + .with_context(|| format!("{method} failed in TUI")) +} + +async fn remote_json_rpc_typed( + app_server: &AppServerSession, + method: &str, + params: serde_json::Value, +) -> Result +where + T: DeserializeOwned, +{ + let AppServerRequestHandle::Remote(handle) = app_server.request_handle() else { + bail!("raw JSON-RPC requests are only supported by the remote app-server client"); + }; + let response = handle + .request_json_rpc(JSONRPCRequest { + id: goal_request_id(), + method: method.to_string(), + params: Some(params), + trace: None, + }) + .await + .with_context(|| format!("{method} failed in TUI"))?; + let result = + response.map_err(|source| anyhow::anyhow!("{method} failed in TUI: {}", source.message))?; + serde_json::from_value(result).with_context(|| format!("{method} returned invalid data")) +} + +fn goal_request_id() -> RequestId { + RequestId::String(format!("goal-files-{}", Uuid::new_v4())) +} + pub(crate) fn codex_home_for_app_server( app_server: &AppServerSession, local_codex_home: &AbsolutePathBuf, ) -> Option { if app_server.uses_remote_workspace() { - app_server - .remote_codex_home() - .map(|path| GoalFilePath::from_remote(path, app_server.remote_platform_family())) + app_server.remote_codex_home().map(str::to_string) } else { - Some(GoalFilePath::from_local(local_codex_home)) + Some(local_codex_home.display().to_string()) } } fn local_goal_file_path(path: &GoalFilePath) -> Result { - AbsolutePathBuf::from_absolute_path_checked(path.as_str()) - .with_context(|| format!("invalid local goal file path {}", path.display())) + AbsolutePathBuf::from_absolute_path_checked(path) + .with_context(|| format!("invalid local goal file path {path}")) +} + +fn join_goal_path(path: &str, segment: impl AsRef) -> GoalFilePath { + let separator = if is_windows_absolute_path(path) { + '\\' + } else { + '/' + }; + let mut path = path.trim_end_matches(['/', '\\']).to_string(); + if !path.ends_with(separator) { + path.push(separator); + } + path.push_str(segment.as_ref()); + path +} + +fn managed_goal_file_path(raw: &str) -> Option { + if !is_windows_absolute_path(raw) && !raw.starts_with('/') { + return None; + } + let normalized = raw.replace('\\', "/"); + let mut parts = normalized.rsplit('/').filter(|part| !part.is_empty()); + let file_name = parts.next()?; + let attachment_id = parts.next()?; + let attachment_dir = parts.next()?; + if file_name == GOAL_FILE_NAME + && attachment_dir == GOAL_ATTACHMENT_DIR + && Uuid::parse_str(attachment_id).is_ok() + { + Some(raw.to_string()) + } else { + None + } } pub(crate) async fn materialize_goal_draft( @@ -233,55 +240,39 @@ pub(crate) async fn materialize_goal_draft( bail!("Goal objective must not be empty."); } let text_elements = draft.text_elements; - let (validation_objective, _) = ChatComposer::expand_pending_pastes( - &objective, - text_elements.clone(), - &draft.pending_pastes, - ); - if validation_objective.trim().is_empty() { - bail!("Goal objective must not be empty."); - } let mut output_dir = None; - let mut materialized_pastes = Vec::new(); + let mut replacements = Vec::new(); for (idx, (placeholder, text)) in draft.pending_pastes.iter().enumerate() { - let path = ensure_output_dir(store, codex_home, &mut output_dir) - .await? - .join(format!("pasted-text-{}.txt", idx + 1)); + let path = join_goal_path( + &ensure_output_dir(store, codex_home, &mut output_dir).await?, + format!("pasted-text-{}.txt", idx + 1), + ); write_file(store, path.clone(), text.as_bytes().to_vec()).await?; if !placeholder.is_empty() { - materialized_pastes.push(( - placeholder.clone(), - format!("pasted text file: {}", path.display()), - )); + replacements.push((placeholder.clone(), format!("pasted text file: {path}"))); } } - let (expanded_objective, text_elements) = - ChatComposer::expand_pending_pastes(&objective, text_elements, &materialized_pastes); - objective = expanded_objective; let mut image_lines = Vec::new(); - let mut materialized_images = Vec::new(); for (idx, image) in draft.local_images.iter().enumerate() { let extension = image_extension(&image.path); - let path = ensure_output_dir(store, codex_home, &mut output_dir) - .await? - .join(format!("image-{}.{}", idx + 1, extension)); + let path = join_goal_path( + &ensure_output_dir(store, codex_home, &mut output_dir).await?, + format!("image-{}.{}", idx + 1, extension), + ); let bytes = fs::read(&image.path) .with_context(|| format!("Could not read goal image {}", image.path.display()))?; write_file(store, path.clone(), bytes).await?; if image.placeholder.is_empty() { - image_lines.push(format!("- [Image #{}]: {}", idx + 1, path.display())); + image_lines.push(format!("- [Image #{}]: {path}", idx + 1)); } else { - materialized_images.push(( - image.placeholder.clone(), - format!("image file: {}", path.display()), - )); + replacements.push((image.placeholder.clone(), format!("image file: {path}"))); } } let (expanded_objective, _) = - ChatComposer::expand_pending_pastes(&objective, text_elements, &materialized_images); + ChatComposer::expand_pending_pastes(&objective, text_elements, &replacements); objective = expanded_objective.trim().to_string(); append_section(&mut objective, "Referenced image files:", image_lines); @@ -296,9 +287,10 @@ pub(crate) async fn materialize_goal_draft( ); if objective.chars().count() > MAX_THREAD_GOAL_OBJECTIVE_CHARS { - let path = ensure_output_dir(store, codex_home, &mut output_dir) - .await? - .join(GOAL_FILE_NAME); + let path = join_goal_path( + &ensure_output_dir(store, codex_home, &mut output_dir).await?, + GOAL_FILE_NAME, + ); write_file(store, path.clone(), objective.as_bytes().to_vec()).await?; objective = objective_file_reference(&path)?; } @@ -316,9 +308,9 @@ pub(crate) async fn objective_text_for_edit( let bytes = store .read_file(path.clone()) .await - .with_context(|| format!("Could not read goal objective file {}", path.display()))?; + .with_context(|| format!("Could not read goal objective file {path}"))?; String::from_utf8(bytes) - .with_context(|| format!("Goal objective file {} is not valid UTF-8", path.display())) + .with_context(|| format!("Goal objective file {path} is not valid UTF-8")) } pub(crate) fn objective_file_path(objective: &str) -> Option { @@ -328,7 +320,7 @@ pub(crate) fn objective_file_path(objective: &str) -> Option { .strip_prefix(GOAL_FILE_PREFIX) .map(str::trim) .filter(|path| !path.is_empty()) - .and_then(GoalFilePath::from_managed_reference)?; + .and_then(managed_goal_file_path)?; if lines.next() != Some(GOAL_FILE_INSTRUCTION) { return None; } @@ -337,10 +329,7 @@ pub(crate) fn objective_file_path(objective: &str) -> Option { } pub(crate) fn objective_file_reference(path: &GoalFilePath) -> Result { - let reference = format!( - "{GOAL_FILE_PREFIX}{}\n{GOAL_FILE_INSTRUCTION}", - path.display() - ); + let reference = format!("{GOAL_FILE_PREFIX}{path}\n{GOAL_FILE_INSTRUCTION}"); let actual_chars = reference.chars().count(); if actual_chars > MAX_THREAD_GOAL_OBJECTIVE_CHARS { bail!( @@ -360,18 +349,14 @@ async fn ensure_output_dir( } let codex_home = codex_home .context("App server did not report $CODEX_HOME; cannot materialize goal files")?; - let path = codex_home - .join(GOAL_ATTACHMENT_DIR) - .join(Uuid::new_v4().to_string()); + let path = join_goal_path( + &join_goal_path(codex_home, GOAL_ATTACHMENT_DIR), + Uuid::new_v4().to_string(), + ); store .create_directory(path.clone()) .await - .with_context(|| { - format!( - "Could not create goal attachment directory {}", - path.display() - ) - })?; + .with_context(|| format!("Could not create goal attachment directory {path}"))?; *output_dir = Some(path.clone()); Ok(path) } @@ -384,7 +369,7 @@ async fn write_file( store .write_file(path.clone(), bytes) .await - .with_context(|| format!("Could not write goal file {}", path.display())) + .with_context(|| format!("Could not write goal file {path}")) } fn is_windows_absolute_path(path: &str) -> bool { @@ -409,11 +394,18 @@ fn append_section(objective: &mut String, heading: &str, lines: Vec) { objective.push_str(&lines.join("\n")); } -fn image_extension(path: &Path) -> &str { +fn image_extension(path: &Path) -> String { path.extension() .and_then(|extension| extension.to_str()) + .map(|extension| { + extension + .chars() + .filter(char::is_ascii_alphanumeric) + .take(8) + .collect::() + }) .filter(|extension| !extension.is_empty()) - .unwrap_or("png") + .unwrap_or_else(|| "png".to_string()) } #[cfg(test)] diff --git a/codex-rs/tui/src/goal_files_tests.rs b/codex-rs/tui/src/goal_files_tests.rs index 3985328b2f..e5cec75452 100644 --- a/codex-rs/tui/src/goal_files_tests.rs +++ b/codex-rs/tui/src/goal_files_tests.rs @@ -3,40 +3,19 @@ use super::*; use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS; use codex_protocol::user_input::TextElement; use pretty_assertions::assert_eq; -use std::path::Path; - -struct LocalStore; - -impl GoalFileStore for LocalStore { - async fn create_directory(&mut self, path: GoalFilePath) -> Result<()> { - fs::create_dir_all(path.as_str())?; - Ok(()) - } - - async fn write_file(&mut self, path: GoalFilePath, bytes: Vec) -> Result<()> { - fs::write(path.as_str(), bytes)?; - Ok(()) - } - - async fn read_file(&mut self, path: GoalFilePath) -> Result> { - Ok(fs::read(path.as_str())?) - } -} #[derive(Default)] struct RecordingStore { - created_dirs: Vec, writes: Vec<(String, Vec)>, } impl GoalFileStore for RecordingStore { - async fn create_directory(&mut self, path: GoalFilePath) -> Result<()> { - self.created_dirs.push(path.as_str().to_string()); + async fn create_directory(&mut self, _path: GoalFilePath) -> Result<()> { Ok(()) } async fn write_file(&mut self, path: GoalFilePath, bytes: Vec) -> Result<()> { - self.writes.push((path.as_str().to_string(), bytes)); + self.writes.push((path, bytes)); Ok(()) } @@ -45,71 +24,40 @@ impl GoalFileStore for RecordingStore { .iter() .find(|(write_path, _)| write_path == path.as_str()) .map(|(_, bytes)| bytes.clone()) - .ok_or_else(|| anyhow::anyhow!("missing recording for {}", path.display())) + .ok_or_else(|| anyhow::anyhow!("missing recording for {path}")) } } #[tokio::test] -async fn materializes_and_reads_oversized_objective_through_store() { +async fn materializes_oversized_objective_with_remote_windows_path() { let temp_dir = tempfile::tempdir().expect("tempdir"); - let codex_home = local_goal_home(temp_dir.path()); - let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1); - let mut store = LocalStore; + let image_path = temp_dir.path().join("local-image.png"); + fs::write(&image_path, b"png bytes").expect("write image"); + let paste_placeholder = "[Pasted Content 5 chars]"; + let image_placeholder = "[Image #1]"; + let objective = format!( + "Use {paste_placeholder} and {image_placeholder}. {}", + "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1) + ); + let text_elements = [paste_placeholder, image_placeholder] + .into_iter() + .map(|placeholder| { + let start = objective.find(placeholder).expect("placeholder"); + TextElement::new( + (start..start + placeholder.len()).into(), + Some(placeholder.to_string()), + ) + }) + .collect(); + let codex_home = r"C:\Users\codex\.codex".to_string(); + let mut store = RecordingStore::default(); let reference = materialize_goal_draft( &mut store, Some(&codex_home), GoalDraft { objective: objective.clone(), - ..Default::default() - }, - ) - .await - .expect("materialize goal draft"); - - let path = objective_file_path(&reference).expect("goal file path"); - assert_eq!( - fs::read_to_string(path.as_str()).expect("read file"), - objective - ); - let edit_text = objective_text_for_edit(&mut store, &reference) - .await - .expect("read objective text"); - assert_eq!(edit_text, objective); -} - -#[tokio::test] -async fn materializes_paste_and_image_through_store() { - let temp_dir = tempfile::tempdir().expect("tempdir"); - let codex_home = local_goal_home(temp_dir.path()); - let image_path = temp_dir.path().join("local-image.png"); - fs::write(&image_path, b"png bytes").expect("write image"); - let mut store = LocalStore; - let objective = "Use [Pasted Content 5 chars] and [Image #1]".to_string(); - let paste_placeholder = "[Pasted Content 5 chars]"; - let image_placeholder = "[Image #1]"; - let paste_start = objective - .find(paste_placeholder) - .expect("paste placeholder"); - let image_start = objective - .find(image_placeholder) - .expect("image placeholder"); - - let objective = materialize_goal_draft( - &mut store, - Some(&codex_home), - GoalDraft { - objective, - text_elements: vec![ - TextElement::new( - (paste_start..paste_start + paste_placeholder.len()).into(), - Some(paste_placeholder.to_string()), - ), - TextElement::new( - (image_start..image_start + image_placeholder.len()).into(), - Some(image_placeholder.to_string()), - ), - ], + text_elements, pending_pastes: vec![(paste_placeholder.to_string(), "hello".to_string())], local_images: vec![LocalImageAttachment { placeholder: image_placeholder.to_string(), @@ -121,46 +69,24 @@ async fn materializes_paste_and_image_through_store() { .await .expect("materialize goal draft"); - let paste_path = path_after(&objective, "pasted text file: "); - let image_path = path_after(&objective, "image file: "); - assert_eq!(fs::read_to_string(paste_path).expect("read paste"), "hello"); - assert_eq!(fs::read(image_path).expect("read image"), b"png bytes"); -} - -#[tokio::test] -async fn materializes_oversized_objective_with_windows_remote_path() { - let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1); - let codex_home = GoalFilePath::from_remote(r"C:\Users\codex\.codex", Some("windows")); - let mut store = RecordingStore::default(); - - let reference = materialize_goal_draft( - &mut store, - Some(&codex_home), - GoalDraft { - objective: objective.clone(), - ..Default::default() - }, - ) - .await - .expect("materialize goal draft"); - - assert_eq!(store.created_dirs.len(), 1); - assert_eq!(store.writes.len(), 1); - let (path, bytes) = &store.writes[0]; - assert!(path.starts_with(r"C:\Users\codex\.codex\attachments\")); - assert!(path.ends_with(r"\goal-objective.md")); - assert_eq!(bytes, objective.as_bytes()); - assert_eq!( - objective_file_path(&reference) - .expect("goal file path") - .as_str(), - path + let path = objective_file_path(&reference).expect("goal file path"); + assert!( + path.as_str() + .starts_with(r"C:\Users\codex\.codex\attachments\") ); + assert!(path.as_str().ends_with(r"\goal-objective.md")); + let edit_text = objective_text_for_edit(&mut store, &reference) + .await + .expect("read objective text"); + assert!(edit_text.contains(r"pasted text file: C:\Users\codex\.codex\attachments\")); + assert!(edit_text.contains(r"image file: C:\Users\codex\.codex\attachments\")); + assert!(store.writes.iter().any(|(_, bytes)| bytes == b"hello")); + assert!(store.writes.iter().any(|(_, bytes)| bytes == b"png bytes")); } #[tokio::test] async fn plain_objective_does_not_need_codex_home() { - let mut store = LocalStore; + let mut store = RecordingStore::default(); let objective = materialize_goal_draft( &mut store, @@ -175,40 +101,3 @@ async fn plain_objective_does_not_need_codex_home() { assert_eq!(objective, "read src/lib.rs"); } - -#[tokio::test] -async fn oversized_objective_requires_codex_home() { - let mut store = LocalStore; - - let err = materialize_goal_draft( - &mut store, - /*codex_home*/ None, - GoalDraft { - objective: "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1), - ..Default::default() - }, - ) - .await - .expect_err("oversized objective should require codex home"); - - assert!( - err.to_string().contains("$CODEX_HOME"), - "expected codex home error, got {err:#}" - ); -} - -fn path_after(text: &str, prefix: &str) -> String { - let path = text - .split_once(prefix) - .unwrap_or_else(|| panic!("expected {prefix:?} in {text:?}")) - .1 - .split_whitespace() - .next() - .expect("path"); - path.to_string() -} - -fn local_goal_home(path: &Path) -> GoalFilePath { - let path = AbsolutePathBuf::from_absolute_path_checked(path).expect("absolute codex home"); - GoalFilePath::from_local(&path) -}