From 4c2e730488997145ad9bfa07f193293aeb30c083 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 20 Apr 2026 22:39:17 -0700 Subject: [PATCH 1/3] Organize context fragments (#18794) Organize context fragments under `core/context`. Implement same trait on all of them. --- codex-rs/Cargo.lock | 11 -- codex-rs/Cargo.toml | 2 - codex-rs/core-skills/Cargo.toml | 1 - codex-rs/core-skills/src/injection.rs | 15 +- codex-rs/core/Cargo.toml | 1 - codex-rs/core/src/agent/control_tests.rs | 5 +- codex-rs/core/src/arc_monitor_tests.rs | 15 +- .../{ => context}/contextual_user_message.rs | 73 +++++----- .../contextual_user_message_tests.rs | 8 +- .../src/{ => context}/environment_context.rs | 133 ++++++++--------- .../environment_context_tests.rs | 134 +++--------------- codex-rs/core/src/context/fragment.rs | 82 +++++++++++ codex-rs/core/src/context/mod.rs | 21 +++ .../core/src/context/skill_instructions.rs | 33 +++++ .../core/src/context/subagent_notification.rs | 34 +++++ codex-rs/core/src/context/turn_aborted.rs | 26 ++++ .../core/src/context/user_instructions.rs | 17 +++ .../core/src/context/user_shell_command.rs | 40 ++++++ codex-rs/core/src/context_manager/updates.rs | 9 +- codex-rs/core/src/event_mapping.rs | 4 +- codex-rs/core/src/instructions/mod.rs | 1 - codex-rs/core/src/lib.rs | 4 +- codex-rs/core/src/memories/phase1.rs | 2 +- codex-rs/core/src/session/mod.rs | 10 +- codex-rs/core/src/session/tests.rs | 4 +- codex-rs/core/src/session/turn.rs | 8 +- codex-rs/core/src/session_prefix.rs | 15 +- codex-rs/core/src/tasks/mod.rs | 19 +-- codex-rs/core/src/user_shell_command.rs | 36 ++--- codex-rs/core/src/user_shell_command_tests.rs | 12 +- codex-rs/instructions/BUILD.bazel | 16 --- codex-rs/instructions/Cargo.toml | 20 --- codex-rs/instructions/src/fragment.rs | 61 -------- codex-rs/instructions/src/lib.rs | 11 -- .../instructions/src/user_instructions.rs | 56 -------- .../src/user_instructions_tests.rs | 72 ---------- 36 files changed, 452 insertions(+), 559 deletions(-) rename codex-rs/core/src/{ => context}/contextual_user_message.rs (50%) rename codex-rs/core/src/{ => context}/contextual_user_message_tests.rs (94%) rename codex-rs/core/src/{ => context}/environment_context.rs (61%) rename codex-rs/core/src/{ => context}/environment_context_tests.rs (56%) create mode 100644 codex-rs/core/src/context/fragment.rs create mode 100644 codex-rs/core/src/context/mod.rs create mode 100644 codex-rs/core/src/context/skill_instructions.rs create mode 100644 codex-rs/core/src/context/subagent_notification.rs create mode 100644 codex-rs/core/src/context/turn_aborted.rs create mode 100644 codex-rs/core/src/context/user_instructions.rs create mode 100644 codex-rs/core/src/context/user_shell_command.rs delete mode 100644 codex-rs/core/src/instructions/mod.rs delete mode 100644 codex-rs/instructions/BUILD.bazel delete mode 100644 codex-rs/instructions/Cargo.toml delete mode 100644 codex-rs/instructions/src/fragment.rs delete mode 100644 codex-rs/instructions/src/lib.rs delete mode 100644 codex-rs/instructions/src/user_instructions.rs delete mode 100644 codex-rs/instructions/src/user_instructions_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 72d23456b2..15ca924a8b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1941,7 +1941,6 @@ dependencies = [ "codex-feedback", "codex-git-utils", "codex-hooks", - "codex-instructions", "codex-login", "codex-mcp", "codex-model-provider", @@ -2074,7 +2073,6 @@ dependencies = [ "codex-app-server-protocol", "codex-config", "codex-exec-server", - "codex-instructions", "codex-login", "codex-otel", "codex-protocol", @@ -2319,15 +2317,6 @@ dependencies = [ "tempfile", ] -[[package]] -name = "codex-instructions" -version = "0.0.0" -dependencies = [ - "codex-protocol", - "pretty_assertions", - "serde", -] - [[package]] name = "codex-keyring-store" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 4c15e47074..19987fb8f0 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -31,7 +31,6 @@ members = [ "core-plugins", "core-skills", "hooks", - "instructions", "secrets", "exec", "exec-server", @@ -143,7 +142,6 @@ codex-install-context = { path = "install-context" } codex-file-search = { path = "file-search" } codex-git-utils = { path = "git-utils" } codex-hooks = { path = "hooks" } -codex-instructions = { path = "instructions" } codex-keyring-store = { path = "keyring-store" } codex-linux-sandbox = { path = "linux-sandbox" } codex-lmstudio = { path = "lmstudio" } diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index 09278c8d0e..355374114a 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -18,7 +18,6 @@ codex-analytics = { workspace = true } codex-app-server-protocol = { workspace = true } codex-config = { workspace = true } codex-exec-server = { workspace = true } -codex-instructions = { workspace = true } codex-login = { workspace = true } codex-otel = { workspace = true } codex-protocol = { workspace = true } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 1ccd447cf0..ed06cc578e 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -10,19 +10,24 @@ use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::TrackEventsContext; use codex_exec_server::LOCAL_FS; -use codex_instructions::SkillInstructions; use codex_otel::SessionTelemetry; -use codex_protocol::models::ResponseItem; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; #[derive(Debug, Default)] pub struct SkillInjections { - pub items: Vec, + pub items: Vec, pub warnings: Vec, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillInjection { + pub name: String, + pub path: String, + pub contents: String, +} + pub async fn build_skill_injections( mentioned_skills: &[SkillMetadata], loaded_skills: Option<&SkillLoadOutcome>, @@ -56,11 +61,11 @@ pub async fn build_skill_injections( skill_path: skill.path_to_skills_md.to_path_buf(), invocation_type: InvocationType::Explicit, }); - result.items.push(ResponseItem::from(SkillInstructions { + result.items.push(SkillInjection { name: skill.name.clone(), path: skill.path_to_skills_md.to_string_lossy().into_owned(), contents, - })); + }); } Err(err) => { emit_skill_injected_metric(otel, skill, "error"); diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 8770aaf487..6ef6741fd9 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -48,7 +48,6 @@ codex-shell-command = { workspace = true } codex-execpolicy = { workspace = true } codex-git-utils = { workspace = true } codex-hooks = { workspace = true } -codex-instructions = { workspace = true } codex-network-proxy = { workspace = true } codex-otel = { workspace = true } codex-plugin = { workspace = true } diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index b444d8c67c..10850ef8c7 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -5,7 +5,8 @@ use crate::agent::agent_status_from_event; use crate::config::AgentRoleConfig; use crate::config::Config; use crate::config::ConfigBuilder; -use crate::contextual_user_message::SUBAGENT_NOTIFICATION_OPEN_TAG; +use crate::context::ContextualUserFragment; +use crate::context::SubagentNotification; use assert_matches::assert_matches; use codex_features::Feature; use codex_login::CodexAuth; @@ -127,7 +128,7 @@ fn has_subagent_notification(history_items: &[ResponseItem]) -> bool { } content.iter().any(|content_item| match content_item { ContentItem::InputText { text } | ContentItem::OutputText { text } => { - text.contains(SUBAGENT_NOTIFICATION_OPEN_TAG) + SubagentNotification::matches_text(text) } ContentItem::InputImage { .. } => false, }) diff --git a/codex-rs/core/src/arc_monitor_tests.rs b/codex-rs/core/src/arc_monitor_tests.rs index 6bf1aa98f0..5fd691b930 100644 --- a/codex-rs/core/src/arc_monitor_tests.rs +++ b/codex-rs/core/src/arc_monitor_tests.rs @@ -1,5 +1,6 @@ use std::env; use std::ffi::OsStr; +use std::path::PathBuf; use std::sync::Arc; use pretty_assertions::assert_eq; @@ -16,6 +17,7 @@ use wiremock::matchers::path; use super::*; use crate::agent_identity::AgentIdentityManager; use crate::agent_identity::RegisteredAgentTask; +use crate::context::ContextualUserFragment; use crate::session::tests::make_session_and_context; use chrono::Utc; use codex_login::AuthCredentialsStoreMode; @@ -143,11 +145,16 @@ async fn build_arc_monitor_request_includes_relevant_history_and_null_policies() .await; session .record_into_history( - &[ - crate::contextual_user_message::ENVIRONMENT_CONTEXT_FRAGMENT.into_message( - "\n/tmp\n".to_string(), + &[ContextualUserFragment::into( + crate::context::EnvironmentContext::new( + Some(PathBuf::from("/tmp")), + "zsh".to_string(), + /*current_date*/ None, + /*timezone*/ None, + /*network*/ None, + /*subagents*/ None, ), - ], + )], &turn_context, ) .await; diff --git a/codex-rs/core/src/contextual_user_message.rs b/codex-rs/core/src/context/contextual_user_message.rs similarity index 50% rename from codex-rs/core/src/contextual_user_message.rs rename to codex-rs/core/src/context/contextual_user_message.rs index 68a86cc885..aeb54d61d7 100644 --- a/codex-rs/core/src/contextual_user_message.rs +++ b/codex-rs/core/src/context/contextual_user_message.rs @@ -1,50 +1,47 @@ -use codex_instructions::AGENTS_MD_FRAGMENT; -use codex_instructions::ContextualUserFragmentDefinition; -use codex_instructions::SKILL_FRAGMENT; use codex_protocol::items::HookPromptItem; use codex_protocol::items::parse_hook_prompt_fragment; use codex_protocol::models::ContentItem; -use codex_protocol::protocol::ENVIRONMENT_CONTEXT_CLOSE_TAG; -use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; -pub(crate) const USER_SHELL_COMMAND_OPEN_TAG: &str = ""; -pub(crate) const USER_SHELL_COMMAND_CLOSE_TAG: &str = ""; -pub(crate) const TURN_ABORTED_OPEN_TAG: &str = ""; -pub(crate) const TURN_ABORTED_CLOSE_TAG: &str = ""; -pub(crate) const SUBAGENT_NOTIFICATION_OPEN_TAG: &str = ""; -pub(crate) const SUBAGENT_NOTIFICATION_CLOSE_TAG: &str = ""; +use super::EnvironmentContext; +use super::FragmentRegistration; +use super::FragmentRegistrationProxy; +use super::SkillInstructions; +use super::SubagentNotification; +use super::TurnAborted; +use super::UserInstructions; +use super::UserShellCommand; -pub(crate) const ENVIRONMENT_CONTEXT_FRAGMENT: ContextualUserFragmentDefinition = - ContextualUserFragmentDefinition::new( - ENVIRONMENT_CONTEXT_OPEN_TAG, - ENVIRONMENT_CONTEXT_CLOSE_TAG, - ); -pub(crate) const USER_SHELL_COMMAND_FRAGMENT: ContextualUserFragmentDefinition = - ContextualUserFragmentDefinition::new( - USER_SHELL_COMMAND_OPEN_TAG, - USER_SHELL_COMMAND_CLOSE_TAG, - ); -pub(crate) const TURN_ABORTED_FRAGMENT: ContextualUserFragmentDefinition = - ContextualUserFragmentDefinition::new(TURN_ABORTED_OPEN_TAG, TURN_ABORTED_CLOSE_TAG); -pub(crate) const SUBAGENT_NOTIFICATION_FRAGMENT: ContextualUserFragmentDefinition = - ContextualUserFragmentDefinition::new( - SUBAGENT_NOTIFICATION_OPEN_TAG, - SUBAGENT_NOTIFICATION_CLOSE_TAG, - ); +static USER_INSTRUCTIONS_REGISTRATION: FragmentRegistrationProxy = + FragmentRegistrationProxy::new(); +static ENVIRONMENT_CONTEXT_REGISTRATION: FragmentRegistrationProxy = + FragmentRegistrationProxy::new(); +static SKILL_INSTRUCTIONS_REGISTRATION: FragmentRegistrationProxy = + FragmentRegistrationProxy::new(); +static USER_SHELL_COMMAND_REGISTRATION: FragmentRegistrationProxy = + FragmentRegistrationProxy::new(); +static TURN_ABORTED_REGISTRATION: FragmentRegistrationProxy = + FragmentRegistrationProxy::new(); +static SUBAGENT_NOTIFICATION_REGISTRATION: FragmentRegistrationProxy = + FragmentRegistrationProxy::new(); -const CONTEXTUAL_USER_FRAGMENTS: &[ContextualUserFragmentDefinition] = &[ - AGENTS_MD_FRAGMENT, - ENVIRONMENT_CONTEXT_FRAGMENT, - SKILL_FRAGMENT, - USER_SHELL_COMMAND_FRAGMENT, - TURN_ABORTED_FRAGMENT, - SUBAGENT_NOTIFICATION_FRAGMENT, +static CONTEXTUAL_USER_FRAGMENTS: &[&dyn FragmentRegistration] = &[ + &USER_INSTRUCTIONS_REGISTRATION, + &ENVIRONMENT_CONTEXT_REGISTRATION, + &SKILL_INSTRUCTIONS_REGISTRATION, + &USER_SHELL_COMMAND_REGISTRATION, + &TURN_ABORTED_REGISTRATION, + &SUBAGENT_NOTIFICATION_REGISTRATION, +]; + +static MEMORY_EXCLUDED_CONTEXTUAL_USER_FRAGMENTS: &[&dyn FragmentRegistration] = &[ + &USER_INSTRUCTIONS_REGISTRATION, + &SKILL_INSTRUCTIONS_REGISTRATION, ]; fn is_standard_contextual_user_text(text: &str) -> bool { CONTEXTUAL_USER_FRAGMENTS .iter() - .any(|definition| definition.matches_text(text)) + .any(|fragment| fragment.matches_text(text)) } /// Returns whether a contextual user fragment should be omitted from memory @@ -59,7 +56,9 @@ pub(crate) fn is_memory_excluded_contextual_user_fragment(content_item: &Content let ContentItem::InputText { text } = content_item else { return false; }; - AGENTS_MD_FRAGMENT.matches_text(text) || SKILL_FRAGMENT.matches_text(text) + MEMORY_EXCLUDED_CONTEXTUAL_USER_FRAGMENTS + .iter() + .any(|fragment| fragment.matches_text(text)) } pub(crate) fn is_contextual_user_fragment(content_item: &ContentItem) -> bool { diff --git a/codex-rs/core/src/contextual_user_message_tests.rs b/codex-rs/core/src/context/contextual_user_message_tests.rs similarity index 94% rename from codex-rs/core/src/contextual_user_message_tests.rs rename to codex-rs/core/src/context/contextual_user_message_tests.rs index 406f03c27f..d52cf27adb 100644 --- a/codex-rs/core/src/contextual_user_message_tests.rs +++ b/codex-rs/core/src/context/contextual_user_message_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::context::ContextualUserFragment; use codex_protocol::items::HookPromptFragment; use codex_protocol::items::build_hook_prompt_message; use codex_protocol::models::ResponseItem; @@ -20,10 +21,9 @@ fn detects_agents_instructions_fragment() { #[test] fn detects_subagent_notification_fragment_case_insensitively() { - assert!( - SUBAGENT_NOTIFICATION_FRAGMENT - .matches_text("{}") - ); + assert!(SubagentNotification::matches_text( + "{}" + )); } #[test] diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/context/environment_context.rs similarity index 61% rename from codex-rs/core/src/environment_context.rs rename to codex-rs/core/src/context/environment_context.rs index 44929b8947..c676bfb9d9 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/context/environment_context.rs @@ -1,34 +1,40 @@ -use crate::contextual_user_message::ENVIRONMENT_CONTEXT_FRAGMENT; use crate::session::turn_context::TurnContext; use crate::shell::Shell; -use codex_protocol::models::ResponseItem; use codex_protocol::protocol::TurnContextItem; use codex_protocol::protocol::TurnContextNetworkItem; -use serde::Deserialize; -use serde::Serialize; use std::path::PathBuf; -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename = "environment_context", rename_all = "snake_case")] +use super::ContextualUserFragment; + +#[derive(Debug, Clone, PartialEq)] pub(crate) struct EnvironmentContext { - pub cwd: Option, - pub shell: Shell, - pub current_date: Option, - pub timezone: Option, - pub network: Option, - pub subagents: Option, + pub(crate) cwd: Option, + pub(crate) shell: String, + pub(crate) current_date: Option, + pub(crate) timezone: Option, + pub(crate) network: Option, + pub(crate) subagents: Option, } -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Default)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] pub(crate) struct NetworkContext { allowed_domains: Vec, denied_domains: Vec, } +impl NetworkContext { + pub(crate) fn new(allowed_domains: Vec, denied_domains: Vec) -> Self { + Self { + allowed_domains, + denied_domains, + } + } +} + impl EnvironmentContext { - pub fn new( + pub(crate) fn new( cwd: Option, - shell: Shell, + shell: String, current_date: Option, timezone: Option, network: Option, @@ -47,7 +53,7 @@ impl EnvironmentContext { /// Compares two environment contexts, ignoring the shell. Useful when /// comparing turn to turn, since the initial environment_context will /// include the shell, and then it is not configurable from turn to turn. - pub fn equals_except_shell(&self, other: &EnvironmentContext) -> bool { + pub(crate) fn equals_except_shell(&self, other: &EnvironmentContext) -> bool { let EnvironmentContext { cwd, current_date, @@ -63,39 +69,34 @@ impl EnvironmentContext { && self.subagents == *subagents } - pub fn diff_from_turn_context_item( + pub(crate) fn diff_from_turn_context_item( before: &TurnContextItem, - after: &TurnContext, - shell: &Shell, + after: &EnvironmentContext, ) -> Self { let before_network = Self::network_from_turn_context_item(before); - let after_network = Self::network_from_turn_context(after); - let cwd = if before.cwd.as_path() != after.cwd.as_path() { - Some(after.cwd.to_path_buf()) - } else { - None + let cwd = match &after.cwd { + Some(cwd) if before.cwd.as_path() != cwd.as_path() => Some(cwd.clone()), + _ => None, }; - let current_date = after.current_date.clone(); - let timezone = after.timezone.clone(); - let network = if before_network != after_network { - after_network + let network = if before_network != after.network { + after.network.clone() } else { before_network }; EnvironmentContext::new( cwd, - shell.clone(), - current_date, - timezone, + after.shell.clone(), + after.current_date.clone(), + after.timezone.clone(), network, /*subagents*/ None, ) } - pub fn from_turn_context(turn_context: &TurnContext, shell: &Shell) -> Self { + pub(crate) fn from_turn_context(turn_context: &TurnContext, shell: &Shell) -> Self { Self::new( Some(turn_context.cwd.to_path_buf()), - shell.clone(), + shell.name().to_string(), turn_context.current_date.clone(), turn_context.timezone.clone(), Self::network_from_turn_context(turn_context), @@ -103,10 +104,13 @@ impl EnvironmentContext { ) } - pub fn from_turn_context_item(turn_context_item: &TurnContextItem, shell: &Shell) -> Self { + pub(crate) fn from_turn_context_item( + turn_context_item: &TurnContextItem, + shell: String, + ) -> Self { Self::new( Some(turn_context_item.cwd.clone()), - shell.clone(), + shell, turn_context_item.current_date.clone(), turn_context_item.timezone.clone(), Self::network_from_turn_context_item(turn_context_item), @@ -114,7 +118,7 @@ impl EnvironmentContext { ) } - pub fn with_subagents(mut self, subagents: String) -> Self { + pub(crate) fn with_subagents(mut self, subagents: String) -> Self { if !subagents.is_empty() { self.subagents = Some(subagents); } @@ -129,18 +133,18 @@ impl EnvironmentContext { .network .as_ref()?; - Some(NetworkContext { - allowed_domains: network + Some(NetworkContext::new( + network .domains .as_ref() .and_then(codex_config::NetworkDomainPermissionsToml::allowed_domains) .unwrap_or_default(), - denied_domains: network + network .domains .as_ref() .and_then(codex_config::NetworkDomainPermissionsToml::denied_domains) .unwrap_or_default(), - }) + )) } fn network_from_turn_context_item( @@ -150,40 +154,33 @@ impl EnvironmentContext { allowed_domains, denied_domains, } = turn_context_item.network.as_ref()?; - Some(NetworkContext { - allowed_domains: allowed_domains.clone(), - denied_domains: denied_domains.clone(), - }) + Some(NetworkContext::new( + allowed_domains.clone(), + denied_domains.clone(), + )) } } -impl EnvironmentContext { - /// Serializes the environment context to XML. Libraries like `quick-xml` - /// require custom macros to handle Enums with newtypes, so we just do it - /// manually, to keep things simple. Output looks like: - /// - /// ```xml - /// - /// ... - /// ... - /// - /// ``` - pub fn serialize_to_xml(self) -> String { +impl ContextualUserFragment for EnvironmentContext { + const ROLE: &'static str = "user"; + const START_MARKER: &'static str = codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; + const END_MARKER: &'static str = codex_protocol::protocol::ENVIRONMENT_CONTEXT_CLOSE_TAG; + + fn body(&self) -> String { let mut lines = Vec::new(); - if let Some(cwd) = self.cwd { + if let Some(cwd) = &self.cwd { lines.push(format!(" {}", cwd.to_string_lossy())); } - let shell_name = self.shell.name(); - lines.push(format!(" {shell_name}")); - if let Some(current_date) = self.current_date { + lines.push(format!(" {}", self.shell)); + if let Some(current_date) = &self.current_date { lines.push(format!(" {current_date}")); } - if let Some(timezone) = self.timezone { + if let Some(timezone) = &self.timezone { lines.push(format!(" {timezone}")); } - match self.network { - Some(ref network) => { + match &self.network { + Some(network) => { lines.push(" ".to_string()); for allowed in &network.allowed_domains { lines.push(format!(" {allowed}")); @@ -198,18 +195,12 @@ impl EnvironmentContext { // lines.push(" ".to_string()); } } - if let Some(subagents) = self.subagents { + if let Some(subagents) = &self.subagents { lines.push(" ".to_string()); lines.extend(subagents.lines().map(|line| format!(" {line}"))); lines.push(" ".to_string()); } - ENVIRONMENT_CONTEXT_FRAGMENT.wrap(lines.join("\n")) - } -} - -impl From for ResponseItem { - fn from(ec: EnvironmentContext) -> Self { - ENVIRONMENT_CONTEXT_FRAGMENT.into_message(ec.serialize_to_xml()) + format!("\n{}", lines.join("\n")) } } diff --git a/codex-rs/core/src/environment_context_tests.rs b/codex-rs/core/src/context/environment_context_tests.rs similarity index 56% rename from codex-rs/core/src/environment_context_tests.rs rename to codex-rs/core/src/context/environment_context_tests.rs index 073f1fe169..84f8c0d99f 100644 --- a/codex-rs/core/src/environment_context_tests.rs +++ b/codex-rs/core/src/context/environment_context_tests.rs @@ -3,13 +3,15 @@ use crate::shell::ShellType; use super::*; use core_test_support::test_path_buf; use pretty_assertions::assert_eq; +use std::path::PathBuf; -fn fake_shell() -> Shell { - Shell { +fn fake_shell_name() -> String { + let shell = crate::shell::Shell { shell_type: ShellType::Bash, shell_path: PathBuf::from("/bin/bash"), shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - } + }; + shell.name().to_string() } #[test] @@ -17,7 +19,7 @@ fn serialize_workspace_write_environment_context() { let cwd = test_path_buf("/repo"); let context = EnvironmentContext::new( Some(cwd.clone()), - fake_shell(), + fake_shell_name(), Some("2026-02-26".to_string()), Some("America/Los_Angeles".to_string()), /*network*/ None, @@ -34,18 +36,18 @@ fn serialize_workspace_write_environment_context() { cwd = cwd.display(), ); - assert_eq!(context.serialize_to_xml(), expected); + assert_eq!(context.render(), expected); } #[test] fn serialize_environment_context_with_network() { - let network = NetworkContext { - allowed_domains: vec!["api.example.com".to_string(), "*.openai.com".to_string()], - denied_domains: vec!["blocked.example.com".to_string()], - }; + let network = NetworkContext::new( + vec!["api.example.com".to_string(), "*.openai.com".to_string()], + vec!["blocked.example.com".to_string()], + ); let context = EnvironmentContext::new( Some(test_path_buf("/repo")), - fake_shell(), + fake_shell_name(), Some("2026-02-26".to_string()), Some("America/Los_Angeles".to_string()), Some(network), @@ -67,14 +69,14 @@ fn serialize_environment_context_with_network() { test_path_buf("/repo").display() ); - assert_eq!(context.serialize_to_xml(), expected); + assert_eq!(context.render(), expected); } #[test] fn serialize_read_only_environment_context() { let context = EnvironmentContext::new( /*cwd*/ None, - fake_shell(), + fake_shell_name(), Some("2026-02-26".to_string()), Some("America/Los_Angeles".to_string()), /*network*/ None, @@ -87,74 +89,14 @@ fn serialize_read_only_environment_context() { America/Los_Angeles "#; - assert_eq!(context.serialize_to_xml(), expected); -} - -#[test] -fn serialize_external_sandbox_environment_context() { - let context = EnvironmentContext::new( - /*cwd*/ None, - fake_shell(), - Some("2026-02-26".to_string()), - Some("America/Los_Angeles".to_string()), - /*network*/ None, - /*subagents*/ None, - ); - - let expected = r#" - bash - 2026-02-26 - America/Los_Angeles -"#; - - assert_eq!(context.serialize_to_xml(), expected); -} - -#[test] -fn serialize_external_sandbox_with_restricted_network_environment_context() { - let context = EnvironmentContext::new( - /*cwd*/ None, - fake_shell(), - Some("2026-02-26".to_string()), - Some("America/Los_Angeles".to_string()), - /*network*/ None, - /*subagents*/ None, - ); - - let expected = r#" - bash - 2026-02-26 - America/Los_Angeles -"#; - - assert_eq!(context.serialize_to_xml(), expected); -} - -#[test] -fn serialize_full_access_environment_context() { - let context = EnvironmentContext::new( - /*cwd*/ None, - fake_shell(), - Some("2026-02-26".to_string()), - Some("America/Los_Angeles".to_string()), - /*network*/ None, - /*subagents*/ None, - ); - - let expected = r#" - bash - 2026-02-26 - America/Los_Angeles -"#; - - assert_eq!(context.serialize_to_xml(), expected); + assert_eq!(context.render(), expected); } #[test] fn equals_except_shell_compares_cwd() { let context1 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - fake_shell(), + fake_shell_name(), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -162,7 +104,7 @@ fn equals_except_shell_compares_cwd() { ); let context2 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - fake_shell(), + fake_shell_name(), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -171,33 +113,11 @@ fn equals_except_shell_compares_cwd() { assert!(context1.equals_except_shell(&context2)); } -#[test] -fn equals_except_shell_ignores_sandbox_policy() { - let context1 = EnvironmentContext::new( - Some(PathBuf::from("/repo")), - fake_shell(), - /*current_date*/ None, - /*timezone*/ None, - /*network*/ None, - /*subagents*/ None, - ); - let context2 = EnvironmentContext::new( - Some(PathBuf::from("/repo")), - fake_shell(), - /*current_date*/ None, - /*timezone*/ None, - /*network*/ None, - /*subagents*/ None, - ); - - assert!(context1.equals_except_shell(&context2)); -} - #[test] fn equals_except_shell_compares_cwd_differences() { let context1 = EnvironmentContext::new( Some(PathBuf::from("/repo1")), - fake_shell(), + fake_shell_name(), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -205,7 +125,7 @@ fn equals_except_shell_compares_cwd_differences() { ); let context2 = EnvironmentContext::new( Some(PathBuf::from("/repo2")), - fake_shell(), + fake_shell_name(), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -219,11 +139,7 @@ fn equals_except_shell_compares_cwd_differences() { fn equals_except_shell_ignores_shell() { let context1 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - Shell { - shell_type: ShellType::Bash, - shell_path: "/bin/bash".into(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }, + "bash".to_string(), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -231,11 +147,7 @@ fn equals_except_shell_ignores_shell() { ); let context2 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - Shell { - shell_type: ShellType::Zsh, - shell_path: "/bin/zsh".into(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }, + "zsh".to_string(), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -249,7 +161,7 @@ fn equals_except_shell_ignores_shell() { fn serialize_environment_context_with_subagents() { let context = EnvironmentContext::new( Some(test_path_buf("/repo")), - fake_shell(), + fake_shell_name(), Some("2026-02-26".to_string()), Some("America/Los_Angeles".to_string()), /*network*/ None, @@ -270,5 +182,5 @@ fn serialize_environment_context_with_subagents() { test_path_buf("/repo").display() ); - assert_eq!(context.serialize_to_xml(), expected); + assert_eq!(context.render(), expected); } diff --git a/codex-rs/core/src/context/fragment.rs b/codex-rs/core/src/context/fragment.rs new file mode 100644 index 0000000000..e0e6d03303 --- /dev/null +++ b/codex-rs/core/src/context/fragment.rs @@ -0,0 +1,82 @@ +use codex_protocol::models::ContentItem; +use codex_protocol::models::ResponseItem; +use std::marker::PhantomData; + +/// Type-erased registration for a contextual user fragment. +/// +/// Implementations are used by context filtering code to recognize injected +/// fragments without constructing the concrete context payload. +pub(crate) trait FragmentRegistration: Sync { + fn matches_text(&self, text: &str) -> bool; +} + +pub(crate) struct FragmentRegistrationProxy { + _marker: PhantomData T>, +} + +impl FragmentRegistrationProxy { + pub(crate) const fn new() -> Self { + Self { + _marker: PhantomData, + } + } +} + +impl FragmentRegistration for FragmentRegistrationProxy { + fn matches_text(&self, text: &str) -> bool { + T::matches_text(text) + } +} + +/// Context payload that is injected as a user-authored message fragment. +/// +/// Implementations own the response role, start/end markers used to recognize +/// the fragment, and provide the fragment body appended directly after the +/// start marker. The default helpers wrap that body and convert it into the +/// response item shape expected by model input assembly. +pub(crate) trait ContextualUserFragment { + const ROLE: &'static str; + const START_MARKER: &'static str; + const END_MARKER: &'static str; + + fn body(&self) -> String; + + fn matches_text(text: &str) -> bool + where + Self: Sized, + { + let trimmed = text.trim_start(); + let starts_with_marker = trimmed + .get(..Self::START_MARKER.len()) + .is_some_and(|candidate| candidate.eq_ignore_ascii_case(Self::START_MARKER)); + let trimmed = trimmed.trim_end(); + let ends_with_marker = trimmed + .get(trimmed.len().saturating_sub(Self::END_MARKER.len())..) + .is_some_and(|candidate| candidate.eq_ignore_ascii_case(Self::END_MARKER)); + starts_with_marker && ends_with_marker + } + + fn render(&self) -> String { + format!( + "{}{}\n{}", + Self::START_MARKER, + self.body(), + Self::END_MARKER + ) + } + + fn into(self) -> ResponseItem + where + Self: Sized, + { + ResponseItem::Message { + id: None, + role: Self::ROLE.to_string(), + content: vec![ContentItem::InputText { + text: self.render(), + }], + end_turn: None, + phase: None, + } + } +} diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs new file mode 100644 index 0000000000..94c5849f9d --- /dev/null +++ b/codex-rs/core/src/context/mod.rs @@ -0,0 +1,21 @@ +mod contextual_user_message; +mod environment_context; +mod fragment; +mod skill_instructions; +mod subagent_notification; +mod turn_aborted; +mod user_instructions; +mod user_shell_command; + +pub(crate) use contextual_user_message::is_contextual_user_fragment; +pub(crate) use contextual_user_message::is_memory_excluded_contextual_user_fragment; +pub(crate) use contextual_user_message::parse_visible_hook_prompt_message; +pub(crate) use environment_context::EnvironmentContext; +pub(crate) use fragment::ContextualUserFragment; +pub(crate) use fragment::FragmentRegistration; +pub(crate) use fragment::FragmentRegistrationProxy; +pub(crate) use skill_instructions::SkillInstructions; +pub(crate) use subagent_notification::SubagentNotification; +pub(crate) use turn_aborted::TurnAborted; +pub(crate) use user_instructions::UserInstructions; +pub(crate) use user_shell_command::UserShellCommand; diff --git a/codex-rs/core/src/context/skill_instructions.rs b/codex-rs/core/src/context/skill_instructions.rs new file mode 100644 index 0000000000..90271b807e --- /dev/null +++ b/codex-rs/core/src/context/skill_instructions.rs @@ -0,0 +1,33 @@ +use codex_core_skills::injection::SkillInjection; + +use super::ContextualUserFragment; + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct SkillInstructions { + pub(crate) name: String, + pub(crate) path: String, + pub(crate) contents: String, +} + +impl From<&SkillInjection> for SkillInstructions { + fn from(skill: &SkillInjection) -> Self { + Self { + name: skill.name.clone(), + path: skill.path.clone(), + contents: skill.contents.clone(), + } + } +} + +impl ContextualUserFragment for SkillInstructions { + const ROLE: &'static str = "user"; + const START_MARKER: &'static str = ""; + const END_MARKER: &'static str = ""; + + fn body(&self) -> String { + format!( + "\n{}\n{}\n{}", + self.name, self.path, self.contents + ) + } +} diff --git a/codex-rs/core/src/context/subagent_notification.rs b/codex-rs/core/src/context/subagent_notification.rs new file mode 100644 index 0000000000..16f7fb4747 --- /dev/null +++ b/codex-rs/core/src/context/subagent_notification.rs @@ -0,0 +1,34 @@ +use codex_protocol::protocol::AgentStatus; + +use super::ContextualUserFragment; + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct SubagentNotification { + pub(crate) agent_reference: String, + pub(crate) status: AgentStatus, +} + +impl SubagentNotification { + pub(crate) fn new(agent_reference: impl Into, status: AgentStatus) -> Self { + Self { + agent_reference: agent_reference.into(), + status, + } + } +} + +impl ContextualUserFragment for SubagentNotification { + const ROLE: &'static str = "user"; + const START_MARKER: &'static str = ""; + const END_MARKER: &'static str = ""; + + fn body(&self) -> String { + format!( + "\n{}", + serde_json::json!({ + "agent_path": &self.agent_reference, + "status": &self.status, + }) + ) + } +} diff --git a/codex-rs/core/src/context/turn_aborted.rs b/codex-rs/core/src/context/turn_aborted.rs new file mode 100644 index 0000000000..69620c0042 --- /dev/null +++ b/codex-rs/core/src/context/turn_aborted.rs @@ -0,0 +1,26 @@ +use super::ContextualUserFragment; + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct TurnAborted { + pub(crate) guidance: String, +} + +impl TurnAborted { + pub(crate) const INTERRUPTED_GUIDANCE: &'static str = "The user interrupted the previous turn on purpose. Any running unified exec processes may still be running in the background. If any tools/commands were aborted, they may have partially executed."; + + pub(crate) fn new(guidance: impl Into) -> Self { + Self { + guidance: guidance.into(), + } + } +} + +impl ContextualUserFragment for TurnAborted { + const ROLE: &'static str = "user"; + const START_MARKER: &'static str = ""; + const END_MARKER: &'static str = ""; + + fn body(&self) -> String { + format!("\n{}", self.guidance) + } +} diff --git a/codex-rs/core/src/context/user_instructions.rs b/codex-rs/core/src/context/user_instructions.rs new file mode 100644 index 0000000000..97efc4f45c --- /dev/null +++ b/codex-rs/core/src/context/user_instructions.rs @@ -0,0 +1,17 @@ +use super::ContextualUserFragment; + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct UserInstructions { + pub(crate) directory: String, + pub(crate) text: String, +} + +impl ContextualUserFragment for UserInstructions { + const ROLE: &'static str = "user"; + const START_MARKER: &'static str = "# AGENTS.md instructions for "; + const END_MARKER: &'static str = ""; + + fn body(&self) -> String { + format!("{}\n\n\n{}", self.directory, self.text) + } +} diff --git a/codex-rs/core/src/context/user_shell_command.rs b/codex-rs/core/src/context/user_shell_command.rs new file mode 100644 index 0000000000..8bbbb952d8 --- /dev/null +++ b/codex-rs/core/src/context/user_shell_command.rs @@ -0,0 +1,40 @@ +use std::time::Duration; + +use super::ContextualUserFragment; + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct UserShellCommand { + pub(crate) command: String, + pub(crate) exit_code: i32, + pub(crate) duration_seconds: f64, + pub(crate) output: String, +} + +impl UserShellCommand { + pub(crate) fn new( + command: impl Into, + exit_code: i32, + duration: Duration, + output: impl Into, + ) -> Self { + Self { + command: command.into(), + exit_code, + duration_seconds: duration.as_secs_f64(), + output: output.into(), + } + } +} + +impl ContextualUserFragment for UserShellCommand { + const ROLE: &'static str = "user"; + const START_MARKER: &'static str = ""; + const END_MARKER: &'static str = ""; + + fn body(&self) -> String { + format!( + "\n\n{}\n\n\nExit code: {}\nDuration: {:.4} seconds\nOutput:\n{}\n", + self.command, self.exit_code, self.duration_seconds, self.output, + ) + } +} diff --git a/codex-rs/core/src/context_manager/updates.rs b/codex-rs/core/src/context_manager/updates.rs index 5ced481530..a3ccf49f83 100644 --- a/codex-rs/core/src/context_manager/updates.rs +++ b/codex-rs/core/src/context_manager/updates.rs @@ -1,4 +1,5 @@ -use crate::environment_context::EnvironmentContext; +use crate::context::ContextualUserFragment; +use crate::context::EnvironmentContext; use crate::session::PreviousTurnSettings; use crate::session::turn_context::TurnContext; use crate::shell::Shell; @@ -21,14 +22,14 @@ fn build_environment_update_item( } let prev = previous?; - let prev_context = EnvironmentContext::from_turn_context_item(prev, shell); + let prev_context = EnvironmentContext::from_turn_context_item(prev, shell.name().to_string()); let next_context = EnvironmentContext::from_turn_context(next, shell); if prev_context.equals_except_shell(&next_context) { return None; } - Some(ResponseItem::from( - EnvironmentContext::diff_from_turn_context_item(prev, next, shell), + Some(ContextualUserFragment::into( + EnvironmentContext::diff_from_turn_context_item(prev, &next_context), )) } diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 21e13f6c15..e7c79e6dd2 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -20,8 +20,8 @@ use codex_protocol::user_input::UserInput; use tracing::warn; use uuid::Uuid; -use crate::contextual_user_message::is_contextual_user_fragment; -use crate::contextual_user_message::parse_visible_hook_prompt_message; +use crate::context::is_contextual_user_fragment; +use crate::context::parse_visible_hook_prompt_message; use crate::web_search::web_search_action_detail; const CONTEXTUAL_DEVELOPER_PREFIXES: &[&str] = &[ diff --git a/codex-rs/core/src/instructions/mod.rs b/codex-rs/core/src/instructions/mod.rs deleted file mode 100644 index a1f77ba5e8..0000000000 --- a/codex-rs/core/src/instructions/mod.rs +++ /dev/null @@ -1 +0,0 @@ -pub(crate) use codex_instructions::UserInstructions; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 1809789a8e..fc96aceaf9 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -27,9 +27,8 @@ mod commit_attribution; pub mod config; pub mod config_loader; pub mod connectors; +mod context; mod context_manager; -mod contextual_user_message; -mod environment_context; pub mod exec; pub mod exec_env; mod exec_policy; @@ -41,7 +40,6 @@ mod git_info_tests; mod guardian; mod hook_runtime; mod installation_id; -pub(crate) mod instructions; pub(crate) mod landlock; pub use landlock::spawn_command_under_linux_sandbox; pub(crate) mod mcp; diff --git a/codex-rs/core/src/memories/phase1.rs b/codex-rs/core/src/memories/phase1.rs index 9d46374f68..4f19caa5c5 100644 --- a/codex-rs/core/src/memories/phase1.rs +++ b/codex-rs/core/src/memories/phase1.rs @@ -1,7 +1,7 @@ use crate::Prompt; use crate::RolloutRecorder; use crate::config::Config; -use crate::contextual_user_message::is_memory_excluded_contextual_user_fragment; +use crate::context::is_memory_excluded_contextual_user_fragment; use crate::memories::metrics; use crate::memories::phase_one; use crate::memories::phase_one::PRUNE_BATCH_SIZE; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index f1fea3517a..3ea826c491 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -19,6 +19,7 @@ use crate::commit_attribution::commit_message_trailer_instruction; use crate::compact; use crate::config::ManagedFeatures; use crate::connectors; +use crate::context::ContextualUserFragment; use crate::default_skill_metadata_budget; use crate::exec_policy::ExecPolicyManager; use crate::installation_id::resolve_installation_id; @@ -153,7 +154,6 @@ use crate::config::StartedNetworkProxy; use crate::config::resolve_web_search_mode_for_turn; use crate::context_manager::ContextManager; use crate::context_manager::TotalTokenUsageBreakdown; -use crate::environment_context::EnvironmentContext; use crate::thread_rollout_truncation::initial_history_has_prior_user_turns; use codex_config::CONFIG_TOML_FILE; use codex_config::types::McpServerConfig; @@ -249,9 +249,9 @@ use crate::SkillLoadOutcome; use crate::SkillMetadata; use crate::SkillsManager; use crate::agents_md::AgentsMdManager; +use crate::context::UserInstructions; use crate::exec_policy::ExecPolicyUpdateError; use crate::guardian::GuardianReviewSessionManager; -use crate::instructions::UserInstructions; use crate::mcp::McpManager; use crate::memories; use crate::network_policy_decision::execpolicy_network_rule_amendment; @@ -2487,7 +2487,7 @@ impl Session { text: user_instructions.to_string(), directory: turn_context.cwd.to_string_lossy().into_owned(), } - .serialize_to_text(), + .render(), ); } if turn_context.config.include_environment_context { @@ -2497,9 +2497,9 @@ impl Session { .format_environment_context_subagents(self.conversation_id) .await; contextual_user_sections.push( - EnvironmentContext::from_turn_context(turn_context, shell.as_ref()) + crate::context::EnvironmentContext::from_turn_context(turn_context, shell.as_ref()) .with_subagents(subagents) - .serialize_to_xml(), + .render(), ); } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index f901e5bee4..18cefba934 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -11,6 +11,8 @@ use crate::config_loader::NetworkDomainPermissionsToml; use crate::config_loader::RequirementSource; use crate::config_loader::Sourced; use crate::config_loader::project_trust_key; +use crate::context::ContextualUserFragment; +use crate::context::TurnAborted; use crate::exec::ExecCapturePolicy; use crate::function_tool::FunctionCallError; use crate::shell::default_user_shell; @@ -6349,7 +6351,7 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { let ContentItem::InputText { text } = content_item else { return false; }; - text.contains(crate::contextual_user_message::TURN_ABORTED_OPEN_TAG) + TurnAborted::matches_text(text) }) }), "expected a model-visible turn aborted marker in history after interrupt" diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index efbcca6716..f613802244 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -16,6 +16,7 @@ use crate::compact::run_inline_auto_compact_task; use crate::compact::should_use_remote_compact_task; use crate::compact_remote::run_inline_remote_auto_compact_task; use crate::connectors; +use crate::context::ContextualUserFragment; use crate::feedback_tags; use crate::hook_runtime::PendingInputHookDisposition; use crate::hook_runtime::emit_hook_completed_events; @@ -241,7 +242,7 @@ pub(crate) async fn run_turn( turn_context.sub_id.clone(), ); let SkillInjections { - items: skill_items, + items: skill_injections, warnings: skill_warnings, } = build_skill_injections( &mentioned_skills, @@ -257,6 +258,11 @@ pub(crate) async fn run_turn( .await; } + let skill_items: Vec = skill_injections + .iter() + .map(|skill| ContextualUserFragment::into(crate::context::SkillInstructions::from(skill))) + .collect(); + let plugin_items = build_plugin_injections(&mentioned_plugins, &mcp_tools, &available_connectors); let mentioned_plugin_metadata = mentioned_plugins diff --git a/codex-rs/core/src/session_prefix.rs b/codex-rs/core/src/session_prefix.rs index 42f213a1d3..b50d1f9070 100644 --- a/codex-rs/core/src/session_prefix.rs +++ b/codex-rs/core/src/session_prefix.rs @@ -1,20 +1,17 @@ use codex_protocol::protocol::AgentStatus; -/// Helpers for model-visible session state markers that are stored in user-role -/// messages but are not user intent. -use crate::contextual_user_message::SUBAGENT_NOTIFICATION_FRAGMENT; +use crate::context::ContextualUserFragment; +use crate::context::SubagentNotification; + +// Helpers for model-visible session state markers that are stored in user-role +// messages but are not user intent. // TODO(jif) unify with structured schema pub(crate) fn format_subagent_notification_message( agent_reference: &str, status: &AgentStatus, ) -> String { - let payload_json = serde_json::json!({ - "agent_path": agent_reference, - "status": status, - }) - .to_string(); - SUBAGENT_NOTIFICATION_FRAGMENT.wrap(payload_json) + SubagentNotification::new(agent_reference, status.clone()).render() } pub(crate) fn format_subagent_context_line( diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index e7030d1d94..9c19087204 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -19,8 +19,7 @@ use tracing::info_span; use tracing::trace; use tracing::warn; -use crate::contextual_user_message::TURN_ABORTED_CLOSE_TAG; -use crate::contextual_user_message::TURN_ABORTED_OPEN_TAG; +use crate::context::ContextualUserFragment; use crate::hook_runtime::PendingInputHookDisposition; use crate::hook_runtime::inspect_pending_input; use crate::hook_runtime::record_additional_contexts; @@ -39,7 +38,6 @@ use codex_otel::TURN_MEMORY_METRIC; use codex_otel::TURN_NETWORK_PROXY_METRIC; use codex_otel::TURN_TOKEN_USAGE_METRIC; use codex_otel::TURN_TOOL_CALL_METRIC; -use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::EventMsg; @@ -62,22 +60,13 @@ pub(crate) use user_shell::UserShellCommandTask; pub(crate) use user_shell::execute_user_shell_command; const GRACEFULL_INTERRUPTION_TIMEOUT_MS: u64 = 100; -const TURN_ABORTED_INTERRUPTED_GUIDANCE: &str = "The user interrupted the previous turn on purpose. Any running unified exec processes may still be running in the background. If any tools/commands were aborted, they may have partially executed."; /// Shared model-visible marker used by both the real interrupt path and /// interrupted fork snapshots. pub(crate) fn interrupted_turn_history_marker() -> ResponseItem { - ResponseItem::Message { - id: None, - role: "user".to_string(), - content: vec![ContentItem::InputText { - text: format!( - "{TURN_ABORTED_OPEN_TAG}\n{TURN_ABORTED_INTERRUPTED_GUIDANCE}\n{TURN_ABORTED_CLOSE_TAG}" - ), - }], - end_turn: None, - phase: None, - } + ContextualUserFragment::into(crate::context::TurnAborted::new( + crate::context::TurnAborted::INTERRUPTED_GUIDANCE, + )) } fn emit_turn_network_proxy_metric( diff --git a/codex-rs/core/src/user_shell_command.rs b/codex-rs/core/src/user_shell_command.rs index e244be0d2a..cf034faca0 100644 --- a/codex-rs/core/src/user_shell_command.rs +++ b/codex-rs/core/src/user_shell_command.rs @@ -1,45 +1,27 @@ -use std::time::Duration; - use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::ResponseItem; -use crate::contextual_user_message::USER_SHELL_COMMAND_FRAGMENT; +use crate::context::ContextualUserFragment; +use crate::context::UserShellCommand; use crate::session::turn_context::TurnContext; use crate::tools::format_exec_output_str; -fn format_duration_line(duration: Duration) -> String { - let duration_seconds = duration.as_secs_f64(); - format!("Duration: {duration_seconds:.4} seconds") -} - -fn format_user_shell_command_body( +fn user_shell_command_fragment( command: &str, exec_output: &ExecToolCallOutput, turn_context: &TurnContext, -) -> String { - let mut sections = Vec::new(); - sections.push("".to_string()); - sections.push(command.to_string()); - sections.push("".to_string()); - sections.push("".to_string()); - sections.push(format!("Exit code: {}", exec_output.exit_code)); - sections.push(format_duration_line(exec_output.duration)); - sections.push("Output:".to_string()); - sections.push(format_exec_output_str( - exec_output, - turn_context.truncation_policy, - )); - sections.push("".to_string()); - sections.join("\n") +) -> UserShellCommand { + let output = format_exec_output_str(exec_output, turn_context.truncation_policy); + UserShellCommand::new(command, exec_output.exit_code, exec_output.duration, output) } +#[cfg(test)] pub fn format_user_shell_command_record( command: &str, exec_output: &ExecToolCallOutput, turn_context: &TurnContext, ) -> String { - let body = format_user_shell_command_body(command, exec_output, turn_context); - USER_SHELL_COMMAND_FRAGMENT.wrap(body) + user_shell_command_fragment(command, exec_output, turn_context).render() } pub fn user_shell_command_record_item( @@ -47,7 +29,7 @@ pub fn user_shell_command_record_item( exec_output: &ExecToolCallOutput, turn_context: &TurnContext, ) -> ResponseItem { - USER_SHELL_COMMAND_FRAGMENT.into_message(format_user_shell_command_record( + ContextualUserFragment::into(user_shell_command_fragment( command, exec_output, turn_context, diff --git a/codex-rs/core/src/user_shell_command_tests.rs b/codex-rs/core/src/user_shell_command_tests.rs index 7e68ca5fee..996d66919a 100644 --- a/codex-rs/core/src/user_shell_command_tests.rs +++ b/codex-rs/core/src/user_shell_command_tests.rs @@ -1,16 +1,18 @@ use super::*; +use crate::context::ContextualUserFragment; +use crate::context::UserShellCommand; use crate::session::tests::make_session_and_context; use codex_protocol::exec_output::StreamOutput; use codex_protocol::models::ContentItem; use pretty_assertions::assert_eq; +use std::time::Duration; #[test] fn detects_user_shell_command_text_variants() { - assert!( - USER_SHELL_COMMAND_FRAGMENT - .matches_text("\necho hi\n") - ); - assert!(!USER_SHELL_COMMAND_FRAGMENT.matches_text("echo hi")); + assert!(UserShellCommand::matches_text( + "\necho hi\n" + )); + assert!(!UserShellCommand::matches_text("echo hi")); } #[tokio::test] diff --git a/codex-rs/instructions/BUILD.bazel b/codex-rs/instructions/BUILD.bazel deleted file mode 100644 index d3cdbd19d1..0000000000 --- a/codex-rs/instructions/BUILD.bazel +++ /dev/null @@ -1,16 +0,0 @@ -load("//:defs.bzl", "codex_rust_crate") - -codex_rust_crate( - name = "instructions", - crate_name = "codex_instructions", - compile_data = glob( - include = ["**"], - exclude = [ - "BUILD.bazel", - "Cargo.toml", - ], - allow_empty = True, - ) + [ - "//codex-rs:node-version.txt", - ], -) diff --git a/codex-rs/instructions/Cargo.toml b/codex-rs/instructions/Cargo.toml deleted file mode 100644 index cdaa3d7368..0000000000 --- a/codex-rs/instructions/Cargo.toml +++ /dev/null @@ -1,20 +0,0 @@ -[package] -edition.workspace = true -license.workspace = true -name = "codex-instructions" -version.workspace = true - -[lib] -doctest = false -name = "codex_instructions" -path = "src/lib.rs" - -[lints] -workspace = true - -[dependencies] -codex-protocol = { workspace = true } -serde = { workspace = true, features = ["derive"] } - -[dev-dependencies] -pretty_assertions = { workspace = true } diff --git a/codex-rs/instructions/src/fragment.rs b/codex-rs/instructions/src/fragment.rs deleted file mode 100644 index 590dac379b..0000000000 --- a/codex-rs/instructions/src/fragment.rs +++ /dev/null @@ -1,61 +0,0 @@ -use codex_protocol::models::ContentItem; -use codex_protocol::models::ResponseItem; - -pub(crate) const AGENTS_MD_START_MARKER: &str = "# AGENTS.md instructions for "; -pub(crate) const AGENTS_MD_END_MARKER: &str = ""; -pub(crate) const SKILL_OPEN_TAG: &str = ""; -pub(crate) const SKILL_CLOSE_TAG: &str = ""; - -#[derive(Clone, Copy)] -pub struct ContextualUserFragmentDefinition { - start_marker: &'static str, - end_marker: &'static str, -} - -impl ContextualUserFragmentDefinition { - pub const fn new(start_marker: &'static str, end_marker: &'static str) -> Self { - Self { - start_marker, - end_marker, - } - } - - pub fn matches_text(&self, text: &str) -> bool { - let trimmed = text.trim_start(); - let starts_with_marker = trimmed - .get(..self.start_marker.len()) - .is_some_and(|candidate| candidate.eq_ignore_ascii_case(self.start_marker)); - let trimmed = trimmed.trim_end(); - let ends_with_marker = trimmed - .get(trimmed.len().saturating_sub(self.end_marker.len())..) - .is_some_and(|candidate| candidate.eq_ignore_ascii_case(self.end_marker)); - starts_with_marker && ends_with_marker - } - - pub const fn start_marker(&self) -> &'static str { - self.start_marker - } - - pub const fn end_marker(&self) -> &'static str { - self.end_marker - } - - pub fn wrap(&self, body: String) -> String { - format!("{}\n{}\n{}", self.start_marker, body, self.end_marker) - } - - pub fn into_message(self, text: String) -> ResponseItem { - ResponseItem::Message { - id: None, - role: "user".to_string(), - content: vec![ContentItem::InputText { text }], - end_turn: None, - phase: None, - } - } -} - -pub const AGENTS_MD_FRAGMENT: ContextualUserFragmentDefinition = - ContextualUserFragmentDefinition::new(AGENTS_MD_START_MARKER, AGENTS_MD_END_MARKER); -pub const SKILL_FRAGMENT: ContextualUserFragmentDefinition = - ContextualUserFragmentDefinition::new(SKILL_OPEN_TAG, SKILL_CLOSE_TAG); diff --git a/codex-rs/instructions/src/lib.rs b/codex-rs/instructions/src/lib.rs deleted file mode 100644 index 1b1f81550c..0000000000 --- a/codex-rs/instructions/src/lib.rs +++ /dev/null @@ -1,11 +0,0 @@ -//! User and skill instruction payloads and contextual user fragment markers for Codex prompts. - -mod fragment; -mod user_instructions; - -pub use fragment::AGENTS_MD_FRAGMENT; -pub use fragment::ContextualUserFragmentDefinition; -pub use fragment::SKILL_FRAGMENT; -pub use user_instructions::SkillInstructions; -pub use user_instructions::USER_INSTRUCTIONS_PREFIX; -pub use user_instructions::UserInstructions; diff --git a/codex-rs/instructions/src/user_instructions.rs b/codex-rs/instructions/src/user_instructions.rs deleted file mode 100644 index 4fd266e766..0000000000 --- a/codex-rs/instructions/src/user_instructions.rs +++ /dev/null @@ -1,56 +0,0 @@ -use serde::Deserialize; -use serde::Serialize; - -use codex_protocol::models::ResponseItem; - -use crate::fragment::AGENTS_MD_FRAGMENT; -use crate::fragment::AGENTS_MD_START_MARKER; -use crate::fragment::SKILL_FRAGMENT; - -pub const USER_INSTRUCTIONS_PREFIX: &str = AGENTS_MD_START_MARKER; - -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename = "user_instructions", rename_all = "snake_case")] -pub struct UserInstructions { - pub directory: String, - pub text: String, -} - -impl UserInstructions { - pub fn serialize_to_text(&self) -> String { - format!( - "{prefix}{directory}\n\n\n{contents}\n{suffix}", - prefix = AGENTS_MD_FRAGMENT.start_marker(), - directory = self.directory, - contents = self.text, - suffix = AGENTS_MD_FRAGMENT.end_marker(), - ) - } -} - -impl From for ResponseItem { - fn from(ui: UserInstructions) -> Self { - AGENTS_MD_FRAGMENT.into_message(ui.serialize_to_text()) - } -} - -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename = "skill_instructions", rename_all = "snake_case")] -pub struct SkillInstructions { - pub name: String, - pub path: String, - pub contents: String, -} - -impl From for ResponseItem { - fn from(si: SkillInstructions) -> Self { - SKILL_FRAGMENT.into_message(SKILL_FRAGMENT.wrap(format!( - "{}\n{}\n{}", - si.name, si.path, si.contents - ))) - } -} - -#[cfg(test)] -#[path = "user_instructions_tests.rs"] -mod tests; diff --git a/codex-rs/instructions/src/user_instructions_tests.rs b/codex-rs/instructions/src/user_instructions_tests.rs deleted file mode 100644 index 75f35d11b0..0000000000 --- a/codex-rs/instructions/src/user_instructions_tests.rs +++ /dev/null @@ -1,72 +0,0 @@ -use super::*; -use codex_protocol::models::ContentItem; -use codex_protocol::models::ResponseItem; -use pretty_assertions::assert_eq; - -use crate::fragment::AGENTS_MD_FRAGMENT; -use crate::fragment::SKILL_FRAGMENT; - -#[test] -fn test_user_instructions() { - let user_instructions = UserInstructions { - directory: "test_directory".to_string(), - text: "test_text".to_string(), - }; - let response_item: ResponseItem = user_instructions.into(); - - let ResponseItem::Message { role, content, .. } = response_item else { - panic!("expected ResponseItem::Message"); - }; - - assert_eq!(role, "user"); - - let [ContentItem::InputText { text }] = content.as_slice() else { - panic!("expected one InputText content item"); - }; - - assert_eq!( - text, - "# AGENTS.md instructions for test_directory\n\n\ntest_text\n", - ); -} - -#[test] -fn test_is_user_instructions() { - assert!(AGENTS_MD_FRAGMENT.matches_text( - "# AGENTS.md instructions for test_directory\n\n\ntest_text\n" - )); - assert!(!AGENTS_MD_FRAGMENT.matches_text("test_text")); -} - -#[test] -fn test_skill_instructions() { - let skill_instructions = SkillInstructions { - name: "demo-skill".to_string(), - path: "skills/demo/SKILL.md".to_string(), - contents: "body".to_string(), - }; - let response_item: ResponseItem = skill_instructions.into(); - - let ResponseItem::Message { role, content, .. } = response_item else { - panic!("expected ResponseItem::Message"); - }; - - assert_eq!(role, "user"); - - let [ContentItem::InputText { text }] = content.as_slice() else { - panic!("expected one InputText content item"); - }; - - assert_eq!( - text, - "\ndemo-skill\nskills/demo/SKILL.md\nbody\n", - ); -} - -#[test] -fn test_is_skill_instructions() { - assert!(SKILL_FRAGMENT.matches_text( - "\ndemo-skill\nskills/demo/SKILL.md\nbody\n" - )); - assert!(!SKILL_FRAGMENT.matches_text("regular text")); -} From d62421d32299aa5fdc30b131471eb06f03f1c91a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 20 Apr 2026 22:41:54 -0700 Subject: [PATCH 2/3] chore: document intentional await-holding cases (#18423) ## Why This PR prepares the stack to enable Clippy await-holding lints that were left disabled in #18178. The mechanical lock-scope cleanup is handled separately; this PR is the documentation/configuration layer for the remaining await-across-guard sites. Without explicit annotations, reviewers and future maintainers cannot tell whether an await-holding warning is a real concurrency smell or an intentional serialization boundary. ## What changed - Configures `clippy.toml` so `await_holding_invalid_type` also covers `tokio::sync::{MutexGuard,RwLockReadGuard,RwLockWriteGuard}`. - Adds targeted `#[expect(clippy::await_holding_invalid_type, reason = ...)]` annotations for intentional async guard lifetimes. - Documents the main categories of intentional cases: active-turn state transitions that must remain atomic, session-owned MCP manager accesses, remote-control websocket serialization, JS REPL kernel/process serialization, OAuth persistence, external bearer token refresh serialization, and tests that intentionally serialize shared global or session-owned state. - For external bearer token refresh, documents the existing serialization boundary: holding `cached_token` across the provider command prevents concurrent cache misses from starting duplicate refresh commands, and the current behavior is small enough that an explicit expectation is easier to maintain than adding another synchronization primitive. ## Verification - `cargo clippy -p codex-login --all-targets` - `cargo clippy -p codex-connectors --all-targets` - `cargo clippy -p codex-core --all-targets` - The follow-up PR #18698 enables `await_holding_invalid_type` and `await_holding_lock` as workspace `deny` lints, so any undocumented remaining offender will fail Clippy. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18423). * #18698 * __->__ #18423 --- .../app-server/src/codex_message_processor.rs | 8 +++ .../src/message_processor/tracing_tests.rs | 8 +++ .../src/transport/remote_control/websocket.rs | 12 ++++ codex-rs/clippy.toml | 5 ++ .../src/endpoint/responses_websocket.rs | 4 ++ codex-rs/connectors/src/lib.rs | 8 +++ codex-rs/core/src/guardian/review_session.rs | 4 ++ codex-rs/core/src/mcp_tool_call.rs | 12 ++++ codex-rs/core/src/session/handlers.rs | 4 ++ codex-rs/core/src/session/mcp.rs | 28 +++++++++ codex-rs/core/src/session/mod.rs | 60 +++++++++++++++++++ codex-rs/core/src/session/session.rs | 4 ++ codex-rs/core/src/session/tests.rs | 4 ++ .../core/src/session/tests/guardian_tests.rs | 4 ++ codex-rs/core/src/session/turn.rs | 8 +++ codex-rs/core/src/tools/code_mode/mod.rs | 4 ++ codex-rs/core/src/tools/handlers/dynamic.rs | 4 ++ .../core/src/tools/handlers/mcp_resource.rs | 8 +++ .../core/src/tools/handlers/tool_suggest.rs | 8 +++ codex-rs/core/src/tools/js_repl/mod.rs | 16 +++++ codex-rs/core/src/tools/registry.rs | 4 ++ codex-rs/core/src/tools/router_tests.rs | 12 ++++ codex-rs/login/src/auth/external_bearer.rs | 4 ++ codex-rs/rmcp-client/src/oauth.rs | 8 +++ 24 files changed, 241 insertions(+) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 64d60bee34..dc7ec5554c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -8126,6 +8126,10 @@ impl CodexMessageProcessor { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "listener subscription must be serialized against pending thread unloads" + )] async fn ensure_conversation_listener_task( listener_task_context: ListenerTaskContext, conversation_id: ThreadId, @@ -8933,6 +8937,10 @@ async fn handle_thread_listener_command( } #[allow(clippy::too_many_arguments)] +#[expect( + clippy::await_holding_invalid_type, + reason = "running-thread resume subscription must be serialized against pending unloads" +)] async fn handle_pending_thread_resume_request( conversation_id: ThreadId, conversation: &Arc, diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index adf583d8f1..83f8bc98d7 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -505,6 +505,10 @@ where } #[tokio::test(flavor = "current_thread")] +#[expect( + clippy::await_holding_invalid_type, + reason = "test serializes access to global tracing state for its full duration" +)] async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { let _guard = tracing_test_guard().lock().await; let mut harness = TracingHarness::new().await?; @@ -584,6 +588,10 @@ async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> } #[tokio::test(flavor = "current_thread")] +#[expect( + clippy::await_holding_invalid_type, + reason = "test serializes access to global tracing state for its full duration" +)] async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { let _guard = tracing_test_guard().lock().await; let mut harness = TracingHarness::new().await?; diff --git a/codex-rs/app-server/src/transport/remote_control/websocket.rs b/codex-rs/app-server/src/transport/remote_control/websocket.rs index ac91882f52..dee177d30e 100644 --- a/codex-rs/app-server/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server/src/transport/remote_control/websocket.rs @@ -205,6 +205,10 @@ impl RemoteControlWebsocket { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "remote-control client shutdown must serialize tracker state" + )] pub(crate) async fn run( mut self, app_server_client_name_rx: Option>, @@ -416,6 +420,10 @@ impl RemoteControlWebsocket { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "remote-control server event receiver is shared across reconnects" + )] async fn run_server_writer_inner( state: Arc>, server_event_rx: Arc>>, @@ -557,6 +565,10 @@ impl RemoteControlWebsocket { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "remote-control client tracking must stay serialized while processing inbound events" + )] async fn run_websocket_reader_inner( client_tracker: Arc>, state: Arc>, diff --git a/codex-rs/clippy.toml b/codex-rs/clippy.toml index a2aae6591b..2feed8a487 100644 --- a/codex-rs/clippy.toml +++ b/codex-rs/clippy.toml @@ -1,5 +1,10 @@ allow-expect-in-tests = true allow-unwrap-in-tests = true +await-holding-invalid-types = [ + "tokio::sync::MutexGuard", + "tokio::sync::RwLockReadGuard", + "tokio::sync::RwLockWriteGuard", +] disallowed-methods = [ { path = "ratatui::style::Color::Rgb", reason = "Use ANSI colors, which work better in various terminal themes." }, { path = "ratatui::style::Color::Indexed", reason = "Use ANSI colors, which work better in various terminal themes." }, diff --git a/codex-rs/codex-api/src/endpoint/responses_websocket.rs b/codex-rs/codex-api/src/endpoint/responses_websocket.rs index bc72aee411..aea9b917fb 100644 --- a/codex-rs/codex-api/src/endpoint/responses_websocket.rs +++ b/codex-rs/codex-api/src/endpoint/responses_websocket.rs @@ -229,6 +229,10 @@ impl ResponsesWebsocketConnection { let current_span = Span::current(); tokio::spawn( + #[expect( + clippy::await_holding_invalid_type, + reason = "the guard serializes exclusive use of the websocket stream for the lifetime of the response stream" + )] async move { if let Some(model) = server_model { let _ = tx_event.send(Ok(ResponseEvent::ServerModel(model))).await; diff --git a/codex-rs/connectors/src/lib.rs b/codex-rs/connectors/src/lib.rs index e13270a78c..e6260d0e1d 100644 --- a/codex-rs/connectors/src/lib.rs +++ b/codex-rs/connectors/src/lib.rs @@ -446,6 +446,10 @@ mod tests { } #[tokio::test] + #[expect( + clippy::await_holding_invalid_type, + reason = "test serializes access to the shared connector cache for its full duration" + )] async fn list_all_connectors_uses_shared_cache() -> anyhow::Result<()> { let _cache_guard = ALL_CONNECTORS_CACHE_TEST_LOCK.lock().await; @@ -486,6 +490,10 @@ mod tests { } #[tokio::test] + #[expect( + clippy::await_holding_invalid_type, + reason = "test serializes access to the shared connector cache for its full duration" + )] async fn list_all_connectors_merges_and_normalizes_directory_apps() -> anyhow::Result<()> { let _cache_guard = ALL_CONNECTORS_CACHE_TEST_LOCK.lock().await; diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 3a8e475744..5754d090b4 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -265,6 +265,10 @@ impl GuardianReviewSessionManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "review session selection and trunk spawning must stay serialized" + )] pub(crate) async fn run_review( &self, params: GuardianReviewSessionParams, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 32c7cf879e..c5d4068d27 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -494,6 +494,10 @@ async fn execute_mcp_tool_call( ) } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP sandbox metadata reads through the session-owned manager guard" +)] async fn augment_mcp_tool_request_meta_with_sandbox_state( sess: &Session, turn_context: &TurnContext, @@ -1067,6 +1071,10 @@ fn mcp_tool_approval_callsite_mode( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP approval metadata reads through the session-owned manager guard" +)] pub(crate) async fn lookup_mcp_tool_metadata( sess: &Session, turn_context: &TurnContext, @@ -1149,6 +1157,10 @@ fn get_mcp_app_resource_uri( }) } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP app metadata reads through the session-owned manager guard" +)] async fn lookup_mcp_app_usage_metadata( sess: &Session, server: &str, diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index 36a4491546..71efd23326 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -468,6 +468,10 @@ pub async fn reload_user_config(sess: &Arc) { sess.reload_user_config_layer().await; } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP tool listing reads through the session-owned manager guard" +)] pub async fn list_mcp_tools(sess: &Session, config: &Arc, sub_id: String) { let mcp_connection_manager = sess.services.mcp_connection_manager.read().await; let auth = sess.services.auth_manager.auth().await; diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index d15ca2cae8..be7504d9e7 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -1,6 +1,10 @@ use super::*; impl Session { + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_mcp_server_elicitation( &self, turn_context: &TurnContext, @@ -80,6 +84,10 @@ impl Session { rx_response.await.ok() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and manager fallback must stay serialized" + )] pub async fn resolve_elicitation( &self, server_name: String, @@ -111,6 +119,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource calls are serialized through the session-owned manager guard" + )] pub async fn list_resources( &self, server: &str, @@ -124,6 +136,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource calls are serialized through the session-owned manager guard" + )] pub async fn list_resource_templates( &self, server: &str, @@ -137,6 +153,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource calls are serialized through the session-owned manager guard" + )] pub async fn read_resource( &self, server: &str, @@ -150,6 +170,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP tool calls are serialized through the session-owned manager guard" + )] pub async fn call_tool( &self, server: &str, @@ -165,6 +189,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP tool metadata reads through the session-owned manager guard" + )] pub(crate) async fn resolve_mcp_tool_info(&self, tool_name: &ToolName) -> Option { self.services .mcp_connection_manager diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 3ea826c491..46d0c248b8 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -1786,6 +1786,10 @@ impl Session { /// be used to derive the available decisions via /// [ExecApprovalRequestEvent::default_available_decisions]. #[allow(clippy::too_many_arguments)] + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_command_approval( &self, turn_context: &TurnContext, @@ -1857,6 +1861,10 @@ impl Session { rx_approve.await.unwrap_or(ReviewDecision::Abort) } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_patch_approval( &self, turn_context: &TurnContext, @@ -1893,6 +1901,10 @@ impl Session { rx_approve } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_permissions( self: &Arc, turn_context: &Arc, @@ -2023,6 +2035,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_user_input( &self, turn_context: &TurnContext, @@ -2055,6 +2071,10 @@ impl Session { rx_response.await.ok() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_user_input_response( &self, sub_id: &str, @@ -2080,6 +2100,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_request_permissions_response( &self, call_id: &str, @@ -2136,6 +2160,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn reads must stay consistent with the matching turn state" + )] pub(crate) async fn granted_turn_permissions(&self) -> Option { let active = self.active_turn.lock().await; let active = active.as_ref()?; @@ -2148,6 +2176,10 @@ impl Session { state.granted_permissions() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_dynamic_tool_response(&self, call_id: &str, response: DynamicToolResponse) { let entry = { let mut active = self.active_turn.lock().await; @@ -2169,6 +2201,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_approval(&self, approval_id: &str, decision: ReviewDecision) { let entry = { let mut active = self.active_turn.lock().await; @@ -2329,6 +2365,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP app context rendering reads through the session-owned manager guard" + )] pub(crate) async fn build_initial_context( &self, turn_context: &TurnContext, @@ -2791,6 +2831,10 @@ impl Session { /// Inject additional user input into the currently active turn. /// /// Returns the active turn id when accepted. + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn steer_input( &self, input: Vec, @@ -2850,6 +2894,10 @@ impl Session { } /// Returns the input if there was no task running to inject into. + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn inject_response_items( &self, input: Vec, @@ -2923,6 +2971,10 @@ impl Session { self.mailbox_rx.lock().await.has_pending_trigger_turn() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn prepend_pending_input(&self, input: Vec) -> Result<(), ()> { let mut active = self.active_turn.lock().await; match active.as_mut() { @@ -2935,6 +2987,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn get_pending_input(&self) -> Vec { let (pending_input, accepts_mailbox_delivery) = { let mut active = self.active_turn.lock().await; @@ -2990,6 +3046,10 @@ impl Session { !self.idle_pending_input.lock().await.is_empty() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state reads must remain atomic" + )] pub async fn has_pending_input(&self) -> bool { let (has_turn_pending_input, accepts_mailbox_delivery) = { let active = self.active_turn.lock().await; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index a2080bbeab..16e86e3aea 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -209,6 +209,10 @@ pub(crate) struct AppServerClientMetadata { impl Session { #[instrument(name = "session_init", level = "info", skip_all)] #[allow(clippy::too_many_arguments)] + #[expect( + clippy::await_holding_invalid_type, + reason = "session initialization must serialize access through session-owned manager guards" + )] pub(crate) async fn new( mut session_configuration: SessionConfiguration, config: Arc, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 18cefba934..97b6eba8b3 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -6359,6 +6359,10 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { } #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn fatal_tool_error_stops_turn_and_reports_error() { let (session, turn_context, _rx) = make_session_and_context_with_rx().await; let tools = { diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 108d095756..9b1172d578 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -489,6 +489,10 @@ async fn process_compacted_history_preserves_separate_guardian_developer_message #[tokio::test] #[cfg(unix)] +#[expect( + clippy::await_holding_invalid_type, + reason = "test mutates active turn state directly to seed granted permissions" +)] async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_permissions_feature() { let (mut session, turn_context_raw) = make_session_and_context().await; session diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index f613802244..4f6b062f9f 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -128,6 +128,10 @@ use tracing::warn; /// - If the model sends only an assistant message, we record it in the /// conversation history and consider the turn complete. /// +#[expect( + clippy::await_holding_invalid_type, + reason = "turn execution must keep active-turn state transitions atomic" +)] pub(crate) async fn run_turn( sess: Arc, turn_context: Arc, @@ -1149,6 +1153,10 @@ async fn run_sampling_request( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "tool router construction reads through the session-owned manager guard" +)] pub(crate) async fn built_tools( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/src/tools/code_mode/mod.rs b/codex-rs/core/src/tools/code_mode/mod.rs index 5049836393..575d3f9a89 100644 --- a/codex-rs/core/src/tools/code_mode/mod.rs +++ b/codex-rs/core/src/tools/code_mode/mod.rs @@ -259,6 +259,10 @@ pub(super) async fn build_enabled_tools( collect_code_mode_tool_definitions(&specs) } +#[expect( + clippy::await_holding_invalid_type, + reason = "nested tool router construction reads through the session-owned manager guard" +)] async fn build_nested_router(exec: &ExecContext) -> ToolRouter { let nested_tools_config = exec.turn.tools_config.for_code_mode_nested_tools(); let listed_mcp_tools = exec diff --git a/codex-rs/core/src/tools/handlers/dynamic.rs b/codex-rs/core/src/tools/handlers/dynamic.rs index 140d586be9..c989d70f19 100644 --- a/codex-rs/core/src/tools/handlers/dynamic.rs +++ b/codex-rs/core/src/tools/handlers/dynamic.rs @@ -71,6 +71,10 @@ impl ToolHandler for DynamicToolHandler { } } +#[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and dynamic tool response registration must remain atomic" +)] async fn request_dynamic_tool( session: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/src/tools/handlers/mcp_resource.rs b/codex-rs/core/src/tools/handlers/mcp_resource.rs index 67f44cb699..fa4a066741 100644 --- a/codex-rs/core/src/tools/handlers/mcp_resource.rs +++ b/codex-rs/core/src/tools/handlers/mcp_resource.rs @@ -240,6 +240,10 @@ impl ToolHandler for McpResourceHandler { } } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource listing reads through the session-owned manager guard" +)] async fn handle_list_resources( session: Arc, turn: Arc, @@ -344,6 +348,10 @@ async fn handle_list_resources( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource template listing reads through the session-owned manager guard" +)] async fn handle_list_resource_templates( session: Arc, turn: Arc, diff --git a/codex-rs/core/src/tools/handlers/tool_suggest.rs b/codex-rs/core/src/tools/handlers/tool_suggest.rs index cc433601bc..5c51c90d66 100644 --- a/codex-rs/core/src/tools/handlers/tool_suggest.rs +++ b/codex-rs/core/src/tools/handlers/tool_suggest.rs @@ -34,6 +34,10 @@ impl ToolHandler for ToolSuggestHandler { ToolKind::Function } + #[expect( + clippy::await_holding_invalid_type, + reason = "tool suggestion discovery reads through the session-owned manager guard" + )] async fn handle(&self, invocation: ToolInvocation) -> Result { let ToolInvocation { payload, @@ -193,6 +197,10 @@ async fn verify_tool_suggestion_completed( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "connector cache refresh reads through the session-owned manager guard" +)] async fn refresh_missing_suggested_connectors( session: &crate::session::session::Session, turn: &crate::session::turn_context::TurnContext, diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 539c773852..ec4443aca8 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -849,6 +849,10 @@ impl JsReplManager { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "js_repl kernel initialization must be serialized with kernel state" + )] pub async fn execute_with_cancellation( &self, session: Arc, @@ -1177,6 +1181,10 @@ impl JsReplManager { Ok(kernel_path) } + #[expect( + clippy::await_holding_invalid_type, + reason = "js_repl stdin writes must be serialized per kernel" + )] async fn write_message( stdin: &Arc>, msg: &HostToKernel, @@ -1224,6 +1232,10 @@ impl JsReplManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "js_repl child shutdown must serialize process inspection and termination" + )] async fn kill_kernel_child(child: &Arc>, reason: &'static str) { let mut guard = child.lock().await; let pid = guard.id(); @@ -1547,6 +1559,10 @@ impl JsReplManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "nested js_repl tool routing reads through the session-owned manager guard" + )] async fn run_tool_request(exec: ExecContext, req: RunToolRequest) -> RunToolResult { if is_js_repl_internal_tool(&req.tool_name) { let error = "js_repl cannot invoke itself".to_string(); diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 4c0e3fbea8..aca1d10144 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -239,6 +239,10 @@ impl ToolRegistry { // } // } + #[expect( + clippy::await_holding_invalid_type, + reason = "tool dispatch must keep active-turn accounting atomic" + )] pub(crate) async fn dispatch_any( &self, invocation: ToolInvocation, diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index e466bdb1c7..c83c65fc4e 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -15,6 +15,10 @@ use super::ToolRouter; use super::ToolRouterParams; #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { let (session, mut turn) = make_session_and_context().await; turn.tools_config.js_repl_tools_only = true; @@ -70,6 +74,10 @@ async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { } #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn js_repl_tools_only_allows_js_repl_source_calls() -> anyhow::Result<()> { let (session, mut turn) = make_session_and_context().await; turn.tools_config.js_repl_tools_only = true; @@ -175,6 +183,10 @@ async fn js_repl_tools_only_blocks_namespaced_js_repl_tool() -> anyhow::Result<( } #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn parallel_support_does_not_match_namespaced_local_tool_names() -> anyhow::Result<()> { let (session, turn) = make_session_and_context().await; let mcp_tools = session diff --git a/codex-rs/login/src/auth/external_bearer.rs b/codex-rs/login/src/auth/external_bearer.rs index c179f48619..c528596014 100644 --- a/codex-rs/login/src/auth/external_bearer.rs +++ b/codex-rs/login/src/auth/external_bearer.rs @@ -33,6 +33,10 @@ impl ExternalAuth for BearerTokenRefresher { AuthMode::ApiKey } + #[expect( + clippy::await_holding_invalid_type, + reason = "external bearer cache misses intentionally hold cached_token across the provider command to avoid duplicate refreshes" + )] async fn resolve(&self) -> io::Result> { let access_token = { let mut cached = self.state.cached_token.lock().await; diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 979a7ad217..5adfae9347 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -283,6 +283,10 @@ impl OAuthPersistor { /// Persists the latest stored credentials if they have changed. /// Deletes the credentials if they are no longer present. + #[expect( + clippy::await_holding_invalid_type, + reason = "AuthorizationManager async access must be serialized through its mutex" + )] pub(crate) async fn persist_if_needed(&self) -> Result<()> { let (client_id, maybe_credentials) = { let manager = self.inner.authorization_manager.clone(); @@ -335,6 +339,10 @@ impl OAuthPersistor { Ok(()) } + #[expect( + clippy::await_holding_invalid_type, + reason = "AuthorizationManager async access must be serialized through its mutex" + )] pub(crate) async fn refresh_if_needed(&self) -> Result<()> { let expires_at = { let guard = self.inner.last_credentials.lock().await; From 30bc98760f6c041f7cf7215b3c28a1fb351bf10c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 20 Apr 2026 22:42:35 -0700 Subject: [PATCH 3/3] chore: enable await-holding clippy lints Follow-up to https://github.com/openai/codex/pull/18178, where we said the await-holding clippy rule would be enabled separately. Enable `await_holding_lock` and `await_holding_invalid_type` after the preceding commits fixed or explicitly documented the current offenders. --- .bazelrc | 2 ++ codex-rs/Cargo.toml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/.bazelrc b/.bazelrc index 3a49a22c20..fb6e93e58c 100644 --- a/.bazelrc +++ b/.bazelrc @@ -82,6 +82,8 @@ build:clippy --@rules_rust//rust/settings:clippy.toml=//codex-rs:clippy.toml # in their own `Cargo.toml`, but `rules_rust` Bazel clippy does not read Cargo lint levels. # `clippy.toml` can configure lint behavior, but it cannot set allow/warn/deny/forbid levels. build:clippy --@rules_rust//rust/settings:clippy_flag=-Dwarnings +build:clippy --@rules_rust//rust/settings:clippy_flag=--deny=clippy::await_holding_invalid_type +build:clippy --@rules_rust//rust/settings:clippy_flag=--deny=clippy::await_holding_lock build:clippy --@rules_rust//rust/settings:clippy_flag=--deny=clippy::expect_used build:clippy --@rules_rust//rust/settings:clippy_flag=--deny=clippy::identity_op build:clippy --@rules_rust//rust/settings:clippy_flag=--deny=clippy::manual_clamp diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 19987fb8f0..f4be9c1abc 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -383,6 +383,8 @@ zeroize = "1.8.2" rust = {} [workspace.lints.clippy] +await_holding_invalid_type = "deny" +await_holding_lock = "deny" expect_used = "deny" identity_op = "deny" manual_clamp = "deny"