mirror of
https://github.com/openai/codex.git
synced 2026-11-09 00:39:32 +00:00
Move Guardian image selection into shared context sections (#43601)
## What changed Move bounded transcript image collection into `codex-guardian-context` and expose it through an optional `TranscriptImages` section. Update the async scorer to consume this section, preserving source filtering, image detail, omission accounting, and the existing limits of four images and 8 MiB of image URL data. ## Testing Add coverage for source order, tool-output filtering, disabled image collection, oldest-image eviction, oversized images, and omitted-byte accounting. GitOrigin-RevId: 097799ea1be6d73b347088f959de1fd23f02ec2c
This commit is contained in:
1 parent
f5331dc237
commit
b4373e53ab
10 files changed
+316
-99
No files matched your search
@@ -0,0 +1,141 @@
|
||||
//! Bounded image selection shared by Guardian consumers.
|
||||
//! Keeps source order and evicts oldest images using the existing count/byte caps.
|
||||
//! Consumers still choose source visibility and model-specific image detail.
|
||||
|
||||
use crate::ContextSection;
|
||||
use crate::SectionContributor;
|
||||
use crate::SectionError;
|
||||
use crate::SectionInput;
|
||||
use crate::SectionScope;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
use codex_protocol::models::ImageDetail;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use std::collections::VecDeque;
|
||||
|
||||
const MAX_TRANSCRIPT_IMAGES: usize = 4;
|
||||
const MAX_TRANSCRIPT_IMAGE_BYTES: usize = 8 * 1024 * 1024;
|
||||
|
||||
/// Request-local image policy and a frozen view of captured REPL screenshots.
|
||||
#[derive(Clone, Copy)]
|
||||
pub struct TranscriptImageInput<'a> {
|
||||
pub enabled: bool,
|
||||
pub include_tool_outputs: bool,
|
||||
pub node_repl_images: &'a [ContentItem],
|
||||
}
|
||||
|
||||
/// Selected images and byte omissions for the consumer's existing telemetry.
|
||||
#[derive(Clone, Default, PartialEq)]
|
||||
pub struct TranscriptImages {
|
||||
pub images: Vec<ContentItem>,
|
||||
pub omitted_bytes: usize,
|
||||
}
|
||||
|
||||
impl std::fmt::Debug for TranscriptImages {
|
||||
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
formatter
|
||||
.debug_struct("TranscriptImages")
|
||||
.field("count", &self.images.len())
|
||||
.field("omitted_bytes", &self.omitted_bytes)
|
||||
.finish_non_exhaustive()
|
||||
}
|
||||
}
|
||||
|
||||
impl TranscriptImages {
|
||||
/// Selects images without changing their detail or allocating a second history.
|
||||
pub fn collect<'a>(
|
||||
items: impl IntoIterator<Item = &'a ResponseItem>,
|
||||
input: TranscriptImageInput<'_>,
|
||||
) -> Self {
|
||||
if !input.enabled {
|
||||
return TranscriptImages {
|
||||
images: Vec::new(),
|
||||
omitted_bytes: 0,
|
||||
};
|
||||
}
|
||||
|
||||
let mut images = VecDeque::new();
|
||||
let mut image_bytes = 0usize;
|
||||
let mut omitted_bytes = 0usize;
|
||||
let mut include_image = |image_url: &str, detail: Option<ImageDetail>| {
|
||||
if image_url.len() > MAX_TRANSCRIPT_IMAGE_BYTES {
|
||||
omitted_bytes = omitted_bytes.saturating_add(image_url.len());
|
||||
return;
|
||||
}
|
||||
while images.len() >= MAX_TRANSCRIPT_IMAGES
|
||||
|| image_bytes + image_url.len() > MAX_TRANSCRIPT_IMAGE_BYTES
|
||||
{
|
||||
let Some(ContentItem::InputImage { image_url, .. }) = images.pop_front() else {
|
||||
break;
|
||||
};
|
||||
image_bytes -= image_url.len();
|
||||
omitted_bytes = omitted_bytes.saturating_add(image_url.len());
|
||||
}
|
||||
image_bytes += image_url.len();
|
||||
images.push_back(ContentItem::InputImage {
|
||||
image_url: image_url.to_owned(),
|
||||
detail,
|
||||
});
|
||||
};
|
||||
|
||||
for item in items {
|
||||
match item {
|
||||
ResponseItem::Message { role, content, .. }
|
||||
if matches!(role.as_str(), "user" | "assistant") =>
|
||||
{
|
||||
for item in content {
|
||||
if let ContentItem::InputImage { image_url, detail } = item {
|
||||
include_image(image_url, *detail);
|
||||
}
|
||||
}
|
||||
}
|
||||
ResponseItem::FunctionCallOutput { output, .. }
|
||||
| ResponseItem::CustomToolCallOutput { output, .. }
|
||||
if input.include_tool_outputs =>
|
||||
{
|
||||
if let Some(content) = output.content_items() {
|
||||
for item in content {
|
||||
if let FunctionCallOutputContentItem::InputImage { image_url, detail } =
|
||||
item
|
||||
{
|
||||
include_image(image_url, *detail);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
if input.include_tool_outputs {
|
||||
for image in input.node_repl_images {
|
||||
if let ContentItem::InputImage { image_url, detail } = image {
|
||||
include_image(image_url, *detail);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
TranscriptImages {
|
||||
images: images.into_iter().collect(),
|
||||
omitted_bytes,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) struct TranscriptImagesSection;
|
||||
impl SectionContributor for TranscriptImagesSection {
|
||||
fn scope(&self) -> SectionScope {
|
||||
SectionScope::Shared
|
||||
}
|
||||
fn contribute(&self, input: &SectionInput<'_>) -> Result<Option<ContextSection>, SectionError> {
|
||||
Ok(input.images.map(|images| {
|
||||
ContextSection::TranscriptImages(TranscriptImages::collect(
|
||||
input.history.items(),
|
||||
images,
|
||||
))
|
||||
}))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "images_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,95 @@
|
||||
use super::MAX_TRANSCRIPT_IMAGE_BYTES;
|
||||
use super::TranscriptImageInput;
|
||||
use super::TranscriptImages;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ImageDetail;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn image(url: &str) -> ContentItem {
|
||||
ContentItem::InputImage {
|
||||
image_url: url.into(),
|
||||
detail: Some(ImageDetail::High),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn image_selection_preserves_source_policy_order_and_both_limits() {
|
||||
let history = [
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".into(),
|
||||
content: ["first", "second", "third", "fourth"].map(image).to_vec(),
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
},
|
||||
ResponseItem::FunctionCallOutput {
|
||||
id: None,
|
||||
call_id: Some("screenshot".into()),
|
||||
name: None,
|
||||
namespace: None,
|
||||
output: FunctionCallOutputPayload::from_content_items(vec![
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "tool".into(),
|
||||
detail: Some(ImageDetail::High),
|
||||
},
|
||||
]),
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
},
|
||||
];
|
||||
let repl = [image("repl")];
|
||||
let input = TranscriptImageInput {
|
||||
enabled: true,
|
||||
include_tool_outputs: true,
|
||||
node_repl_images: &repl,
|
||||
};
|
||||
assert_eq!(
|
||||
TranscriptImages::collect(&history, input),
|
||||
TranscriptImages {
|
||||
images: ["third", "fourth", "tool", "repl"].map(image).to_vec(),
|
||||
omitted_bytes: "firstsecond".len(),
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
TranscriptImages::collect(
|
||||
&history,
|
||||
TranscriptImageInput {
|
||||
include_tool_outputs: false,
|
||||
..input
|
||||
}
|
||||
),
|
||||
TranscriptImages {
|
||||
images: ["first", "second", "third", "fourth"].map(image).to_vec(),
|
||||
omitted_bytes: 0,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
TranscriptImages::collect(
|
||||
&history,
|
||||
TranscriptImageInput {
|
||||
enabled: false,
|
||||
..input
|
||||
}
|
||||
),
|
||||
TranscriptImages::default()
|
||||
);
|
||||
|
||||
let oversized = "x".repeat(MAX_TRANSCRIPT_IMAGE_BYTES + 1);
|
||||
let fits_alone = "y".repeat(MAX_TRANSCRIPT_IMAGE_BYTES);
|
||||
let repl = [image(&oversized), image(&fits_alone), image("recent")];
|
||||
assert_eq!(
|
||||
TranscriptImages::collect(
|
||||
&[],
|
||||
TranscriptImageInput {
|
||||
node_repl_images: &repl,
|
||||
..input
|
||||
}
|
||||
),
|
||||
TranscriptImages {
|
||||
images: vec![image("recent")],
|
||||
omitted_bytes: oversized.len() + fits_alone.len(),
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -50,7 +50,10 @@ mod action;
|
||||
mod authorization;
|
||||
mod entry;
|
||||
mod history;
|
||||
mod images;
|
||||
mod permissions;
|
||||
pub use images::TranscriptImageInput;
|
||||
pub use images::TranscriptImages;
|
||||
mod trusted_skills;
|
||||
mod trusted_tool;
|
||||
pub use trusted_skills::TrustedSkills;
|
||||
@@ -122,6 +125,8 @@ pub struct SectionInput<'a> {
|
||||
pub trusted_tool: Option<&'a TrustedTool>,
|
||||
/// Current-turn and delegated skill paths verified and bounded by the host.
|
||||
pub trusted_skill_paths: &'a [String],
|
||||
/// Optional consumer image policy; no history images are added implicitly.
|
||||
pub images: Option<TranscriptImageInput<'a>>,
|
||||
}
|
||||
|
||||
/// Supplies repeatable, zero-copy access to a host-owned conversation snapshot.
|
||||
@@ -216,6 +221,7 @@ pub fn default_registry() -> &'static SectionRegistry {
|
||||
registry.register(RetainedUserInstructionsSection);
|
||||
registry.register(TrustedUserAnswersSection);
|
||||
registry.register(ConversationTranscriptSection);
|
||||
registry.register(images::TranscriptImagesSection);
|
||||
registry.register(permissions::PermissionContextSection);
|
||||
registry.register(action::PlannedActionSection);
|
||||
registry
|
||||
|
||||
@@ -93,6 +93,7 @@ fn registry_collects_target_specific_sections_in_registration_order() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
});
|
||||
let async_sections = registry.collect(&SectionInput {
|
||||
target: ContextTarget::Async,
|
||||
@@ -105,6 +106,7 @@ fn registry_collects_target_specific_sections_in_registration_order() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
@@ -168,6 +170,7 @@ fn registry_skips_optional_sections_and_stops_on_missing_required_evidence() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
}),
|
||||
Err(error.clone())
|
||||
);
|
||||
@@ -232,6 +235,7 @@ fn reused_registry_preserves_section_identity_and_source_roles() {
|
||||
previous_reviews: Some(&reviews),
|
||||
trusted_tool: Some(&tool),
|
||||
trusted_skill_paths: &["debug-secret/SKILL.md".into()],
|
||||
images: None,
|
||||
})
|
||||
.unwrap();
|
||||
assert!(!format!("{context:?}").contains("debug-secret"));
|
||||
@@ -288,6 +292,7 @@ fn reused_registry_preserves_section_identity_and_source_roles() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
})
|
||||
.unwrap(),
|
||||
vec![ContextSection::ConversationTranscript { items: Vec::new() }]
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
use crate::ConversationTranscriptEntry;
|
||||
use crate::PlannedAction;
|
||||
use crate::PreviousReviews;
|
||||
use crate::TranscriptImages;
|
||||
use crate::TrustedSkills;
|
||||
use crate::TrustedTool;
|
||||
|
||||
@@ -23,5 +24,6 @@ pub enum ContextSection<T = ConversationTranscriptEntry> {
|
||||
PreviousReviews(PreviousReviews),
|
||||
TrustedTool(TrustedTool),
|
||||
TrustedSkills(TrustedSkills),
|
||||
TranscriptImages(TranscriptImages),
|
||||
PermissionContext { items: Vec<String> },
|
||||
}
|
||||
@@ -103,6 +103,7 @@ fn registered_transcript_preserves_shared_roles_and_node_repl_tool_attribution()
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
})
|
||||
.expect("transcript collection should succeed");
|
||||
|
||||
@@ -180,6 +181,7 @@ fn excluded_tool_calls_still_attribute_included_results() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
})
|
||||
.expect("transcript collection should succeed");
|
||||
|
||||
@@ -276,6 +278,7 @@ fn outputs_with_call_ids_or_explicit_names_are_retained() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
})
|
||||
.expect("transcript collection should succeed");
|
||||
let mut expected = vec![
|
||||
@@ -326,6 +329,7 @@ fn reused_registry_applies_current_history_sources_and_entry_limits() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
})
|
||||
.expect("transcript collection should succeed");
|
||||
assert_eq!(
|
||||
@@ -363,6 +367,7 @@ fn reused_registry_applies_current_history_sources_and_entry_limits() {
|
||||
previous_reviews: None,
|
||||
trusted_tool: None,
|
||||
trusted_skill_paths: &[],
|
||||
images: None,
|
||||
})
|
||||
.expect("transcript collection should succeed");
|
||||
let mut expected = vec![ConversationTranscriptEntry {
|
||||
|
||||
Reference in new issue
Block a user