mirror of
https://github.com/openai/codex.git
synced 2026-09-13 11:47:17 +00:00
Store guardian transcript boundary on review session
Keep the parent transcript checkpoint on the cached guardian review session so follow-up guardian prompts only inject transcript evidence since the last terminal guardian review. Also preserve that checkpoint across guardian trunk replacement and add tests for approved and denied follow-up reviews. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -67,7 +67,14 @@ pub(crate) async fn build_guardian_prompt_items(
|
||||
request: GuardianApprovalRequest,
|
||||
) -> serde_json::Result<Vec<UserInput>> {
|
||||
let history = session.clone_history().await;
|
||||
let transcript_entries = collect_guardian_transcript_entries(history.raw_items());
|
||||
let history_items = history.raw_items();
|
||||
let start_index = session
|
||||
.guardian_review_session
|
||||
.parent_history_boundary()
|
||||
.await
|
||||
.map(|boundary| boundary.min(history_items.len()))
|
||||
.unwrap_or_default();
|
||||
let transcript_entries = collect_guardian_transcript_entries(&history_items[start_index..]);
|
||||
let planned_action_json = format_guardian_action_pretty(&request)?;
|
||||
|
||||
let (transcript_entries, omission_note) =
|
||||
|
||||
@@ -52,6 +52,14 @@ fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str {
|
||||
}
|
||||
}
|
||||
|
||||
async fn mark_terminal_guardian_review_boundary(session: &Session) {
|
||||
let boundary = session.clone_history().await.raw_items().len();
|
||||
session
|
||||
.guardian_review_session
|
||||
.set_parent_history_boundary(Some(boundary))
|
||||
.await;
|
||||
}
|
||||
|
||||
/// Whether this turn should route `on-request` approval prompts through the
|
||||
/// guardian reviewer instead of surfacing them to the user. ARC may still
|
||||
/// block actions earlier in the flow.
|
||||
@@ -101,6 +109,7 @@ async fn run_guardian_review(
|
||||
.as_ref()
|
||||
.is_some_and(CancellationToken::is_cancelled)
|
||||
{
|
||||
mark_terminal_guardian_review_boundary(session.as_ref()).await;
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
@@ -151,6 +160,7 @@ async fn run_guardian_review(
|
||||
evidence: vec![],
|
||||
},
|
||||
GuardianReviewOutcome::Aborted => {
|
||||
mark_terminal_guardian_review_boundary(session.as_ref()).await;
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
@@ -187,6 +197,7 @@ async fn run_guardian_review(
|
||||
} else {
|
||||
GuardianAssessmentStatus::Denied
|
||||
};
|
||||
mark_terminal_guardian_review_boundary(session.as_ref()).await;
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
@@ -249,14 +260,15 @@ pub(crate) async fn review_approval_request_with_cancel(
|
||||
/// it is pinned to a read-only sandbox with `approval_policy = never` and
|
||||
/// nonessential agent features disabled. When the cached trunk session is idle,
|
||||
/// later approvals append onto that same guardian conversation to preserve a
|
||||
/// stable prompt-cache key. If the trunk is already busy, the review runs in an
|
||||
/// ephemeral fork from the last committed trunk rollout so parallel approvals
|
||||
/// do not block each other or mutate the cached thread. The trunk is recreated
|
||||
/// when the effective review-session config changes, and any future compaction
|
||||
/// must continue to preserve the guardian policy as exact top-level developer
|
||||
/// context. It may still reuse the parent's managed-network allowlist for
|
||||
/// read-only checks, but it intentionally runs without inherited exec-policy
|
||||
/// rules.
|
||||
/// stable prompt-cache key. That cached trunk also carries the parent-history
|
||||
/// checkpoint used to slice future guardian transcript evidence. If the trunk
|
||||
/// is already busy, the review runs in an ephemeral fork from the last
|
||||
/// committed trunk rollout so parallel approvals do not block each other or
|
||||
/// mutate the cached thread. The trunk is recreated when the effective
|
||||
/// review-session config changes, and any future compaction must continue to
|
||||
/// preserve the guardian policy as exact top-level developer context. It may
|
||||
/// still reuse the parent's managed-network allowlist for read-only checks, but
|
||||
/// it intentionally runs without inherited exec-policy rules.
|
||||
pub(super) async fn run_guardian_review_session(
|
||||
session: Arc<Session>,
|
||||
turn: Arc<TurnContext>,
|
||||
|
||||
@@ -89,6 +89,10 @@ struct GuardianReviewSession {
|
||||
has_prior_review: AtomicBool,
|
||||
review_lock: Mutex<()>,
|
||||
last_committed_rollout_items: Mutex<Option<Vec<RolloutItem>>>,
|
||||
// Parent-session history index captured after the latest terminal guardian
|
||||
// review. Future guardian prompts use it to slice parent transcript
|
||||
// evidence without persisting extra rollout metadata.
|
||||
parent_history_boundary: Mutex<Option<usize>>,
|
||||
}
|
||||
|
||||
struct EphemeralReviewCleanup {
|
||||
@@ -155,6 +159,14 @@ impl GuardianReviewSessionReuseKey {
|
||||
}
|
||||
|
||||
impl GuardianReviewSession {
|
||||
async fn parent_history_boundary(&self) -> Option<usize> {
|
||||
*self.parent_history_boundary.lock().await
|
||||
}
|
||||
|
||||
async fn set_parent_history_boundary(&self, boundary: Option<usize>) {
|
||||
*self.parent_history_boundary.lock().await = boundary;
|
||||
}
|
||||
|
||||
async fn shutdown(&self) {
|
||||
self.cancel_token.cancel();
|
||||
let _ = self.codex.shutdown_and_wait().await;
|
||||
@@ -228,6 +240,21 @@ impl Drop for EphemeralReviewCleanup {
|
||||
}
|
||||
|
||||
impl GuardianReviewSessionManager {
|
||||
pub(crate) async fn parent_history_boundary(&self) -> Option<usize> {
|
||||
let trunk = self.state.lock().await.trunk.clone();
|
||||
match trunk {
|
||||
Some(trunk) => trunk.parent_history_boundary().await,
|
||||
None => None,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn set_parent_history_boundary(&self, boundary: Option<usize>) {
|
||||
let trunk = self.state.lock().await.trunk.clone();
|
||||
if let Some(trunk) = trunk {
|
||||
trunk.set_parent_history_boundary(boundary).await;
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn shutdown(&self) {
|
||||
let (review_session, ephemeral_reviews) = {
|
||||
let mut state = self.state.lock().await;
|
||||
@@ -251,6 +278,7 @@ impl GuardianReviewSessionManager {
|
||||
let deadline = tokio::time::Instant::now() + GUARDIAN_REVIEW_TIMEOUT;
|
||||
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(¶ms.spawn_config);
|
||||
let mut stale_trunk_to_shutdown = None;
|
||||
let mut spawned_replacement_trunk = false;
|
||||
let trunk_candidate = match run_before_review_deadline(
|
||||
deadline,
|
||||
params.external_cancel.as_ref(),
|
||||
@@ -288,6 +316,7 @@ impl GuardianReviewSessionManager {
|
||||
}
|
||||
Err(outcome) => return outcome,
|
||||
};
|
||||
spawned_replacement_trunk = true;
|
||||
state.trunk = Some(Arc::clone(&review_session));
|
||||
}
|
||||
|
||||
@@ -296,6 +325,14 @@ impl GuardianReviewSessionManager {
|
||||
Err(outcome) => return outcome,
|
||||
};
|
||||
|
||||
if spawned_replacement_trunk
|
||||
&& let (Some(stale_trunk), Some(trunk)) =
|
||||
(stale_trunk_to_shutdown.as_ref(), trunk_candidate.as_ref())
|
||||
{
|
||||
let boundary = stale_trunk.parent_history_boundary().await;
|
||||
trunk.set_parent_history_boundary(boundary).await;
|
||||
}
|
||||
|
||||
if let Some(review_session) = stale_trunk_to_shutdown {
|
||||
review_session.shutdown_in_background();
|
||||
}
|
||||
@@ -356,6 +393,7 @@ impl GuardianReviewSessionManager {
|
||||
has_prior_review: AtomicBool::new(false),
|
||||
review_lock: Mutex::new(()),
|
||||
last_committed_rollout_items: Mutex::new(None),
|
||||
parent_history_boundary: Mutex::new(None),
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -375,6 +413,7 @@ impl GuardianReviewSessionManager {
|
||||
has_prior_review: AtomicBool::new(false),
|
||||
review_lock: Mutex::new(()),
|
||||
last_committed_rollout_items: Mutex::new(None),
|
||||
parent_history_boundary: Mutex::new(None),
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -483,6 +522,7 @@ async fn spawn_guardian_review_session(
|
||||
has_prior_review: AtomicBool::new(has_prior_review),
|
||||
review_lock: Mutex::new(()),
|
||||
last_committed_rollout_items: Mutex::new(None),
|
||||
parent_history_boundary: Mutex::new(None),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -119,6 +119,16 @@ async fn seed_guardian_parent_history(session: &Arc<Session>, turn: &Arc<TurnCon
|
||||
.await;
|
||||
}
|
||||
|
||||
fn guardian_prompt_text(items: &[codex_protocol::user_input::UserInput]) -> String {
|
||||
items
|
||||
.iter()
|
||||
.map(|item| match item {
|
||||
codex_protocol::user_input::UserInput::Text { text, .. } => text.as_str(),
|
||||
other => panic!("expected text-only guardian prompt item, got {other:?}"),
|
||||
})
|
||||
.collect::<String>()
|
||||
}
|
||||
|
||||
fn guardian_snapshot_options() -> ContextSnapshotOptions {
|
||||
ContextSnapshotOptions::default()
|
||||
.strip_capability_instructions()
|
||||
@@ -431,6 +441,178 @@ async fn routes_approval_to_guardian_requires_auto_only_review_policy() {
|
||||
assert!(routes_approval_to_guardian(&turn));
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn approved_guardian_review_moves_parent_transcript_boundary() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian-approved"),
|
||||
ev_assistant_message(
|
||||
"msg-guardian-approved",
|
||||
"{\"risk_level\":\"low\",\"risk_score\":5,\"rationale\":\"approved\",\"evidence\":[]}",
|
||||
),
|
||||
ev_completed("resp-guardian-approved"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let (session, turn) = guardian_test_session_and_turn(&server).await;
|
||||
seed_guardian_parent_history(&session, &turn).await;
|
||||
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-approved-1".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string()],
|
||||
cwd: PathBuf::from("/repo/codex-rs/core"),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Need to push the first docs fix.".to_string()),
|
||||
},
|
||||
None,
|
||||
)
|
||||
.await;
|
||||
assert_eq!(decision, ReviewDecision::Approved);
|
||||
|
||||
session
|
||||
.record_into_history(
|
||||
&[
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: "Push the release branch too.".to_string(),
|
||||
}],
|
||||
end_turn: None,
|
||||
phase: None,
|
||||
},
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "assistant".to_string(),
|
||||
content: vec![ContentItem::OutputText {
|
||||
text: "I now need approval to push the release branch.".to_string(),
|
||||
}],
|
||||
end_turn: None,
|
||||
phase: None,
|
||||
},
|
||||
],
|
||||
turn.as_ref(),
|
||||
)
|
||||
.await;
|
||||
|
||||
let prompt = build_guardian_prompt_items(
|
||||
session.as_ref(),
|
||||
None,
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-approved-2".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string(), "origin".to_string()],
|
||||
cwd: PathBuf::from("/repo/codex-rs/core"),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Need to push the release branch.".to_string()),
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
let prompt_text = guardian_prompt_text(&prompt);
|
||||
|
||||
assert!(!prompt_text.contains("Please check the repo visibility"));
|
||||
assert!(!prompt_text.contains("The repo is public; I now need approval"));
|
||||
assert!(prompt_text.contains("Push the release branch too."));
|
||||
assert!(prompt_text.contains("I now need approval to push the release branch."));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn denied_guardian_review_moves_parent_transcript_boundary() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian-denied"),
|
||||
ev_assistant_message(
|
||||
"msg-guardian-denied",
|
||||
"{\"risk_level\":\"high\",\"risk_score\":95,\"rationale\":\"denied\",\"evidence\":[]}",
|
||||
),
|
||||
ev_completed("resp-guardian-denied"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let (session, turn) = guardian_test_session_and_turn(&server).await;
|
||||
seed_guardian_parent_history(&session, &turn).await;
|
||||
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-denied-1".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string(), "--force".to_string()],
|
||||
cwd: PathBuf::from("/repo/codex-rs/core"),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Need to force push the first docs fix.".to_string()),
|
||||
},
|
||||
None,
|
||||
)
|
||||
.await;
|
||||
assert_eq!(decision, ReviewDecision::Denied);
|
||||
|
||||
session
|
||||
.record_into_history(
|
||||
&[
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: "Push the hotfix branch instead.".to_string(),
|
||||
}],
|
||||
end_turn: None,
|
||||
phase: None,
|
||||
},
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "assistant".to_string(),
|
||||
content: vec![ContentItem::OutputText {
|
||||
text: "I need approval to push the hotfix branch.".to_string(),
|
||||
}],
|
||||
end_turn: None,
|
||||
phase: None,
|
||||
},
|
||||
],
|
||||
turn.as_ref(),
|
||||
)
|
||||
.await;
|
||||
|
||||
let prompt = build_guardian_prompt_items(
|
||||
session.as_ref(),
|
||||
None,
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-denied-2".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string(), "hotfix".to_string()],
|
||||
cwd: PathBuf::from("/repo/codex-rs/core"),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Need to push the hotfix branch.".to_string()),
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
let prompt_text = guardian_prompt_text(&prompt);
|
||||
|
||||
assert!(!prompt_text.contains("Please check the repo visibility"));
|
||||
assert!(!prompt_text.contains("The repo is public; I now need approval"));
|
||||
assert!(prompt_text.contains("Push the hotfix branch instead."));
|
||||
assert!(prompt_text.contains("I need approval to push the hotfix branch."));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_guardian_transcript_reserves_separate_budget_for_tool_evidence() {
|
||||
let repeated = "signal ".repeat(8_000);
|
||||
|
||||
Reference in New Issue
Block a user