From ff9507e7efeea21872a0c1f51db58d07ae4bdf71 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Sat, 18 Jul 2026 15:06:57 -0400 Subject: [PATCH 1/3] fix(rollout): bound deserialization stack usage --- codex-rs/protocol/src/protocol.rs | 127 +++++++++++++++++++++-- codex-rs/rollout/src/list.rs | 42 +++++--- codex-rs/thread-store/src/live_thread.rs | 33 ++++-- 3 files changed, 165 insertions(+), 37 deletions(-) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 114ed7f4ed53..a96996aef6f7 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -3077,13 +3077,6 @@ impl<'de> Deserialize<'de> for SessionMetaLine { where D: Deserializer<'de>, { - #[derive(Deserialize)] - struct SessionMetaLineFields { - #[serde(flatten)] - meta: SessionMeta, - git: Option, - } - let mut value = Value::deserialize(deserializer)?; let fields = value .as_object_mut() @@ -3095,13 +3088,17 @@ impl<'de> Deserialize<'de> for SessionMetaLine { .ok_or_else(|| D::Error::missing_field("id"))?; fields.insert("session_id".to_string(), thread_id); } - let SessionMetaLineFields { meta, git } = - serde_json::from_value(value).map_err(D::Error::custom)?; + let git = fields + .remove("git") + .map(serde_json::from_value) + .transpose() + .map_err(D::Error::custom)?; + let meta = serde_json::from_value(value).map_err(D::Error::custom)?; Ok(Self { meta, git }) } } -#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema, TS)] +#[derive(Serialize, Debug, Clone, JsonSchema, TS)] #[serde(tag = "type", content = "payload", rename_all = "snake_case")] pub enum RolloutItem { SessionMeta(SessionMetaLine), @@ -3111,6 +3108,52 @@ pub enum RolloutItem { EventMsg(EventMsg), } +impl<'de> Deserialize<'de> for RolloutItem { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let mut value = Value::deserialize(deserializer)?; + let fields = value + .as_object_mut() + .ok_or_else(|| D::Error::custom("rollout item must be an object"))?; + let item_type = fields + .remove("type") + .ok_or_else(|| D::Error::missing_field("type")) + .and_then(|value| serde_json::from_value::(value).map_err(D::Error::custom))?; + let payload = fields + .remove("payload") + .ok_or_else(|| D::Error::missing_field("payload"))?; + match item_type.as_str() { + "session_meta" => serde_json::from_value(payload) + .map(Self::SessionMeta) + .map_err(D::Error::custom), + "response_item" => serde_json::from_value(payload) + .map(Self::ResponseItem) + .map_err(D::Error::custom), + "compacted" => serde_json::from_value(payload) + .map(Self::Compacted) + .map_err(D::Error::custom), + "turn_context" => serde_json::from_value(payload) + .map(Self::TurnContext) + .map_err(D::Error::custom), + "event_msg" => serde_json::from_value(payload) + .map(Self::EventMsg) + .map_err(D::Error::custom), + _ => Err(D::Error::unknown_variant( + &item_type, + &[ + "session_meta", + "response_item", + "compacted", + "turn_context", + "event_msg", + ], + )), + } + } +} + #[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, TS)] pub struct CompactedItem { pub message: String, @@ -3256,7 +3299,7 @@ impl Mul for TruncationPolicy { } } -#[derive(Serialize, Deserialize, Clone, JsonSchema)] +#[derive(Serialize, Clone, JsonSchema)] pub struct RolloutLine { pub timestamp: String, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -3265,6 +3308,33 @@ pub struct RolloutLine { pub item: RolloutItem, } +impl<'de> Deserialize<'de> for RolloutLine { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let mut value = Value::deserialize(deserializer)?; + let fields = value + .as_object_mut() + .ok_or_else(|| D::Error::custom("rollout line must be an object"))?; + let timestamp = fields + .remove("timestamp") + .ok_or_else(|| D::Error::missing_field("timestamp")) + .and_then(|value| serde_json::from_value(value).map_err(D::Error::custom))?; + let ordinal = fields + .remove("ordinal") + .map(serde_json::from_value) + .transpose() + .map_err(D::Error::custom)?; + let item = serde_json::from_value(value).map_err(D::Error::custom)?; + Ok(Self { + timestamp, + ordinal, + item, + }) + } +} + #[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, TS)] pub struct GitInfo { /// Current commit hash (SHA) @@ -5555,6 +5625,41 @@ mod tests { Ok(()) } + #[test] + fn rollout_line_deserializes_flattened_session_meta() -> Result<()> { + let thread_id = ThreadId::from_string("67e55044-10b1-426f-9247-bb680e5fe0c8")?; + let line: RolloutLine = serde_json::from_value(json!({ + "timestamp": "2026-07-04T00:00:00Z", + "ordinal": 7, + "type": "session_meta", + "payload": { + "id": thread_id, + "timestamp": "2026-07-04T00:00:00Z", + "instructions": null, + "cwd": test_path_buf("/home/user/project"), + "originator": "codex_cli_rs", + "cli_version": "0.0.0", + "source": "exec", + "git": { + "branch": "main" + } + } + }))?; + + assert_eq!(line.timestamp, "2026-07-04T00:00:00Z"); + assert_eq!(line.ordinal, Some(7)); + let RolloutItem::SessionMeta(meta_line) = line.item else { + anyhow::bail!("expected session_meta rollout item"); + }; + assert_eq!(meta_line.meta.id, thread_id); + assert_eq!(meta_line.meta.session_id, thread_id.into()); + assert_eq!( + meta_line.git.and_then(|git| git.branch).as_deref(), + Some("main") + ); + Ok(()) + } + #[test] fn turn_context_item_serializes_network_when_present() -> Result<()> { let item = TurnContextItem { diff --git a/codex-rs/rollout/src/list.rs b/codex-rs/rollout/src/list.rs index 6e5af96b6194..ebc192376ca9 100644 --- a/codex-rs/rollout/src/list.rs +++ b/codex-rs/rollout/src/list.rs @@ -24,6 +24,7 @@ use crate::state_db; use codex_file_search as file_search; use codex_protocol::ThreadId; use codex_protocol::items::TurnItem; +use codex_protocol::models::ResponseItem; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; use codex_protocol::protocol::SessionMetaLine; @@ -1185,22 +1186,33 @@ pub async fn read_head_for_summary(path: &Path) -> io::Result(trimmed) { - match rollout_line.item { - RolloutItem::SessionMeta(session_meta_line) => { - if let Ok(value) = serde_json::to_value(session_meta_line) { - head.push(value); - } - } - RolloutItem::ResponseItem(item) => { - if let Ok(value) = serde_json::to_value(item) { - head.push(value); - } - } - RolloutItem::Compacted(_) - | RolloutItem::TurnContext(_) - | RolloutItem::EventMsg(_) => {} + let Ok(mut value) = serde_json::from_str::(trimmed) else { + continue; + }; + let Some(fields) = value.as_object_mut() else { + continue; + }; + let Some(item_type) = fields + .get("type") + .and_then(serde_json::Value::as_str) + .map(str::to_owned) + else { + continue; + }; + let Some(payload) = fields.remove("payload") else { + continue; + }; + let normalized = match item_type.as_str() { + "session_meta" => { + serde_json::from_value::(payload).and_then(serde_json::to_value) + } + "response_item" => { + serde_json::from_value::(payload).and_then(serde_json::to_value) } + _ => continue, + }; + if let Ok(value) = normalized { + head.push(value); } } diff --git a/codex-rs/thread-store/src/live_thread.rs b/codex-rs/thread-store/src/live_thread.rs index 13e6a70bc8cb..cef2363c382c 100644 --- a/codex-rs/thread-store/src/live_thread.rs +++ b/codex-rs/thread-store/src/live_thread.rs @@ -19,6 +19,7 @@ use crate::StoredThread; use crate::StoredThreadHistory; use crate::ThreadMetadataPatch; use crate::ThreadStore; +use crate::ThreadStoreError; use crate::ThreadStoreResult; use crate::UpdateThreadMetadataParams; use crate::thread_metadata_sync::ThreadMetadataSync; @@ -152,17 +153,27 @@ impl LiveThread { .await .observe_appended_items(persisted_items.as_slice()); if let Some(update) = update { - self.thread_store - .update_thread_metadata(UpdateThreadMetadataParams { - thread_id: self.thread_id, - patch: update.patch.clone(), - include_archived: true, - }) - .await?; - self.metadata_sync - .lock() - .await - .mark_pending_update_applied(&update); + let thread_store = Arc::clone(&self.thread_store); + let metadata_sync = Arc::clone(&self.metadata_sync); + let thread_id = self.thread_id; + tokio::spawn(async move { + thread_store + .update_thread_metadata(UpdateThreadMetadataParams { + thread_id, + patch: update.patch.clone(), + include_archived: true, + }) + .await?; + metadata_sync + .lock() + .await + .mark_pending_update_applied(&update); + ThreadStoreResult::Ok(()) + }) + .await + .map_err(|err| ThreadStoreError::Internal { + message: format!("thread metadata update task failed: {err}"), + })??; } Ok(()) } From 2ce3d02e1b5e6d7ef336a3043587b6d6da1399e8 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Sat, 18 Jul 2026 15:07:14 -0400 Subject: [PATCH 2/3] feat(auto-review): bound background review execution --- .../schema/json/ClientRequest.json | 58 ++ .../codex_app_server_protocol.schemas.json | 250 ++++++- .../codex_app_server_protocol.v2.schemas.json | 250 ++++++- .../v2/AutoReviewDispositionWriteParams.json | 38 + .../AutoReviewDispositionWriteResponse.json | 62 ++ .../v2/AutoReviewSummaryReadResponse.json | 173 ++++- .../schema/typescript/ClientRequest.ts | 3 +- .../schema/typescript/v2/AutoReviewBudget.ts | 5 + .../v2/AutoReviewDispositionAction.ts | 5 + .../v2/AutoReviewDispositionActor.ts | 5 + .../v2/AutoReviewDispositionWriteParams.ts | 6 + .../v2/AutoReviewDispositionWriteResponse.ts | 6 + .../v2/AutoReviewFindingDisposition.ts | 5 + .../v2/AutoReviewFindingDispositionRecord.ts | 7 + .../typescript/v2/AutoReviewRunSummary.ts | 6 +- .../typescript/v2/AutoReviewTerminalReason.ts | 5 + .../schema/typescript/v2/AutoReviewUsage.ts | 5 + .../schema/typescript/v2/index.ts | 9 + .../src/protocol/common.rs | 5 + .../src/protocol/v2/review.rs | 102 +++ codex-rs/app-server/README.md | 3 + codex-rs/app-server/src/message_processor.rs | 5 + codex-rs/app-server/src/request_processors.rs | 9 + .../src/request_processors/turn_processor.rs | 202 +++++- .../tests/common/test_app_server.rs | 10 + codex-rs/app-server/tests/suite/v2/review.rs | 158 ++++ codex-rs/auto-review/src/lib.rs | 301 +++++++- codex-rs/auto-review/src/lib_tests.rs | 90 +++ codex-rs/config/src/config_toml.rs | 8 + codex-rs/core/config.schema.json | 24 + codex-rs/core/src/config/config_tests.rs | 42 +- codex-rs/core/src/config/mod.rs | 56 +- .../core/src/context/auto_review_awareness.rs | 88 ++- codex-rs/core/src/review_persistence.rs | 680 +++++++++++++++++- .../src/session/background_auto_review.rs | 145 ++-- codex-rs/core/src/session/review.rs | 10 +- codex-rs/core/src/tasks/review.rs | 565 +++++++++++++-- .../tools/handlers/auto_review_disposition.rs | 287 ++++++++ .../handlers/auto_review_disposition_spec.rs | 48 ++ codex-rs/core/src/tools/handlers/mod.rs | 3 + codex-rs/core/src/tools/spec_plan.rs | 4 + codex-rs/core/src/tools/spec_plan_tests.rs | 17 + codex-rs/core/tests/suite/review.rs | 225 +++++- codex-rs/tui/src/app.rs | 12 + codex-rs/tui/src/app/app_server_events.rs | 4 +- codex-rs/tui/src/app/tests.rs | 38 + codex-rs/tui/src/app/thread_routing.rs | 2 +- .../tui/src/chatwidget/tests/app_server.rs | 4 + .../src/history_cell/auto_review_status.rs | 98 +++ codex-rs/tui/src/history_cell/tests.rs | 78 ++ 50 files changed, 4065 insertions(+), 156 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteParams.json create mode 100644 codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteResponse.json create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewBudget.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionAction.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionActor.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteParams.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteResponse.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDisposition.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDispositionRecord.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewTerminalReason.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewUsage.ts create mode 100644 codex-rs/core/src/tools/handlers/auto_review_disposition.rs create mode 100644 codex-rs/core/src/tools/handlers/auto_review_disposition_spec.rs diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index dfd105463971..59ce7a4ebe3b 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -150,6 +150,40 @@ } ] }, + "AutoReviewDispositionAction": { + "enum": [ + "repair", + "defer", + "obsolete" + ], + "type": "string" + }, + "AutoReviewDispositionWriteParams": { + "properties": { + "action": { + "$ref": "#/definitions/AutoReviewDispositionAction" + }, + "reason": { + "default": null, + "type": [ + "string", + "null" + ] + }, + "runId": { + "type": "string" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "action", + "runId", + "threadId" + ], + "type": "object" + }, "AutoReviewFindingDetailReadParams": { "properties": { "findingId": { @@ -5943,6 +5977,30 @@ "title": "Review/findingDetail/readRequest", "type": "object" }, + { + "properties": { + "id": { + "$ref": "#/definitions/RequestId" + }, + "method": { + "enum": [ + "review/disposition/write" + ], + "title": "Review/disposition/writeRequestMethod", + "type": "string" + }, + "params": { + "$ref": "#/definitions/AutoReviewDispositionWriteParams" + } + }, + "required": [ + "id", + "method", + "params" + ], + "title": "Review/disposition/writeRequest", + "type": "object" + }, { "properties": { "id": { 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 2878eda47966..ffb0920e4712 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 @@ -1525,6 +1525,30 @@ "title": "Review/findingDetail/readRequest", "type": "object" }, + { + "properties": { + "id": { + "$ref": "#/definitions/v2/RequestId" + }, + "method": { + "enum": [ + "review/disposition/write" + ], + "title": "Review/disposition/writeRequestMethod", + "type": "string" + }, + "params": { + "$ref": "#/definitions/v2/AutoReviewDispositionWriteParams" + } + }, + "required": [ + "id", + "method", + "params" + ], + "title": "Review/disposition/writeRequest", + "type": "object" + }, { "properties": { "id": { @@ -6938,6 +6962,43 @@ } ] }, + "AutoReviewBudget": { + "properties": { + "maxElapsedMs": { + "format": "uint64", + "minimum": 0.0, + "type": "integer" + }, + "maxFindings": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxOutputBytes": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxScopeBytes": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxTotalTokens": { + "format": "uint64", + "minimum": 0.0, + "type": "integer" + } + }, + "required": [ + "maxElapsedMs", + "maxFindings", + "maxOutputBytes", + "maxScopeBytes", + "maxTotalTokens" + ], + "type": "object" + }, "AutoReviewDecisionSource": { "description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.", "enum": [ @@ -7023,6 +7084,67 @@ ], "type": "object" }, + "AutoReviewDispositionAction": { + "enum": [ + "repair", + "defer", + "obsolete" + ], + "type": "string" + }, + "AutoReviewDispositionActor": { + "enum": [ + "user", + "agent", + "system" + ], + "type": "string" + }, + "AutoReviewDispositionWriteParams": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "action": { + "$ref": "#/definitions/v2/AutoReviewDispositionAction" + }, + "reason": { + "default": null, + "type": [ + "string", + "null" + ] + }, + "runId": { + "type": "string" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "action", + "runId", + "threadId" + ], + "title": "AutoReviewDispositionWriteParams", + "type": "object" + }, + "AutoReviewDispositionWriteResponse": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "findingDisposition": { + "$ref": "#/definitions/v2/AutoReviewFindingDispositionRecord" + }, + "runId": { + "type": "string" + } + }, + "required": [ + "findingDisposition", + "runId" + ], + "title": "AutoReviewDispositionWriteResponse", + "type": "object" + }, "AutoReviewFindingDetailReadParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -7117,6 +7239,41 @@ "title": "AutoReviewFindingDetailReadResponse", "type": "object" }, + "AutoReviewFindingDisposition": { + "enum": [ + "needsAttention", + "repairing", + "deferred", + "obsolete" + ], + "type": "string" + }, + "AutoReviewFindingDispositionRecord": { + "properties": { + "actor": { + "$ref": "#/definitions/v2/AutoReviewDispositionActor" + }, + "disposition": { + "$ref": "#/definitions/v2/AutoReviewFindingDisposition" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "updatedAt": { + "format": "int64", + "type": "integer" + } + }, + "required": [ + "actor", + "disposition", + "updatedAt" + ], + "type": "object" + }, "AutoReviewFreshness": { "enum": [ "current", @@ -7134,6 +7291,16 @@ }, "AutoReviewRunSummary": { "properties": { + "budget": { + "anyOf": [ + { + "$ref": "#/definitions/v2/AutoReviewBudget" + }, + { + "type": "null" + } + ] + }, "completedAt": { "format": "int64", "type": [ @@ -7150,6 +7317,16 @@ "null" ] }, + "findingDisposition": { + "anyOf": [ + { + "$ref": "#/definitions/v2/AutoReviewFindingDispositionRecord" + }, + { + "type": "null" + } + ] + }, "freshness": { "$ref": "#/definitions/v2/AutoReviewFreshness" }, @@ -7182,8 +7359,21 @@ "status": { "$ref": "#/definitions/v2/BackgroundAutoReviewStatus" }, + "terminalReason": { + "anyOf": [ + { + "$ref": "#/definitions/v2/AutoReviewTerminalReason" + }, + { + "type": "null" + } + ] + }, "truncated": { "type": "boolean" + }, + "usage": { + "$ref": "#/definitions/v2/AutoReviewUsage" } }, "required": [ @@ -7195,7 +7385,8 @@ "source", "startedAt", "status", - "truncated" + "truncated", + "usage" ], "type": "object" }, @@ -7287,6 +7478,63 @@ "title": "AutoReviewSummaryReadResponse", "type": "object" }, + "AutoReviewTerminalReason": { + "enum": [ + "budgetScope", + "budgetElapsed", + "budgetTotalTokens", + "budgetOutput", + "budgetFindingCount", + "emptyOutput", + "staleTarget" + ], + "type": "string" + }, + "AutoReviewUsage": { + "properties": { + "elapsedMs": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "findingCount": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "outputBytes": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "scopeBytes": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "totalTokens": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + } + }, + "type": "object" + }, "BackgroundAutoReviewControlAction": { "enum": [ "cancel", 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 866b0ae394f8..4559a8b9bd7f 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 @@ -1077,6 +1077,43 @@ } ] }, + "AutoReviewBudget": { + "properties": { + "maxElapsedMs": { + "format": "uint64", + "minimum": 0.0, + "type": "integer" + }, + "maxFindings": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxOutputBytes": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxScopeBytes": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxTotalTokens": { + "format": "uint64", + "minimum": 0.0, + "type": "integer" + } + }, + "required": [ + "maxElapsedMs", + "maxFindings", + "maxOutputBytes", + "maxScopeBytes", + "maxTotalTokens" + ], + "type": "object" + }, "AutoReviewDecisionSource": { "description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.", "enum": [ @@ -1162,6 +1199,67 @@ ], "type": "object" }, + "AutoReviewDispositionAction": { + "enum": [ + "repair", + "defer", + "obsolete" + ], + "type": "string" + }, + "AutoReviewDispositionActor": { + "enum": [ + "user", + "agent", + "system" + ], + "type": "string" + }, + "AutoReviewDispositionWriteParams": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "action": { + "$ref": "#/definitions/AutoReviewDispositionAction" + }, + "reason": { + "default": null, + "type": [ + "string", + "null" + ] + }, + "runId": { + "type": "string" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "action", + "runId", + "threadId" + ], + "title": "AutoReviewDispositionWriteParams", + "type": "object" + }, + "AutoReviewDispositionWriteResponse": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "findingDisposition": { + "$ref": "#/definitions/AutoReviewFindingDispositionRecord" + }, + "runId": { + "type": "string" + } + }, + "required": [ + "findingDisposition", + "runId" + ], + "title": "AutoReviewDispositionWriteResponse", + "type": "object" + }, "AutoReviewFindingDetailReadParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -1256,6 +1354,41 @@ "title": "AutoReviewFindingDetailReadResponse", "type": "object" }, + "AutoReviewFindingDisposition": { + "enum": [ + "needsAttention", + "repairing", + "deferred", + "obsolete" + ], + "type": "string" + }, + "AutoReviewFindingDispositionRecord": { + "properties": { + "actor": { + "$ref": "#/definitions/AutoReviewDispositionActor" + }, + "disposition": { + "$ref": "#/definitions/AutoReviewFindingDisposition" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "updatedAt": { + "format": "int64", + "type": "integer" + } + }, + "required": [ + "actor", + "disposition", + "updatedAt" + ], + "type": "object" + }, "AutoReviewFreshness": { "enum": [ "current", @@ -1273,6 +1406,16 @@ }, "AutoReviewRunSummary": { "properties": { + "budget": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewBudget" + }, + { + "type": "null" + } + ] + }, "completedAt": { "format": "int64", "type": [ @@ -1289,6 +1432,16 @@ "null" ] }, + "findingDisposition": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewFindingDispositionRecord" + }, + { + "type": "null" + } + ] + }, "freshness": { "$ref": "#/definitions/AutoReviewFreshness" }, @@ -1321,8 +1474,21 @@ "status": { "$ref": "#/definitions/BackgroundAutoReviewStatus" }, + "terminalReason": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewTerminalReason" + }, + { + "type": "null" + } + ] + }, "truncated": { "type": "boolean" + }, + "usage": { + "$ref": "#/definitions/AutoReviewUsage" } }, "required": [ @@ -1334,7 +1500,8 @@ "source", "startedAt", "status", - "truncated" + "truncated", + "usage" ], "type": "object" }, @@ -1426,6 +1593,63 @@ "title": "AutoReviewSummaryReadResponse", "type": "object" }, + "AutoReviewTerminalReason": { + "enum": [ + "budgetScope", + "budgetElapsed", + "budgetTotalTokens", + "budgetOutput", + "budgetFindingCount", + "emptyOutput", + "staleTarget" + ], + "type": "string" + }, + "AutoReviewUsage": { + "properties": { + "elapsedMs": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "findingCount": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "outputBytes": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "scopeBytes": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "totalTokens": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + } + }, + "type": "object" + }, "BackgroundAutoReviewControlAction": { "enum": [ "cancel", @@ -2977,6 +3201,30 @@ "title": "Review/findingDetail/readRequest", "type": "object" }, + { + "properties": { + "id": { + "$ref": "#/definitions/RequestId" + }, + "method": { + "enum": [ + "review/disposition/write" + ], + "title": "Review/disposition/writeRequestMethod", + "type": "string" + }, + "params": { + "$ref": "#/definitions/AutoReviewDispositionWriteParams" + } + }, + "required": [ + "id", + "method", + "params" + ], + "title": "Review/disposition/writeRequest", + "type": "object" + }, { "properties": { "id": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteParams.json b/codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteParams.json new file mode 100644 index 000000000000..a0fec38987c2 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteParams.json @@ -0,0 +1,38 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "AutoReviewDispositionAction": { + "enum": [ + "repair", + "defer", + "obsolete" + ], + "type": "string" + } + }, + "properties": { + "action": { + "$ref": "#/definitions/AutoReviewDispositionAction" + }, + "reason": { + "default": null, + "type": [ + "string", + "null" + ] + }, + "runId": { + "type": "string" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "action", + "runId", + "threadId" + ], + "title": "AutoReviewDispositionWriteParams", + "type": "object" +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteResponse.json b/codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteResponse.json new file mode 100644 index 000000000000..e16774214c61 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/json/v2/AutoReviewDispositionWriteResponse.json @@ -0,0 +1,62 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "AutoReviewDispositionActor": { + "enum": [ + "user", + "agent", + "system" + ], + "type": "string" + }, + "AutoReviewFindingDisposition": { + "enum": [ + "needsAttention", + "repairing", + "deferred", + "obsolete" + ], + "type": "string" + }, + "AutoReviewFindingDispositionRecord": { + "properties": { + "actor": { + "$ref": "#/definitions/AutoReviewDispositionActor" + }, + "disposition": { + "$ref": "#/definitions/AutoReviewFindingDisposition" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "updatedAt": { + "format": "int64", + "type": "integer" + } + }, + "required": [ + "actor", + "disposition", + "updatedAt" + ], + "type": "object" + } + }, + "properties": { + "findingDisposition": { + "$ref": "#/definitions/AutoReviewFindingDispositionRecord" + }, + "runId": { + "type": "string" + } + }, + "required": [ + "findingDisposition", + "runId" + ], + "title": "AutoReviewDispositionWriteResponse", + "type": "object" +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/json/v2/AutoReviewSummaryReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/AutoReviewSummaryReadResponse.json index 51094b586774..169ea46ca961 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/AutoReviewSummaryReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/AutoReviewSummaryReadResponse.json @@ -1,6 +1,43 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "AutoReviewBudget": { + "properties": { + "maxElapsedMs": { + "format": "uint64", + "minimum": 0.0, + "type": "integer" + }, + "maxFindings": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxOutputBytes": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxScopeBytes": { + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "maxTotalTokens": { + "format": "uint64", + "minimum": 0.0, + "type": "integer" + } + }, + "required": [ + "maxElapsedMs", + "maxFindings", + "maxOutputBytes", + "maxScopeBytes", + "maxTotalTokens" + ], + "type": "object" + }, "AutoReviewDiagnosticsSummary": { "properties": { "cancelledRuns": { @@ -72,6 +109,49 @@ ], "type": "object" }, + "AutoReviewDispositionActor": { + "enum": [ + "user", + "agent", + "system" + ], + "type": "string" + }, + "AutoReviewFindingDisposition": { + "enum": [ + "needsAttention", + "repairing", + "deferred", + "obsolete" + ], + "type": "string" + }, + "AutoReviewFindingDispositionRecord": { + "properties": { + "actor": { + "$ref": "#/definitions/AutoReviewDispositionActor" + }, + "disposition": { + "$ref": "#/definitions/AutoReviewFindingDisposition" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "updatedAt": { + "format": "int64", + "type": "integer" + } + }, + "required": [ + "actor", + "disposition", + "updatedAt" + ], + "type": "object" + }, "AutoReviewFreshness": { "enum": [ "current", @@ -89,6 +169,16 @@ }, "AutoReviewRunSummary": { "properties": { + "budget": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewBudget" + }, + { + "type": "null" + } + ] + }, "completedAt": { "format": "int64", "type": [ @@ -105,6 +195,16 @@ "null" ] }, + "findingDisposition": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewFindingDispositionRecord" + }, + { + "type": "null" + } + ] + }, "freshness": { "$ref": "#/definitions/AutoReviewFreshness" }, @@ -137,8 +237,21 @@ "status": { "$ref": "#/definitions/BackgroundAutoReviewStatus" }, + "terminalReason": { + "anyOf": [ + { + "$ref": "#/definitions/AutoReviewTerminalReason" + }, + { + "type": "null" + } + ] + }, "truncated": { "type": "boolean" + }, + "usage": { + "$ref": "#/definitions/AutoReviewUsage" } }, "required": [ @@ -150,7 +263,8 @@ "source", "startedAt", "status", - "truncated" + "truncated", + "usage" ], "type": "object" }, @@ -183,6 +297,63 @@ ], "type": "object" }, + "AutoReviewTerminalReason": { + "enum": [ + "budgetScope", + "budgetElapsed", + "budgetTotalTokens", + "budgetOutput", + "budgetFindingCount", + "emptyOutput", + "staleTarget" + ], + "type": "string" + }, + "AutoReviewUsage": { + "properties": { + "elapsedMs": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "findingCount": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "outputBytes": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "scopeBytes": { + "format": "uint", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "totalTokens": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + } + }, + "type": "object" + }, "BackgroundAutoReviewStatus": { "enum": [ "pending", diff --git a/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts b/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts index 69c2904e538e..0ffcb17e3111 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts @@ -8,6 +8,7 @@ import type { GitDiffToRemoteParams } from "./GitDiffToRemoteParams"; import type { InitializeParams } from "./InitializeParams"; import type { RequestId } from "./RequestId"; import type { AppsListParams } from "./v2/AppsListParams"; +import type { AutoReviewDispositionWriteParams } from "./v2/AutoReviewDispositionWriteParams"; import type { AutoReviewFindingDetailReadParams } from "./v2/AutoReviewFindingDetailReadParams"; import type { AutoReviewSummaryReadParams } from "./v2/AutoReviewSummaryReadParams"; import type { BackgroundAutoReviewControlParams } from "./v2/BackgroundAutoReviewControlParams"; @@ -91,4 +92,4 @@ import type { WindowsSandboxSetupStartParams } from "./v2/WindowsSandboxSetupSta /** * Request from the client to the server. */ -export type ClientRequest ={ "method": "initialize", id: RequestId, params: InitializeParams, } | { "method": "thread/start", id: RequestId, params: ThreadStartParams, } | { "method": "thread/resume", id: RequestId, params: ThreadResumeParams, } | { "method": "thread/fork", id: RequestId, params: ThreadForkParams, } | { "method": "thread/archive", id: RequestId, params: ThreadArchiveParams, } | { "method": "thread/unsubscribe", id: RequestId, params: ThreadUnsubscribeParams, } | { "method": "thread/name/set", id: RequestId, params: ThreadSetNameParams, } | { "method": "thread/goal/set", id: RequestId, params: ThreadGoalSetParams, } | { "method": "thread/goal/get", id: RequestId, params: ThreadGoalGetParams, } | { "method": "thread/goal/clear", id: RequestId, params: ThreadGoalClearParams, } | { "method": "thread/metadata/update", id: RequestId, params: ThreadMetadataUpdateParams, } | { "method": "thread/unarchive", id: RequestId, params: ThreadUnarchiveParams, } | { "method": "thread/compact/start", id: RequestId, params: ThreadCompactStartParams, } | { "method": "thread/shellCommand", id: RequestId, params: ThreadShellCommandParams, } | { "method": "thread/approveGuardianDeniedAction", id: RequestId, params: ThreadApproveGuardianDeniedActionParams, } | { "method": "thread/rollback", id: RequestId, params: ThreadRollbackParams, } | { "method": "thread/list", id: RequestId, params: ThreadListParams, } | { "method": "thread/loaded/list", id: RequestId, params: ThreadLoadedListParams, } | { "method": "thread/read", id: RequestId, params: ThreadReadParams, } | { "method": "thread/inject_items", id: RequestId, params: ThreadInjectItemsParams, } | { "method": "skills/list", id: RequestId, params: SkillsListParams, } | { "method": "skills/extraRoots/set", id: RequestId, params: SkillsExtraRootsSetParams, } | { "method": "hooks/list", id: RequestId, params: HooksListParams, } | { "method": "marketplace/add", id: RequestId, params: MarketplaceAddParams, } | { "method": "marketplace/remove", id: RequestId, params: MarketplaceRemoveParams, } | { "method": "marketplace/upgrade", id: RequestId, params: MarketplaceUpgradeParams, } | { "method": "plugin/list", id: RequestId, params: PluginListParams, } | { "method": "plugin/installed", id: RequestId, params: PluginInstalledParams, } | { "method": "plugin/read", id: RequestId, params: PluginReadParams, } | { "method": "plugin/skill/read", id: RequestId, params: PluginSkillReadParams, } | { "method": "plugin/share/save", id: RequestId, params: PluginShareSaveParams, } | { "method": "plugin/share/updateTargets", id: RequestId, params: PluginShareUpdateTargetsParams, } | { "method": "plugin/share/list", id: RequestId, params: PluginShareListParams, } | { "method": "plugin/share/checkout", id: RequestId, params: PluginShareCheckoutParams, } | { "method": "plugin/share/delete", id: RequestId, params: PluginShareDeleteParams, } | { "method": "app/list", id: RequestId, params: AppsListParams, } | { "method": "fs/readFile", id: RequestId, params: FsReadFileParams, } | { "method": "fs/writeFile", id: RequestId, params: FsWriteFileParams, } | { "method": "fs/createDirectory", id: RequestId, params: FsCreateDirectoryParams, } | { "method": "fs/getMetadata", id: RequestId, params: FsGetMetadataParams, } | { "method": "fs/readDirectory", id: RequestId, params: FsReadDirectoryParams, } | { "method": "fs/remove", id: RequestId, params: FsRemoveParams, } | { "method": "fs/copy", id: RequestId, params: FsCopyParams, } | { "method": "fs/watch", id: RequestId, params: FsWatchParams, } | { "method": "fs/unwatch", id: RequestId, params: FsUnwatchParams, } | { "method": "skills/config/write", id: RequestId, params: SkillsConfigWriteParams, } | { "method": "plugin/install", id: RequestId, params: PluginInstallParams, } | { "method": "plugin/uninstall", id: RequestId, params: PluginUninstallParams, } | { "method": "turn/start", id: RequestId, params: TurnStartParams, } | { "method": "turn/steer", id: RequestId, params: TurnSteerParams, } | { "method": "turn/interrupt", id: RequestId, params: TurnInterruptParams, } | { "method": "review/start", id: RequestId, params: ReviewStartParams, } | { "method": "review/background/control", id: RequestId, params: BackgroundAutoReviewControlParams, } | { "method": "review/summary/read", id: RequestId, params: AutoReviewSummaryReadParams, } | { "method": "review/findingDetail/read", id: RequestId, params: AutoReviewFindingDetailReadParams, } | { "method": "model/list", id: RequestId, params: ModelListParams, } | { "method": "modelProvider/capabilities/read", id: RequestId, params: ModelProviderCapabilitiesReadParams, } | { "method": "experimentalFeature/list", id: RequestId, params: ExperimentalFeatureListParams, } | { "method": "permissionProfile/list", id: RequestId, params: PermissionProfileListParams, } | { "method": "experimentalFeature/enablement/set", id: RequestId, params: ExperimentalFeatureEnablementSetParams, } | { "method": "mcpServer/oauth/login", id: RequestId, params: McpServerOauthLoginParams, } | { "method": "config/mcpServer/reload", id: RequestId, params: undefined, } | { "method": "mcpServerStatus/list", id: RequestId, params: ListMcpServerStatusParams, } | { "method": "mcpServer/resource/read", id: RequestId, params: McpResourceReadParams, } | { "method": "mcpServer/tool/call", id: RequestId, params: McpServerToolCallParams, } | { "method": "windowsSandbox/setupStart", id: RequestId, params: WindowsSandboxSetupStartParams, } | { "method": "windowsSandbox/readiness", id: RequestId, params: undefined, } | { "method": "account/login/start", id: RequestId, params: LoginAccountParams, } | { "method": "account/login/cancel", id: RequestId, params: CancelLoginAccountParams, } | { "method": "account/switchActive", id: RequestId, params: SwitchActiveAccountParams, } | { "method": "account/list", id: RequestId, params: undefined, } | { "method": "account/remove", id: RequestId, params: RemoveAccountParams, } | { "method": "account/logout", id: RequestId, params: undefined, } | { "method": "account/rateLimits/read", id: RequestId, params: undefined, } | { "method": "account/usage/read", id: RequestId, params: undefined, } | { "method": "account/sendAddCreditsNudgeEmail", id: RequestId, params: SendAddCreditsNudgeEmailParams, } | { "method": "feedback/upload", id: RequestId, params: FeedbackUploadParams, } | { "method": "command/exec", id: RequestId, params: CommandExecParams, } | { "method": "command/exec/write", id: RequestId, params: CommandExecWriteParams, } | { "method": "command/exec/terminate", id: RequestId, params: CommandExecTerminateParams, } | { "method": "command/exec/resize", id: RequestId, params: CommandExecResizeParams, } | { "method": "config/read", id: RequestId, params: ConfigReadParams, } | { "method": "externalAgentConfig/detect", id: RequestId, params: ExternalAgentConfigDetectParams, } | { "method": "externalAgentConfig/import", id: RequestId, params: ExternalAgentConfigImportParams, } | { "method": "config/value/write", id: RequestId, params: ConfigValueWriteParams, } | { "method": "config/batchWrite", id: RequestId, params: ConfigBatchWriteParams, } | { "method": "configRequirements/read", id: RequestId, params: undefined, } | { "method": "account/read", id: RequestId, params: GetAccountParams, } | { "method": "getConversationSummary", id: RequestId, params: GetConversationSummaryParams, } | { "method": "gitDiffToRemote", id: RequestId, params: GitDiffToRemoteParams, } | { "method": "getAuthStatus", id: RequestId, params: GetAuthStatusParams, } | { "method": "fuzzyFileSearch", id: RequestId, params: FuzzyFileSearchParams, }; +export type ClientRequest ={ "method": "initialize", id: RequestId, params: InitializeParams, } | { "method": "thread/start", id: RequestId, params: ThreadStartParams, } | { "method": "thread/resume", id: RequestId, params: ThreadResumeParams, } | { "method": "thread/fork", id: RequestId, params: ThreadForkParams, } | { "method": "thread/archive", id: RequestId, params: ThreadArchiveParams, } | { "method": "thread/unsubscribe", id: RequestId, params: ThreadUnsubscribeParams, } | { "method": "thread/name/set", id: RequestId, params: ThreadSetNameParams, } | { "method": "thread/goal/set", id: RequestId, params: ThreadGoalSetParams, } | { "method": "thread/goal/get", id: RequestId, params: ThreadGoalGetParams, } | { "method": "thread/goal/clear", id: RequestId, params: ThreadGoalClearParams, } | { "method": "thread/metadata/update", id: RequestId, params: ThreadMetadataUpdateParams, } | { "method": "thread/unarchive", id: RequestId, params: ThreadUnarchiveParams, } | { "method": "thread/compact/start", id: RequestId, params: ThreadCompactStartParams, } | { "method": "thread/shellCommand", id: RequestId, params: ThreadShellCommandParams, } | { "method": "thread/approveGuardianDeniedAction", id: RequestId, params: ThreadApproveGuardianDeniedActionParams, } | { "method": "thread/rollback", id: RequestId, params: ThreadRollbackParams, } | { "method": "thread/list", id: RequestId, params: ThreadListParams, } | { "method": "thread/loaded/list", id: RequestId, params: ThreadLoadedListParams, } | { "method": "thread/read", id: RequestId, params: ThreadReadParams, } | { "method": "thread/inject_items", id: RequestId, params: ThreadInjectItemsParams, } | { "method": "skills/list", id: RequestId, params: SkillsListParams, } | { "method": "skills/extraRoots/set", id: RequestId, params: SkillsExtraRootsSetParams, } | { "method": "hooks/list", id: RequestId, params: HooksListParams, } | { "method": "marketplace/add", id: RequestId, params: MarketplaceAddParams, } | { "method": "marketplace/remove", id: RequestId, params: MarketplaceRemoveParams, } | { "method": "marketplace/upgrade", id: RequestId, params: MarketplaceUpgradeParams, } | { "method": "plugin/list", id: RequestId, params: PluginListParams, } | { "method": "plugin/installed", id: RequestId, params: PluginInstalledParams, } | { "method": "plugin/read", id: RequestId, params: PluginReadParams, } | { "method": "plugin/skill/read", id: RequestId, params: PluginSkillReadParams, } | { "method": "plugin/share/save", id: RequestId, params: PluginShareSaveParams, } | { "method": "plugin/share/updateTargets", id: RequestId, params: PluginShareUpdateTargetsParams, } | { "method": "plugin/share/list", id: RequestId, params: PluginShareListParams, } | { "method": "plugin/share/checkout", id: RequestId, params: PluginShareCheckoutParams, } | { "method": "plugin/share/delete", id: RequestId, params: PluginShareDeleteParams, } | { "method": "app/list", id: RequestId, params: AppsListParams, } | { "method": "fs/readFile", id: RequestId, params: FsReadFileParams, } | { "method": "fs/writeFile", id: RequestId, params: FsWriteFileParams, } | { "method": "fs/createDirectory", id: RequestId, params: FsCreateDirectoryParams, } | { "method": "fs/getMetadata", id: RequestId, params: FsGetMetadataParams, } | { "method": "fs/readDirectory", id: RequestId, params: FsReadDirectoryParams, } | { "method": "fs/remove", id: RequestId, params: FsRemoveParams, } | { "method": "fs/copy", id: RequestId, params: FsCopyParams, } | { "method": "fs/watch", id: RequestId, params: FsWatchParams, } | { "method": "fs/unwatch", id: RequestId, params: FsUnwatchParams, } | { "method": "skills/config/write", id: RequestId, params: SkillsConfigWriteParams, } | { "method": "plugin/install", id: RequestId, params: PluginInstallParams, } | { "method": "plugin/uninstall", id: RequestId, params: PluginUninstallParams, } | { "method": "turn/start", id: RequestId, params: TurnStartParams, } | { "method": "turn/steer", id: RequestId, params: TurnSteerParams, } | { "method": "turn/interrupt", id: RequestId, params: TurnInterruptParams, } | { "method": "review/start", id: RequestId, params: ReviewStartParams, } | { "method": "review/background/control", id: RequestId, params: BackgroundAutoReviewControlParams, } | { "method": "review/summary/read", id: RequestId, params: AutoReviewSummaryReadParams, } | { "method": "review/findingDetail/read", id: RequestId, params: AutoReviewFindingDetailReadParams, } | { "method": "review/disposition/write", id: RequestId, params: AutoReviewDispositionWriteParams, } | { "method": "model/list", id: RequestId, params: ModelListParams, } | { "method": "modelProvider/capabilities/read", id: RequestId, params: ModelProviderCapabilitiesReadParams, } | { "method": "experimentalFeature/list", id: RequestId, params: ExperimentalFeatureListParams, } | { "method": "permissionProfile/list", id: RequestId, params: PermissionProfileListParams, } | { "method": "experimentalFeature/enablement/set", id: RequestId, params: ExperimentalFeatureEnablementSetParams, } | { "method": "mcpServer/oauth/login", id: RequestId, params: McpServerOauthLoginParams, } | { "method": "config/mcpServer/reload", id: RequestId, params: undefined, } | { "method": "mcpServerStatus/list", id: RequestId, params: ListMcpServerStatusParams, } | { "method": "mcpServer/resource/read", id: RequestId, params: McpResourceReadParams, } | { "method": "mcpServer/tool/call", id: RequestId, params: McpServerToolCallParams, } | { "method": "windowsSandbox/setupStart", id: RequestId, params: WindowsSandboxSetupStartParams, } | { "method": "windowsSandbox/readiness", id: RequestId, params: undefined, } | { "method": "account/login/start", id: RequestId, params: LoginAccountParams, } | { "method": "account/login/cancel", id: RequestId, params: CancelLoginAccountParams, } | { "method": "account/switchActive", id: RequestId, params: SwitchActiveAccountParams, } | { "method": "account/list", id: RequestId, params: undefined, } | { "method": "account/remove", id: RequestId, params: RemoveAccountParams, } | { "method": "account/logout", id: RequestId, params: undefined, } | { "method": "account/rateLimits/read", id: RequestId, params: undefined, } | { "method": "account/usage/read", id: RequestId, params: undefined, } | { "method": "account/sendAddCreditsNudgeEmail", id: RequestId, params: SendAddCreditsNudgeEmailParams, } | { "method": "feedback/upload", id: RequestId, params: FeedbackUploadParams, } | { "method": "command/exec", id: RequestId, params: CommandExecParams, } | { "method": "command/exec/write", id: RequestId, params: CommandExecWriteParams, } | { "method": "command/exec/terminate", id: RequestId, params: CommandExecTerminateParams, } | { "method": "command/exec/resize", id: RequestId, params: CommandExecResizeParams, } | { "method": "config/read", id: RequestId, params: ConfigReadParams, } | { "method": "externalAgentConfig/detect", id: RequestId, params: ExternalAgentConfigDetectParams, } | { "method": "externalAgentConfig/import", id: RequestId, params: ExternalAgentConfigImportParams, } | { "method": "config/value/write", id: RequestId, params: ConfigValueWriteParams, } | { "method": "config/batchWrite", id: RequestId, params: ConfigBatchWriteParams, } | { "method": "configRequirements/read", id: RequestId, params: undefined, } | { "method": "account/read", id: RequestId, params: GetAccountParams, } | { "method": "getConversationSummary", id: RequestId, params: GetConversationSummaryParams, } | { "method": "gitDiffToRemote", id: RequestId, params: GitDiffToRemoteParams, } | { "method": "getAuthStatus", id: RequestId, params: GetAuthStatusParams, } | { "method": "fuzzyFileSearch", id: RequestId, params: FuzzyFileSearchParams, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewBudget.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewBudget.ts new file mode 100644 index 000000000000..d0bb3e5193b2 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewBudget.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 AutoReviewBudget = { maxScopeBytes: number, maxElapsedMs: number, maxTotalTokens: number, maxOutputBytes: number, maxFindings: number, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionAction.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionAction.ts new file mode 100644 index 000000000000..abd094d3ab60 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionAction.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 AutoReviewDispositionAction = "repair" | "defer" | "obsolete"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionActor.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionActor.ts new file mode 100644 index 000000000000..db39385c3a23 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionActor.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 AutoReviewDispositionActor = "user" | "agent" | "system"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteParams.ts new file mode 100644 index 000000000000..39ffebb0bcf0 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteParams.ts @@ -0,0 +1,6 @@ +// 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. +import type { AutoReviewDispositionAction } from "./AutoReviewDispositionAction"; + +export type AutoReviewDispositionWriteParams = { threadId: string, runId: string, action: AutoReviewDispositionAction, reason?: string | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteResponse.ts new file mode 100644 index 000000000000..2e42cf732dbb --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDispositionWriteResponse.ts @@ -0,0 +1,6 @@ +// 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. +import type { AutoReviewFindingDispositionRecord } from "./AutoReviewFindingDispositionRecord"; + +export type AutoReviewDispositionWriteResponse = { runId: string, findingDisposition: AutoReviewFindingDispositionRecord, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDisposition.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDisposition.ts new file mode 100644 index 000000000000..e7be3100ae9b --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDisposition.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 AutoReviewFindingDisposition = "needsAttention" | "repairing" | "deferred" | "obsolete"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDispositionRecord.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDispositionRecord.ts new file mode 100644 index 000000000000..58c54a9725fb --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewFindingDispositionRecord.ts @@ -0,0 +1,7 @@ +// 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. +import type { AutoReviewDispositionActor } from "./AutoReviewDispositionActor"; +import type { AutoReviewFindingDisposition } from "./AutoReviewFindingDisposition"; + +export type AutoReviewFindingDispositionRecord = { disposition: AutoReviewFindingDisposition, actor: AutoReviewDispositionActor, reason: string | null, updatedAt: number, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRunSummary.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRunSummary.ts index 3b99c0bb72bb..b53f2053232d 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRunSummary.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewRunSummary.ts @@ -1,8 +1,12 @@ // 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. +import type { AutoReviewBudget } from "./AutoReviewBudget"; +import type { AutoReviewFindingDispositionRecord } from "./AutoReviewFindingDispositionRecord"; import type { AutoReviewFreshness } from "./AutoReviewFreshness"; import type { AutoReviewRunSource } from "./AutoReviewRunSource"; +import type { AutoReviewTerminalReason } from "./AutoReviewTerminalReason"; +import type { AutoReviewUsage } from "./AutoReviewUsage"; import type { BackgroundAutoReviewStatus } from "./BackgroundAutoReviewStatus"; -export type AutoReviewRunSummary = { runId: string, status: BackgroundAutoReviewStatus, source: AutoReviewRunSource, freshness: AutoReviewFreshness, startedAt: number, completedAt: number | null, model: string | null, errorSummary: string | null, renderedFindings: number, omittedFindings: number, truncated: boolean, content: string, }; +export type AutoReviewRunSummary = { runId: string, status: BackgroundAutoReviewStatus, source: AutoReviewRunSource, freshness: AutoReviewFreshness, startedAt: number, completedAt: number | null, model: string | null, errorSummary: string | null, renderedFindings: number, omittedFindings: number, truncated: boolean, content: string, budget: AutoReviewBudget | null, usage: AutoReviewUsage, terminalReason: AutoReviewTerminalReason | null, findingDisposition: AutoReviewFindingDispositionRecord | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewTerminalReason.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewTerminalReason.ts new file mode 100644 index 000000000000..09ebe698abee --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewTerminalReason.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 AutoReviewTerminalReason = "budgetScope" | "budgetElapsed" | "budgetTotalTokens" | "budgetOutput" | "budgetFindingCount" | "emptyOutput" | "staleTarget"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewUsage.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewUsage.ts new file mode 100644 index 000000000000..7cb6970b4408 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewUsage.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 AutoReviewUsage = { scopeBytes: number | null, elapsedMs: number | null, totalTokens: number | null, outputBytes: number | null, findingCount: number | 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 0bb8b00f6a1a..9d084c526b1f 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -36,17 +36,26 @@ export type { AppsListResponse } from "./AppsListResponse"; export type { AskForApproval } from "./AskForApproval"; export type { AttestationGenerateParams } from "./AttestationGenerateParams"; export type { AttestationGenerateResponse } from "./AttestationGenerateResponse"; +export type { AutoReviewBudget } from "./AutoReviewBudget"; export type { AutoReviewDecisionSource } from "./AutoReviewDecisionSource"; export type { AutoReviewDetailKind } from "./AutoReviewDetailKind"; export type { AutoReviewDiagnosticsSummary } from "./AutoReviewDiagnosticsSummary"; +export type { AutoReviewDispositionAction } from "./AutoReviewDispositionAction"; +export type { AutoReviewDispositionActor } from "./AutoReviewDispositionActor"; +export type { AutoReviewDispositionWriteParams } from "./AutoReviewDispositionWriteParams"; +export type { AutoReviewDispositionWriteResponse } from "./AutoReviewDispositionWriteResponse"; export type { AutoReviewFindingDetailReadParams } from "./AutoReviewFindingDetailReadParams"; export type { AutoReviewFindingDetailReadResponse } from "./AutoReviewFindingDetailReadResponse"; +export type { AutoReviewFindingDisposition } from "./AutoReviewFindingDisposition"; +export type { AutoReviewFindingDispositionRecord } from "./AutoReviewFindingDispositionRecord"; export type { AutoReviewFreshness } from "./AutoReviewFreshness"; export type { AutoReviewRunSource } from "./AutoReviewRunSource"; export type { AutoReviewRunSummary } from "./AutoReviewRunSummary"; export type { AutoReviewStatusCount } from "./AutoReviewStatusCount"; export type { AutoReviewSummaryReadParams } from "./AutoReviewSummaryReadParams"; export type { AutoReviewSummaryReadResponse } from "./AutoReviewSummaryReadResponse"; +export type { AutoReviewTerminalReason } from "./AutoReviewTerminalReason"; +export type { AutoReviewUsage } from "./AutoReviewUsage"; export type { BackgroundAutoReviewControlAction } from "./BackgroundAutoReviewControlAction"; export type { BackgroundAutoReviewControlParams } from "./BackgroundAutoReviewControlParams"; export type { BackgroundAutoReviewControlReason } from "./BackgroundAutoReviewControlReason"; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 7f02468b6c0b..fe8c5046993d 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -835,6 +835,11 @@ client_request_definitions! { serialization: thread_id(params.thread_id), response: v2::AutoReviewFindingDetailReadResponse, }, + AutoReviewDispositionWrite => "review/disposition/write" { + params: v2::AutoReviewDispositionWriteParams, + serialization: thread_id(params.thread_id), + response: v2::AutoReviewDispositionWriteResponse, + }, ModelList => "model/list" { params: v2::ModelListParams, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/review.rs b/codex-rs/app-server-protocol/src/protocol/v2/review.rs index 3535c362f7a7..b35c7e286b80 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/review.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/review.rs @@ -189,6 +189,79 @@ pub struct AutoReviewRunSummary { pub omitted_findings: usize, pub truncated: bool, pub content: String, + pub budget: Option, + pub usage: AutoReviewUsage, + pub terminal_reason: Option, + pub finding_disposition: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct AutoReviewBudget { + pub max_scope_bytes: usize, + #[ts(type = "number")] + pub max_elapsed_ms: u64, + #[ts(type = "number")] + pub max_total_tokens: u64, + pub max_output_bytes: usize, + pub max_findings: usize, +} + +#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct AutoReviewUsage { + pub scope_bytes: Option, + #[ts(type = "number | null")] + pub elapsed_ms: Option, + #[ts(type = "number | null")] + pub total_tokens: Option, + pub output_bytes: Option, + pub finding_count: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum AutoReviewTerminalReason { + BudgetScope, + BudgetElapsed, + BudgetTotalTokens, + BudgetOutput, + BudgetFindingCount, + EmptyOutput, + StaleTarget, +} + +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum AutoReviewFindingDisposition { + NeedsAttention, + Repairing, + Deferred, + Obsolete, +} + +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum AutoReviewDispositionActor { + User, + Agent, + System, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct AutoReviewFindingDispositionRecord { + pub disposition: AutoReviewFindingDisposition, + pub actor: AutoReviewDispositionActor, + pub reason: Option, + #[ts(type = "number")] + pub updated_at: i64, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] @@ -257,6 +330,35 @@ pub struct AutoReviewFindingDetailReadResponse { pub content: String, } +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum AutoReviewDispositionAction { + Repair, + Defer, + Obsolete, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct AutoReviewDispositionWriteParams { + pub thread_id: String, + pub run_id: String, + pub action: AutoReviewDispositionAction, + #[serde(default)] + #[ts(optional = nullable)] + pub reason: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct AutoReviewDispositionWriteResponse { + pub run_id: String, + pub finding_disposition: AutoReviewFindingDispositionRecord, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(tag = "type", rename_all = "camelCase")] #[ts(tag = "type", export_to = "v2/")] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 14496540656a..b1a99ce27156 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -222,6 +222,9 @@ Example with notification opt-out: - `thread/realtime/stop` — stop the active realtime session for the thread (experimental); returns `{}`. - `review/start` — kick off Codex’s automated reviewer for a thread; responds like `turn/start` and emits `item/started`/`item/completed` notifications with `enteredReviewMode` and `exitedReviewMode` items, plus a final assistant `agentMessage` containing the review. - `review/background/control` — request control of an active background auto-review run by `runId`; supports `cancel` and `supersede`, returns `{}` when accepted, and any status change is emitted through `review/backgroundStatus/changed`. +- `review/summary/read` — read bounded Background Review summaries and diagnostics for a thread, including effective budgets, observed usage, terminal reasons, and finding disposition. +- `review/findingDetail/read` — read bounded persisted detail for a Background Review run or one stable finding id. +- `review/disposition/write` — disposition current Background Review findings as `repair`, `defer`, or `obsolete`; obsolete requires a reason and the response returns the durable disposition record. - `command/exec` — run a single command under the server sandbox without starting a thread/turn (handy for utilities and validation). - `command/exec/write` — write base64-decoded stdin bytes to a running `command/exec` session or close stdin; returns `{}`. - `command/exec/resize` — resize a running PTY-backed `command/exec` session by `processId`; returns `{}`. diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 84054e25a896..f8814688d01a 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -1303,6 +1303,11 @@ impl MessageProcessor { .auto_review_finding_detail_read(params) .await } + ClientRequest::AutoReviewDispositionWrite { params, .. } => { + self.turn_processor + .auto_review_disposition_write(params) + .await + } ClientRequest::McpServerOauthLogin { params, .. } => { self.mcp_processor.mcp_server_oauth_login(params).await } diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index 8fe01e0ee2d6..b2255060176f 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -38,16 +38,25 @@ use codex_app_server_protocol::AppsListParams; use codex_app_server_protocol::AppsListResponse; use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::AuthMode; +use codex_app_server_protocol::AutoReviewBudget as ApiAutoReviewBudget; use codex_app_server_protocol::AutoReviewDetailKind; use codex_app_server_protocol::AutoReviewDiagnosticsSummary; +use codex_app_server_protocol::AutoReviewDispositionAction; +use codex_app_server_protocol::AutoReviewDispositionActor as ApiAutoReviewDispositionActor; +use codex_app_server_protocol::AutoReviewDispositionWriteParams; +use codex_app_server_protocol::AutoReviewDispositionWriteResponse; use codex_app_server_protocol::AutoReviewFindingDetailReadParams; use codex_app_server_protocol::AutoReviewFindingDetailReadResponse; +use codex_app_server_protocol::AutoReviewFindingDisposition as ApiAutoReviewFindingDisposition; +use codex_app_server_protocol::AutoReviewFindingDispositionRecord as ApiAutoReviewFindingDispositionRecord; use codex_app_server_protocol::AutoReviewFreshness as ApiAutoReviewFreshness; use codex_app_server_protocol::AutoReviewRunSource as ApiAutoReviewRunSource; use codex_app_server_protocol::AutoReviewRunSummary; use codex_app_server_protocol::AutoReviewStatusCount; use codex_app_server_protocol::AutoReviewSummaryReadParams; use codex_app_server_protocol::AutoReviewSummaryReadResponse; +use codex_app_server_protocol::AutoReviewTerminalReason as ApiAutoReviewTerminalReason; +use codex_app_server_protocol::AutoReviewUsage as ApiAutoReviewUsage; use codex_app_server_protocol::BackgroundAutoReviewControlParams; use codex_app_server_protocol::BackgroundAutoReviewControlResponse; use codex_app_server_protocol::BackgroundAutoReviewStatus as ApiBackgroundAutoReviewStatus; diff --git a/codex-rs/app-server/src/request_processors/turn_processor.rs b/codex-rs/app-server/src/request_processors/turn_processor.rs index ae7d8259edfe..f204f710eeab 100644 --- a/codex-rs/app-server/src/request_processors/turn_processor.rs +++ b/codex-rs/app-server/src/request_processors/turn_processor.rs @@ -1,6 +1,13 @@ use super::*; use codex_app_server_protocol::BackgroundAutoReviewControlReason as ApiBackgroundAutoReviewControlReason; +use codex_auto_review::AutoReviewBudget as CoreAutoReviewBudget; use codex_auto_review::AutoReviewDiagnostics; +use codex_auto_review::AutoReviewDispositionActor as CoreAutoReviewDispositionActor; +use codex_auto_review::AutoReviewFindingDisposition as CoreAutoReviewFindingDisposition; +use codex_auto_review::AutoReviewFindingDispositionRecord as CoreAutoReviewFindingDispositionRecord; +use codex_auto_review::AutoReviewRunState; +use codex_auto_review::AutoReviewTerminalReason as CoreAutoReviewTerminalReason; +use codex_auto_review::AutoReviewUsage as CoreAutoReviewUsage; use codex_auto_review::ReviewCoordination; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_protocol::protocol::AdditionalContextEntry as CoreAdditionalContextEntry; @@ -239,6 +246,15 @@ impl TurnRequestProcessor { .map(|response| Some(response.into())) } + pub(crate) async fn auto_review_disposition_write( + &self, + params: AutoReviewDispositionWriteParams, + ) -> Result, JSONRPCErrorError> { + self.auto_review_disposition_write_inner(params) + .await + .map(|response| Some(response.into())) + } + fn track_error_response( &self, request_id: &ConnectionRequestId, @@ -443,8 +459,23 @@ impl TurnRequestProcessor { } } - fn auto_review_run_summary(run: &AutoReviewRunProjection) -> AutoReviewRunSummary { + fn auto_review_run_summary( + store: &AutoReviewStore, + run: &AutoReviewRunProjection, + ) -> AutoReviewRunSummary { let summary = &run.summary; + let state = match store.load_run_state(&run.run_id) { + Ok(state) => state, + Err(err) => { + warn!( + run_id = %run.run_id, + error = %err, + "failed to load auto review run state for summary" + ); + None + } + }; + let usage = Self::api_auto_review_usage(run, state.as_ref()); AutoReviewRunSummary { run_id: run.run_id.clone(), status: Self::api_auto_review_status(run.status.clone()), @@ -458,6 +489,106 @@ impl TurnRequestProcessor { omitted_findings: summary.omitted_findings, truncated: summary.truncated, content: summary.content.clone(), + budget: state + .as_ref() + .and_then(|state| state.budget.as_ref()) + .map(Self::api_auto_review_budget), + usage, + terminal_reason: state + .as_ref() + .and_then(|state| state.terminal_reason) + .map(Self::api_auto_review_terminal_reason), + finding_disposition: state + .as_ref() + .and_then(|state| state.finding_disposition.as_ref()) + .map(Self::api_auto_review_finding_disposition_record), + } + } + + fn api_auto_review_budget(budget: &CoreAutoReviewBudget) -> ApiAutoReviewBudget { + ApiAutoReviewBudget { + max_scope_bytes: budget.max_scope_bytes, + max_elapsed_ms: budget.max_elapsed_ms, + max_total_tokens: budget.max_total_tokens, + max_output_bytes: budget.max_output_bytes, + max_findings: budget.max_findings, + } + } + + fn api_auto_review_usage( + run: &AutoReviewRunProjection, + state: Option<&AutoReviewRunState>, + ) -> ApiAutoReviewUsage { + let CoreAutoReviewUsage { + scope_bytes, + mut elapsed_ms, + total_tokens, + output_bytes, + finding_count, + } = state.map(|state| state.usage.clone()).unwrap_or_default(); + if run.status.is_in_flight() { + let started_at_ms = run.started_at_unix_secs.saturating_mul(1_000); + let current_elapsed_ms = chrono::Utc::now() + .timestamp_millis() + .saturating_sub(started_at_ms); + if let Ok(current_elapsed_ms) = u64::try_from(current_elapsed_ms) { + elapsed_ms = Some(elapsed_ms.unwrap_or_default().max(current_elapsed_ms)); + } + } + ApiAutoReviewUsage { + scope_bytes, + elapsed_ms, + total_tokens, + output_bytes, + finding_count, + } + } + + fn api_auto_review_terminal_reason( + reason: CoreAutoReviewTerminalReason, + ) -> ApiAutoReviewTerminalReason { + match reason { + CoreAutoReviewTerminalReason::BudgetScope => ApiAutoReviewTerminalReason::BudgetScope, + CoreAutoReviewTerminalReason::BudgetElapsed => { + ApiAutoReviewTerminalReason::BudgetElapsed + } + CoreAutoReviewTerminalReason::BudgetTotalTokens => { + ApiAutoReviewTerminalReason::BudgetTotalTokens + } + CoreAutoReviewTerminalReason::BudgetOutput => ApiAutoReviewTerminalReason::BudgetOutput, + CoreAutoReviewTerminalReason::BudgetFindingCount => { + ApiAutoReviewTerminalReason::BudgetFindingCount + } + CoreAutoReviewTerminalReason::EmptyOutput => ApiAutoReviewTerminalReason::EmptyOutput, + CoreAutoReviewTerminalReason::StaleTarget => ApiAutoReviewTerminalReason::StaleTarget, + } + } + + fn api_auto_review_finding_disposition_record( + record: &CoreAutoReviewFindingDispositionRecord, + ) -> ApiAutoReviewFindingDispositionRecord { + ApiAutoReviewFindingDispositionRecord { + disposition: match record.disposition { + CoreAutoReviewFindingDisposition::NeedsAttention => { + ApiAutoReviewFindingDisposition::NeedsAttention + } + CoreAutoReviewFindingDisposition::Repairing => { + ApiAutoReviewFindingDisposition::Repairing + } + CoreAutoReviewFindingDisposition::Deferred => { + ApiAutoReviewFindingDisposition::Deferred + } + CoreAutoReviewFindingDisposition::Obsolete => { + ApiAutoReviewFindingDisposition::Obsolete + } + }, + actor: match record.actor { + CoreAutoReviewDispositionActor::User => ApiAutoReviewDispositionActor::User, + CoreAutoReviewDispositionActor::Agent => ApiAutoReviewDispositionActor::Agent, + CoreAutoReviewDispositionActor::System => ApiAutoReviewDispositionActor::System, + }, + reason: record.reason.clone(), + updated_at: record.updated_at_unix_secs, } } @@ -1403,7 +1534,8 @@ impl TurnRequestProcessor { .worktree_path .as_deref() .unwrap_or(self.config.cwd.as_path()); - let runs = AutoReviewStore::for_scope(&self.config.codex_home, store_scope) + let store = AutoReviewStore::for_scope(&self.config.codex_home, store_scope); + let runs = store .list_runs() .map_err(|err| internal_error(format!("failed to list auto review runs: {err}")))?; @@ -1414,11 +1546,11 @@ impl TurnRequestProcessor { latest: projection .latest .as_ref() - .map(Self::auto_review_run_summary), + .map(|run| Self::auto_review_run_summary(&store, run)), current: projection .current .as_ref() - .map(Self::auto_review_run_summary), + .map(|run| Self::auto_review_run_summary(&store, run)), status_counts: projection .status_counts .iter() @@ -1430,6 +1562,68 @@ impl TurnRequestProcessor { }) } + async fn auto_review_disposition_write_inner( + &self, + params: AutoReviewDispositionWriteParams, + ) -> Result { + let AutoReviewDispositionWriteParams { + thread_id, + run_id, + action, + reason, + } = params; + let (_, thread) = self.load_thread(&thread_id).await?; + let active_review_target = CoreReviewTarget::UncommittedChanges; + let active_target = self.auto_review_target_for_thread(thread.as_ref()).await; + let store_scope = active_target + .worktree_path + .as_deref() + .unwrap_or(self.config.cwd.as_path()); + let store = AutoReviewStore::for_scope(&self.config.codex_home, store_scope); + let run = store + .load_run(&run_id) + .map_err(|err| invalid_params(format!("invalid auto review run: {err}")))?; + if !matches!(action, AutoReviewDispositionAction::Obsolete) + && !run.can_read_detail(&active_target, &active_review_target) + { + return Err(invalid_params( + "auto review repair/defer disposition requires current findings".to_string(), + )); + } + let disposition = match action { + AutoReviewDispositionAction::Repair => CoreAutoReviewFindingDisposition::Repairing, + AutoReviewDispositionAction::Defer => CoreAutoReviewFindingDisposition::Deferred, + AutoReviewDispositionAction::Obsolete => CoreAutoReviewFindingDisposition::Obsolete, + }; + if matches!(action, AutoReviewDispositionAction::Repair) { + store + .detail(&run_id, /*finding_id*/ None, /*max_bytes*/ 1) + .map_err(|err| { + invalid_params(format!("auto review repair detail is unavailable: {err}")) + })?; + } + let record = CoreAutoReviewFindingDispositionRecord { + disposition, + actor: CoreAutoReviewDispositionActor::User, + reason: reason + .map(|reason| reason.trim().to_string()) + .filter(|reason| !reason.is_empty()), + updated_at_unix_secs: chrono::Utc::now().timestamp(), + }; + let state = store + .set_finding_disposition(&run_id, record) + .map_err(|err| invalid_params(format!("invalid auto review disposition: {err}")))?; + let finding_disposition = state.finding_disposition.as_ref().ok_or_else(|| { + internal_error("auto review disposition was not persisted".to_string()) + })?; + Ok(AutoReviewDispositionWriteResponse { + run_id, + finding_disposition: Self::api_auto_review_finding_disposition_record( + finding_disposition, + ), + }) + } + async fn auto_review_finding_detail_read_inner( &self, params: AutoReviewFindingDetailReadParams, diff --git a/codex-rs/app-server/tests/common/test_app_server.rs b/codex-rs/app-server/tests/common/test_app_server.rs index 1748bd7eb68a..53a2e37c561f 100644 --- a/codex-rs/app-server/tests/common/test_app_server.rs +++ b/codex-rs/app-server/tests/common/test_app_server.rs @@ -12,6 +12,7 @@ use tokio::process::ChildStdout; use anyhow::Context; use codex_app_server_protocol::AppsListParams; +use codex_app_server_protocol::AutoReviewDispositionWriteParams; use codex_app_server_protocol::AutoReviewFindingDetailReadParams; use codex_app_server_protocol::AutoReviewSummaryReadParams; use codex_app_server_protocol::BackgroundAutoReviewControlParams; @@ -1165,6 +1166,15 @@ impl TestAppServer { self.send_request("review/findingDetail/read", params).await } + /// Send a `review/disposition/write` JSON-RPC request (v2). + pub async fn send_auto_review_disposition_write_request( + &mut self, + params: AutoReviewDispositionWriteParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("review/disposition/write", params).await + } + pub async fn send_windows_sandbox_setup_start_request( &mut self, params: WindowsSandboxSetupStartParams, diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 9f9dbff1ffe0..92198ad4dc4a 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -6,8 +6,13 @@ use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_shell_command_sse_response; use app_test_support::to_response; use codex_app_server_protocol::AutoReviewDetailKind; +use codex_app_server_protocol::AutoReviewDispositionAction; +use codex_app_server_protocol::AutoReviewDispositionActor as ApiAutoReviewDispositionActor; +use codex_app_server_protocol::AutoReviewDispositionWriteParams; +use codex_app_server_protocol::AutoReviewDispositionWriteResponse; use codex_app_server_protocol::AutoReviewFindingDetailReadParams; use codex_app_server_protocol::AutoReviewFindingDetailReadResponse; +use codex_app_server_protocol::AutoReviewFindingDisposition as ApiAutoReviewFindingDisposition; use codex_app_server_protocol::AutoReviewFreshness as ApiAutoReviewFreshness; use codex_app_server_protocol::AutoReviewRunSource as ApiAutoReviewRunSource; use codex_app_server_protocol::AutoReviewSummaryReadParams; @@ -39,11 +44,17 @@ use codex_app_server_protocol::TurnItemsView; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStatus; use codex_app_server_protocol::UserInput as V2UserInput; +use codex_auto_review::AutoReviewBudget; +use codex_auto_review::AutoReviewDispositionActor; +use codex_auto_review::AutoReviewFindingDisposition; +use codex_auto_review::AutoReviewFindingDispositionRecord; use codex_auto_review::AutoReviewRun; use codex_auto_review::AutoReviewRunSource; +use codex_auto_review::AutoReviewRunState; use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewRunTarget; use codex_auto_review::AutoReviewStore; +use codex_auto_review::AutoReviewUsage; use codex_auto_review::ReviewCoordination; use codex_auto_review::SCHEMA_VERSION; use codex_git_utils::collect_git_info; @@ -817,6 +828,153 @@ async fn auto_review_summary_read_returns_current_summary_and_counts() -> Result Ok(()) } +#[tokio::test] +async fn auto_review_disposition_write_updates_durable_attention_state() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = TestAppServer::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_default_thread(&mut mcp).await?; + let thread_cwd = std::fs::canonicalize(codex_home.path())?; + let (run, output) = sample_auto_review_run("run_disposition", &thread_cwd, "Stored body"); + save_auto_review_fixture(codex_home.path(), &thread_cwd, &run, &output)?; + let store = AutoReviewStore::for_scope(codex_home.path(), &thread_cwd); + let mut state = AutoReviewRunState::new(&run.run_id); + state.budget = Some(AutoReviewBudget { + max_scope_bytes: 120_000, + max_elapsed_ms: 300_000, + max_total_tokens: 250_000, + max_output_bytes: 65_536, + max_findings: 20, + }); + state.usage = AutoReviewUsage { + scope_bytes: Some(12_000), + elapsed_ms: Some(5_000), + total_tokens: Some(25_000), + output_bytes: Some(2_000), + finding_count: Some(1), + }; + state.finding_disposition = Some(AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::NeedsAttention, + actor: AutoReviewDispositionActor::System, + reason: None, + updated_at_unix_secs: 2, + }); + store.save_run_state(&state)?; + + let request_id = mcp + .send_auto_review_disposition_write_request(AutoReviewDispositionWriteParams { + thread_id: thread_id.clone(), + run_id: run.run_id.clone(), + action: AutoReviewDispositionAction::Defer, + reason: Some("acknowledged for the next dogfood pass".to_string()), + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let response = to_response::(response)?; + assert_eq!(response.run_id, run.run_id); + assert_eq!( + response.finding_disposition.disposition, + ApiAutoReviewFindingDisposition::Deferred + ); + assert_eq!( + response.finding_disposition.actor, + ApiAutoReviewDispositionActor::User + ); + + let summary_request_id = mcp + .send_auto_review_summary_read_request(AutoReviewSummaryReadParams { thread_id }) + .await?; + let summary_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(summary_request_id)), + ) + .await??; + let summary = to_response::(summary_response)?; + let current = summary.current.expect("current run summary"); + assert_eq!( + current + .budget + .as_ref() + .map(|budget| budget.max_total_tokens), + Some(250_000) + ); + assert_eq!(current.usage.total_tokens, Some(25_000)); + assert_eq!( + current + .finding_disposition + .as_ref() + .map(|record| record.disposition), + Some(ApiAutoReviewFindingDisposition::Deferred) + ); + + Ok(()) +} + +#[tokio::test] +async fn auto_review_repair_disposition_requires_durable_detail() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = TestAppServer::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_default_thread(&mut mcp).await?; + let thread_cwd = std::fs::canonicalize(codex_home.path())?; + let (run, output) = sample_auto_review_run("run_missing_detail", &thread_cwd, "Stored body"); + save_auto_review_fixture(codex_home.path(), &thread_cwd, &run, &output)?; + let store = AutoReviewStore::for_scope(codex_home.path(), &thread_cwd); + let mut state = AutoReviewRunState::new(&run.run_id); + state.finding_disposition = Some(AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::NeedsAttention, + actor: AutoReviewDispositionActor::System, + reason: None, + updated_at_unix_secs: 2, + }); + store.save_run_state(&state)?; + std::fs::remove_file(store.output_path(&run.run_id)?)?; + + let request_id = mcp + .send_auto_review_disposition_write_request(AutoReviewDispositionWriteParams { + thread_id, + run_id: run.run_id.clone(), + action: AutoReviewDispositionAction::Repair, + reason: None, + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert!( + error + .error + .message + .contains("auto review repair detail is unavailable"), + "unexpected message: {}", + error.error.message + ); + let persisted_state = store + .load_run_state(&run.run_id)? + .expect("durable run state"); + assert_eq!( + persisted_state + .finding_disposition + .as_ref() + .map(|record| record.disposition), + Some(AutoReviewFindingDisposition::NeedsAttention) + ); + + Ok(()) +} + #[tokio::test] async fn auto_review_summary_read_returns_duplicate_skip_diagnostics() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; diff --git a/codex-rs/auto-review/src/lib.rs b/codex-rs/auto-review/src/lib.rs index c27a219e4447..b58f2376f912 100644 --- a/codex-rs/auto-review/src/lib.rs +++ b/codex-rs/auto-review/src/lib.rs @@ -2,6 +2,7 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; use std::path::Path; use std::path::PathBuf; +use std::sync::Mutex; use anyhow::Context; use anyhow::Result; @@ -33,11 +34,14 @@ const STATE_DIR: &str = "state"; const REVIEW_DIR: &str = "review"; const RUNS_FILENAME: &str = "runs.json"; const RUN_METADATA_DIR: &str = "run-metadata"; +const RUN_STATES_DIR: &str = "run-states"; const OUTPUTS_DIR: &str = "outputs"; const OMITTED_TEMPLATE_PREFIX: &str = "... "; const OMITTED_TEMPLATE_SUFFIX: &str = " more finding(s) omitted"; const DUPLICATE_AUTO_REVIEW_SCOPE_CANCEL_REASON: &str = "duplicate_auto_review_scope"; +static AUTO_REVIEW_RUN_STATE_WRITE_LOCK: Mutex<()> = Mutex::new(()); + #[derive(Debug, Clone)] pub struct AutoReviewStore { root: PathBuf, @@ -377,6 +381,12 @@ impl AutoReviewStore { "failed to prune stale auto review run metadata" ); } + if let Err(err) = self.prune_run_states_except(&index) { + tracing::warn!( + error = %err, + "failed to prune stale auto review run states" + ); + } Ok(path) } @@ -563,6 +573,54 @@ impl AutoReviewStore { Ok(run) } + pub fn load_run_state(&self, run_id: &str) -> Result> { + validate_safe_id(run_id).context("auto review run_id")?; + self.load_run_state_unlocked(run_id) + } + + pub fn save_run_state(&self, state: &AutoReviewRunState) -> Result { + let _guard = AUTO_REVIEW_RUN_STATE_WRITE_LOCK + .lock() + .unwrap_or_else(|err| err.into_inner()); + self.save_run_state_unlocked(state) + } + + pub fn update_run_state(&self, run_id: &str, update: F) -> Result + where + F: FnOnce(&mut AutoReviewRunState) -> Result<()>, + { + validate_safe_id(run_id).context("auto review run_id")?; + let _guard = AUTO_REVIEW_RUN_STATE_WRITE_LOCK + .lock() + .unwrap_or_else(|err| err.into_inner()); + let mut state = self + .load_run_state_unlocked(run_id)? + .unwrap_or_else(|| AutoReviewRunState::new(run_id)); + update(&mut state)?; + state.validate()?; + self.save_run_state_unlocked(&state)?; + Ok(state) + } + + pub fn set_finding_disposition( + &self, + run_id: &str, + disposition: AutoReviewFindingDispositionRecord, + ) -> Result { + let run = self.load_run(run_id)?; + if run.status != AutoReviewRunStatus::Completed { + anyhow::bail!("auto review run is not completed: {run_id}"); + } + if run.finding_count == 0 { + anyhow::bail!("auto review run has no findings to disposition: {run_id}"); + } + disposition.validate()?; + self.update_run_state(run_id, |state| { + state.finding_disposition = Some(disposition); + Ok(()) + }) + } + pub fn list_runs(&self) -> Result> { let mut runs = self.load_index_for_read()?.runs; runs.sort_by(|left, right| left.run_id.cmp(&right.run_id)); @@ -656,6 +714,36 @@ impl AutoReviewStore { .join(format!("{run_id}.json"))) } + fn run_state_path(&self, run_id: &str) -> Result { + validate_safe_id(run_id).context("auto review run_id")?; + Ok(self + .root + .join(RUN_STATES_DIR) + .join(format!("{run_id}.json"))) + } + + fn load_run_state_unlocked(&self, run_id: &str) -> Result> { + let path = self.run_state_path(run_id)?; + if !path.exists() { + return Ok(None); + } + let json = std::fs::read_to_string(&path) + .with_context(|| format!("failed to read auto review run state {run_id}"))?; + let state: AutoReviewRunState = serde_json::from_str(&json) + .with_context(|| format!("failed to parse auto review run state {run_id}"))?; + state.validate()?; + Ok(Some(state)) + } + + fn save_run_state_unlocked(&self, state: &AutoReviewRunState) -> Result { + state.validate()?; + let path = self.run_state_path(&state.run_id)?; + let json = serde_json::to_string_pretty(state)?; + write_atomically(&path, &format!("{json}\n")) + .with_context(|| format!("failed to write auto review run state {}", path.display()))?; + Ok(path) + } + fn save_run_metadata(&self, run: &AutoReviewRun) -> Result<()> { let path = self.run_metadata_path(&run.run_id)?; let json = serde_json::to_string_pretty(run)?; @@ -717,6 +805,45 @@ impl AutoReviewStore { Ok(()) } + fn prune_run_states_except(&self, index: &AutoReviewRunsIndex) -> Result<()> { + let states_dir = self.root.join(RUN_STATES_DIR); + if !states_dir.exists() { + return Ok(()); + } + let retained_run_ids = index + .runs + .iter() + .map(|run| run.run_id.as_str()) + .collect::>(); + let entries = std::fs::read_dir(&states_dir).with_context(|| { + format!( + "failed to read auto review run states directory {}", + states_dir.display() + ) + })?; + for entry in entries { + let entry = entry.with_context(|| { + format!( + "failed to read auto review run states directory {}", + states_dir.display() + ) + })?; + let path = entry.path(); + if path.extension().and_then(|ext| ext.to_str()) != Some("json") { + continue; + } + let Some(run_id) = path.file_stem().and_then(|stem| stem.to_str()) else { + continue; + }; + if validate_safe_id(run_id).is_ok() && !retained_run_ids.contains(run_id) { + std::fs::remove_file(&path).with_context(|| { + format!("failed to remove auto review run state {}", path.display()) + })?; + } + } + Ok(()) + } + fn load_metadata_run(&self, run_id: &str) -> Result { let path = self.run_metadata_path(run_id)?; let json = std::fs::read_to_string(&path) @@ -966,6 +1093,170 @@ pub struct AutoReviewRun { pub omitted_finding_digest_count: usize, } +pub const RUN_STATE_SCHEMA_VERSION: u32 = 1; + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +#[serde(rename_all = "snake_case")] +pub struct AutoReviewRunState { + pub schema_version: u32, + pub run_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub budget: Option, + #[serde(default)] + pub usage: AutoReviewUsage, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub terminal_reason: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub finding_disposition: Option, +} + +impl AutoReviewRunState { + pub fn new(run_id: impl Into) -> Self { + Self { + schema_version: RUN_STATE_SCHEMA_VERSION, + run_id: run_id.into(), + budget: None, + usage: AutoReviewUsage::default(), + terminal_reason: None, + finding_disposition: None, + } + } + + fn validate(&self) -> Result<()> { + if self.schema_version != RUN_STATE_SCHEMA_VERSION { + anyhow::bail!( + "unsupported auto review run state schema version: {}", + self.schema_version + ); + } + validate_safe_id(&self.run_id).context("auto review run state run_id")?; + if let Some(budget) = &self.budget { + budget.validate()?; + } + if let Some(disposition) = &self.finding_disposition { + disposition.validate()?; + } + Ok(()) + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +#[serde(rename_all = "snake_case")] +pub struct AutoReviewBudget { + pub max_scope_bytes: usize, + pub max_elapsed_ms: u64, + pub max_total_tokens: u64, + pub max_output_bytes: usize, + pub max_findings: usize, +} + +impl AutoReviewBudget { + pub fn validate(&self) -> Result<()> { + if self.max_scope_bytes == 0 + || self.max_elapsed_ms == 0 + || self.max_total_tokens == 0 + || self.max_output_bytes == 0 + || self.max_findings == 0 + { + anyhow::bail!("auto review budget limits must all be positive"); + } + Ok(()) + } +} + +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +#[serde(rename_all = "snake_case")] +pub struct AutoReviewUsage { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub scope_bytes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub elapsed_ms: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub total_tokens: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub output_bytes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub finding_count: Option, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum AutoReviewTerminalReason { + BudgetScope, + BudgetElapsed, + BudgetTotalTokens, + BudgetOutput, + BudgetFindingCount, + EmptyOutput, + StaleTarget, +} + +impl AutoReviewTerminalReason { + pub const fn cancel_reason(self) -> &'static str { + match self { + Self::BudgetScope => "budget_scope", + Self::BudgetElapsed => "budget_elapsed", + Self::BudgetTotalTokens => "budget_total_tokens", + Self::BudgetOutput => "budget_output", + Self::BudgetFindingCount => "budget_finding_count", + Self::EmptyOutput => "empty_output", + Self::StaleTarget => "stale_target", + } + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum AutoReviewFindingDisposition { + NeedsAttention, + Repairing, + Deferred, + Obsolete, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum AutoReviewDispositionActor { + User, + Agent, + System, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +#[serde(rename_all = "snake_case")] +pub struct AutoReviewFindingDispositionRecord { + pub disposition: AutoReviewFindingDisposition, + pub actor: AutoReviewDispositionActor, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reason: Option, + pub updated_at_unix_secs: i64, +} + +impl AutoReviewFindingDispositionRecord { + fn validate(&self) -> Result<()> { + if matches!(self.disposition, AutoReviewFindingDisposition::Obsolete) + && self + .reason + .as_deref() + .is_none_or(|reason| reason.trim().is_empty()) + { + anyhow::bail!("obsolete auto review disposition requires a reason"); + } + if self + .reason + .as_deref() + .is_some_and(|reason| reason.len() > SUMMARY_MAX_FIELD_BYTES) + { + anyhow::bail!("auto review disposition reason exceeds {SUMMARY_MAX_FIELD_BYTES} bytes"); + } + Ok(()) + } +} + impl AutoReviewRun { pub fn sort_key(&self) -> (i64, &str) { ( @@ -1053,10 +1344,12 @@ impl AutoReviewRun { } pub fn freshness(&self, active_target: &AutoReviewRunTarget) -> AutoReviewFreshness { - if self.freshness == AutoReviewRunFreshness::Lost { - return AutoReviewFreshness::Stale; - } - if self.freshness == AutoReviewRunFreshness::Superseded { + if matches!( + self.freshness, + AutoReviewRunFreshness::Lost + | AutoReviewRunFreshness::Superseded + | AutoReviewRunFreshness::Obsolete + ) { return AutoReviewFreshness::Stale; } self.target.freshness(active_target) diff --git a/codex-rs/auto-review/src/lib_tests.rs b/codex-rs/auto-review/src/lib_tests.rs index 92d56f29c13d..f0187315125c 100644 --- a/codex-rs/auto-review/src/lib_tests.rs +++ b/codex-rs/auto-review/src/lib_tests.rs @@ -7,18 +7,25 @@ use codex_protocol::protocol::ReviewOutputEvent; use codex_protocol::protocol::ReviewTarget; use pretty_assertions::assert_eq; +use super::AutoReviewBudget; use super::AutoReviewDetailKind; use super::AutoReviewDiagnostics; +use super::AutoReviewDispositionActor; use super::AutoReviewDuplicateDisposition; +use super::AutoReviewFindingDisposition; +use super::AutoReviewFindingDispositionRecord; use super::AutoReviewFreshness; use super::AutoReviewLedgerProjection; use super::AutoReviewRun; use super::AutoReviewRunFreshness; use super::AutoReviewRunSource; +use super::AutoReviewRunState; use super::AutoReviewRunStatus; use super::AutoReviewRunTarget; use super::AutoReviewRunsIndex; use super::AutoReviewStore; +use super::AutoReviewTerminalReason; +use super::AutoReviewUsage; use super::DEFAULT_MAX_RUNS; use super::DETAIL_MAX_BYTES; use super::SCHEMA_VERSION; @@ -52,6 +59,85 @@ fn save_and_load_run_round_trips_compact_index() -> anyhow::Result<()> { Ok(()) } +#[test] +fn save_and_load_run_state_preserves_run_schema_compatibility() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let output = sample_output(vec![sample_finding("Title")]); + store.save_run(&sample_run("run_1", &output))?; + let mut state = AutoReviewRunState::new("run_1"); + state.budget = Some(AutoReviewBudget { + max_scope_bytes: 120_000, + max_elapsed_ms: 300_000, + max_total_tokens: 250_000, + max_output_bytes: 65_536, + max_findings: 20, + }); + state.usage = AutoReviewUsage { + scope_bytes: Some(12_000), + elapsed_ms: Some(4_000), + total_tokens: Some(25_000), + output_bytes: Some(2_000), + finding_count: Some(1), + }; + state.terminal_reason = Some(AutoReviewTerminalReason::BudgetTotalTokens); + + store.save_run_state(&state)?; + + assert_eq!(store.load_run_state("run_1")?, Some(state)); + let index_text = std::fs::read_to_string(store.runs_path())?; + assert!(!index_text.contains("max_total_tokens")); + assert!(!index_text.contains("terminal_reason")); + assert_eq!(store.load_run("run_1")?.schema_version, SCHEMA_VERSION); + Ok(()) +} + +#[test] +fn set_finding_disposition_is_durable_and_audited() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let output = sample_output(vec![sample_finding("Title")]); + store.save_run(&sample_run("run_1", &output))?; + let disposition = AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::Deferred, + actor: AutoReviewDispositionActor::User, + reason: Some("acknowledged for follow-up".to_string()), + updated_at_unix_secs: 42, + }; + + let state = store.set_finding_disposition("run_1", disposition.clone())?; + + assert_eq!(state.finding_disposition, Some(disposition)); + assert_eq!(store.load_run_state("run_1")?, Some(state)); + Ok(()) +} + +#[test] +fn obsolete_finding_disposition_requires_reason() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let output = sample_output(vec![sample_finding("Title")]); + store.save_run(&sample_run("run_1", &output))?; + + let err = store + .set_finding_disposition( + "run_1", + AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::Obsolete, + actor: AutoReviewDispositionActor::Agent, + reason: None, + updated_at_unix_secs: 42, + }, + ) + .expect_err("obsolete disposition without a reason should fail"); + + assert!(err.to_string().contains("requires a reason")); + Ok(()) +} + #[test] fn scoped_stores_separate_repos_under_one_home() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; @@ -1574,6 +1660,10 @@ fn lifecycle_freshness_overrides_matching_target_freshness() { AutoReviewRunFreshness::Superseded, AutoReviewRunStatus::Superseded, ), + ( + AutoReviewRunFreshness::Obsolete, + AutoReviewRunStatus::Completed, + ), ] { let run = AutoReviewRun { freshness, diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index d266633f47df..84068b33c8d9 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -559,6 +559,14 @@ pub struct AutoReviewToml { /// to 120000. Reviews whose diff exceeds this limit are recorded as /// skipped rather than launched. pub background_max_diff_bytes: Option, + /// Maximum wall-clock runtime in seconds for automatic background reviews. + pub background_max_elapsed_seconds: Option, + /// Maximum cumulative token usage for automatic background reviews. + pub background_max_total_tokens: Option, + /// Maximum serialized reviewer output size in bytes. + pub background_max_output_bytes: Option, + /// Maximum number of findings accepted from one background review. + pub background_max_findings: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index bcbf8a917cc7..b618e337a070 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -420,6 +420,30 @@ "minimum": 0.0, "type": "integer" }, + "background_max_elapsed_seconds": { + "description": "Maximum wall-clock runtime in seconds for automatic background reviews.", + "format": "uint64", + "minimum": 0.0, + "type": "integer" + }, + "background_max_findings": { + "description": "Maximum number of findings accepted from one background review.", + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "background_max_output_bytes": { + "description": "Maximum serialized reviewer output size in bytes.", + "format": "uint", + "minimum": 0.0, + "type": "integer" + }, + "background_max_total_tokens": { + "description": "Maximum cumulative token usage for automatic background reviews.", + "format": "uint64", + "minimum": 0.0, + "type": "integer" + }, "policy": { "description": "Additional policy instructions inserted into the guardian prompt.", "type": "string" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 2f3fb91e7b70..97a7fb212524 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -6695,6 +6695,10 @@ fn config_toml_deserializes_auto_review_policy() { [auto_review] policy = "Use the user-configured guardian policy." background_max_diff_bytes = 64000 +background_max_elapsed_seconds = 90 +background_max_total_tokens = 75000 +background_max_output_bytes = 32000 +background_max_findings = 12 "#, ) .expect("TOML deserialization should succeed"); @@ -6711,10 +6715,15 @@ background_max_diff_bytes = 64000 .and_then(|auto_review| auto_review.background_max_diff_bytes), Some(64000) ); + let auto_review = cfg.auto_review.as_ref().expect("auto_review config"); + assert_eq!(auto_review.background_max_elapsed_seconds, Some(90)); + assert_eq!(auto_review.background_max_total_tokens, Some(75_000)); + assert_eq!(auto_review.background_max_output_bytes, Some(32_000)); + assert_eq!(auto_review.background_max_findings, Some(12)); } #[tokio::test] -async fn load_config_sets_background_auto_review_diff_limit() -> std::io::Result<()> { +async fn load_config_sets_background_auto_review_budget() -> std::io::Result<()> { let codex_home = TempDir::new()?; let default_config = Config::load_from_base_config_with_overrides( ConfigToml::default(), @@ -6726,14 +6735,28 @@ async fn load_config_sets_background_auto_review_diff_limit() -> std::io::Result ) .await?; assert_eq!( - default_config.background_auto_review_max_diff_bytes, - Some(crate::config::DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_DIFF_BYTES) + default_config.background_auto_review_budget.max_scope_bytes, + crate::config::DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_DIFF_BYTES + ); + assert_eq!( + default_config.background_auto_review_budget.max_elapsed_ms, + crate::config::DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_ELAPSED_MS + ); + assert_eq!( + default_config + .background_auto_review_budget + .max_total_tokens, + crate::config::DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_TOTAL_TOKENS ); let configured = ConfigToml { auto_review: Some(AutoReviewToml { policy: None, background_max_diff_bytes: Some(64_000), + background_max_elapsed_seconds: Some(90), + background_max_total_tokens: Some(75_000), + background_max_output_bytes: Some(32_000), + background_max_findings: Some(12), }), ..Default::default() }; @@ -6747,8 +6770,14 @@ async fn load_config_sets_background_auto_review_diff_limit() -> std::io::Result ) .await?; assert_eq!( - configured_config.background_auto_review_max_diff_bytes, - Some(64_000) + configured_config.background_auto_review_budget, + codex_auto_review::AutoReviewBudget { + max_scope_bytes: 64_000, + max_elapsed_ms: 90_000, + max_total_tokens: 75_000, + max_output_bytes: 32_000, + max_findings: 12, + } ); Ok(()) @@ -6761,6 +6790,7 @@ async fn load_config_uses_auto_review_guardian_policy_config() -> std::io::Resul auto_review: Some(AutoReviewToml { policy: Some(" Use the user-configured guardian policy. ".to_string()), background_max_diff_bytes: None, + ..Default::default() }), ..Default::default() }; @@ -6799,6 +6829,7 @@ async fn requirements_guardian_policy_beats_auto_review() -> std::io::Result<()> auto_review: Some(AutoReviewToml { policy: Some("Use the user-configured guardian policy.".to_string()), background_max_diff_bytes: None, + ..Default::default() }), ..Default::default() }; @@ -6830,6 +6861,7 @@ async fn load_config_ignores_empty_auto_review_guardian_policy_config() -> std:: auto_review: Some(AutoReviewToml { policy: Some(" ".to_string()), background_max_diff_bytes: None, + ..Default::default() }), ..Default::default() }; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 9879f2aa998a..98e0360270cd 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -8,6 +8,7 @@ use crate::unified_exec::MIN_EMPTY_YIELD_TIME_MS; use crate::windows_sandbox::WindowsSandboxLevelExt; use crate::windows_sandbox::resolve_windows_sandbox_mode; use crate::windows_sandbox::resolve_windows_sandbox_private_desktop; +use codex_auto_review::AutoReviewBudget; use codex_config::CloudConfigBundleLoader; use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; @@ -26,6 +27,7 @@ use codex_config::Sourced; use codex_config::ThreadConfigLoader; use codex_config::ValidationConfig; use codex_config::config_toml::AgentRoleBackendToml; +use codex_config::config_toml::AutoReviewToml; use codex_config::config_toml::ConfigLockfileToml; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::DEFAULT_PROJECT_DOC_MAX_BYTES; @@ -167,6 +169,45 @@ pub(crate) use resolved_permission_profile::PermissionProfileState; const DEFAULT_IGNORE_LARGE_UNTRACKED_DIRS: i64 = 200; const DEFAULT_IGNORE_LARGE_UNTRACKED_FILES: i64 = 10 * 1024 * 1024; pub(crate) const DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_DIFF_BYTES: usize = 120_000; +pub(crate) const DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_ELAPSED_MS: u64 = 5 * 60 * 1_000; +pub(crate) const DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_TOTAL_TOKENS: u64 = 250_000; +pub(crate) const DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_OUTPUT_BYTES: usize = 64 * 1024; +pub(crate) const DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_FINDINGS: usize = 20; + +fn resolve_background_auto_review_budget( + auto_review: Option<&AutoReviewToml>, +) -> std::io::Result { + let max_elapsed_seconds = auto_review + .and_then(|config| config.background_max_elapsed_seconds) + .unwrap_or(DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_ELAPSED_MS / 1_000); + let budget = AutoReviewBudget { + max_scope_bytes: auto_review + .and_then(|config| config.background_max_diff_bytes) + .unwrap_or(DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_DIFF_BYTES), + max_elapsed_ms: max_elapsed_seconds.checked_mul(1_000).ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "[auto_review] background_max_elapsed_seconds is too large", + ) + })?, + max_total_tokens: auto_review + .and_then(|config| config.background_max_total_tokens) + .unwrap_or(DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_TOTAL_TOKENS), + max_output_bytes: auto_review + .and_then(|config| config.background_max_output_bytes) + .unwrap_or(DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_OUTPUT_BYTES), + max_findings: auto_review + .and_then(|config| config.background_max_findings) + .unwrap_or(DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_FINDINGS), + }; + budget.validate().map_err(|err| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("invalid [auto_review] background budget: {err}"), + ) + })?; + Ok(budget) +} /// Compatibility-only config retained so legacy `ghost_snapshot` settings /// continue to load even though snapshots are no longer produced. @@ -604,11 +645,8 @@ pub struct Config { /// Model used specifically for review sessions. pub review_model: Option, - /// Maximum diff size (in bytes) for automatic background reviews. Reviews - /// whose diff exceeds this limit are recorded as skipped rather than - /// launched. Corresponds to `[auto_review] background_max_diff_bytes` in - /// config.toml. - pub background_auto_review_max_diff_bytes: Option, + /// Effective hard limits for automatic background reviews. + pub background_auto_review_budget: AutoReviewBudget, /// Patch-local validation policy. pub validation: ValidationConfig, @@ -3541,11 +3579,9 @@ impl Config { model, service_tier, review_model, - background_auto_review_max_diff_bytes: cfg - .auto_review - .as_ref() - .and_then(|ar| ar.background_max_diff_bytes) - .or(Some(DEFAULT_BACKGROUND_AUTO_REVIEW_MAX_DIFF_BYTES)), + background_auto_review_budget: resolve_background_auto_review_budget( + cfg.auto_review.as_ref(), + )?, validation: cfg.validation.unwrap_or_default(), model_context_window: cfg.model_context_window, model_auto_compact_token_limit: cfg.model_auto_compact_token_limit, diff --git a/codex-rs/core/src/context/auto_review_awareness.rs b/codex-rs/core/src/context/auto_review_awareness.rs index 549d689f1e51..c370a44ede5d 100644 --- a/codex-rs/core/src/context/auto_review_awareness.rs +++ b/codex-rs/core/src/context/auto_review_awareness.rs @@ -1,8 +1,12 @@ +use std::collections::BTreeMap; use std::path::Path; use anyhow::Result; +use codex_auto_review::AutoReviewFindingDisposition; use codex_auto_review::AutoReviewLedgerProjection; use codex_auto_review::AutoReviewRun; +use codex_auto_review::AutoReviewRunState; +use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewRunTarget; use codex_auto_review::AutoReviewStore; use codex_auto_review::AutoReviewSummary; @@ -83,7 +87,8 @@ async fn build_auto_review_awareness_inner( let active_review_target = ReviewTarget::UncommittedChanges; let active_target = collect_auto_review_target(codex_home, cwd, &active_review_target).await; let store_scope = active_target.worktree_path.as_deref().unwrap_or(cwd); - let runs = AutoReviewStore::for_scope(codex_home, store_scope).list_runs()?; + let store = AutoReviewStore::for_scope(codex_home, store_scope); + let runs = store.list_runs()?; if runs.is_empty() && active_snapshot.pending_run_id.is_none() && active_snapshot.running_run_id.is_none() @@ -91,19 +96,50 @@ async fn build_auto_review_awareness_inner( return Ok(None); } - Ok(render_awareness( + let run_states = runs + .iter() + .filter(|run| { + run.finding_count > 0 && run.can_read_detail(&active_target, &active_review_target) + }) + .filter_map(|run| { + store + .load_run_state(&run.run_id) + .ok() + .flatten() + .map(|state| (run.run_id.clone(), state)) + }) + .collect(); + Ok(render_awareness_with_states( &runs, + &run_states, &active_target, &active_review_target, &active_snapshot, )) } +#[cfg(test)] fn render_awareness( runs: &[AutoReviewRun], active_target: &AutoReviewRunTarget, active_review_target: &ReviewTarget, active_snapshot: &BackgroundAutoReviewActiveSnapshot, +) -> Option { + render_awareness_with_states( + runs, + &BTreeMap::new(), + active_target, + active_review_target, + active_snapshot, + ) +} + +fn render_awareness_with_states( + runs: &[AutoReviewRun], + run_states: &BTreeMap, + active_target: &AutoReviewRunTarget, + active_review_target: &ReviewTarget, + active_snapshot: &BackgroundAutoReviewActiveSnapshot, ) -> Option { let mut lines = Vec::new(); let mut status_lines = Vec::new(); @@ -124,6 +160,9 @@ fn render_awareness( } for run in &visible_runs { + if !run_has_actionable_disposition(run, run_states.get(&run.run_id)) { + continue; + } let summary = run.project(active_target, active_review_target).summary; if !summary.content.is_empty() { match ¤t_summary { @@ -134,9 +173,19 @@ fn render_awareness( } let current_finding_lines = current_summary.map(|(run, summary)| { + let disposition = run_states + .get(&run.run_id) + .and_then(|state| state.finding_disposition.as_ref()) + .map(|record| match record.disposition { + AutoReviewFindingDisposition::NeedsAttention => "needs_attention", + AutoReviewFindingDisposition::Repairing => "repairing", + AutoReviewFindingDisposition::Deferred => "deferred", + AutoReviewFindingDisposition::Obsolete => "obsolete", + }) + .unwrap_or("needs_attention"); let mut lines = vec![format!( - "- current findings from run {}: {} rendered, {} omitted", - run.run_id, summary.rendered_findings, summary.omitted_findings + "- current findings from run {}: disposition={disposition}, {} rendered, {} omitted", + run.run_id, summary.rendered_findings, summary.omitted_findings, )]; lines.extend(summary.content.lines().map(|line| format!(" {line}"))); if summary.truncated { @@ -145,10 +194,12 @@ fn render_awareness( lines }); - if status_lines.is_empty() - && current_finding_lines.is_none() - && projection.status_counts.is_empty() - { + let has_diagnostic_state = visible_runs.iter().any(|run| { + !matches!(run.status, AutoReviewRunStatus::Completed) + || (run.finding_count > 0 + && run_has_actionable_disposition(run, run_states.get(&run.run_id))) + }); + if status_lines.is_empty() && current_finding_lines.is_none() && !has_diagnostic_state { return None; } @@ -164,14 +215,18 @@ fn render_awareness( lines.push(format!( "- finding detail is available by stable run_id/finding_id; normal turns include summaries only (max {SUMMARY_MAX_BYTES} bytes)." )); + lines.push( + "- disposition required: use auto_review_disposition with repair, defer, or obsolete (obsolete requires a reason)." + .to_string(), + ); } - if !projection.status_counts.is_empty() { + if has_diagnostic_state && !projection.status_counts.is_empty() { lines.push("- recent run status counts:".to_string()); for count in projection.status_counts.iter().take(MAX_STATUS_LINES) { lines.push(format!(" - {}: {}", count.label(), count.count)); } } - if let Some(diagnostics) = projection.diagnostics { + if has_diagnostic_state && let Some(diagnostics) = projection.diagnostics { lines.push(format!( "- recent run diagnostics: {}", diagnostics.compact_line() @@ -181,6 +236,19 @@ fn render_awareness( AutoReviewAwareness::new(lines.join("\n")) } +fn run_has_actionable_disposition(run: &AutoReviewRun, state: Option<&AutoReviewRunState>) -> bool { + run.finding_count > 0 + && state + .and_then(|state| state.finding_disposition.as_ref()) + .is_none_or(|record| { + matches!( + record.disposition, + AutoReviewFindingDisposition::NeedsAttention + | AutoReviewFindingDisposition::Repairing + ) + }) +} + fn run_is_visible_for_awareness(run: &AutoReviewRun, active_review_target: &ReviewTarget) -> bool { !matches!( (&run.review_target, active_review_target), diff --git a/codex-rs/core/src/review_persistence.rs b/codex-rs/core/src/review_persistence.rs index b4a4118f84e4..851f474643ab 100644 --- a/codex-rs/core/src/review_persistence.rs +++ b/codex-rs/core/src/review_persistence.rs @@ -1,14 +1,21 @@ use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use std::sync::Mutex; +use codex_auto_review::AutoReviewBudget; +use codex_auto_review::AutoReviewDispositionActor; use codex_auto_review::AutoReviewDuplicateMatch; +use codex_auto_review::AutoReviewFindingDisposition; +use codex_auto_review::AutoReviewFindingDispositionRecord; use codex_auto_review::AutoReviewRun; use codex_auto_review::AutoReviewRunFreshness; use codex_auto_review::AutoReviewRunSource; use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewRunTarget; use codex_auto_review::AutoReviewStore; +use codex_auto_review::AutoReviewTerminalReason; +use codex_auto_review::AutoReviewUsage; use codex_auto_review::ReviewCoordination; use codex_auto_review::SCHEMA_VERSION; use codex_auto_review::finding_digests; @@ -39,6 +46,29 @@ pub(crate) struct ReviewPersistenceContext { model: Option, reasoning_effort: Option, prompt_token_estimate: Option, + background_budget: Option, + scope_bytes: Option, + interruption: Arc>>, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum ReviewInterruptionStatus { + Cancelled, + Superseded, +} + +#[derive(Clone, Debug)] +struct ReviewInterruption { + status: ReviewInterruptionStatus, + error_summary: String, + superseded_by: Option, + cancel_reason: Option, +} + +#[derive(Debug)] +pub(crate) struct SavedReviewInterruption { + pub(crate) status: ReviewInterruptionStatus, + pub(crate) error_summary: String, } impl ReviewPersistenceContext { @@ -70,6 +100,9 @@ impl ReviewPersistenceContext { model, reasoning_effort, prompt_token_estimate, + background_budget: None, + scope_bytes: None, + interruption: Arc::new(Mutex::new(None)), } } @@ -96,6 +129,25 @@ impl ReviewPersistenceContext { self } + pub(crate) fn with_background_budget( + mut self, + budget: AutoReviewBudget, + scope_bytes: usize, + ) -> Self { + self.background_budget = Some(budget); + self.scope_bytes = Some(scope_bytes); + self + } + + pub(crate) fn with_scope_bytes(mut self, scope_bytes: usize) -> Self { + self.scope_bytes = Some(scope_bytes); + self + } + + pub(crate) fn background_budget(&self) -> Option<&AutoReviewBudget> { + self.background_budget.as_ref() + } + pub(crate) fn target(&self) -> &AutoReviewRunTarget { &self.target } @@ -121,6 +173,16 @@ impl ReviewPersistenceContext { &self.review_target } + pub(crate) async fn completion_freshness(&self, codex_home: &Path) -> AutoReviewRunFreshness { + let active = + collect_auto_review_target(codex_home, &self.store_scope, &self.review_target).await; + match self.target.freshness(&active) { + codex_auto_review::AutoReviewFreshness::Current => AutoReviewRunFreshness::Current, + codex_auto_review::AutoReviewFreshness::Stale + | codex_auto_review::AutoReviewFreshness::Detached => AutoReviewRunFreshness::Obsolete, + } + } + pub(crate) fn save_running(&self, codex_home: impl AsRef) -> bool { self.save_run( codex_home, @@ -141,42 +203,150 @@ impl ReviewPersistenceContext { ) } - pub(crate) fn save_completed( + pub(crate) fn save_completed_with_freshness( &self, codex_home: impl AsRef, output: &ReviewOutputEvent, token_usage: Option<&TokenUsage>, + freshness: AutoReviewRunFreshness, + usage: AutoReviewUsage, ) -> bool { - self.save_run( + let codex_home = codex_home.as_ref(); + let disposition = if output.findings.is_empty() { + None + } else if freshness == AutoReviewRunFreshness::Current { + Some(AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::NeedsAttention, + actor: AutoReviewDispositionActor::System, + reason: None, + updated_at_unix_secs: now_unix_secs(), + }) + } else { + Some(AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::Obsolete, + actor: AutoReviewDispositionActor::System, + reason: Some("review target changed before completion".to_string()), + updated_at_unix_secs: now_unix_secs(), + }) + }; + let has_stale_findings = + freshness != AutoReviewRunFreshness::Current && !output.findings.is_empty(); + self.save_run_with_metadata_and_state( codex_home, AutoReviewRunStatus::Completed, Some(output), token_usage, /*error_summary*/ None, + freshness, + None, + None, + /*saved_token_estimate*/ None, + move |state| { + merge_usage(&mut state.usage, usage); + state.finding_disposition = disposition; + if has_stale_findings { + state.terminal_reason = Some(AutoReviewTerminalReason::StaleTarget); + } + }, ) } - pub(crate) fn save_cancelled(&self, codex_home: impl AsRef) -> bool { - self.save_cancelled_with_summary( + pub(crate) fn save_cancelled_with_summary( + &self, + codex_home: impl AsRef, + error_summary: String, + ) -> bool { + self.save_run( codex_home, - AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string(), + AutoReviewRunStatus::Cancelled, + /*output*/ None, + /*token_usage*/ None, + Some(error_summary), ) } - pub(crate) fn save_cancelled_with_summary( + pub(crate) fn save_cancelled_with_summary_and_reason( &self, codex_home: impl AsRef, error_summary: String, + cancel_reason: String, ) -> bool { - self.save_run( + self.save_run_with_metadata( codex_home, AutoReviewRunStatus::Cancelled, /*output*/ None, /*token_usage*/ None, Some(error_summary), + AutoReviewRunFreshness::Current, + None, + Some(cancel_reason), + /*saved_token_estimate*/ None, ) } + pub(crate) fn set_cancelled_interruption(&self, error_summary: String, cancel_reason: String) { + self.set_interruption(ReviewInterruption { + status: ReviewInterruptionStatus::Cancelled, + error_summary, + superseded_by: None, + cancel_reason: Some(cancel_reason), + }); + } + + pub(crate) fn set_superseded_interruption( + &self, + error_summary: String, + superseded_by: Option, + ) { + self.set_interruption(ReviewInterruption { + status: ReviewInterruptionStatus::Superseded, + error_summary, + superseded_by, + cancel_reason: None, + }); + } + + pub(crate) fn save_interrupted( + &self, + codex_home: impl AsRef, + ) -> Option { + let interruption = { + let pending = self.interruption.lock().unwrap_or_else(|err| { + tracing::warn!("review interruption lock was poisoned; continuing"); + err.into_inner() + }); + pending.clone() + } + .unwrap_or_else(|| ReviewInterruption { + status: ReviewInterruptionStatus::Cancelled, + error_summary: AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string(), + superseded_by: None, + cancel_reason: None, + }); + let saved = match interruption.status { + ReviewInterruptionStatus::Cancelled => { + if let Some(cancel_reason) = interruption.cancel_reason.clone() { + self.save_cancelled_with_summary_and_reason( + codex_home, + interruption.error_summary.clone(), + cancel_reason, + ) + } else { + self.save_cancelled_with_summary(codex_home, interruption.error_summary.clone()) + } + } + ReviewInterruptionStatus::Superseded => self.save_superseded_with_summary( + codex_home, + interruption.error_summary.clone(), + interruption.superseded_by, + ), + }; + saved.then_some(SavedReviewInterruption { + status: interruption.status, + error_summary: interruption.error_summary, + }) + } + pub(crate) fn save_superseded_with_summary( &self, codex_home: impl AsRef, @@ -211,6 +381,96 @@ impl ReviewPersistenceContext { ) } + pub(crate) fn save_empty_output( + &self, + codex_home: impl AsRef, + error_summary: String, + token_usage: Option<&TokenUsage>, + usage: AutoReviewUsage, + ) -> bool { + let codex_home = codex_home.as_ref(); + self.save_run_with_metadata_and_state( + codex_home, + AutoReviewRunStatus::Failed, + /*output*/ None, + token_usage, + Some(error_summary), + AutoReviewRunFreshness::Current, + None, + Some( + AutoReviewTerminalReason::EmptyOutput + .cancel_reason() + .to_string(), + ), + /*saved_token_estimate*/ None, + move |state| { + merge_usage(&mut state.usage, usage); + state.terminal_reason = Some(AutoReviewTerminalReason::EmptyOutput); + }, + ) + } + + pub(crate) fn save_budget_cancelled( + &self, + codex_home: impl AsRef, + reason: AutoReviewTerminalReason, + error_summary: String, + token_usage: Option<&TokenUsage>, + usage: AutoReviewUsage, + ) -> bool { + let codex_home = codex_home.as_ref(); + self.save_run_with_metadata_and_state( + codex_home, + AutoReviewRunStatus::Cancelled, + /*output*/ None, + token_usage, + Some(error_summary), + AutoReviewRunFreshness::Current, + None, + Some(reason.cancel_reason().to_string()), + /*saved_token_estimate*/ None, + move |state| { + merge_usage(&mut state.usage, usage); + state.terminal_reason = Some(reason); + }, + ) + } + + pub(crate) fn save_budget_skipped( + &self, + codex_home: impl AsRef, + reason: AutoReviewTerminalReason, + error_summary: String, + usage: AutoReviewUsage, + ) -> bool { + let codex_home = codex_home.as_ref(); + self.save_run_with_metadata_and_state( + codex_home, + AutoReviewRunStatus::Skipped, + /*output*/ None, + /*token_usage*/ None, + Some(error_summary), + AutoReviewRunFreshness::Current, + None, + Some(reason.cancel_reason().to_string()), + /*saved_token_estimate*/ None, + move |state| { + merge_usage(&mut state.usage, usage); + state.terminal_reason = Some(reason); + }, + ) + } + + pub(crate) fn record_progress( + &self, + codex_home: impl AsRef, + usage: AutoReviewUsage, + ) -> bool { + self.update_run_state(codex_home.as_ref(), |state| { + merge_usage(&mut state.usage, usage); + }) + } + pub(crate) fn save_skipped(&self, codex_home: impl AsRef, error_summary: String) -> bool { self.save_run( codex_home, @@ -275,6 +535,34 @@ impl ReviewPersistenceContext { superseded_by: Option, cancel_reason: Option, saved_token_estimate: Option, + ) -> bool { + self.save_run_with_metadata_and_state( + codex_home, + status, + output, + token_usage, + error_summary, + freshness, + superseded_by, + cancel_reason, + saved_token_estimate, + |_| {}, + ) + } + + #[allow(clippy::too_many_arguments)] + fn save_run_with_metadata_and_state( + &self, + codex_home: impl AsRef, + status: AutoReviewRunStatus, + output: Option<&ReviewOutputEvent>, + token_usage: Option<&TokenUsage>, + error_summary: Option, + freshness: AutoReviewRunFreshness, + superseded_by: Option, + cancel_reason: Option, + saved_token_estimate: Option, + update_state: impl FnOnce(&mut codex_auto_review::AutoReviewRunState), ) -> bool { let codex_home = codex_home.as_ref(); let completed_at_unix_secs = if is_terminal_status(&status) { @@ -290,14 +578,24 @@ impl ReviewPersistenceContext { if let Ok(existing) = store.load_run(&self.run_id) && is_terminal_status(&existing.status) { - if !is_terminal_status(&status) { - tracing::debug!( - run_id = %self.run_id, - existing_status = ?existing.status, - "skipping non-terminal auto review run write after terminal status" + let corrects_interrupted_lifecycle = existing.status == AutoReviewRunStatus::Cancelled + && existing.cancel_reason.is_none() + && existing.superseded_by.is_none() + && is_explicit_lifecycle_update( + &status, + superseded_by.as_deref(), + cancel_reason.as_deref(), ); + if !corrects_interrupted_lifecycle { + if !is_terminal_status(&status) { + tracing::debug!( + run_id = %self.run_id, + existing_status = ?existing.status, + "skipping non-terminal auto review run write after terminal status" + ); + } + return false; } - return false; } if let Err(err) = store.list_runs() { tracing::warn!( @@ -307,6 +605,9 @@ impl ReviewPersistenceContext { ); return false; } + if !self.update_run_state(codex_home, update_state) { + return false; + } if let Some(output) = output && let Err(err) = store.save_output(&self.run_id, output) { @@ -355,18 +656,89 @@ impl ReviewPersistenceContext { } true } + + fn update_run_state( + &self, + codex_home: &Path, + update: impl FnOnce(&mut codex_auto_review::AutoReviewRunState), + ) -> bool { + let store = AutoReviewStore::for_scope(codex_home, &self.store_scope); + match store.update_run_state(&self.run_id, |state| { + if let Some(budget) = &self.background_budget { + state.budget = Some(budget.clone()); + } + if let Some(scope_bytes) = self.scope_bytes { + state.usage.scope_bytes = Some(scope_bytes); + } + update(state); + Ok(()) + }) { + Ok(_) => true, + Err(err) => { + tracing::warn!( + run_id = %self.run_id, + error = %err, + "failed to persist auto review run state" + ); + false + } + } + } + + fn set_interruption(&self, interruption: ReviewInterruption) { + let mut pending = self.interruption.lock().unwrap_or_else(|err| { + tracing::warn!("review interruption lock was poisoned; continuing"); + err.into_inner() + }); + *pending = Some(interruption); + } } fn is_terminal_status(status: &AutoReviewRunStatus) -> bool { status.is_terminal() } +fn is_explicit_lifecycle_update( + status: &AutoReviewRunStatus, + superseded_by: Option<&str>, + cancel_reason: Option<&str>, +) -> bool { + matches!( + status, + AutoReviewRunStatus::Cancelled | AutoReviewRunStatus::Superseded + ) && (status == &AutoReviewRunStatus::Superseded + || superseded_by.is_some() + || cancel_reason.is_some()) +} + fn token_count_u64(token_usage: &TokenUsage) -> Option { - u64::try_from(token_usage.total_tokens) + let reconstructed_total = token_usage + .non_cached_input() + .saturating_add(token_usage.cached_input()) + .saturating_add(token_usage.output_tokens.max(0)); + u64::try_from(token_usage.total_tokens.max(reconstructed_total)) .ok() .filter(|token_count| *token_count > 0) } +fn merge_usage(current: &mut AutoReviewUsage, update: AutoReviewUsage) { + if update.scope_bytes.is_some() { + current.scope_bytes = update.scope_bytes; + } + if update.elapsed_ms.is_some() { + current.elapsed_ms = update.elapsed_ms; + } + if update.total_tokens.is_some() { + current.total_tokens = update.total_tokens; + } + if update.output_bytes.is_some() { + current.output_bytes = update.output_bytes; + } + if update.finding_count.is_some() { + current.finding_count = update.finding_count; + } +} + fn duplicate_saved_token_estimate(duplicate: &AutoReviewDuplicateMatch) -> Option { duplicate.token_count.or(duplicate.prompt_token_estimate) } @@ -437,6 +809,9 @@ fn now_unix_secs() -> i64 { #[cfg(test)] mod tests { use super::*; + use codex_protocol::protocol::ReviewCodeLocation; + use codex_protocol::protocol::ReviewFinding; + use codex_protocol::protocol::ReviewLineRange; use codex_protocol::protocol::ReviewPersistence; use codex_protocol::protocol::TokenUsage; use tempfile::TempDir; @@ -457,7 +832,10 @@ mod tests { ) .await; - persistence.save_cancelled(codex_home.path()); + persistence.save_cancelled_with_summary( + codex_home.path(), + AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string(), + ); persistence.save_running(codex_home.path()); let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); @@ -499,6 +877,82 @@ mod tests { ); } + #[tokio::test] + async fn explicit_cancel_reason_corrects_generic_interruption() { + let codex_home = TempDir::new().expect("create temp codex home"); + let cwd = TempDir::new().expect("create temp cwd"); + let persistence = ReviewPersistenceContext::new( + "corrected-cancelled".to_string(), + ReviewPersistence::BackgroundAutoReview, + ReviewTarget::UncommittedChanges, + codex_home.path(), + cwd.path(), + Some("test-model".to_string()), + /*reasoning_effort*/ None, + /*prompt_token_estimate*/ None, + ) + .await; + + persistence.save_cancelled_with_summary( + codex_home.path(), + AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string(), + ); + persistence.save_cancelled_with_summary_and_reason( + codex_home.path(), + "background auto review was cancelled by request".to_string(), + "background_auto_review_control".to_string(), + ); + + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); + let run = store + .load_run("corrected-cancelled") + .expect("load persisted review run"); + assert_eq!(run.status, AutoReviewRunStatus::Cancelled); + assert_eq!( + run.error_summary.as_deref(), + Some("background auto review was cancelled by request") + ); + assert_eq!( + run.cancel_reason.as_deref(), + Some("background_auto_review_control") + ); + } + + #[tokio::test] + async fn explicit_supersede_corrects_generic_interruption() { + let codex_home = TempDir::new().expect("create temp codex home"); + let cwd = TempDir::new().expect("create temp cwd"); + let persistence = ReviewPersistenceContext::new( + "corrected-superseded".to_string(), + ReviewPersistence::BackgroundAutoReview, + ReviewTarget::UncommittedChanges, + codex_home.path(), + cwd.path(), + Some("test-model".to_string()), + /*reasoning_effort*/ None, + /*prompt_token_estimate*/ None, + ) + .await; + + persistence.save_cancelled_with_summary( + codex_home.path(), + AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string(), + ); + persistence.save_superseded_with_summary( + codex_home.path(), + "background auto review was superseded by run replacement-run".to_string(), + Some("replacement-run".to_string()), + ); + + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); + let run = store + .load_run("corrected-superseded") + .expect("load persisted review run"); + assert_eq!(run.status, AutoReviewRunStatus::Superseded); + assert_eq!(run.freshness, AutoReviewRunFreshness::Superseded); + assert_eq!(run.superseded_by.as_deref(), Some("replacement-run")); + } + #[tokio::test] async fn save_superseded_with_summary_records_supersede_metadata() { let codex_home = TempDir::new().expect("create temp codex home"); @@ -652,11 +1106,24 @@ mod tests { .await; let output = ReviewOutputEvent::default(); let token_usage = TokenUsage { - total_tokens: 25_915, - ..TokenUsage::default() + input_tokens: 100, + cached_input_tokens: 90, + output_tokens: 10, + reasoning_output_tokens: 0, + total_tokens: 20, }; - persistence.save_completed(codex_home.path(), &output, Some(&token_usage)); + persistence.save_completed_with_freshness( + codex_home.path(), + &output, + Some(&token_usage), + AutoReviewRunFreshness::Current, + AutoReviewUsage { + total_tokens: Some(110), + finding_count: Some(0), + ..Default::default() + }, + ); let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); let run = store @@ -665,10 +1132,185 @@ mod tests { assert_eq!(run.model.as_deref(), Some("test-model")); assert_eq!(run.reasoning_effort.as_deref(), Some("medium")); assert_eq!(run.prompt_token_estimate, Some(42_000)); - assert_eq!(run.token_count, Some(25_915)); + assert_eq!(run.token_count, Some(110)); assert_eq!(run.saved_token_estimate, None); } + #[tokio::test] + async fn save_budget_cancelled_records_limits_usage_and_reason() { + let codex_home = TempDir::new().expect("create temp codex home"); + let cwd = TempDir::new().expect("create temp cwd"); + let persistence = ReviewPersistenceContext::new( + "budget-cancelled".to_string(), + ReviewPersistence::BackgroundAutoReview, + ReviewTarget::UncommittedChanges, + codex_home.path(), + cwd.path(), + Some("test-model".to_string()), + /*reasoning_effort*/ None, + /*prompt_token_estimate*/ None, + ) + .await + .with_background_budget(test_budget(), 12_000); + let usage = AutoReviewUsage { + scope_bytes: Some(12_000), + elapsed_ms: Some(30_000), + total_tokens: Some(250_001), + ..Default::default() + }; + let token_usage = TokenUsage { + total_tokens: 250_001, + ..Default::default() + }; + + assert!(persistence.save_budget_cancelled( + codex_home.path(), + AutoReviewTerminalReason::BudgetTotalTokens, + "background review exceeded token budget".to_string(), + Some(&token_usage), + usage.clone(), + )); + + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); + let run = store + .load_run("budget-cancelled") + .expect("load persisted review run"); + let state = store + .load_run_state("budget-cancelled") + .expect("load persisted review state") + .expect("review state should exist"); + assert_eq!(run.status, AutoReviewRunStatus::Cancelled); + assert_eq!(run.cancel_reason.as_deref(), Some("budget_total_tokens")); + assert_eq!(run.token_count, Some(250_001)); + assert_eq!(state.budget, Some(test_budget())); + assert_eq!(state.usage, usage); + assert_eq!( + state.terminal_reason, + Some(AutoReviewTerminalReason::BudgetTotalTokens) + ); + } + + #[tokio::test] + async fn completed_current_findings_require_attention() { + let codex_home = TempDir::new().expect("create temp codex home"); + let cwd = TempDir::new().expect("create temp cwd"); + let persistence = ReviewPersistenceContext::new( + "actionable-findings".to_string(), + ReviewPersistence::BackgroundAutoReview, + ReviewTarget::UncommittedChanges, + codex_home.path(), + cwd.path(), + Some("test-model".to_string()), + /*reasoning_effort*/ None, + /*prompt_token_estimate*/ None, + ) + .await + .with_background_budget(test_budget(), 12_000); + let output = output_with_finding(cwd.path()); + + assert!(persistence.save_completed_with_freshness( + codex_home.path(), + &output, + /*token_usage*/ None, + AutoReviewRunFreshness::Current, + AutoReviewUsage { + finding_count: Some(1), + ..Default::default() + }, + )); + + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); + let state = store + .load_run_state("actionable-findings") + .expect("load persisted review state") + .expect("review state should exist"); + let disposition = state + .finding_disposition + .expect("current findings should require disposition"); + assert_eq!( + disposition.disposition, + AutoReviewFindingDisposition::NeedsAttention + ); + assert_eq!(disposition.actor, AutoReviewDispositionActor::System); + } + + #[tokio::test] + async fn completed_stale_findings_are_marked_obsolete() { + let codex_home = TempDir::new().expect("create temp codex home"); + let cwd = TempDir::new().expect("create temp cwd"); + let persistence = ReviewPersistenceContext::new( + "stale-findings".to_string(), + ReviewPersistence::BackgroundAutoReview, + ReviewTarget::UncommittedChanges, + codex_home.path(), + cwd.path(), + Some("test-model".to_string()), + /*reasoning_effort*/ None, + /*prompt_token_estimate*/ None, + ) + .await + .with_background_budget(test_budget(), 12_000); + let output = output_with_finding(cwd.path()); + + assert!(persistence.save_completed_with_freshness( + codex_home.path(), + &output, + /*token_usage*/ None, + AutoReviewRunFreshness::Obsolete, + AutoReviewUsage { + finding_count: Some(1), + ..Default::default() + }, + )); + + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); + let run = store + .load_run("stale-findings") + .expect("load persisted review run"); + let state = store + .load_run_state("stale-findings") + .expect("load persisted review state") + .expect("review state should exist"); + assert_eq!(run.freshness, AutoReviewRunFreshness::Obsolete); + assert_eq!( + state.terminal_reason, + Some(AutoReviewTerminalReason::StaleTarget) + ); + assert_eq!( + state + .finding_disposition + .expect("stale finding disposition") + .disposition, + AutoReviewFindingDisposition::Obsolete + ); + } + + fn test_budget() -> AutoReviewBudget { + AutoReviewBudget { + max_scope_bytes: 120_000, + max_elapsed_ms: 300_000, + max_total_tokens: 250_000, + max_output_bytes: 65_536, + max_findings: 20, + } + } + + fn output_with_finding(cwd: &Path) -> ReviewOutputEvent { + ReviewOutputEvent { + findings: vec![ReviewFinding { + title: "[P1] Finding".to_string(), + body: "Finding body".to_string(), + confidence_score: 0.9, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: cwd.join("src/lib.rs"), + line_range: ReviewLineRange { start: 1, end: 2 }, + }, + }], + ..Default::default() + } + } + #[tokio::test] async fn collect_target_records_current_snapshot_epoch() { let codex_home = TempDir::new().expect("create temp codex home"); diff --git a/codex-rs/core/src/session/background_auto_review.rs b/codex-rs/core/src/session/background_auto_review.rs index 34486b8662d3..67da26aa1452 100644 --- a/codex-rs/core/src/session/background_auto_review.rs +++ b/codex-rs/core/src/session/background_auto_review.rs @@ -5,6 +5,8 @@ use std::time::Duration; use codex_auto_review::AutoReviewDuplicateDisposition; use codex_auto_review::AutoReviewDuplicateMatch; use codex_auto_review::AutoReviewStore; +use codex_auto_review::AutoReviewTerminalReason; +use codex_auto_review::AutoReviewUsage; use codex_auto_review::ReviewCoordination; use codex_auto_review::ReviewLockGuard; use codex_git_utils::diff_fingerprint; @@ -36,6 +38,7 @@ use crate::turn_timing::now_unix_timestamp_ms; const BACKGROUND_AUTO_REVIEW_DEBOUNCE: Duration = Duration::from_secs(2); const BACKGROUND_AUTO_REVIEW_LOCK_RETRY: Duration = Duration::from_millis(500); const BACKGROUND_AUTO_REVIEW_LOCK_RETRY_INTERVAL: Duration = Duration::from_millis(25); +const BACKGROUND_AUTO_REVIEW_CONTROL_CANCEL_REASON: &str = "background_auto_review_control"; impl Session { pub(crate) async fn record_background_auto_review_turn_start( @@ -136,7 +139,11 @@ impl Session { .map(|effort| effort.to_string()), /*prompt_token_estimate*/ None, ) - .await; + .await + .with_background_budget( + turn_context.config.background_auto_review_budget.clone(), + turn_diff.len(), + ); if let Some(duplicate) = self .resolve_durable_background_auto_review_duplicate(&persistence) .await @@ -260,16 +267,20 @@ impl Session { }, user_facing_hint: Some("current turn changes".to_string()), }; - if let Some(max_bytes) = turn_context.config.background_auto_review_max_diff_bytes - && let Some(error_summary) = - background_auto_review_size_limit_summary(&turn_diff, None, max_bytes) + let max_scope_bytes = turn_context + .config + .background_auto_review_budget + .max_scope_bytes; + if let Some(error_summary) = + background_auto_review_size_limit_summary(&turn_diff, None, max_scope_bytes) { debug!(%error_summary, "background auto review skipped: oversized diff"); - self.record_skipped_background_auto_review( + self.record_scope_budget_skipped_background_auto_review( &persistence, generation, &fingerprint, error_summary, + turn_diff.len(), ) .await; return; @@ -281,19 +292,20 @@ impl Session { return; } }; - if let Some(max_bytes) = turn_context.config.background_auto_review_max_diff_bytes - && let Some(error_summary) = background_auto_review_size_limit_summary( - &turn_diff, - Some(resolved.prompt.as_str()), - max_bytes, - ) - { + let scope_bytes = resolved.prompt.len(); + let persistence = persistence.with_scope_bytes(scope_bytes); + if let Some(error_summary) = background_auto_review_size_limit_summary( + &turn_diff, + Some(resolved.prompt.as_str()), + max_scope_bytes, + ) { debug!(%error_summary, "background auto review skipped: oversized review scope"); - self.record_skipped_background_auto_review( + self.record_scope_budget_skipped_background_auto_review( &persistence, generation, &fingerprint, error_summary, + scope_bytes, ) .await; return; @@ -538,6 +550,40 @@ impl Session { } } + async fn record_scope_budget_skipped_background_auto_review( + self: &Arc, + persistence: &ReviewPersistenceContext, + generation: u64, + fingerprint: &str, + error_summary: String, + scope_bytes: usize, + ) { + let codex_home = self.codex_home().await; + { + let mut state = self.state.lock().await; + state + .background_auto_review + .clear_pending_review(generation, fingerprint); + } + if persistence.save_budget_skipped( + codex_home, + AutoReviewTerminalReason::BudgetScope, + error_summary.clone(), + AutoReviewUsage { + scope_bytes: Some(scope_bytes), + ..Default::default() + }, + ) { + record_background_review_status( + Arc::clone(self), + persistence, + BackgroundAutoReviewStatus::Skipped, + Some(error_summary), + ) + .await; + } + } + async fn record_skipped_duplicate_background_auto_review( self: &Arc, persistence: &ReviewPersistenceContext, @@ -770,9 +816,32 @@ impl Session { running_review: BackgroundAutoReviewRunningHandle, error_summary: String, ) { + let completion = running_review.completion; + if completion.is_done() { + return; + } + running_review.persistence.set_cancelled_interruption( + error_summary.clone(), + BACKGROUND_AUTO_REVIEW_CONTROL_CANCEL_REASON.to_string(), + ); + let notified = completion.notified(); + tokio::pin!(notified); + notified.as_mut().enable(); + running_review.cancellation_token.cancel(); + if !completion.is_done() + && tokio::time::timeout(Duration::from_millis(100), notified.as_mut()) + .await + .is_err() + { + warn!("background auto review did not finish promptly after cancellation"); + } if running_review .persistence - .save_cancelled_with_summary(self.codex_home().await, error_summary.clone()) + .save_cancelled_with_summary_and_reason( + self.codex_home().await, + error_summary.clone(), + BACKGROUND_AUTO_REVIEW_CONTROL_CANCEL_REASON.to_string(), + ) { record_background_review_status( Arc::clone(self), @@ -782,31 +851,32 @@ impl Session { ) .await; } + } + + async fn supersede_running_background_auto_review( + self: &Arc, + running_review: BackgroundAutoReviewRunningHandle, + error_summary: String, + superseded_by: Option, + ) { let completion = running_review.completion; if completion.is_done() { return; } + running_review + .persistence + .set_superseded_interruption(error_summary.clone(), superseded_by.clone()); let notified = completion.notified(); tokio::pin!(notified); notified.as_mut().enable(); running_review.cancellation_token.cancel(); - if completion.is_done() { - return; - } - if tokio::time::timeout(Duration::from_millis(100), notified.as_mut()) - .await - .is_err() + if !completion.is_done() + && tokio::time::timeout(Duration::from_millis(100), notified.as_mut()) + .await + .is_err() { - warn!("background auto review did not finish promptly after cancellation"); + warn!("background auto review did not finish promptly after supersede"); } - } - - async fn supersede_running_background_auto_review( - self: &Arc, - running_review: BackgroundAutoReviewRunningHandle, - error_summary: String, - superseded_by: Option, - ) { if running_review.persistence.save_superseded_with_summary( self.codex_home().await, error_summary.clone(), @@ -820,23 +890,6 @@ impl Session { ) .await; } - let completion = running_review.completion; - if completion.is_done() { - return; - } - let notified = completion.notified(); - tokio::pin!(notified); - notified.as_mut().enable(); - running_review.cancellation_token.cancel(); - if completion.is_done() { - return; - } - if tokio::time::timeout(Duration::from_millis(100), notified.as_mut()) - .await - .is_err() - { - warn!("background auto review did not finish promptly after supersede"); - } } pub(crate) async fn clear_background_auto_review(self: &Arc, generation: u64) { diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index bb8b88b31ea8..b0853e341697 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -401,7 +401,7 @@ pub(super) async fn record_background_review_status( status: BackgroundAutoReviewStatus, error_summary: Option, ) { - sess.send_event_raw(Event { + let event = Event { id: persistence.run_id().to_string(), msg: EventMsg::BackgroundAutoReviewStatus(BackgroundAutoReviewStatusEvent { run_id: persistence.run_id().to_string(), @@ -409,6 +409,12 @@ pub(super) async fn record_background_review_status( review_target: persistence.review_target().clone(), error_summary, }), + }; + if let Err(err) = tokio::spawn(async move { + sess.send_event_raw(event).await; }) - .await; + .await + { + tracing::warn!(error = %err, "background auto review status task failed"); + } } diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index adbb8127e9be..349e17bf9488 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -1,5 +1,9 @@ use std::sync::Arc; +use std::time::Duration; +use codex_auto_review::AutoReviewBudget; +use codex_auto_review::AutoReviewTerminalReason; +use codex_auto_review::AutoReviewUsage; use codex_prompts::render_review_exit_interrupted; use codex_prompts::render_review_exit_success; use codex_protocol::config_types::WebSearchMode; @@ -16,14 +20,16 @@ use codex_protocol::protocol::ExitedReviewModeEvent; use codex_protocol::protocol::ItemCompletedEvent; use codex_protocol::protocol::ReviewOutputEvent; use codex_protocol::protocol::SubAgentSource; +use codex_protocol::protocol::TokenUsage; use tokio_util::sync::CancellationToken; use crate::codex_delegate::run_codex_thread_one_shot; use crate::config::Constrained; use crate::review_format::format_review_findings_block; use crate::review_format::render_review_output_text; -use crate::review_persistence::AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY; +use crate::review_persistence::ReviewInterruptionStatus; use crate::review_persistence::ReviewPersistenceContext; +use crate::review_persistence::SavedReviewInterruption; use crate::session::Codex; use crate::session::TurnInput; use crate::session::session::Session; @@ -35,10 +41,19 @@ use codex_protocol::user_input::UserInput; use super::SessionTask; use super::SessionTaskContext; +const BACKGROUND_AUTO_REVIEW_PROGRESS_INTERVAL: Duration = Duration::from_secs(1); + pub(crate) struct ReviewTask { persistence: Option, } +struct ReviewExecution { + output: Option, + token_usage: Option, + usage: AutoReviewUsage, + terminal_reason: Option, +} + impl ReviewTask { pub(crate) fn new() -> Self { Self { persistence: None } @@ -93,12 +108,11 @@ impl SessionTask for ReviewTask { .is_none_or(ReviewPersistenceContext::is_manual); if let Some(persistence) = self.persistence.clone() { let codex_home = session.codex_home().await; - if persistence.save_cancelled(codex_home) { - send_background_auto_review_status( + if let Some(interruption) = persistence.save_interrupted(codex_home) { + send_interrupted_background_auto_review_status( session.clone_session(), &persistence, - BackgroundAutoReviewStatus::Cancelled, - Some(AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string()), + interruption, ) .await; } @@ -143,54 +157,143 @@ async fn run_review_task( } } + let review_started = tokio::time::Instant::now(); + let budget = persistence + .as_ref() + .and_then(ReviewPersistenceContext::background_budget) + .cloned(); + let codex_home = session.codex_home().await; + // Start sub-codex conversation and get the receiver for events. - let (output, token_usage) = match start_review_conversation( + let start_conversation = Box::pin(start_review_conversation( session.clone(), ctx.clone(), user_input, cancellation_token.clone(), - ) - .await - { - Some(review_codex) => { - let output = process_review_events(session.clone(), ctx.clone(), &review_codex).await; - let token_usage = review_codex.session.token_usage_info().await; - (output, token_usage.map(|info| info.total_token_usage)) + )); + let start_result: Result, ReviewExecution> = if let Some(budget) = &budget { + let remaining = + Duration::from_millis(budget.max_elapsed_ms).saturating_sub(review_started.elapsed()); + tokio::select! { + biased; + review_codex = start_conversation => Ok(review_codex), + _ = tokio::time::sleep(remaining) => { + cancellation_token.cancel(); + Err(ReviewExecution { + output: None, + token_usage: None, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_millis(review_started.elapsed())), + ..Default::default() + }, + terminal_reason: Some(AutoReviewTerminalReason::BudgetElapsed), + }) + } + } + } else { + Ok(start_conversation.await) + }; + let execution = match start_result { + Err(execution) => execution, + Ok(Some(review_codex)) => { + Box::pin(execute_review_conversation( + session.clone(), + ctx.clone(), + &review_codex, + persistence.as_ref(), + &codex_home, + budget.as_ref(), + review_started, + cancellation_token.clone(), + )) + .await + } + Ok(None) => { + let elapsed_ms = elapsed_millis(review_started.elapsed()); + let terminal_reason = budget.as_ref().and_then(|budget| { + elapsed_reaches_budget(elapsed_ms, budget) + .then_some(AutoReviewTerminalReason::BudgetElapsed) + }); + ReviewExecution { + output: None, + token_usage: None, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + ..Default::default() + }, + terminal_reason, + } } - None => (None, None), }; let cancelled = cancellation_token.is_cancelled(); let should_exit_review_mode = persistence .as_ref() .is_none_or(ReviewPersistenceContext::is_manual); - if !cancelled && should_exit_review_mode { - exit_review_mode(session.clone_session(), output.clone(), ctx.clone()).await; + if should_exit_review_mode && (!cancelled || execution.output.is_some()) { + exit_review_mode( + session.clone_session(), + execution.output.clone(), + ctx.clone(), + ) + .await; } if let Some(persistence) = persistence { - let codex_home = session.codex_home().await; - if cancelled { - if persistence.save_cancelled(codex_home) { + if let Some(output) = execution.output.as_ref() { + let freshness = if persistence.is_background() { + persistence.completion_freshness(&codex_home).await + } else { + codex_auto_review::AutoReviewRunFreshness::Current + }; + if persistence.save_completed_with_freshness( + &codex_home, + output, + execution.token_usage.as_ref(), + freshness, + execution.usage, + ) { send_background_auto_review_status( session.clone_session(), &persistence, - BackgroundAutoReviewStatus::Cancelled, - Some(AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY.to_string()), + BackgroundAutoReviewStatus::Completed, + None, ) .await; } - } else if let Some(output) = output.as_ref() { - if persistence.save_completed(codex_home, output, token_usage.as_ref()) { + } else if let Some(reason) = execution.terminal_reason { + let error_summary = + background_review_budget_summary(reason, budget.as_ref(), &execution.usage); + if persistence.save_budget_cancelled( + &codex_home, + reason, + error_summary.clone(), + execution.token_usage.as_ref(), + execution.usage, + ) { send_background_auto_review_status( session.clone_session(), &persistence, - BackgroundAutoReviewStatus::Completed, - None, + BackgroundAutoReviewStatus::Cancelled, + Some(error_summary), + ) + .await; + } + } else if cancelled { + if let Some(interruption) = persistence.save_interrupted(codex_home) { + send_interrupted_background_auto_review_status( + session.clone_session(), + &persistence, + interruption, ) .await; } } else { let error_summary = "review ended without producing review output".to_string(); - if persistence.save_failed(codex_home, error_summary.clone(), token_usage.as_ref()) { + if persistence.save_empty_output( + &codex_home, + error_summary.clone(), + execution.token_usage.as_ref(), + execution.usage, + ) { send_background_auto_review_status( session.clone_session(), &persistence, @@ -204,6 +307,24 @@ async fn run_review_task( None } +async fn send_interrupted_background_auto_review_status( + session: Arc, + persistence: &ReviewPersistenceContext, + interruption: SavedReviewInterruption, +) { + let status = match interruption.status { + ReviewInterruptionStatus::Cancelled => BackgroundAutoReviewStatus::Cancelled, + ReviewInterruptionStatus::Superseded => BackgroundAutoReviewStatus::Superseded, + }; + send_background_auto_review_status( + session, + persistence, + status, + Some(interruption.error_summary), + ) + .await; +} + async fn send_background_auto_review_status( session: Arc, persistence: &ReviewPersistenceContext, @@ -213,17 +334,22 @@ async fn send_background_auto_review_status( if !persistence.is_background() { return; } - session - .send_event_raw(Event { - id: persistence.run_id().to_string(), - msg: EventMsg::BackgroundAutoReviewStatus(BackgroundAutoReviewStatusEvent { - run_id: persistence.run_id().to_string(), - status, - review_target: persistence.review_target().clone(), - error_summary, - }), - }) - .await; + let event = Event { + id: persistence.run_id().to_string(), + msg: EventMsg::BackgroundAutoReviewStatus(BackgroundAutoReviewStatusEvent { + run_id: persistence.run_id().to_string(), + status, + review_target: persistence.review_target().clone(), + error_summary, + }), + }; + if let Err(err) = tokio::spawn(async move { + session.send_event_raw(event).await; + }) + .await + { + tracing::warn!(error = %err, "background auto review status task failed"); + } } async fn start_review_conversation( @@ -275,7 +401,7 @@ async fn process_review_events( session: Arc, ctx: Arc, review_codex: &Codex, -) -> Option { +) -> Option { let mut prev_agent_message: Option = None; while let Ok(event) = review_codex.next_event().await { match event.clone().msg { @@ -297,15 +423,9 @@ async fn process_review_events( }) | EventMsg::AgentMessageContentDelta(AgentMessageContentDeltaEvent { .. }) => {} EventMsg::TurnComplete(task_complete) => { - // Parse review output from the last agent message (if present). - let out = task_complete - .last_agent_message - .as_deref() - .map(parse_review_output_event); - return out; + return task_complete.last_agent_message; } EventMsg::TurnAborted(_) => { - // Cancellation or abort: consumer will finalize with None. return None; } other => { @@ -316,10 +436,363 @@ async fn process_review_events( } } } - // Channel closed without TurnComplete: treat as interrupted. None } +async fn execute_review_conversation( + session: Arc, + ctx: Arc, + review_codex: &Codex, + persistence: Option<&ReviewPersistenceContext>, + codex_home: &std::path::Path, + budget: Option<&AutoReviewBudget>, + started: tokio::time::Instant, + cancellation_token: CancellationToken, +) -> ReviewExecution { + let events = Box::pin(process_review_events(session, ctx, review_codex)); + let raw_output = if let Some(budget) = budget { + let monitor = Box::pin(wait_for_review_budget( + review_codex, + persistence, + codex_home, + budget, + started, + )); + tokio::select! { + biased; + raw_output = events => raw_output, + execution = monitor => { + cancellation_token.cancel(); + return execution; + } + } + } else { + events.await + }; + let token_usage = review_codex + .session + .token_usage_info() + .await + .map(|info| info.total_token_usage); + let total_tokens = token_usage.as_ref().and_then(review_token_count); + let elapsed_ms = elapsed_millis(started.elapsed()); + if budget.is_some_and(|budget| elapsed_exceeds_budget(elapsed_ms, budget)) { + return ReviewExecution { + output: None, + token_usage, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + ..Default::default() + }, + terminal_reason: Some(AutoReviewTerminalReason::BudgetElapsed), + }; + } + if budget.is_some_and(|budget| { + total_tokens.is_some_and(|total_tokens| total_tokens_exceed_budget(total_tokens, budget)) + }) { + return ReviewExecution { + output: None, + token_usage, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + ..Default::default() + }, + terminal_reason: Some(AutoReviewTerminalReason::BudgetTotalTokens), + }; + } + let Some(raw_output) = raw_output else { + return ReviewExecution { + output: None, + token_usage, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + ..Default::default() + }, + terminal_reason: None, + }; + }; + let output_bytes = raw_output.len(); + if budget.is_some_and(|budget| raw_output_exceeds_budget(&raw_output, budget)) { + return ReviewExecution { + output: None, + token_usage, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + output_bytes: Some(output_bytes), + ..Default::default() + }, + terminal_reason: Some(AutoReviewTerminalReason::BudgetOutput), + }; + } + let output = parse_review_output_event(&raw_output); + let finding_count = output.findings.len(); + if budget.is_some_and(|budget| findings_exceed_budget(&output, budget)) { + return ReviewExecution { + output: None, + token_usage, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + output_bytes: Some(output_bytes), + finding_count: Some(finding_count), + ..Default::default() + }, + terminal_reason: Some(AutoReviewTerminalReason::BudgetFindingCount), + }; + } + ReviewExecution { + output: Some(output), + token_usage, + usage: AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + output_bytes: Some(output_bytes), + finding_count: Some(finding_count), + ..Default::default() + }, + terminal_reason: None, + } +} + +async fn wait_for_review_budget( + review_codex: &Codex, + persistence: Option<&ReviewPersistenceContext>, + codex_home: &std::path::Path, + budget: &AutoReviewBudget, + started: tokio::time::Instant, +) -> ReviewExecution { + let mut last_persisted_elapsed_ms = 0; + let mut last_persisted_total_tokens = None; + loop { + tokio::time::sleep(Duration::from_millis(100)).await; + let elapsed_ms = elapsed_millis(started.elapsed()); + let token_usage = review_codex + .session + .token_usage_info() + .await + .map(|info| info.total_token_usage); + let total_tokens = token_usage.as_ref().and_then(review_token_count); + let usage = AutoReviewUsage { + elapsed_ms: Some(elapsed_ms), + total_tokens, + ..Default::default() + }; + if let Some(persistence) = persistence + && (elapsed_ms.saturating_sub(last_persisted_elapsed_ms) + >= elapsed_millis(BACKGROUND_AUTO_REVIEW_PROGRESS_INTERVAL) + || total_tokens != last_persisted_total_tokens) + { + persistence.record_progress(codex_home, usage.clone()); + last_persisted_elapsed_ms = elapsed_ms; + last_persisted_total_tokens = total_tokens; + } + if elapsed_reaches_budget(elapsed_ms, budget) { + return ReviewExecution { + output: None, + token_usage, + usage, + terminal_reason: Some(AutoReviewTerminalReason::BudgetElapsed), + }; + } + if total_tokens.is_some_and(|total_tokens| total_tokens_reach_budget(total_tokens, budget)) + { + return ReviewExecution { + output: None, + token_usage, + usage, + terminal_reason: Some(AutoReviewTerminalReason::BudgetTotalTokens), + }; + } + } +} + +fn review_token_count(token_usage: &TokenUsage) -> Option { + let reconstructed_total = token_usage + .non_cached_input() + .saturating_add(token_usage.cached_input()) + .saturating_add(token_usage.output_tokens.max(0)); + u64::try_from(token_usage.total_tokens.max(reconstructed_total)) + .ok() + .filter(|total_tokens| *total_tokens > 0) +} + +fn elapsed_reaches_budget(elapsed_ms: u64, budget: &AutoReviewBudget) -> bool { + elapsed_ms >= budget.max_elapsed_ms +} + +fn elapsed_exceeds_budget(elapsed_ms: u64, budget: &AutoReviewBudget) -> bool { + elapsed_ms > budget.max_elapsed_ms +} + +fn total_tokens_reach_budget(total_tokens: u64, budget: &AutoReviewBudget) -> bool { + total_tokens >= budget.max_total_tokens +} + +fn total_tokens_exceed_budget(total_tokens: u64, budget: &AutoReviewBudget) -> bool { + total_tokens > budget.max_total_tokens +} + +fn raw_output_exceeds_budget(raw_output: &str, budget: &AutoReviewBudget) -> bool { + raw_output.len() > budget.max_output_bytes +} + +fn findings_exceed_budget(output: &ReviewOutputEvent, budget: &AutoReviewBudget) -> bool { + output.findings.len() > budget.max_findings +} + +fn elapsed_millis(elapsed: Duration) -> u64 { + u64::try_from(elapsed.as_millis()).unwrap_or(u64::MAX) +} + +fn background_review_budget_summary( + reason: AutoReviewTerminalReason, + budget: Option<&AutoReviewBudget>, + usage: &AutoReviewUsage, +) -> String { + let Some(budget) = budget else { + return "background review stopped after exceeding its execution budget".to_string(); + }; + match reason { + AutoReviewTerminalReason::BudgetElapsed => format!( + "background review exceeded elapsed budget: {} ms >= {} ms", + usage.elapsed_ms.unwrap_or_default(), + budget.max_elapsed_ms + ), + AutoReviewTerminalReason::BudgetTotalTokens => format!( + "background review exceeded token budget: {} tokens >= {} tokens", + usage.total_tokens.unwrap_or_default(), + budget.max_total_tokens + ), + AutoReviewTerminalReason::BudgetOutput => format!( + "background review exceeded output budget: {} bytes > {} bytes", + usage.output_bytes.unwrap_or_default(), + budget.max_output_bytes + ), + AutoReviewTerminalReason::BudgetFindingCount => format!( + "background review exceeded finding budget: {} findings > {} findings", + usage.finding_count.unwrap_or_default(), + budget.max_findings + ), + AutoReviewTerminalReason::BudgetScope + | AutoReviewTerminalReason::EmptyOutput + | AutoReviewTerminalReason::StaleTarget => { + "background review stopped after exceeding its execution budget".to_string() + } + } +} + +#[cfg(test)] +mod budget_tests { + use super::*; + use codex_protocol::protocol::ReviewCodeLocation; + use codex_protocol::protocol::ReviewFinding; + use codex_protocol::protocol::ReviewLineRange; + use std::path::PathBuf; + + #[test] + fn oversized_output_exceeds_budget() { + let budget = test_budget(); + assert!(raw_output_exceeds_budget( + &"x".repeat(budget.max_output_bytes + 1), + &budget + )); + assert!(!raw_output_exceeds_budget( + &"x".repeat(budget.max_output_bytes), + &budget + )); + } + + #[test] + fn excess_findings_exceed_budget() { + let budget = AutoReviewBudget { + max_findings: 1, + ..test_budget() + }; + let finding = ReviewFinding { + title: "[P1] Finding".to_string(), + body: "body".to_string(), + confidence_score: 0.9, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from("/tmp/src/lib.rs"), + line_range: ReviewLineRange { start: 1, end: 1 }, + }, + }; + let output = ReviewOutputEvent { + findings: vec![finding.clone(), finding], + ..Default::default() + }; + + assert!(findings_exceed_budget(&output, &budget)); + } + + #[test] + fn budget_summary_is_structured_and_bounded() { + let summary = background_review_budget_summary( + AutoReviewTerminalReason::BudgetTotalTokens, + Some(&test_budget()), + &AutoReviewUsage { + total_tokens: Some(250_001), + ..Default::default() + }, + ); + assert_eq!( + summary, + "background review exceeded token budget: 250001 tokens >= 250000 tokens" + ); + } + + #[test] + fn elapsed_budget_is_enforced_at_the_deadline() { + let budget = test_budget(); + assert!(!elapsed_reaches_budget(budget.max_elapsed_ms - 1, &budget)); + assert!(elapsed_reaches_budget(budget.max_elapsed_ms, &budget)); + assert!(!elapsed_exceeds_budget(budget.max_elapsed_ms, &budget)); + assert!(elapsed_exceeds_budget(budget.max_elapsed_ms + 1, &budget)); + } + + #[test] + fn token_count_includes_cached_input() { + let token_usage = TokenUsage { + input_tokens: 100, + cached_input_tokens: 90, + output_tokens: 10, + reasoning_output_tokens: 0, + total_tokens: 20, + }; + + assert_eq!(review_token_count(&token_usage), Some(110)); + } + + #[test] + fn completed_output_at_exact_token_limit_is_allowed() { + let budget = test_budget(); + assert!(total_tokens_reach_budget(budget.max_total_tokens, &budget)); + assert!(!total_tokens_exceed_budget( + budget.max_total_tokens, + &budget + )); + assert!(total_tokens_exceed_budget( + budget.max_total_tokens + 1, + &budget + )); + } + + fn test_budget() -> AutoReviewBudget { + AutoReviewBudget { + max_scope_bytes: 120_000, + max_elapsed_ms: 300_000, + max_total_tokens: 250_000, + max_output_bytes: 64, + max_findings: 20, + } + } +} + /// Parse a ReviewOutputEvent from a text blob returned by the reviewer model. /// If the text is valid JSON matching ReviewOutputEvent, deserialize it. /// Otherwise, attempt to extract the first JSON object substring and parse it. diff --git a/codex-rs/core/src/tools/handlers/auto_review_disposition.rs b/codex-rs/core/src/tools/handlers/auto_review_disposition.rs new file mode 100644 index 000000000000..c5fa12c7997f --- /dev/null +++ b/codex-rs/core/src/tools/handlers/auto_review_disposition.rs @@ -0,0 +1,287 @@ +use codex_auto_review::AutoReviewDispositionActor; +use codex_auto_review::AutoReviewFindingDisposition; +use codex_auto_review::AutoReviewFindingDispositionRecord; +use codex_auto_review::AutoReviewStore; +use codex_protocol::protocol::ReviewTarget; +use codex_tools::ToolName; +use codex_tools::ToolSpec; +use serde::Deserialize; +use serde_json::Value; +use serde_json::json; + +use crate::function_tool::FunctionCallError; +use crate::review_persistence::collect_auto_review_target; +use crate::tools::context::FunctionToolOutput; +use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolPayload; +use crate::tools::context::boxed_tool_output; +use crate::tools::handlers::auto_review_disposition_spec::AUTO_REVIEW_DISPOSITION_TOOL_NAME; +use crate::tools::handlers::auto_review_disposition_spec::create_auto_review_disposition_tool; +use crate::tools::handlers::parse_arguments; +use crate::tools::registry::CoreToolRuntime; +use crate::tools::registry::ToolExecutor; +use crate::turn_timing::now_unix_timestamp_ms; + +const AUTO_REVIEW_REPAIR_RESPONSE_MAX_BYTES: usize = 4 * 1024; + +pub(crate) struct AutoReviewDispositionHandler; + +#[derive(Debug, Deserialize)] +struct AutoReviewDispositionArgs { + run_id: String, + action: AutoReviewDispositionAction, + #[serde(default)] + reason: Option, +} + +#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +enum AutoReviewDispositionAction { + Repair, + Defer, + Obsolete, +} + +#[async_trait::async_trait] +impl ToolExecutor for AutoReviewDispositionHandler { + fn tool_name(&self) -> ToolName { + ToolName::plain(AUTO_REVIEW_DISPOSITION_TOOL_NAME) + } + + fn spec(&self) -> ToolSpec { + create_auto_review_disposition_tool() + } + + async fn handle( + &self, + invocation: ToolInvocation, + ) -> Result, FunctionCallError> { + let ToolInvocation { turn, payload, .. } = invocation; + let arguments = match payload { + ToolPayload::Function { arguments } => arguments, + _ => { + return Err(FunctionCallError::RespondToModel(format!( + "{AUTO_REVIEW_DISPOSITION_TOOL_NAME} handler received unsupported payload" + ))); + } + }; + let args: AutoReviewDispositionArgs = parse_arguments(&arguments)?; + let active_review_target = ReviewTarget::UncommittedChanges; + let cwd = turn + .environments + .single_local_environment_cwd() + .map(std::convert::AsRef::as_ref) + .unwrap_or_else(|| turn.config.cwd.as_ref()); + let active_target = + collect_auto_review_target(turn.config.codex_home.as_ref(), cwd, &active_review_target) + .await; + let store_scope = active_target.worktree_path.as_deref().unwrap_or(cwd); + let store = AutoReviewStore::for_scope(turn.config.codex_home.as_ref(), store_scope); + let run = store.load_run(&args.run_id).map_err(respond_to_model)?; + if args.action != AutoReviewDispositionAction::Obsolete + && !run.can_read_detail(&active_target, &active_review_target) + { + return Err(FunctionCallError::RespondToModel( + "repair/defer requires a completed Background Review with current findings" + .to_string(), + )); + } + + let reason = args + .reason + .map(|reason| reason.trim().to_string()) + .filter(|reason| !reason.is_empty()); + let disposition = match args.action { + AutoReviewDispositionAction::Repair => AutoReviewFindingDisposition::Repairing, + AutoReviewDispositionAction::Defer => AutoReviewFindingDisposition::Deferred, + AutoReviewDispositionAction::Obsolete => AutoReviewFindingDisposition::Obsolete, + }; + let is_repair = args.action == AutoReviewDispositionAction::Repair; + let repair_detail = if is_repair { + Some( + store + .detail( + &args.run_id, + /*finding_id*/ None, + AUTO_REVIEW_REPAIR_RESPONSE_MAX_BYTES, + ) + .map_err(respond_to_model)?, + ) + } else { + None + }; + let record = AutoReviewFindingDispositionRecord { + disposition, + actor: AutoReviewDispositionActor::Agent, + reason: reason.or_else(|| { + (args.action == AutoReviewDispositionAction::Repair) + .then(|| "bounded repair turn opened by agent".to_string()) + }), + updated_at_unix_secs: now_unix_timestamp_ms() / 1_000, + }; + let state = store + .set_finding_disposition(&args.run_id, record) + .map_err(respond_to_model)?; + let record = state.finding_disposition.ok_or_else(|| { + FunctionCallError::Fatal("auto review disposition was not persisted".to_string()) + })?; + + let mut response = json!({ + "status": "ok", + "run_id": args.run_id, + "disposition": record.disposition, + "actor": record.actor, + "reason": record.reason, + }); + if let Some(detail) = repair_detail { + response["repair_detail"] = json!({ + "bytes": detail.bytes, + "original_bytes": detail.original_bytes, + "max_bytes": detail.max_bytes, + "truncated": detail.truncated, + "finding_count": detail.finding_count, + "omitted_findings": detail.omitted_findings, + "content": detail.content, + }); + } + let output = if is_repair { + serialize_bounded_repair_response(response)? + } else { + serialize_response(&response)? + }; + Ok(boxed_tool_output(FunctionToolOutput::from_text( + output, + Some(true), + ))) + } +} + +impl CoreToolRuntime for AutoReviewDispositionHandler {} + +fn respond_to_model(err: impl std::fmt::Display) -> FunctionCallError { + FunctionCallError::RespondToModel(err.to_string()) +} + +fn serialize_bounded_repair_response(mut response: Value) -> Result { + let output = serialize_response(&response)?; + if output.len() <= AUTO_REVIEW_REPAIR_RESPONSE_MAX_BYTES { + return Ok(output); + } + let content = response + .pointer("/repair_detail/content") + .and_then(Value::as_str) + .ok_or_else(|| { + FunctionCallError::Fatal( + "auto review repair response is missing detail content".to_string(), + ) + })? + .to_string(); + let was_truncated = response + .pointer("/repair_detail/truncated") + .and_then(Value::as_bool) + .unwrap_or_default(); + let mut boundaries = content + .char_indices() + .map(|(index, _)| index) + .collect::>(); + if boundaries.first().copied() != Some(0) { + boundaries.push(0); + } + if boundaries.last().copied() != Some(content.len()) { + boundaries.push(content.len()); + } + boundaries.sort_unstable(); + boundaries.dedup(); + + let mut lower = 0; + let mut upper = boundaries.len().saturating_sub(1); + while lower < upper { + let middle = (lower + upper + 1) / 2; + let end = boundaries[middle]; + set_repair_detail_content( + &mut response, + &content[..end], + was_truncated || end < content.len(), + )?; + if serialize_response(&response)?.len() <= AUTO_REVIEW_REPAIR_RESPONSE_MAX_BYTES { + lower = middle; + } else { + upper = middle - 1; + } + } + + let end = boundaries[lower]; + set_repair_detail_content( + &mut response, + &content[..end], + was_truncated || end < content.len(), + )?; + let output = serialize_response(&response)?; + if output.len() > AUTO_REVIEW_REPAIR_RESPONSE_MAX_BYTES { + return Err(FunctionCallError::Fatal( + "auto review repair response metadata exceeds the output limit".to_string(), + )); + } + Ok(output) +} + +fn set_repair_detail_content( + response: &mut Value, + content: &str, + truncated: bool, +) -> Result<(), FunctionCallError> { + let detail = response + .get_mut("repair_detail") + .and_then(Value::as_object_mut) + .ok_or_else(|| { + FunctionCallError::Fatal( + "auto review repair response is missing detail metadata".to_string(), + ) + })?; + detail.insert("bytes".to_string(), json!(content.len())); + detail.insert("truncated".to_string(), json!(truncated)); + detail.insert("content".to_string(), json!(content)); + Ok(()) +} + +fn serialize_response(response: &Value) -> Result { + serde_json::to_string(response).map_err(|err| { + FunctionCallError::Fatal(format!( + "failed to serialize {AUTO_REVIEW_DISPOSITION_TOOL_NAME} response: {err}" + )) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn repair_response_is_bounded_after_json_escaping() { + let response = json!({ + "status": "ok", + "run_id": "run-1", + "disposition": "repairing", + "actor": "agent", + "reason": "bounded repair turn opened by agent", + "repair_detail": { + "bytes": 4096, + "original_bytes": 4096, + "max_bytes": 4096, + "truncated": false, + "finding_count": 1, + "omitted_findings": 0, + "content": "\u{0000}".repeat(4096), + }, + }); + + let output = serialize_bounded_repair_response(response).expect("bounded response"); + + assert!(output.len() <= AUTO_REVIEW_REPAIR_RESPONSE_MAX_BYTES); + let parsed: Value = serde_json::from_str(&output).expect("valid JSON response"); + assert_eq!( + parsed.pointer("/repair_detail/truncated"), + Some(&Value::Bool(true)) + ); + } +} diff --git a/codex-rs/core/src/tools/handlers/auto_review_disposition_spec.rs b/codex-rs/core/src/tools/handlers/auto_review_disposition_spec.rs new file mode 100644 index 000000000000..86f0057305a5 --- /dev/null +++ b/codex-rs/core/src/tools/handlers/auto_review_disposition_spec.rs @@ -0,0 +1,48 @@ +use codex_tools::JsonSchema; +use codex_tools::ResponsesApiTool; +use codex_tools::ToolSpec; +use serde_json::json; +use std::collections::BTreeMap; + +pub(crate) const AUTO_REVIEW_DISPOSITION_TOOL_NAME: &str = "auto_review_disposition"; + +pub(crate) fn create_auto_review_disposition_tool() -> ToolSpec { + let properties = BTreeMap::from([ + ( + "run_id".to_string(), + JsonSchema::string(Some( + "Stable Background Review run id from auto-review awareness.".to_string(), + )), + ), + ( + "action".to_string(), + JsonSchema::string_enum( + vec![json!("repair"), json!("defer"), json!("obsolete")], + Some( + "repair opens a bounded repair context, defer acknowledges the findings for later, and obsolete dismisses them with a required reason." + .to_string(), + ), + ), + ), + ( + "reason".to_string(), + JsonSchema::string(Some( + "Short audited reason. Required when action is obsolete.".to_string(), + )), + ), + ]); + + ToolSpec::Function(ResponsesApiTool { + name: AUTO_REVIEW_DISPOSITION_TOOL_NAME.to_string(), + description: "Disposition current Background Review findings. Use repair before applying a bounded fix, defer to acknowledge them for later, or obsolete with a reason when they no longer apply. Repair returns only bounded review detail." + .to_string(), + strict: false, + defer_loading: None, + parameters: JsonSchema::object( + properties, + Some(vec!["run_id".to_string(), "action".to_string()]), + Some(false.into()), + ), + output_schema: None, + }) +} diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index bb89d3a0ddd0..28f94deae16b 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -2,6 +2,8 @@ pub(crate) mod agent_jobs; pub(crate) mod agent_jobs_spec; pub(crate) mod apply_patch; pub(crate) mod apply_patch_spec; +mod auto_review_disposition; +pub(crate) mod auto_review_disposition_spec; mod code_bridge; pub(crate) mod code_bridge_spec; mod dynamic; @@ -50,6 +52,7 @@ use crate::session::turn_context::TurnEnvironment; pub(crate) use crate::tools::code_mode::CodeModeExecuteHandler; pub(crate) use crate::tools::code_mode::CodeModeWaitHandler; pub use apply_patch::ApplyPatchHandler; +pub(crate) use auto_review_disposition::AutoReviewDispositionHandler; pub use code_bridge::CodeBridgeHandler; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::AskForApproval; diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index 3571fa5d7462..8b7c57f0394a 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -4,6 +4,7 @@ use crate::session::turn_context::TurnContext; use crate::tools::code_mode::execute_spec::create_code_mode_tool; use crate::tools::context::ToolInvocation; use crate::tools::handlers::ApplyPatchHandler; +use crate::tools::handlers::AutoReviewDispositionHandler; use crate::tools::handlers::CodeBridgeHandler; use crate::tools::handlers::CodeModeExecuteHandler; use crate::tools::handlers::CodeModeWaitHandler; @@ -646,6 +647,9 @@ fn add_core_utility_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut let environment_mode = turn_context.tool_environment_mode(); planned_tools.add(PlanHandler); + if !turn_context.session_source.is_non_root_agent() { + planned_tools.add(AutoReviewDispositionHandler); + } let code_bridge_tool_name = ToolName::plain(CODE_BRIDGE_TOOL_NAME); let code_bridge_owned_by_extension_tool = context .extension_tool_executors diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 54562edbbfbd..cdf06118f1ec 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -192,6 +192,23 @@ async fn probe(configure_turn: impl FnOnce(&mut TurnContext)) -> ToolPlanProbe { probe_with(configure_turn, ToolPlanInputs::default()).await } +#[tokio::test] +async fn root_session_exposes_auto_review_disposition() { + let plan = probe(|_| {}).await; + plan.assert_visible_contains(&["auto_review_disposition"]); + plan.assert_registered_contains(&["auto_review_disposition"]); +} + +#[tokio::test] +async fn review_subagent_cannot_disposition_findings() { + let plan = probe(|turn| { + turn.session_source = SessionSource::SubAgent(SubAgentSource::Review); + }) + .await; + plan.assert_visible_lacks(&["auto_review_disposition"]); + plan.assert_registered_lacks(&["auto_review_disposition"]); +} + fn set_feature(turn: &mut TurnContext, feature: Feature, enabled: bool) { if enabled { turn.features diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index b4e5c43fa537..e6c649fc0d1c 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -3,6 +3,7 @@ use codex_auto_review::AutoReviewRunFreshness; use codex_auto_review::AutoReviewRunSource; use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewStore; +use codex_auto_review::AutoReviewTerminalReason; use codex_auto_review::ReviewCoordination; use codex_auto_review::SCHEMA_VERSION as AUTO_REVIEW_SCHEMA_VERSION; use codex_core::CodexThread; @@ -374,6 +375,27 @@ async fn review_op_with_persistence_writes_auto_review_run() { assert_eq!(run.finding_digests.len(), 1); assert_eq!(run.finding_digests[0].finding_id, "f1"); assert_eq!(run.finding_digests[0].title, "Persist this finding"); + let state = AutoReviewStore::for_scope( + codex_home.path(), + run.target + .worktree_path + .as_deref() + .expect("manual review worktree path"), + ) + .load_run_state(&run.run_id) + .expect("load manual auto review state") + .expect("manual auto review state"); + let disposition = state + .finding_disposition + .expect("manual findings should require disposition"); + assert_eq!( + disposition.disposition, + codex_auto_review::AutoReviewFindingDisposition::NeedsAttention + ); + assert_eq!( + disposition.actor, + codex_auto_review::AutoReviewDispositionActor::System + ); let detail = load_auto_review_finding_detail(codex_home.path(), &run.run_id, "f1") .expect("load auto review finding detail"); assert!(detail.content.contains("Persist this finding")); @@ -2201,7 +2223,7 @@ async fn automatic_background_review_skips_oversized_diff() -> anyhow::Result<() let test = test_codex() .with_home(codex_home.clone()) .with_config(|config| { - config.background_auto_review_max_diff_bytes = Some(1); + config.background_auto_review_budget.max_scope_bytes = 1; }) .build_with_streaming_server(&server) .await?; @@ -2273,7 +2295,7 @@ async fn automatic_background_review_skips_oversized_wrapped_scope() -> anyhow:: let test = test_codex() .with_home(codex_home.clone()) .with_config(|config| { - config.background_auto_review_max_diff_bytes = Some(360); + config.background_auto_review_budget.max_scope_bytes = 360; }) .build_with_streaming_server(&server) .await?; @@ -2318,6 +2340,185 @@ async fn automatic_background_review_skips_oversized_wrapped_scope() -> anyhow:: Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn automatic_background_review_cancels_at_token_budget() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let patch = "*** Begin Patch\n*** Add File: auto_background_review_token_budget.txt\n+review me\n*** End Patch"; + let review_json = serde_json::json!({ + "findings": [], + "overall_correctness": "patch is correct", + "overall_explanation": "Review completed above the configured token ceiling.", + "overall_confidence_score": 0.9 + }) + .to_string(); + let (server, _completions) = start_streaming_sse_server(vec![ + vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_custom_tool_call("auto-bg-token-budget", patch), + ev_completed("resp-1"), + ]), + }], + vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_assistant_message("msg-1", "patch applied"), + ev_completed("resp-2"), + ]), + }], + vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_assistant_message("msg-2", &review_json), + ev_completed_with_tokens("resp-3", 101), + ]), + }], + ]) + .await; + let codex_home = Arc::new(TempDir::new()?); + let test = test_codex() + .with_home(codex_home.clone()) + .with_config(|config| { + config.background_auto_review_budget.max_total_tokens = 100; + }) + .build_with_streaming_server(&server) + .await?; + init_git_repo(test.cwd_path()); + + test.submit_turn("create a file for a bounded background review") + .await?; + let pending = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Pending, + None, + ) + .await; + let running = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Running, + Some(pending.run_id.as_str()), + ) + .await; + let cancelled = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Cancelled, + Some(running.run_id.as_str()), + ) + .await; + + assert!( + cancelled + .error_summary + .as_deref() + .is_some_and(|summary| summary.contains("101 tokens >= 100 tokens")) + ); + let run = load_single_auto_review_run(codex_home.path())?; + let state = load_auto_review_run_state(codex_home.path(), &run.run_id)?; + assert_eq!(run.status, AutoReviewRunStatus::Cancelled); + assert_eq!(run.cancel_reason.as_deref(), Some("budget_total_tokens")); + assert_eq!(run.token_count, Some(101)); + assert_eq!(state.usage.total_tokens, Some(101)); + assert_eq!( + state.budget.map(|budget| budget.max_total_tokens), + Some(100) + ); + assert_eq!( + state.terminal_reason, + Some(AutoReviewTerminalReason::BudgetTotalTokens) + ); + + server.shutdown().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn automatic_background_review_cancels_at_elapsed_budget() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let patch = "*** Begin Patch\n*** Add File: auto_background_review_elapsed_budget.txt\n+review me\n*** End Patch"; + let (release_review_tx, release_review_rx) = oneshot::channel(); + let (server, _completions) = start_streaming_sse_server(vec![ + vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_custom_tool_call("auto-bg-elapsed-budget", patch), + ev_completed("resp-1"), + ]), + }], + vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_assistant_message("msg-1", "patch applied"), + ev_completed("resp-2"), + ]), + }], + vec![ + StreamingSseChunk { + gate: None, + body: streaming_sse_event(ev_response_created("resp-3")), + }, + StreamingSseChunk { + gate: Some(release_review_rx), + body: streaming_sse_event(ev_completed("resp-3")), + }, + ], + ]) + .await; + let codex_home = Arc::new(TempDir::new()?); + let test = test_codex() + .with_home(codex_home.clone()) + .with_config(|config| { + config.background_auto_review_budget.max_elapsed_ms = 100; + }) + .build_with_streaming_server(&server) + .await?; + init_git_repo(test.cwd_path()); + + test.submit_turn("create a file for an elapsed-budget review") + .await?; + let pending = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Pending, + None, + ) + .await; + let running = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Running, + Some(pending.run_id.as_str()), + ) + .await; + let cancelled = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Cancelled, + Some(running.run_id.as_str()), + ) + .await; + + assert!( + cancelled + .error_summary + .as_deref() + .is_some_and(|summary| summary.contains("elapsed budget")) + ); + let run = load_single_auto_review_run(codex_home.path())?; + let state = load_auto_review_run_state(codex_home.path(), &run.run_id)?; + assert_eq!(run.status, AutoReviewRunStatus::Cancelled); + assert_eq!(run.cancel_reason.as_deref(), Some("budget_elapsed")); + assert_eq!( + state.terminal_reason, + Some(AutoReviewTerminalReason::BudgetElapsed) + ); + assert!(state.usage.elapsed_ms.is_some_and(|elapsed| elapsed >= 100)); + + drop(release_review_tx); + server.shutdown().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn automatic_background_review_suppresses_unchanged_dirty_diff() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); @@ -3589,6 +3790,26 @@ fn load_auto_review_finding_detail( anyhow::bail!("auto review run {run_id} not found") } +fn load_auto_review_run_state( + codex_home: &std::path::Path, + run_id: &str, +) -> anyhow::Result { + let review_dir = codex_home.join("state/review"); + for entry in std::fs::read_dir(&review_dir) + .map_err(|err| anyhow::anyhow!("auto review state dir: {err}"))? + { + let entry = entry?; + let store_root = entry.path().join("auto-review"); + let store = AutoReviewStore::from_store_root(store_root); + if store.load_run(run_id).is_ok() + && let Some(state) = store.load_run_state(run_id)? + { + return Ok(state); + } + } + anyhow::bail!("auto review run state {run_id} not found") +} + async fn submit_user_input_without_waiting( codex: &CodexThread, cwd: &std::path::Path, diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 1056048c9cc6..9072db249dfb 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -202,6 +202,18 @@ use tokio::sync::mpsc::unbounded_channel; use tokio::task::JoinHandle; use toml::Value as TomlValue; use uuid::Uuid; + +fn background_auto_review_status_has_summary(status: BackgroundAutoReviewStatus) -> bool { + matches!( + status, + BackgroundAutoReviewStatus::Completed + | BackgroundAutoReviewStatus::Failed + | BackgroundAutoReviewStatus::Cancelled + | BackgroundAutoReviewStatus::Superseded + | BackgroundAutoReviewStatus::Skipped + ) +} + mod agent_message_consolidation; mod agent_navigation; mod app_server_event_targets; diff --git a/codex-rs/tui/src/app/app_server_events.rs b/codex-rs/tui/src/app/app_server_events.rs index 9e5c741ced99..9bff963ee360 100644 --- a/codex-rs/tui/src/app/app_server_events.rs +++ b/codex-rs/tui/src/app/app_server_events.rs @@ -4,6 +4,7 @@ use super::App; use super::app_server_event_targets::ServerNotificationThreadTarget; use super::app_server_event_targets::server_notification_thread_target; use super::app_server_event_targets::server_request_thread_id; +use super::background_auto_review_status_has_summary; use crate::app_command::AppCommand; use crate::app_event::AppEvent; use crate::app_event::ConnectorsSnapshot; @@ -18,7 +19,6 @@ use codex_app_server_protocol::Account; use codex_app_server_protocol::AccountLoginCompletedNotification; use codex_app_server_protocol::AccountUpdatedNotification; use codex_app_server_protocol::AuthMode; -use codex_app_server_protocol::BackgroundAutoReviewStatus; use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::ServerRequest; use codex_protocol::ThreadId; @@ -151,7 +151,7 @@ impl App { let auto_review_summary_target = match ¬ification { ServerNotification::BackgroundAutoReviewStatusChanged(notification) - if notification.status == BackgroundAutoReviewStatus::Completed => + if background_auto_review_status_has_summary(notification.status) => { ThreadId::from_string(¬ification.thread_id) .ok() diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 01e16deb8084..a055b756507b 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -628,6 +628,44 @@ async fn replay_thread_snapshot_fetches_missing_completed_auto_review_summary() ); } +#[tokio::test] +async fn replay_thread_snapshot_fetches_missing_cancelled_auto_review_summary() { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + while app_event_rx.try_recv().is_ok() {} + let thread_id = ThreadId::new(); + + app.replay_thread_snapshot( + ThreadEventSnapshot { + session: Some(test_thread_session( + thread_id, + test_path_buf("/tmp/project"), + )), + turns: Vec::new(), + events: vec![ThreadBufferedEvent::Notification( + auto_review_status_notification( + thread_id, + "run-background-cancelled", + BackgroundAutoReviewStatus::Cancelled, + ), + )], + input_state: None, + }, + /*resume_restored_queue*/ true, + ); + + let emitted_events = drain_app_events(&mut app_event_rx); + assert!( + emitted_events.iter().any(|event| matches!( + event, + AppEvent::FetchAutoReviewSummary { + thread_id: fetched_thread_id, + run_id, + } if *fetched_thread_id == thread_id && run_id == "run-background-cancelled" + )), + "expected replay to fetch the cancelled auto-review summary, got {emitted_events:?}" + ); +} + #[tokio::test] async fn replay_thread_snapshot_replay_only_does_not_fetch_auto_review_summary() { let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 9bf2cae37277..70efeeab57d3 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -1545,7 +1545,7 @@ impl App { else { return; }; - if notification.status != BackgroundAutoReviewStatus::Completed + if !background_auto_review_status_has_summary(notification.status) || replayed_auto_review_summaries.contains(¬ification.run_id) { return; diff --git a/codex-rs/tui/src/chatwidget/tests/app_server.rs b/codex-rs/tui/src/chatwidget/tests/app_server.rs index 0f549c000325..24a601988240 100644 --- a/codex-rs/tui/src/chatwidget/tests/app_server.rs +++ b/codex-rs/tui/src/chatwidget/tests/app_server.rs @@ -502,6 +502,10 @@ async fn stale_auto_review_summary_result_is_ignored() { omitted_findings: 0, truncated: false, content: String::new(), + budget: None, + usage: Default::default(), + terminal_reason: None, + finding_disposition: None, }; chat.handle_auto_review_summary_loaded( diff --git a/codex-rs/tui/src/history_cell/auto_review_status.rs b/codex-rs/tui/src/history_cell/auto_review_status.rs index f32adb7044f8..ea7092194d19 100644 --- a/codex-rs/tui/src/history_cell/auto_review_status.rs +++ b/codex-rs/tui/src/history_cell/auto_review_status.rs @@ -2,10 +2,12 @@ use super::*; use codex_app_server_protocol::AutoReviewDiagnosticsSummary; +use codex_app_server_protocol::AutoReviewFindingDisposition; use codex_app_server_protocol::AutoReviewFreshness; use codex_app_server_protocol::AutoReviewRunSummary; use codex_app_server_protocol::AutoReviewStatusCount; use codex_app_server_protocol::AutoReviewSummaryReadResponse; +use codex_app_server_protocol::AutoReviewTerminalReason; use codex_app_server_protocol::BackgroundAutoReviewStatus; use codex_app_server_protocol::BackgroundAutoReviewStatusChangedNotification; use codex_app_server_protocol::ReviewTarget; @@ -151,6 +153,37 @@ fn push_summary_metadata(lines: &mut Vec>, summary: &AutoReviewRun if summary.truncated { spans.push(" · truncated".dim()); } + if let Some(record) = summary.finding_disposition.as_ref() { + spans.push(" · ".dim()); + spans.push(Span::from(disposition_label(record.disposition)).yellow()); + } + if let Some(reason) = summary.terminal_reason { + spans.push(" · ".dim()); + spans.push(Span::from(format!("stopped: {}", terminal_reason_label(reason))).red()); + } + if let Some(budget) = summary.budget.as_ref() { + spans.push(" · ".dim()); + spans.push( + Span::from(format!( + "elapsed {}/{} · tokens {}/{} · scope {}/{} · output {}/{} · findings {}/{}", + display_optional_duration(summary.usage.elapsed_ms), + display_duration(budget.max_elapsed_ms), + display_optional_count(summary.usage.total_tokens), + display_count(budget.max_total_tokens), + display_optional_bytes(summary.usage.scope_bytes), + display_bytes(budget.max_scope_bytes), + display_optional_bytes(summary.usage.output_bytes), + display_bytes(budget.max_output_bytes), + summary + .usage + .finding_count + .map(|count| count.to_string()) + .unwrap_or_else(|| "?".to_string()), + budget.max_findings, + )) + .dim(), + ); + } if let Some(error_summary) = summary .error_summary .as_deref() @@ -229,6 +262,71 @@ fn finding_count_label(count: usize) -> String { } } +fn disposition_label(disposition: AutoReviewFindingDisposition) -> &'static str { + match disposition { + AutoReviewFindingDisposition::NeedsAttention => "needs attention", + AutoReviewFindingDisposition::Repairing => "repairing", + AutoReviewFindingDisposition::Deferred => "deferred", + AutoReviewFindingDisposition::Obsolete => "obsolete", + } +} + +fn terminal_reason_label(reason: AutoReviewTerminalReason) -> &'static str { + match reason { + AutoReviewTerminalReason::BudgetScope => "scope budget", + AutoReviewTerminalReason::BudgetElapsed => "elapsed budget", + AutoReviewTerminalReason::BudgetTotalTokens => "token budget", + AutoReviewTerminalReason::BudgetOutput => "output budget", + AutoReviewTerminalReason::BudgetFindingCount => "finding budget", + AutoReviewTerminalReason::EmptyOutput => "empty output", + AutoReviewTerminalReason::StaleTarget => "stale target", + } +} + +fn display_optional_duration(milliseconds: Option) -> String { + milliseconds + .map(display_duration) + .unwrap_or_else(|| "?".to_string()) +} + +fn display_duration(milliseconds: u64) -> String { + if milliseconds >= 60_000 { + format!("{}m", milliseconds / 60_000) + } else if milliseconds >= 1_000 { + format!("{}s", milliseconds / 1_000) + } else { + format!("{milliseconds}ms") + } +} + +fn display_optional_count(count: Option) -> String { + count.map(display_count).unwrap_or_else(|| "?".to_string()) +} + +fn display_count(count: u64) -> String { + if count >= 1_000_000 { + format!("{}m", count / 1_000_000) + } else if count >= 1_000 { + format!("{}k", count / 1_000) + } else { + count.to_string() + } +} + +fn display_optional_bytes(bytes: Option) -> String { + bytes.map(display_bytes).unwrap_or_else(|| "?".to_string()) +} + +fn display_bytes(bytes: usize) -> String { + if bytes >= 1024 * 1024 { + format!("{}MiB", bytes / (1024 * 1024)) + } else if bytes >= 1024 { + format!("{}KiB", bytes / 1024) + } else { + format!("{bytes}B") + } +} + fn freshness_label(freshness: AutoReviewFreshness) -> &'static str { match freshness { AutoReviewFreshness::Current => "current", diff --git a/codex-rs/tui/src/history_cell/tests.rs b/codex-rs/tui/src/history_cell/tests.rs index 59d440dcc3f6..c5e372a733fb 100644 --- a/codex-rs/tui/src/history_cell/tests.rs +++ b/codex-rs/tui/src/history_cell/tests.rs @@ -9,12 +9,18 @@ use crate::legacy_core::config::ConfigBuilder; use crate::session_state::ThreadSessionState; use crate::wrapping::word_wrap_lines; use codex_app_server_protocol::AskForApproval; +use codex_app_server_protocol::AutoReviewBudget; use codex_app_server_protocol::AutoReviewDiagnosticsSummary; +use codex_app_server_protocol::AutoReviewDispositionActor; +use codex_app_server_protocol::AutoReviewFindingDisposition; +use codex_app_server_protocol::AutoReviewFindingDispositionRecord; use codex_app_server_protocol::AutoReviewFreshness; use codex_app_server_protocol::AutoReviewRunSource; use codex_app_server_protocol::AutoReviewRunSummary; use codex_app_server_protocol::AutoReviewStatusCount; use codex_app_server_protocol::AutoReviewSummaryReadResponse; +use codex_app_server_protocol::AutoReviewTerminalReason; +use codex_app_server_protocol::AutoReviewUsage; use codex_app_server_protocol::BackgroundAutoReviewStatus; use codex_app_server_protocol::BackgroundAutoReviewStatusChangedNotification; use codex_app_server_protocol::McpAuthStatus; @@ -246,6 +252,10 @@ fn auto_review_summary( omitted_findings: 0, truncated: false, content: content.to_string(), + budget: None, + usage: Default::default(), + terminal_reason: None, + finding_disposition: None, } } @@ -290,6 +300,74 @@ fn auto_review_summary_clean_snapshot() { "); } +#[test] +fn auto_review_summary_surfaces_budget_usage_and_terminal_reason() { + let mut summary = auto_review_summary( + "run-budget", + AutoReviewFreshness::Current, + /*rendered_findings*/ 0, + "", + ); + summary.status = BackgroundAutoReviewStatus::Cancelled; + summary.error_summary = Some("background review exceeded token budget".to_string()); + summary.budget = Some(AutoReviewBudget { + max_scope_bytes: 120 * 1024, + max_elapsed_ms: 5 * 60 * 1_000, + max_total_tokens: 250_000, + max_output_bytes: 64 * 1024, + max_findings: 20, + }); + summary.usage = AutoReviewUsage { + scope_bytes: Some(32 * 1024), + elapsed_ms: Some(4 * 60 * 1_000), + total_tokens: Some(250_000), + output_bytes: None, + finding_count: None, + }; + summary.terminal_reason = Some(AutoReviewTerminalReason::BudgetTotalTokens); + let response = AutoReviewSummaryReadResponse { + latest: Some(summary), + current: None, + status_counts: Vec::new(), + diagnostics: None, + }; + + let rendered = + render_lines(&new_auto_review_summary_cell(&response).display_lines(160)).join("\n"); + + assert!(rendered.contains("stopped: token budget")); + assert!(rendered.contains("elapsed 4m/5m")); + assert!(rendered.contains("tokens 250k/250k")); + assert!(rendered.contains("scope 32KiB/120KiB")); +} + +#[test] +fn auto_review_summary_surfaces_finding_disposition() { + let mut summary = auto_review_summary( + "run-attention", + AutoReviewFreshness::Current, + /*rendered_findings*/ 1, + "[P1] f1: Fix the regression", + ); + summary.finding_disposition = Some(AutoReviewFindingDispositionRecord { + disposition: AutoReviewFindingDisposition::NeedsAttention, + actor: AutoReviewDispositionActor::System, + reason: None, + updated_at: 1_700_000_001_000, + }); + let response = AutoReviewSummaryReadResponse { + latest: Some(summary.clone()), + current: Some(summary), + status_counts: Vec::new(), + diagnostics: None, + }; + + let rendered = + render_lines(&new_auto_review_summary_cell(&response).display_lines(120)).join("\n"); + + assert!(rendered.contains("needs attention")); +} + #[test] fn auto_review_summary_findings_snapshot() { let mut summary = auto_review_summary( From 0e3eb23201b06767c27dbc299e41206ec935c811 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Sat, 18 Jul 2026 15:48:32 -0400 Subject: [PATCH 3/3] fix(tui): handle narrow diff rendering --- codex-rs/tui/src/diff_render.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 350c3c7aa059..89acd559adb3 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -457,7 +457,12 @@ fn render_changes_block(rows: Vec, wrap_cols: usize, cwd: &Path) -> Vec>, width: u16, height: u16) { let mut terminal = Terminal::new(TestBackend::new(width, height)).expect("terminal"); terminal