mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Add bounded tool-result metadata support to executed tool calls (#44336)
## What changed - Add host-recorded `tool_result_metadata` snapshots with size limits, redacted debug output, and protection against deserialization from untrusted input. MCP capture remains disabled. - Preserve accepted metadata across retries and delayed Code Mode results. Shed raw metadata before source evidence, calls, or completion markers when the request budget is exceeded. - Send raw metadata only to allowed OpenAI and ChatGPT HTTPS endpoints, filtering by the resolved destination for HTTP and WebSocket requests. - Encode source parse failures as an array entry with type `parse_failed` and an empty ID. ## Testing Add coverage for metadata bounds and redaction, retained-call updates, budget shedding, disabled MCP capture, and HTTP/WebSocket destination filtering. GitOrigin-RevId: b1a396323ad0623167cdd9e20ff9453f0d01bbf7
This commit is contained in:
@@ -865,6 +865,26 @@ impl ModelClient {
|
||||
Ok(request)
|
||||
}
|
||||
|
||||
fn filter_tool_result_metadata(input: &mut [ResponseItem], api_provider: &ApiProvider) {
|
||||
// Check the resolved destination only when sending, not for local budget estimates.
|
||||
// HTTP and WS (including v2 compaction) share this raw-metadata-only filter.
|
||||
let result_metadata_allowed =
|
||||
url::Url::parse(&api_provider.base_url)
|
||||
.ok()
|
||||
.is_some_and(|url| {
|
||||
url.scheme() == "https"
|
||||
&& url.host_str().is_some_and(|host| {
|
||||
host == "api.openai.com"
|
||||
|| codex_http_client::is_allowed_chatgpt_host(host)
|
||||
})
|
||||
});
|
||||
if !result_metadata_allowed {
|
||||
for item in input {
|
||||
item.clear_tool_result_metadata();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn prepare_response_items_for_request(&self, input: &mut [ResponseItem]) {
|
||||
for item in input {
|
||||
if item.id().is_some_and(|id| !id.is_prefixed()) {
|
||||
@@ -1479,6 +1499,10 @@ impl ModelClientSession {
|
||||
service_tier.clone(),
|
||||
responses_metadata,
|
||||
)?;
|
||||
ModelClient::filter_tool_result_metadata(
|
||||
&mut request.input,
|
||||
&client_setup.api_provider,
|
||||
);
|
||||
self.client.set_guardian_metadata(
|
||||
&mut request.client_metadata,
|
||||
responses_metadata.parent_response_id.as_deref(),
|
||||
@@ -1631,6 +1655,10 @@ impl ModelClientSession {
|
||||
service_tier.clone(),
|
||||
responses_metadata,
|
||||
)?;
|
||||
ModelClient::filter_tool_result_metadata(
|
||||
&mut request.input,
|
||||
&client_setup.api_provider,
|
||||
);
|
||||
if endpoint == ResponsesEndpoint::Guardian {
|
||||
request.service_tier = None;
|
||||
}
|
||||
|
||||
@@ -44,7 +44,13 @@ use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::CodexErrorDetails;
|
||||
use codex_protocol::models::BaseInstructions;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::ExecutedToolCall;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::models::ToolResultMetadata;
|
||||
use codex_protocol::models::ToolResultSource;
|
||||
use codex_protocol::models::ToolResultSources;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
@@ -83,6 +89,11 @@ use tracing_subscriber::layer::Context as LayerContext;
|
||||
use tracing_subscriber::layer::SubscriberExt;
|
||||
use tracing_subscriber::registry::LookupSpan;
|
||||
use tracing_subscriber::util::SubscriberInitExt;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
const TEST_INSTALLATION_ID: &str = "11111111-1111-4111-8111-111111111111";
|
||||
|
||||
@@ -164,6 +175,159 @@ fn test_model_info() -> ModelInfo {
|
||||
.expect("deserialize test model info")
|
||||
}
|
||||
|
||||
fn output_with_tool_result_metadata(metadata: ToolResultMetadata) -> ResponseItem {
|
||||
let mut call = ExecutedToolCall::new("test_tool".to_string(), json!({ "query": "keep" }));
|
||||
call.set_tool_result_sources(ToolResultSources::new(vec![ToolResultSource {
|
||||
r#type: "test_resource".to_string(),
|
||||
id: "R1".to_string(),
|
||||
}]));
|
||||
call.set_tool_result_metadata(metadata);
|
||||
let mut output = ResponseItem::from(ResponseInputItem::FunctionCallOutput {
|
||||
call_id: "tool-call".to_string(),
|
||||
output: FunctionCallOutputPayload::from_text("unchanged tool result".to_string()),
|
||||
});
|
||||
output.append_executed_tool_calls(vec![call]);
|
||||
output.mark_tool_calls_complete();
|
||||
output
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn responses_request_limits_raw_tool_metadata_to_resolved_first_party_https_endpoint()
|
||||
-> anyhow::Result<()> {
|
||||
let provider =
|
||||
ModelProviderInfo::create_openai_provider(Some("https://api.openai.com/v1".to_string()));
|
||||
let mut api_provider = provider.to_api_provider(/*auth_mode*/ None)?;
|
||||
let mut client = test_model_client(SessionSource::Cli);
|
||||
Arc::get_mut(&mut client.state)
|
||||
.expect("test client should have unique session state")
|
||||
.provider = create_model_provider(provider, /*auth_manager*/ None);
|
||||
let output = output_with_tool_result_metadata(ToolResultMetadata::new(&json!({
|
||||
"private": { "resource": "raw-result-metadata" },
|
||||
})));
|
||||
let without_raw_metadata = output_with_tool_result_metadata(ToolResultMetadata::default());
|
||||
let prompt = Prompt {
|
||||
input: vec![output.clone()],
|
||||
..Default::default()
|
||||
};
|
||||
let responses_metadata = test_responses_metadata_for_client(
|
||||
&client,
|
||||
/*turn_id*/ None,
|
||||
format!("{}:0", client.state.thread_id),
|
||||
/*parent_thread_id*/ None,
|
||||
TestCodexResponsesRequestKind::Turn,
|
||||
);
|
||||
for (base_url, allowed) in [
|
||||
("https://api.openai.com/v1", true),
|
||||
("https://chatgpt.com/backend-api/codex", true),
|
||||
("https://api.chatgpt-staging.com/v1", true),
|
||||
("https://proxy.example.com/v1", false),
|
||||
("http://api.openai.com/v1", false),
|
||||
("https://api.openai.com.evil.example/v1", false),
|
||||
("https://chatgpt.com.evil.example/v1", false),
|
||||
("https://api.openai.com@proxy.example.com/v1", false),
|
||||
("not a URL", false),
|
||||
] {
|
||||
api_provider.base_url = base_url.to_string();
|
||||
for responses_lite in [false, true] {
|
||||
let mut model = test_model_info();
|
||||
model.use_responses_lite = responses_lite;
|
||||
let mut request = client.build_responses_request(
|
||||
&prompt,
|
||||
&model,
|
||||
/*effort*/ None,
|
||||
codex_protocol::config_types::ReasoningSummary::None,
|
||||
/*service_tier*/ None,
|
||||
&responses_metadata,
|
||||
)?;
|
||||
ModelClient::filter_tool_result_metadata(&mut request.input, &api_provider);
|
||||
assert_eq!(
|
||||
request.input.last(),
|
||||
Some(if allowed {
|
||||
&output
|
||||
} else {
|
||||
&without_raw_metadata
|
||||
}),
|
||||
"resolved endpoint: {base_url}, responses_lite: {responses_lite}",
|
||||
);
|
||||
assert_eq!(prompt.input, vec![output.clone()]);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn responses_http_omits_raw_tool_metadata_for_openai_named_custom_endpoint()
|
||||
-> anyhow::Result<()> {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/v1/responses"))
|
||||
.respond_with(
|
||||
ResponseTemplate::new(/*status*/ 200)
|
||||
.insert_header("content-type", "text/event-stream")
|
||||
.set_body_string(concat!(
|
||||
"data: {\"type\":\"response.created\",\"response\":{\"id\":\"resp-1\"}}\n\n",
|
||||
"data: {\"type\":\"response.completed\",\"response\":{\"id\":\"resp-1\"}}\n\n",
|
||||
)),
|
||||
)
|
||||
.expect(/*requests*/ 1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
let mut provider =
|
||||
ModelProviderInfo::create_openai_provider(Some(format!("{}/v1", server.uri())));
|
||||
provider.requires_openai_auth = false;
|
||||
provider.supports_websockets = false;
|
||||
let mut client = test_model_client(SessionSource::Cli);
|
||||
Arc::get_mut(&mut client.state)
|
||||
.expect("test client should have unique session state")
|
||||
.provider = create_model_provider(provider, /*auth_manager*/ None);
|
||||
let output = output_with_tool_result_metadata(ToolResultMetadata::new(&json!({
|
||||
"private": "raw-result-metadata",
|
||||
})));
|
||||
let prompt = Prompt {
|
||||
input: vec![output.clone()],
|
||||
..Default::default()
|
||||
};
|
||||
let responses_metadata = test_responses_metadata_for_client(
|
||||
&client,
|
||||
/*turn_id*/ None,
|
||||
format!("{}:0", client.state.thread_id),
|
||||
/*parent_thread_id*/ None,
|
||||
TestCodexResponsesRequestKind::Turn,
|
||||
);
|
||||
let mut session = client.new_session();
|
||||
let mut stream = session
|
||||
.stream(
|
||||
&prompt,
|
||||
&test_model_info(),
|
||||
&test_session_telemetry(),
|
||||
/*effort*/ None,
|
||||
codex_protocol::config_types::ReasoningSummary::None,
|
||||
/*service_tier*/ None,
|
||||
&responses_metadata,
|
||||
&InferenceTraceContext::disabled(),
|
||||
)
|
||||
.await?;
|
||||
let mut completed = false;
|
||||
while let Some(event) = stream.next().await {
|
||||
if let ResponseEvent::Completed { response_id, .. } = event? {
|
||||
assert_eq!(response_id, "resp-1");
|
||||
completed = true;
|
||||
}
|
||||
}
|
||||
assert!(completed);
|
||||
let requests = server.received_requests().await.expect("received requests");
|
||||
assert_eq!(requests.len(), 1);
|
||||
let body: serde_json::Value = serde_json::from_slice(&requests[0].body)?;
|
||||
assert_eq!(
|
||||
body["input"],
|
||||
serde_json::to_value(vec![output_with_tool_result_metadata(
|
||||
ToolResultMetadata::default(),
|
||||
)])?,
|
||||
);
|
||||
assert_eq!(prompt.input, vec![output]);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn responses_lite_prefix_ids_track_thread_and_payload() -> anyhow::Result<()> {
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
@@ -49,6 +49,21 @@ static TEST_MODEL_PRESETS: Lazy<Vec<ModelPreset>> = Lazy::new(|| {
|
||||
presets
|
||||
});
|
||||
|
||||
/// Reattaches request-only observations to a completed turn's history for capture assertions.
|
||||
/// Tests inspect this separately from the destination-filtered HTTP/WS request.
|
||||
pub async fn history_with_tool_call_metadata(
|
||||
thread: &crate::CodexThread,
|
||||
) -> Vec<codex_protocol::models::ResponseItem> {
|
||||
let history = thread.conversation_history_snapshot().await;
|
||||
let mut items = history.items().cloned().collect::<Vec<_>>();
|
||||
thread
|
||||
.session
|
||||
.services
|
||||
.executed_tool_calls
|
||||
.attach_to_prompt(&mut items, &mut Default::default());
|
||||
items
|
||||
}
|
||||
|
||||
/// Test-only provider that supplies no user instructions.
|
||||
#[derive(Debug, Default)]
|
||||
pub struct EmptyUserInstructionsProvider;
|
||||
|
||||
@@ -102,6 +102,8 @@ impl ToolInvocation {
|
||||
pub struct McpToolOutput {
|
||||
pub result: CallToolResult,
|
||||
pub tool_input: JsonValue,
|
||||
// Keep the original metadata for hooks; this flag only controls analytics capture.
|
||||
pub(crate) result_metadata_capture_allowed: bool,
|
||||
pub wall_time: Duration,
|
||||
pub original_image_detail_supported: bool,
|
||||
pub truncation_policy: TruncationPolicy,
|
||||
@@ -139,6 +141,13 @@ impl ToolOutput for McpToolOutput {
|
||||
self.result.code_mode_result(payload)
|
||||
}
|
||||
|
||||
fn tool_result_metadata(&self) -> Option<&JsonValue> {
|
||||
if !self.result_metadata_capture_allowed {
|
||||
return None;
|
||||
}
|
||||
self.result.meta.as_ref()
|
||||
}
|
||||
|
||||
fn post_tool_use_input(&self, _payload: &ToolPayload) -> Option<JsonValue> {
|
||||
Some(self.tool_input.clone())
|
||||
}
|
||||
|
||||
@@ -96,6 +96,7 @@ fn mcp_tool_output_response_item_includes_wall_time() {
|
||||
meta: None,
|
||||
},
|
||||
tool_input: json!({}),
|
||||
result_metadata_capture_allowed: false,
|
||||
wall_time: std::time::Duration::from_millis(1250),
|
||||
original_image_detail_supported: false,
|
||||
truncation_policy: TruncationPolicy::Bytes(1024),
|
||||
@@ -142,6 +143,7 @@ fn mcp_tool_output_response_item_truncates_large_structured_content() {
|
||||
meta: None,
|
||||
},
|
||||
tool_input: json!({}),
|
||||
result_metadata_capture_allowed: false,
|
||||
wall_time: std::time::Duration::from_millis(1250),
|
||||
original_image_detail_supported: false,
|
||||
truncation_policy: TruncationPolicy::Bytes(128),
|
||||
@@ -192,6 +194,7 @@ fn mcp_tool_output_response_item_preserves_content_items() {
|
||||
meta: None,
|
||||
},
|
||||
tool_input: json!({}),
|
||||
result_metadata_capture_allowed: false,
|
||||
wall_time: std::time::Duration::from_millis(500),
|
||||
original_image_detail_supported: false,
|
||||
truncation_policy: TruncationPolicy::Bytes(1024),
|
||||
@@ -251,6 +254,7 @@ fn mcp_tool_output_code_mode_result_preserves_content_without_private_metadata(
|
||||
})),
|
||||
},
|
||||
tool_input: json!({}),
|
||||
result_metadata_capture_allowed: false,
|
||||
wall_time: std::time::Duration::from_millis(1250),
|
||||
original_image_detail_supported: false,
|
||||
truncation_policy,
|
||||
|
||||
@@ -60,7 +60,7 @@ struct RetainedToolCalls {
|
||||
runtime_cell_id: Option<CellId>,
|
||||
// Invocation IDs stay local and are retained only with their original output's calls.
|
||||
call_index_by_id: HashMap<String, usize>,
|
||||
sources_updated: bool,
|
||||
result_metadata_updated: bool,
|
||||
}
|
||||
|
||||
#[derive(Default, PartialEq, Eq)]
|
||||
@@ -147,9 +147,9 @@ impl ExecutedToolCalls {
|
||||
result: &dyn ToolOutput,
|
||||
) {
|
||||
if self.state.is_some()
|
||||
&& let Some(sources) = result.tool_result_sources()
|
||||
&& let Some(metadata) = result.tool_result_metadata()
|
||||
{
|
||||
self.record_tool_result_sources(source, call_id, sources);
|
||||
self.record_tool_result_metadata(source, call_id, metadata);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -282,15 +282,17 @@ impl ExecutedToolCalls {
|
||||
state.pending_nested_calls += 1;
|
||||
}
|
||||
|
||||
fn record_tool_result_sources(
|
||||
fn record_tool_result_metadata(
|
||||
&self,
|
||||
source: &ToolCallSource,
|
||||
call_id: &str,
|
||||
result_sources: codex_protocol::models::ToolResultSources,
|
||||
metadata: &JsonValue,
|
||||
) -> bool {
|
||||
let Some(state) = &self.state else {
|
||||
return false;
|
||||
};
|
||||
let metadata = codex_protocol::models::ToolResultMetadata::new(metadata);
|
||||
let has_metadata = metadata.is_some();
|
||||
let mut state = state
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
@@ -304,7 +306,8 @@ impl ExecutedToolCalls {
|
||||
.and_then(|cell| cell.pending_calls.get_mut(call_id)),
|
||||
};
|
||||
if let Some(call) = call {
|
||||
return call.set_tool_result_sources(result_sources);
|
||||
call.set_tool_result_metadata(metadata);
|
||||
return has_metadata;
|
||||
}
|
||||
let ToolCallSource::CodeMode { cell_id, .. } = source else {
|
||||
return false;
|
||||
@@ -319,8 +322,9 @@ impl ExecutedToolCalls {
|
||||
return false;
|
||||
};
|
||||
// Older retry copies must not overwrite this output's accepted result metadata.
|
||||
retained.sources_updated = true;
|
||||
retained.calls[index].set_tool_result_sources(result_sources)
|
||||
retained.result_metadata_updated = true;
|
||||
retained.calls[index].set_tool_result_metadata(metadata);
|
||||
has_metadata
|
||||
}
|
||||
|
||||
pub(crate) fn register_cell(&self, cell_id: &CellId, output_call_id: &str) {
|
||||
|
||||
@@ -39,7 +39,7 @@ impl ExecutedToolCalls {
|
||||
!state
|
||||
.retained_calls
|
||||
.get(key)
|
||||
.is_some_and(|retained| retained.sources_updated)
|
||||
.is_some_and(|retained| retained.result_metadata_updated)
|
||||
});
|
||||
let mut pending_retry_outputs = retry_cache.keys().cloned().collect::<HashSet<_>>();
|
||||
let mut pending_retained_outputs =
|
||||
@@ -128,7 +128,7 @@ impl ExecutedToolCalls {
|
||||
cell_id: cell_id.clone(),
|
||||
runtime_cell_id,
|
||||
call_index_by_id,
|
||||
sources_updated: false,
|
||||
result_metadata_updated: false,
|
||||
},
|
||||
);
|
||||
calls
|
||||
@@ -189,11 +189,10 @@ impl ExecutedToolCalls {
|
||||
{
|
||||
let runtime_cell_id =
|
||||
previous.and_then(|retained| retained.runtime_cell_id.clone());
|
||||
// Budget rewriting or duplicate outputs invalidate the original index slots.
|
||||
// Metadata-only shedding preserves slots; changed calls or duplicate outputs do not.
|
||||
let call_index_by_id = previous
|
||||
.filter(|retained| {
|
||||
unique_output
|
||||
&& metadata.executed_tool_calls.as_ref() == Some(&retained.calls)
|
||||
unique_output && metadata.has_same_tool_calls(&retained.calls)
|
||||
})
|
||||
.map(|retained| retained.call_index_by_id.clone())
|
||||
.unwrap_or_default();
|
||||
@@ -205,8 +204,8 @@ impl ExecutedToolCalls {
|
||||
}
|
||||
retained.complete |= metadata.tool_calls_complete == Some(true);
|
||||
retained.call_index_by_id = call_index_by_id;
|
||||
retained.sources_updated =
|
||||
previous.is_some_and(|retained| retained.sources_updated);
|
||||
retained.result_metadata_updated =
|
||||
previous.is_some_and(|retained| retained.result_metadata_updated);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::models::ToolResultSources;
|
||||
use codex_protocol::models::ToolResultMetadata;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
|
||||
@@ -20,10 +20,10 @@ fn output(call_id: &str) -> ResponseItem {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn recorder_preserves_session_and_turn_gates_and_lazy_result_sources() {
|
||||
struct SourceLookup(std::cell::Cell<usize>);
|
||||
async fn recorder_preserves_session_and_turn_gates_and_lazy_result_metadata() {
|
||||
struct MetadataLookup(std::cell::Cell<usize>, JsonValue);
|
||||
|
||||
impl ToolOutput for SourceLookup {
|
||||
impl ToolOutput for MetadataLookup {
|
||||
fn log_output(&self) -> String {
|
||||
panic!("recording must not read the diagnostic output")
|
||||
}
|
||||
@@ -36,9 +36,9 @@ async fn recorder_preserves_session_and_turn_gates_and_lazy_result_sources() {
|
||||
panic!("recording must not rebuild the tool result")
|
||||
}
|
||||
|
||||
fn tool_result_sources(&self) -> Option<ToolResultSources> {
|
||||
fn tool_result_metadata(&self) -> Option<&JsonValue> {
|
||||
self.0.set(self.0.get() + 1);
|
||||
Some(ToolResultSources::new(Vec::new()))
|
||||
Some(&self.1)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -74,9 +74,9 @@ async fn recorder_preserves_session_and_turn_gates_and_lazy_result_sources() {
|
||||
encrypted_function_args: None,
|
||||
};
|
||||
calls.record_tool_call(&call, &ToolCallSource::Direct, &step);
|
||||
let result = SourceLookup(std::cell::Cell::new(0));
|
||||
let result = MetadataLookup(std::cell::Cell::new(0), json!({}));
|
||||
calls.record_accepted_result(&ToolCallSource::Direct, &call.call_id, &result);
|
||||
// Accepted sources follow session availability even when this turn stops recording calls.
|
||||
// Accepted metadata follows session availability even when this turn stops recording calls.
|
||||
assert_eq!(result.0.get(), usize::from(session_enabled));
|
||||
|
||||
let input = serde_json::from_value(json!({
|
||||
@@ -97,7 +97,7 @@ async fn recorder_preserves_session_and_turn_gates_and_lazy_result_sources() {
|
||||
}
|
||||
let mut expected_call =
|
||||
ExecutedToolCall::new("test_tool".to_string(), json!({"value": 7}));
|
||||
assert!(expected_call.set_tool_result_sources(ToolResultSources::new(Vec::new())));
|
||||
expected_call.set_tool_result_metadata(ToolResultMetadata::new(&json!({})));
|
||||
let expected_calls = vec![expected_call];
|
||||
assert_eq!(
|
||||
prompt[1]
|
||||
@@ -128,11 +128,8 @@ fn executed_tool_call_recorder_bounds_pending_calls_and_preserves_overflow() {
|
||||
ToolMode::Direct,
|
||||
);
|
||||
}
|
||||
assert!(recorder.record_tool_result_sources(
|
||||
&ToolCallSource::Direct,
|
||||
"direct-0",
|
||||
ToolResultSources::new(Vec::new())
|
||||
));
|
||||
let metadata = json!({ "arbitrary-key": ["R1", null] });
|
||||
assert!(recorder.record_tool_result_metadata(&ToolCallSource::Direct, "direct-0", &metadata));
|
||||
|
||||
let cell_id = CellId::new("bounded-cell".to_string());
|
||||
recorder.start_cell(&cell_id, "bounded-output");
|
||||
@@ -163,6 +160,10 @@ fn executed_tool_call_recorder_bounds_pending_calls_and_preserves_overflow() {
|
||||
state.direct_calls.len(),
|
||||
MAX_PENDING_EXECUTED_TOOL_CALLS + 1
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::to_value(state.direct_calls.get("direct-0").unwrap()).unwrap()["tool_result_metadata"],
|
||||
metadata,
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::to_value(
|
||||
state
|
||||
@@ -384,16 +385,8 @@ fn tool_call_completeness_requires_finished_lossless_recording() {
|
||||
|
||||
#[test]
|
||||
fn tool_call_completeness_survives_waits_without_changing_deltas() {
|
||||
let sources = ToolResultSources::new(vec![codex_protocol::models::ToolResultSource {
|
||||
r#type: "test_channel".to_string(),
|
||||
id: "CLATE".to_string(),
|
||||
}]);
|
||||
for (truncated, sources) in [
|
||||
(false, sources.clone()),
|
||||
(true, sources),
|
||||
(false, ToolResultSources::parse_failed()),
|
||||
(true, ToolResultSources::parse_failed()),
|
||||
] {
|
||||
let metadata = json!({ "provider": { "ids": ["CLATE"] } });
|
||||
for truncated in [false, true] {
|
||||
let recorder = new_recorder();
|
||||
let cell_id = CellId::new("multi-wait".to_string());
|
||||
recorder.start_cell(&cell_id, "exec");
|
||||
@@ -431,30 +424,22 @@ fn tool_call_completeness_survives_waits_without_changing_deltas() {
|
||||
cell_id: cell_id.to_string(),
|
||||
runtime_tool_call_id: "runtime-call".to_string(),
|
||||
};
|
||||
assert!(!recorder.record_tool_result_sources(
|
||||
assert!(!recorder.record_tool_result_metadata(
|
||||
&source("other-cell"),
|
||||
"nested-0",
|
||||
ToolResultSources::new(Vec::new()),
|
||||
&json!({}),
|
||||
));
|
||||
let source = source(cell_id.as_str());
|
||||
assert!(recorder.record_tool_result_sources(
|
||||
&source,
|
||||
"nested-0",
|
||||
sources.clone(),
|
||||
));
|
||||
assert!(recorder.record_tool_result_metadata(&source, "nested-0", &metadata));
|
||||
let mut delayed_call =
|
||||
ExecutedToolCall::new("nested_tool".to_string(), json!({}));
|
||||
assert!(delayed_call.set_tool_result_sources(sources.clone()));
|
||||
delayed_call.set_tool_result_metadata(ToolResultMetadata::new(&metadata));
|
||||
let mut delayed_output = output("exec");
|
||||
delayed_output.append_executed_tool_calls(vec![delayed_call]);
|
||||
delayed_output.set_tool_call_cell_id("exec");
|
||||
expected[0] = delayed_output;
|
||||
assert!(recorder.record_tool_result_sources(
|
||||
&source,
|
||||
"nested-1",
|
||||
ToolResultSources::new(Vec::new())
|
||||
));
|
||||
assert!(call.set_tool_result_sources(ToolResultSources::new(Vec::new())));
|
||||
assert!(recorder.record_tool_result_metadata(&source, "nested-1", &json!({})));
|
||||
call.set_tool_result_metadata(ToolResultMetadata::new(&json!({})));
|
||||
}
|
||||
expected_output.append_executed_tool_calls(vec![call]);
|
||||
} else if index == 3 {
|
||||
@@ -548,10 +533,10 @@ fn request_truncation_prevents_completion_after_compaction() {
|
||||
runtime_tool_call_id: "runtime-call".to_string(),
|
||||
};
|
||||
for index in 0..4 {
|
||||
assert!(!recorder.record_tool_result_sources(
|
||||
assert!(!recorder.record_tool_result_metadata(
|
||||
&source,
|
||||
&format!("nested-{index}"),
|
||||
ToolResultSources::new(Vec::new()),
|
||||
&json!({}),
|
||||
));
|
||||
}
|
||||
|
||||
@@ -569,7 +554,58 @@ fn request_truncation_prevents_completion_after_compaction() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn source_shedding_preserves_completion_after_compaction() {
|
||||
fn result_metadata_updates_the_exact_retained_call_and_marks_oversized_retries() {
|
||||
let recorder = new_recorder();
|
||||
let cell_id = CellId::new("metadata-cell".to_string());
|
||||
recorder.start_cell(&cell_id, "exec");
|
||||
let call = ExecutedToolCall::new("nested_tool".to_string(), json!({}));
|
||||
for call_id in ["first", "second"] {
|
||||
recorder.record_nested_tool_call(
|
||||
cell_id.clone(),
|
||||
call_id.to_string(),
|
||||
call.clone(),
|
||||
/*original_bytes*/ 2,
|
||||
);
|
||||
}
|
||||
recorder.finish_cell_recording(&cell_id);
|
||||
let mut retry_cache = HashMap::new();
|
||||
let mut initial = [output("exec")];
|
||||
assert!(recorder.attach_pending_to_prompt(&mut initial, &mut retry_cache));
|
||||
|
||||
let metadata = json!({ "all-keys": { "ids": ["R1"] }, "other": true });
|
||||
let source = |cell_id: &str| ToolCallSource::CodeMode {
|
||||
cell_id: cell_id.to_string(),
|
||||
runtime_tool_call_id: "runtime-call".to_string(),
|
||||
};
|
||||
assert!(!recorder.record_tool_result_metadata(&source("other-cell"), "first", &metadata));
|
||||
assert!(!recorder.record_tool_result_metadata(&source(cell_id.as_str()), "unknown", &metadata));
|
||||
assert!(recorder.record_tool_result_metadata(&source(cell_id.as_str()), "first", &metadata));
|
||||
|
||||
let mut expected_call = call.clone();
|
||||
expected_call.set_tool_result_metadata(ToolResultMetadata::new(&metadata));
|
||||
let mut expected = output("exec");
|
||||
expected.append_executed_tool_calls(vec![expected_call, call]);
|
||||
expected.set_tool_call_cell_id("exec");
|
||||
expected.mark_tool_calls_complete();
|
||||
let mut retry = [output("exec")];
|
||||
assert!(recorder.attach_pending_to_prompt(&mut retry, &mut retry_cache));
|
||||
assert_eq!(retry, [expected]);
|
||||
|
||||
assert!(recorder.record_tool_result_metadata(
|
||||
&source(cell_id.as_str()),
|
||||
"first",
|
||||
&json!({ "oversized": "x".repeat(32 * 1024) }),
|
||||
));
|
||||
let mut expected = serde_json::to_value(&initial).unwrap();
|
||||
expected[0]["internal_chat_message_metadata_passthrough"]["executed_tool_calls"][0]["tool_result_metadata"] =
|
||||
json!("omitted_due_to_size_limit");
|
||||
let mut retry = [output("exec")];
|
||||
assert!(recorder.attach_pending_to_prompt(&mut retry, &mut retry_cache));
|
||||
assert_eq!(serde_json::to_value(&retry).unwrap(), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn result_metadata_shedding_preserves_completion_after_compaction() {
|
||||
let recorder = new_recorder();
|
||||
let cell_id = CellId::new("compacted-cell".to_string());
|
||||
recorder.start_cell(&cell_id, "exec");
|
||||
@@ -592,52 +628,48 @@ fn source_shedding_preserves_completion_after_compaction() {
|
||||
};
|
||||
let mut retry_cache = HashMap::new();
|
||||
recorder.attach_pending_to_prompt(&mut [output("exec")], &mut retry_cache);
|
||||
assert!(
|
||||
recorder.record_tool_result_sources(
|
||||
&source,
|
||||
"nested-0",
|
||||
ToolResultSources::new(
|
||||
(0..32)
|
||||
.map(|index| codex_protocol::models::ToolResultSource {
|
||||
r#type: "test_channel".to_string(),
|
||||
id: format!("C{index:063}"),
|
||||
})
|
||||
.collect()
|
||||
),
|
||||
)
|
||||
);
|
||||
// This snapshot fits alone, but pushes the four calls over the aggregate budget.
|
||||
assert!(recorder.record_tool_result_metadata(
|
||||
&source,
|
||||
"nested-0",
|
||||
&json!({ "provider_data": "x".repeat(4 * 1024) }),
|
||||
));
|
||||
|
||||
assert!(recorder.record_tool_result_sources(
|
||||
assert!(recorder.record_tool_result_metadata(
|
||||
&source,
|
||||
"nested-1",
|
||||
ToolResultSources::parse_failed()
|
||||
&json!({ "status": "unavailable" })
|
||||
));
|
||||
let mut initial = [output("exec")];
|
||||
assert!(recorder.attach_pending_to_prompt(&mut initial, &mut retry_cache));
|
||||
let calls = initial[0]
|
||||
.executed_tool_call_metadata()
|
||||
.and_then(|metadata| metadata.executed_tool_calls.as_ref())
|
||||
.expect("source shedding must preserve recorded calls");
|
||||
assert_eq!(
|
||||
calls,
|
||||
&vec![ExecutedToolCall::new("nested_tool".to_string(), arguments); 4]
|
||||
);
|
||||
assert!(!recorder.record_tool_result_sources(
|
||||
&source,
|
||||
"nested-0",
|
||||
ToolResultSources::new(Vec::new())
|
||||
));
|
||||
.expect("metadata shedding must preserve recorded calls");
|
||||
let mut expected_calls = vec![ExecutedToolCall::new("nested_tool".to_string(), arguments); 4];
|
||||
for call in &mut expected_calls[..2] {
|
||||
call.set_tool_result_metadata(ToolResultMetadata::new(&json!("omitted_due_to_size_limit")));
|
||||
}
|
||||
assert_eq!(calls, &expected_calls);
|
||||
let mut retry = [output("exec")];
|
||||
assert!(recorder.attach_pending_to_prompt(&mut retry, &mut retry_cache));
|
||||
assert_eq!(retry, initial);
|
||||
|
||||
// A different call can still attach a small result after the oversized metadata is shed.
|
||||
let metadata = json!({ "id": "R2" });
|
||||
assert!(recorder.record_tool_result_metadata(&source, "nested-2", &metadata));
|
||||
let mut expected_calls = calls.clone();
|
||||
expected_calls[2].set_tool_result_metadata(ToolResultMetadata::new(&metadata));
|
||||
let mut expected = output("exec");
|
||||
expected.append_executed_tool_calls(expected_calls);
|
||||
expected.set_tool_call_cell_id("exec");
|
||||
let mut retry = [output("exec")];
|
||||
assert!(recorder.attach_pending_to_prompt(&mut retry, &mut retry_cache));
|
||||
assert_eq!(retry, [expected]);
|
||||
|
||||
recorder.attach_pending_to_prompt(&mut [], &mut retry_cache);
|
||||
assert!(retry_cache.is_empty());
|
||||
assert!(!recorder.record_tool_result_sources(
|
||||
&source,
|
||||
"nested-1",
|
||||
ToolResultSources::new(Vec::new())
|
||||
));
|
||||
assert!(!recorder.record_tool_result_metadata(&source, "nested-2", &metadata));
|
||||
recorder.register_cell(&cell_id, "wait");
|
||||
recorder.finish_cell_recording(&cell_id);
|
||||
let mut final_output = [output("wait")];
|
||||
|
||||
@@ -184,6 +184,9 @@ impl McpHandler {
|
||||
self.tool_info.tool.name.as_ref(),
|
||||
)
|
||||
.await;
|
||||
// Use the executed call's binding; a later catalog refresh must not change eligibility.
|
||||
// Only the new metadata is internal; tool execution and call accounting are not.
|
||||
let result_metadata_capture_allowed = false;
|
||||
let mcp_tool = prepared_mcp_call.as_ref().map(|call| {
|
||||
McpToolContext::from_prepared_call(
|
||||
call,
|
||||
@@ -241,6 +244,7 @@ impl McpHandler {
|
||||
Ok(boxed_tool_output(McpToolOutput {
|
||||
result: result.result,
|
||||
tool_input: result.tool_input,
|
||||
result_metadata_capture_allowed,
|
||||
wall_time: started.elapsed(),
|
||||
original_image_detail_supported: can_request_original_image_detail(
|
||||
&step_context.settings.model_info,
|
||||
@@ -566,6 +570,7 @@ mod tests {
|
||||
use crate::tools::registry::PostToolUsePayload;
|
||||
use crate::tools::registry::PreToolUsePayload;
|
||||
use crate::turn_diff_tracker::TurnDiffTracker;
|
||||
use codex_features::Feature;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::time::Duration;
|
||||
@@ -692,11 +697,21 @@ mod tests {
|
||||
"file_id": "file_123"
|
||||
}
|
||||
}),
|
||||
result_metadata_capture_allowed: false,
|
||||
wall_time: Duration::from_millis(42),
|
||||
original_image_detail_supported: true,
|
||||
truncation_policy: codex_utils_output_truncation::TruncationPolicy::Bytes(1024),
|
||||
};
|
||||
let (session, turn) = make_session_and_context().await;
|
||||
let mut session = session;
|
||||
let mut turn = turn;
|
||||
Arc::make_mut(&mut turn.config)
|
||||
.features
|
||||
.enable(Feature::ExecutedToolCallMetadata)
|
||||
.expect("test feature must be configurable");
|
||||
let recorder =
|
||||
crate::tools::executed_tool_calls::ExecutedToolCalls::new(&turn.config.features);
|
||||
session.services.executed_tool_calls = recorder.clone();
|
||||
let turn = Arc::new(turn);
|
||||
let handler = McpHandler::new(tool_info("filesystem", "filesystem", "read_file"))
|
||||
.expect("MCP tool spec should build");
|
||||
@@ -730,6 +745,34 @@ mod tests {
|
||||
}),
|
||||
})
|
||||
);
|
||||
|
||||
// Reuse the direct invocation to catch recording placed after the Code Mode-only return.
|
||||
let mut output = output;
|
||||
output.result.meta = Some(json!({ "provider/custom": { "items": [1, null] } }));
|
||||
output.result_metadata_capture_allowed = true;
|
||||
recorder.record_tool_call(
|
||||
&crate::tools::router::ToolCall {
|
||||
tool_name: invocation.tool_name.clone(),
|
||||
call_id: invocation.call_id.clone(),
|
||||
payload: invocation.payload.clone(),
|
||||
encrypted_function_args: None,
|
||||
},
|
||||
&invocation.source,
|
||||
&invocation.step_context,
|
||||
);
|
||||
handler.on_tool_result_accepted(&invocation, &output);
|
||||
let mut items = [codex_protocol::models::ResponseItem::from(
|
||||
output.to_response_item(&invocation.call_id, &invocation.payload),
|
||||
)];
|
||||
recorder.attach_to_prompt(&mut items, &mut Default::default());
|
||||
assert_eq!(
|
||||
serde_json::to_value(items[0].executed_tool_call_metadata()).unwrap()["executed_tool_calls"],
|
||||
json!([{
|
||||
"name": codex_tools::code_mode_name_for_tool_name(&invocation.tool_name),
|
||||
"arguments": { "path": "/tmp/notes.txt" },
|
||||
"tool_result_metadata": { "provider/custom": { "items": [1, null] } },
|
||||
}]),
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -243,8 +243,8 @@ impl ToolOutput for PostToolUseFeedbackOutput {
|
||||
self.original.code_mode_result(payload)
|
||||
}
|
||||
|
||||
fn tool_result_sources(&self) -> Option<codex_protocol::models::ToolResultSources> {
|
||||
self.original.tool_result_sources()
|
||||
fn tool_result_metadata(&self) -> Option<&Value> {
|
||||
self.original.tool_result_metadata()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -609,6 +609,7 @@ fn post_tool_use_feedback_output_preserves_fallback_token_limit_override(
|
||||
meta: None,
|
||||
},
|
||||
tool_input: serde_json::json!({}),
|
||||
result_metadata_capture_allowed: false,
|
||||
wall_time: Duration::ZERO,
|
||||
original_image_detail_supported: false,
|
||||
truncation_policy,
|
||||
@@ -691,6 +692,43 @@ fn post_tool_use_feedback_output_keeps_code_mode_result_typed() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test_case::test_case(false; "success")]
|
||||
#[test_case::test_case(true; "tool error")]
|
||||
fn post_tool_use_feedback_output_preserves_mcp_result_metadata(tool_error: bool) {
|
||||
let metadata = serde_json::json!({
|
||||
"openai/resource_access": { "resources": [] },
|
||||
"provider/custom": { "items": [1, null, { "value": true }] },
|
||||
});
|
||||
let result = PostToolUseFeedbackOutput {
|
||||
original: Box::new(crate::tools::context::McpToolOutput {
|
||||
result: codex_protocol::mcp::CallToolResult {
|
||||
content: vec![serde_json::json!({
|
||||
"type": "text",
|
||||
"text": "original result",
|
||||
})],
|
||||
structured_content: None,
|
||||
is_error: Some(tool_error),
|
||||
meta: Some(metadata.clone()),
|
||||
},
|
||||
tool_input: serde_json::json!({ "query": "rewritten query" }),
|
||||
result_metadata_capture_allowed: true,
|
||||
wall_time: std::time::Duration::ZERO,
|
||||
original_image_detail_supported: false,
|
||||
truncation_policy: codex_utils_output_truncation::TruncationPolicy::Bytes(64),
|
||||
}),
|
||||
model_visible: FunctionToolOutput::from_text(
|
||||
"unrelated hook feedback".to_string(),
|
||||
/*success*/ None,
|
||||
),
|
||||
};
|
||||
let payload = ToolPayload::Function {
|
||||
arguments: "{}".to_string(),
|
||||
};
|
||||
let original_result = result.original.code_mode_result(&payload);
|
||||
assert_eq!(result.tool_result_metadata(), Some(&metadata));
|
||||
assert_eq!(result.code_mode_result(&payload), original_result);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dispatch_uses_canonical_tool_names_for_lifecycle_contributors() -> anyhow::Result<()> {
|
||||
let (mut session, turn) = crate::session::tests::make_session_and_context().await;
|
||||
|
||||
Reference in New Issue
Block a user