From 663eb5fbdda690ca4471d92b3f9a29db43c5ba22 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 10 Sep 2026 18:33:46 +0000 Subject: [PATCH] Invalidate cached Guardian approvals for unscored permission widening (#44617) ## Why An unscored `exec_command` requesting additional permissions could reuse an earlier Guardian v2 approval score, even though that score did not cover the expanded permissions. ## What changed When `UnscoredAction::AgeScore` applies, invalidate the cached score for default-namespace `exec_command` calls with `sandbox_permissions` set to `with_additional_permissions` by marking the call as a scoring failure. ## Testing Extend the review-scope regression test to verify that an ordinary sandboxed command preserves cached approval, while a command requesting additional network permissions clears it and causes review to fail closed with `scoring_failure`. GitOrigin-RevId: d6e3e1ec7d618dff2ef03ffcb92869249f0e547a --- .../guardian-v2/src/async_scorer/extension.rs | 15 +++++ .../src/async_scorer/extension_tests.rs | 62 ++++++++++++------- 2 files changed, 56 insertions(+), 21 deletions(-) diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 0b98fd0029..964329567c 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -32,6 +32,7 @@ use codex_extension_api::ThreadStartInput; use codex_extension_api::ToolFinishInput; use codex_extension_api::ToolLifecycleContributor; use codex_extension_api::ToolLifecycleFuture; +use codex_extension_api::ToolPayload; use codex_extension_api::ToolStartInput; use codex_features::Feature; use codex_guardian_context::ContextTarget; @@ -277,6 +278,20 @@ impl GuardianV2Extension { .fetch_add(/*val*/ 1, Ordering::Relaxed) .saturating_add(/*rhs*/ 1); score_progress.wrapper_lag.record(&input, index); + // Unscored permission widening must not reuse an earlier approval score. + if input.tool_name.is_default_namespace() + && input.tool_name.name == "exec_command" + && let ToolPayload::Function { arguments } = input.payload + && let Ok(arguments) = serde_json::from_str::(arguments) + && arguments + .get("sandbox_permissions") + .and_then(serde_json::Value::as_str) + == Some("with_additional_permissions") + { + score_progress + .latest_failed_tool_call + .fetch_max(index, Ordering::Release); + } } UnscoredAction::InvalidateScore => { let index = score_progress diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index dc5469ae74..438308b8a8 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -490,7 +490,7 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { arguments: r#"{"cmd":"pwd"}"#.to_owned(), }; let additional_permissions = ToolPayload::Function { - arguments: r#"{"cmd":"pwd","sandbox_permissions":"with_additional_permissions"}"# + arguments: r#"{"cmd":"pwd","sandbox_permissions":"with_additional_permissions","additional_permissions":{"network":{"enabled":true}}}"# .to_owned(), }; let unsandboxed = ToolPayload::Function { @@ -539,6 +539,13 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { let fixture = GuardianFailureFixture::new().await?; let thread_store = fixture.test.codex.thread_extension_data(); + let mut score = thread_store + .get::() + .expect("fixture should publish a score") + .as_ref() + .clone(); + score.scores.insert("action_risk".to_owned(), 0.0); + thread_store.insert(score); let score_progress = thread_store .get::() .expect("Guardian v2 should track score progress per thread"); @@ -547,30 +554,42 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { .load(Ordering::Acquire); let turn_store = ExtensionData::new("turn-1"); let tool_name = ToolName::plain("exec_command"); - let payload = ToolPayload::Function { - arguments: r#"{"cmd":"pwd"}"#.to_owned(), - }; + for (call_id, payload, expected_decision) in [ + ("call-2", sandboxed, Some(ReviewDecision::Approved)), + ("call-3", additional_permissions, None), + ] { + fixture.registry.tool_lifecycle_contributors()[0] + .on_tool_start(ToolStartInput { + session_store: &fixture.session_store, + thread_store, + turn_store: &turn_store, + turn_id: "turn-1", + root_turn_id: None, + call_id, + originating_item_id: None, + tool_name: &tool_name, + mcp_tool: None, + payload: &payload, + conversation_history: Arc::new(TestConversationHistory(Vec::new())), + source: ToolCallSource::Direct, + }) + .await; - fixture.registry.tool_lifecycle_contributors()[0] - .on_tool_start(ToolStartInput { - session_store: &fixture.session_store, - thread_store, - turn_store: &turn_store, - turn_id: "turn-1", - root_turn_id: None, - call_id: "call-2", - originating_item_id: None, - tool_name: &tool_name, - mcp_tool: None, - payload: &payload, - conversation_history: Arc::new(TestConversationHistory(Vec::new())), - source: ToolCallSource::Direct, - }) - .await; + assert_eq!( + cached_approval( + &fixture.registry, + thread_store, + r#"{"tool":"exec_command","cmd":"pwd"}"#, + /*metrics*/ None, + ) + .await, + expected_decision, + ); + } assert_eq!( score_progress.latest_tool_call.load(Ordering::Acquire), - latest_scored_tool_call + 1 + latest_scored_tool_call + 2 ); assert_eq!( score_progress @@ -578,6 +597,7 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { .load(Ordering::Acquire), latest_scored_tool_call ); + fixture.assert_fails_closed("scoring_failure").await?; Ok(()) }