Merge 6643dcf735 into sapling-pr-archive-bolinfest

.github/ISSUE_TEMPLATE/4-feature-request.yml

@@ -0,0 +1,31 @@
+name: 🎁 Feature Request
+description: Propose a new feature for Codex
+labels:
+  - enhancement
+  - needs triage
+body:
+  - type: markdown
+    attributes:
+      value: |
+        Is Codex missing a feature that you'd like to see? Feel free to propose it here.
+
+        Before you submit a feature:
+        1. Search existing issues for similar features. If you find one, 👍 it rather than opening a new one.
+        2. The Codex team will try to balance the varying needs of the community when prioritizing or rejecting new features. Not all features will be accepted. See [Contributing](https://github.com/openai/codex#contributing) for more details.
+
+  - type: textarea
+    id: feature
+    attributes:
+      label: What feature would you like to see?
+    validations:
+      required: true
+  - type: textarea
+    id: author
+    attributes:
+      label: Are you interested in implementing this feature?
+      description: Please wait for acknowledgement before implementing or opening a PR.
+  - type: textarea
+    id: notes
+    attributes:
+      label: Additional information
+      description: Is there anything else you think we should know?

.github/pull_request_template.md

@@ -0,0 +1,6 @@
+# External (non-OpenAI) Pull Request Requirements
+
+Before opening this Pull Request, please read the "Contributing" section of the README or your PR may be closed:
+https://github.com/openai/codex#contributing
+
+If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes.

README.md

@@ -566,9 +566,13 @@ We're excited to launch a **$1 million initiative** supporting open source proje
 
 ## Contributing
 
-This project is under active development and the code will likely change pretty significantly. We'll update this message once that's complete!
+This project is under active development and the code will likely change pretty significantly.
 
-More broadly we welcome contributions - whether you are opening your very first pull request or you're a seasoned maintainer. At the same time we care about reliability and long-term maintainability, so the bar for merging code is intentionally **high**. The guidelines below spell out what "high-quality" means in practice and should make the whole process transparent and friendly.
+**At the moment, we only plan to prioritize reviewing external contributions for bugs or security fixes.**
+
+If you want to add a new feature or change the behavior of an existing one, please open an issue proposing the feature and get approval from an OpenAI team member before spending time building it.
+
+**New contributions that don't go through this process may be closed** if they aren't aligned with our current roadmap or conflict with other priorities/upcoming features.
 
 ### Development workflow
 
@@ -593,8 +597,9 @@ More broadly we welcome contributions - whether you are opening your very first
 ### Review process
 
 1. One maintainer will be assigned as a primary reviewer.
-2. We may ask for changes - please do not take this personally. We value the work, we just also value consistency and long-term maintainability.
-3. When there is consensus that the PR meets the bar, a maintainer will squash-and-merge.
+2. If your PR adds a new feature that was not previously discussed and approved, we may choose to close your PR (see [Contributing](#contributing)).
+3. We may ask for changes - please do not take this personally. We value the work, but we also value consistency and long-term maintainability.
+5. When there is consensus that the PR meets the bar, a maintainer will squash-and-merge.
 
 ### Community values
 

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 {

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();
@@ -1606,6 +1618,7 @@ async fn handle_response_item(
                 for item in content {
                     let text = match item {
                         ReasoningItemContent::ReasoningText { text } => text,
+                        ReasoningItemContent::Text { text } => text,
                     };
                     let event = Event {
                         id: sub_id.to_string(),

codex-rs/core/src/models.rs

@@ -45,7 +45,7 @@ pub enum ResponseItem {
     Reasoning {
         id: String,
         summary: Vec<ReasoningItemReasoningSummary>,
-        #[serde(default, skip_serializing_if = "Option::is_none")]
+        #[serde(default, skip_serializing_if = "should_serialize_reasoning_content")]
         content: Option<Vec<ReasoningItemContent>>,
         encrypted_content: Option<String>,
     },
@@ -81,6 +81,15 @@ pub enum ResponseItem {
     Other,
 }
 
+fn should_serialize_reasoning_content(content: &Option<Vec<ReasoningItemContent>>) -> bool {
+    match content {
+        Some(content) => !content
+            .iter()
+            .any(|c| matches!(c, ReasoningItemContent::ReasoningText { .. })),
+        None => false,
+    }
+}
+
 impl From<ResponseInputItem> for ResponseItem {
     fn from(item: ResponseInputItem) -> Self {
         match item {
@@ -142,6 +151,7 @@ pub enum ReasoningItemReasoningSummary {
 #[serde(tag = "type", rename_all = "snake_case")]
 pub enum ReasoningItemContent {
     ReasoningText { text: String },
+    Text { text: String },
 }
 
 impl From<Vec<InputItem>> for ResponseInputItem {

codex-rs/tui/src/chatwidget.rs

@@ -133,6 +133,7 @@ impl ChatWidget<'_> {
     fn on_agent_message(&mut self, message: String) {
         let sink = AppEventHistorySink(self.app_event_tx.clone());
         let finished = self.stream.apply_final_answer(&message, &sink);
+        self.last_stream_kind = Some(StreamKind::Answer);
         self.handle_if_stream_finished(finished);
         self.mark_needs_redraw();
     }
@@ -145,9 +146,10 @@ impl ChatWidget<'_> {
         self.handle_streaming_delta(StreamKind::Reasoning, delta);
     }
 
-    fn on_agent_reasoning_final(&mut self) {
+    fn on_agent_reasoning_final(&mut self, text: String) {
         let sink = AppEventHistorySink(self.app_event_tx.clone());
-        let finished = self.stream.finalize(StreamKind::Reasoning, false, &sink);
+        let finished = self.stream.apply_final_reasoning(&text, &sink);
+        self.last_stream_kind = Some(StreamKind::Reasoning);
         self.handle_if_stream_finished(finished);
         self.mark_needs_redraw();
     }
@@ -633,9 +635,9 @@ impl ChatWidget<'_> {
             | EventMsg::AgentReasoningRawContentDelta(AgentReasoningRawContentDeltaEvent {
                 delta,
             }) => self.on_agent_reasoning_delta(delta),
-            EventMsg::AgentReasoning(AgentReasoningEvent { .. })
-            | EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { .. }) => {
-                self.on_agent_reasoning_final()
+            EventMsg::AgentReasoning(AgentReasoningEvent { text })
+            | EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { text }) => {
+                self.on_agent_reasoning_final(text)
             }
             EventMsg::AgentReasoningSectionBreak(_) => self.on_reasoning_section_break(),
             EventMsg::TaskStarted => self.on_task_started(),

codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap

@@ -0,0 +1,10 @@
+---
+source: tui/src/chatwidget/tests.rs
+assertion_line: 886
+expression: combined
+---
+thinking
+I will first analyze the request.
+
+codex
+Here is the result.

codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap

@@ -0,0 +1,9 @@
+---
+source: tui/src/chatwidget/tests.rs
+expression: combined
+---
+thinking
+I will first analyze the request.
+
+codex
+Here is the result.

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

@@ -12,6 +12,7 @@ use codex_core::plan_tool::UpdatePlanArgs;
 use codex_core::protocol::AgentMessageDeltaEvent;
 use codex_core::protocol::AgentMessageEvent;
 use codex_core::protocol::AgentReasoningDeltaEvent;
+use codex_core::protocol::AgentReasoningEvent;
 use codex_core::protocol::ApplyPatchApprovalRequestEvent;
 use codex_core::protocol::Event;
 use codex_core::protocol::EventMsg;
@@ -24,6 +25,7 @@ use codex_core::protocol::TaskCompleteEvent;
 use crossterm::event::KeyCode;
 use crossterm::event::KeyEvent;
 use crossterm::event::KeyModifiers;
+use insta::assert_snapshot;
 use pretty_assertions::assert_eq;
 use std::fs::File;
 use std::io::BufRead;
@@ -413,46 +415,6 @@ async fn binary_size_transcript_matches_ideal_fixture() {
     assert_eq!(visible_after, ideal);
 }
 
-#[test]
-fn final_longer_answer_after_single_char_delta_is_complete() {
-    let (mut chat, rx, _op_rx) = make_chatwidget_manual();
-
-    // Simulate a stray delta without newline (e.g., punctuation).
-    chat.handle_codex_event(Event {
-        id: "sub-x".into(),
-        msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: "?".into() }),
-    });
-
-    // Now send the full final answer with no newline.
-    let full = "Hi! How can I help with codex-rs today? Want me to explore the repo, run tests, or work on a specific change?";
-    chat.handle_codex_event(Event {
-        id: "sub-x".into(),
-        msg: EventMsg::AgentMessage(AgentMessageEvent {
-            message: full.into(),
-        }),
-    });
-
-    // Drain and assert the full message appears in history.
-    let cells = drain_insert_history(&rx);
-    let mut found = false;
-    for lines in &cells {
-        let s = lines
-            .iter()
-            .flat_map(|l| l.spans.iter())
-            .map(|sp| sp.content.clone())
-            .collect::<String>();
-        if s.contains(full) {
-            found = true;
-            break;
-        }
-    }
-    assert!(
-        found,
-        "expected full final message to be flushed to history, cells={:?}",
-        cells.len()
-    );
-}
-
 #[test]
 fn apply_patch_events_emit_history_cells() {
     let (mut chat, rx, _op_rx) = make_chatwidget_manual();
@@ -923,3 +885,91 @@ fn multiple_agent_messages_in_single_turn_emit_multiple_headers() {
     let second_idx = combined.find("Second message").unwrap();
     assert!(first_idx < second_idx, "messages out of order: {combined}");
 }
+
+#[test]
+fn final_reasoning_then_message_without_deltas_are_rendered() {
+    let (mut chat, rx, _op_rx) = make_chatwidget_manual();
+
+    // No deltas; only final reasoning followed by final message.
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentReasoning(AgentReasoningEvent {
+            text: "I will first analyze the request.".into(),
+        }),
+    });
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentMessage(AgentMessageEvent {
+            message: "Here is the result.".into(),
+        }),
+    });
+
+    // Drain history and snapshot the combined visible content.
+    let cells = drain_insert_history(&rx);
+    let combined = cells
+        .iter()
+        .map(|lines| lines_to_single_string(lines))
+        .collect::<String>();
+    assert_snapshot!(combined);
+}
+
+#[test]
+fn deltas_then_same_final_message_are_rendered_snapshot() {
+    let (mut chat, rx, _op_rx) = make_chatwidget_manual();
+
+    // Stream some reasoning deltas first.
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent {
+            delta: "I will ".into(),
+        }),
+    });
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent {
+            delta: "first analyze the ".into(),
+        }),
+    });
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent {
+            delta: "request.".into(),
+        }),
+    });
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentReasoning(AgentReasoningEvent {
+            text: "request.".into(),
+        }),
+    });
+
+    // Then stream answer deltas, followed by the exact same final message.
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent {
+            delta: "Here is the ".into(),
+        }),
+    });
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent {
+            delta: "result.".into(),
+        }),
+    });
+
+    chat.handle_codex_event(Event {
+        id: "s1".into(),
+        msg: EventMsg::AgentMessage(AgentMessageEvent {
+            message: "Here is the result.".into(),
+        }),
+    });
+
+    // Snapshot the combined visible content to ensure we render as expected
+    // when deltas are followed by the identical final message.
+    let cells = drain_insert_history(&rx);
+    let combined = cells
+        .iter()
+        .map(|lines| lines_to_single_string(lines))
+        .collect::<String>();
+    assert_snapshot!(combined);
+}

