Address goal materialization review feedback

codex-rs/tui/src/app/tests.rs

@@ -88,6 +88,7 @@ use codex_protocol::models::ActivePermissionProfile;
 use codex_protocol::models::FileSystemPermissions;
 use codex_protocol::models::NetworkPermissions;
 use codex_protocol::models::PermissionProfile;
+use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS;
 use codex_protocol::request_permissions::RequestPermissionProfile;
 use codex_protocol::user_input::TextElement;
 use codex_utils_absolute_path::AbsolutePathBuf;
@@ -4148,6 +4149,51 @@ async fn make_test_app_with_channels() -> (
     )
 }
 
+#[tokio::test]
+async fn set_thread_goal_draft_materializes_long_objective_before_goal_set() -> Result<()> {
+    let mut app = make_test_app().await;
+    let mut app_server = crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref())
+        .await
+        .expect("embedded app server");
+    let started = app_server
+        .start_thread(app.chat_widget.config_ref())
+        .await
+        .expect("thread/start should succeed");
+    let thread_id = started.session.thread_id;
+    app.enqueue_primary_thread_session(started.session, started.turns)
+        .await
+        .expect("primary thread should be registered");
+    let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1);
+
+    app.set_thread_goal_draft(
+        &mut app_server,
+        thread_id,
+        crate::goal_files::GoalDraft {
+            objective: objective.clone(),
+            ..Default::default()
+        },
+        crate::app_event::ThreadGoalSetMode::ConfirmIfExists,
+    )
+    .await;
+
+    let goal = app_server
+        .thread_goal_get(thread_id)
+        .await?
+        .goal
+        .expect("goal should be set");
+    let path = crate::goal_files::objective_file_path(&goal.objective)
+        .expect("goal objective should be a managed file reference");
+    assert!(path.contains("attachments"));
+    assert_eq!(
+        crate::goal_files::objective_text_for_edit(&mut app_server, &goal.objective)
+            .await
+            .expect("managed goal file should be readable"),
+        objective
+    );
+    app_server.shutdown().await?;
+    Ok(())
+}
+
 fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState {
     ThreadSessionState {
         thread_id,

codex-rs/tui/src/app_server_session.rs

@@ -12,6 +12,8 @@ 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;
@@ -27,10 +29,17 @@ 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;
@@ -126,6 +135,8 @@ 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 serde_json::json;
 use std::collections::HashMap;
 use std::path::PathBuf;
 use std::sync::atomic::AtomicBool;
@@ -931,6 +942,123 @@ impl AppServerSession {
             .wrap_err("thread/goal/clear failed in TUI")
     }
 
+    pub(crate) async fn fs_create_directory_path(
+        &mut self,
+        path: &str,
+        recursive: bool,
+    ) -> Result<()> {
+        let _: FsCreateDirectoryResponse = self
+            .request_fs_path(
+                "fs/createDirectory",
+                path,
+                |request_id, path| ClientRequest::FsCreateDirectory {
+                    request_id,
+                    params: FsCreateDirectoryParams {
+                        path,
+                        recursive: Some(recursive),
+                    },
+                },
+                json!({
+                    "path": path,
+                    "recursive": recursive,
+                }),
+            )
+            .await?;
+        Ok(())
+    }
+
+    pub(crate) async fn fs_write_file_path(&mut self, path: &str, bytes: Vec<u8>) -> Result<()> {
+        let data_base64 = STANDARD.encode(bytes);
+        let _: FsWriteFileResponse = self
+            .request_fs_path(
+                "fs/writeFile",
+                path,
+                |request_id, path| ClientRequest::FsWriteFile {
+                    request_id,
+                    params: FsWriteFileParams {
+                        path,
+                        data_base64: data_base64.clone(),
+                    },
+                },
+                json!({
+                    "path": path,
+                    "dataBase64": data_base64,
+                }),
+            )
+            .await?;
+        Ok(())
+    }
+
+    pub(crate) async fn fs_read_file_path(&mut self, path: &str) -> Result<Vec<u8>> {
+        let response: FsReadFileResponse = self
+            .request_fs_path(
+                "fs/readFile",
+                path,
+                |request_id, path| ClientRequest::FsReadFile {
+                    request_id,
+                    params: FsReadFileParams { path },
+                },
+                json!({ "path": path }),
+            )
+            .await?;
+        STANDARD
+            .decode(response.data_base64)
+            .wrap_err("fs/readFile returned invalid base64 data")
+    }
+
+    async fn request_fs_path<T>(
+        &mut self,
+        method: &str,
+        path: &str,
+        local_request: impl FnOnce(RequestId, AbsolutePathBuf) -> ClientRequest,
+        remote_params: serde_json::Value,
+    ) -> Result<T>
+    where
+        T: DeserializeOwned,
+    {
+        let request_id = self.next_request_id();
+        if self.uses_remote_workspace() {
+            return self
+                .request_remote_json_rpc_typed(request_id, method, remote_params)
+                .await;
+        }
+        let path = AbsolutePathBuf::from_absolute_path_checked(path)
+            .wrap_err_with(|| format!("invalid local app-server fs path {path}"))?;
+        self.client
+            .request_typed(local_request(request_id, path))
+            .await
+            .wrap_err_with(|| format!("{method} failed in TUI"))
+    }
+
+    async fn request_remote_json_rpc_typed<T>(
+        &self,
+        request_id: RequestId,
+        method: &str,
+        params: serde_json::Value,
+    ) -> Result<T>
+    where
+        T: DeserializeOwned,
+    {
+        let AppServerRequestHandle::Remote(handle) = self.request_handle() else {
+            color_eyre::eyre::bail!(
+                "raw JSON-RPC requests are only supported by the remote app-server client"
+            );
+        };
+        let response = handle
+            .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

codex-rs/tui/src/goal_files.rs

@@ -4,6 +4,7 @@
 //! server's Codex home directory. The persisted goal objective keeps references
 //! to those files so later continuations can read them by path.
 
+use std::collections::HashMap;
 use std::fs;
 use std::path::Path;
 
@@ -13,23 +14,9 @@ use crate::bottom_pane::LocalImageAttachment;
 use anyhow::Context;
 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;
 
 const GOAL_ATTACHMENT_DIR: &str = "attachments";
@@ -74,113 +61,24 @@ pub(crate) type GoalFilePath = String;
 
 impl GoalFileStore for AppServerSession {
     async fn create_directory(&mut self, path: GoalFilePath) -> Result<()> {
-        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(())
+        self.fs_create_directory_path(&path, /*recursive*/ true)
+            .await
+            .map_err(|err| anyhow::anyhow!("{err}"))
     }
 
     async fn write_file(&mut self, path: GoalFilePath, bytes: Vec<u8>) -> Result<()> {
-        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(())
+        self.fs_write_file_path(&path, bytes)
+            .await
+            .map_err(|err| anyhow::anyhow!("{err}"))
     }
 
     async fn read_file(&mut self, path: GoalFilePath) -> Result<Vec<u8>> {
-        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")
+        self.fs_read_file_path(&path)
+            .await
+            .map_err(|err| anyhow::anyhow!("{err}"))
     }
 }
 
-async fn request_goal_fs<T>(
-    app_server: &AppServerSession,
-    method: &str,
-    local_request: ClientRequest,
-    remote_params: serde_json::Value,
-) -> Result<T>
-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<T>(
-    app_server: &AppServerSession,
-    method: &str,
-    params: serde_json::Value,
-) -> Result<T>
-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,
@@ -192,11 +90,6 @@ pub(crate) fn codex_home_for_app_server(
     }
 }
 
-fn local_goal_file_path(path: &GoalFilePath) -> Result<AbsolutePathBuf> {
-    AbsolutePathBuf::from_absolute_path_checked(path)
-        .with_context(|| format!("invalid local goal file path {path}"))
-}
-
 fn join_goal_path(path: &str, segment: impl AsRef<str>) -> GoalFilePath {
     let separator = if is_windows_absolute_path(path) {
         '\\'
@@ -240,13 +133,20 @@ pub(crate) async fn materialize_goal_draft(
         bail!("Goal objective must not be empty.");
     }
     let text_elements = draft.text_elements;
+    let mut active_paste_placeholders =
+        active_paste_placeholder_counts(&objective, &text_elements, &draft.pending_pastes);
 
     let mut output_dir = None;
     let mut replacements = Vec::new();
-    for (idx, (placeholder, text)) in draft.pending_pastes.iter().enumerate() {
+    let mut paste_idx = 0;
+    for (placeholder, text) in draft.pending_pastes.iter() {
+        if !take_active_placeholder(&mut active_paste_placeholders, placeholder) {
+            continue;
+        }
+        paste_idx += 1;
         let path = join_goal_path(
             &ensure_output_dir(store, codex_home, &mut output_dir).await?,
-            format!("pasted-text-{}.txt", idx + 1),
+            format!("pasted-text-{paste_idx}.txt"),
         );
         write_file(store, path.clone(), text.as_bytes().to_vec()).await?;
 
@@ -298,6 +198,37 @@ pub(crate) async fn materialize_goal_draft(
     Ok(objective)
 }
 
+fn active_paste_placeholder_counts(
+    objective: &str,
+    text_elements: &[TextElement],
+    pending_pastes: &[(String, String)],
+) -> HashMap<String, usize> {
+    let mut counts = pending_pastes
+        .iter()
+        .map(|(placeholder, _)| (placeholder.clone(), 0))
+        .collect::<HashMap<_, _>>();
+    for element in text_elements {
+        if let Some(count) = element
+            .placeholder(objective)
+            .and_then(|placeholder| counts.get_mut(placeholder))
+        {
+            *count += 1;
+        }
+    }
+    counts
+}
+
+fn take_active_placeholder(counts: &mut HashMap<String, usize>, placeholder: &str) -> bool {
+    let Some(count) = counts.get_mut(placeholder) else {
+        return false;
+    };
+    if *count == 0 {
+        return false;
+    }
+    *count -= 1;
+    true
+}
+
 pub(crate) async fn objective_text_for_edit(
     store: &mut impl GoalFileStore,
     objective: &str,

codex-rs/tui/src/goal_files_tests.rs

@@ -101,3 +101,23 @@ async fn plain_objective_does_not_need_codex_home() {
 
     assert_eq!(objective, "read src/lib.rs");
 }
+
+#[tokio::test]
+async fn deleted_paste_placeholder_does_not_materialize_or_need_codex_home() {
+    let mut store = RecordingStore::default();
+
+    let objective = materialize_goal_draft(
+        &mut store,
+        /*codex_home*/ None,
+        GoalDraft {
+            objective: "small goal".to_string(),
+            pending_pastes: vec![("[Pasted Content 5 chars]".to_string(), "hello".to_string())],
+            ..Default::default()
+        },
+    )
+    .await
+    .expect("materialize plain goal draft");
+
+    assert_eq!(objective, "small goal");
+    assert!(store.writes.is_empty());
+}