mirror of
https://github.com/openai/codex.git
synced 2026-11-14 14:49:31 +00:00
Move Guardian reviewer lifecycle into the extension (#45537)
## Why Guardian reviews must stop when their parent shuts down or their history is reset. Shutdown must also finish reviewer cleanup before closing the parent's persistent history, including when a review is waiting to retry after a rate limit. ## What changed - Let the Guardian extension own reviewer startup, prewarming, and shutdown through `ThreadManager`, tracking and joining outstanding work during teardown. - Cancel reviews on history reset or parent shutdown and reject decisions returned after cancellation, including cached extension decisions. - Keep background prewarm previews from overwriting the active turn's model metadata. ## Testing Extend regression coverage to verify reviewer cleanup after parent shutdown, prompt shutdown during a 60-second Guardian retry with the network request denied, and preservation of active model review requirements during prewarming. GitOrigin-RevId: 05efc369b9a1642a10365eac8a09da77f3f6e28c
This commit is contained in:
1 parent
12b0164a48
commit
18d7ace221
31 files changed
+495
-308
No files matched your search
@@ -1052,6 +1052,19 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review(
|
||||
.await
|
||||
.expect("guardian trunk rollout path");
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
// Parent stop joins ThreadManager cleanup through the real extension registration.
|
||||
assert!(
|
||||
test.codex
|
||||
.thread_extension_data()
|
||||
.get::<codex_guardian_reviewer::ReviewerTasks>()
|
||||
.expect("Guardian tasks")
|
||||
.tasks
|
||||
.is_empty()
|
||||
);
|
||||
assert!(matches!(
|
||||
test.thread_store.flush_thread(guardian_thread_id).await,
|
||||
Err(codex_thread_store::ThreadStoreError::ThreadNotFound { .. })
|
||||
));
|
||||
let guardian_rollout = fs::read_to_string(guardian_rollout_path)?
|
||||
.lines()
|
||||
.map(codex_rollout::parse_rollout_line)
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_exec_server::LOCAL_ENVIRONMENT_ID;
|
||||
use codex_exec_server::REMOTE_ENVIRONMENT_ID;
|
||||
use codex_exec_server::RemoveOptions;
|
||||
use codex_features::Feature;
|
||||
use codex_history::RolloutItem;
|
||||
use codex_network_proxy::NetworkProxyConfig;
|
||||
use codex_protocol::approvals::NetworkApprovalContext;
|
||||
use codex_protocol::approvals::NetworkApprovalProtocol;
|
||||
@@ -45,6 +46,7 @@ use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionProfile;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_thread_store::LoadThreadHistoryParams;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::PathExt;
|
||||
@@ -1582,12 +1584,16 @@ async fn failed_network_policy_amendment_denies_request_and_does_not_approve_hos
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(ApprovalsReviewer::User; "user_approval")]
|
||||
#[test_case(ApprovalsReviewer::AutoReview; "shutdown_during_guardian_retry")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[cfg_attr(
|
||||
not(target_os = "linux"),
|
||||
ignore = "requires the trusted Linux proxy bridge"
|
||||
)]
|
||||
async fn unattributed_network_request_uses_active_turn_environment_fallback() -> Result<()> {
|
||||
async fn unattributed_network_request_uses_active_turn_environment_fallback(
|
||||
approvals_reviewer: ApprovalsReviewer,
|
||||
) -> Result<()> {
|
||||
skip_if_target_windows!(Ok(()), "uses a raw TCP proxy fixture");
|
||||
skip_if_host_windows!(Ok(()));
|
||||
skip_if_no_network!(Ok(()));
|
||||
@@ -1610,7 +1616,7 @@ async fn unattributed_network_request_uses_active_turn_environment_fallback() ->
|
||||
&test,
|
||||
"hold the active turn",
|
||||
vec![local(test.config.cwd.clone())],
|
||||
ApprovalsReviewer::User,
|
||||
approvals_reviewer,
|
||||
AskForApproval::OnRequest,
|
||||
)
|
||||
.await?;
|
||||
@@ -1623,6 +1629,64 @@ async fn unattributed_network_request_uses_active_turn_environment_fallback() ->
|
||||
.context("expected managed network proxy")?
|
||||
.http_addr
|
||||
.clone();
|
||||
if approvals_reviewer == ApprovalsReviewer::AutoReview {
|
||||
mount_sse_once_match(
|
||||
&server,
|
||||
is_guardian_request,
|
||||
core_test_support::responses::sse_failed(
|
||||
"guardian-first-failure",
|
||||
"rate_limit_exceeded",
|
||||
"Rate limit exceeded. Please try again in 0s.",
|
||||
),
|
||||
)
|
||||
.await;
|
||||
let retrying = mount_sse_once_match(
|
||||
&server,
|
||||
is_guardian_request,
|
||||
core_test_support::responses::sse_failed(
|
||||
"guardian-rate-limited",
|
||||
"rate_limit_exceeded",
|
||||
"Rate limit exceeded. Please try again in 60s.",
|
||||
),
|
||||
)
|
||||
.await;
|
||||
let proxy_request = tokio::spawn(raw_http_proxy_request(proxy_addr, NETWORK_TEST_HOST));
|
||||
wait_for_response_request(&retrying).await;
|
||||
let request = retrying.single_request().body_json();
|
||||
let thread_id = request["client_metadata"]["thread_id"]
|
||||
.as_str()
|
||||
.context("Guardian thread id")?;
|
||||
let thread_id = codex_protocol::ThreadId::from_string(thread_id)?;
|
||||
// Internal reviewers are hidden from get_thread. Their persisted terminal event
|
||||
// confirms the reviewer's own retries finished before testing Guardian's retry wait.
|
||||
tokio::time::timeout(Duration::from_secs(5), async {
|
||||
loop {
|
||||
test.thread_store.flush_thread(thread_id).await?;
|
||||
let history = test
|
||||
.thread_store
|
||||
.load_latest_model_context(LoadThreadHistoryParams {
|
||||
thread_id,
|
||||
include_archived: false,
|
||||
})
|
||||
.await?;
|
||||
if history.items.iter().any(|item| {
|
||||
matches!(item, RolloutItem::EventMsg(EventMsg::TurnComplete(event)) if event.error.is_some())
|
||||
}) {
|
||||
return Ok::<(), anyhow::Error>(());
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
}
|
||||
})
|
||||
.await
|
||||
.context("Guardian reviewer did not finish its rate-limited turn")??;
|
||||
tokio::time::timeout(Duration::from_secs(5), test.codex.shutdown_and_wait())
|
||||
.await
|
||||
.context("parent shutdown waited for Guardian's 60-second retry")??;
|
||||
let response = tokio::time::timeout(Duration::from_secs(5), proxy_request).await???;
|
||||
assert!(response.starts_with("HTTP/1.1 403"), "{response}");
|
||||
assert_eq!(retrying.requests().len(), 1);
|
||||
return Ok(());
|
||||
}
|
||||
let proxy_request = tokio::spawn(raw_http_proxy_request(proxy_addr, NETWORK_TEST_HOST));
|
||||
let approval = expect_network_approval(&test, LOCAL_ENVIRONMENT_ID).await?;
|
||||
assert_eq!(approval.command, ["network-access", NETWORK_TEST_TARGET]);
|
||||
|
||||
Reference in new issue
Block a user