mirror of
https://github.com/openai/codex.git
synced 2026-10-21 03:38:57 +00:00
Notify clients when Guardian requires strict review (#39157)
## What changed - Add the experimental `autoApprovalReview/strictReviewRequired` app-server notification with the review's `threadId`, `turnId`, and `startedAtMs`. - Emit it when Guardian v2 leaves an approval review in progress because the action risk is elevated or its score is stale. - Export the notification through the Rust, TypeScript, and JSON schema surfaces and route it to the associated thread. ## Testing - Cover strict-review reason tracking for elevated and stale risk scores. - Verify that high-risk approval routing emits one notification whose identifiers and timestamp match the review-started event. GitOrigin-RevId: e65352894520bfb756c75b2c95e2e149f9767bb2
This commit is contained in:
@@ -3611,6 +3611,27 @@
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"StrictReviewRequiredNotification": {
|
||||
"properties": {
|
||||
"startedAtMs": {
|
||||
"description": "Unix timestamp (in milliseconds) when this review started.",
|
||||
"format": "int64",
|
||||
"type": "integer"
|
||||
},
|
||||
"threadId": {
|
||||
"type": "string"
|
||||
},
|
||||
"turnId": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"startedAtMs",
|
||||
"threadId",
|
||||
"turnId"
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"SubAgentActivityKind": {
|
||||
"enum": [
|
||||
"started",
|
||||
@@ -6642,6 +6663,26 @@
|
||||
"title": "Item/autoApprovalReview/completedNotification",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"method": {
|
||||
"enum": [
|
||||
"autoApprovalReview/strictReviewRequired"
|
||||
],
|
||||
"title": "AutoApprovalReview/strictReviewRequiredNotificationMethod",
|
||||
"type": "string"
|
||||
},
|
||||
"params": {
|
||||
"$ref": "#/definitions/StrictReviewRequiredNotification"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"method",
|
||||
"params"
|
||||
],
|
||||
"title": "AutoApprovalReview/strictReviewRequiredNotification",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"method": {
|
||||
|
||||
+43
@@ -4897,6 +4897,26 @@
|
||||
"title": "Item/autoApprovalReview/completedNotification",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"method": {
|
||||
"enum": [
|
||||
"autoApprovalReview/strictReviewRequired"
|
||||
],
|
||||
"title": "AutoApprovalReview/strictReviewRequiredNotificationMethod",
|
||||
"type": "string"
|
||||
},
|
||||
"params": {
|
||||
"$ref": "#/definitions/v2/StrictReviewRequiredNotification"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"method",
|
||||
"params"
|
||||
],
|
||||
"title": "AutoApprovalReview/strictReviewRequiredNotification",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"method": {
|
||||
@@ -18765,6 +18785,29 @@
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"StrictReviewRequiredNotification": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"startedAtMs": {
|
||||
"description": "Unix timestamp (in milliseconds) when this review started.",
|
||||
"format": "int64",
|
||||
"type": "integer"
|
||||
},
|
||||
"threadId": {
|
||||
"type": "string"
|
||||
},
|
||||
"turnId": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"startedAtMs",
|
||||
"threadId",
|
||||
"turnId"
|
||||
],
|
||||
"title": "StrictReviewRequiredNotification",
|
||||
"type": "object"
|
||||
},
|
||||
"SubAgentActivityKind": {
|
||||
"enum": [
|
||||
"started",
|
||||
|
||||
+43
@@ -15062,6 +15062,26 @@
|
||||
"title": "Item/autoApprovalReview/completedNotification",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"method": {
|
||||
"enum": [
|
||||
"autoApprovalReview/strictReviewRequired"
|
||||
],
|
||||
"title": "AutoApprovalReview/strictReviewRequiredNotificationMethod",
|
||||
"type": "string"
|
||||
},
|
||||
"params": {
|
||||
"$ref": "#/definitions/StrictReviewRequiredNotification"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"method",
|
||||
"params"
|
||||
],
|
||||
"title": "AutoApprovalReview/strictReviewRequiredNotification",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"method": {
|
||||
@@ -16517,6 +16537,29 @@
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"StrictReviewRequiredNotification": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"startedAtMs": {
|
||||
"description": "Unix timestamp (in milliseconds) when this review started.",
|
||||
"format": "int64",
|
||||
"type": "integer"
|
||||
},
|
||||
"threadId": {
|
||||
"type": "string"
|
||||
},
|
||||
"turnId": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"startedAtMs",
|
||||
"threadId",
|
||||
"turnId"
|
||||
],
|
||||
"title": "StrictReviewRequiredNotification",
|
||||
"type": "object"
|
||||
},
|
||||
"SubAgentActivityKind": {
|
||||
"enum": [
|
||||
"started",
|
||||
|
||||
+23
@@ -0,0 +1,23 @@
|
||||
{
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"startedAtMs": {
|
||||
"description": "Unix timestamp (in milliseconds) when this review started.",
|
||||
"format": "int64",
|
||||
"type": "integer"
|
||||
},
|
||||
"threadId": {
|
||||
"type": "string"
|
||||
},
|
||||
"turnId": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"startedAtMs",
|
||||
"threadId",
|
||||
"turnId"
|
||||
],
|
||||
"title": "StrictReviewRequiredNotification",
|
||||
"type": "object"
|
||||
}
|
||||
BIN
Binary file not shown.
BIN
Binary file not shown.
File diff suppressed because one or more lines are too long
+2
-1
File diff suppressed because one or more lines are too long
+9
@@ -0,0 +1,9 @@
|
||||
// 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 StrictReviewRequiredNotification = { threadId: string, turnId: string,
|
||||
/**
|
||||
* Unix timestamp (in milliseconds) when this review started.
|
||||
*/
|
||||
startedAtMs: number, };
|
||||
@@ -423,6 +423,7 @@ export type { SkillsListParams } from "./SkillsListParams";
|
||||
export type { SkillsListResponse } from "./SkillsListResponse";
|
||||
export type { SortDirection } from "./SortDirection";
|
||||
export type { SpendControlLimitSnapshot } from "./SpendControlLimitSnapshot";
|
||||
export type { StrictReviewRequiredNotification } from "./StrictReviewRequiredNotification";
|
||||
export type { SubAgentActivityKind } from "./SubAgentActivityKind";
|
||||
export type { SubagentMigration } from "./SubagentMigration";
|
||||
export type { TerminalInteractionNotification } from "./TerminalInteractionNotification";
|
||||
|
||||
@@ -1824,6 +1824,8 @@ server_notification_definitions! {
|
||||
ItemStarted => "item/started" (v2::ItemStartedNotification),
|
||||
ItemGuardianApprovalReviewStarted => "item/autoApprovalReview/started" (v2::ItemGuardianApprovalReviewStartedNotification),
|
||||
ItemGuardianApprovalReviewCompleted => "item/autoApprovalReview/completed" (v2::ItemGuardianApprovalReviewCompletedNotification),
|
||||
#[experimental("autoApprovalReview/strictReviewRequired")]
|
||||
StrictReviewRequired => "autoApprovalReview/strictReviewRequired" (v2::StrictReviewRequiredNotification),
|
||||
ItemCompleted => "item/completed" (v2::ItemCompletedNotification),
|
||||
/// This event is internal-only. Used by Codex Cloud.
|
||||
RawResponseItemCompleted => "rawResponseItem/completed" (v2::RawResponseItemCompletedNotification),
|
||||
|
||||
@@ -35,6 +35,17 @@ pub struct GuardianWarningNotification {
|
||||
pub message: String,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct StrictReviewRequiredNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
/// Unix timestamp (in milliseconds) when this review started.
|
||||
#[ts(type = "number")]
|
||||
pub started_at_ms: i64,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
|
||||
@@ -1626,6 +1626,7 @@ All items emit shared lifecycle events:
|
||||
- `item/completed` — sends the final `item` once that work itself finishes (for example, after a tool call or message completes); treat this as the authoritative execution/result state.
|
||||
- `item/autoApprovalReview/started` — [UNSTABLE] temporary auto-review notification carrying `{threadId, turnId, targetItemId, review, action}` when approval auto-review begins. This shape is expected to change soon.
|
||||
- `item/autoApprovalReview/completed` — [UNSTABLE] temporary auto-review notification carrying `{threadId, turnId, targetItemId, review, action}` when approval auto-review resolves. This shape is expected to change soon.
|
||||
- `autoApprovalReview/strictReviewRequired` — experimental notification carrying `{threadId, turnId, startedAtMs}` whenever elevated or stale Guardian v2 risk requires synchronous approval review.
|
||||
|
||||
`review` is [UNSTABLE] and currently has `{status, riskLevel?, userAuthorization?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `riskLevel` is one of `"low"`, `"medium"`, `"high"`, or `"critical"` when present. `userAuthorization` is one of `"unknown"`, `"low"`, `"medium"`, or `"high"` when present. `action` is a tagged union with `type: "command" | "execve" | "applyPatch" | "networkAccess" | "mcpToolCall"`. Command-like actions include a `source` discriminator (`"shell"` or `"unifiedExec"`). These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the auto-review app protocol is still being designed.
|
||||
|
||||
|
||||
@@ -53,6 +53,7 @@ use codex_app_server_protocol::RawResponseItemCompletedNotification;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequestPayload;
|
||||
use codex_app_server_protocol::StrictReviewRequiredNotification;
|
||||
use codex_app_server_protocol::ThreadGoalUpdatedNotification;
|
||||
use codex_app_server_protocol::ThreadItem;
|
||||
use codex_app_server_protocol::ThreadRealtimeClosedNotification;
|
||||
@@ -89,6 +90,7 @@ use codex_app_server_protocol::guardian_auto_approval_review_notification;
|
||||
use codex_app_server_protocol::item_event_to_server_notification;
|
||||
use codex_core::CodexThread;
|
||||
use codex_core::ThreadManager;
|
||||
use codex_guardian_v2::StrictReviewReason;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::items::CollabAgentTool as CoreCollabAgentTool;
|
||||
use codex_protocol::items::TurnItem as CoreTurnItem;
|
||||
@@ -321,6 +323,22 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
&assessment,
|
||||
);
|
||||
outgoing.send_server_notification(notification).await;
|
||||
if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress
|
||||
&& conversation
|
||||
.thread_extension_data()
|
||||
.remove::<StrictReviewReason>()
|
||||
.is_some()
|
||||
{
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::StrictReviewRequired(
|
||||
StrictReviewRequiredNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: assessment_turn_id.clone(),
|
||||
started_at_ms: assessment.started_at_ms,
|
||||
},
|
||||
))
|
||||
.await;
|
||||
}
|
||||
let completion_status = match assessment.status {
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::Denied
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::Aborted => {
|
||||
|
||||
@@ -21,6 +21,7 @@ use axum::routing::get;
|
||||
use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ItemGuardianApprovalReviewStartedNotification;
|
||||
use codex_app_server_protocol::StrictReviewRequiredNotification;
|
||||
use codex_app_server_protocol::ThreadForkParams;
|
||||
use codex_app_server_protocol::ThreadForkResponse;
|
||||
use codex_app_server_protocol::ThreadResumeParams;
|
||||
@@ -368,6 +369,35 @@ async fn guardian_v2_routes_tool_approvals(
|
||||
responses_state.guardian_reviews.load(Ordering::SeqCst),
|
||||
expected_guardian_reviews
|
||||
);
|
||||
let strict_review_count = app_server
|
||||
.pending_notification_methods()
|
||||
.into_iter()
|
||||
.filter(|method| method == "autoApprovalReview/strictReviewRequired")
|
||||
.count();
|
||||
assert_eq!(
|
||||
strict_review_count,
|
||||
usize::from(matches!(risk, GuardianRisk::High))
|
||||
);
|
||||
if matches!(risk, GuardianRisk::High) {
|
||||
let review_started: ItemGuardianApprovalReviewStartedNotification = timeout(
|
||||
TIMEOUT,
|
||||
app_server.read_notification("item/autoApprovalReview/started"),
|
||||
)
|
||||
.await??;
|
||||
let strict_review: StrictReviewRequiredNotification = timeout(
|
||||
TIMEOUT,
|
||||
app_server.read_notification("autoApprovalReview/strictReviewRequired"),
|
||||
)
|
||||
.await??;
|
||||
assert_eq!(
|
||||
strict_review,
|
||||
StrictReviewRequiredNotification {
|
||||
thread_id: review_started.thread_id,
|
||||
turn_id: review_started.turn_id,
|
||||
started_at_ms: review_started.started_at_ms,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
mcp_server_handle.abort();
|
||||
responses_server.abort();
|
||||
|
||||
@@ -181,6 +181,13 @@ fn truncate_action_value(value: &mut serde_json::Value, max_tokens: usize) {
|
||||
serde_json::Value::Null | serde_json::Value::Bool(_) | serde_json::Value::Number(_) => {}
|
||||
}
|
||||
}
|
||||
/// Explains why Guardian v2 requires synchronous approval review.
|
||||
#[derive(Debug, Eq, PartialEq)]
|
||||
pub enum StrictReviewReason {
|
||||
ElevatedRisk,
|
||||
StaleScore,
|
||||
}
|
||||
|
||||
struct GuardianV2Enabled;
|
||||
|
||||
#[derive(Default)]
|
||||
@@ -291,14 +298,20 @@ impl ApprovalReviewContributor for GuardianV2Extension {
|
||||
.load(Ordering::Acquire),
|
||||
);
|
||||
if tool_call_lag > guardian_config.max_tool_call_lag {
|
||||
thread_store.insert(StrictReviewReason::StaleScore);
|
||||
return None;
|
||||
}
|
||||
|
||||
thread_store
|
||||
let score = thread_store
|
||||
.get::<SecurityRiskScore>()
|
||||
.and_then(|score| score.scores.get("action_risk").copied())
|
||||
.filter(|score| *score < guardian_config.review_threshold)
|
||||
.map(|_| ReviewDecision::Approved)
|
||||
.and_then(|score| score.scores.get("action_risk").copied())?;
|
||||
if score < guardian_config.review_threshold {
|
||||
return Some(ReviewDecision::Approved);
|
||||
}
|
||||
if score >= guardian_config.review_threshold {
|
||||
thread_store.insert(StrictReviewReason::ElevatedRisk);
|
||||
}
|
||||
None
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -44,6 +44,7 @@ use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
|
||||
use super::GuardianV2ScoreProgress;
|
||||
use super::StrictReviewReason;
|
||||
use super::encrypted_parent_compaction;
|
||||
use crate::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS;
|
||||
use crate::config::DEFAULT_PARENT_COMPACTION_TOKENS;
|
||||
@@ -477,6 +478,7 @@ max_recent_non_user_entries = 8
|
||||
}
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(thread_store.get::<StrictReviewReason>(), None);
|
||||
thread_store.insert(SecurityRiskScore {
|
||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]),
|
||||
sampled_at: None,
|
||||
@@ -487,6 +489,10 @@ max_recent_non_user_entries = 8
|
||||
.await,
|
||||
None
|
||||
);
|
||||
assert_eq!(
|
||||
thread_store.remove::<StrictReviewReason>().as_deref(),
|
||||
Some(&StrictReviewReason::ElevatedRisk)
|
||||
);
|
||||
thread_store.insert(SecurityRiskScore {
|
||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.55)]),
|
||||
sampled_at: None,
|
||||
@@ -523,6 +529,10 @@ max_recent_non_user_entries = 8
|
||||
.await,
|
||||
None
|
||||
);
|
||||
assert_eq!(
|
||||
thread_store.remove::<StrictReviewReason>().as_deref(),
|
||||
Some(&StrictReviewReason::StaleScore)
|
||||
);
|
||||
|
||||
score_progress
|
||||
.latest_scored_tool_call
|
||||
|
||||
@@ -3,6 +3,7 @@ mod extension;
|
||||
mod sampler;
|
||||
mod transcript;
|
||||
|
||||
pub use extension::StrictReviewReason;
|
||||
pub use extension::install;
|
||||
pub use sampler::LunaSampler;
|
||||
pub use sampler::LunaSamplerConfig;
|
||||
|
||||
@@ -90,6 +90,9 @@ pub(super) fn server_notification_thread_target(
|
||||
ServerNotification::ItemGuardianApprovalReviewCompleted(notification) => {
|
||||
Some(notification.thread_id.as_str())
|
||||
}
|
||||
ServerNotification::StrictReviewRequired(notification) => {
|
||||
Some(notification.thread_id.as_str())
|
||||
}
|
||||
ServerNotification::ItemCompleted(notification) => Some(notification.thread_id.as_str()),
|
||||
ServerNotification::RawResponseItemCompleted(notification) => {
|
||||
Some(notification.thread_id.as_str())
|
||||
|
||||
@@ -203,6 +203,7 @@ impl ChatWidget {
|
||||
| ServerNotification::ThreadArchived(_)
|
||||
| ServerNotification::ThreadDeleted(_)
|
||||
| ServerNotification::ThreadUnarchived(_)
|
||||
| ServerNotification::StrictReviewRequired(_)
|
||||
| ServerNotification::RawResponseItemCompleted(_)
|
||||
| ServerNotification::RawResponseCompleted(_)
|
||||
| ServerNotification::CommandExecOutputDelta(_)
|
||||
|
||||
Reference in New Issue
Block a user