From 9c4879f3a5bfdc7fb5401b7d3ebcdc9a77c27aa2 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Thu, 10 Sep 2026 15:47:12 +0000 Subject: [PATCH] Preserve complete actions in Guardian approval reviews (#44569) ## Why Truncating action arguments can leave approval reviewers evaluating incomplete actions. Large actions need complete review input and explicit handling when they exceed the review budget. ## What changed - Remove action truncation and the fixed synchronous action byte limit. Admit complete actions against the whole-request budget, splitting long text losslessly into bounded transport parts and accounting for their framing. - Route actions exceeding the asynchronous action budget to synchronous review. Prevent cached scores from covering oversized calls, including expanded approval arguments, while allowing later small actions to recover score reuse. - Request user approval when optional review exhausts its local input budget. Keep required review and compaction service failures closed to approval, and retire exhausted review sessions. ## Testing Add coverage for complete large-action delivery, optional user fallback, required-review denial, subsequent review recovery, async overflow through MCP approval routing, and lossless text splitting with budget accounting. GitOrigin-RevId: 08f06e68a94b2f779480dd6b6cf5bc30b241f6dd --- .../suite/v2/guardian_action_budget_tests.rs | 216 +++++++++++++++ .../app-server/tests/suite/v2/guardian_v2.rs | 3 + .../core/src/guardian/approval_request.rs | 70 +---- codex-rs/core/src/guardian/decision.rs | 14 +- codex-rs/core/src/guardian/feedback.rs | 4 +- codex-rs/core/src/guardian/input_budget.rs | 10 +- codex-rs/core/src/guardian/mod.rs | 2 - codex-rs/core/src/guardian/prompt.rs | 4 +- codex-rs/core/src/guardian/request_budget.rs | 8 +- codex-rs/core/src/guardian/review_request.rs | 6 +- codex-rs/core/src/guardian/review_session.rs | 34 ++- codex-rs/core/src/guardian/tests.rs | 41 --- codex-rs/core/src/session/turn.rs | 6 + .../core/src/unified_exec/process_manager.rs | 10 +- .../tests/suite/guardian_context_budget.rs | 250 +++++++++++++++++- .../src/contributors/approval_review.rs | 5 +- codex-rs/ext/extension-api/tests/registry.rs | 6 +- .../ext/guardian-reviewer/src/completion.rs | 46 +++- .../ext/guardian-reviewer/src/feedback.rs | 1 + codex-rs/ext/guardian-reviewer/src/outcome.rs | 9 +- .../ext/guardian-reviewer/src/reporting.rs | 3 +- codex-rs/ext/guardian-reviewer/src/retry.rs | 3 +- .../ext/guardian-reviewer/src/retry_tests.rs | 4 + codex-rs/ext/guardian-reviewer/src/review.rs | 7 +- .../guardian-v2/src/async_scorer/action.rs | 136 +--------- .../src/async_scorer/action_tests.rs | 117 ++------ .../guardian-v2/src/async_scorer/approval.rs | 30 ++- .../guardian-v2/src/async_scorer/extension.rs | 82 ++++-- .../src/async_scorer/extension_tests.rs | 158 +++-------- codex-rs/guardian-context/src/action.rs | 6 +- codex-rs/guardian-context/src/budget.rs | 16 +- codex-rs/guardian-context/src/composition.rs | 40 ++- .../guardian-context/src/composition_tests.rs | 41 +++ 33 files changed, 841 insertions(+), 547 deletions(-) create mode 100644 codex-rs/app-server/tests/suite/v2/guardian_action_budget_tests.rs diff --git a/codex-rs/app-server/tests/suite/v2/guardian_action_budget_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_action_budget_tests.rs new file mode 100644 index 0000000000..d9b7291651 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/guardian_action_budget_tests.rs @@ -0,0 +1,216 @@ +//! Exercises async overflow and score recovery through real MCP approval routing. + +use super::*; +use pretty_assertions::assert_eq; +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn oversized_async_action_requires_sync_review_and_later_scores_recover() -> Result<()> { + skip_if_no_network!(Ok(())); + let state = Arc::new(MockResponsesState::default()); + let gates = [Arc::new(Notify::new()), Arc::new(Notify::new())]; + let parent_gates = gates.clone(); + let oversized = "required argument ".repeat(/*n*/ 100) + "required suffix"; + let action_message = oversized.clone(); + let oversized_waiting = Arc::new(Notify::new()); + let permission_gate = Arc::clone(&gates[1]); + 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( + move |State(state): State>, Json(request): Json| { + let gates = parent_gates.clone(); + let oversized = action_message.clone(); + async move { + if request.pointer("/client_metadata/x-openai-subagent") + == Some(&json!("guardian")) + { + return parent_response(State(state), Json(request)) + .await + .into_response(); + } + let index = + state.parent_requests.fetch_add(/*val*/ 1, Ordering::SeqCst); + if index == 1 { + gates[0].notified().await; + } + let actions: &[usize] = match index { + 0 => &[0], + 1 => &[1, 2], + 2 => &[3], + _ => &[], + }; + let response_id = format!("response-{index}"); + let mut events = vec![responses::ev_response_created(&response_id)]; + for &action in actions { + let call_id = format!("action-{action}"); + let message = if action == 1 { + oversized.clone() + } else { + format!("small-{action}") + }; + events.push(responses::ev_function_call_with_namespace( + &call_id, + &format!("mcp__{TEST_SERVER_NAME}"), + TEST_TOOL_NAME, + &json!({"message": message}).to_string(), + )); + } + if actions.is_empty() { + events.push(responses::ev_assistant_message("done", "done")); + } + events.push(responses::ev_completed(&response_id)); + ( + [(header::CONTENT_TYPE, "text/event-stream")], + responses::sse(events), + ) + .into_response() + } + }, + ), + ) + .route( + "/approval-barrier", + post(move |Json(input): Json| { + let oversized_waiting = Arc::clone(&oversized_waiting); + let permission_gate = Arc::clone(&permission_gate); + async move { + let message = input["tool_input"]["message"].as_str().unwrap_or_default(); + if input["hook_event_name"] == "PermissionRequest" + && message.starts_with("required argument") + { + // Let the later call start scoring only after overflow was recorded. + oversized_waiting.notify_one(); + permission_gate.notified().await; + } else if input["hook_event_name"] == "PreToolUse" && message == "small-2" { + oversized_waiting.notified().await; + } + Json(json!({})) + } + }), + ) + .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(/*sensitive_action*/ None).await?; + let codex_home = TempDir::new()?; + let hook_path = codex_home.path().join("approval-barrier.py"); + let endpoint = format!("{responses_url}/approval-barrier"); + std::fs::write( + &hook_path, + format!( + r#"import sys +import urllib.request + +request = urllib.request.Request({endpoint:?}, data=sys.stdin.buffer.read(), + headers={{"Content-Type": "application/json"}}) +with urllib.request.urlopen(request, timeout=30) as response: + print(response.read().decode()) +"# + ), + )?; + let command = serde_json::to_string(&format!("python3 \"{}\"", hook_path.display()))?; + let hooks = ["PreToolUse", "PermissionRequest"] + .map(|event| format!( + "[[hooks.{event}]]\nmatcher = '^mcp__'\n[[hooks.{event}.hooks]]\ntype = 'command'\ncommand = {command}\n" + )) + .join("\n"); + std::fs::write(codex_home.path().join("requirements.toml"), hooks)?; + let analytics_server = responses::start_mock_server().await; + mount_analytics_capture(&analytics_server, codex_home.path()).await?; + MockResponsesConfig::new(&responses_url) + .with_model(MODEL) + .with_provider_config("supports_websockets = false") + .with_approval_policy("on-request") + .with_root_config(&format!("approvals_reviewer = \"auto_review\"\nchatgpt_base_url = \"{}\"", analytics_server.uri())) + .with_extra_config(&format!( + "[mcp_servers.{TEST_SERVER_NAME}]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"prompt\"\n\n[analytics]\nenabled = true\n\n[features.guardianv2]\nenabled = true\nmax_action_tokens = 128\n\n[features.guardianv2.review_scope]\ncomputer_use_only = false" + )) + .enable_feature(Feature::GuardianApproval) + .write(codex_home.path())?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(TIMEOUT) + .await?; + let thread = app_server + .start_thread(ThreadStartParams::default()) + .await? + .thread; + state.allow_guardian_review.notify_one(); + let id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + input: vec![UserInput::Text { + text: USER_CONTEXT.to_owned(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(id)).await??; + for (index, gate) in gates.iter().enumerate() { + wait_for_luna_request(&state, index).await?; + // The oversized call is still in its permission hook; + // only the seed and the later small call can have begun synchronous review. + wait_for_guardian_reviews(&state, 1 + index).await?; + // First establish a permissive score; after overflow, establish a new one. + state.allow_luna.notify_one(); + timeout(TIMEOUT, async { + loop { + let events = captured_analytics_events(&analytics_server).await; + if events + .iter() + .filter(|event| { + event["event_type"] == "codex_guardian_v2_classification" + && event["event_params"]["outcome"] == "success" + }) + .count() + > index + { + break; + } + tokio::time::sleep(Duration::from_millis(/*millis*/ 25)).await; + } + }) + .await?; + gate.notify_one(); + } + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; + assert_eq!(completed.turn.status, TurnStatus::Completed); + wait_for_luna_request(&state, /*index*/ 2).await?; + app_server.shutdown_gracefully().await?; + // First call, oversized call, and recovery call need review. The fourth reuses + // the recovered score. Only the three small actions reach the async model. + assert_eq!( + ( + state.guardian_reviews.load(Ordering::SeqCst), + state + .luna_requests + .lock() + .expect("Luna request lock should not be poisoned") + .len() + ), + (3, 3) + ); + let reviews = state + .guardian_requests + .lock() + .expect("Guardian request lock should not be poisoned"); + let action = reviews[2]["input"] + .as_array() + .expect("Guardian request should contain input items") + .iter() + .rev() + .filter_map(|item| item["content"].as_array()) + .flatten() + .filter_map(|part| part["text"].as_str()) + .filter_map(|text| serde_json::from_str::(text).ok()) + .find(|value| value["tool"] == "mcp_tool_call") + .expect("complete MCP action"); + assert_eq!(action["arguments"], json!({"message": oversized})); + mcp_server.abort(); + responses_server.abort(); + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index ab5b6ecd31..ea22b528a2 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -92,6 +92,9 @@ mod policy; #[path = "guardian_code_mode_tests.rs"] mod code_mode; +#[path = "guardian_action_budget_tests.rs"] +mod action_budget; + const TIMEOUT: Duration = Duration::from_secs(30); const MODEL: &str = "mock-model"; const REQUIRED_MODEL: &str = "protected-model"; diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index eb7dc06868..ffed746fc8 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -12,13 +12,8 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathUri; use serde::Serialize; -use serde::ser::Error as _; use serde_json::Value; -use super::GUARDIAN_MAX_ACTION_BYTES; -use super::GUARDIAN_MAX_ACTION_STRING_TOKENS; -use super::prompt::guardian_truncate_text; - #[derive(Debug, Clone, PartialEq)] pub(crate) enum GuardianApprovalRequest { ExecCommand { @@ -249,49 +244,6 @@ fn guardian_command_source_tool_name(source: GuardianCommandSource) -> &'static } } -fn truncate_guardian_action_value(value: Value) -> (Value, bool) { - match value { - Value::String(text) => { - let (text, truncated) = - guardian_truncate_text(&text, GUARDIAN_MAX_ACTION_STRING_TOKENS); - (Value::String(text), truncated) - } - Value::Array(values) => { - let mut truncated = false; - let values = values - .into_iter() - .map(|value| { - let (value, value_truncated) = truncate_guardian_action_value(value); - truncated |= value_truncated; - value - }) - .collect::>(); - (Value::Array(values), truncated) - } - Value::Object(values) => { - let mut entries = values.into_iter().collect::>(); - entries.sort_by(|(left, _), (right, _)| left.cmp(right)); - let mut truncated = false; - let values = entries - .into_iter() - .map(|(key, value)| { - let (value, value_truncated) = truncate_guardian_action_value(value); - truncated |= value_truncated; - (key, value) - }) - .collect(); - (Value::Object(values), truncated) - } - other => (other, false), - } -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct FormattedGuardianAction { - pub(crate) text: String, - pub(crate) truncated: bool, -} - pub(crate) fn guardian_approval_request_to_json( action: &GuardianApprovalRequest, ) -> serde_json::Result { @@ -584,26 +536,10 @@ pub(crate) fn guardian_request_turn_id<'a>( pub(crate) fn format_guardian_action_pretty( action: &GuardianApprovalRequest, -) -> serde_json::Result { - let value = guardian_action_for_review(action)?; - let (value, truncated) = truncate_guardian_action_value(value); - let text = enforce_guardian_action_byte_limit(serde_json::to_string_pretty(&value)?)?; - Ok(FormattedGuardianAction { text, truncated }) -} - -fn enforce_guardian_action_byte_limit(text: String) -> serde_json::Result { - if text.len() > GUARDIAN_MAX_ACTION_BYTES { - return Err(serde_json::Error::custom(format!( - "Guardian action exceeds the {GUARDIAN_MAX_ACTION_BYTES}-byte review limit" - ))); - } - Ok(text) -} - -pub(crate) fn format_guardian_action_compact( - action: &GuardianApprovalRequest, ) -> serde_json::Result { - enforce_guardian_action_byte_limit(serde_json::to_string(&guardian_action_for_review(action)?)?) + let mut value = guardian_action_for_review(action)?; + value.sort_all_objects(); + serde_json::to_string_pretty(&value) } fn guardian_action_for_review(action: &GuardianApprovalRequest) -> serde_json::Result { diff --git a/codex-rs/core/src/guardian/decision.rs b/codex-rs/core/src/guardian/decision.rs index fe5329d5ae..22ba7dc8d3 100644 --- a/codex-rs/core/src/guardian/decision.rs +++ b/codex-rs/core/src/guardian/decision.rs @@ -79,9 +79,6 @@ pub(crate) async fn decide_approval( .as_ref() .is_some_and(CancellationToken::is_cancelled) || reasons.retry.is_some() - || request.request.as_ref().is_ok_and(|request| { - super::approval_request::format_guardian_action_compact(request).is_err() - }) || matches!(&request.request, Ok(GuardianApprovalRequest::ExecCommand { sandbox_permissions, .. }) if sandbox_permissions.requires_escalated_permissions()); let full_access = context.environments().has_full_access( @@ -115,7 +112,10 @@ pub(crate) async fn decide_approval( review_id: review_id.clone(), request: request.clone(), reasons, - options, + options: GuardianReviewOptions { + require_guardian, + ..options + }, }); let input = ApprovalDecisionInput { approval_id: &review_id, @@ -179,9 +179,7 @@ pub(crate) async fn decide_approval( record_guardian_non_denial(&session, turn_id).await; Some(ReviewDecision::Approved) } - Some(ApprovalDecision::Allow) => { - Some(runtime.review(GuardianReviewReason::FreshRequired).await) - } + Some(ApprovalDecision::Allow) => runtime.review(GuardianReviewReason::FreshRequired).await, Some(ApprovalDecision::AskUser) if !require_guardian => None, None if !require_guardian && !super::review::routes_approval_policy_to_guardian( @@ -192,7 +190,7 @@ pub(crate) async fn decide_approval( None } None | Some(ApprovalDecision::AskUser) => { - Some(runtime.review(GuardianReviewReason::Policy).await) + runtime.review(GuardianReviewReason::Policy).await } } } diff --git a/codex-rs/core/src/guardian/feedback.rs b/codex-rs/core/src/guardian/feedback.rs index 64c964611f..b227af8a5f 100644 --- a/codex-rs/core/src/guardian/feedback.rs +++ b/codex-rs/core/src/guardian/feedback.rs @@ -36,8 +36,8 @@ pub(super) async fn record_failed_review( target_item_id: guardian_request_target_item_id(¶ms.request), reviewer_thread_id: reviewer.thread_id(), model: ¶ms.model, - action: &action.text, - action_truncated: action.truncated, + action: &action, + action_truncated: false, instructions: Some(&instructions.text), history: history.raw_items().collect(), }); diff --git a/codex-rs/core/src/guardian/input_budget.rs b/codex-rs/core/src/guardian/input_budget.rs index e1f7b42445..70df94beb9 100644 --- a/codex-rs/core/src/guardian/input_budget.rs +++ b/codex-rs/core/src/guardian/input_budget.rs @@ -52,7 +52,13 @@ pub(crate) async fn check_pending(session: &Session, turn: &TurnContext) -> Code }, GuardianBudgetOmission.render(), ) - .map_err(|error| CodexErr::InvalidRequest(error.to_string()))?; + .map_err(|_error| { + session + .services + .thread_extension_data + .insert(super::request_budget::ExhaustedReviewBudget::Detected); + CodexErr::ContextWindowExceeded + })?; } Ok(()) } @@ -144,7 +150,7 @@ pub(crate) async fn finalize( session .services .thread_extension_data - .insert(super::request_budget::ExhaustedReviewBudget); + .insert(super::request_budget::ExhaustedReviewBudget::Detected); match error { codex_guardian_context::SectionError::EvidenceLimitExceeded { .. } => { CodexErr::ContextWindowExceeded diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 862d2eb995..d405d982a7 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -69,8 +69,6 @@ const GUARDIAN_MAX_TOOL_ENTRY_TOKENS: usize = codex_guardian_context::ContextPro .tool_tokens; pub(crate) const GUARDIAN_MAX_ROOT_MESSAGE_TOKENS: usize = 900; pub(crate) const GUARDIAN_MAX_NODE_REPL_TOOL_RESULT_TOKENS: usize = 6_000; -pub(crate) const GUARDIAN_MAX_ACTION_BYTES: usize = 50_000 * 4; -const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000; /// Captures review inputs from the issuing step without retaining its MCP bindings or tool router. /// Background network approvals and Unix interception use the active task's resolved settings. diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index 953cb94c61..d5260ddb32 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -42,7 +42,6 @@ pub(crate) struct GuardianPromptItems { pub(crate) context: ComposedContext, pub(crate) transcript_cursor: GuardianTranscriptCursor, pub(crate) node_repl_evidence_sequence: u64, - pub(crate) reviewed_action_truncated: bool, } /// Points to the end of the transcript that the guardian has already reviewed. @@ -120,7 +119,7 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn( .fragments; let planned_action_json = format_guardian_action_pretty(&request)?; let planned_action = PlannedAction { - json: planned_action_json.text, + json: planned_action_json, tool_descriptions: if let GuardianApprovalRequest::McpToolCall { tool_description, connector_description, @@ -234,7 +233,6 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn( context, transcript_cursor, node_repl_evidence_sequence, - reviewed_action_truncated: planned_action_json.truncated, }) } diff --git a/codex-rs/core/src/guardian/request_budget.rs b/codex-rs/core/src/guardian/request_budget.rs index 24cc7948d8..0f9fa0e0fe 100644 --- a/codex-rs/core/src/guardian/request_budget.rs +++ b/codex-rs/core/src/guardian/request_budget.rs @@ -19,7 +19,11 @@ pub(super) const INPUT_TOKEN_MARGIN: usize = 256; // Keep a failed reviewer out of reuse unless compaction and a fresh budget // check succeed. Also distinguishes local budget failures from backend errors. -pub(crate) struct ExhaustedReviewBudget; +pub(crate) enum ExhaustedReviewBudget { + Detected, + // A compaction service failure must not be mistaken for local exhaustion. + Compacting, +} pub(crate) fn observe(telemetry: &SessionTelemetry, request: &ResponsesApiRequest) -> usize { let total = estimate_request_tokens(request); @@ -74,7 +78,7 @@ pub(crate) fn check_prompt( session .services .thread_extension_data - .insert(ExhaustedReviewBudget); + .insert(ExhaustedReviewBudget::Detected); return Err(CodexErr::ContextWindowExceeded); } session diff --git a/codex-rs/core/src/guardian/review_request.rs b/codex-rs/core/src/guardian/review_request.rs index c546c5aebc..7683c8834f 100644 --- a/codex-rs/core/src/guardian/review_request.rs +++ b/codex-rs/core/src/guardian/review_request.rs @@ -116,6 +116,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { let completed = report.complete( GuardianReviewOutcome::Error(GuardianReviewError::Cancelled), turn.model_info(), + self.options.require_guardian, GuardianReviewAnalyticsResult::without_session(), completed_at_ms, ); @@ -150,7 +151,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { format_guardian_action_pretty(&request).ok().map(|action| { ( evidence, - action.text, + action, authorization_version, root_authorization_version, ) @@ -191,7 +192,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { prepared: PreparedApproval, mut outcome: GuardianReviewOutcome, analytics_result: GuardianReviewAnalyticsResult, - ) -> ReviewDecision { + ) -> Option { let PreparedApproval { request: _, turn, @@ -225,6 +226,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { let completed = report.complete( outcome, turn.model_info(), + self.options.require_guardian, analytics_result, completed_at_ms, ); diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 1176d5e34a..75495b185b 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -39,7 +39,6 @@ use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::InputModality; use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; -#[cfg(test)] use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -610,7 +609,6 @@ async fn run_review_on_session( ); } }; - let reviewed_action_truncated = prompt_items.reviewed_action_truncated; let transcript_cursor = prompt_items.transcript_cursor; let node_repl_evidence_admission = (prompt_items.node_repl_evidence_sequence > last_admitted_node_repl_response_sequence) @@ -691,7 +689,6 @@ async fn run_review_on_session( response_sequence, }); } - analytics_result.reviewed_action_truncated = reviewed_action_truncated; let outcome = wait_for_guardian_review( review_session, @@ -719,14 +716,29 @@ async fn run_review_on_session( state.prior_review_count = state.prior_review_count.saturating_add(1); state.last_reviewed_transcript_cursor = Some(transcript_cursor); } - let keep_review_session = outcome.1 - && review_session - .session - .services - .thread_extension_data - .remove::() - .is_none(); - (outcome.0, keep_review_session, analytics_result) + let budget_exhausted = review_session + .session + .services + .thread_extension_data + .remove::(); + let result = match outcome.0 { + GuardianReviewSessionOutcome::SessionFailed { + error_info: Some(CodexErrorInfo::ContextWindowExceeded), + .. + } if matches!( + budget_exhausted.as_deref(), + Some(super::request_budget::ExhaustedReviewBudget::Detected) + ) => + { + GuardianReviewSessionOutcome::InputBudgetExceeded + } + result => result, + }; + ( + result, + outcome.1 && budget_exhausted.is_none(), + analytics_result, + ) } async fn ensure_guardian_followup_reminder(review_session: &GuardianReviewSession) { diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index c3a51011be..9ecea33961 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -7,7 +7,6 @@ use crate::config::NetworkProxySpec; use crate::config::PermissionProfileSnapshot; use crate::config::test_config; use crate::environment_selection::TurnEnvironmentState; -use crate::guardian::approval_request::format_guardian_action_compact; use crate::guardian::approval_request::guardian_request_target_item_id; use crate::guardian::prompt::BUNDLED_GUARDIAN_POLICY; use crate::guardian::prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE; @@ -1130,46 +1129,6 @@ fn guardian_truncate_text_keeps_prefix_suffix_and_xml_marker() { assert!(was_truncated); } -#[test] -fn guardian_action_formatters_reject_large_aggregate_payloads() { - let file: PathUri = test_path_buf("/tmp/file").abs().into(); - let action = GuardianApprovalRequest::ApplyPatch { - id: "patch-1".to_string(), - cwd: test_path_buf("/tmp").abs().into(), - files: vec![file; 20_000], - patch: String::new(), - }; - - for error in [ - format_guardian_action_pretty(&action).map(|_| ()), - format_guardian_action_compact(&action).map(|_| ()), - ] { - assert_eq!( - error - .expect_err("aggregate action should exceed the review limit") - .to_string(), - "Guardian action exceeds the 200000-byte review limit" - ); - } -} - -#[test] -fn format_guardian_action_pretty_reports_no_truncation_for_small_payload() -> serde_json::Result<()> -{ - let action = GuardianApprovalRequest::ApplyPatch { - id: "patch-1".to_string(), - cwd: test_path_buf("/tmp").abs().into(), - files: Vec::new(), - patch: "line\n".to_string(), - }; - - let rendered = format_guardian_action_pretty(&action)?; - - assert!(rendered.text.contains("\"tool\": \"apply_patch\"")); - assert!(!rendered.truncated); - Ok(()) -} - #[test] fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json::Result<()> { let action = GuardianApprovalRequest::McpToolCall { diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 8940adb91f..497ca64dd9 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -333,6 +333,9 @@ pub(crate) async fn run_turn( } // Incoming evidence can overflow even below the normal history // threshold. Keep it pending while compacting, then select once more. + sess.services + .thread_extension_data + .insert(crate::guardian::ExhaustedReviewBudget::Compacting); run_auto_compact( &sess, Arc::clone(&first_step_context), @@ -704,6 +707,9 @@ pub(crate) async fn run_turn( // token-budget resets must fail closed and retire the reviewer. // Retry once per model step, so ineffective compaction cannot loop. guardian_budget_compacted = true; + sess.services + .thread_extension_data + .insert(crate::guardian::ExhaustedReviewBudget::Compacting); run_auto_compact( &sess, Arc::clone(&step_context), diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index d68669e41b..c04c1a897a 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -865,11 +865,9 @@ impl UnifiedExecProcessManager { .map_err(|_| unreviewable_input_error())?; // Bound the entire serialized action plus its reason, including JSON // escaping. Reject, never execute an unreviewed tail. - let oversized = reviewed.text.len().saturating_add(approval_reason.len()) - > MAX_STDIN_APPROVAL_BYTES; - let size_check_result = if reviewed.truncated { - "formatter_truncated" - } else if oversized { + let oversized = + reviewed.len().saturating_add(approval_reason.len()) > MAX_STDIN_APPROVAL_BYTES; + let size_check_result = if oversized { "over_limit" } else { "within_limit" @@ -884,7 +882,7 @@ impl UnifiedExecProcessManager { /*inc*/ 1, &[("result", size_check_result), ("input_kind", input_kind)], ); - if reviewed.truncated || oversized { + if oversized { return Err(unreviewable_input_error()); } let approval_context = ApprovalContext { diff --git a/codex-rs/core/tests/suite/guardian_context_budget.rs b/codex-rs/core/tests/suite/guardian_context_budget.rs index 05441e50d1..c8933c9096 100644 --- a/codex-rs/core/tests/suite/guardian_context_budget.rs +++ b/codex-rs/core/tests/suite/guardian_context_budget.rs @@ -2,6 +2,8 @@ //! Parent token-budget mode must not replace Guardian's summary compaction. use anyhow::Result; +use codex_config::test_support::CloudConfigBundleFixture; +use codex_core::TurnInputRequest; use codex_core::config::Constrained; use codex_core::config::CurrentTimeReminderConfig; use codex_core::config::RolloutBudgetConfig; @@ -9,6 +11,10 @@ use codex_features::Feature; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::openai_models::AutoReviewMessages; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; +use codex_protocol::user_input::UserInput; use core_test_support::responses; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -19,6 +25,7 @@ use core_test_support::responses::sse; use core_test_support::skip_if_no_network; use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use serde_json::json; use test_case::test_case; @@ -28,6 +35,7 @@ enum ReviewerResponse { Decision, ToolContinuation, UncompactableContinuation, + CompactionError, NextReview, } @@ -35,6 +43,7 @@ enum ReviewerResponse { #[test_case(1, ReviewerResponse::Decision; "required_context_fails_closed")] #[test_case(4_500, ReviewerResponse::ToolContinuation; "oversized_tool_continuation_compacts")] #[test_case(4_500, ReviewerResponse::UncompactableContinuation; "ineffective_compaction_fails_closed")] +#[test_case(4_500, ReviewerResponse::CompactionError; "compaction_service_error_does_not_request_user_approval")] #[test_case(6_000, ReviewerResponse::NextReview; "incoming_review_compacts_existing_history")] async fn review_respects_complete_context_budget( window: i64, @@ -47,6 +56,15 @@ async fn review_respects_complete_context_budget( ); let server = responses::start_mock_server().await; let mut builder = test_codex() + .with_cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement( + if matches!(reviewer_response, ReviewerResponse::CompactionError) { + "" + } else { + "[auto_review]\nrequired_on_models = [\"gpt-5.5\"]\n" + }, + ), + ) .with_model_info_override("gpt-5.6-luna", move |model| { model.context_window = Some(window); model @@ -85,7 +103,9 @@ async fn review_respects_complete_context_budget( }); if matches!( reviewer_response, - ReviewerResponse::ToolContinuation | ReviewerResponse::UncompactableContinuation + ReviewerResponse::ToolContinuation + | ReviewerResponse::UncompactableContinuation + | ReviewerResponse::CompactionError ) { builder = builder .with_code_mode_host_program(codex_utils_cargo_bin::cargo_bin("codex-code-mode-host")?); @@ -131,7 +151,8 @@ async fn review_respects_complete_context_budget( .to_string(), ), ReviewerResponse::ToolContinuation - | ReviewerResponse::UncompactableContinuation => ev_custom_tool_call( + | ReviewerResponse::UncompactableContinuation + | ReviewerResponse::CompactionError => ev_custom_tool_call( "reviewer-inspect", "exec", "text('inspection-output'.repeat(600));", @@ -155,7 +176,9 @@ async fn review_respects_complete_context_budget( } if matches!( reviewer_response, - ReviewerResponse::UncompactableContinuation | ReviewerResponse::NextReview + ReviewerResponse::UncompactableContinuation + | ReviewerResponse::NextReview + | ReviewerResponse::CompactionError ) { events.extend([ sse(vec![ @@ -175,7 +198,10 @@ async fn review_respects_complete_context_budget( ]), ]); } - if !matches!(reviewer_response, ReviewerResponse::Decision) { + if !matches!( + reviewer_response, + ReviewerResponse::Decision | ReviewerResponse::CompactionError + ) { let summary = if matches!( reviewer_response, ReviewerResponse::UncompactableContinuation @@ -200,7 +226,19 @@ async fn review_respects_complete_context_budget( ]), ); } - let responses = responses::mount_sse_sequence(&server, events).await; + let mut events = events + .into_iter() + .map(responses::sse_response) + .collect::>(); + if matches!(reviewer_response, ReviewerResponse::CompactionError) { + events.insert( + /*index*/ 2, + wiremock::ResponseTemplate::new(400).set_body_json(json!({ + "error": {"code": "context_length_exceeded", "message": "compaction service context window exceeded"} + })), + ); + } + let responses = responses::mount_response_sequence(&server, events).await; test.submit_text_turn("Run the command if the approval reviewer allows it.") .await?; let (compact_requests, requests): (Vec<_>, Vec<_>) = responses @@ -218,9 +256,7 @@ async fn review_respects_complete_context_budget( assert!(guardian_requests.is_empty()); let output = requests[1].function_call_output("exec-over-budget"); assert!( - output - .to_string() - .contains("evidence exceeds limits for section request_budget"), + output.to_string().contains("reviewer input budget"), "expected required-evidence budget rejection: {output}" ); } else { @@ -264,12 +300,20 @@ async fn review_respects_complete_context_budget( .expect("parent continues after the rejected review") .function_call_output("exec-over-budget"); assert!( - output - .to_string() - .contains("Codex ran out of room in the model's context window"), + output.to_string().contains("reviewer input budget"), "expected context-budget rejection: {output}" ); } + if matches!(reviewer_response, ReviewerResponse::CompactionError) { + assert!( + requests + .last() + .expect("parent resumes after review") + .function_call_output("exec-over-budget") + .to_string() + .contains("context window") + ); + } let request = guardian_requests[0]; let context = request.message_input_texts("user").join("\n"); assert!(context.contains("")); @@ -281,7 +325,9 @@ async fn review_respects_complete_context_budget( assert!(developer_context.contains("")); if matches!( reviewer_response, - ReviewerResponse::UncompactableContinuation | ReviewerResponse::NextReview + ReviewerResponse::UncompactableContinuation + | ReviewerResponse::NextReview + | ReviewerResponse::CompactionError ) { let instruction = if matches!(reviewer_response, ReviewerResponse::NextReview) { "New instructions: retry the requested command and keep all files private. " @@ -345,3 +391,183 @@ async fn review_respects_complete_context_budget( test.codex.shutdown_and_wait().await?; Ok(()) } + +#[derive(Clone, Copy)] +enum OversizedActionReview { + Fits, + UserFallback, + RequiredGuardian, +} + +#[test_case(OversizedActionReview::Fits; "large_action_receives_automatic_review")] +#[test_case(OversizedActionReview::UserFallback; "optional_review_requests_user_approval")] +#[test_case(OversizedActionReview::RequiredGuardian; "required_review_rejects_incomplete_action")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn oversized_action_preserves_review_policy_and_next_review( + review: OversizedActionReview, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!( + Ok(()), + "Guardian approval actions require host-native paths" + ); + let server = responses::start_mock_server().await; + let mut builder = test_codex() + .with_model_info_override("gpt-5.5", |model| { + model.auto_review_model_override = Some("gpt-5.6-luna".to_owned()); + }) + .with_model_info_override("gpt-5.6-luna", move |model| { + model.context_window = Some(if matches!(review, OversizedActionReview::Fits) { + 160_000 + } else { + 16_000 + }); + }) + .with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + }); + if matches!(review, OversizedActionReview::RequiredGuardian) { + builder = builder.with_model("gpt-5.5").with_cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement( + "[auto_review]\nrequired_on_models = [\"gpt-5.5\"]\n", + ), + ); + } + let test = builder.build_with_auto_env(&server).await?; + let oversized_command = + "echo ".to_owned() + &"large-action".repeat(/*n*/ 20_000) + "required suffix"; + let oversized = json!({ + "cmd": oversized_command, + "sandbox_permissions": "require_escalated", + "justification": "Run the requested command.", + }) + .to_string(); + let next = json!({ + "cmd": "echo complete-action-review", + "sandbox_permissions": "require_escalated", + "justification": "Run the requested command.", + }) + .to_string(); + let mut events = vec![ + sse(vec![ + ev_function_call("oversized", "exec_command", &oversized), + ev_completed("oversized"), + ]), + sse(vec![ + ev_function_call("next", "exec_command", &next), + ev_completed("next"), + ]), + sse(vec![ + ev_assistant_message("decision", r#"{"outcome":"allow"}"#), + ev_completed("review"), + ]), + sse(vec![ + ev_assistant_message("done", "done"), + ev_completed("done"), + ]), + ]; + if matches!(review, OversizedActionReview::Fits) { + // A real policy denial must not fall through to a user prompt. Avoid + // executing the huge command while asserting its complete review input. + events.insert( + /*index*/ 1, + sse(vec![ + ev_assistant_message( + "decision", + r#"{"outcome":"deny","rationale":"denied by classifier"}"#, + ), + ev_completed("large-review"), + ]), + ); + } + let response = responses::mount_sse_sequence(&server, events).await; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Run the commands if the approval reviewer allows them.".to_owned(), + text_elements: Vec::new(), + }])) + .await?; + let mut terminal_assessment = None; + let event = wait_for_event(&test.codex, |event| { + if let EventMsg::GuardianAssessment(assessment) = event + && assessment.status != codex_protocol::protocol::GuardianAssessmentStatus::InProgress + { + terminal_assessment = Some((assessment.status, assessment.risk_level)); + } + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + if let EventMsg::ExecApprovalRequest(approval) = event { + assert!( + matches!(review, OversizedActionReview::UserFallback), + "only exhausted optional review may ask the user" + ); + assert_eq!( + terminal_assessment, + Some(( + codex_protocol::protocol::GuardianAssessmentStatus::Aborted, + None + )) + ); + assert_eq!(approval.command.last(), Some(&oversized_command)); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::denied("rejected by user"), + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + } else { + assert!( + !matches!(review, OversizedActionReview::UserFallback), + "exhausted optional review must request user approval" + ); + } + let requests = response.requests(); + let fits = matches!(review, OversizedActionReview::Fits); + assert_eq!(requests.len(), if fits { 5 } else { 4 }); + assert!( + requests[if fits { 2 } else { 1 }] + .function_call_output("oversized") + .to_string() + .contains(match review { + OversizedActionReview::Fits => "denied by classifier", + OversizedActionReview::UserFallback => "rejected by user", + OversizedActionReview::RequiredGuardian => "reviewer input budget", + }) + ); + let reviews = requests + .iter() + .filter(|request| request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian") + .collect::>(); + assert_eq!(reviews.len(), if fits { 2 } else { 1 }); + if fits { + let parts = reviews[0].message_input_texts("user"); + assert!(parts.concat().contains(&oversized_command)); + assert!(parts.iter().all(|part| part.len() <= 36_000)); + } + let context = reviews + .last() + .expect("review of the next action") + .message_input_texts("user") + .concat(); + assert!(context.contains("echo complete-action-review")); + assert!( + requests + .last() + .expect("parent resumes after review") + .function_call_output("next") + .to_string() + .contains("complete-action-review") + ); + test.codex.shutdown_and_wait().await?; + Ok(()) +} diff --git a/codex-rs/ext/extension-api/src/contributors/approval_review.rs b/codex-rs/ext/extension-api/src/contributors/approval_review.rs index 6b3f836d88..6ba01fe241 100644 --- a/codex-rs/ext/extension-api/src/contributors/approval_review.rs +++ b/codex-rs/ext/extension-api/src/contributors/approval_review.rs @@ -63,12 +63,13 @@ pub enum ApprovalDecision { } /// Runs an extension-owned synchronous review already bound to an action by the host. -/// Implementations must not resolve approval routing or reuse an async score. +/// Implementations must not reuse an async score. `None` requests the host user +/// flow when automatic review exhausts its input budget and host policy permits it. pub trait SynchronousApprovalReviewer: Send + Sync { fn review( &self, reason: codex_protocol::approvals::GuardianReviewReason, - ) -> crate::ExtensionFuture<'_, codex_protocol::protocol::ReviewDecision>; + ) -> crate::ExtensionFuture<'_, Option>; } /// Inputs to Guardian's policy choice. Conversation and scores stay thread-owned. diff --git a/codex-rs/ext/extension-api/tests/registry.rs b/codex-rs/ext/extension-api/tests/registry.rs index b58f45384a..c27718b9a5 100644 --- a/codex-rs/ext/extension-api/tests/registry.rs +++ b/codex-rs/ext/extension-api/tests/registry.rs @@ -168,10 +168,10 @@ impl codex_extension_api::SynchronousApprovalReviewer for AllContributors { fn review( &self, _reason: codex_protocol::approvals::GuardianReviewReason, - ) -> ExtensionFuture<'_, codex_protocol::protocol::ReviewDecision> { - Box::pin(std::future::ready( + ) -> ExtensionFuture<'_, Option> { + Box::pin(std::future::ready(Some( codex_protocol::protocol::ReviewDecision::Approved, - )) + ))) } } diff --git a/codex-rs/ext/guardian-reviewer/src/completion.rs b/codex-rs/ext/guardian-reviewer/src/completion.rs index 0c1e035a24..e9ecd3d4b5 100644 --- a/codex-rs/ext/guardian-reviewer/src/completion.rs +++ b/codex-rs/ext/guardian-reviewer/src/completion.rs @@ -32,6 +32,8 @@ const TIMEOUT_INSTRUCTIONS: &str = concat!( "Do not assume the action is unsafe based on the timeout alone. ", "You may retry once, or ask the user for guidance or explicit approval.", ); +const INPUT_BUDGET_MESSAGE: &str = + "the complete action and minimum review context exceed the reviewer input budget"; pub fn guardian_timeout_message(model_info: &ModelInfo) -> String { model_info @@ -44,7 +46,8 @@ pub fn guardian_timeout_message(model_info: &ModelInfo) -> String { } pub struct ReviewCompletion { - pub decision: ReviewDecision, + /// `None` requests user approval after optional review exhausts its input budget. + pub decision: Option, pub event: GuardianAssessmentEvent, pub warning: Option, pub analytics: GuardianReviewAnalyticsResult, @@ -55,6 +58,7 @@ pub struct ReviewCompletion { pub fn complete_review( outcome: GuardianReviewOutcome, model: &ModelInfo, + require_guardian: bool, mut event: GuardianAssessmentEvent, mut analytics: GuardianReviewAnalyticsResult, ) -> ReviewCompletion { @@ -83,6 +87,20 @@ pub fn complete_review( } GuardianReviewOutcome::Error(error) => { analytics.failure_reason = Some(error.failure_reason()); + if matches!(error, GuardianReviewError::InputBudgetExceeded) && !require_guardian { + let rationale = format!("Automatic approval review failed: {INPUT_BUDGET_MESSAGE}"); + analytics.decision = GuardianReviewDecision::Aborted; + analytics.terminal_status = GuardianReviewTerminalStatus::Aborted; + event.status = GuardianAssessmentStatus::Aborted; + event.rationale = Some(rationale.clone()); + return ReviewCompletion { + decision: None, + event, + warning: Some(rationale), + analytics, + assessment_outcome: None, + }; + } match error { GuardianReviewError::Timeout => { let rationale = "Automatic approval review timed out while evaluating the requested approval.".to_string(); @@ -91,7 +109,7 @@ pub fn complete_review( event.status = GuardianAssessmentStatus::TimedOut; event.rationale = Some(rationale.clone()); return ReviewCompletion { - decision: ReviewDecision::TimedOut, + decision: Some(ReviewDecision::TimedOut), event, warning: Some(rationale), analytics, @@ -103,16 +121,26 @@ pub fn complete_review( analytics.terminal_status = GuardianReviewTerminalStatus::Aborted; event.status = GuardianAssessmentStatus::Aborted; return ReviewCompletion { - decision: ReviewDecision::Abort, + decision: Some(ReviewDecision::Abort), event, warning: None, analytics, assessment_outcome: None, }; } - GuardianReviewError::PromptBuild { message } - | GuardianReviewError::Session { message, .. } - | GuardianReviewError::Parse { message } => { + GuardianReviewError::InputBudgetExceeded + | GuardianReviewError::PromptBuild { .. } + | GuardianReviewError::Session { .. } + | GuardianReviewError::Parse { .. } => { + let message = match &error { + GuardianReviewError::InputBudgetExceeded => INPUT_BUDGET_MESSAGE, + GuardianReviewError::PromptBuild { message } + | GuardianReviewError::Session { message, .. } + | GuardianReviewError::Parse { message } => message, + GuardianReviewError::Timeout | GuardianReviewError::Cancelled => { + "guardian review failed" + } + }; analytics.decision = GuardianReviewDecision::Denied; analytics.terminal_status = GuardianReviewTerminalStatus::FailedClosed; let rationale = format!("Automatic approval review failed: {message}"); @@ -120,9 +148,9 @@ pub fn complete_review( event.status = GuardianAssessmentStatus::Denied; event.rationale = Some(rationale.clone()); return ReviewCompletion { - decision: ReviewDecision::denied(format!( + decision: Some(ReviewDecision::denied(format!( "{rationale}\n{REVIEW_FAILURE_INSTRUCTIONS}" - )), + ))), event, warning: Some(rationale), analytics, @@ -177,7 +205,7 @@ pub fn complete_review( )) }; ReviewCompletion { - decision, + decision: Some(decision), event, warning: Some(warning), analytics, diff --git a/codex-rs/ext/guardian-reviewer/src/feedback.rs b/codex-rs/ext/guardian-reviewer/src/feedback.rs index 7a9c3a20a3..7317f71fbb 100644 --- a/codex-rs/ext/guardian-reviewer/src/feedback.rs +++ b/codex-rs/ext/guardian-reviewer/src/feedback.rs @@ -74,6 +74,7 @@ impl<'a> FailedReviewFeedback<'a> { GuardianReviewSessionOutcome::Completed(Ok(None)) => ("missing_decision", None), GuardianReviewSessionOutcome::Completed(Err(_)) | GuardianReviewSessionOutcome::PromptBuildFailed(_) + | GuardianReviewSessionOutcome::InputBudgetExceeded | GuardianReviewSessionOutcome::SessionFailed { .. } => ("failed", None), GuardianReviewSessionOutcome::TimedOut => ("timed_out", None), GuardianReviewSessionOutcome::Aborted => ("aborted", None), diff --git a/codex-rs/ext/guardian-reviewer/src/outcome.rs b/codex-rs/ext/guardian-reviewer/src/outcome.rs index 03e4bc4fb9..2e53da5ca6 100644 --- a/codex-rs/ext/guardian-reviewer/src/outcome.rs +++ b/codex-rs/ext/guardian-reviewer/src/outcome.rs @@ -13,6 +13,7 @@ pub enum GuardianReviewOutcome { #[derive(Debug)] pub enum GuardianReviewError { + InputBudgetExceeded, PromptBuild { message: String, }, @@ -61,7 +62,9 @@ impl GuardianReviewError { pub fn failure_reason(&self) -> GuardianReviewFailureReason { match self { Self::PromptBuild { .. } => GuardianReviewFailureReason::PromptBuildError, - Self::Session { .. } => GuardianReviewFailureReason::SessionError, + Self::Session { .. } | Self::InputBudgetExceeded => { + GuardianReviewFailureReason::SessionError + } Self::Parse { .. } => GuardianReviewFailureReason::ParseError, Self::Timeout => GuardianReviewFailureReason::Timeout, Self::Cancelled => GuardianReviewFailureReason::Cancelled, @@ -73,6 +76,7 @@ impl GuardianReviewError { pub enum GuardianReviewSessionOutcome { Completed(anyhow::Result>), PromptBuildFailed(anyhow::Error), + InputBudgetExceeded, SessionFailed { error: anyhow::Error, error_info: Option, @@ -102,6 +106,9 @@ impl From for GuardianReviewOutcome { GuardianReviewSessionOutcome::PromptBuildFailed(error) => { Self::Error(GuardianReviewError::prompt_build(error)) } + GuardianReviewSessionOutcome::InputBudgetExceeded => { + Self::Error(GuardianReviewError::InputBudgetExceeded) + } GuardianReviewSessionOutcome::SessionFailed { error, error_info, diff --git a/codex-rs/ext/guardian-reviewer/src/reporting.rs b/codex-rs/ext/guardian-reviewer/src/reporting.rs index c2e2815e87..bcc974cd47 100644 --- a/codex-rs/ext/guardian-reviewer/src/reporting.rs +++ b/codex-rs/ext/guardian-reviewer/src/reporting.rs @@ -92,13 +92,14 @@ impl ReviewReport { &self, outcome: GuardianReviewOutcome, model: &ModelInfo, + require_guardian: bool, analytics: GuardianReviewAnalyticsResult, completed_at_ms: i64, ) -> ReviewCompletion { let mut event = self.started.clone(); event.completed_at_ms = Some(completed_at_ms); event.decision_source = Some(GuardianAssessmentDecisionSource::Agent); - crate::complete_review(outcome, model, event, analytics) + crate::complete_review(outcome, model, require_guardian, event, analytics) } pub fn track( diff --git a/codex-rs/ext/guardian-reviewer/src/retry.rs b/codex-rs/ext/guardian-reviewer/src/retry.rs index 49dda5d213..79d516215b 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry.rs @@ -110,7 +110,8 @@ fn should_retry_guardian_review(outcome: &GuardianReviewOutcome) -> bool { }, GuardianReviewOutcome::Completed(_) | GuardianReviewOutcome::Error( - GuardianReviewError::PromptBuild { .. } + GuardianReviewError::InputBudgetExceeded + | GuardianReviewError::PromptBuild { .. } | GuardianReviewError::Session { error_info: None, .. } diff --git a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs index d017e3d654..ce335db96f 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs @@ -71,6 +71,10 @@ fn guardian_review_retry_only_retries_transient_session_and_parse_errors() { .collect::>(); outcomes.extend([ (GuardianReviewOutcome::Completed(assessment), false), + ( + GuardianReviewOutcome::Error(GuardianReviewError::InputBudgetExceeded), + false, + ), ( GuardianReviewOutcome::Error(GuardianReviewError::prompt_build(anyhow::anyhow!( "prompt" diff --git a/codex-rs/ext/guardian-reviewer/src/review.rs b/codex-rs/ext/guardian-reviewer/src/review.rs index cdb49634fa..bf0704ad15 100644 --- a/codex-rs/ext/guardian-reviewer/src/review.rs +++ b/codex-rs/ext/guardian-reviewer/src/review.rs @@ -17,6 +17,7 @@ use crate::GuardianReviewSessionLimits; /// Runtime operations bound to one immutable approval action and its issuing context. /// Preparation captures trusted evidence. Completion must invalidate stale approvals /// and may only satisfy the review gate, never expand the action's execution authority. +/// Completion may return `None` for user approval only when host policy permits it. pub trait ReviewHost: Send + Sync { type Prepared: Send + Sync; @@ -36,7 +37,7 @@ pub trait ReviewHost: Send + Sync { prepared: Self::Prepared, outcome: GuardianReviewOutcome, analytics: GuardianReviewAnalyticsResult, - ) -> impl Future + Send; + ) -> impl Future> + Send; } /// One review bound by the host before Guardian's approval policy chooses to run it. @@ -51,12 +52,12 @@ impl SynchronousReview { } impl SynchronousApprovalReviewer for SynchronousReview { - fn review(&self, reason: GuardianReviewReason) -> ExtensionFuture<'_, ReviewDecision> { + fn review(&self, reason: GuardianReviewReason) -> ExtensionFuture<'_, Option> { Box::pin(async move { let deadline = Instant::now() + crate::REVIEW_TIMEOUT; let prepared = match self.host.prepare(reason, deadline).await { Ok(prepared) => prepared, - Err(decision) => return decision, + Err(decision) => return Some(decision), }; let (outcome, analytics) = Box::pin(crate::run_with_retry( GuardianReviewSessionLimits { diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/action.rs b/codex-rs/ext/guardian-v2/src/async_scorer/action.rs index eea41dfe0c..c373ea1ae8 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/action.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/action.rs @@ -1,25 +1,26 @@ -//! Renders bounded planned-action JSON while preserving tool and call identity. -//! String truncation precedes omission of optional fields. +//! Renders complete planned-action JSON or rejects it for synchronous review. +//! Action arguments cannot be shortened to fit the asynchronous classifier budget. use codex_extension_api::ToolName; use codex_extension_api::ToolPayload; use codex_protocol::protocol::TruncationPolicy; use serde_json::json; -use super::transcript::truncate_entry; - pub(super) struct GuardianAction { pub(super) tool_name: ToolName, pub(super) payload: ToolPayload, } -pub(super) struct RenderedAction { - pub(super) text: String, - pub(super) original_bytes: usize, +#[derive(Debug, thiserror::Error)] +pub(super) enum ActionRenderError { + #[error("Guardian action exceeds the {max_tokens}-token review limit")] + TooLarge { max_tokens: usize }, + #[error(transparent)] + Serialization(#[from] serde_json::Error), } impl GuardianAction { - pub(super) fn render(self, max_action_tokens: usize) -> serde_json::Result { + pub(super) fn render(self, max_action_tokens: usize) -> Result { let arguments = match self.payload { ToolPayload::Function { arguments } => { serde_json::from_str(&arguments).unwrap_or(serde_json::Value::String(arguments)) @@ -42,129 +43,16 @@ impl GuardianAction { .for_each(serde_json::Value::sort_all_objects); let max_action_bytes = TruncationPolicy::Tokens(max_action_tokens).byte_budget(); let rendered = serde_json::to_string_pretty(&action)?; - let original_bytes = rendered.len(); if rendered.len().saturating_add(1) <= max_action_bytes { - return Ok(RenderedAction { - text: rendered, - original_bytes, - }); + return Ok(rendered); } - if let Some(rendered) = fit_action_to_budget(&action, max_action_bytes, max_action_tokens)? - { - return Ok(RenderedAction { - text: rendered, - original_bytes, - }); - } - - let mut omission_key = "_guardian_omitted_fields".to_owned(); - while action.contains_key(&omission_key) { - omission_key.push('_'); - } - let mut retained = serde_json::Map::new(); - for key in ["tool", "call_id"] { - if let Some(value) = action.get(key) { - retained.insert(key.to_owned(), value.clone()); - } - } - let mut omitted = action.len().saturating_sub(retained.len()); - retained.insert(omission_key.clone(), json!(omitted)); - - let mut optional_fields = action - .iter() - .filter(|(key, _)| !matches!(key.as_str(), "tool" | "call_id")) - .collect::>(); - optional_fields.sort_by_key(|(key, _)| { - !matches!( - key.as_str(), - "arguments" | "cmd" | "command" | "input" | "patch" | "path" | "url" - ) - }); - for (key, value) in optional_fields { - let mut candidate = retained.clone(); - candidate.insert(key.clone(), value.clone()); - candidate.insert(omission_key.clone(), json!(omitted.saturating_sub(1))); - candidate.sort_keys(); - let minimized = render_action_with_limit(&candidate, /*max_tokens*/ 0)?; - if minimized.len().saturating_add(1) <= max_action_bytes { - retained = candidate; - omitted = omitted.saturating_sub(1); - } - } - - retained.sort_keys(); - let rendered = fit_action_to_budget(&retained, max_action_bytes, max_action_tokens)? - .ok_or_else(|| { - serde_json::Error::io(std::io::Error::other(format!( - "Guardian action identity exceeds the {max_action_tokens}-token limit" - ))) - })?; - Ok(RenderedAction { - text: rendered, - original_bytes, + Err(ActionRenderError::TooLarge { + max_tokens: max_action_tokens, }) } } -fn fit_action_to_budget( - action: &serde_json::Map, - max_action_bytes: usize, - max_action_tokens: usize, -) -> serde_json::Result> { - let mut low = 0usize; - let mut high = max_action_tokens.saturating_add(1); - let mut best = None; - - while low < high { - let max_tokens = low + (high - low) / 2; - let rendered = render_action_with_limit(action, max_tokens)?; - if rendered.len().saturating_add(1) <= max_action_bytes { - best = Some(rendered); - low = max_tokens.saturating_add(1); - } else { - high = max_tokens; - } - } - - Ok(best) -} - -fn render_action_with_limit( - action: &serde_json::Map, - max_tokens: usize, -) -> serde_json::Result { - let mut truncated = action.clone(); - for (key, value) in &mut truncated { - if !matches!(key.as_str(), "tool" | "call_id") { - truncate_action_value(value, max_tokens); - } - } - serde_json::to_string_pretty(&truncated) -} - -fn truncate_action_value(value: &mut serde_json::Value, max_tokens: usize) { - match value { - serde_json::Value::String(text) => { - let truncated = truncate_entry(text, max_tokens); - if truncated.len() < text.len() { - *text = truncated; - } - } - serde_json::Value::Array(values) => { - for value in values { - truncate_action_value(value, max_tokens); - } - } - serde_json::Value::Object(values) => { - for value in values.values_mut() { - truncate_action_value(value, max_tokens); - } - } - serde_json::Value::Null | serde_json::Value::Bool(_) | serde_json::Value::Number(_) => {} - } -} - #[cfg(test)] #[path = "action_tests.rs"] mod tests; diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/action_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/action_tests.rs index cfa52a59e2..d49e6d3baf 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/action_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/action_tests.rs @@ -1,96 +1,37 @@ -//! Tests for bounded action rendering and field retention. +//! Complete action rendering preserves nested arguments and rejects overflow. +use super::*; use anyhow::Result; -use codex_extension_api::ToolName; -use codex_extension_api::ToolPayload; -use codex_protocol::protocol::TruncationPolicy; use pretty_assertions::assert_eq; -use serde_json::json; - -use super::GuardianAction; -use crate::async_scorer::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; +use serde_json::Value; #[test] -fn guardian_action_bounds_structurally_oversized_arrays() -> Result<()> { - let action = GuardianAction { - tool_name: ToolName::plain("inspect_values"), - payload: ToolPayload::Function { - arguments: json!({ - "call_id": "genuine-call", - "tool": "spoofed-tool", - "values": (0..6_000).collect::>(), - }) - .to_string(), - }, +fn guardian_action_preserves_complete_arguments_at_budget_boundary() -> Result<()> { + let arguments = json!({ + "call_id": "genuine-call", + "tool": "untrusted-tool", + "attachments": [{"content": "🦀\"\\\n".repeat(/*n*/ 40) + "required suffix"}], + "metadata": {"description": "required argument", "values": [1, false, null]}, + }); + let render = |max_action_tokens| { + GuardianAction { + tool_name: ToolName::plain("inspect_values"), + payload: ToolPayload::Function { + arguments: arguments.to_string(), + }, + } + .render(max_action_tokens) }; - - let rendered = action.render(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS)?.text; - assert!( - rendered.len().saturating_add(1) - <= TruncationPolicy::Tokens(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS).byte_budget() - ); - let action = serde_json::from_str::(&rendered)?; - assert_eq!( - action, - json!({ - "_guardian_omitted_fields": 1, - "call_id": "genuine-call", - "tool": "inspect_values", - }) - ); - - Ok(()) -} - -#[test] -fn guardian_action_bounds_structurally_oversized_object_keys() -> Result<()> { - let oversized_key = "oversized_key_".to_owned() - + &"k".repeat(TruncationPolicy::Tokens(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS).byte_budget()); - let mut arguments = serde_json::Map::from_iter([ - ( - "_guardian_omitted_fields".to_owned(), - json!("actual-tool-argument"), - ), - ("call_id".to_owned(), json!("genuine-call")), - ("cmd".to_owned(), json!("remove-important-file")), - ("tool".to_owned(), json!("spoofed-tool")), - (oversized_key.clone(), json!(true)), - ]); - for index in 0..600 { - arguments.insert(format!("a_{index:04}_{}", "k".repeat(64)), json!(index)); - } - let original_field_count = arguments.len(); - let action = GuardianAction { - tool_name: ToolName::plain("inspect_fields"), - payload: ToolPayload::Function { - arguments: serde_json::Value::Object(arguments).to_string(), - }, - }; - - let rendered = action.render(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS)?.text; - assert!( - rendered.len().saturating_add(1) - <= TruncationPolicy::Tokens(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS).byte_budget() - ); - let action = serde_json::from_str::(&rendered)?; - let fields = action - .as_object() - .expect("the bounded action must remain a JSON object"); - assert_eq!(fields.get("tool"), Some(&json!("inspect_fields"))); - assert_eq!(fields.get("call_id"), Some(&json!("genuine-call"))); - assert_eq!(fields.get("cmd"), Some(&json!("remove-important-file"))); - assert_eq!( - fields.get("_guardian_omitted_fields"), - Some(&json!("actual-tool-argument")) - ); - assert!(fields.len() < original_field_count); - assert!(!fields.contains_key(&oversized_key)); - assert!( - fields - .get("_guardian_omitted_fields_") - .and_then(serde_json::Value::as_u64) - .is_some_and(|omitted| omitted > 0) - ); - + let mut expected = arguments.clone(); + expected["tool"] = json!("inspect_values"); + let bytes = serde_json::to_string_pretty(&expected)?.len(); + let budget = TruncationPolicy::Bytes(bytes.saturating_add(1)).token_budget(); + let rendered = render(budget)?; + assert_eq!(serde_json::from_str::(&rendered)?, expected); + assert_eq!(rendered.len(), bytes); + assert!(matches!( + render(budget - 1), + Err(ActionRenderError::TooLarge { .. }) + )); Ok(()) } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs index 9f7ebb085f..b6440cb17d 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs @@ -22,6 +22,7 @@ use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::openai_models::GuardianReviewMode; use codex_protocol::openai_models::GuardianScope; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::TruncationPolicy; use codex_protocol::security_risk::SecurityRiskScore; use std::sync::Weak; use std::sync::atomic::Ordering; @@ -116,7 +117,10 @@ impl GuardianApprovalReviewer { ?reason, "reviewing approval" ); - ApprovalDecision::Reviewed(input.synchronous_reviewer.review(reason).await) + match input.synchronous_reviewer.review(reason).await { + Some(decision) => ApprovalDecision::Reviewed(decision), + None => ApprovalDecision::AskUser, + } } } @@ -132,6 +136,30 @@ async fn cached_evidence( record_fast_decision(metrics, "deferred", "missing_score"); return Err(GuardianReviewReason::MissingScore); }; + // Elicitations and intercepted execs can expand beyond the original scored action. + let mut action = input.action.clone(); + if action.get("tool").and_then(serde_json::Value::as_str) == Some("mcp_tool_call") + && let Some(fields) = action.as_object_mut() + { + // Match the action renderer: host descriptions are optional, arguments are not. + fields.remove("tool_description"); + fields.remove("connector_description"); + } + let max_action_bytes = TruncationPolicy::Tokens(config.max_action_tokens).byte_budget(); + let action_fits = serde_json::to_string_pretty(&action) + .is_ok_and(|action| action.len().saturating_add(1) <= max_action_bytes); + if !action_fits + || input.tool_call_id.is_some_and(|call_id| { + progress + .oversized_tool_calls + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .contains(call_id) + }) + { + record_fast_decision(metrics, "deferred", "scoring_failure"); + return Err(GuardianReviewReason::ScoringFailure); + } let context_mode = store .get_or_init(GuardianReviewEvidence::default) .context_mode(); 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 a69759ac04..0b98fd0029 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -1,6 +1,7 @@ use super::transcript::ContextInput; use codex_core::context::ContextualUserFragment; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::sync::Arc; use std::sync::Mutex; use std::sync::Weak; @@ -28,6 +29,7 @@ use codex_extension_api::SkillInvocationContributor; use codex_extension_api::SkillInvocationInput; use codex_extension_api::ThreadLifecycleContributor; use codex_extension_api::ThreadStartInput; +use codex_extension_api::ToolFinishInput; use codex_extension_api::ToolLifecycleContributor; use codex_extension_api::ToolLifecycleFuture; use codex_extension_api::ToolStartInput; @@ -46,8 +48,8 @@ use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::has_full_access; use codex_protocol::security_risk::SecurityRiskScore; +use super::action::ActionRenderError; use super::action::GuardianAction; -use super::action::RenderedAction; use super::authorization::ScoreAuthorization; use super::config::GuardianV2Config; use super::coverage::UnscoredAction; @@ -82,6 +84,9 @@ pub(super) struct GuardianV2ScoreProgress { pub(super) js_executions: AtomicUsize, pub(super) latest_scored_tool_call: AtomicUsize, pub(super) latest_failed_tool_call: AtomicUsize, + // Keep overflow attached to each active call even after a newer score succeeds. + // The host's finish callback removes entries on completion, failure, or cancellation. + pub(super) oversized_tool_calls: Mutex>, // Serialize successful score publication with its authorization metadata. pub(super) authorization: Mutex>, metrics: Option>, @@ -210,6 +215,18 @@ impl ToolLifecycleContributor for GuardianV2Extension { fn on_tool_start<'a>(&'a self, input: ToolStartInput<'a>) -> ToolLifecycleFuture<'a> { Box::pin(self.score_tool(input)) } + + fn on_tool_finish<'a>(&'a self, input: ToolFinishInput<'a>) -> ToolLifecycleFuture<'a> { + Box::pin(async move { + if let Some(progress) = input.thread_store.get::() { + progress + .oversized_tool_calls + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .remove(input.call_id); + } + }) + } } impl GuardianV2Extension { @@ -427,6 +444,45 @@ impl GuardianV2Extension { tool_name: input.tool_name.clone(), payload: input.payload.clone(), }; + let planned_action = match action.render(guardian_config.max_action_tokens) { + Ok(text) => text, + Err(ActionRenderError::TooLarge { .. }) => { + score_progress + .oversized_tool_calls + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(input.call_id.to_owned()); + score_progress + .latest_failed_tool_call + .fetch_max(tool_call_index, Ordering::Release); + Self::record_fail_closed_score(input.thread_store, sampled_at); + record_classification( + metrics.as_deref(), + classification_started_at.elapsed(), + "failure", + Some("input_too_large"), + ); + return; + } + Err(error) => { + score_progress + .latest_failed_tool_call + .fetch_max(tool_call_index, Ordering::Release); + Self::record_fail_closed_score(input.thread_store, sampled_at); + record_classification( + metrics.as_deref(), + classification_started_at.elapsed(), + "failure", + Some("action_serialization_error"), + ); + event_sink.emit_warning(ExtensionWarning { + thread_id, + turn_id: Some(turn_id), + message: format!("Guardian V2 action serialization failed: {error}"), + }); + return; + } + }; let review_model_override = parent_model .as_ref() .and_then(|model| model.auto_review_model_override.clone()); @@ -489,30 +545,6 @@ impl GuardianV2Extension { model: parent_model.clone(), ..score_authorization }; - let planned_action = match action.render(guardian_config.max_action_tokens) { - Ok(RenderedAction { - text, - original_bytes, - }) => { - truncations.record("action", original_bytes, text.len()); - text - } - Err(error) => { - Self::record_fail_closed_score(thread.thread_extension_data(), sampled_at); - record_classification( - metrics.as_deref(), - classification_started_at.elapsed(), - "failure", - Some("action_serialization_error"), - ); - event_sink.emit_warning(ExtensionWarning { - thread_id, - turn_id: Some(turn_id), - message: format!("Guardian V2 action serialization failed: {error}"), - }); - return; - } - }; let action_section = PlannedAction { json: planned_action.clone(), tool_descriptions: None, 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 2d462841d5..c6c05509ec 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 @@ -71,7 +71,6 @@ use super::GuardianV2Extension; use super::GuardianV2ScoreProgress; use crate::async_scorer::config::CLASSIFICATION_OUTPUT_INSTRUCTIONS; -use crate::async_scorer::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; use crate::async_scorer::config::DEFAULT_PARENT_COMPACTION_TOKENS; use crate::async_scorer::config::GuardianV2Config; use crate::async_scorer::coverage::GuardianPolicy; @@ -90,8 +89,6 @@ use crate::async_scorer::sampler::tests::proxy_websocket_servers_with_http; use crate::async_scorer::transcript::MAX_MESSAGE_ENTRY_TOKENS; use crate::async_scorer::transcript::MAX_TOOL_ENTRY_TOKENS; use crate::async_scorer::transcript::truncate_entry; -use crate::async_scorer::truncation::CLASSIFICATION_TRUNCATION_BYTES_METRIC; -use crate::async_scorer::truncation::CLASSIFICATION_TRUNCATION_METRIC; use codex_features::GuardianV2ReviewScopeConfigToml; const TEST_GUARDIAN_POLICY: &str = @@ -1204,19 +1201,6 @@ async fn contributor_fails_closed_when_model_configuration_is_invalid() -> Resul fixture.assert_fails_closed("elevated_risk").await } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn contributor_fails_closed_when_action_serialization_fails() -> Result<()> { - skip_if_no_network!(Ok(())); - - let fixture = GuardianFailureFixture::new().await?; - let oversized_tool_name = ToolName::plain( - "x".repeat(TruncationPolicy::Tokens(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS).byte_budget()), - ); - - fixture.score_tool(oversized_tool_name).await; - fixture.assert_fails_closed("elevated_risk").await -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn contributor_fails_closed_when_luna_classification_fails() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1388,7 +1372,7 @@ max_recent_non_user_entries = 8 internal_chat_message_metadata_passthrough: None, }, ]; - let arguments = json!({"body": "x".repeat(4_000)}).to_string(); + let arguments = json!({"body": "x".repeat(/*n*/ 128)}).to_string(); let (request, test, registry) = sample_configured_conversation_history( conversation_history, &arguments, @@ -1433,16 +1417,11 @@ max_recent_non_user_entries = 8 .as_str() .expect("planned action should be a text item"); assert!(action.len() <= TruncationPolicy::Tokens(/*limit*/ 128).byte_budget()); - let original_action_bytes = i64::try_from( - serde_json::to_string_pretty(&json!({ - "body": "x".repeat(4_000), - "tool": "read_file", - }))? - .len(), - )?; let action: serde_json::Value = serde_json::from_str(action)?; - let retained_action_bytes = i64::try_from(serde_json::to_string_pretty(&action)?.len())?; - assert_eq!(action["tool"], "read_file"); + assert_eq!( + action, + json!({"body": "x".repeat(/*n*/ 128), "tool": "read_file"}) + ); let thread_store = test.codex.thread_extension_data(); let score_progress = thread_store @@ -1454,7 +1433,7 @@ max_recent_non_user_entries = 8 .latest_scored_tool_call .load(Ordering::Acquire) == 0 - || metrics.classification_samples().len() < 14 + || metrics.classification_samples().len() < 10 { tokio::task::yield_now().await; } @@ -1507,6 +1486,33 @@ max_recent_non_user_entries = 8 Some(ReviewDecision::Approved) ); + // A small scored call does not cover expanded elicitation arguments or an + // intercepted exec with no tool-call ID. Both must still fit the action budget. + for (argument, expected) in [ + ("small".to_owned(), Some(ReviewDecision::Approved)), + ("expanded argument ".repeat(/*n*/ 128), None), + ] { + for action in [ + json!({ + "tool": "mcp_tool_call", "server": "example", "id": "call-1", + "arguments": {"body": argument}, + "tool_description": "optional metadata ".repeat(/*n*/ 128), + }), + json!({"tool": "exec_command", "program": "example", "argv": [argument]}), + ] { + assert_eq!( + cached_approval( + ®istry, + thread_store, + &action.to_string(), + /*metrics*/ None + ) + .await, + expected + ); + } + } + assert_eq!( score_progress .latest_scored_tool_call @@ -1607,34 +1613,7 @@ max_recent_non_user_entries = 8 classification_duration_ms, vec![("outcome".to_owned(), "success".to_owned())], ), - RecordedMetric::Counter( - CLASSIFICATION_TRUNCATION_METRIC.to_owned(), - 1, - vec![ - ("component".to_owned(), "action".to_owned()), - ("disposition".to_owned(), "truncated".to_owned()), - ], - ), ]) - .chain( - [ - ("original", original_action_bytes), - ("retained", retained_action_bytes), - ("omitted", original_action_bytes - retained_action_bytes), - ] - .into_iter() - .map(|(measurement, bytes)| { - RecordedMetric::Histogram( - CLASSIFICATION_TRUNCATION_BYTES_METRIC.to_owned(), - bytes, - vec![ - ("component".to_owned(), "action".to_owned()), - ("disposition".to_owned(), "truncated".to_owned()), - ("measurement".to_owned(), measurement.to_owned()), - ], - ) - }), - ) .chain([ RecordedMetric::Histogram(TOOL_CALL_LAG_METRIC.to_owned(), 0, vec![]), fast_decision_metric("deferred", "elevated_risk"), @@ -1787,7 +1766,7 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu internal_chat_message_metadata_passthrough: None, }, ]; - let arguments = json!({"body": "x".repeat(4_000)}).to_string(); + let arguments = json!({"body": "x".repeat(/*n*/ 128)}).to_string(); let local_config = "[features.guardianv2]\nenabled = true\nreview_threshold = 0.70\n"; let (request, test, registry) = sample_configured_conversation_history( conversation_history, @@ -3212,78 +3191,13 @@ async fn legacy_contributor_can_disable_parent_compaction_reuse() -> Result<()> Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn contributor_bounds_oversized_actions_and_fairly_truncates_nested_fields() -> Result<()> { - skip_if_no_network!(Ok(())); - - let arguments = json!({ - "attachments": [{ - "content": "🦀\"\\\n".repeat(20_000), - "name": "financials.csv", - }], - "call_id": "untrusted-call", - "metadata": { "reason": "b".repeat(100_000) }, - "path": "a".repeat(100_000), - "recipient": "finance@example.com", - "tool": "untrusted-tool", - }) - .to_string(); - let (request, _test, _registry) = - sample_conversation_history(Vec::new(), &arguments, Some(TEST_GUARDIAN_POLICY)).await?; - let content = request["input"][2]["content"] - .as_array() - .expect("Luna user content should contain separate text items"); - let action_text = content[content.len() - 2]["text"] - .as_str() - .expect("the current action should be an input text item"); - let action = serde_json::from_str::(action_text)?; - let max_action_bytes = - TruncationPolicy::Tokens(DEFAULT_MODEL_CONTEXT_ITEM_TOKENS).byte_budget(); - assert!(action_text.ends_with('\n')); - assert!( - action_text.len() <= max_action_bytes, - "the complete model-visible action must remain bounded" - ); - assert!( - action_text.len() >= max_action_bytes * 9 / 10, - "water-filling should use the available action budget" - ); - assert_eq!(action["tool"], "read_file"); - assert_eq!(action["call_id"], "untrusted-call"); - assert_eq!(action["recipient"], "finance@example.com"); - assert_eq!(action["attachments"][0]["name"], "financials.csv"); - assert!(action.get("arguments_preview").is_none()); - assert!(action.get("truncated").is_none()); - let retained_values = [ - &action["path"], - &action["metadata"]["reason"], - &action["attachments"][0]["content"], - ] - .map(|value| { - value - .as_str() - .expect("action string field should remain present") - }); - for text in retained_values { - assert!(text.contains(" codex_extension_api::ExtensionFuture<'_, ReviewDecision> { - Box::pin(async { ReviewDecision::denied("cache miss") }) + ) -> codex_extension_api::ExtensionFuture<'_, Option> { + Box::pin(async { Some(ReviewDecision::denied("cache miss")) }) } } diff --git a/codex-rs/guardian-context/src/action.rs b/codex-rs/guardian-context/src/action.rs index 9881fbdb95..8b2022cc28 100644 --- a/codex-rs/guardian-context/src/action.rs +++ b/codex-rs/guardian-context/src/action.rs @@ -1,6 +1,6 @@ //! Planned-action prompt framing shared by the two production reviewers. -//! Hosts retain their action serialization and truncation policies; this module -//! only renders already-bounded JSON and reasons, preserving content-item boundaries. +//! Hosts serialize complete actions; whole-request admission bounds the input. +//! This module frames JSON and reasons before composition splits long text for transport. use crate::ContextSection; use crate::SectionContributor; @@ -8,7 +8,7 @@ use crate::SectionError; use crate::SectionInput; use crate::SectionScope; -/// Host-prepared action evidence. JSON and optional reason must already be bounded. +/// Host-prepared action evidence. JSON stays complete through request admission. #[derive(Clone, PartialEq)] pub struct PlannedAction { pub json: String, diff --git a/codex-rs/guardian-context/src/budget.rs b/codex-rs/guardian-context/src/budget.rs index 9a9d0d79a9..7f172a9c01 100644 --- a/codex-rs/guardian-context/src/budget.rs +++ b/codex-rs/guardian-context/src/budget.rs @@ -168,7 +168,21 @@ pub fn estimate_input_tokens(item: &ResponseItem) -> usize { } pub(super) fn content_tokens(item: &ContentItem) -> usize { - let bytes = ByteCount::measure(|counter| serde_json::to_writer(counter, item)); + let mut bytes = ByteCount::measure(|counter| serde_json::to_writer(counter, item)); + if let ContentItem::InputText { text } = item { + let extra_parts = crate::composition::bounded_text_parts(text) + .count() + .saturating_sub(1); + let framing = ByteCount::measure(|counter| { + serde_json::to_writer( + counter, + &ContentItem::InputText { + text: String::new(), + }, + ) + }); + bytes = bytes.saturating_add(extra_parts.saturating_mul(framing.saturating_add(1))); + } adjusted_tokens(bytes, std::slice::from_ref(item)) } diff --git a/codex-rs/guardian-context/src/composition.rs b/codex-rs/guardian-context/src/composition.rs index fe1a5d437b..04aa6bae56 100644 --- a/codex-rs/guardian-context/src/composition.rs +++ b/codex-rs/guardian-context/src/composition.rs @@ -2,10 +2,12 @@ //! Profiles retain the host-selected transcript slice; composition owns //! framing, message boundaries and section placement, without retaining history. //! Each content item keeps its selection policy until transport conversion. +//! Long text splits losslessly only after admission, preserving whole-entry selection. use codex_context_fragments::ContextualUserFragment; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; +use codex_protocol::protocol::TruncationPolicy; use codex_protocol::user_input::UserInput; use crate::ActionPresentation; @@ -272,10 +274,13 @@ impl ComposedContext { }; for item in content { inputs.push(match item.content { - ContentItem::InputText { text } => UserInput::Text { - text, - text_elements: Vec::new(), - }, + ContentItem::InputText { text } => { + inputs.extend(bounded_text_parts(&text).map(|part| UserInput::Text { + text: part.to_owned(), + text_elements: Vec::new(), + })); + continue; + } ContentItem::InputImage { image_url, detail } => { UserInput::Image { image_url, detail } } @@ -297,7 +302,18 @@ impl ComposedContext { for section in self.sections { match section.delivery { SectionDelivery::UserContent(content) => { - user_content.extend(content.into_iter().map(|item| item.content)) + for item in content { + match item.content { + ContentItem::InputText { text } => { + user_content.extend(bounded_text_parts(&text).map(|part| { + ContentItem::InputText { + text: part.to_owned(), + } + })); + } + content => user_content.push(content), + } + } } SectionDelivery::Message(message) => { if !user_content.is_empty() { @@ -314,6 +330,20 @@ impl ComposedContext { } } +/// Bounds individual text parts without changing source text or entry identity. +/// Budget estimates include the extra wire framing before transport conversion. +pub(super) fn bounded_text_parts(text: &str) -> impl Iterator { + let mut remaining = Some(text); + std::iter::from_fn(move || { + let text = remaining.take()?; + let end = text.floor_char_boundary(TruncationPolicy::Tokens(9_000).byte_budget()); + if end < text.len() { + remaining = Some(&text[end..]); + } + Some(&text[..end]) + }) +} + pub(super) fn user_message(content: Vec) -> ResponseItem { ResponseItem::Message { id: None, diff --git a/codex-rs/guardian-context/src/composition_tests.rs b/codex-rs/guardian-context/src/composition_tests.rs index 4db9d5a3ca..4095f00748 100644 --- a/codex-rs/guardian-context/src/composition_tests.rs +++ b/codex-rs/guardian-context/src/composition_tests.rs @@ -54,3 +54,44 @@ fn delivery_preserves_arbitrary_message_boundaries_and_rejects_them_for_sync() { }) ); } + +#[test] +fn long_text_delivery_is_lossless_bounded_and_fully_budgeted() { + let text = "é🙂\"\n".repeat(/*n*/ 20_000); + let context = ComposedContext { + sections: vec![SectionOutput { + id: "planned_action", + delivery: text_content(vec![text.clone()]), + }], + truncations: Vec::new(), + }; + let estimated = context.estimated_tokens(); + let messages = context.clone().into_messages(); + let ResponseItem::Message { content, .. } = &messages[0] else { + panic!("user message") + }; + let parts = content + .iter() + .map(|item| match item { + ContentItem::InputText { text } => text.as_str(), + _ => panic!("text part"), + }) + .collect::>(); + assert_eq!(parts.concat(), text); + assert!( + parts + .iter() + .all(|part| part.len() <= TruncationPolicy::Tokens(9_000).byte_budget()) + ); + assert!(estimated >= crate::estimate_input_tokens(&messages[0])); + assert_eq!( + context.into_user_inputs().unwrap(), + parts + .into_iter() + .map(|part| UserInput::Text { + text: part.to_owned(), + text_elements: Vec::new(), + }) + .collect::>() + ); +}