Use model-specific auto-review outcome instructions (#39741)

## What changed

- Add `rejection_instructions` and `timeout_instructions` to catalog-provided auto-review messages.
- Use the acting model's instructions for denied and timed-out reviews across tool approvals, shell escalation, and MCP elicitation responses.
- Fall back to the existing instructions only when a catalog value is absent, while preserving explicit empty-string overrides.

## Testing

- Cover catalog overrides, legacy fallbacks, empty values, and separation between acting-model and reviewer-model messages.

GitOrigin-RevId: c5b2c2dbdaefd45d1d658651dd1abaeb6d8c93da
This commit is contained in:
rhan-oai
2026-08-20 16:52:16 +00:00
committed by copyberry
parent 88da5520d4
commit 4f38432d87
12 changed files with 292 additions and 33 deletions
+17 -3
View File
@@ -10,6 +10,7 @@ use codex_extension_api::ThreadIdleCause;
use codex_features::Feature;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER;
use codex_protocol::openai_models::ModelInfo;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::CodexErrorInfo;
use codex_protocol::protocol::EventMsg;
@@ -101,8 +102,14 @@ pub(crate) fn new_guardian_review_id() -> String {
uuid::Uuid::new_v4().to_string()
}
pub(crate) fn guardian_timeout_message() -> String {
GUARDIAN_TIMEOUT_INSTRUCTIONS.to_string()
pub(crate) fn guardian_timeout_message(model_info: &ModelInfo) -> String {
model_info
.model_messages
.as_ref()
.and_then(|messages| messages.auto_review.as_ref())
.and_then(|messages| messages.timeout_instructions.as_deref())
.unwrap_or(GUARDIAN_TIMEOUT_INSTRUCTIONS)
.to_string()
}
#[derive(Debug)]
@@ -665,8 +672,15 @@ async fn run_guardian_review(
} else {
assessment.rationale.trim()
};
let rejection_instructions = turn
.model_info
.model_messages
.as_ref()
.and_then(|messages| messages.auto_review.as_ref())
.and_then(|messages| messages.rejection_instructions.as_deref())
.unwrap_or(GUARDIAN_REJECTION_INSTRUCTIONS);
ReviewDecision::denied(format!(
"This action was rejected due to unacceptable risk.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}"
"This action was rejected due to unacceptable risk.\nReason: {rationale}\n{rejection_instructions}"
))
}
}
@@ -1912,6 +1912,8 @@ mod tests {
auto_review: Some(AutoReviewMessages {
policy: Some("Use the catalog Guardian policy.".to_string()),
policy_template: Some(catalog_template.to_string()),
rejection_instructions: None,
timeout_instructions: None,
}),
permissions: None,
multi_agent: None,
@@ -1948,6 +1950,8 @@ mod tests {
auto_review: Some(AutoReviewMessages {
policy: Some(String::new()),
policy_template: None,
rejection_instructions: None,
timeout_instructions: None,
}),
permissions: None,
multi_agent: None,
@@ -1992,6 +1996,8 @@ mod tests {
auto_review: Some(AutoReviewMessages {
policy: Some(catalog_policy.to_string()),
policy_template: Some(String::new()),
rejection_instructions: None,
timeout_instructions: None,
}),
permissions: None,
multi_agent: None,
+19 -1
View File
@@ -1427,10 +1427,28 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
#[test]
fn guardian_timeout_message_distinguishes_timeout_from_policy_denial() {
let message = guardian_timeout_message();
let mut model = codex_models_manager::model_info::model_info_from_slug("acting-model");
model.model_messages = None;
let message = guardian_timeout_message(&model);
assert!(message.contains("did not finish before its deadline"));
assert!(message.contains("retry once"));
assert!(!message.contains("unacceptable risk"));
for timeout_instructions in [None, Some("Catalog timeout instructions."), Some("")] {
model.model_messages = Some(
serde_json::from_value(serde_json::json!({
"auto_review": {
"policy": "review policy",
"timeout_instructions": timeout_instructions,
},
}))
.expect("model messages should deserialize"),
);
assert_eq!(
guardian_timeout_message(&model),
timeout_instructions.unwrap_or(&message),
);
}
}
#[tokio::test]
+1 -1
View File
@@ -283,7 +283,7 @@ pub(crate) async fn handle_mcp_tool_call(
&call_id,
invocation,
item_metadata.clone(),
crate::guardian::guardian_timeout_message(),
crate::guardian::guardian_timeout_message(&turn_context.model_info),
/*already_started*/ true,
)
.await
+9 -5
View File
@@ -29,6 +29,7 @@ use codex_protocol::mcp_approval_meta::TOOL_DESCRIPTION_KEY as MCP_ELICITATION_T
use codex_protocol::mcp_approval_meta::TOOL_NAME_KEY as MCP_ELICITATION_TOOL_NAME_KEY;
use codex_protocol::mcp_approval_meta::TOOL_PARAMS_KEY as MCP_ELICITATION_TOOL_PARAMS_KEY;
use codex_protocol::mcp_approval_meta::TOOL_TITLE_KEY as MCP_ELICITATION_TOOL_TITLE_KEY;
use codex_protocol::openai_models::ModelInfo;
use codex_rmcp_client::Elicitation;
use rmcp::model::ElicitationAction;
use rmcp::model::RequestMetaObject;
@@ -784,8 +785,9 @@ async fn review_guardian_mcp_elicitation(
)
.await;
return Ok(matches!(decision, ReviewDecision::Approved)
.then(|| mcp_elicitation_response_from_guardian_decision(decision)));
return Ok(matches!(decision, ReviewDecision::Approved).then(|| {
mcp_elicitation_response_from_guardian_decision(decision, &turn_context.model_info)
}));
}
let approval_policy = mcp_config.approval_policy.value();
@@ -855,6 +857,7 @@ async fn review_guardian_mcp_elicitation(
.await;
Ok(Some(mcp_elicitation_response_from_guardian_decision(
decision,
&turn_context.model_info,
)))
}
@@ -1005,6 +1008,7 @@ fn mcp_elicitation_request_id(id: &RequestId) -> String {
fn mcp_elicitation_response_from_guardian_decision(
decision: ReviewDecision,
model_info: &ModelInfo,
) -> ElicitationResponse {
match decision {
ReviewDecision::Approved
@@ -1017,9 +1021,9 @@ fn mcp_elicitation_response_from_guardian_decision(
meta: Some(mcp_elicitation_auto_meta()),
},
ReviewDecision::Denied { rejection } => mcp_elicitation_decline_with_message(rejection),
ReviewDecision::TimedOut => {
mcp_elicitation_decline_with_message(crate::guardian::guardian_timeout_message())
}
ReviewDecision::TimedOut => mcp_elicitation_decline_with_message(
crate::guardian::guardian_timeout_message(model_info),
),
ReviewDecision::Abort => ElicitationResponse {
action: ElicitationAction::Cancel,
content: None,
+35 -7
View File
@@ -217,8 +217,9 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() {
#[test]
fn guardian_decisions_map_to_elicitation_responses_without_session_state() {
let model = codex_models_manager::model_info::model_info_from_slug("acting-model");
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::Approved),
mcp_elicitation_response_from_guardian_decision(ReviewDecision::Approved, &model),
ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(json!({})),
@@ -228,9 +229,10 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() {
}
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::denied(
"Denied by Guardian",
)),
mcp_elicitation_response_from_guardian_decision(
ReviewDecision::denied("Denied by Guardian"),
&model,
),
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
@@ -241,18 +243,18 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() {
}
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut),
mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut, &model),
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
"message": crate::guardian::guardian_timeout_message(),
"message": crate::guardian::guardian_timeout_message(&model),
})),
}
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::Abort),
mcp_elicitation_response_from_guardian_decision(ReviewDecision::Abort, &model),
ElicitationResponse {
action: ElicitationAction::Cancel,
content: None,
@@ -262,3 +264,29 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() {
}
);
}
#[test]
fn guardian_elicitation_timeout_uses_acting_model_instructions() {
let mut model = codex_models_manager::model_info::model_info_from_slug("acting-model");
for timeout_instructions in ["Catalog timeout instructions.", ""] {
model.model_messages = Some(
serde_json::from_value(json!({
"auto_review": {
"timeout_instructions": timeout_instructions,
},
}))
.expect("model messages should deserialize"),
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut, &model),
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
"message": timeout_instructions,
})),
}
);
}
}
+6 -3
View File
@@ -36,6 +36,7 @@ use codex_protocol::approvals::NetworkApprovalProtocol;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::error::CodexErr;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::openai_models::ModelInfo;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkPolicyRuleAction;
@@ -452,7 +453,7 @@ struct ApprovalResolution {
}
impl ApprovalResolution {
fn into_tool_result(self) -> Result<ReviewDecision, ToolError> {
fn into_tool_result(self, model_info: &ModelInfo) -> Result<ReviewDecision, ToolError> {
let source = self.source;
match self.decision {
ReviewDecision::ApprovedMcpPolicyAmendment => {
@@ -474,7 +475,9 @@ impl ApprovalResolution {
Err(ToolError::Rejected(rejection.to_string()))
}
ReviewDecision::Denied { rejection } => Err(ToolError::Rejected(rejection)),
ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())),
ReviewDecision::TimedOut => {
Err(ToolError::Rejected(guardian_timeout_message(model_info)))
}
ReviewDecision::Abort => Err(ToolError::Codex(CodexErr::TurnAborted)),
decision => Ok(decision),
}
@@ -543,7 +546,7 @@ impl Session {
_ => {}
}
}
resolution.into_tool_result()
resolution.into_tool_result(&ctx.review_context.turn().model_info)
}
async fn request_reviewer_approval(
+28 -3
View File
@@ -1,4 +1,5 @@
use super::*;
use codex_models_manager::model_info::model_info_from_slug;
use codex_protocol::approvals::NetworkPolicyAmendment;
use pretty_assertions::assert_eq;
@@ -15,7 +16,7 @@ fn approval_resolution_rejects_denied_network_policy_amendment() {
};
assert!(matches!(
resolution.into_tool_result(),
resolution.into_tool_result(&model_info_from_slug("acting-model")),
Err(ToolError::Rejected(rejection)) if rejection == "rejected by user"
));
}
@@ -28,7 +29,7 @@ fn approval_resolution_rejects_mcp_policy_amendment() {
};
assert!(matches!(
resolution.into_tool_result(),
resolution.into_tool_result(&model_info_from_slug("acting-model")),
Err(ToolError::Rejected(rejection)) if rejection == "Error while requesting approval"
));
}
@@ -41,7 +42,7 @@ fn approval_resolution_aborts_turn_when_approval_is_aborted() {
};
assert!(matches!(
resolution.into_tool_result(),
resolution.into_tool_result(&model_info_from_slug("acting-model")),
Err(ToolError::Codex(error))
if matches!(
error.details(),
@@ -50,6 +51,30 @@ fn approval_resolution_aborts_turn_when_approval_is_aborted() {
));
}
#[test]
fn approval_resolution_uses_acting_model_timeout_instructions() {
let mut model = model_info_from_slug("acting-model");
for timeout_instructions in ["Catalog timeout instructions.", ""] {
model.model_messages = Some(
serde_json::from_value(serde_json::json!({
"auto_review": {
"timeout_instructions": timeout_instructions,
},
}))
.expect("model messages should deserialize"),
);
let resolution = ApprovalResolution {
decision: ReviewDecision::TimedOut,
source: ApprovalResolutionSource::Guardian,
};
assert!(matches!(
resolution.into_tool_result(&model),
Err(ToolError::Rejected(rejection)) if rejection == timeout_instructions
));
}
}
#[test]
fn guardian_cwd_preserves_drive_shaped_local_posix_path() {
let native_cwd = AbsolutePathBuf::try_from(std::path::PathBuf::from("/C:/workspace"))
@@ -551,7 +551,9 @@ impl CoreShellActionProvider {
EscalationDecision::deny(Some(rejection))
}
ReviewDecision::TimedOut => EscalationDecision::deny(Some(
crate::guardian::guardian_timeout_message(),
crate::guardian::guardian_timeout_message(
&self.review_context.turn().model_info,
),
)),
ReviewDecision::ApprovedMcpPolicyAmendment => {
error!("Shell escalation received ApprovedMcpPolicyAmendment");