mirror of
https://github.com/openai/codex.git
synced 2026-11-05 16:29:31 +00:00
Support catalog messages for non-request approval policies (#34434)
## What changed - Add model-catalog approval message variants for `never` and `unless_trusted`. - Select the catalog message that matches the active approval policy, while retaining the existing built-in text when that variant is absent. - Treat an explicitly empty variant as an instruction to suppress the built-in approval text, consistent with `on_request` messages. ## Testing - Cover variant selection, fallback and empty-message behavior, catalog deserialization, and the initial permissions message sent to the model. GitOrigin-RevId: a0f8d41a08645f39b80093be53f200eeee18ca25
This commit is contained in:
1 parent
fd3c1dc13d
commit
2be7d3bcd9
6 files changed
+135
-11
No files matched your search
@@ -72,6 +72,8 @@ fn permissions_state(
|
|||||||
let approval_messages = ApprovalMessages {
|
let approval_messages = ApprovalMessages {
|
||||||
on_request: Some("Ask for approval.".to_string()),
|
on_request: Some("Ask for approval.".to_string()),
|
||||||
on_request_auto_review: None,
|
on_request_auto_review: None,
|
||||||
|
never: None,
|
||||||
|
unless_trusted: None,
|
||||||
};
|
};
|
||||||
let permission_messages = PermissionMessages {
|
let permission_messages = PermissionMessages {
|
||||||
danger_full_access: Some("Full access.".to_string()),
|
danger_full_access: Some("Full access.".to_string()),
|
||||||
|
|||||||
@@ -51,6 +51,8 @@ fn model_with_approval_messages(
|
|||||||
approvals: Some(ApprovalMessages {
|
approvals: Some(ApprovalMessages {
|
||||||
on_request: Some(on_request.to_string()),
|
on_request: Some(on_request.to_string()),
|
||||||
on_request_auto_review: Some(on_request_auto_review.to_string()),
|
on_request_auto_review: Some(on_request_auto_review.to_string()),
|
||||||
|
never: None,
|
||||||
|
unless_trusted: None,
|
||||||
}),
|
}),
|
||||||
auto_review: None,
|
auto_review: None,
|
||||||
permissions: None,
|
permissions: None,
|
||||||
@@ -179,6 +181,69 @@ async fn model_change_appends_new_catalog_approval_message() -> Result<()> {
|
|||||||
Ok(())
|
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)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn catalog_permission_message_is_sent_initially_and_after_model_change() -> Result<()> {
|
async fn catalog_permission_message_is_sent_initially_and_after_model_change() -> Result<()> {
|
||||||
skip_if_no_network!(Ok(()));
|
skip_if_no_network!(Ok(()));
|
||||||
|
|||||||
@@ -20,6 +20,8 @@ fn base_instruction_override_preserves_catalog_approval_messages() {
|
|||||||
let approvals = ApprovalMessages {
|
let approvals = ApprovalMessages {
|
||||||
on_request: Some("user approvals".to_string()),
|
on_request: Some("user approvals".to_string()),
|
||||||
on_request_auto_review: Some("auto approvals".to_string()),
|
on_request_auto_review: Some("auto approvals".to_string()),
|
||||||
|
never: None,
|
||||||
|
unless_trusted: None,
|
||||||
};
|
};
|
||||||
model.model_messages = Some(ModelMessages {
|
model.model_messages = Some(ModelMessages {
|
||||||
instructions_template: Some("template".to_string()),
|
instructions_template: Some("template".to_string()),
|
||||||
@@ -57,6 +59,8 @@ fn disabled_personality_preserves_catalog_approval_messages() {
|
|||||||
let approvals = ApprovalMessages {
|
let approvals = ApprovalMessages {
|
||||||
on_request: Some("user approvals".to_string()),
|
on_request: Some("user approvals".to_string()),
|
||||||
on_request_auto_review: None,
|
on_request_auto_review: None,
|
||||||
|
never: None,
|
||||||
|
unless_trusted: None,
|
||||||
};
|
};
|
||||||
model.model_messages = Some(ModelMessages {
|
model.model_messages = Some(ModelMessages {
|
||||||
instructions_template: Some("template".to_string()),
|
instructions_template: Some("template".to_string()),
|
||||||
|
|||||||
@@ -229,12 +229,15 @@ fn approval_text(
|
|||||||
exec_permission_approvals_enabled: bool,
|
exec_permission_approvals_enabled: bool,
|
||||||
request_permissions_tool_enabled: bool,
|
request_permissions_tool_enabled: bool,
|
||||||
) -> String {
|
) -> String {
|
||||||
if approval_policy == AskForApproval::OnRequest
|
if let Some(approval_messages) = approval_messages {
|
||||||
&& let Some(approval_messages) = approval_messages
|
let selected = match &approval_policy {
|
||||||
{
|
AskForApproval::OnRequest => match approvals_reviewer {
|
||||||
let selected = match approvals_reviewer {
|
ApprovalsReviewer::User => approval_messages.on_request.as_ref(),
|
||||||
ApprovalsReviewer::User => approval_messages.on_request.as_ref(),
|
ApprovalsReviewer::AutoReview => approval_messages.on_request_auto_review.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 {
|
if let Some(selected) = selected {
|
||||||
return selected.clone();
|
return selected.clone();
|
||||||
|
|||||||
@@ -387,18 +387,35 @@ fn catalog_approval_messages_select_reviewer_variant() {
|
|||||||
let messages = ApprovalMessages {
|
let messages = ApprovalMessages {
|
||||||
on_request: Some("user catalog approvals".to_string()),
|
on_request: Some("user catalog approvals".to_string()),
|
||||||
on_request_auto_review: Some("auto-review 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 [
|
for (approval_policy, reviewer, expected) in [
|
||||||
(ApprovalsReviewer::User, "user catalog approvals"),
|
|
||||||
(
|
(
|
||||||
|
AskForApproval::OnRequest,
|
||||||
|
ApprovalsReviewer::User,
|
||||||
|
"user catalog approvals",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
AskForApproval::OnRequest,
|
||||||
ApprovalsReviewer::AutoReview,
|
ApprovalsReviewer::AutoReview,
|
||||||
"auto-review catalog approvals",
|
"auto-review catalog approvals",
|
||||||
),
|
),
|
||||||
|
(
|
||||||
|
AskForApproval::Never,
|
||||||
|
ApprovalsReviewer::AutoReview,
|
||||||
|
"never catalog approvals",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
AskForApproval::UnlessTrusted,
|
||||||
|
ApprovalsReviewer::AutoReview,
|
||||||
|
"unless-trusted catalog approvals",
|
||||||
|
),
|
||||||
] {
|
] {
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
approval_text(
|
approval_text(
|
||||||
AskForApproval::OnRequest,
|
approval_policy,
|
||||||
reviewer,
|
reviewer,
|
||||||
Some(&messages),
|
Some(&messages),
|
||||||
&Policy::empty(),
|
&Policy::empty(),
|
||||||
@@ -415,6 +432,8 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() {
|
|||||||
let messages = ApprovalMessages {
|
let messages = ApprovalMessages {
|
||||||
on_request: Some(String::new()),
|
on_request: Some(String::new()),
|
||||||
on_request_auto_review: None,
|
on_request_auto_review: None,
|
||||||
|
never: None,
|
||||||
|
unless_trusted: None,
|
||||||
};
|
};
|
||||||
let mut exec_policy = Policy::empty();
|
let mut exec_policy = Policy::empty();
|
||||||
exec_policy
|
exec_policy
|
||||||
@@ -452,10 +471,12 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[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 {
|
let messages = ApprovalMessages {
|
||||||
on_request: None,
|
on_request: None,
|
||||||
on_request_auto_review: Some("unused catalog approvals".to_string()),
|
on_request_auto_review: Some("unused catalog approvals".to_string()),
|
||||||
|
never: None,
|
||||||
|
unless_trusted: None,
|
||||||
};
|
};
|
||||||
|
|
||||||
let on_request = approval_text(
|
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);
|
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]
|
#[test]
|
||||||
fn auto_review_approvals_append_auto_review_specific_guidance() {
|
fn auto_review_approvals_append_auto_review_specific_guidance() {
|
||||||
let text = approval_text(
|
let text = approval_text(
|
||||||
|
|||||||
@@ -515,6 +515,8 @@ pub struct ModelMessages {
|
|||||||
pub struct ApprovalMessages {
|
pub struct ApprovalMessages {
|
||||||
pub on_request: Option<String>,
|
pub on_request: Option<String>,
|
||||||
pub on_request_auto_review: 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)]
|
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)]
|
||||||
@@ -765,7 +767,8 @@ mod tests {
|
|||||||
"instructions_template": null,
|
"instructions_template": null,
|
||||||
"instructions_variables": null,
|
"instructions_variables": null,
|
||||||
"approvals": {
|
"approvals": {
|
||||||
"on_request": ""
|
"on_request": "",
|
||||||
|
"never": ""
|
||||||
}
|
}
|
||||||
}"#,
|
}"#,
|
||||||
)
|
)
|
||||||
@@ -776,6 +779,8 @@ mod tests {
|
|||||||
Some(ApprovalMessages {
|
Some(ApprovalMessages {
|
||||||
on_request: Some(String::new()),
|
on_request: Some(String::new()),
|
||||||
on_request_auto_review: None,
|
on_request_auto_review: None,
|
||||||
|
never: Some(String::new()),
|
||||||
|
unless_trusted: None,
|
||||||
})
|
})
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in new issue
Block a user