From a62510e0ae9a95f074e99820daa1bc82f73c113e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 17:54:12 -0700 Subject: [PATCH 1/5] fix: verify notifications are sent with the conversationId set (#2278) This updates `CodexMessageProcessor` so that each notification it sends for a `EventMsg` from a `CodexConversation` such that: - The `params` always has an appropriate `conversationId` field. - The `method` is now includes the name of the `EventMsg` type rather than using `codex/event` as the `method` type for all notifications. (We currently prefix the method name with `codex/event/`, but I think that should go away once we formalize the notification schema in `wire_format.rs`.) As part of this, we update `test_codex_jsonrpc_conversation_flow()` to verify that the `task_finished` notification has made it through the system instead of sleeping for 5s and "hoping" the server finished processing the task. Note we have seen some flakiness in some of our other, similar integration tests, and I expect adding a similar check would help in those cases, as well. --- .../mcp-server/src/codex_message_processor.rs | 25 ++++++++++++----- codex-rs/mcp-server/src/outgoing_message.rs | 11 +++++--- .../tests/codex_message_processor_flow.rs | 25 ++++++++++++++--- .../mcp-server/tests/common/mcp_process.rs | 27 +++++++++++++++++++ 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index c0a6ab287c..5e0e4cad6e 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -15,7 +15,7 @@ use crate::error_code::INTERNAL_ERROR_CODE; use crate::error_code::INVALID_REQUEST_ERROR_CODE; use crate::json_to_toml::json_to_toml; use crate::outgoing_message::OutgoingMessageSender; -use crate::outgoing_message::OutgoingNotificationMeta; +use crate::outgoing_message::OutgoingNotification; use crate::wire_format::AddConversationListenerParams; use crate::wire_format::AddConversationSubscriptionResponse; use crate::wire_format::CodexRequest; @@ -176,7 +176,6 @@ impl CodexMessageProcessor { self.conversation_listeners .insert(subscription_id, cancel_tx); let outgoing_for_task = self.outgoing.clone(); - let add_listener_request_id = request_id.clone(); tokio::spawn(async move { loop { tokio::select! { @@ -193,10 +192,24 @@ impl CodexMessageProcessor { } }; - outgoing_for_task.send_event_as_notification( - &event, - Some(OutgoingNotificationMeta::new(Some(add_listener_request_id.clone()))), - ) + let method = format!("codex/event/{}", event.msg); + let mut params = match serde_json::to_value(event) { + Ok(serde_json::Value::Object(map)) => map, + Ok(_) => { + tracing::error!("event did not serialize to an object"); + continue; + } + Err(err) => { + tracing::error!("failed to serialize event: {err}"); + continue; + } + }; + params.insert("conversationId".to_string(), conversation_id.to_string().into()); + + outgoing_for_task.send_notification(OutgoingNotification { + method, + params: Some(params.into()), + }) .await; } } diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index 3a77cbf2a0..f13b8b324f 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -114,16 +114,21 @@ impl OutgoingMessageSender { event_json }; - let outgoing_message = OutgoingMessage::Notification(OutgoingNotification { + self.send_notification(OutgoingNotification { method: "codex/event".to_string(), params: Some(params.clone()), - }); - let _ = self.sender.send(outgoing_message).await; + }) + .await; self.send_event_as_notification_new_schema(event, Some(params.clone())) .await; } + pub(crate) async fn send_notification(&self, notification: OutgoingNotification) { + let outgoing_message = OutgoingMessage::Notification(notification); + let _ = self.sender.send(outgoing_message).await; + } + // should be backwards compatible. // it will replace send_event_as_notification eventually. async fn send_event_as_notification_new_schema( diff --git a/codex-rs/mcp-server/tests/codex_message_processor_flow.rs b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs index c81a04a5f4..7b7845609e 100644 --- a/codex-rs/mcp-server/tests/codex_message_processor_flow.rs +++ b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs @@ -15,6 +15,7 @@ use mcp_test_support::McpProcess; use mcp_test_support::create_final_assistant_message_sse_response; use mcp_test_support::create_mock_chat_completions_server; use mcp_test_support::create_shell_sse_response; +use mcp_types::JSONRPCNotification; use mcp_types::JSONRPCResponse; use mcp_types::RequestId; use pretty_assertions::assert_eq; @@ -123,10 +124,26 @@ async fn test_codex_jsonrpc_conversation_flow() { let SendUserMessageResponse {} = to_response::(send_user_resp) .expect("deserialize sendUserMessage response"); - // Give the server time to process the user's request. - tokio::time::sleep(std::time::Duration::from_millis(5_000)).await; - - // Could verify that some notifications were received? + // Verify the task_finished notification is received. + // Note this also ensures that the final request to the server was made. + let task_finished_notification: JSONRPCNotification = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await + .expect("task_finished_notification timeout") + .expect("task_finished_notification resp"); + let serde_json::Value::Object(map) = task_finished_notification + .params + .expect("notification should have params") + else { + panic!("task_finished_notification should have params"); + }; + assert_eq!( + map.get("conversationId") + .expect("should have conversationId"), + &serde_json::Value::String(conversation_id.to_string()) + ); // 4) removeConversationListener let remove_listener_id = mcp diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 4181479b5f..a659b1d950 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -374,6 +374,33 @@ impl McpProcess { } } + pub async fn read_stream_until_notification_message( + &mut self, + method: &str, + ) -> anyhow::Result { + loop { + let message = self.read_jsonrpc_message().await?; + eprint!("message: {message:?}"); + + match message { + JSONRPCMessage::Notification(notification) => { + if notification.method == method { + return Ok(notification); + } + } + JSONRPCMessage::Request(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Request: {message:?}"); + } + JSONRPCMessage::Error(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Error: {message:?}"); + } + JSONRPCMessage::Response(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Response: {message:?}"); + } + } + } + } + pub async fn read_stream_until_configured_response_message( &mut self, ) -> anyhow::Result { From f1be7978cf516c393b904003eccac688c16de512 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 13 Aug 2025 18:39:58 -0700 Subject: [PATCH 2/5] Parse reasoning text content (#2277) Sometimes COT is returns as text content instead of `ReasoningText`. We should parse it but not serialize back on requests. --------- Co-authored-by: Ahmed Ibrahim --- codex-rs/core/src/codex.rs | 1 + codex-rs/core/src/models.rs | 12 +- codex-rs/tui/src/chatwidget.rs | 12 +- ...e_final_message_are_rendered_snapshot.snap | 10 ++ ...n_message_without_deltas_are_rendered.snap | 9 ++ codex-rs/tui/src/chatwidget/tests.rs | 130 ++++++++++++------ codex-rs/tui/src/streaming/controller.rs | 50 +++++-- codex-rs/tui/src/streaming/mod.rs | 3 + 8 files changed, 169 insertions(+), 58 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 207f75a72f..e9453a2490 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1606,6 +1606,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(), diff --git a/codex-rs/core/src/models.rs b/codex-rs/core/src/models.rs index e052bc43a4..da1e31b706 100644 --- a/codex-rs/core/src/models.rs +++ b/codex-rs/core/src/models.rs @@ -45,7 +45,7 @@ pub enum ResponseItem { Reasoning { id: String, summary: Vec, - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "should_serialize_reasoning_content")] content: Option>, encrypted_content: Option, }, @@ -81,6 +81,15 @@ pub enum ResponseItem { Other, } +fn should_serialize_reasoning_content(content: &Option>) -> bool { + match content { + Some(content) => !content + .iter() + .any(|c| matches!(c, ReasoningItemContent::ReasoningText { .. })), + None => false, + } +} + impl From 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> for ResponseInputItem { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 517a9a35f5..9f7f0518e2 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/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(), diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap new file mode 100644 index 0000000000..8258ea0b28 --- /dev/null +++ b/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. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap new file mode 100644 index 0000000000..c90ec2733d --- /dev/null +++ b/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. diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 0a7dd93368..143dd4d035 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/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::(); - 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::(); + 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::(); + assert_snapshot!(combined); +} diff --git a/codex-rs/tui/src/streaming/controller.rs b/codex-rs/tui/src/streaming/controller.rs index 5201d35942..161a111173 100644 --- a/codex-rs/tui/src/streaming/controller.rs +++ b/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) } } diff --git a/codex-rs/tui/src/streaming/mod.rs b/codex-rs/tui/src/streaming/mod.rs index c6f4ad2149..bb4fdcb6ff 100644 --- a/codex-rs/tui/src/streaming/mod.rs +++ b/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() From e8ffecd632584e57bf3a670ecf46c54ad4f3e424 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Wed, 13 Aug 2025 18:56:29 -0700 Subject: [PATCH 3/5] Clarify PR/Contribution guidelines and issue templates (#2281) Co-authored-by: Dylan --- .github/ISSUE_TEMPLATE/4-feature-request.yml | 31 ++++++++++++++++++++ .github/pull_request_template.md | 6 ++++ README.md | 13 +++++--- 3 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/4-feature-request.yml create mode 100644 .github/pull_request_template.md diff --git a/.github/ISSUE_TEMPLATE/4-feature-request.yml b/.github/ISSUE_TEMPLATE/4-feature-request.yml new file mode 100644 index 0000000000..70cd7c756d --- /dev/null +++ b/.github/ISSUE_TEMPLATE/4-feature-request.yml @@ -0,0 +1,31 @@ +name: 🎁 Feature Request +description: Propose a new feature for Codex +labels: + - feature + - 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? diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000000..03cedab29c --- /dev/null +++ b/.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. diff --git a/README.md b/README.md index 0c01654d66..596362a30a 100644 --- a/README.md +++ b/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 From 6d0eb9128e1ab1a049220000ff811334f58de74b Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Thu, 14 Aug 2025 12:08:35 +0900 Subject: [PATCH 4/5] Use enhancement tag for feature requests (#2282) --- .github/ISSUE_TEMPLATE/4-feature-request.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/4-feature-request.yml b/.github/ISSUE_TEMPLATE/4-feature-request.yml index 70cd7c756d..fc95a67ec2 100644 --- a/.github/ISSUE_TEMPLATE/4-feature-request.yml +++ b/.github/ISSUE_TEMPLATE/4-feature-request.yml @@ -1,7 +1,7 @@ name: 🎁 Feature Request description: Propose a new feature for Codex labels: - - feature + - enhancement - needs triage body: - type: markdown From 6643dcf735b6adc169238c4717a4cd224376e491 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 21:30:56 -0700 Subject: [PATCH 5/5] fix: make all fields of Session private --- codex-rs/core/src/apply_patch.rs | 10 +++---- codex-rs/core/src/codex.rs | 46 ++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index dc11aed023..21e80406e5 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 e9453a2490..ff726a2426 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>, + current_task: Option, + pending_approvals: HashMap>, + pending_input: Vec, + 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, + tx_event: Sender, /// 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, user_instructions: Option, - pub(crate) approval_policy: AskForApproval, + approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, shell_environment_policy: ShellEnvironmentPolicy, - pub(crate) writable_roots: Mutex>, + writable_roots: Vec, 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) -> 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>, - current_task: Option, - pending_approvals: HashMap>, - pending_input: Vec, - 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, 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::::new();