mirror of
https://github.com/openai/codex.git
synced 2026-09-08 15:50:34 +00:00
Honor model requirements in Guardian computer-use scoring (#42422)
## Why Computer-use-only Guardian scoring should follow the active model's REPL auto-review requirement, including when the model changes within a live thread. ## What changed - Run computer-use scoring and fast approval decisions only when the active model sets `node_repl_auto_review_required`. - Invalidate prior or in-flight scores when a model switch skips scoring, so switching back to a reviewed model cannot revive a stale decision. ## Testing - Cover model switches for both `node_repl` and `cua_repl` MCP servers. - Verify skipped scoring and stale-score rejection across requirement changes. GitOrigin-RevId: 46aec4d017bea8f135b435bcd769b87369b8ce95
This commit is contained in:
@@ -77,6 +77,9 @@ use super::mcp_tool::start_mcp_server_with_tools;
|
||||
#[path = "guardian_v2_history_tests.rs"]
|
||||
mod history;
|
||||
|
||||
#[path = "guardian_v2_model_tests.rs"]
|
||||
mod model_tests;
|
||||
|
||||
const TIMEOUT: Duration = Duration::from_secs(30);
|
||||
const MODEL: &str = "mock-model";
|
||||
const REQUIRED_MODEL: &str = "protected-model";
|
||||
@@ -591,7 +594,8 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
codex_protocol::mcp::is_node_repl_backed_server(server_name)
|
||||
}
|
||||
};
|
||||
let node_repl_review_required = matches!(requirement, ModelReviewRequirement::Required)
|
||||
let node_repl_review_required = (matches!(requirement, ModelReviewRequirement::Required)
|
||||
|| matches!(scope, GuardianToolScope::ComputerUseOnly { .. }))
|
||||
&& codex_protocol::mcp::is_node_repl_backed_server(server_name);
|
||||
let late_root_restriction = matches!(
|
||||
lifecycle,
|
||||
|
||||
137
codex-rs/app-server/tests/suite/v2/guardian_v2_model_tests.rs
Normal file
137
codex-rs/app-server/tests/suite/v2/guardian_v2_model_tests.rs
Normal file
@@ -0,0 +1,137 @@
|
||||
//! Verifies model-required CUA scoring across model changes in a live thread.
|
||||
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use std::sync::atomic::Ordering;
|
||||
|
||||
use anyhow::Result;
|
||||
use app_test_support::MockResponsesConfig;
|
||||
use app_test_support::TestAppServer;
|
||||
use app_test_support::write_models_cache_with_models;
|
||||
use axum::Router;
|
||||
use axum::routing::get;
|
||||
use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_app_server_protocol::TurnCompletedNotification;
|
||||
use codex_app_server_protocol::TurnStartParams;
|
||||
use codex_app_server_protocol::TurnStartResponse;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
use codex_app_server_protocol::UserInput;
|
||||
use codex_features::Feature;
|
||||
use core_test_support::load_default_config_for_test;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use test_case::test_case;
|
||||
use tokio::net::TcpListener;
|
||||
use tokio::time::timeout;
|
||||
|
||||
use super::MODEL;
|
||||
use super::MockResponsesState;
|
||||
use super::TIMEOUT;
|
||||
use super::USER_CONTEXT;
|
||||
use super::luna_websocket;
|
||||
use super::parent_response;
|
||||
use super::start_mcp_server_with_tools;
|
||||
use super::wait_for_luna_request;
|
||||
|
||||
#[test_case("node_repl"; "browser")]
|
||||
#[test_case("cua_repl"; "computer use")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn computer_use_scoring_follows_model_review_requirement(
|
||||
server_name: &'static str,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
const REVIEWED_MODEL: &str = "reviewed-model";
|
||||
let state = Arc::new(MockResponsesState {
|
||||
mcp_server_name: Some(server_name),
|
||||
mcp_tool_sequence: Some(&["js"]),
|
||||
mcp_messages: Mutex::new(vec!["hello"]),
|
||||
..Default::default()
|
||||
});
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
let responses_url = format!("http://{}", listener.local_addr()?);
|
||||
let router = Router::new()
|
||||
.route("/v1/responses", get(luna_websocket).post(parent_response))
|
||||
.with_state(Arc::clone(&state));
|
||||
let responses_server = tokio::spawn(async move {
|
||||
let _ = axum::serve(listener, router).await;
|
||||
});
|
||||
let (mcp_url, mcp_server) =
|
||||
start_mcp_server_with_tools(&["js"], /*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
MockResponsesConfig::new(&responses_url)
|
||||
.with_model(MODEL)
|
||||
.with_provider_config("supports_websockets = false")
|
||||
.with_approval_policy("on-request")
|
||||
.with_root_config("approvals_reviewer = \"auto_review\"")
|
||||
.with_extra_config(&format!(
|
||||
"[mcp_servers.{server_name}]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = true"
|
||||
))
|
||||
.enable_feature(Feature::GuardianApproval)
|
||||
.write(codex_home.path())?;
|
||||
let config = load_default_config_for_test(&codex_home).await;
|
||||
let ordinary_model = codex_core::test_support::construct_model_info_offline(MODEL, &config);
|
||||
let mut reviewed_model =
|
||||
codex_core::test_support::construct_model_info_offline(REVIEWED_MODEL, &config);
|
||||
reviewed_model.node_repl_auto_review_required = true;
|
||||
write_models_cache_with_models(codex_home.path(), vec![ordinary_model, reviewed_model])?;
|
||||
let mut app_server = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.build_initialized_with_timeout(TIMEOUT)
|
||||
.await?;
|
||||
let request_id = app_server
|
||||
.send_thread_start_request_with_auto_env(ThreadStartParams {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let thread: ThreadStartResponse =
|
||||
timeout(TIMEOUT, app_server.read_response(request_id)).await??;
|
||||
|
||||
state.allow_guardian_review.notify_one();
|
||||
for (model, expected_samples) in [
|
||||
(MODEL, 0),
|
||||
(REVIEWED_MODEL, 1),
|
||||
(MODEL, 1),
|
||||
(REVIEWED_MODEL, 2),
|
||||
] {
|
||||
state.parent_requests.store(0, Ordering::SeqCst);
|
||||
app_server.clear_message_buffer();
|
||||
state.allow_luna.notify_one();
|
||||
let request_id = app_server
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.thread.id.clone(),
|
||||
model: Some(model.to_owned()),
|
||||
input: vec![UserInput::Text {
|
||||
text: USER_CONTEXT.to_owned(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(request_id)).await??;
|
||||
let completed: TurnCompletedNotification =
|
||||
timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??;
|
||||
assert_eq!(completed.turn.status, TurnStatus::Completed);
|
||||
if model == REVIEWED_MODEL {
|
||||
wait_for_luna_request(&state, expected_samples - 1).await?;
|
||||
}
|
||||
assert_eq!(
|
||||
state
|
||||
.luna_requests
|
||||
.lock()
|
||||
.expect("luna requests lock")
|
||||
.len(),
|
||||
expected_samples,
|
||||
"only models requiring REPL review should send classifier requests"
|
||||
);
|
||||
}
|
||||
|
||||
app_server.shutdown_gracefully().await?;
|
||||
mcp_server.abort();
|
||||
responses_server.abort();
|
||||
Ok(())
|
||||
}
|
||||
@@ -1391,6 +1391,12 @@ impl ServerHandler for ToolAppsMcpServer {
|
||||
.map_err(|err| {
|
||||
rmcp::ErrorData::internal_error(err.to_string(), /*data*/ None)
|
||||
})?;
|
||||
if matches!(result.action, ElicitationAction::Decline) {
|
||||
return Ok(CallToolResult::error(vec![ContentBlock::text(
|
||||
"Tool execution was declined by Guardian.",
|
||||
)])
|
||||
.into());
|
||||
}
|
||||
assert_eq!(
|
||||
serde_json::to_value(result).expect("elicitation response"),
|
||||
json!({
|
||||
|
||||
@@ -280,6 +280,9 @@ impl ApprovalReviewContributor for GuardianV2Extension {
|
||||
.get("server")
|
||||
.and_then(serde_json::Value::as_str)
|
||||
.is_some_and(is_node_repl_backed_server)
|
||||
|| !thread_store
|
||||
.get::<ModelInfo>()
|
||||
.is_some_and(|model| model.node_repl_auto_review_required)
|
||||
{
|
||||
record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope");
|
||||
return None;
|
||||
@@ -498,16 +501,21 @@ impl GuardianV2Extension {
|
||||
};
|
||||
// Use the live reviewer, not the startup config or per-app reviewer overrides.
|
||||
let snapshot = thread.config_snapshot().await;
|
||||
let parent_model = input.thread_store.get::<ModelInfo>();
|
||||
if snapshot.full_access
|
||||
|| thread.approvals_reviewer_for_turn(input.turn_id).await == ApprovalsReviewer::User
|
||||
|| (guardian_config.review_scope == GuardianV2ReviewScope::ComputerUseOnly
|
||||
&& !parent_model
|
||||
.as_ref()
|
||||
.is_some_and(|model| model.node_repl_auto_review_required))
|
||||
{
|
||||
// A skipped call invalidates older scores, including ones still in flight.
|
||||
// A skipped call invalidates older scores, including ones still in flight
|
||||
// when switching to a model that does not require REPL review.
|
||||
score_progress
|
||||
.latest_failed_tool_call
|
||||
.fetch_max(tool_call_index, Ordering::Release);
|
||||
return;
|
||||
}
|
||||
let parent_model = input.thread_store.get::<ModelInfo>();
|
||||
// Computer-use-only scores cannot approve other tools for required models.
|
||||
if guardian_config.review_scope != GuardianV2ReviewScope::ComputerUseOnly
|
||||
&& parent_model.as_ref().is_some_and(|model| {
|
||||
|
||||
@@ -47,6 +47,7 @@ use codex_protocol::models::MessagePhase;
|
||||
use codex_protocol::models::ReasoningItemReasoningSummary;
|
||||
use codex_protocol::openai_models::GuardianV2ModelConfig;
|
||||
use codex_protocol::openai_models::GuardianV2TranscriptModelConfig;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
@@ -229,6 +230,13 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> {
|
||||
let progress = thread_store
|
||||
.get::<GuardianV2ScoreProgress>()
|
||||
.expect("Guardian v2 should initialize");
|
||||
let mut model = test
|
||||
.thread_manager
|
||||
.get_models_manager()
|
||||
.get_model_info("gpt-5.5", &config.to_models_manager_config())
|
||||
.await;
|
||||
model.node_repl_auto_review_required = true;
|
||||
thread_store.insert(model);
|
||||
let turn_store = ExtensionData::new("turn-1");
|
||||
let tool_name = ToolName::namespaced("mcp__node_repl__", "js");
|
||||
let payload = ToolPayload::Function {
|
||||
@@ -550,6 +558,14 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> {
|
||||
|
||||
let fixture = GuardianFailureFixture::new().await?;
|
||||
let thread_store = fixture.test.codex.thread_extension_data();
|
||||
let mut model = fixture
|
||||
.test
|
||||
.thread_manager
|
||||
.get_models_manager()
|
||||
.get_model_info("gpt-5.5", &fixture.test.config.to_models_manager_config())
|
||||
.await;
|
||||
model.node_repl_auto_review_required = true;
|
||||
thread_store.insert(model);
|
||||
let mut config = thread_store
|
||||
.get::<crate::async_scorer::config::GuardianV2Config>()
|
||||
.expect("Guardian v2 should have initialized")
|
||||
@@ -687,6 +703,41 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> {
|
||||
]
|
||||
);
|
||||
|
||||
let mut model = thread_store.get::<ModelInfo>().unwrap().as_ref().clone();
|
||||
model.node_repl_auto_review_required = false;
|
||||
thread_store.insert(model.clone());
|
||||
fixture
|
||||
.score_tool(ToolName::namespaced("mcp__node_repl__", "js"))
|
||||
.await;
|
||||
assert!(
|
||||
progress.latest_failed_tool_call.load(Ordering::Acquire)
|
||||
> progress.latest_scored_tool_call.load(Ordering::Acquire)
|
||||
);
|
||||
for required in [false, true] {
|
||||
model.node_repl_auto_review_required = required;
|
||||
thread_store.insert(model.clone());
|
||||
// Also reject a low score published by an older, in-flight classifier.
|
||||
thread_store.insert(SecurityRiskScore {
|
||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.0)]),
|
||||
call_id: None,
|
||||
action: None,
|
||||
sampled_at: None,
|
||||
});
|
||||
assert_eq!(
|
||||
fixture
|
||||
.registry
|
||||
.fast_approval_decision(
|
||||
&fixture.session_store,
|
||||
thread_store,
|
||||
r#"{"tool":"mcp_tool_call","server":"node_repl","tool_name":"js"}"#,
|
||||
/*extension_metrics*/ None,
|
||||
)
|
||||
.await,
|
||||
None,
|
||||
"switching back to a reviewed model must not revive a skipped score"
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user