diff --git a/.github/ISSUE_TEMPLATE/4-feature-request.yml b/.github/ISSUE_TEMPLATE/4-feature-request.yml new file mode 100644 index 0000000000..fc95a67ec2 --- /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: + - 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? 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 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 207f75a72f..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(); @@ -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(), 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()