mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix auto-review config compatibility across protocol and SDK (#19113)
## Why This keeps the partial Guardian subagent -> Auto-review rename forward-compatible across mixed Codex installations. Newer binaries need to understand the new `auto_review` spelling, but they cannot write it to shared `~/.codex/config.toml` yet because older CLI/app-server bundles only know `user` and `guardian_subagent` and can fail during config load before recovering. The Python SDK had the opposite compatibility gap: app-server responses can contain `approvalsReviewer: "auto_review"`, but the checked-in generated SDK enum did not accept that value. ## What Changed - Keep `ApprovalsReviewer::AutoReview` readable from both `guardian_subagent` and `auto_review`, while serializing it as `guardian_subagent` in both protocol crates. - Update TUI Auto-review persistence tests so enabling Auto-review writes `approvals_reviewer = "guardian_subagent"` while UI copy still says Auto-review. - Map managed/cloud `feature_requirements.auto_review` to the existing `Feature::GuardianApproval` gate without adding a broad local `[features].auto_review` key or changing config writes. - Add `auto_review` to the Python SDK `ApprovalsReviewer` enum and cover `ThreadResumeResponse` validation. ## Testing - `cargo test -p codex-protocol approvals_reviewer` - `cargo test -p codex-app-server-protocol approvals_reviewer` - `cargo test -p codex-tui update_feature_flags_enabling_guardian_selects_auto_review` - `cargo test -p codex-tui update_feature_flags_enabling_guardian_in_profile_sets_profile_auto_review_policy` - `cargo test -p codex-core feature_requirements_auto_review_disables_guardian_approval` - `pytest sdk/python/tests/test_client_rpc_methods.py::test_thread_resume_response_accepts_auto_review_reviewer` - `git diff --check`
This commit is contained in:
committed by
GitHub
Unverified
parent
305825abd9
commit
17ae906048
@@ -325,7 +325,7 @@ impl From<CoreAskForApproval> for AskForApproval {
|
||||
pub enum ApprovalsReviewer {
|
||||
#[serde(rename = "user")]
|
||||
User,
|
||||
#[serde(rename = "auto_review", alias = "guardian_subagent")]
|
||||
#[serde(rename = "guardian_subagent", alias = "auto_review")]
|
||||
AutoReview,
|
||||
}
|
||||
|
||||
@@ -7500,7 +7500,7 @@ mod tests {
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"),
|
||||
"\"auto_review\""
|
||||
"\"guardian_subagent\""
|
||||
);
|
||||
|
||||
for value in ["user", "auto_review", "guardian_subagent"] {
|
||||
|
||||
@@ -6610,6 +6610,28 @@ async fn feature_requirements_normalize_effective_feature_values() -> std::io::R
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn feature_requirements_auto_review_disables_guardian_approval() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
feature_requirements: Some(crate::config_loader::FeatureRequirementsToml {
|
||||
entries: BTreeMap::from([("auto_review".to_string(), false)]),
|
||||
}),
|
||||
..Default::default()
|
||||
}))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(!config.features.enabled(Feature::GuardianApproval));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn browser_feature_requirements_are_valid() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -199,6 +199,11 @@ fn parse_feature_requirements(
|
||||
) -> BTreeMap<Feature, bool> {
|
||||
let mut pinned_features = BTreeMap::new();
|
||||
for (key, enabled) in feature_requirements.entries {
|
||||
if key == "auto_review" {
|
||||
pinned_features.insert(Feature::GuardianApproval, enabled);
|
||||
continue;
|
||||
}
|
||||
|
||||
if let Some(feature) = canonical_feature_for_key(&key) {
|
||||
pinned_features.insert(feature, enabled);
|
||||
continue;
|
||||
|
||||
@@ -87,8 +87,8 @@ pub enum ApprovalsReviewer {
|
||||
#[default]
|
||||
#[serde(rename = "user")]
|
||||
User,
|
||||
#[serde(rename = "auto_review", alias = "guardian_subagent")]
|
||||
#[strum(serialize = "auto_review")]
|
||||
#[serde(rename = "guardian_subagent", alias = "auto_review")]
|
||||
#[strum(serialize = "guardian_subagent")]
|
||||
AutoReview,
|
||||
}
|
||||
|
||||
@@ -609,7 +609,7 @@ mod tests {
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"),
|
||||
"\"auto_review\""
|
||||
"\"guardian_subagent\""
|
||||
);
|
||||
|
||||
for value in ["user", "auto_review", "guardian_subagent"] {
|
||||
|
||||
@@ -1675,7 +1675,7 @@ async fn update_feature_flags_enabling_guardian_selects_auto_review() -> Result<
|
||||
|
||||
let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?;
|
||||
assert!(config.contains("guardian_approval = true"));
|
||||
assert!(config.contains("approvals_reviewer = \"auto_review\""));
|
||||
assert!(config.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(config.contains("approval_policy = \"on-request\""));
|
||||
assert!(config.contains("sandbox_mode = \"workspace-write\""));
|
||||
Ok(())
|
||||
@@ -1835,7 +1835,7 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review
|
||||
);
|
||||
|
||||
let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?;
|
||||
assert!(config.contains("approvals_reviewer = \"auto_review\""));
|
||||
assert!(config.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(config.contains("guardian_approval = true"));
|
||||
assert!(config.contains("approval_policy = \"on-request\""));
|
||||
assert!(config.contains("sandbox_mode = \"workspace-write\""));
|
||||
@@ -1969,7 +1969,7 @@ async fn update_feature_flags_enabling_guardian_in_profile_sets_profile_auto_rev
|
||||
);
|
||||
assert_eq!(
|
||||
profile_config.get("approvals_reviewer"),
|
||||
Some(&TomlValue::String("auto_review".to_string()))
|
||||
Some(&TomlValue::String("guardian_subagent".to_string()))
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -724,7 +724,7 @@ mod tests {
|
||||
rendered.contains("allowed_approval_policies: on-request (source: cloud requirements)")
|
||||
);
|
||||
assert!(rendered.contains(
|
||||
"allowed_approvals_reviewers: auto_review (source: MDM managed_config.toml (legacy))"
|
||||
"allowed_approvals_reviewers: guardian_subagent (source: MDM managed_config.toml (legacy))"
|
||||
));
|
||||
assert!(
|
||||
rendered.contains(
|
||||
@@ -779,7 +779,7 @@ mod tests {
|
||||
|
||||
let rendered = render_to_text(&render_debug_config_lines(&stack));
|
||||
assert!(rendered.contains(
|
||||
"allowed_approvals_reviewers: auto_review (source: MDM managed_config.toml (legacy))"
|
||||
"allowed_approvals_reviewers: guardian_subagent (source: MDM managed_config.toml (legacy))"
|
||||
));
|
||||
assert!(!rendered.contains("Requirements:\n <none>"));
|
||||
}
|
||||
|
||||
@@ -146,6 +146,7 @@ class AppToolsConfig(BaseModel):
|
||||
|
||||
class ApprovalsReviewer(Enum):
|
||||
user = "user"
|
||||
auto_review = "auto_review"
|
||||
guardian_subagent = "guardian_subagent"
|
||||
|
||||
|
||||
|
||||
@@ -4,7 +4,12 @@ from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
from codex_app_server.client import AppServerClient, _params_dict
|
||||
from codex_app_server.generated.v2_all import ThreadListParams, ThreadTokenUsageUpdatedNotification
|
||||
from codex_app_server.generated.v2_all import (
|
||||
ApprovalsReviewer,
|
||||
ThreadListParams,
|
||||
ThreadResumeResponse,
|
||||
ThreadTokenUsageUpdatedNotification,
|
||||
)
|
||||
from codex_app_server.models import UnknownNotification
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[1]
|
||||
@@ -40,6 +45,34 @@ def test_generated_v2_bundle_has_single_shared_plan_type_definition() -> None:
|
||||
assert source.count("class PlanType(") == 1
|
||||
|
||||
|
||||
def test_thread_resume_response_accepts_auto_review_reviewer() -> None:
|
||||
response = ThreadResumeResponse.model_validate(
|
||||
{
|
||||
"approvalPolicy": "on-request",
|
||||
"approvalsReviewer": "auto_review",
|
||||
"cwd": "/tmp",
|
||||
"model": "gpt-5",
|
||||
"modelProvider": "openai",
|
||||
"sandbox": {"type": "dangerFullAccess"},
|
||||
"thread": {
|
||||
"cliVersion": "1.0.0",
|
||||
"createdAt": 1,
|
||||
"cwd": "/tmp",
|
||||
"ephemeral": False,
|
||||
"id": "thread-1",
|
||||
"modelProvider": "openai",
|
||||
"preview": "",
|
||||
"source": "cli",
|
||||
"status": {"type": "idle"},
|
||||
"turns": [],
|
||||
"updatedAt": 1,
|
||||
},
|
||||
}
|
||||
)
|
||||
|
||||
assert response.approvals_reviewer is ApprovalsReviewer.auto_review
|
||||
|
||||
|
||||
def test_notifications_are_typed_with_canonical_v2_methods() -> None:
|
||||
client = AppServerClient()
|
||||
event = client._coerce_notification(
|
||||
@@ -89,7 +122,9 @@ def test_unknown_notifications_fall_back_to_unknown_payloads() -> None:
|
||||
|
||||
def test_invalid_notification_payload_falls_back_to_unknown() -> None:
|
||||
client = AppServerClient()
|
||||
event = client._coerce_notification("thread/tokenUsage/updated", {"threadId": "missing"})
|
||||
event = client._coerce_notification(
|
||||
"thread/tokenUsage/updated", {"threadId": "missing"}
|
||||
)
|
||||
|
||||
assert event.method == "thread/tokenUsage/updated"
|
||||
assert isinstance(event.payload, UnknownNotification)
|
||||
|
||||
Reference in New Issue
Block a user