mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Rename approvals reviewer variant to auto-review (#19056)
## Why `approvals_reviewer` now uses `auto_review` as the canonical config/API value after #18504, but the Rust enum variant and nearby helper/test names still used `GuardianSubagent` / guardian approval wording. That made follow-up code and reviews confusing even though the external value had already moved to Auto-review. ## What changed - Renamed `ApprovalsReviewer::GuardianSubagent` to `ApprovalsReviewer::AutoReview`. - Updated protocol, app-server, config, core, TUI, exec, and analytics test callsites. - Renamed nearby helper/test names from guardian approval wording to Auto-review wording where they refer to the approvals reviewer mode. - Preserved wire compatibility: - `auto_review` remains the canonical serialized value. - `guardian_subagent` remains accepted as a legacy alias. This intentionally does not rename the `[features].guardian_approval` key, `Feature::GuardianApproval`, `core/src/guardian`, analytics event names, or app-server Guardian review event types. ## Verification - `cargo test -p codex-protocol approvals_reviewer_serializes_auto_review_and_accepts_legacy_guardian_subagent` - `cargo test -p codex-app-server-protocol approvals_reviewer_serializes_auto_review_and_accepts_legacy_guardian_subagent` - `cargo test -p codex-config approvals_reviewer` - `cargo test -p codex-tui update_feature_flags` - `cargo test -p codex-core permissions_instructions` - `cargo test -p codex-tui permissions_selection`
This commit is contained in:
committed by
GitHub
Unverified
parent
eed0e07825
commit
83ec1eb5d6
@@ -251,7 +251,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref");
|
||||
let mut config = (*parent_ctx.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
parent_ctx.config = Arc::new(config);
|
||||
parent_ctx
|
||||
.approval_policy
|
||||
@@ -363,7 +363,7 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() {
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref");
|
||||
let mut config = (*parent_ctx.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
parent_ctx.config = Arc::new(config);
|
||||
parent_ctx
|
||||
.approval_policy
|
||||
|
||||
@@ -6741,10 +6741,7 @@ approvals_reviewer = "guardian_subagent"
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -6757,17 +6754,14 @@ async fn requirements_disallowing_default_approvals_reviewer_falls_back_to_requi
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
|
||||
..Default::default()
|
||||
}))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -6786,17 +6780,14 @@ async fn root_approvals_reviewer_falls_back_when_disallowed_by_requirements() ->
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
|
||||
..Default::default()
|
||||
}))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
assert!(
|
||||
config.startup_warnings.iter().any(|warning| {
|
||||
warning
|
||||
@@ -6826,17 +6817,14 @@ approvals_reviewer = "user"
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
|
||||
..Default::default()
|
||||
}))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -6857,7 +6845,7 @@ async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirem
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
allowed_approvals_reviewers: Some(vec![
|
||||
ApprovalsReviewer::User,
|
||||
ApprovalsReviewer::GuardianSubagent,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
]),
|
||||
..Default::default()
|
||||
}))
|
||||
@@ -6865,10 +6853,7 @@ async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirem
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
assert!(
|
||||
config
|
||||
.startup_warnings
|
||||
|
||||
@@ -1048,7 +1048,7 @@ impl From<LegacyManagedConfigToml> for ConfigRequirementsToml {
|
||||
}
|
||||
if let Some(approvals_reviewer) = approvals_reviewer {
|
||||
let mut allowed_reviewers = vec![approvals_reviewer];
|
||||
if approvals_reviewer == ApprovalsReviewer::GuardianSubagent {
|
||||
if approvals_reviewer == ApprovalsReviewer::AutoReview {
|
||||
allowed_reviewers.push(ApprovalsReviewer::User);
|
||||
}
|
||||
config_requirements_toml.allowed_approvals_reviewers = Some(allowed_reviewers);
|
||||
@@ -1135,7 +1135,7 @@ foo = "xyzzy"
|
||||
fn legacy_managed_config_backfill_allows_user_when_guardian_is_required() {
|
||||
let legacy = LegacyManagedConfigToml {
|
||||
approval_policy: None,
|
||||
approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
sandbox_mode: None,
|
||||
};
|
||||
|
||||
@@ -1143,10 +1143,7 @@ foo = "xyzzy"
|
||||
|
||||
assert_eq!(
|
||||
requirements.allowed_approvals_reviewers,
|
||||
Some(vec![
|
||||
ApprovalsReviewer::GuardianSubagent,
|
||||
ApprovalsReviewer::User,
|
||||
])
|
||||
Some(vec![ApprovalsReviewer::AutoReview, ApprovalsReviewer::User,])
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -188,7 +188,7 @@ fn approval_text(
|
||||
),
|
||||
};
|
||||
|
||||
if approvals_reviewer == ApprovalsReviewer::GuardianSubagent
|
||||
if approvals_reviewer == ApprovalsReviewer::AutoReview
|
||||
&& approval_policy != AskForApproval::Never
|
||||
{
|
||||
format!("{text}\n\n{AUTO_REVIEW_APPROVAL_SUFFIX}")
|
||||
|
||||
@@ -197,10 +197,10 @@ fn on_request_includes_tool_guidance_alongside_inline_permission_guidance_when_b
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_subagent_approvals_append_guardian_specific_guidance() {
|
||||
fn auto_review_approvals_append_auto_review_specific_guidance() {
|
||||
let text = approval_text(
|
||||
AskForApproval::OnRequest,
|
||||
ApprovalsReviewer::GuardianSubagent,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
&Policy::empty(),
|
||||
/*exec_permission_approvals_enabled*/ false,
|
||||
/*request_permissions_tool_enabled*/ false,
|
||||
@@ -212,10 +212,10 @@ fn guardian_subagent_approvals_append_guardian_specific_guidance() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_subagent_approvals_omit_guardian_specific_guidance_when_approval_is_never() {
|
||||
fn auto_review_approvals_omit_auto_review_specific_guidance_when_approval_is_never() {
|
||||
let text = approval_text(
|
||||
AskForApproval::Never,
|
||||
ApprovalsReviewer::GuardianSubagent,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
&Policy::empty(),
|
||||
/*exec_permission_approvals_enabled*/ false,
|
||||
/*request_permissions_tool_enabled*/ false,
|
||||
|
||||
@@ -146,7 +146,7 @@ pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool {
|
||||
matches!(
|
||||
turn.approval_policy.value(),
|
||||
AskForApproval::OnRequest | AskForApproval::Granular(_)
|
||||
) && turn.config.approvals_reviewer == ApprovalsReviewer::GuardianSubagent
|
||||
) && turn.config.approvals_reviewer == ApprovalsReviewer::AutoReview
|
||||
}
|
||||
|
||||
pub(crate) fn is_guardian_reviewer_source(
|
||||
|
||||
@@ -909,7 +909,7 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() {
|
||||
|
||||
assert!(!routes_approval_to_guardian(&turn));
|
||||
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
turn.config = Arc::new(config);
|
||||
|
||||
assert!(routes_approval_to_guardian(&turn));
|
||||
@@ -919,7 +919,7 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() {
|
||||
async fn routes_approval_to_guardian_allows_granular_review_policy() {
|
||||
let (_session, mut turn) = crate::session::tests::make_session_and_context().await;
|
||||
let mut config = (*turn.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
turn.config = Arc::new(config);
|
||||
turn.approval_policy
|
||||
.set(AskForApproval::Granular(GranularApprovalConfig {
|
||||
|
||||
@@ -1411,7 +1411,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() {
|
||||
.expect("test setup should allow updating approval policy");
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
let config = Arc::new(config);
|
||||
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
|
||||
config.codex_home.to_path_buf(),
|
||||
@@ -1490,7 +1490,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() {
|
||||
.expect("test setup should allow updating approval policy");
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
let config = Arc::new(config);
|
||||
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
|
||||
config.codex_home.to_path_buf(),
|
||||
@@ -1947,7 +1947,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.chatgpt_base_url = server.uri();
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
let config = Arc::new(config);
|
||||
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
|
||||
config.codex_home.to_path_buf(),
|
||||
|
||||
@@ -3827,7 +3827,7 @@ async fn user_turn_updates_approvals_reviewer() {
|
||||
}],
|
||||
cwd: config.cwd.to_path_buf(),
|
||||
approval_policy: config.permissions.approval_policy.value(),
|
||||
approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::GuardianSubagent),
|
||||
approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::AutoReview),
|
||||
sandbox_policy: config.permissions.sandbox_policy.get().clone(),
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
effort: config.model_reasoning_effort,
|
||||
@@ -3843,7 +3843,7 @@ async fn user_turn_updates_approvals_reviewer() {
|
||||
let state = session.state.lock().await;
|
||||
assert_eq!(
|
||||
state.session_configuration.approvals_reviewer,
|
||||
codex_config::types::ApprovalsReviewer::GuardianSubagent
|
||||
codex_config::types::ApprovalsReviewer::AutoReview
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -88,7 +88,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() {
|
||||
.enable(Feature::GuardianApproval)
|
||||
.expect("test setup should allow enabling guardian approvals");
|
||||
let mut config = (*turn_context_raw.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
let config = Arc::new(config);
|
||||
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
|
||||
@@ -166,7 +166,7 @@ async fn request_permissions_guardian_review_stops_when_cancelled() {
|
||||
.enable(Feature::GuardianApproval)
|
||||
.expect("test setup should allow enabling guardian approvals");
|
||||
let mut config = (*turn_context_raw.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
let config = Arc::new(config);
|
||||
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
|
||||
|
||||
Reference in New Issue
Block a user