From e88626621bad4bb2655fa8b5fc7d2f73d68e168d Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 27 May 2026 00:23:15 -0700 Subject: [PATCH] fix(auto-review) skip legacy notify for auto review threads (#24714) ## Summary Clear inherited legacy `notify` from Guardian review session config, since we should not be passing auto review threads into `notify` targets. Keeps legacy notify payload and hook runtime behavior unchanged for normal user turns. ## Testing - [x] add a Guardian config regression and dedicated Guardian integration test so review sessions cannot inherit parent notify hooks --- codex-rs/core/src/guardian/review_session.rs | 1 + codex-rs/core/src/guardian/tests.rs | 19 +++ codex-rs/core/tests/suite/guardian_review.rs | 169 +++++++++++++++++++ codex-rs/core/tests/suite/mod.rs | 2 + 4 files changed, 191 insertions(+) create mode 100644 codex-rs/core/tests/suite/guardian_review.rs diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index e56d5ce17..67f733e0b 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -905,6 +905,7 @@ pub(crate) fn build_guardian_review_session_config( .map(guardian_policy_prompt_with_config) .unwrap_or_else(guardian_policy_prompt), ); + guardian_config.notify = None; guardian_config.developer_instructions = None; guardian_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); guardian_config diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 0544b2777..00f0d9be3 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -2215,6 +2215,25 @@ async fn guardian_review_session_config_clears_parent_developer_instructions() { ); } +#[tokio::test] +async fn guardian_review_session_config_clears_legacy_notify() { + let mut parent_config = test_config().await; + parent_config.notify = Some(vec![ + "/path/to/notify".to_string(), + "turn-ended".to_string(), + ]); + + let guardian_config = build_guardian_review_session_config_for_test( + &parent_config, + /*live_network_config*/ None, + "active-model", + /*reasoning_effort*/ None, + ) + .expect("guardian config"); + + assert_eq!(guardian_config.notify, None); +} + #[tokio::test] async fn guardian_review_session_config_uses_live_network_proxy_state() { let mut parent_config = test_config().await; diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs new file mode 100644 index 000000000..5b76d09ae --- /dev/null +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -0,0 +1,169 @@ +#![cfg(not(target_os = "windows"))] + +use anyhow::Result; +use codex_core::config::Constrained; +use codex_core::sandboxing::SandboxPermissions; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::SandboxPolicy; +use codex_protocol::user_input::UserInput; +use core_test_support::fs_wait; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::skip_if_sandbox; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::time::Duration; +use tempfile::TempDir; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_review_session_does_not_inherit_legacy_notify() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let notify_dir = TempDir::new()?; + let notify_script = notify_dir.path().join("notify.sh"); + fs::write( + ¬ify_script, + r#"#!/bin/bash +set -e +payload_path="$(dirname "${0}")/notify.jsonl" +printf '%s\n' "${@: -1}" >> "${payload_path}""#, + )?; + fs::set_permissions(¬ify_script, fs::Permissions::from_mode(0o755))?; + let notify_file = notify_dir.path().join("notify.jsonl"); + let notify_script_str = notify_script.to_str().unwrap().to_string(); + let sandbox_policy_for_config = sandbox_policy.clone(); + + let mut builder = test_codex().with_config(move |config| { + config.notify = Some(vec![notify_script_str]); + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config + .set_legacy_sandbox_policy(sandbox_policy_for_config) + .expect("set sandbox policy"); + }); + let test = builder.build(&server).await?; + + let output_file = test.cwd.path().join("guardian-review-notify.txt"); + let command = format!("printf guardian-approved > {}", output_file.display()); + let tool_args = json!({ + "cmd": command, + "yield_time_ms": 1_000_u64, + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "Exercise Guardian approval routing.", + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-parent-tool"), + ev_function_call( + "exec-call", + "exec_command", + &serde_json::to_string(&tool_args)?, + ), + ev_completed("resp-parent-tool"), + ]), + sse(vec![ + ev_response_created("resp-guardian-review"), + ev_assistant_message( + "msg-guardian-review", + &json!({ + "risk_level": "low", + "user_authorization": "high", + "outcome": "allow", + "rationale": "The command writes a marker file in the workspace.", + }) + .to_string(), + ), + ev_completed("resp-guardian-review"), + ]), + sse(vec![ + ev_response_created("resp-parent-done"), + ev_assistant_message("msg-parent-done", "done"), + ev_completed("resp-parent-done"), + ]), + ], + ) + .await; + + test.codex + .submit(Op::UserInput { + environments: None, + items: vec![UserInput::Text { + text: "run a command that requires Guardian review".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + cwd: Some(test.cwd.path().to_path_buf()), + approval_policy: Some(approval_policy), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + sandbox_policy: Some(sandbox_policy), + ..Default::default() + }, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let guardian_request = responses + .requests() + .into_iter() + .find(|request| request.body_contains_text("Exercise Guardian approval routing.")) + .expect("expected Guardian review request"); + assert!(guardian_request.body_contains_text(&command)); + + fs_wait::wait_for_path_exists(¬ify_file, Duration::from_secs(5)).await?; + tokio::time::sleep(Duration::from_millis(100)).await; + let notify_payload_raw = tokio::fs::read_to_string(¬ify_file).await?; + let payloads: Vec = notify_payload_raw + .lines() + .map(serde_json::from_str::) + .collect::>()?; + + assert_eq!( + payloads.len(), + 1, + "unexpected notify payloads: {payloads:?}" + ); + assert_eq!( + payloads[0]["input-messages"], + json!(["run a command that requires Guardian review"]) + ); + assert_eq!(payloads[0]["last-assistant-message"], json!("done")); + assert!( + !notify_payload_raw.contains( + "The following is the Codex agent history whose request action you are assessing." + ), + "Guardian review transcript leaked into legacy notify payload: {notify_payload_raw}" + ); + assert_eq!(fs::read_to_string(&output_file)?, "guardian-approved"); + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index f47de672b..f4ab6c1d6 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -50,6 +50,8 @@ mod deprecation_notice; mod exec; mod exec_policy; mod fork_thread; +#[cfg(not(target_os = "windows"))] +mod guardian_review; mod hierarchical_agents; #[cfg(not(target_os = "windows"))] mod hooks;