[model messages v2] use catalog approval messages for all approval policies

This commit is contained in:
Roy Han
2026-07-20 17:19:44 -07:00
committed by Dylan Hurd
parent eaa0b48cc5
commit 2c40f50e50
5 changed files with 133 additions and 11 deletions
@@ -51,6 +51,8 @@ fn model_with_approval_messages(
approvals: Some(ApprovalMessages {
on_request: Some(on_request.to_string()),
on_request_auto_review: Some(on_request_auto_review.to_string()),
never: None,
unless_trusted: None,
}),
auto_review: None,
permissions: None,
@@ -179,6 +181,69 @@ async fn model_change_appends_new_catalog_approval_message() -> Result<()> {
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn catalog_non_on_request_approval_messages_are_sent_in_initial_permissions() -> Result<()> {
skip_if_no_network!(Ok(()));
for (approval_policy, approvals, expected, unexpected) in [
(
AskForApproval::Never,
ApprovalMessages {
on_request: None,
on_request_auto_review: None,
never: Some("catalog never approval instructions".to_string()),
unless_trusted: None,
},
"catalog never approval instructions",
"Approval policy is currently never",
),
(
AskForApproval::UnlessTrusted,
ApprovalMessages {
on_request: None,
on_request_auto_review: None,
never: None,
unless_trusted: Some("catalog unless-trusted approval instructions".to_string()),
},
"catalog unless-trusted approval instructions",
"`approval_policy` is `unless-trusted`",
),
] {
let server = start_mock_server().await;
let req = mount_sse_once(
&server,
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
)
.await;
let model_slug = "catalog-non-on-request-approvals-model";
let mut model = model_info_from_slug(model_slug);
model.model_messages = Some(ModelMessages {
instructions_template: None,
instructions_variables: None,
approvals: Some(approvals),
auto_review: None,
permissions: None,
});
let mut builder = test_codex()
.with_model(model_slug)
.with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config.model_catalog = Some(ModelsResponse {
models: vec![model],
});
});
let test = builder.build(&server).await?;
submit_text_turn(&test, "hello").await?;
let permissions = permissions_texts(&req.single_request());
assert_eq!(permissions.len(), 1);
assert!(permissions[0].contains(expected));
assert!(!permissions[0].contains(unexpected));
}
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn catalog_permission_message_is_sent_initially_and_after_model_change() -> Result<()> {
skip_if_no_network!(Ok(()));
@@ -20,6 +20,8 @@ fn base_instruction_override_preserves_catalog_approval_messages() {
let approvals = ApprovalMessages {
on_request: Some("user approvals".to_string()),
on_request_auto_review: Some("auto approvals".to_string()),
never: None,
unless_trusted: None,
};
model.model_messages = Some(ModelMessages {
instructions_template: Some("template".to_string()),
@@ -57,6 +59,8 @@ fn disabled_personality_preserves_catalog_approval_messages() {
let approvals = ApprovalMessages {
on_request: Some("user approvals".to_string()),
on_request_auto_review: None,
never: None,
unless_trusted: None,
};
model.model_messages = Some(ModelMessages {
instructions_template: Some("template".to_string()),
@@ -229,12 +229,15 @@ fn approval_text(
exec_permission_approvals_enabled: bool,
request_permissions_tool_enabled: bool,
) -> String {
if approval_policy == AskForApproval::OnRequest
&& let Some(approval_messages) = approval_messages
{
let selected = match approvals_reviewer {
ApprovalsReviewer::User => approval_messages.on_request.as_ref(),
ApprovalsReviewer::AutoReview => approval_messages.on_request_auto_review.as_ref(),
if let Some(approval_messages) = approval_messages {
let selected = match &approval_policy {
AskForApproval::OnRequest => match approvals_reviewer {
ApprovalsReviewer::User => approval_messages.on_request.as_ref(),
ApprovalsReviewer::AutoReview => approval_messages.on_request_auto_review.as_ref(),
},
AskForApproval::Never => approval_messages.never.as_ref(),
AskForApproval::UnlessTrusted => approval_messages.unless_trusted.as_ref(),
AskForApproval::Granular(_) => None,
};
if let Some(selected) = selected {
return selected.clone();
@@ -387,18 +387,35 @@ fn catalog_approval_messages_select_reviewer_variant() {
let messages = ApprovalMessages {
on_request: Some("user catalog approvals".to_string()),
on_request_auto_review: Some("auto-review catalog approvals".to_string()),
never: Some("never catalog approvals".to_string()),
unless_trusted: Some("unless-trusted catalog approvals".to_string()),
};
for (reviewer, expected) in [
(ApprovalsReviewer::User, "user catalog approvals"),
for (approval_policy, reviewer, expected) in [
(
AskForApproval::OnRequest,
ApprovalsReviewer::User,
"user catalog approvals",
),
(
AskForApproval::OnRequest,
ApprovalsReviewer::AutoReview,
"auto-review catalog approvals",
),
(
AskForApproval::Never,
ApprovalsReviewer::AutoReview,
"never catalog approvals",
),
(
AskForApproval::UnlessTrusted,
ApprovalsReviewer::AutoReview,
"unless-trusted catalog approvals",
),
] {
assert_eq!(
approval_text(
AskForApproval::OnRequest,
approval_policy,
reviewer,
Some(&messages),
&Policy::empty(),
@@ -415,6 +432,8 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() {
let messages = ApprovalMessages {
on_request: Some(String::new()),
on_request_auto_review: None,
never: None,
unless_trusted: None,
};
let mut exec_policy = Policy::empty();
exec_policy
@@ -452,10 +471,12 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() {
}
#[test]
fn missing_catalog_key_and_non_on_request_policy_use_legacy_approval_text() {
fn missing_catalog_key_uses_legacy_approval_text() {
let messages = ApprovalMessages {
on_request: None,
on_request_auto_review: Some("unused catalog approvals".to_string()),
never: None,
unless_trusted: None,
};
let on_request = approval_text(
@@ -479,6 +500,30 @@ fn missing_catalog_key_and_non_on_request_policy_use_legacy_approval_text() {
assert_eq!(never, APPROVAL_POLICY_NEVER);
}
#[test]
fn empty_catalog_non_on_request_approval_messages_suppress_legacy_approval_text() {
let messages = ApprovalMessages {
on_request: None,
on_request_auto_review: None,
never: Some(String::new()),
unless_trusted: Some(String::new()),
};
for approval_policy in [AskForApproval::Never, AskForApproval::UnlessTrusted] {
assert_eq!(
approval_text(
approval_policy,
ApprovalsReviewer::AutoReview,
Some(&messages),
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ true,
),
""
);
}
}
#[test]
fn auto_review_approvals_append_auto_review_specific_guidance() {
let text = approval_text(
+6 -1
View File
@@ -513,6 +513,8 @@ pub struct ModelMessages {
pub struct ApprovalMessages {
pub on_request: Option<String>,
pub on_request_auto_review: Option<String>,
pub never: Option<String>,
pub unless_trusted: Option<String>,
}
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)]
@@ -763,7 +765,8 @@ mod tests {
"instructions_template": null,
"instructions_variables": null,
"approvals": {
"on_request": ""
"on_request": "",
"never": ""
}
}"#,
)
@@ -774,6 +777,8 @@ mod tests {
Some(ApprovalMessages {
on_request: Some(String::new()),
on_request_auto_review: None,
never: Some(String::new()),
unless_trusted: None,
})
);
}