mirror of
https://github.com/openai/codex.git
synced 2026-09-20 12:47:38 +00:00
Preserve Guardian's reusable history prefix across approval requests (#46279)
## Why Changing previous review decisions or trusted tool and skill evidence should not invalidate the reusable conversation history prefix. ## What changed Move previous reviews, trusted tool metadata, and trusted skills after the transcript and permission context in Guardian context composition. Keep them before the current action, with history remaining user-role evidence. ## Testing Add a regression test that varies reviews, tools, skills, and actions while asserting an identical history prefix, both with and without retained context. Update the Guardian v2 integration test to verify the separate transcript and action messages. GitOrigin-RevId: 8f5b570b17b6715beac526afff7d52139d13a3ad
This commit is contained in:
@@ -1187,21 +1187,46 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
vec![
|
||||
json!(["additional_tools", "developer", null]),
|
||||
json!(["message", "developer", ["guardian.classifier_instructions"]]),
|
||||
json!(["message", "user", null]),
|
||||
json!(["message", "developer", null]),
|
||||
json!(["message", "developer", ["guardian.trusted_tool"]]),
|
||||
json!(["message", "developer", ["guardian.trusted_skills"]]),
|
||||
json!(["message", "user", null]),
|
||||
],
|
||||
);
|
||||
let content = input[5]["content"].as_array().expect("untrusted evidence");
|
||||
let texts = content
|
||||
let history = input[2]["content"].as_array().expect("untrusted history");
|
||||
let history_texts = history
|
||||
.iter()
|
||||
.filter_map(|item| item["text"].as_str())
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(texts.first().copied(), Some(">>> TRANSCRIPT START\n"));
|
||||
assert!(texts.iter().any(|text| text.contains(USER_CONTEXT)));
|
||||
assert!(texts.iter().any(|text| text.contains("guardian-0")));
|
||||
assert_eq!(texts.last().copied(), Some(">>> APPROVAL REQUEST END\n"));
|
||||
assert_eq!(
|
||||
history_texts.first().copied(),
|
||||
Some(">>> TRANSCRIPT START\n")
|
||||
);
|
||||
assert!(history_texts.iter().any(|text| text.contains(USER_CONTEXT)));
|
||||
assert!(history_texts.iter().any(|text| text.contains("guardian-0")));
|
||||
assert_eq!(
|
||||
history_texts.last().copied(),
|
||||
Some(">>> TRANSCRIPT END\n\n")
|
||||
);
|
||||
assert!(history.iter().all(|item| item["type"] == "input_text"));
|
||||
|
||||
let content = input[6]["content"].as_array().expect("action and images");
|
||||
let action_texts = content
|
||||
.iter()
|
||||
.filter_map(|item| item["text"].as_str())
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
&action_texts[..2],
|
||||
&[
|
||||
"The Codex agent has requested the following action:\n",
|
||||
">>> APPROVAL REQUEST START\n",
|
||||
],
|
||||
);
|
||||
assert_eq!(
|
||||
action_texts.last().copied(),
|
||||
Some(">>> APPROVAL REQUEST END\n")
|
||||
);
|
||||
assert!(
|
||||
content[..content.len() - 1]
|
||||
.iter()
|
||||
|
||||
@@ -3,6 +3,8 @@
|
||||
//! framing, message boundaries and section placement, without retaining history.
|
||||
//! Each content item keeps its selection policy until transport conversion.
|
||||
//! Long text splits losslessly only after admission, preserving whole-entry selection.
|
||||
//! Action-specific attestations follow the transcript so they do not invalidate
|
||||
//! the reusable history prefix when previous decisions or tool evidence change.
|
||||
|
||||
use codex_context_fragments::ContextualUserFragment;
|
||||
use codex_protocol::models::ContentItem;
|
||||
@@ -119,31 +121,31 @@ impl CollectedContext {
|
||||
for section in self.sections {
|
||||
let (position, id, delivery) = match section {
|
||||
ContextSection::PreviousReviews(reviews) => (
|
||||
1,
|
||||
6,
|
||||
"previous_reviews",
|
||||
SectionDelivery::Message(Box::new(reviews.into_message())),
|
||||
),
|
||||
ContextSection::TrustedTool(tool) => (
|
||||
2,
|
||||
7,
|
||||
"trusted_tool",
|
||||
SectionDelivery::Message(Box::new(ContextualUserFragment::into(tool))),
|
||||
),
|
||||
ContextSection::TrustedSkills(skills) => (
|
||||
3,
|
||||
8,
|
||||
"trusted_skills",
|
||||
SectionDelivery::Message(Box::new(ContextualUserFragment::into(skills))),
|
||||
),
|
||||
ContextSection::RootConversation { items } => {
|
||||
(4, "root_conversation", text_content(items))
|
||||
(1, "root_conversation", text_content(items))
|
||||
}
|
||||
ContextSection::SenderUserMessages { items } => {
|
||||
(4, "sender_user_messages", text_content(items))
|
||||
(1, "sender_user_messages", text_content(items))
|
||||
}
|
||||
ContextSection::RetainedUserInstructions { items } => {
|
||||
(5, "retained_user_instructions", text_content(items))
|
||||
(2, "retained_user_instructions", text_content(items))
|
||||
}
|
||||
ContextSection::TrustedUserAnswers { items } => {
|
||||
(6, "trusted_user_answers", text_content(items))
|
||||
(3, "trusted_user_answers", text_content(items))
|
||||
}
|
||||
ContextSection::ConversationTranscript { .. } => {
|
||||
let transcript =
|
||||
@@ -173,7 +175,7 @@ impl CollectedContext {
|
||||
items.push(Budgeted::required(format!("\n{note}\n")));
|
||||
}
|
||||
(
|
||||
7,
|
||||
4,
|
||||
"conversation_transcript",
|
||||
SectionDelivery::UserContent(
|
||||
items
|
||||
@@ -187,7 +189,7 @@ impl CollectedContext {
|
||||
)
|
||||
}
|
||||
ContextSection::PermissionContext { items } => {
|
||||
(8, "permissions", text_content(items))
|
||||
(5, "permissions", text_content(items))
|
||||
}
|
||||
ContextSection::TranscriptImages(images) => {
|
||||
if images.omitted_bytes > 0 {
|
||||
|
||||
139
codex-rs/guardian-context/tests/cache_prefix.rs
Normal file
139
codex-rs/guardian-context/tests/cache_prefix.rs
Normal file
@@ -0,0 +1,139 @@
|
||||
//! Exercises cache-prefix stability through the public collection and composition API.
|
||||
|
||||
use codex_context_fragments::ContextualUserFragment;
|
||||
use codex_guardian_context::ActionPresentation;
|
||||
use codex_guardian_context::ContextPresentation;
|
||||
use codex_guardian_context::ContextProfile;
|
||||
use codex_guardian_context::ContextTarget;
|
||||
use codex_guardian_context::PlannedAction;
|
||||
use codex_guardian_context::PlannedActionKind;
|
||||
use codex_guardian_context::PreviousReviews;
|
||||
use codex_guardian_context::SectionHistory;
|
||||
use codex_guardian_context::SectionInput;
|
||||
use codex_guardian_context::TrustedSkills;
|
||||
use codex_guardian_context::TrustedTool;
|
||||
use codex_guardian_context::default_registry;
|
||||
use codex_history::RetainedContext;
|
||||
use codex_history::RetainedInputSource;
|
||||
use codex_history::RetainedUserMessage;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
struct History {
|
||||
items: Vec<ResponseItem>,
|
||||
retained: Option<RetainedContext>,
|
||||
}
|
||||
|
||||
impl SectionHistory for History {
|
||||
fn items(&self) -> Box<dyn Iterator<Item = &ResponseItem> + Send + '_> {
|
||||
Box::new(self.items.iter())
|
||||
}
|
||||
|
||||
fn retained_context(&self) -> Option<&RetainedContext> {
|
||||
self.retained.as_ref()
|
||||
}
|
||||
}
|
||||
|
||||
fn user_message(texts: Vec<String>) -> ResponseItem {
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_owned(),
|
||||
content: texts
|
||||
.into_iter()
|
||||
.map(|text| ContentItem::InputText { text })
|
||||
.collect(),
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn changing_attestations_preserves_history_before_the_current_action() {
|
||||
let instruction = "Inspect staging only. Do not publish.";
|
||||
let mut retained = RetainedContext::default();
|
||||
retained.record_user_message(
|
||||
RetainedUserMessage {
|
||||
turn_id: "turn-1".to_owned(),
|
||||
message_id: None,
|
||||
text: instruction.to_owned(),
|
||||
complete: true,
|
||||
},
|
||||
RetainedInputSource::Local(None),
|
||||
);
|
||||
// Legacy review history and Felix's retained, thread-owned history use the
|
||||
// same composer. Neither may put changing attestations before that history.
|
||||
for retained in [None, Some(retained)] {
|
||||
let history = History {
|
||||
items: vec![user_message(vec![instruction.to_owned()])],
|
||||
retained,
|
||||
};
|
||||
let profile = ContextProfile::asynchronous();
|
||||
let mut previous_prefix = None;
|
||||
for generation in 0..2 {
|
||||
let reviews = PreviousReviews::try_from_fragments(vec![format!(
|
||||
"Host-attested decision {generation}: denied."
|
||||
)])
|
||||
.unwrap();
|
||||
let tool = TrustedTool {
|
||||
server: format!("server-{generation}"),
|
||||
connector_id: None,
|
||||
source: "user configuration".to_owned(),
|
||||
};
|
||||
let skills = TrustedSkills {
|
||||
paths: vec![format!("/skills/skill-{generation}/SKILL.md")],
|
||||
};
|
||||
let action = PlannedAction {
|
||||
json: format!(r#"{{"tool":"inspect","path":"file-{generation}"}}"#),
|
||||
tool_descriptions: None,
|
||||
kind: PlannedActionKind::Command,
|
||||
reason: None,
|
||||
};
|
||||
let collected = default_registry()
|
||||
.prepare(&SectionInput {
|
||||
target: ContextTarget::Async,
|
||||
history: &history,
|
||||
transcript: &profile.transcript,
|
||||
root_conversation: &[],
|
||||
trusted_user_answers: &[],
|
||||
planned_action: Some(&action),
|
||||
permissions: None,
|
||||
previous_reviews: Some(&reviews),
|
||||
trusted_tool: Some(&tool),
|
||||
trusted_skill_paths: &skills.paths,
|
||||
images: None,
|
||||
node_repl: None,
|
||||
})
|
||||
.unwrap();
|
||||
let transcript = profile.render_transcript(
|
||||
collected.transcript_entries(),
|
||||
/*entry_number_offset*/ 0,
|
||||
);
|
||||
let messages = collected
|
||||
.compose(ContextPresentation::Async, transcript)
|
||||
.unwrap()
|
||||
.into_messages();
|
||||
let (prefix, suffix) = messages.split_first().expect("history prefix");
|
||||
let ResponseItem::Message { role, content, .. } = prefix else {
|
||||
panic!("history remains untrusted user evidence");
|
||||
};
|
||||
assert_eq!(role, "user");
|
||||
assert!(content.iter().any(|item| matches!(
|
||||
item,
|
||||
ContentItem::InputText { text } if text == ">>> TRANSCRIPT END\n\n"
|
||||
)));
|
||||
if let Some(previous) = previous_prefix.replace(prefix.clone()) {
|
||||
assert_eq!(prefix, &previous);
|
||||
}
|
||||
assert_eq!(
|
||||
suffix,
|
||||
[
|
||||
reviews.into_message(),
|
||||
ContextualUserFragment::into(tool),
|
||||
ContextualUserFragment::into(skills),
|
||||
user_message(action.render(ActionPresentation::Async)),
|
||||
]
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user