mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Handle standalone tool outputs as external context (#39791)
## What changed - Treat standalone `function_call_output` items without a `call_id` as external context, including items injected into new and forked threads, and mark the thread memory mode polluted when `memories.disable_on_external_context` is enabled. - Include these outputs in guardian transcripts with their namespaced tool name and a placeholder for non-text content. - Allow image generation to reuse images from standalone and otherwise unpaired function or custom-tool outputs. ## Testing - Cover injected thread items, external-context detection, guardian transcript rendering, and recent-image selection. GitOrigin-RevId: ca4e8c16a4b731e1256a5956908d00f785b4feb1
This commit is contained in:
@@ -13,10 +13,13 @@ use codex_app_server_protocol::TurnStartResponse;
|
||||
use codex_app_server_protocol::UserInput as V2UserInput;
|
||||
use codex_core::RolloutRecorder;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_rollout::InitialHistory;
|
||||
use codex_rollout::RolloutItem;
|
||||
use codex_state::StateRuntime;
|
||||
use codex_utils_absolute_path::test_support::PathExt;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::responses::strip_response_item_id;
|
||||
use core_test_support::responses::strip_response_item_ids_from_json;
|
||||
@@ -40,8 +43,15 @@ async fn thread_inject_items_adds_raw_response_items_to_thread_history() -> Resu
|
||||
|
||||
let codex_home = TempDir::new()?;
|
||||
MockResponsesConfig::new(&server.uri())
|
||||
.enable_feature(Feature::Sqlite)
|
||||
.enable_feature(Feature::RetainClientDeveloperMessages)
|
||||
.with_extra_config("[memories]\ndisable_on_external_context = true")
|
||||
.write(codex_home.path())?;
|
||||
let state_db = StateRuntime::init(
|
||||
codex_state::SqliteConfig::new_for_testing(codex_home.path().abs()),
|
||||
"mock_provider".into(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
@@ -100,6 +110,13 @@ async fn thread_inject_items_adds_raw_response_items_to_thread_history() -> Resu
|
||||
.await?;
|
||||
let _response: ThreadInjectItemsResponse =
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(inject_req)).await??;
|
||||
assert_eq!(
|
||||
state_db
|
||||
.get_thread_memory_mode(ThreadId::from_string(&thread.id)?)
|
||||
.await?
|
||||
.as_deref(),
|
||||
Some("polluted")
|
||||
);
|
||||
|
||||
let rollout_path = thread.path.as_ref().context("thread path missing")?;
|
||||
let history = RolloutRecorder::get_rollout_history(rollout_path).await?;
|
||||
|
||||
@@ -595,6 +595,26 @@ pub(crate) fn collect_guardian_transcript_entries<'a>(
|
||||
};
|
||||
non_empty_entry(kind, text)
|
||||
}),
|
||||
ResponseItem::FunctionCallOutput {
|
||||
call_id: None,
|
||||
name: Some(name),
|
||||
namespace,
|
||||
output,
|
||||
..
|
||||
} => {
|
||||
let text = output
|
||||
.body
|
||||
.to_text()
|
||||
.unwrap_or_else(|| "[non-text output]".into());
|
||||
let name = match namespace {
|
||||
Some(namespace) => format!("{namespace}.{name}"),
|
||||
None => name.to_string(),
|
||||
};
|
||||
non_empty_entry(
|
||||
GuardianTranscriptEntryKind::Tool(format!("tool {name} result")),
|
||||
text,
|
||||
)
|
||||
}
|
||||
_ => None,
|
||||
};
|
||||
|
||||
|
||||
@@ -40,6 +40,7 @@ use codex_protocol::ThreadId;
|
||||
use codex_protocol::approvals::NetworkApprovalProtocol;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
@@ -1046,6 +1047,44 @@ fn collect_guardian_transcript_entries_includes_recent_tool_calls_and_output() {
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_guardian_transcript_entries_preserves_named_unpaired_tool_sources() {
|
||||
let mut items = vec![ResponseItem::FunctionCallOutput {
|
||||
id: None,
|
||||
call_id: None,
|
||||
name: Some("notifications".to_string()),
|
||||
namespace: Some("slack".to_string()),
|
||||
output: codex_protocol::models::FunctionCallOutputPayload::from_text(
|
||||
"new message".to_string(),
|
||||
),
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
}];
|
||||
|
||||
assert_eq!(
|
||||
collect_guardian_transcript_entries(&items),
|
||||
vec![GuardianTranscriptEntry {
|
||||
kind: GuardianTranscriptEntryKind::Tool("tool slack.notifications result".to_string()),
|
||||
text: "new message".to_string(),
|
||||
}]
|
||||
);
|
||||
|
||||
if let ResponseItem::FunctionCallOutput { output, .. } = &mut items[0] {
|
||||
*output = codex_protocol::models::FunctionCallOutputPayload::from_content_items(vec![
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "data:image/png;base64,image".to_string(),
|
||||
detail: None,
|
||||
},
|
||||
]);
|
||||
}
|
||||
assert_eq!(
|
||||
collect_guardian_transcript_entries(&items),
|
||||
vec![GuardianTranscriptEntry {
|
||||
kind: GuardianTranscriptEntryKind::Tool("tool slack.notifications result".to_string()),
|
||||
text: "[non-text output]".to_string(),
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_truncate_text_keeps_prefix_suffix_and_xml_marker() {
|
||||
let content = "prefix ".repeat(200) + &" suffix".repeat(200);
|
||||
|
||||
@@ -42,6 +42,7 @@ use crate::session::step_context::StepContext;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::session_prefix::format_inter_agent_completion_message;
|
||||
use crate::skills_load_input_from_config;
|
||||
use crate::stream_events_utils::mark_thread_memory_mode_polluted_if_external_context;
|
||||
use crate::turn_metadata::TurnMetadataState;
|
||||
use crate::turn_timing::now_unix_timestamp_ms;
|
||||
use async_channel::Receiver;
|
||||
@@ -1346,12 +1347,13 @@ impl Session {
|
||||
let mut state = self.state.lock().await;
|
||||
state.set_next_turn_is_first(!has_prior_user_turns);
|
||||
}
|
||||
match conversation_history {
|
||||
let turn_context = match conversation_history {
|
||||
InitialHistory::New | InitialHistory::Cleared => {
|
||||
// Defer initial context insertion until the first real turn starts so
|
||||
// turn/start overrides can be merged before we write model-visible context.
|
||||
self.set_previous_turn_settings(/*previous_turn_settings*/ None)
|
||||
.await;
|
||||
None
|
||||
}
|
||||
InitialHistory::Resumed(resumed_history) => {
|
||||
let turn_context = self.new_default_turn().await;
|
||||
@@ -1401,6 +1403,7 @@ impl Session {
|
||||
if !is_subagent {
|
||||
let _ = self.flush_rollout().await;
|
||||
}
|
||||
None
|
||||
}
|
||||
InitialHistory::Forked(mut rollout_items) => {
|
||||
let turn_context = self.new_default_turn().await;
|
||||
@@ -1449,6 +1452,19 @@ impl Session {
|
||||
if !is_subagent {
|
||||
let _ = self.flush_rollout().await;
|
||||
}
|
||||
Some(turn_context)
|
||||
}
|
||||
};
|
||||
if let Some(turn_context) = turn_context
|
||||
&& turn_context.config.memories.disable_on_external_context
|
||||
{
|
||||
let history = self.clone_history().await;
|
||||
if let Some(item) = history
|
||||
.raw_items()
|
||||
.find(|item| matches!(item, ResponseItem::FunctionCallOutput { call_id: None, .. }))
|
||||
{
|
||||
mark_thread_memory_mode_polluted_if_external_context(self, &turn_context, item)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -3087,6 +3103,13 @@ impl Session {
|
||||
let rollout_items: Vec<RolloutItem> =
|
||||
items.into_iter().map(RolloutItem::ResponseItem).collect();
|
||||
self.persist_rollout_items(&rollout_items).await;
|
||||
if turn_context.config.memories.disable_on_external_context
|
||||
&& let Some(item) = response_items
|
||||
.iter()
|
||||
.find(|item| matches!(item, ResponseItem::FunctionCallOutput { call_id: None, .. }))
|
||||
{
|
||||
mark_thread_memory_mode_polluted_if_external_context(self, turn_context, item).await;
|
||||
}
|
||||
self.send_raw_response_items(turn_context, &response_items)
|
||||
.await;
|
||||
}
|
||||
|
||||
@@ -135,6 +135,7 @@ fn response_item_may_include_external_context(item: &ResponseItem) -> bool {
|
||||
ResponseItem::ToolSearchCall { .. }
|
||||
| ResponseItem::ToolSearchOutput { .. }
|
||||
| ResponseItem::WebSearchCall { .. }
|
||||
| ResponseItem::FunctionCallOutput { call_id: None, .. }
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -70,6 +70,14 @@ fn external_context_pollution_items_include_web_search_and_tool_search() {
|
||||
tools: Vec::new(),
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
},
|
||||
ResponseItem::FunctionCallOutput {
|
||||
id: None,
|
||||
call_id: None,
|
||||
name: Some("notifications".to_string()),
|
||||
namespace: Some("slack".to_string()),
|
||||
output: FunctionCallOutputPayload::from_text("new message".to_string()),
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
},
|
||||
];
|
||||
|
||||
assert!(
|
||||
|
||||
@@ -247,6 +247,29 @@ impl TranscriptConfig {
|
||||
);
|
||||
(role, output)
|
||||
}
|
||||
ResponseItem::FunctionCallOutput {
|
||||
call_id: None,
|
||||
name,
|
||||
namespace,
|
||||
output,
|
||||
..
|
||||
} => {
|
||||
if !self.sources.contains(&TranscriptSource::ToolOutputs) {
|
||||
continue;
|
||||
}
|
||||
let output = output
|
||||
.body
|
||||
.to_text()
|
||||
.unwrap_or_else(|| "[non-text output]".into());
|
||||
let role = match (name.as_deref(), namespace.as_deref()) {
|
||||
(Some(name), Some(namespace)) => {
|
||||
format!("tool {namespace}.{name} result")
|
||||
}
|
||||
(Some(name), None) => format!("tool {name} result"),
|
||||
(None, _) => "tool result".to_owned(),
|
||||
};
|
||||
(role, output)
|
||||
}
|
||||
ResponseItem::Reasoning {
|
||||
summary, content, ..
|
||||
} => {
|
||||
@@ -289,7 +312,6 @@ impl TranscriptConfig {
|
||||
("tool web_search call".to_owned(), text)
|
||||
}
|
||||
ResponseItem::AdditionalTools { .. }
|
||||
| ResponseItem::FunctionCallOutput { call_id: None, .. }
|
||||
| ResponseItem::ImageGenerationCall { .. }
|
||||
| ResponseItem::ToolSearchCall { .. }
|
||||
| ResponseItem::ToolSearchOutput { .. }
|
||||
|
||||
@@ -485,6 +485,36 @@ fn transcript_truncates_tool_results_using_standard_budget() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn transcript_preserves_named_unpaired_tool_source() {
|
||||
let mut items = vec![ResponseItem::FunctionCallOutput {
|
||||
id: None,
|
||||
call_id: None,
|
||||
name: Some("notifications".to_owned()),
|
||||
namespace: Some("slack".to_owned()),
|
||||
output: FunctionCallOutputPayload::from_text("new message".to_owned()),
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
}];
|
||||
|
||||
assert_eq!(
|
||||
TranscriptConfig::default().build(&items),
|
||||
vec!["[1] tool slack.notifications result: new message\n"]
|
||||
);
|
||||
|
||||
if let ResponseItem::FunctionCallOutput { output, .. } = &mut items[0] {
|
||||
*output = FunctionCallOutputPayload::from_content_items(vec![
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "data:image/png;base64,image".to_owned(),
|
||||
detail: None,
|
||||
},
|
||||
]);
|
||||
}
|
||||
assert_eq!(
|
||||
TranscriptConfig::default().build(&items),
|
||||
vec!["[1] tool slack.notifications result: [non-text output]\n"]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_reasoning_counts_against_message_budget() {
|
||||
let mut items = (0..8)
|
||||
|
||||
@@ -138,10 +138,10 @@ async fn recent_image_fallback_selects_newest_images_in_chronological_order() {
|
||||
},
|
||||
ResponseItem::FunctionCallOutput {
|
||||
id: None,
|
||||
call_id: Some("orphan-call".to_string()),
|
||||
name: None,
|
||||
namespace: None,
|
||||
output: image_output("orphan"),
|
||||
call_id: None,
|
||||
name: Some("notifications".to_string()),
|
||||
namespace: Some("slack".to_string()),
|
||||
output: image_output("standalone"),
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
},
|
||||
];
|
||||
@@ -151,7 +151,7 @@ async fn recent_image_fallback_selects_newest_images_in_chronological_order() {
|
||||
&ImagegenArgs {
|
||||
prompt: "change the lighting".to_string(),
|
||||
referenced_image_paths: None,
|
||||
num_last_images_to_include: Some(4),
|
||||
num_last_images_to_include: Some(5),
|
||||
},
|
||||
&history,
|
||||
&[],
|
||||
@@ -160,7 +160,7 @@ async fn recent_image_fallback_selects_newest_images_in_chronological_order() {
|
||||
.expect("history-backed edit request should build"),
|
||||
ImageRequest::Edit(expected_edit_request(
|
||||
"change the lighting",
|
||||
&["user-2", "mcp", "code-mode", "generated"],
|
||||
&["user-2", "mcp", "code-mode", "generated", "standalone"],
|
||||
))
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
use std::collections::HashSet;
|
||||
use std::io;
|
||||
|
||||
use base64::Engine;
|
||||
@@ -470,34 +469,6 @@ async fn request_for_call_args(
|
||||
}
|
||||
|
||||
fn recent_images(history: &[ResponseItem], count: usize) -> Vec<ImageUrl> {
|
||||
let mut function_call_ids = HashSet::new();
|
||||
let mut custom_tool_call_ids = HashSet::new();
|
||||
for item in history {
|
||||
match item {
|
||||
ResponseItem::FunctionCall { call_id, .. } => {
|
||||
function_call_ids.insert(call_id.as_str());
|
||||
}
|
||||
ResponseItem::CustomToolCall { call_id, .. } => {
|
||||
custom_tool_call_ids.insert(call_id.as_str());
|
||||
}
|
||||
ResponseItem::AdditionalTools { .. }
|
||||
| ResponseItem::Message { .. }
|
||||
| ResponseItem::AgentMessage { .. }
|
||||
| ResponseItem::Reasoning { .. }
|
||||
| ResponseItem::LocalShellCall { .. }
|
||||
| ResponseItem::ToolSearchCall { .. }
|
||||
| ResponseItem::FunctionCallOutput { .. }
|
||||
| ResponseItem::CustomToolCallOutput { .. }
|
||||
| ResponseItem::ToolSearchOutput { .. }
|
||||
| ResponseItem::WebSearchCall { .. }
|
||||
| ResponseItem::ImageGenerationCall { .. }
|
||||
| ResponseItem::Compaction { .. }
|
||||
| ResponseItem::CompactionTrigger { .. }
|
||||
| ResponseItem::ContextCompaction { .. }
|
||||
| ResponseItem::Other => {}
|
||||
}
|
||||
}
|
||||
|
||||
let mut images = Vec::with_capacity(count);
|
||||
'history: for item in history.iter().rev() {
|
||||
let mut image_urls = Vec::new();
|
||||
@@ -510,16 +481,8 @@ fn recent_images(history: &[ResponseItem], count: usize) -> Vec<ImageUrl> {
|
||||
| ContentItem::OutputText { .. } => None,
|
||||
}));
|
||||
}
|
||||
ResponseItem::FunctionCallOutput {
|
||||
call_id: Some(call_id),
|
||||
output,
|
||||
..
|
||||
} if function_call_ids.contains(call_id.as_str()) => {
|
||||
image_urls.extend(output_image_urls(output));
|
||||
}
|
||||
ResponseItem::CustomToolCallOutput {
|
||||
call_id, output, ..
|
||||
} if custom_tool_call_ids.contains(call_id.as_str()) => {
|
||||
ResponseItem::FunctionCallOutput { output, .. }
|
||||
| ResponseItem::CustomToolCallOutput { output, .. } => {
|
||||
image_urls.extend(output_image_urls(output));
|
||||
}
|
||||
ResponseItem::ImageGenerationCall { result, .. } if !result.is_empty() => {
|
||||
@@ -532,8 +495,6 @@ fn recent_images(history: &[ResponseItem], count: usize) -> Vec<ImageUrl> {
|
||||
| ResponseItem::FunctionCall { .. }
|
||||
| ResponseItem::ToolSearchCall { .. }
|
||||
| ResponseItem::CustomToolCall { .. }
|
||||
| ResponseItem::FunctionCallOutput { .. }
|
||||
| ResponseItem::CustomToolCallOutput { .. }
|
||||
| ResponseItem::ToolSearchOutput { .. }
|
||||
| ResponseItem::WebSearchCall { .. }
|
||||
| ResponseItem::ImageGenerationCall { .. }
|
||||
|
||||
Reference in New Issue
Block a user