codex-rs/tui/src/streaming/controller.rs

@@ -143,6 +143,10 @@ impl StreamController {
         };
         let cfg = self.config.clone();
         let state = self.state_mut(kind);
+        // Record that at least one delta was received for this stream
+        if !delta.is_empty() {
+            state.has_seen_delta = true;
+        }
         state.collector.push_delta(delta);
         if delta.contains('\n') {
             let newly_completed = state.collector.commit_complete_lines(&cfg);
@@ -263,19 +267,41 @@ impl StreamController {
     /// Apply a full final answer: replace queued content with only the remaining tail,
     /// then finalize immediately and notify completion.
     pub(crate) fn apply_final_answer(&mut self, message: &str, sink: &impl HistorySink) -> bool {
-        self.begin(StreamKind::Answer, sink);
-        if !message.is_empty() {
-            let mut msg_with_nl = message.to_string();
-            if !msg_with_nl.ends_with('\n') {
-                msg_with_nl.push('\n');
+        self.apply_full_final(StreamKind::Answer, message, true, sink)
+    }
+
+    pub(crate) fn apply_final_reasoning(&mut self, message: &str, sink: &impl HistorySink) -> bool {
+        self.apply_full_final(StreamKind::Reasoning, message, false, sink)
+    }
+
+    fn apply_full_final(
+        &mut self,
+        kind: StreamKind,
+        message: &str,
+        immediate: bool,
+        sink: &impl HistorySink,
+    ) -> bool {
+        self.begin(kind, sink);
+
+        {
+            let state = self.state_mut(kind);
+            // Only inject the final full message if we have not seen any deltas for this stream.
+            // If deltas were received, rely on the collector's existing buffer to avoid duplication.
+            if !state.has_seen_delta && !message.is_empty() {
+                // normalize to end with newline
+                let mut msg = message.to_owned();
+                if !msg.ends_with('\n') {
+                    msg.push('\n');
+                }
+
+                // replace while preserving already committed count
+                let committed = state.collector.committed_count();
+                state
+                    .collector
+                    .replace_with_and_mark_committed(&msg, committed);
             }
-            let state = self.state_mut(StreamKind::Answer);
-            let already_committed = state.collector.committed_count();
-            // Preserve previously committed count so finalize emits only the remaining tail.
-            state
-                .collector
-                .replace_with_and_mark_committed(&msg_with_nl, already_committed);
         }
-        self.finalize(StreamKind::Answer, true, sink)
+
+        self.finalize(kind, immediate, sink)
     }
 }

codex-rs/tui/src/streaming/mod.rs

@@ -11,6 +11,7 @@ pub(crate) enum StreamKind {
 pub(crate) struct StreamState {
     pub(crate) collector: MarkdownStreamCollector,
     pub(crate) streamer: AnimatedLineStreamer,
+    pub(crate) has_seen_delta: bool,
 }
 
 impl StreamState {
@@ -18,11 +19,13 @@ impl StreamState {
         Self {
             collector: MarkdownStreamCollector::new(),
             streamer: AnimatedLineStreamer::new(),
+            has_seen_delta: false,
         }
     }
     pub(crate) fn clear(&mut self) {
         self.collector.clear();
         self.streamer.clear();
+        self.has_seen_delta = false;
     }
     pub(crate) fn step(&mut self) -> crate::markdown_stream::StepResult {
         self.streamer.step()