mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Preserve Guardian user answers across compaction (#41852)
## Why Compaction removes the original `request_user_input` tool call from model-visible history, causing its trusted user answer to be omitted from subsequent Guardian reviews. ## What changed Match recorded user answers against retained review history so they remain available through compaction and review-history eviction. Explicit rollback still removes the associated answer. ## Testing Extend the Guardian history integration test to verify trusted user answers survive compaction and eviction but not rollback. GitOrigin-RevId: 00681ee8df51cf36421b6fdc5446565dff3776fe
This commit is contained in:
1 parent
1c1e17782a
commit
305eed102d
2 files changed
+66
-4
No files matched your search
@@ -95,14 +95,14 @@ impl GuardianReviewEvidence {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Returns bounded answers whose original, host-observed tool calls remain in history.
|
/// Returns bounded answers whose original, host-observed tool calls remain in review history.
|
||||||
pub fn user_input_fragments(&self, history: &dyn ConversationHistorySnapshot) -> Vec<String> {
|
pub fn user_input_fragments(&self, history: &dyn ConversationHistorySnapshot) -> Vec<String> {
|
||||||
let state = self.0.lock().unwrap_or_else(PoisonError::into_inner);
|
let state = self.0.lock().unwrap_or_else(PoisonError::into_inner);
|
||||||
state
|
state
|
||||||
.user_inputs
|
.user_inputs
|
||||||
.iter()
|
.iter()
|
||||||
.filter(|(recorded_call_id, _)| {
|
.filter(|(recorded_call_id, _)| {
|
||||||
history.items().any(|item| {
|
history.review_items().any(|item| {
|
||||||
matches!(
|
matches!(
|
||||||
item,
|
item,
|
||||||
ResponseItem::FunctionCall { call_id, .. }
|
ResponseItem::FunctionCall { call_id, .. }
|
||||||
|
|||||||
@@ -1,12 +1,16 @@
|
|||||||
//! Exercises retained review history through compaction, eviction, and rollback.
|
//! Exercises retained review history through compaction, eviction, and rollback.
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
|
use codex_core::TurnInputRequest;
|
||||||
use codex_core::config::Constrained;
|
use codex_core::config::Constrained;
|
||||||
use codex_features::Feature;
|
use codex_features::Feature;
|
||||||
use codex_protocol::config_types::ApprovalsReviewer;
|
use codex_protocol::config_types::ApprovalsReviewer;
|
||||||
use codex_protocol::protocol::AskForApproval;
|
use codex_protocol::protocol::AskForApproval;
|
||||||
use codex_protocol::protocol::EventMsg;
|
use codex_protocol::protocol::EventMsg;
|
||||||
use codex_protocol::protocol::Op;
|
use codex_protocol::protocol::Op;
|
||||||
|
use codex_protocol::request_user_input::RequestUserInputAnswer;
|
||||||
|
use codex_protocol::request_user_input::RequestUserInputResponse;
|
||||||
|
use codex_protocol::user_input::UserInput;
|
||||||
use core_test_support::responses::ev_assistant_message;
|
use core_test_support::responses::ev_assistant_message;
|
||||||
use core_test_support::responses::ev_completed;
|
use core_test_support::responses::ev_completed;
|
||||||
use core_test_support::responses::ev_function_call;
|
use core_test_support::responses::ev_function_call;
|
||||||
@@ -17,7 +21,10 @@ use core_test_support::skip_if_no_network;
|
|||||||
use core_test_support::skip_if_wine_exec;
|
use core_test_support::skip_if_wine_exec;
|
||||||
use core_test_support::test_codex::test_codex;
|
use core_test_support::test_codex::test_codex;
|
||||||
use core_test_support::wait_for_event;
|
use core_test_support::wait_for_event;
|
||||||
|
use core_test_support::wait_for_event_match;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
|
use serde_json::json;
|
||||||
|
use std::collections::HashMap;
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() -> Result<()> {
|
async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() -> Result<()> {
|
||||||
@@ -30,6 +37,10 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() ->
|
|||||||
let test = test_codex()
|
let test = test_codex()
|
||||||
.with_config(|config| {
|
.with_config(|config| {
|
||||||
config.features.enable(Feature::TokenBudget).unwrap();
|
config.features.enable(Feature::TokenBudget).unwrap();
|
||||||
|
config
|
||||||
|
.features
|
||||||
|
.enable(Feature::DefaultModeRequestUserInput)
|
||||||
|
.unwrap();
|
||||||
config.update_plan_enabled = true;
|
config.update_plan_enabled = true;
|
||||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||||
@@ -48,6 +59,20 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() ->
|
|||||||
sse(inspection),
|
sse(inspection),
|
||||||
sse(vec![
|
sse(vec![
|
||||||
ev_function_call("inspect-latest", "update_plan", plan),
|
ev_function_call("inspect-latest", "update_plan", plan),
|
||||||
|
ev_function_call(
|
||||||
|
"confirm-publish",
|
||||||
|
"request_user_input",
|
||||||
|
&json!({
|
||||||
|
"questions": [{
|
||||||
|
"id": "publish", "header": "Publish", "question": "May I publish?",
|
||||||
|
"options": [
|
||||||
|
{"label": "Yes", "description": "Publish the change."},
|
||||||
|
{"label": "No", "description": "Keep the change local."}
|
||||||
|
]
|
||||||
|
}]
|
||||||
|
})
|
||||||
|
.to_string(),
|
||||||
|
),
|
||||||
ev_completed("latest-inspection"),
|
ev_completed("latest-inspection"),
|
||||||
]),
|
]),
|
||||||
sse(vec![
|
sse(vec![
|
||||||
@@ -57,8 +82,34 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() ->
|
|||||||
],
|
],
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
test.submit_text_turn("Only publish to a private repository.")
|
test.codex
|
||||||
|
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
|
||||||
|
text: "Only publish to a private repository.".into(),
|
||||||
|
text_elements: Vec::new(),
|
||||||
|
}]))
|
||||||
.await?;
|
.await?;
|
||||||
|
let question = wait_for_event_match(&test.codex, |event| match event {
|
||||||
|
EventMsg::RequestUserInput(request) => Some(request.clone()),
|
||||||
|
_ => None,
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
test.codex
|
||||||
|
.submit(Op::UserInputAnswer {
|
||||||
|
id: question.turn_id,
|
||||||
|
response: RequestUserInputResponse {
|
||||||
|
answers: HashMap::from([(
|
||||||
|
"publish".to_owned(),
|
||||||
|
RequestUserInputAnswer {
|
||||||
|
answers: vec!["Do not publish anything.".to_owned()],
|
||||||
|
},
|
||||||
|
)]),
|
||||||
|
},
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
wait_for_event(&test.codex, |event| {
|
||||||
|
matches!(event, EventMsg::TurnComplete(_))
|
||||||
|
})
|
||||||
|
.await;
|
||||||
test.codex.submit(Op::Compact).await?;
|
test.codex.submit(Op::Compact).await?;
|
||||||
wait_for_event(&test.codex, |event| {
|
wait_for_event(&test.codex, |event| {
|
||||||
matches!(event, EventMsg::TurnComplete(_))
|
matches!(event, EventMsg::TurnComplete(_))
|
||||||
@@ -96,6 +147,14 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() ->
|
|||||||
let transcript = serde_json::to_string(&guardian.input())?;
|
let transcript = serde_json::to_string(&guardian.input())?;
|
||||||
assert!(transcript.contains(prompt));
|
assert!(transcript.contains(prompt));
|
||||||
if retained {
|
if retained {
|
||||||
|
let trusted_answers = transcript
|
||||||
|
.split_once(">>> TRUSTED USER ANSWERS START")
|
||||||
|
.expect("trusted answers survive compaction")
|
||||||
|
.1
|
||||||
|
.split_once(">>> TRUSTED USER ANSWERS END")
|
||||||
|
.expect("trusted answers end marker")
|
||||||
|
.0;
|
||||||
|
assert!(trusted_answers.contains("user: Do not publish anything."));
|
||||||
let positions = [
|
let positions = [
|
||||||
"Only publish to a private repository.",
|
"Only publish to a private repository.",
|
||||||
"tool update_plan call",
|
"tool update_plan call",
|
||||||
@@ -114,7 +173,8 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() ->
|
|||||||
requests[0]
|
requests[0]
|
||||||
.input()
|
.input()
|
||||||
.iter()
|
.iter()
|
||||||
.all(|item| item["call_id"] != "inspect-0")
|
.all(|item| item["call_id"] != "inspect-0"
|
||||||
|
&& item["call_id"] != "confirm-publish")
|
||||||
);
|
);
|
||||||
test.codex.ensure_rollout_materialized().await;
|
test.codex.ensure_rollout_materialized().await;
|
||||||
test.codex
|
test.codex
|
||||||
@@ -125,6 +185,8 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() ->
|
|||||||
})
|
})
|
||||||
.await;
|
.await;
|
||||||
} else {
|
} else {
|
||||||
|
assert!(!transcript.contains(">>> TRUSTED USER ANSWERS START"));
|
||||||
|
assert!(!transcript.contains("Do not publish anything."));
|
||||||
assert!(!transcript.contains("Only publish to a private repository."));
|
assert!(!transcript.contains("Only publish to a private repository."));
|
||||||
assert!(!transcript.contains("tool update_plan call"));
|
assert!(!transcript.contains("tool update_plan call"));
|
||||||
assert!(!transcript.contains("tool update_plan result"));
|
assert!(!transcript.contains("tool update_plan result"));
|
||||||
|
|||||||
Reference in new issue
Block a user