mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Keep history extension tools out of Guardian reviews (#41861)
## What changed - Initialize Guardian reviewer subthreads with empty extension data. - Remove the special registration that exposed inherited read-only `history` tools in the Guardian tool plan. GitOrigin-RevId: 7700f033faab125d59d11bc2d82955c67689e3c3
This commit is contained in:
1 parent
09f4c45068
commit
2c8cfbf44f
3 files changed
+1
-215
No files matched your search
@@ -9,10 +9,8 @@ use codex_app_server_protocol::ThreadStartParams;
|
|||||||
use codex_app_server_protocol::TurnStartParams;
|
use codex_app_server_protocol::TurnStartParams;
|
||||||
use codex_app_server_protocol::UserInput;
|
use codex_app_server_protocol::UserInput;
|
||||||
use codex_config::types::AuthCredentialsStoreMode;
|
use codex_config::types::AuthCredentialsStoreMode;
|
||||||
use codex_features::Feature;
|
|
||||||
use core_test_support::load_default_config_for_test;
|
use core_test_support::load_default_config_for_test;
|
||||||
use core_test_support::responses;
|
use core_test_support::responses;
|
||||||
use core_test_support::skip_if_wine_exec;
|
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
use serde_json::json;
|
use serde_json::json;
|
||||||
@@ -223,181 +221,6 @@ async fn app_server_uses_configured_notes_backend_for_context_window_hints(
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
||||||
async fn guardian_can_read_parent_history_without_inheriting_notes_tools() -> Result<()> {
|
|
||||||
// TODO: Remove after Guardian accepts executor-native cwd across host operating systems.
|
|
||||||
skip_if_wine_exec!(
|
|
||||||
Ok(()),
|
|
||||||
"Guardian approval currently rejects a Windows executor cwd on the Linux host"
|
|
||||||
);
|
|
||||||
|
|
||||||
let server = responses::start_mock_server().await;
|
|
||||||
Mock::given(method("POST"))
|
|
||||||
.and(path("/backend-api/codex/alpha/notes/v2/thread_hint"))
|
|
||||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({"text": ""})))
|
|
||||||
.expect(1)
|
|
||||||
.mount(&server)
|
|
||||||
.await;
|
|
||||||
Mock::given(method("POST"))
|
|
||||||
.and(path("/backend-api/codex/alpha/history/v2/list_items"))
|
|
||||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
|
||||||
"results": [],
|
|
||||||
"n_returned": 0,
|
|
||||||
})))
|
|
||||||
.expect(1)
|
|
||||||
.mount(&server)
|
|
||||||
.await;
|
|
||||||
let response_mock = responses::mount_sse_sequence(
|
|
||||||
&server,
|
|
||||||
vec![
|
|
||||||
responses::sse(vec![
|
|
||||||
responses::ev_response_created("resp-parent-command"),
|
|
||||||
responses::ev_function_call(
|
|
||||||
"parent-shell-call",
|
|
||||||
"exec_command",
|
|
||||||
&json!({
|
|
||||||
"cmd": "echo guardian",
|
|
||||||
"sandbox_permissions": "require_escalated",
|
|
||||||
"justification": "Review a command using parent history.",
|
|
||||||
})
|
|
||||||
.to_string(),
|
|
||||||
),
|
|
||||||
responses::ev_completed("resp-parent-command"),
|
|
||||||
]),
|
|
||||||
responses::sse(vec![
|
|
||||||
responses::ev_response_created("resp-guardian-history"),
|
|
||||||
responses::ev_function_call_with_namespace(
|
|
||||||
"guardian-history-call",
|
|
||||||
"history",
|
|
||||||
"list_items",
|
|
||||||
&json!({"role": "user"}).to_string(),
|
|
||||||
),
|
|
||||||
responses::ev_completed("resp-guardian-history"),
|
|
||||||
]),
|
|
||||||
responses::sse(vec![
|
|
||||||
responses::ev_response_created("resp-guardian-review"),
|
|
||||||
responses::ev_assistant_message(
|
|
||||||
"guardian-review",
|
|
||||||
&json!({
|
|
||||||
"outcome": "deny",
|
|
||||||
"rationale": "The original user instructions do not authorize this command.",
|
|
||||||
})
|
|
||||||
.to_string(),
|
|
||||||
),
|
|
||||||
responses::ev_completed("resp-guardian-review"),
|
|
||||||
]),
|
|
||||||
responses::sse(vec![
|
|
||||||
responses::ev_response_created("resp-parent-done"),
|
|
||||||
responses::ev_assistant_message("parent-done", "Done"),
|
|
||||||
responses::ev_completed("resp-parent-done"),
|
|
||||||
]),
|
|
||||||
],
|
|
||||||
)
|
|
||||||
.await;
|
|
||||||
|
|
||||||
let codex_home = TempDir::new()?;
|
|
||||||
MockResponsesConfig::new(&server.uri())
|
|
||||||
.with_model_provider("openai-custom")
|
|
||||||
.with_provider_name("OpenAI")
|
|
||||||
.with_provider_base_url(&format!("{}/backend-api/codex", server.uri()))
|
|
||||||
.with_provider_config("supports_websockets = false\nrequires_openai_auth = true")
|
|
||||||
.with_approval_policy("on-request")
|
|
||||||
.with_root_config("approvals_reviewer = \"auto_review\"")
|
|
||||||
.enable_feature(Feature::GuardianApproval)
|
|
||||||
.with_extra_config(
|
|
||||||
"[features.token_budget]\nenabled = true\nuse_history_notes_extension = true",
|
|
||||||
)
|
|
||||||
.write(codex_home.path())?;
|
|
||||||
write_chatgpt_auth(
|
|
||||||
codex_home.path(),
|
|
||||||
ChatGptAuthFixture::new("access-chatgpt"),
|
|
||||||
AuthCredentialsStoreMode::File,
|
|
||||||
)?;
|
|
||||||
|
|
||||||
let mut app_server = TestAppServer::builder()
|
|
||||||
.with_codex_home(codex_home.path())
|
|
||||||
.with_env_overrides(&[("OPENAI_API_KEY", None)])
|
|
||||||
.build_initialized()
|
|
||||||
.await?;
|
|
||||||
let thread = app_server
|
|
||||||
.start_thread(ThreadStartParams::default())
|
|
||||||
.await?
|
|
||||||
.thread;
|
|
||||||
timeout(
|
|
||||||
DEFAULT_READ_TIMEOUT,
|
|
||||||
app_server.start_turn_and_wait_for_completion(TurnStartParams {
|
|
||||||
thread_id: thread.id.clone(),
|
|
||||||
input: vec![UserInput::Text {
|
|
||||||
text: "Review a command using the original user instructions".to_string(),
|
|
||||||
text_elements: Vec::new(),
|
|
||||||
}],
|
|
||||||
..Default::default()
|
|
||||||
}),
|
|
||||||
)
|
|
||||||
.await??;
|
|
||||||
|
|
||||||
let requests = response_mock.requests();
|
|
||||||
assert_eq!(requests.len(), 4);
|
|
||||||
let guardian_request = &requests[1];
|
|
||||||
let guardian_body = guardian_request.body_json();
|
|
||||||
assert_eq!(
|
|
||||||
guardian_body["client_metadata"]["x-openai-subagent"],
|
|
||||||
"guardian"
|
|
||||||
);
|
|
||||||
let guardian_tools = guardian_body.get("tools").or_else(|| {
|
|
||||||
guardian_body["input"]
|
|
||||||
.as_array()?
|
|
||||||
.iter()
|
|
||||||
.find(|item| item["type"] == "additional_tools")?
|
|
||||||
.get("tools")
|
|
||||||
});
|
|
||||||
let guardian_tools = json!({"tools": guardian_tools.expect("Guardian tool definitions")});
|
|
||||||
for tool_name in ["list_windows", "list_items", "read_item", "search_contents"] {
|
|
||||||
assert!(
|
|
||||||
responses::namespace_child_tool(&guardian_tools, "history", tool_name).is_some(),
|
|
||||||
"Guardian should expose history.{tool_name}"
|
|
||||||
);
|
|
||||||
}
|
|
||||||
for tool_name in [
|
|
||||||
"list_files_by_prefix",
|
|
||||||
"read_file",
|
|
||||||
"search_contents",
|
|
||||||
"append_to_file",
|
|
||||||
"write_file",
|
|
||||||
] {
|
|
||||||
assert!(
|
|
||||||
requests[0].tool_by_name("notes", tool_name).is_some(),
|
|
||||||
"the parent should retain notes.{tool_name}"
|
|
||||||
);
|
|
||||||
assert!(
|
|
||||||
responses::namespace_child_tool(&guardian_tools, "notes", tool_name).is_none(),
|
|
||||||
"Guardian must not inherit notes.{tool_name}"
|
|
||||||
);
|
|
||||||
}
|
|
||||||
assert_eq!(
|
|
||||||
requests[2].function_call_output_text("guardian-history-call"),
|
|
||||||
Some(json!({"results": [], "n_returned": 0}).to_string())
|
|
||||||
);
|
|
||||||
|
|
||||||
let backend_requests = server.received_requests().await.expect("recorded requests");
|
|
||||||
let history_request = backend_requests
|
|
||||||
.iter()
|
|
||||||
.find(|request| request.url.path() == "/backend-api/codex/alpha/history/v2/list_items")
|
|
||||||
.expect("Guardian history request");
|
|
||||||
assert_eq!(
|
|
||||||
history_request.body_json::<Value>()?,
|
|
||||||
json!({
|
|
||||||
"role": "user",
|
|
||||||
"context": {
|
|
||||||
"session_id": thread.id,
|
|
||||||
"current_agent_name": "/root",
|
|
||||||
},
|
|
||||||
})
|
|
||||||
);
|
|
||||||
|
|
||||||
Ok(())
|
|
||||||
}
|
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn history_notes_and_async_message_emit_control_tool_analytics() -> Result<()> {
|
async fn history_notes_and_async_message_emit_control_tool_analytics() -> Result<()> {
|
||||||
let encrypted_query = format!("enc_query_{}", "x".repeat(1_001));
|
let encrypted_query = format!("enc_query_{}", "x".repeat(1_001));
|
||||||
|
|||||||
@@ -42,10 +42,6 @@ use codex_protocol::turn_input::TurnStartOptions;
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
use crate::session::completed_session_loop_termination;
|
use crate::session::completed_session_loop_termination;
|
||||||
|
|
||||||
pub(crate) struct GuardianReadOnlyHistoryTools(
|
|
||||||
pub(crate) Vec<Arc<dyn for<'call> codex_tools::ToolExecutor<codex_tools::ToolCall<'call>>>>,
|
|
||||||
);
|
|
||||||
|
|
||||||
/// Start an interactive sub-Codex thread and return its runtime and IO channels.
|
/// Start an interactive sub-Codex thread and return its runtime and IO channels.
|
||||||
///
|
///
|
||||||
/// Delegates never request approvals, and the returned IO yields their public events.
|
/// Delegates never request approvals, and the returned IO yields their public events.
|
||||||
@@ -85,27 +81,6 @@ pub(crate) async fn run_codex_thread_interactive(
|
|||||||
};
|
};
|
||||||
let session_source = SessionSource::SubAgent(subagent_source.clone());
|
let session_source = SessionSource::SubAgent(subagent_source.clone());
|
||||||
let is_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source);
|
let is_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source);
|
||||||
let mut thread_extension_init = codex_extension_api::ExtensionDataInit::default();
|
|
||||||
if is_guardian_reviewer {
|
|
||||||
let history_tools = crate::tools::spec_plan::extension_tool_executors(
|
|
||||||
parent_session.as_ref(),
|
|
||||||
parent_ctx.extension_data.as_ref(),
|
|
||||||
)
|
|
||||||
.filter(|executor| {
|
|
||||||
let name = executor.tool_name();
|
|
||||||
matches!(
|
|
||||||
(name.namespace.as_deref(), name.name.as_str()),
|
|
||||||
(
|
|
||||||
Some("history"),
|
|
||||||
"list_windows" | "list_items" | "read_item" | "search_contents"
|
|
||||||
)
|
|
||||||
)
|
|
||||||
})
|
|
||||||
.collect::<Vec<_>>();
|
|
||||||
if !history_tools.is_empty() {
|
|
||||||
thread_extension_init.insert(GuardianReadOnlyHistoryTools(history_tools));
|
|
||||||
}
|
|
||||||
}
|
|
||||||
let extensions = if is_guardian_reviewer {
|
let extensions = if is_guardian_reviewer {
|
||||||
codex_extension_api::empty_extension_registry()
|
codex_extension_api::empty_extension_registry()
|
||||||
} else {
|
} else {
|
||||||
@@ -148,7 +123,7 @@ pub(crate) async fn run_codex_thread_interactive(
|
|||||||
parent_rollout_thread_trace: codex_rollout_trace::ThreadTraceContext::disabled(),
|
parent_rollout_thread_trace: codex_rollout_trace::ThreadTraceContext::disabled(),
|
||||||
parent_trace: None,
|
parent_trace: None,
|
||||||
environment_selections: parent_environments.to_selections(),
|
environment_selections: parent_environments.to_selections(),
|
||||||
thread_extension_init,
|
thread_extension_init: codex_extension_api::ExtensionDataInit::default(),
|
||||||
client_mcp_extensions: parent_session.services.client_mcp_extensions.clone(),
|
client_mcp_extensions: parent_session.services.client_mcp_extensions.clone(),
|
||||||
reserved_thread_id: None,
|
reserved_thread_id: None,
|
||||||
analytics_events_client: Some(parent_session.services.analytics_events_client.clone()),
|
analytics_events_client: Some(parent_session.services.analytics_events_client.clone()),
|
||||||
|
|||||||
@@ -153,18 +153,6 @@ pub(crate) fn build_tool_router(
|
|||||||
add_core_tool_sources(&context, &mut registry);
|
add_core_tool_sources(&context, &mut registry);
|
||||||
|
|
||||||
let hosted_specs = if crate::guardian::is_basic_session_source(&turn_context.session_source) {
|
let hosted_specs = if crate::guardian::is_basic_session_source(&turn_context.session_source) {
|
||||||
if let Some(history_tools) = session
|
|
||||||
.services
|
|
||||||
.thread_extension_data
|
|
||||||
.get::<crate::codex_delegate::GuardianReadOnlyHistoryTools>()
|
|
||||||
{
|
|
||||||
append_extension_tool_executors(
|
|
||||||
turn_context,
|
|
||||||
model_info,
|
|
||||||
history_tools.0.iter().cloned(),
|
|
||||||
&mut registry,
|
|
||||||
);
|
|
||||||
}
|
|
||||||
Vec::new()
|
Vec::new()
|
||||||
} else {
|
} else {
|
||||||
let registered_mcp_tools = session.services.mcp_handler_cache.append_mcp_tools(
|
let registered_mcp_tools = session.services.mcp_handler_cache.append_mcp_tools(
|
||||||
|
|||||||
Reference in new issue
Block a user