diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6ee00b1fca5d..9b4c8826d980 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -7385,6 +7385,20 @@ ], "type": "string" }, + "AutoReviewRequirements": { + "properties": { + "requiredOnModels": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + } + }, + "type": "object" + }, "BrowserUseRequirements": { "properties": { "disableAutoReview": { @@ -8791,6 +8805,16 @@ "null" ] }, + "autoReview": { + "anyOf": [ + { + "$ref": "#/definitions/v2/AutoReviewRequirements" + }, + { + "type": "null" + } + ] + }, "browserUse": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index facbaa42f3d4..95e19011b52d 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -1244,6 +1244,20 @@ ], "type": "string" }, + "AutoReviewRequirements": { + "properties": { + "requiredOnModels": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + } + }, + "type": "object" + }, "BrowserUseRequirements": { "properties": { "disableAutoReview": { @@ -4956,6 +4970,16 @@ "null" ] }, + "autoReview": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewRequirements" + }, + { + "type": "null" + } + ] + }, "browserUse": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json index d29ccb7174b5..407fafa3bd64 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json @@ -59,6 +59,20 @@ } ] }, + "AutoReviewRequirements": { + "properties": { + "requiredOnModels": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + } + }, + "type": "object" + }, "BrowserUseRequirements": { "properties": { "disableAutoReview": { @@ -152,6 +166,16 @@ "null" ] }, + "autoReview": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewRequirements" + }, + { + "type": "null" + } + ] + }, "browserUse": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst index 6060ae214162..3b41b1ba532d 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index e1e4638c403c..4cb39eef81d8 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRequirements.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRequirements.ts new file mode 100644 index 000000000000..26c5ae163691 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRequirements.ts @@ -0,0 +1,5 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +export type AutoReviewRequirements = { requiredOnModels: Array | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts index b1a4c229e19f..d393eac3a567 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts @@ -4,6 +4,7 @@ import type { PathUri } from "../PathUri"; import type { WebSearchMode } from "../WebSearchMode"; import type { AskForApproval } from "./AskForApproval"; +import type { AutoReviewRequirements } from "./AutoReviewRequirements"; import type { BrowserUseRequirements } from "./BrowserUseRequirements"; import type { ComputerUseRequirements } from "./ComputerUseRequirements"; import type { FeedbackRequirements } from "./FeedbackRequirements"; @@ -12,4 +13,4 @@ import type { ResidencyRequirement } from "./ResidencyRequirement"; import type { SandboxMode } from "./SandboxMode"; import type { WindowsSandboxSetupMode } from "./WindowsSandboxSetupMode"; -export type ConfigRequirements = {allowedApprovalPolicies: Array | null, allowedSandboxModes: Array | null, allowedWindowsSandboxImplementations: Array | null, allowedPermissionProfiles: { [key in string]?: boolean } | null, defaultPermissions: string | null, allowedWebSearchModes: Array | null, allowManagedHooksOnly: boolean | null, allowAppshots: boolean | null, allowRemoteControl: boolean | null, computerUse: ComputerUseRequirements | null, browserUse: BrowserUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null, models: ModelsRequirements | null, sqliteHome: PathUri | null, logDir: PathUri | null, modelCatalogJson: PathUri | null, checkForUpdateOnStartup: boolean | null, allowLoginShell: boolean | null, feedback: FeedbackRequirements | null, windowsSandboxPrivateDesktop: boolean | null}; +export type ConfigRequirements = {allowedApprovalPolicies: Array | null, allowedSandboxModes: Array | null, allowedWindowsSandboxImplementations: Array | null, allowedPermissionProfiles: { [key in string]?: boolean } | null, defaultPermissions: string | null, allowedWebSearchModes: Array | null, allowManagedHooksOnly: boolean | null, allowAppshots: boolean | null, allowRemoteControl: boolean | null, computerUse: ComputerUseRequirements | null, browserUse: BrowserUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null, autoReview: AutoReviewRequirements | null, models: ModelsRequirements | null, sqliteHome: PathUri | null, logDir: PathUri | null, modelCatalogJson: PathUri | null, checkForUpdateOnStartup: boolean | null, allowLoginShell: boolean | null, feedback: FeedbackRequirements | null, windowsSandboxPrivateDesktop: boolean | null}; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index 23237ddd9840..c3de4bf16288 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -41,6 +41,7 @@ export type { AskForApproval } from "./AskForApproval"; export type { AttestationGenerateParams } from "./AttestationGenerateParams"; export type { AttestationGenerateResponse } from "./AttestationGenerateResponse"; export type { AutoReviewDecisionSource } from "./AutoReviewDecisionSource"; +export type { AutoReviewRequirements } from "./AutoReviewRequirements"; export type { BrowserUseRequirements } from "./BrowserUseRequirements"; export type { ByteRange } from "./ByteRange"; export type { CancelLoginAccountParams } from "./CancelLoginAccountParams"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/config.rs b/codex-rs/app-server-protocol/src/protocol/v2/config.rs index 842e29d516d1..3628963027ec 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/config.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/config.rs @@ -394,6 +394,7 @@ pub struct ConfigRequirements { pub enforce_residency: Option, #[experimental("configRequirements/read.network")] pub network: Option, + pub auto_review: Option, pub models: Option, #[schemars(with = "Option")] pub sqlite_home: Option, @@ -407,6 +408,13 @@ pub struct ConfigRequirements { pub windows_sandbox_private_desktop: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct AutoReviewRequirements { + pub required_on_models: Option>, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index d6a18789d9b3..4210907dab5c 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -1987,6 +1987,7 @@ fn config_requirements_granular_allowed_approval_policy_is_marked_experimental() hooks: None, enforce_residency: None, network: None, + auto_review: None, models: None, sqlite_home: None, log_dir: None, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index e7d4abfaaa93..921b30acca77 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -278,7 +278,7 @@ Example with notification opt-out: - `externalAgentConfig/import/readHistories` — read completed import histories and connector candidates detected from successfully imported session histories. Successful session entries include the original imported title when one was available. Connector candidates include a normalized display `name`, the number of imported sessions that used the connector, and the source metadata field used for detection. - `config/value/write` — write a single config key/value to the user's config.toml on disk; dotted paths such as `desktop.someKey` use the same generic write surface. Writes that overlap a managed requirement are rejected with `configRequirementReadonly`. - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits. Session-static model, reasoning-effort, Plan-mode reasoning-effort, service-tier, and personality defaults do not reload existing threads. -- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including exact managed values (`sqliteHome`, `logDir`, `modelCatalogJson`, `checkForUpdateOnStartup`, `allowLoginShell`, `feedback.enabled`, and `windowsSandboxPrivateDesktop`), allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), remote-control policy (`allowRemoteControl`; `false` force-disables remote control while `true` or `null` preserves existing behavior), computer use policy (`computerUse`), Browser Use policy (`browserUse.disableAutoReview`), pinned feature values (`featureRequirements`, including the default-allowed `in_app_updates` policy that administrators can set to `false`), managed lifecycle hooks (`hooks`, including command handlers with optional `additionalContextLimit` and `mcp_tool` handlers with `server`, `tool`, `input`, `timeoutSec`, and `statusMessage`), `enforceResidency`, managed new-thread defaults (`models.newThread.model`, `models.newThread.modelReasoningEffort`, and `models.newThread.serviceTier`), and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. +- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including exact managed values (`sqliteHome`, `logDir`, `modelCatalogJson`, `checkForUpdateOnStartup`, `allowLoginShell`, `feedback.enabled`, and `windowsSandboxPrivateDesktop`), allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), remote-control policy (`allowRemoteControl`; `false` force-disables remote control while `true` or `null` preserves existing behavior), computer use policy (`computerUse`), Browser Use policy (`browserUse.disableAutoReview`), pinned feature values (`featureRequirements`, including the default-allowed `in_app_updates` policy that administrators can set to `false`), managed lifecycle hooks (`hooks`, including command handlers with optional `additionalContextLimit` and `mcp_tool` handlers with `server`, `tool`, `input`, `timeoutSec`, and `statusMessage`), `enforceResidency`, managed automatic review (`autoReview.requiredOnModels`), model defaults (`models.newThread.model`, `models.newThread.modelReasoningEffort`, and `models.newThread.serviceTier`), and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. ### Example: Start or resume a thread @@ -866,6 +866,15 @@ You can optionally specify config overrides on the new turn. If specified, these - `"user"` — default. Review approval requests directly in the client. - `"auto_review"` — route approval requests to a carefully prompted subagent, which gathers relevant context and applies a risk-based decision framework before approving or denying the request. The legacy value `"guardian_subagent"` is still accepted for compatibility. +Managed `requirements.toml` can require automatic review for specific models: + +```toml +[auto_review] +required_on_models = ["protected-model"] +``` + +Listed models always start with `approvalPolicy: "on-request"` and `approvalsReviewer: "auto_review"`, even when clients provide incompatible startup values. Full Access is automatically downgraded to workspace-write access. Incompatible runtime overrides or disabled Guardian automatic review are rejected. + ```json { "method": "turn/start", "id": 30, "params": { "threadId": "thr_123", diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index b8f082325aa3..8f6750cd3ee0 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -7,6 +7,7 @@ use crate::error_code::invalid_request; use crate::outgoing_message::ConnectionRequestId; use crate::outgoing_message::OutgoingMessageSender; use codex_analytics::AnalyticsEventsClient; +use codex_app_server_protocol::AutoReviewRequirements; use codex_app_server_protocol::BrowserUseRequirements; use codex_app_server_protocol::ClientResponsePayload; use codex_app_server_protocol::ComputerUseRequirements; @@ -415,6 +416,11 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR .enforce_residency .map(map_residency_requirement_to_api), network: requirements.network.map(map_network_requirements_to_api), + auto_review: requirements + .auto_review + .map(|auto_review| AutoReviewRequirements { + required_on_models: auto_review.required_on_models, + }), models: requirements.models.map(|models| ModelsRequirements { new_thread: models.new_thread.map(|new_thread| NewThreadModelDefaults { model: new_thread.model, @@ -648,6 +654,7 @@ mod tests { use super::map_requirements_toml_to_api; use codex_app_server_protocol::FeedbackRequirements; use codex_app_server_protocol::WindowsSandboxSetupMode; + use codex_config::AutoReviewRequirementsToml; use codex_config::ComputerUseRequirementsToml; use codex_config::ConfigRequirementsToml; use codex_config::ModelsRequirementsToml; @@ -717,8 +724,11 @@ mod tests { } #[test] - fn requirements_api_includes_new_thread_model_defaults() { + fn requirements_api_includes_model_auto_review_and_new_thread_defaults() { let mapped = map_requirements_toml_to_api(ConfigRequirementsToml { + auto_review: Some(AutoReviewRequirementsToml { + required_on_models: Some(vec!["gpt-protected".to_string()]), + }), models: Some(ModelsRequirementsToml { new_thread: Some(NewThreadModelDefaultsToml { model: Some("gpt-managed".to_string()), @@ -729,10 +739,15 @@ mod tests { ..ConfigRequirementsToml::default() }); - let defaults = mapped - .models - .and_then(|models| models.new_thread) - .expect("new-thread defaults"); + assert_eq!( + mapped + .auto_review + .expect("managed automatic-review requirements") + .required_on_models, + Some(vec!["gpt-protected".to_string()]) + ); + let models = mapped.models.expect("managed model requirements"); + let defaults = models.new_thread.expect("new-thread defaults"); assert_eq!(defaults.model.as_deref(), Some("gpt-managed")); assert_eq!( defaults.model_reasoning_effort, diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index d6104f8432b7..50a6389bc9df 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -179,11 +179,15 @@ in_app_updates = false } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn config_requirements_read_includes_new_thread_model_defaults() -> Result<()> { +async fn config_requirements_read_includes_model_auto_review_and_new_thread_defaults() -> Result<()> +{ let codex_home = TempDir::new()?; std::fs::write( codex_home.path().join("requirements.toml"), r#" +[auto_review] +required_on_models = ["gpt-protected", "gpt-sensitive"] + [models.new_thread] model = "gpt-managed" model_reasoning_effort = "medium" @@ -200,11 +204,19 @@ service_tier = "fast" let response: ConfigRequirementsReadResponse = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(request_id)).await??; - let defaults = response - .requirements - .and_then(|requirements| requirements.models) - .and_then(|models| models.new_thread) - .expect("managed new-thread defaults"); + let requirements = response.requirements.expect("managed requirements"); + assert_eq!( + requirements + .auto_review + .expect("managed automatic-review requirements") + .required_on_models, + Some(vec![ + "gpt-protected".to_string(), + "gpt-sensitive".to_string() + ]) + ); + let models = requirements.models.expect("managed model requirements"); + let defaults = models.new_thread.expect("managed new-thread defaults"); assert_eq!(defaults.model.as_deref(), Some("gpt-managed")); assert_eq!( defaults.model_reasoning_effort, diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index 48f72cab504d..a40544cd5815 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -44,6 +44,7 @@ mod mcp_server_elicitation; mod mcp_server_status; mod mcp_tool; mod memory_reset; +mod model_auto_review; mod model_list; mod model_provider_capabilities_read; mod multi_agent_v2_developer_instructions; diff --git a/codex-rs/app-server/tests/suite/v2/model_auto_review.rs b/codex-rs/app-server/tests/suite/v2/model_auto_review.rs new file mode 100644 index 000000000000..2689636e9725 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/model_auto_review.rs @@ -0,0 +1,313 @@ +use anyhow::Result; +use app_test_support::MockResponsesConfig; +use app_test_support::TestAppServer; +use app_test_support::create_mock_responses_server_repeating_assistant; +use codex_app_server_protocol::ApprovalsReviewer; +use codex_app_server_protocol::ApprovalsReviewer::AutoReview; +use codex_app_server_protocol::ApprovalsReviewer::User; +use codex_app_server_protocol::AskForApproval; +use codex_app_server_protocol::AskForApproval::Never; +use codex_app_server_protocol::AskForApproval::OnRequest; +use codex_app_server_protocol::JSONRPCError; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::SandboxMode; +use codex_app_server_protocol::SandboxPolicy; +use codex_app_server_protocol::ThreadForkParams as ForkParams; +use codex_app_server_protocol::ThreadForkResponse as ForkResponse; +use codex_app_server_protocol::ThreadResumeParams as ResumeParams; +use codex_app_server_protocol::ThreadResumeResponse as ResumeResponse; +use codex_app_server_protocol::ThreadSettingsUpdateParams as UpdateParams; +use codex_app_server_protocol::ThreadSettingsUpdateResponse as UpdateResponse; +use codex_app_server_protocol::ThreadSettingsUpdatedNotification as SettingsUpdated; +use codex_app_server_protocol::ThreadStartParams as StartParams; +use codex_app_server_protocol::TurnStartParams as TurnParams; +use codex_app_server_protocol::TurnStartResponse as TurnResponse; +use codex_app_server_protocol::UserInput; +use codex_features::Feature; +use pretty_assertions::assert_eq; +use std::time::Duration; +use tempfile::TempDir; +use tokio::time::timeout; + +const TIMEOUT: Duration = Duration::from_secs(10); +const MODEL: &str = "protected-model"; +const REQUIREMENTS: &str = "[auto_review]\nrequired_on_models = [\"protected-model\"]\n"; +const UNSAFE: [(Option, Option); 2] = + [(Some(Never), None), (None, Some(User))]; + +macro_rules! params { + ($ty:ident, $($field:ident $(= $value:expr)?),* $(,)?) => { + $ty { $($field $( : $value)?,)* ..Default::default() } + }; +} + +async fn app_server( + config: MockResponsesConfig, + requirements: &str, +) -> Result<(TempDir, TestAppServer)> { + let home = TempDir::new()?; + config.write(home.path())?; + std::fs::write(home.path().join("requirements.toml"), requirements)?; + let server = TestAppServer::builder() + .with_codex_home(home.path()) + .build_initialized_with_timeout(TIMEOUT) + .await?; + Ok((home, server)) +} + +async fn managed_server() -> Result<(TempDir, TestAppServer)> { + app_server( + MockResponsesConfig::new("http://localhost/unused").with_approval_policy("on-request"), + REQUIREMENTS, + ) + .await +} + +async fn assert_error(server: &mut TestAppServer, request_id: i64, message: &str) -> Result<()> { + let error: JSONRPCError = timeout( + TIMEOUT, + server.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!(error.error.code, -32600); + assert!(error.error.message.contains(message)); + Ok(()) +} + +async fn assert_protected_update(server: &mut TestAppServer) -> Result<()> { + let updated: SettingsUpdated = + timeout(TIMEOUT, server.read_notification("thread/settings/updated")).await??; + let settings = updated.thread_settings; + assert_protected( + &settings.model, + settings.approval_policy, + settings.approvals_reviewer, + ); + Ok(()) +} + +fn assert_protected(model: &str, policy: AskForApproval, reviewer: ApprovalsReviewer) { + assert_eq!((model, policy, reviewer), (MODEL, OnRequest, AutoReview)); +} + +#[tokio::test] +async fn thread_start_enforces_protected_model_auto_review() -> Result<()> { + let (_home, mut server) = managed_server().await?; + let started = server + .start_thread(params!(StartParams, model = Some(MODEL.to_string()))) + .await?; + assert_protected( + &started.model, + started.approval_policy, + started.approvals_reviewer, + ); + for (approval_policy, approvals_reviewer) in UNSAFE { + let started = server + .start_thread(params!( + StartParams, + model = Some(MODEL.to_string()), + approval_policy, + approvals_reviewer, + )) + .await?; + assert_protected( + &started.model, + started.approval_policy, + started.approvals_reviewer, + ); + } + let started = server + .start_thread(params!( + StartParams, + model = Some(MODEL.to_string()), + approval_policy = Some(Never), + approvals_reviewer = Some(User), + sandbox = Some(SandboxMode::DangerFullAccess), + )) + .await?; + assert_protected( + &started.model, + started.approval_policy, + started.approvals_reviewer, + ); + assert!(matches!( + started.sandbox, + SandboxPolicy::WorkspaceWrite { .. } + )); + let (_home, mut disabled) = app_server( + MockResponsesConfig::new("http://localhost/unused") + .with_approval_policy("on-request") + .disable_feature(Feature::GuardianApproval), + REQUIREMENTS, + ) + .await?; + let id = disabled + .send_thread_start_request_with_auto_env(params!( + StartParams, + model = Some(MODEL.to_string()) + )) + .await?; + assert_error(&mut disabled, id, "you need to use auto review").await +} + +#[tokio::test] +async fn thread_and_turn_settings_enforce_protected_model_auto_review() -> Result<()> { + let (_home, mut server) = managed_server().await?; + let thread = server.start_thread(StartParams::default()).await?.thread; + let id = server + .send_thread_settings_update_request(params!( + UpdateParams, + thread_id = thread.id.clone(), + model = Some(MODEL.to_string()) + )) + .await?; + let _: UpdateResponse = timeout(TIMEOUT, server.read_response(id)).await??; + assert_protected_update(&mut server).await?; + for (approval_policy, approvals_reviewer) in UNSAFE { + let id = server + .send_thread_settings_update_request(params!( + UpdateParams, + thread_id = thread.id.clone(), + approval_policy, + approvals_reviewer, + )) + .await?; + assert_error(&mut server, id, "you need to use auto review").await?; + } + let id = server + .send_thread_settings_update_request(params!( + UpdateParams, + thread_id = thread.id.clone(), + sandbox_policy = Some(SandboxPolicy::DangerFullAccess), + )) + .await?; + assert_error(&mut server, id, "you need to use auto review").await?; + let id = server + .send_turn_start_request(params!( + TurnParams, + thread_id = thread.id, + approvals_reviewer = Some(User) + )) + .await?; + assert_error(&mut server, id, "you need to use auto review").await?; + + let turn_thread = server.start_thread(StartParams::default()).await?.thread; + let id = server + .send_turn_start_request(params!( + TurnParams, + thread_id = turn_thread.id.clone(), + model = Some(MODEL.to_string()), + approvals_reviewer = Some(User) + )) + .await?; + assert_error(&mut server, id, "you need to use auto review").await?; + let id = server + .send_turn_start_request(params!( + TurnParams, + thread_id = turn_thread.id, + model = Some(MODEL.to_string()) + )) + .await?; + let _: TurnResponse = timeout(TIMEOUT, server.read_response(id)).await??; + assert_protected_update(&mut server).await +} + +#[tokio::test] +async fn thread_resume_and_fork_upgrade_legacy_protected_model_settings() -> Result<()> { + let responses = create_mock_responses_server_repeating_assistant("Done").await; + let (home, mut legacy) = app_server( + MockResponsesConfig::new(&responses.uri()).with_model(MODEL), + "", + ) + .await?; + let started = legacy.start_thread(StartParams::default()).await?; + assert_eq!( + (started.approval_policy, started.approvals_reviewer), + (Never, User), + ); + let thread_id = started.thread.id; + legacy + .start_turn_and_wait_for_completion(params!( + TurnParams, + thread_id = thread_id.clone(), + input = vec![UserInput::Text { + text: "Save legacy settings".to_string(), + text_elements: Vec::new() + }], + )) + .await?; + drop(legacy); + MockResponsesConfig::new(&responses.uri()) + .with_model("ordinary-model") + .with_approval_policy("on-request") + .write(home.path())?; + std::fs::write(home.path().join("requirements.toml"), REQUIREMENTS)?; + let mut server = TestAppServer::builder() + .with_codex_home(home.path()) + .build_initialized_with_timeout(TIMEOUT) + .await?; + + let id = server + .send_thread_fork_request(params!( + ForkParams, + thread_id = thread_id.clone(), + model = Some(MODEL.to_string()), + approval_policy = Some(Never), + approvals_reviewer = Some(User), + )) + .await?; + let fork: ForkResponse = timeout(TIMEOUT, server.read_response(id)).await??; + assert_protected(&fork.model, fork.approval_policy, fork.approvals_reviewer); + let id = server + .send_thread_fork_request(params!( + ForkParams, + thread_id = thread_id.clone(), + model = Some(MODEL.to_string()) + )) + .await?; + let fork: ForkResponse = timeout(TIMEOUT, server.read_response(id)).await??; + assert_protected(&fork.model, fork.approval_policy, fork.approvals_reviewer); + let id = server + .send_thread_resume_request(params!( + ResumeParams, + thread_id = thread_id.clone(), + approval_policy = Some(Never), + approvals_reviewer = Some(User), + )) + .await?; + let resumed: ResumeResponse = timeout(TIMEOUT, server.read_response(id)).await??; + assert_protected( + &resumed.model, + resumed.approval_policy, + resumed.approvals_reviewer, + ); + let id = server + .send_thread_resume_request(params!(ResumeParams, thread_id)) + .await?; + let resumed: ResumeResponse = timeout(TIMEOUT, server.read_response(id)).await??; + assert_protected( + &resumed.model, + resumed.approval_policy, + resumed.approvals_reviewer, + ); + Ok(()) +} + +#[tokio::test] +async fn thread_settings_update_enforces_global_reviewer_requirements() -> Result<()> { + let (_home, mut server) = app_server( + MockResponsesConfig::new("http://localhost/unused").with_approval_policy("on-request"), + "allowed_approvals_reviewers = [\"auto_review\"]\n", + ) + .await?; + let thread = server.start_thread(StartParams::default()).await?; + assert_eq!(thread.approvals_reviewer, AutoReview); + let id = server + .send_thread_settings_update_request(params!( + UpdateParams, + thread_id = thread.thread.id, + approvals_reviewer = Some(User) + )) + .await?; + assert_error(&mut server, id, "approvals_reviewer").await +} diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index b50936cbdd26..c0bc51ee863e 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -12,6 +12,7 @@ use serde::de::Error as _; use serde::de::value::Error as ValueDeserializerError; use serde::de::value::StrDeserializer; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::fmt; use std::path::PathBuf; use wildmatch::WildMatchPattern; @@ -159,6 +160,7 @@ pub struct ConfigRequirements { pub feedback: Option>, pub approval_policy: ConstrainedWithSource, pub approvals_reviewer: ConstrainedWithSource, + pub auto_review_required_models: Option>>, pub permission_profile: ConstrainedWithSource, pub windows_sandbox_mode: ConstrainedWithSource>, pub windows_sandbox_private_desktop: Option>, @@ -201,6 +203,7 @@ impl Default for ConfigRequirements { Constrained::allow_any_from_default(), /*source*/ None, ), + auto_review_required_models: None, permission_profile: ConstrainedWithSource::new( Constrained::allow_any(PermissionProfile::read_only()), /*source*/ None, @@ -236,6 +239,29 @@ impl Default for ConfigRequirements { } impl ConfigRequirements { + /// Returns whether a model slug or its supported provider alias requires auto-review. + pub fn auto_review_required_for_model(&self, model: &str) -> bool { + let Some(protected_models) = self.auto_review_required_models.as_ref() else { + return false; + }; + + let model = match model.split_once('/') { + Some((namespace, suffix)) + if !namespace.is_empty() + && !suffix.contains('/') + && namespace.chars().all(|character| { + character.is_ascii_alphanumeric() || character == '_' || character == '-' + }) => + { + suffix + } + Some(_) => return false, + None => model, + }; + + protected_models.value.contains(model) + } + pub fn managed_auth_policy(&self) -> ManagedAuthPolicy { ManagedAuthPolicy { allowed_login_methods: self @@ -930,10 +956,16 @@ pub struct ConfigRequirementsToml { #[serde(rename = "experimental_network")] pub network: Option, pub permissions: Option, + pub auto_review: Option, pub models: Option, pub guardian_policy_config: Option, } +#[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] +pub struct AutoReviewRequirementsToml { + pub required_on_models: Option>, +} + #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] pub struct ModelsRequirementsToml { pub new_thread: Option, @@ -1020,6 +1052,7 @@ pub struct ConfigRequirementsWithSources { pub enforce_residency: Option>, pub network: Option>, pub permissions: Option>, + pub auto_review: Option>, pub models: Option>, pub guardian_policy_config: Option>, } @@ -1074,6 +1107,7 @@ impl ConfigRequirementsWithSources { enforce_residency: _, network: _, permissions: _, + auto_review: _, models: _, guardian_policy_config: _, } = &other; @@ -1125,6 +1159,32 @@ impl ConfigRequirementsWithSources { } ); + if let Some(incoming_auto_review) = other.auto_review.take() { + if let Some(existing_auto_review) = self.auto_review.as_mut() { + let mut source_contributed = false; + if let Some(incoming_slugs) = incoming_auto_review.required_on_models { + let protected_slugs = existing_auto_review + .value + .required_on_models + .get_or_insert_default(); + for slug in incoming_slugs { + if !protected_slugs.contains(&slug) { + protected_slugs.push(slug); + source_contributed = true; + } + } + } + if source_contributed && existing_auto_review.source != source { + existing_auto_review.source = RequirementSource::composite([ + existing_auto_review.source.clone(), + source.clone(), + ]); + } + } else { + self.auto_review = Some(Sourced::new(incoming_auto_review, source.clone())); + } + } + if let Some(incoming_apps) = other.apps.take() { if let Some(existing_apps) = self.apps.as_mut() { merge_app_requirements_descending(&mut existing_apps.value, incoming_apps); @@ -1166,6 +1226,7 @@ impl ConfigRequirementsWithSources { enforce_residency, network, permissions, + auto_review, models, guardian_policy_config, } = self; @@ -1201,6 +1262,7 @@ impl ConfigRequirementsWithSources { enforce_residency: enforce_residency.map(|sourced| sourced.value), network: network.map(|sourced| sourced.value), permissions: permissions.map(|sourced| sourced.value), + auto_review: auto_review.map(|sourced| sourced.value), models: models.map(|sourced| sourced.value), guardian_policy_config: guardian_policy_config.map(|sourced| sourced.value), } @@ -1329,6 +1391,12 @@ impl ConfigRequirementsToml { && self.enforce_residency.is_none() && self.network.is_none() && self.permissions.is_none() + && self.auto_review.as_ref().is_none_or(|auto_review| { + auto_review + .required_on_models + .as_ref() + .is_none_or(Vec::is_empty) + }) && self .models .as_ref() @@ -1451,8 +1519,8 @@ impl TryFrom for ConfigRequirements { fn try_from(toml: ConfigRequirementsWithSources) -> Result { // Profile catalog selection remains on ConfigRequirementsToml for // config loading and requirements API projection. Managed new-thread - // defaults also remain there because they are initialization values, - // not runtime constraints. + // defaults also remain there because they are initialization values; + // model-specific auto-review requirements are runtime constraints. let ConfigRequirementsWithSources { allowed_login_methods, allowed_chatgpt_workspaces, @@ -1484,10 +1552,38 @@ impl TryFrom for ConfigRequirements { enforce_residency, network, permissions, + auto_review, models: _, guardian_policy_config, } = toml; + let auto_review_required_models = auto_review + .and_then(|auto_review| { + auto_review + .value + .required_on_models + .map(|slugs| Sourced::new(slugs, auto_review.source)) + }) + .filter(|models| !models.value.is_empty()) + .map(|models| { + let Sourced { value, source } = models; + let mut protected_models = BTreeSet::new(); + for slug in value { + if slug.trim().is_empty() || slug.trim() != slug || slug.contains('/') { + return Err(ConstraintError::InvalidValue { + field_name: "auto_review.required_on_models", + candidate: format!("{slug:?}"), + allowed: "non-empty model slugs without surrounding whitespace or provider namespaces" + .to_string(), + requirement_source: source, + }); + } + protected_models.insert(slug); + } + Ok(Sourced::new(protected_models, source)) + }) + .transpose()?; + if let Some(requirements) = &mcp_servers { validate_mcp_server_requirements( &requirements.value, @@ -1794,6 +1890,7 @@ impl TryFrom for ConfigRequirements { feedback, approval_policy, approvals_reviewer, + auto_review_required_models, permission_profile, windows_sandbox_mode, windows_sandbox_private_desktop, @@ -1971,6 +2068,7 @@ mod tests { enforce_residency, network, permissions, + auto_review, models, guardian_policy_config, } = toml; @@ -2021,6 +2119,7 @@ mod tests { .map(|value| Sourced::new(value, RequirementSource::Unknown)), network: network.map(|value| Sourced::new(value, RequirementSource::Unknown)), permissions: permissions.map(|value| Sourced::new(value, RequirementSource::Unknown)), + auto_review: auto_review.map(|value| Sourced::new(value, RequirementSource::Unknown)), models: models.map(|value| Sourced::new(value, RequirementSource::Unknown)), guardian_policy_config: guardian_policy_config .map(|value| Sourced::new(value, RequirementSource::Unknown)), @@ -2199,6 +2298,34 @@ mod tests { Ok(()) } + #[test] + fn auto_review_required_for_model_matches_exact_provider_aliases() { + let requirements = ConfigRequirements { + auto_review_required_models: Some(Sourced::new( + BTreeSet::from(["protected-model".to_string()]), + RequirementSource::Unknown, + )), + ..Default::default() + }; + + for (model, protected) in [ + ("protected-model", true), + ("protected-model-preview", false), + ("openai-codex/protected-model-preview", false), + ("provider_1/protected-model", true), + ("protected-modelish", false), + ("/protected-model", false), + ("bad.provider/protected-model", false), + ("provider/nested/protected-model", false), + ] { + assert_eq!( + requirements.auto_review_required_for_model(model), + protected, + "{model}" + ); + } + } + #[test] fn merge_unset_fields_copies_every_field_and_sets_sources() { let mut target = ConfigRequirementsWithSources::default(); @@ -2221,6 +2348,9 @@ mod tests { let computer_use = ComputerUseRequirementsToml { allow_locked_computer_use: Some(false), }; + let auto_review = AutoReviewRequirementsToml { + required_on_models: Some(vec!["managed-model".to_string()]), + }; let models = ModelsRequirementsToml { new_thread: Some(NewThreadModelDefaultsToml { model: Some("managed-model".to_string()), @@ -2280,6 +2410,7 @@ mod tests { enforce_residency: Some(enforce_residency), network: None, permissions: None, + auto_review: Some(auto_review.clone()), models: Some(models.clone()), guardian_policy_config: Some(guardian_policy_config.clone()), }; @@ -2349,6 +2480,7 @@ mod tests { enforce_residency: Some(Sourced::new(enforce_residency, enforce_source)), network: None, permissions: None, + auto_review: Some(Sourced::new(auto_review, source.clone())), models: Some(Sourced::new(models, source.clone())), guardian_policy_config: Some(Sourced::new(guardian_policy_config, source)), } diff --git a/codex-rs/config/src/constraint.rs b/codex-rs/config/src/constraint.rs index 8628f3909c1d..e2c7f9430b58 100644 --- a/codex-rs/config/src/constraint.rs +++ b/codex-rs/config/src/constraint.rs @@ -16,6 +16,9 @@ pub enum ConstraintError { requirement_source: RequirementSource, }, + #[error("To use model `{model}`, you need to use auto review.")] + AutoReviewRequired { model: String }, + #[error("field `{field_name}` cannot be empty")] EmptyField { field_name: String }, diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 395764de8dc0..dfe54af894f7 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -60,6 +60,7 @@ pub use config_requirements::AppRequirementToml; pub use config_requirements::AppToolRequirementToml; pub use config_requirements::AppToolsRequirementsToml; pub use config_requirements::AppsRequirementsToml; +pub use config_requirements::AutoReviewRequirementsToml; pub use config_requirements::BrowserUseRequirementsToml; pub use config_requirements::ComputerUseRequirementsToml; pub use config_requirements::ConfigRequirements; diff --git a/codex-rs/config/src/requirements_layers/layer.rs b/codex-rs/config/src/requirements_layers/layer.rs index 8c16eb4824b4..5c25e493aa45 100644 --- a/codex-rs/config/src/requirements_layers/layer.rs +++ b/codex-rs/config/src/requirements_layers/layer.rs @@ -111,6 +111,7 @@ impl ComposableRequirementsLayer { rules: requirements.rules, hooks: requirements.hooks, permissions: requirements.permissions, + auto_review: requirements.auto_review, }, }) } @@ -121,6 +122,7 @@ pub(super) struct DomainMergedRequirementsFields { pub(super) rules: Option, pub(super) hooks: Option, pub(super) permissions: Option, + pub(super) auto_review: Option, } fn parse_layer_toml( @@ -218,6 +220,7 @@ fn strip_special_fields(layer_toml: &mut TomlValue) { remove_top_level_field(layer_toml, "rules"); remove_top_level_field(layer_toml, "hooks"); remove_nested_field_and_prune_empty(layer_toml, &["permissions", "filesystem", "deny_read"]); + remove_nested_field_and_prune_empty(layer_toml, &["auto_review", "required_on_models"]); } fn remove_top_level_field(value: &mut TomlValue, key: &str) -> Option { diff --git a/codex-rs/config/src/requirements_layers/mod.rs b/codex-rs/config/src/requirements_layers/mod.rs index 52a23a29db0e..bd1619f6db9b 100644 --- a/codex-rs/config/src/requirements_layers/mod.rs +++ b/codex-rs/config/src/requirements_layers/mod.rs @@ -1,5 +1,6 @@ mod hooks; mod layer; +mod models; mod permissions; mod rules; mod stack; diff --git a/codex-rs/config/src/requirements_layers/models.rs b/codex-rs/config/src/requirements_layers/models.rs new file mode 100644 index 000000000000..b48ea1488a61 --- /dev/null +++ b/codex-rs/config/src/requirements_layers/models.rs @@ -0,0 +1,59 @@ +//! Model slugs requiring auto-review remain protected across all policy layers. + +use crate::AutoReviewRequirementsToml; +use crate::RequirementSource; +use crate::Sourced; + +use super::stack::merge_output_source; + +#[derive(Default)] +pub(super) struct AutoReviewModelsMergeState { + slugs: Vec, + source: Option, +} + +impl AutoReviewModelsMergeState { + pub(super) fn merge( + &mut self, + incoming: Option, + source: &RequirementSource, + ) { + let Some(incoming_slugs) = incoming + .and_then(|auto_review| auto_review.required_on_models) + .filter(|slugs| !slugs.is_empty()) + else { + return; + }; + + for slug in incoming_slugs { + if !self.slugs.contains(&slug) { + self.slugs.push(slug); + if let Some(existing_source) = self.source.as_mut() { + merge_output_source(existing_source, source); + } else { + self.source = Some(source.clone()); + } + } + } + } + + pub(super) fn apply_to(self, target: &mut Option>) { + if self.slugs.is_empty() { + return; + } + + let source = self.source.unwrap_or(RequirementSource::Unknown); + let Some(existing) = target.as_mut() else { + *target = Some(Sourced::new( + AutoReviewRequirementsToml { + required_on_models: Some(self.slugs), + }, + source, + )); + return; + }; + + existing.value.required_on_models = Some(self.slugs); + merge_output_source(&mut existing.source, &source); + } +} diff --git a/codex-rs/config/src/requirements_layers/stack.rs b/codex-rs/config/src/requirements_layers/stack.rs index 002c6f175711..17aa9c0c0611 100644 --- a/codex-rs/config/src/requirements_layers/stack.rs +++ b/codex-rs/config/src/requirements_layers/stack.rs @@ -12,6 +12,7 @@ //! active managed-dir conflicts. //! - `permissions.filesystem.deny_read` is a high-priority-first union across //! layers. +//! - `auto_review.required_on_models` is a high-priority-first union across layers. use crate::ConfigRequirementsToml; use crate::ConfigRequirementsWithSources; @@ -27,6 +28,7 @@ use super::hooks::HookDirectoryField; use super::hooks::HookMergeState; use super::layer::ComposableRequirementsLayer; use super::layer::RequirementsLayerEntry; +use super::models::AutoReviewModelsMergeState; use super::permissions::DenyReadMergeState; #[derive(Debug, Error, PartialEq, Eq)] @@ -165,6 +167,7 @@ impl RequirementsLayerStack { let mut hooks = HookMergeState::new(hook_directory_field); let mut hooks_output = None; let mut deny_read = DenyReadMergeState::default(); + let mut auto_review_models = AutoReviewModelsMergeState::default(); // Regular TOML fields are folded low-to-high like config. These custom // fields append or union values, so process them high-to-low to keep // priority order visible in the output. @@ -177,10 +180,12 @@ impl RequirementsLayerStack { &layer.source, )?; deny_read.merge(domain_fields.permissions.clone(), &layer.source); + auto_review_models.merge(domain_fields.auto_review.clone(), &layer.source); } output.rules = rules; output.hooks = hooks_output; deny_read.apply_to(&mut output.permissions); + auto_review_models.apply_to(&mut output.auto_review); let output_is_empty = output.clone().into_toml().is_empty(); Ok((!output_is_empty).then_some(output)) @@ -237,6 +242,7 @@ fn populate_merged_regular_fields_with_sources( enforce_residency, network, permissions, + auto_review: _, models, guardian_policy_config, } = requirements; diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index 75a73a0888fb..1a07120f8830 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -169,6 +169,57 @@ service_tier = "fast" ); } +#[test] +fn auto_review_required_models_are_unioned_without_overwriting_new_thread_defaults() { + let low = layer( + "req_low", + "Low", + r#"[auto_review] +required_on_models = ["low-model", "shared-model"] +[models.new_thread] +model = "low-priority-model" +model_reasoning_effort = "low""#, + ); + let high = layer( + "req_high", + "High", + r#"[auto_review] +required_on_models = ["high-model", "shared-model"] +[models.new_thread] +model = "high-priority-model""#, + ); + let expected_source = RequirementSource::composite([high.source.clone(), low.source.clone()]); + let composed = compose_requirements_for_hostname( + vec![ + low, + high, + layer( + "req_empty", + "Empty", + "[auto_review]\nrequired_on_models = []", + ), + ], + /*hostname*/ None, + ) + .expect("compose requirements") + .expect("requirements present"); + + assert_eq!( + composed.clone().into_toml(), + expected_requirements( + r#"[auto_review] +required_on_models = ["high-model", "shared-model", "low-model"] +[models.new_thread] +model = "high-priority-model" +model_reasoning_effort = "low""# + ) + ); + assert_eq!( + composed.auto_review.map(|auto_review| auto_review.source), + Some(expected_source) + ); +} + #[test] fn relative_paths_resolve_against_their_own_layer_base() { let low_dir = tempdir().expect("low-priority requirements directory"); diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index b65c73e5228c..bc76cd0fb3de 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -9480,6 +9480,7 @@ async fn test_requirements_web_search_mode_allowlist_does_not_warn_when_unset() enforce_residency: None, network: None, permissions: None, + auto_review: None, models: None, guardian_policy_config: None, }; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 1dfeae338c0c..5760aeae28cf 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -3270,6 +3270,7 @@ impl Config { feedback: _, approval_policy: mut constrained_approval_policy, approvals_reviewer: mut constrained_approvals_reviewer, + auto_review_required_models: _, permission_profile: mut constrained_permission_profile, windows_sandbox_mode: mut constrained_windows_sandbox_mode, windows_sandbox_private_desktop: _, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index d81b7f59cf91..8a10228587d1 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -506,12 +506,14 @@ pub fn with_app_plugin_sources( pub(crate) fn mcp_approvals_reviewer( config: &Config, + model: Option<&str>, server_name: &str, connector_id: Option<&str>, ) -> ApprovalsReviewer { mcp_approvals_reviewer_from_layers( &config.config_layer_stack, config.approvals_reviewer, + model, server_name, connector_id, ) @@ -520,9 +522,15 @@ pub(crate) fn mcp_approvals_reviewer( pub(crate) fn mcp_approvals_reviewer_from_layers( config_layer_stack: &codex_config::ConfigLayerStack, default_reviewer: ApprovalsReviewer, + model: Option<&str>, server_name: &str, connector_id: Option<&str>, ) -> ApprovalsReviewer { + let requirements = config_layer_stack.requirements(); + if model.is_some_and(|model| requirements.auto_review_required_for_model(model)) { + return ApprovalsReviewer::AutoReview; + } + let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME { apps_config_from_layer_stack(config_layer_stack).and_then(|apps_config| { connector_id @@ -539,11 +547,7 @@ pub(crate) fn mcp_approvals_reviewer_from_layers( }; if let Some(reviewer) = app_reviewer - && config_layer_stack - .requirements() - .approvals_reviewer - .can_set(&reviewer) - .is_ok() + && requirements.approvals_reviewer.can_set(&reviewer).is_ok() { return reviewer; } diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index b97a13b53f7d..44c8fbce8236 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -345,23 +345,39 @@ approvals_reviewer = "{app}" .expect("config should build"); assert_eq!( - mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")), + mcp_approvals_reviewer( + &config, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some("calendar") + ), expected_app ); assert_eq!( - mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("drive")), + mcp_approvals_reviewer( + &config, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some("drive") + ), expected_default ); assert_eq!( mcp_approvals_reviewer( &config, + config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, /*connector_id*/ None ), expected_default ); assert_eq!( - mcp_approvals_reviewer(&config, "custom_server", Some("calendar")), + mcp_approvals_reviewer( + &config, + config.model.as_deref(), + "custom_server", + Some("calendar") + ), expected_global ); } @@ -392,7 +408,12 @@ approvals_reviewer = "user" .expect("config should build"); assert_eq!( - mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")), + mcp_approvals_reviewer( + &config, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some("calendar") + ), ApprovalsReviewer::AutoReview ); } @@ -422,7 +443,12 @@ approvals_reviewer = "user" .expect("config should build"); assert_eq!( - mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")), + mcp_approvals_reviewer( + &config, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some("calendar") + ), ApprovalsReviewer::AutoReview ); } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 352b972e10c1..cffc6baa435b 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1293,6 +1293,7 @@ async fn maybe_request_mcp_tool_approval( let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers( &config.config_layer_stack, config.approvals_reviewer, + Some(turn_context.model_info.slug.as_str()), &invocation.server, metadata.connector_id.as_deref(), ); @@ -1455,6 +1456,7 @@ pub(crate) fn mcp_approvals_reviewer( ) -> ApprovalsReviewer { connectors::mcp_approvals_reviewer( turn_context.config.as_ref(), + Some(turn_context.model_info.slug.as_str()), server_name, metadata.and_then(|metadata| metadata.connector_id.as_deref()), ) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 3c4a2e844336..ab71d9aa9ca4 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -752,6 +752,7 @@ async fn review_guardian_mcp_elicitation( || crate::connectors::mcp_approvals_reviewer_from_layers( &mcp_config.config_layer_stack, ApprovalsReviewer::AutoReview, + Some(turn_context.model_info.slug.as_str()), request.server_name.as_str(), connector_id, ) != ApprovalsReviewer::AutoReview @@ -821,6 +822,7 @@ async fn review_guardian_mcp_elicitation( let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers( &mcp_config.config_layer_stack, mcp_config.approvals_reviewer, + Some(turn_context.model_info.slug.as_str()), request.server_name.as_str(), elicitation_connector_id(&request.elicitation), ); diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 36acaa914ac4..1180a94a4d1c 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -622,6 +622,46 @@ impl Session { config.http_client_factory(), ) .await; + let trusted_guardian_reviewer = + crate::guardian::is_guardian_reviewer_source(&session_source) + && !matches!(conversation_history, InitialHistory::Resumed(_)); + if config + .config_layer_stack + .requirements() + .auto_review_required_for_model(&model) + && !trusted_guardian_reviewer + { + let config = Arc::make_mut(&mut config); + if matches!( + config.legacy_sandbox_policy(), + SandboxPolicy::DangerFullAccess + ) { + let permission_profile = PermissionProfile::workspace_write(); + config + .permissions + .set_permission_profile(permission_profile.clone()) + .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; + if let Some(network) = config.permissions.network.as_ref() { + config.permissions.network = Some( + network + .recompute_for_permission_profile(&permission_profile) + .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?, + ); + } + } + config + .permissions + .approval_policy + .set(AskForApproval::OnRequest) + .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; + config + .config_layer_stack + .requirements() + .approvals_reviewer + .can_set(&ApprovalsReviewer::AutoReview) + .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + } if allow_provider_model_fallback && let Some(requested_model) = config.model.as_ref() && model != *requested_model @@ -709,6 +749,7 @@ impl Session { metrics_service_name, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer, session_source, history_mode, forked_from_thread_id, @@ -718,6 +759,9 @@ impl Session { dynamic_tools, user_shell_override, }; + session_configuration + .validate_auto_review_requirement() + .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; // Generate a unique ID for the lifetime of this session. let session_source_clone = session_configuration.session_source.clone(); diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 138db94f3181..3676d4aaab5f 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -109,6 +109,8 @@ pub(crate) struct SessionConfiguration { pub(super) metrics_service_name: Option, pub(super) app_server_client_name: Option, pub(super) app_server_client_version: Option, + /// Guardian reviewer identity is trusted only when established during an in-memory spawn. + pub(super) trusted_guardian_reviewer: bool, /// Source of the session (cli, vscode, exec, mcp, ...) pub(super) session_source: SessionSource, /// Persisted thread history contract selected when this thread was created. @@ -251,6 +253,38 @@ impl SessionConfiguration { } } + pub(super) fn validate_auto_review_requirement(&self) -> ConstraintResult<()> { + if self.trusted_guardian_reviewer { + return Ok(()); + } + + let requirements = self + .original_config_do_not_use + .config_layer_stack + .requirements(); + let model = self.collaboration_mode.model(); + if !requirements.auto_review_required_for_model(model) { + return Ok(()); + } + + if self.approval_policy.value() == AskForApproval::OnRequest + && self.approvals_reviewer == ApprovalsReviewer::AutoReview + && !self + .file_system_sandbox_policy() + .has_full_disk_write_access() + && self + .original_config_do_not_use + .features + .enabled(Feature::GuardianApproval) + { + return Ok(()); + } + + Err(ConstraintError::AutoReviewRequired { + model: model.to_string(), + }) + } + pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> ConstraintResult { let mut next_configuration = self.clone(); let current_sandbox_policy = self.sandbox_policy(); @@ -303,8 +337,37 @@ impl SessionConfiguration { next_configuration.approval_policy.set(approval_policy)?; } if let Some(approvals_reviewer) = updates.approvals_reviewer { + next_configuration + .original_config_do_not_use + .config_layer_stack + .requirements() + .approvals_reviewer + .can_set(&approvals_reviewer)?; next_configuration.approvals_reviewer = approvals_reviewer; } + if !next_configuration.trusted_guardian_reviewer + && self.collaboration_mode.model() != next_configuration.collaboration_mode.model() + && next_configuration + .original_config_do_not_use + .config_layer_stack + .requirements() + .auto_review_required_for_model(next_configuration.collaboration_mode.model()) + { + if updates.approval_policy.is_none() { + next_configuration + .approval_policy + .set(AskForApproval::OnRequest)?; + } + if updates.approvals_reviewer.is_none() { + next_configuration + .original_config_do_not_use + .config_layer_stack + .requirements() + .approvals_reviewer + .can_set(&ApprovalsReviewer::AutoReview)?; + next_configuration.approvals_reviewer = ApprovalsReviewer::AutoReview; + } + } if let Some(windows_sandbox_level) = updates.windows_sandbox_level { next_configuration.windows_sandbox_level = windows_sandbox_level; } @@ -405,6 +468,7 @@ impl SessionConfiguration { if let Some(app_server_client_version) = updates.app_server_client_version.clone() { next_configuration.app_server_client_version = Some(app_server_client_version); } + next_configuration.validate_auto_review_requirement()?; Ok(next_configuration) } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 1a7732263021..2ef1af21ba33 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -4107,6 +4107,7 @@ async fn set_rate_limits_retains_previous_credits() { metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, @@ -4216,6 +4217,7 @@ async fn set_rate_limits_updates_plan_type_when_present() { metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, @@ -4762,6 +4764,7 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, @@ -5557,6 +5560,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, @@ -5698,6 +5702,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, @@ -5972,6 +5977,7 @@ async fn make_session_with_config_and_rx( metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, @@ -6086,6 +6092,7 @@ async fn make_session_with_history_source_and_agent_control_and_rx( metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: session_source.clone(), history_mode: Default::default(), forked_from_thread_id: None, @@ -7918,6 +7925,7 @@ where metrics_service_name: None, app_server_client_name: None, app_server_client_version: None, + trusted_guardian_reviewer: false, session_source: SessionSource::Exec, history_mode: Default::default(), forked_from_thread_id: None, diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index d70e928dd42a..713113f2cf24 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -2,6 +2,7 @@ #![allow(clippy::unwrap_used)] use anyhow::Result; +use codex_config::test_support::CloudConfigBundleFixture; use codex_config::types::AppToolApproval; use codex_core::config::Config; use codex_features::Feature; @@ -45,6 +46,7 @@ use core_test_support::wait_for_event_match; use pretty_assertions::assert_eq; use serde_json::json; use std::collections::HashMap; +use test_case::test_case; fn set_calendar_approval_mode(config: &mut Config, approval_mode: AppToolApproval) { let approval_mode = match approval_mode { @@ -97,11 +99,12 @@ async fn submit_user_turn( test: &TestCodex, text: &str, approval_policy: AskForApproval, + permission_profile: PermissionProfile, collaboration_mode: Option, ) -> Result<()> { let session_model = test.session_configured.model.clone(); let (sandbox_policy, permission_profile) = - turn_permission_fields(PermissionProfile::Disabled, test.cwd.path()); + turn_permission_fields(permission_profile, test.cwd.path()); test.codex .submit(Op::UserInput { items: vec![UserInput::Text { @@ -205,6 +208,7 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R &test, "Use [$calendar](app://calendar) to create a calendar event.", AskForApproval::OnRequest, + PermissionProfile::Disabled, /*collaboration_mode*/ None, ) .await?; @@ -261,8 +265,11 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian() -> Result<()> -{ +#[test_case(false; "unmanaged model")] +#[test_case(true; "protected model")] +async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian( + protected_model: bool, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -309,9 +316,13 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar .await; let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone()) - .with_config(|config| { - // Use the opposite global reviewer so this route must come from apps._default. - config.approvals_reviewer = ApprovalsReviewer::User; + .with_config(move |config| { + let (reviewer, app_reviewer) = if protected_model { + (ApprovalsReviewer::AutoReview, ApprovalsReviewer::User) + } else { + (ApprovalsReviewer::User, ApprovalsReviewer::AutoReview) + }; + config.approvals_reviewer = reviewer; config .features .enable(Feature::ToolCallMcpElicitation) @@ -319,15 +330,27 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar set_default_app_approval_mode_and_reviewer( config, AppToolApproval::Prompt, - ApprovalsReviewer::AutoReview, + app_reviewer, ); }); + if protected_model { + builder = builder.with_model("gpt-5.4").with_cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement( + "[auto_review]\nrequired_on_models = [\"gpt-5.4\"]\n", + ), + ); + } let test = builder.build(&server).await?; submit_user_turn( &test, "Use [$calendar](app://calendar) to create a calendar event.", AskForApproval::OnRequest, + if protected_model { + PermissionProfile::workspace_write() + } else { + PermissionProfile::Disabled + }, /*collaboration_mode*/ None, ) .await?; @@ -428,6 +451,7 @@ async fn apps_default_writes_prompts_for_writes_but_not_reads() -> Result<()> { &test, "Use [$calendar](app://calendar) to list events, then create one.", AskForApproval::OnRequest, + PermissionProfile::Disabled, /*collaboration_mode*/ None, ) .await?; @@ -598,6 +622,7 @@ async fn mcp_tool_call_metadata_records_prior_request_user_input_tool() -> Resul &test, "Ask for confirmation, then create a calendar event.", AskForApproval::Never, + PermissionProfile::Disabled, Some(CollaborationMode { mode: ModeKind::Plan, settings: Settings { diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 124cfeb423c6..918bb091f016 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -1233,6 +1233,28 @@ impl App { turns: Vec, presentation: ThreadAttachPresentation, ) -> Result<()> { + if let Err(err) = self + .config + .permissions + .approval_policy + .set(session.approval_policy.to_core()) + { + tracing::warn!(%err, "failed to sync app approval policy from thread session"); + } + if let Err(err) = self + .config + .permissions + .set_permission_profile_from_session_snapshot( + PermissionProfileSnapshot::from_session_snapshot( + session.permission_profile.clone(), + session.active_permission_profile.clone(), + ), + ) + { + tracing::warn!(%err, "failed to sync app permissions from thread session"); + } + self.config.approvals_reviewer = session.approvals_reviewer; + let thread_id = session.thread_id; self.primary_thread_id = Some(thread_id); self.primary_session_configured = Some(session.clone()); diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index b3754d764cac..db0329f91bd4 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -2001,6 +2001,17 @@ fn display_permission_profile_from_thread_response( thread_params_mode: ThreadParamsMode, ) -> PermissionProfile { match thread_params_mode { + ThreadParamsMode::Embedded + if matches!( + config.permissions.effective_permission_profile(), + PermissionProfile::Disabled + ) && !matches!( + sandbox, + codex_app_server_protocol::SandboxPolicy::DangerFullAccess + ) => + { + PermissionProfile::from_legacy_sandbox_policy_for_cwd(&sandbox.to_core(), cwd) + } ThreadParamsMode::Embedded => config.permissions.effective_permission_profile(), ThreadParamsMode::Remote => { PermissionProfile::from_legacy_sandbox_policy_for_cwd(&sandbox.to_core(), cwd) diff --git a/codex-rs/tui/src/chatwidget/permission_popups.rs b/codex-rs/tui/src/chatwidget/permission_popups.rs index 9342e2051c41..651d3b875a3d 100644 --- a/codex-rs/tui/src/chatwidget/permission_popups.rs +++ b/codex-rs/tui/src/chatwidget/permission_popups.rs @@ -104,12 +104,17 @@ impl ChatWidget { name: APPROVE_FOR_ME_LABEL.to_string(), description: Some(AUTO_REVIEW_DESCRIPTION.to_string()), is_current: current_review_policy == ApprovalsReviewer::AutoReview - && Self::preset_matches_current( + && (Self::preset_matches_current( current_approval, ¤t_permission_profile, self.config.cwd.as_path(), &preset, - ), + ) || (current_approval == AskForApproval::OnRequest + && self + .config + .config_layer_stack + .requirements() + .auto_review_required_for_model(self.current_model()))), actions: self.permission_mode_actions( &preset, APPROVE_FOR_ME_LABEL.to_string(), @@ -393,8 +398,6 @@ impl ChatWidget { PermissionProfile::Managed { .. } ) && file_system_policy.can_write_path_with_cwd(cwd, cwd) && !file_system_policy.has_full_disk_write_access() - && current_permission_profile.network_sandbox_policy() - == preset.permission_profile.network_sandbox_policy() } _ => current_permission_profile == &preset.permission_profile, } diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index 127e3606d459..538ef3e73c37 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -975,6 +975,7 @@ interrupt_message = false enforce_residency: Some(ResidencyRequirement::Us), network: None, permissions: None, + auto_review: None, models: None, };