mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Include node_repl screenshots in Guardian v2 reviews (#39227)
## Why `node_repl` screenshots are nested tool results and do not appear in the parent conversation history, so Guardian v2 transcript image collection could miss them. ## What changed - Allow Guardian v2 to enable bounded, thread-scoped `node_repl` image capture when transcript images are configured. - Add retained `node_repl` screenshots to review requests when transcript sources include tool outputs, while deduplicating images and keeping them out of the parent turn input. ## Testing - Add an integration test covering screenshot capture without the synchronous Guardian transcript feature flags and verifying that screenshots stay out of the parent request. GitOrigin-RevId: 5f70c5f2e80389aa8b2a286c3b7f9b1104865486
This commit is contained in:
@@ -67,7 +67,7 @@ pub(crate) use model_switch_instructions::ModelSwitchInstructions;
|
||||
pub(crate) use multi_agent_role_instructions::MultiAgentRoleInstructions;
|
||||
pub(crate) use multi_agent_usage_hint::MultiAgentUsageHint;
|
||||
pub(crate) use network_rule_saved::NetworkRuleSaved;
|
||||
pub(crate) use node_repl_review_evidence::NodeReplReviewEvidence;
|
||||
pub use node_repl_review_evidence::NodeReplReviewEvidence;
|
||||
pub(crate) use node_repl_review_evidence::NodeReplReviewEvidenceMode;
|
||||
pub(crate) use node_repl_review_evidence::node_repl_review_evidence_mode;
|
||||
pub use permissions_instructions::ApprovalPromptContext;
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::sync::Mutex;
|
||||
use std::sync::PoisonError;
|
||||
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_protocol::user_input::UserInput::Image;
|
||||
use codex_protocol::user_input::UserInput::Text;
|
||||
@@ -84,14 +85,51 @@ struct NodeReplReviewEvidenceState {
|
||||
responses: VecDeque<Arc<NodeReplReviewResponse>>,
|
||||
next_sequence: u64,
|
||||
retained_bytes: usize,
|
||||
image_capture_enabled: bool,
|
||||
}
|
||||
|
||||
/// Bounded, thread-scoped evidence collected from nested `node_repl` calls.
|
||||
#[derive(Debug, Default)]
|
||||
pub(crate) struct NodeReplReviewEvidence(Mutex<NodeReplReviewEvidenceState>);
|
||||
pub struct NodeReplReviewEvidence(Mutex<NodeReplReviewEvidenceState>);
|
||||
|
||||
impl NodeReplReviewEvidence {
|
||||
pub(crate) const MAX_RETAINED_BYTES: usize = 8 * 1024 * 1024;
|
||||
|
||||
/// Enables screenshot capture even when synchronous Guardian transcripts are disabled.
|
||||
pub fn enable_image_capture(&self) {
|
||||
self.0
|
||||
.lock()
|
||||
.unwrap_or_else(PoisonError::into_inner)
|
||||
.image_capture_enabled = true;
|
||||
}
|
||||
|
||||
pub(crate) fn image_capture_enabled(&self) -> bool {
|
||||
self.0
|
||||
.lock()
|
||||
.unwrap_or_else(PoisonError::into_inner)
|
||||
.image_capture_enabled
|
||||
}
|
||||
|
||||
/// Returns the screenshots currently retained for this thread, oldest first.
|
||||
pub fn images(&self) -> Vec<ContentItem> {
|
||||
let state = self.0.lock().unwrap_or_else(PoisonError::into_inner);
|
||||
let mut seen_images = HashSet::new();
|
||||
state
|
||||
.responses
|
||||
.iter()
|
||||
.flat_map(|response| &response.items)
|
||||
.filter_map(|item| match item {
|
||||
Image { image_url, detail } if seen_images.insert(image_url) => {
|
||||
Some(ContentItem::InputImage {
|
||||
image_url: image_url.clone(),
|
||||
detail: *detail,
|
||||
})
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub(crate) fn record(
|
||||
&self,
|
||||
tool_name: &str,
|
||||
|
||||
@@ -250,9 +250,15 @@ impl CoreToolRuntime for McpHandler {
|
||||
return;
|
||||
};
|
||||
let evidence_mode = node_repl_review_evidence_mode(&invocation.turn);
|
||||
let image_capture_enabled = invocation
|
||||
.session
|
||||
.services
|
||||
.thread_extension_data
|
||||
.get::<NodeReplReviewEvidence>()
|
||||
.is_some_and(|evidence| evidence.image_capture_enabled());
|
||||
if self.tool_info.server_name != "node_repl"
|
||||
|| !result.success_for_logging()
|
||||
|| evidence_mode == NodeReplReviewEvidenceMode::Disabled
|
||||
|| evidence_mode == NodeReplReviewEvidenceMode::Disabled && !image_capture_enabled
|
||||
{
|
||||
return;
|
||||
}
|
||||
@@ -283,7 +289,10 @@ impl CoreToolRuntime for McpHandler {
|
||||
text: text.to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}),
|
||||
Some("image") if evidence_mode == NodeReplReviewEvidenceMode::Multimodal => {
|
||||
Some("image")
|
||||
if evidence_mode == NodeReplReviewEvidenceMode::Multimodal
|
||||
|| image_capture_enabled =>
|
||||
{
|
||||
let payload = item.get("data").and_then(Value::as_str)?;
|
||||
let mime_type = item.get("mimeType").and_then(Value::as_str)?;
|
||||
if payload.is_empty()
|
||||
|
||||
@@ -10,6 +10,7 @@ use codex_core::TurnInputRequest;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::config::CurrentTimeReminderConfig;
|
||||
use codex_core::context::NodeReplReviewEvidence;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_extension_api::ToolCallOutcome;
|
||||
@@ -33,6 +34,8 @@ use codex_protocol::dynamic_tools::DynamicToolNamespaceSpec;
|
||||
use codex_protocol::dynamic_tools::DynamicToolNamespaceTool;
|
||||
use codex_protocol::dynamic_tools::DynamicToolResponse;
|
||||
use codex_protocol::dynamic_tools::DynamicToolSpec;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::ImageDetail;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::openai_models::ToolMode;
|
||||
@@ -4344,6 +4347,77 @@ contentLength=0"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn code_mode_node_repl_screenshots_can_be_captured_without_guardian_transcript_flags()
|
||||
-> Result<()> {
|
||||
skip_if_wine_exec!(
|
||||
Ok(()),
|
||||
"requires a Windows test_stdio_server in the Wine-exec environment"
|
||||
);
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
const SCREENSHOT: &str = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==";
|
||||
let server = responses::start_mock_server().await;
|
||||
let mcp_server_bin = remote_aware_stdio_server_bin()?;
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodeMode)
|
||||
.expect("enable Code Mode");
|
||||
let mcp = serde_json::from_value(serde_json::json!({
|
||||
"command": mcp_server_bin,
|
||||
"environment_id": remote_aware_environment_id(),
|
||||
"env": { "MCP_TEST_ENABLE_NODE_REPL_JS": "1" },
|
||||
"omit_tools_from": ["deferred"],
|
||||
}))
|
||||
.expect("valid node_repl MCP server config");
|
||||
config
|
||||
.mcp_servers
|
||||
.set(HashMap::from([("node_repl".to_owned(), mcp)]))
|
||||
.expect("configure node_repl MCP server");
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
core_test_support::wait_for_mcp_server(&test.codex, "node_repl").await?;
|
||||
let evidence = test
|
||||
.codex
|
||||
.thread_extension_data()
|
||||
.get_or_init(NodeReplReviewEvidence::default);
|
||||
evidence.enable_image_capture();
|
||||
|
||||
let response_mock = responses::mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_custom_tool_call(
|
||||
"node-repl-image",
|
||||
"exec",
|
||||
r#"for (let index = 0; index < 2; index++) await tools.mcp__node_repl__js({ code: 'await nodeRepl.emitImage(await tab.screenshot())' });"#,
|
||||
),
|
||||
ev_completed("response-node-repl"),
|
||||
]),
|
||||
sse(vec![ev_completed("response-done")]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
test.submit_text_turn("capture a computer-use screenshot")
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
evidence.images(),
|
||||
vec![ContentItem::InputImage {
|
||||
image_url: SCREENSHOT.to_owned(),
|
||||
detail: Some(ImageDetail::Low),
|
||||
}]
|
||||
);
|
||||
let parent_request = response_mock
|
||||
.requests()
|
||||
.pop()
|
||||
.expect("parent turn should complete");
|
||||
assert!(!serde_json::to_string(&parent_request.input())?.contains(SCREENSHOT));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg_attr(windows, ignore = "no exec_command on Windows")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[test_case(false, false, false, None; "disabled")]
|
||||
|
||||
@@ -7,6 +7,7 @@ use std::time::SystemTime;
|
||||
|
||||
use codex_core::ThreadManager;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::context::NodeReplReviewEvidence;
|
||||
use codex_extension_api::ApprovalReviewContributor;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionEventSink;
|
||||
@@ -266,6 +267,12 @@ impl ThreadLifecycleContributor<Config> for GuardianV2Extension {
|
||||
|
||||
match sampler {
|
||||
Ok(sampler) => {
|
||||
if guardian_config.transcript.include_images {
|
||||
input
|
||||
.thread_store
|
||||
.get_or_init(NodeReplReviewEvidence::default)
|
||||
.enable_image_capture();
|
||||
}
|
||||
input.thread_store.insert(sampler);
|
||||
input.thread_store.insert(guardian_config);
|
||||
input
|
||||
@@ -424,6 +431,15 @@ impl ToolLifecycleContributor for GuardianV2Extension {
|
||||
.as_ref()
|
||||
.and_then(|model| model.auto_review_model_override.clone());
|
||||
let conversation_history = Arc::clone(&input.conversation_history);
|
||||
let node_repl_images = if guardian_config.transcript.include_images {
|
||||
input
|
||||
.thread_store
|
||||
.get::<NodeReplReviewEvidence>()
|
||||
.map(|evidence| evidence.images())
|
||||
.unwrap_or_default()
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
|
||||
tokio::spawn(async move {
|
||||
let transcript = guardian_config
|
||||
@@ -431,7 +447,7 @@ impl ToolLifecycleContributor for GuardianV2Extension {
|
||||
.build(conversation_history.items());
|
||||
let images = guardian_config
|
||||
.transcript
|
||||
.images(conversation_history.items());
|
||||
.images(conversation_history.items(), node_repl_images);
|
||||
drop(conversation_history);
|
||||
let planned_action = match action.render(guardian_config.max_action_tokens) {
|
||||
Ok(planned_action) => planned_action,
|
||||
|
||||
@@ -63,6 +63,7 @@ impl TranscriptConfig {
|
||||
pub(crate) fn images<'a>(
|
||||
&self,
|
||||
items: impl IntoIterator<Item = &'a ResponseItem>,
|
||||
node_repl_images: impl IntoIterator<Item = ContentItem>,
|
||||
) -> Vec<ContentItem> {
|
||||
if !self.include_images {
|
||||
return Vec::new();
|
||||
@@ -117,6 +118,13 @@ impl TranscriptConfig {
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
if self.sources.contains(&TranscriptSource::ToolOutputs) {
|
||||
for image in node_repl_images {
|
||||
if let ContentItem::InputImage { image_url, detail } = image {
|
||||
include_image(&image_url, detail);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
images.into_iter().collect()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user