From 085f166707b4e3b4bddf34a9599201460d135ea8 Mon Sep 17 00:00:00 2001
From: Michael Bolin <mbolin@openai.com>
Date: Wed, 13 Aug 2025 22:53:54 -0700
Subject: [PATCH] fix: make all fields of Session private (#2285)

As `Session` needs a bit of work, it will make things easier to move
around if we can start by reducing the extent of its public API. This
makes all the fields private, though adds three `pub(crate)` getters.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2285).
* #2287
* #2286
* __->__ #2285
---
 codex-rs/core/src/apply_patch.rs | 10 +++----
 codex-rs/core/src/codex.rs       | 46 ++++++++++++++++++++------------
 codex-rs/core/src/protocol.rs    |  2 +-
 3 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs
index dc11aed0..21e80406 100644
--- a/codex-rs/core/src/apply_patch.rs
+++ b/codex-rs/core/src/apply_patch.rs
@@ -45,17 +45,13 @@ pub(crate) async fn apply_patch(
     call_id: &str,
     action: ApplyPatchAction,
 ) -> InternalApplyPatchInvocation {
-    let writable_roots_snapshot = {
-        #[allow(clippy::unwrap_used)]
-        let guard = sess.writable_roots.lock().unwrap();
-        guard.clone()
-    };
+    let writable_roots_snapshot = sess.get_writable_roots().to_vec();
 
     match assess_patch_safety(
         &action,
-        sess.approval_policy,
+        sess.get_approval_policy(),
         &writable_roots_snapshot,
-        &sess.cwd,
+        sess.get_cwd(),
     ) {
         SafetyCheck::AutoApprove { .. } => {
             InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs
index e9453a24..ff726a24 100644
--- a/codex-rs/core/src/codex.rs
+++ b/codex-rs/core/src/codex.rs
@@ -4,6 +4,7 @@
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::collections::HashSet;
+use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
 use std::sync::Mutex;
@@ -199,23 +200,33 @@ impl Codex {
     }
 }
 
+/// Mutable state of the agent
+#[derive(Default)]
+struct State {
+    approved_commands: HashSet<Vec<String>>,
+    current_task: Option<AgentTask>,
+    pending_approvals: HashMap<String, oneshot::Sender<ReviewDecision>>,
+    pending_input: Vec<ResponseInputItem>,
+    history: ConversationHistory,
+}
+
 /// Context for an initialized model agent
 ///
 /// A session has at most 1 running task at a time, and can be interrupted by user input.
 pub(crate) struct Session {
     client: ModelClient,
-    pub(crate) tx_event: Sender<Event>,
+    tx_event: Sender<Event>,
 
     /// The session's current working directory. All relative paths provided by
     /// the model as well as sandbox policies are resolved against this path
     /// instead of `std::env::current_dir()`.
-    pub(crate) cwd: PathBuf,
+    cwd: PathBuf,
     base_instructions: Option<String>,
     user_instructions: Option<String>,
-    pub(crate) approval_policy: AskForApproval,
+    approval_policy: AskForApproval,
     sandbox_policy: SandboxPolicy,
     shell_environment_policy: ShellEnvironmentPolicy,
-    pub(crate) writable_roots: Mutex<Vec<PathBuf>>,
+    writable_roots: Vec<PathBuf>,
     disable_response_storage: bool,
     tools_config: ToolsConfig,
 
@@ -236,24 +247,24 @@ pub(crate) struct Session {
 }
 
 impl Session {
+    pub(crate) fn get_writable_roots(&self) -> &[PathBuf] {
+        &self.writable_roots
+    }
+
+    pub(crate) fn get_approval_policy(&self) -> AskForApproval {
+        self.approval_policy
+    }
+
+    pub(crate) fn get_cwd(&self) -> &Path {
+        &self.cwd
+    }
+
     fn resolve_path(&self, path: Option<String>) -> PathBuf {
         path.as_ref()
             .map(PathBuf::from)
             .map_or_else(|| self.cwd.clone(), |p| self.cwd.join(p))
     }
-}
 
-/// Mutable state of the agent
-#[derive(Default)]
-struct State {
-    approved_commands: HashSet<Vec<String>>,
-    current_task: Option<AgentTask>,
-    pending_approvals: HashMap<String, oneshot::Sender<ReviewDecision>>,
-    pending_input: Vec<ResponseInputItem>,
-    history: ConversationHistory,
-}
-
-impl Session {
     pub fn set_task(&self, task: AgentTask) {
         let mut state = self.state.lock().unwrap();
         if let Some(current_task) = state.current_task.take() {
@@ -659,6 +670,7 @@ impl AgentTask {
             handle,
         }
     }
+
     fn compact(
         sess: Arc<Session>,
         sub_id: String,
@@ -816,7 +828,7 @@ async fn submission_loop(
                     },
                 };
 
-                let writable_roots = Mutex::new(get_writable_roots(&cwd));
+                let writable_roots = get_writable_roots(&cwd);
 
                 // Error messages to dispatch after SessionConfigured is sent.
                 let mut mcp_connection_errors = Vec::<Event>::new();
diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs
index e984b95f..a7e6b1ed 100644
--- a/codex-rs/core/src/protocol.rs
+++ b/codex-rs/core/src/protocol.rs
@@ -723,7 +723,7 @@ pub enum ReviewDecision {
     Abort,
 }
 
-#[derive(Debug, Clone, Deserialize, Serialize)]
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
 #[serde(rename_all = "snake_case")]
 pub enum FileChange {
     Add {