Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions codex-rs/core/src/context/world_state/permissions_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ fn permissions_state(
let approval_messages = ApprovalMessages {
on_request: Some("Ask for approval.".to_string()),
on_request_auto_review: None,
never: None,
unless_trusted: None,
};
let permission_messages = PermissionMessages {
danger_full_access: Some("Full access.".to_string()),
Expand Down
65 changes: 65 additions & 0 deletions codex-rs/core/tests/suite/permissions_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(()));
Expand Down
4 changes: 4 additions & 0 deletions codex-rs/models-manager/src/model_info_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down Expand Up @@ -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()),
Expand Down
15 changes: 9 additions & 6 deletions codex-rs/prompts/src/permissions_instructions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
53 changes: 49 additions & 4 deletions codex-rs/prompts/src/permissions_instructions_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down
7 changes: 6 additions & 1 deletion codex-rs/protocol/src/openai_models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,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)]
Expand Down Expand Up @@ -765,7 +767,8 @@ mod tests {
"instructions_template": null,
"instructions_variables": null,
"approvals": {
"on_request": ""
"on_request": "",
"never": ""
}
}"#,
)
Expand All @@ -776,6 +779,8 @@ mod tests {
Some(ApprovalMessages {
on_request: Some(String::new()),
on_request_auto_review: None,
never: Some(String::new()),
unless_trusted: None,
})
);
}
Expand Down
Loading