From 99016ec732e4c231ddb5116e159a634a1436d88c Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Thu, 7 May 2026 20:31:41 -0700 Subject: [PATCH] [codex-analytics] plumb protocol-native review timing (#21434) ## Why We want terminal tool review analytics, but the reducer should not stamp review timing from its own wall clock. This PR plumbs review timing through the real protocol and app-server seams so downstream analytics can consume the emitter's timestamps directly. Guardian reviews keep their enriched `started_at` / `completed_at` analytics fields by deriving those legacy second-based values from the same protocol-native millisecond lifecycle timestamps, rather than sampling a separate analytics clock. ## What changed - add `started_at_ms` to user approval request payloads - add `started_at_ms` / `completed_at_ms` to guardian review notifications - preserve Guardian review `started_at` / `completed_at` enrichment from the protocol-native timing source - stamp typed `ServerResponse` analytics facts with app-server-observed `completed_at_ms` - thread the new timing fields through core, protocol, app-server, TUI, and analytics fixtures ## Verification - `cargo test -p codex-app-server outgoing_message --manifest-path codex-rs/Cargo.toml` - `cargo test -p codex-app-server-protocol guardian --manifest-path codex-rs/Cargo.toml` - `cargo test -p codex-tui guardian --manifest-path codex-rs/Cargo.toml` - `cargo test -p codex-analytics analytics_client_tests --manifest-path codex-rs/Cargo.toml` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/21434). * #18748 * __->__ #21434 * #18747 * #17090 * #17089 * #20514 --- codex-rs/analytics/src/client.rs | 3 +- codex-rs/analytics/src/events.rs | 7 ++- codex-rs/analytics/src/facts.rs | 1 + codex-rs/analytics/src/reducer.rs | 1 + ...CommandExecutionRequestApprovalParams.json | 6 ++ .../json/FileChangeRequestApprovalParams.json | 6 ++ .../PermissionsRequestApprovalParams.json | 6 ++ .../schema/json/ServerNotification.json | 18 ++++++ .../schema/json/ServerRequest.json | 18 ++++++ .../codex_app_server_protocol.schemas.json | 36 +++++++++++ .../codex_app_server_protocol.v2.schemas.json | 18 ++++++ ...anApprovalReviewCompletedNotification.json | 12 ++++ ...dianApprovalReviewStartedNotification.json | 6 ++ .../CommandExecutionRequestApprovalParams.ts | 3 + .../v2/FileChangeRequestApprovalParams.ts | 4 ++ ...dianApprovalReviewCompletedNotification.ts | 8 +++ ...ardianApprovalReviewStartedNotification.ts | 4 ++ .../v2/PermissionsRequestApprovalParams.ts | 6 +- .../src/protocol/common.rs | 1 + .../src/protocol/item_builders.rs | 5 ++ .../src/protocol/thread_history.rs | 7 +++ .../src/protocol/v2/item.rs | 15 +++++ .../src/protocol/v2/permissions.rs | 3 + .../src/protocol/v2/tests.rs | 3 + codex-rs/app-server-test-client/src/lib.rs | 2 + .../app-server/src/bespoke_event_handling.rs | 16 +++++ codex-rs/app-server/src/outgoing_message.rs | 17 ++++- codex-rs/app-server/src/transport_tests.rs | 2 + codex-rs/core/src/codex_delegate_tests.rs | 2 + codex-rs/core/src/guardian/review.rs | 12 ++++ codex-rs/core/src/session/mod.rs | 3 + codex-rs/mcp-server/src/codex_tool_runner.rs | 2 + codex-rs/protocol/src/approvals.rs | 10 +++ codex-rs/protocol/src/request_permissions.rs | 2 + codex-rs/rollout/src/recorder_tests.rs | 62 +++++++++++++++++++ codex-rs/tui/src/app/app_server_requests.rs | 4 ++ .../tui/src/app/pending_interactive_replay.rs | 2 + codex-rs/tui/src/app/tests.rs | 3 + codex-rs/tui/src/app/thread_events.rs | 1 + codex-rs/tui/src/auto_review_denials.rs | 2 + codex-rs/tui/src/chatwidget.rs | 19 +++++- .../src/chatwidget/tests/approval_requests.rs | 3 + .../tui/src/chatwidget/tests/exec_flow.rs | 1 + codex-rs/tui/src/chatwidget/tests/guardian.rs | 31 ++++++++++ 44 files changed, 384 insertions(+), 9 deletions(-) diff --git a/codex-rs/analytics/src/client.rs b/codex-rs/analytics/src/client.rs index 6d6d44656..6d46b2ce5 100644 --- a/codex-rs/analytics/src/client.rs +++ b/codex-rs/analytics/src/client.rs @@ -340,8 +340,9 @@ impl AnalyticsEventsClient { }); } - pub fn track_server_response(&self, response: ServerResponse) { + pub fn track_server_response(&self, completed_at_ms: u64, response: ServerResponse) { self.record_fact(AnalyticsFact::ServerResponse { + completed_at_ms, response: Box::new(response), }); } diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 23afd83b7..eaa7daf8f 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -18,6 +18,7 @@ use crate::facts::TurnStatus; use crate::facts::TurnSteerRejectionReason; use crate::facts::TurnSteerResult; use crate::facts::TurnSubmissionType; +use crate::now_unix_millis; use crate::now_unix_seconds; use codex_app_server_protocol::CodexErrorInfo; use codex_app_server_protocol::CommandExecutionSource; @@ -261,7 +262,7 @@ pub struct GuardianReviewTrackContext { approval_request_source: GuardianApprovalRequestSource, reviewed_action: GuardianReviewedAction, review_timeout_ms: u64, - started_at: u64, + pub started_at_ms: u64, started_instant: Instant, } @@ -283,7 +284,7 @@ impl GuardianReviewTrackContext { approval_request_source, reviewed_action, review_timeout_ms, - started_at: now_unix_seconds(), + started_at_ms: now_unix_millis(), started_instant: Instant::now(), } } @@ -316,7 +317,7 @@ impl GuardianReviewTrackContext { tool_call_count: None, time_to_first_token_ms: result.time_to_first_token_ms, completion_latency_ms: Some(self.started_instant.elapsed().as_millis() as u64), - started_at: self.started_at, + started_at: self.started_at_ms / 1_000, completed_at: Some(now_unix_seconds()), input_tokens: result.token_usage.as_ref().map(|usage| usage.input_tokens), cached_input_tokens: result diff --git a/codex-rs/analytics/src/facts.rs b/codex-rs/analytics/src/facts.rs index 861e6534a..d0446e8c0 100644 --- a/codex-rs/analytics/src/facts.rs +++ b/codex-rs/analytics/src/facts.rs @@ -296,6 +296,7 @@ pub(crate) enum AnalyticsFact { request: Box, }, ServerResponse { + completed_at_ms: u64, response: Box, }, Notification(Box), diff --git a/codex-rs/analytics/src/reducer.rs b/codex-rs/analytics/src/reducer.rs index d35fb9660..2ddb59c0c 100644 --- a/codex-rs/analytics/src/reducer.rs +++ b/codex-rs/analytics/src/reducer.rs @@ -325,6 +325,7 @@ impl AnalyticsReducer { } => {} AnalyticsFact::ServerResponse { response: _response, + .. } => {} AnalyticsFact::Custom(input) => match input { CustomAnalyticsFact::SubAgentThreadStarted(input) => { diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index ce587a7f1..5b6c4cd18 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -593,6 +593,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -602,6 +607,7 @@ }, "required": [ "itemId", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/json/FileChangeRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/FileChangeRequestApprovalParams.json index f52e98cd0..f17388aa5 100644 --- a/codex-rs/app-server-protocol/schema/json/FileChangeRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/FileChangeRequestApprovalParams.json @@ -18,6 +18,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -27,6 +32,7 @@ }, "required": [ "itemId", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json index adb50dee4..1383da612 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json @@ -297,6 +297,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -308,6 +313,7 @@ "cwd", "itemId", "permissions", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 5dc3c09a4..4e9e63d30 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1963,6 +1963,11 @@ "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" }, + "completedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review completed.", + "format": "int64", + "type": "integer" + }, "decisionSource": { "$ref": "#/definitions/AutoReviewDecisionSource" }, @@ -1973,6 +1978,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -1989,9 +1999,11 @@ }, "required": [ "action", + "completedAtMs", "decisionSource", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], @@ -2010,6 +2022,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -2028,6 +2045,7 @@ "action", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 51cab5081..9844eac0b 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -417,6 +417,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -426,6 +431,7 @@ }, "required": [ "itemId", + "startedAtMs", "threadId", "turnId" ], @@ -598,6 +604,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -607,6 +618,7 @@ }, "required": [ "itemId", + "startedAtMs", "threadId", "turnId" ], @@ -1587,6 +1599,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -1598,6 +1615,7 @@ "cwd", "itemId", "permissions", + "startedAtMs", "threadId", "turnId" ], 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 5e2484572..d11d49984 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 @@ -2146,6 +2146,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -2155,6 +2160,7 @@ }, "required": [ "itemId", + "startedAtMs", "threadId", "turnId" ], @@ -2411,6 +2417,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -2420,6 +2431,7 @@ }, "required": [ "itemId", + "startedAtMs", "threadId", "turnId" ], @@ -3591,6 +3603,11 @@ "null" ] }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this approval request started.", + "format": "int64", + "type": "integer" + }, "threadId": { "type": "string" }, @@ -3602,6 +3619,7 @@ "cwd", "itemId", "permissions", + "startedAtMs", "threadId", "turnId" ], @@ -9878,6 +9896,11 @@ "action": { "$ref": "#/definitions/v2/GuardianApprovalReviewAction" }, + "completedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review completed.", + "format": "int64", + "type": "integer" + }, "decisionSource": { "$ref": "#/definitions/v2/AutoReviewDecisionSource" }, @@ -9888,6 +9911,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -9904,9 +9932,11 @@ }, "required": [ "action", + "completedAtMs", "decisionSource", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], @@ -9927,6 +9957,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -9945,6 +9980,7 @@ "action", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], 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 6153a54eb..41168a073 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 @@ -6489,6 +6489,11 @@ "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" }, + "completedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review completed.", + "format": "int64", + "type": "integer" + }, "decisionSource": { "$ref": "#/definitions/AutoReviewDecisionSource" }, @@ -6499,6 +6504,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -6515,9 +6525,11 @@ }, "required": [ "action", + "completedAtMs", "decisionSource", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], @@ -6538,6 +6550,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -6556,6 +6573,7 @@ "action", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index 98f44e50a..991d4de05 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -574,6 +574,11 @@ "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" }, + "completedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review completed.", + "format": "int64", + "type": "integer" + }, "decisionSource": { "$ref": "#/definitions/AutoReviewDecisionSource" }, @@ -584,6 +589,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -600,9 +610,11 @@ }, "required": [ "action", + "completedAtMs", "decisionSource", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json index 16e47c2d7..75ffeb753 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -574,6 +574,11 @@ "description": "Stable identifier for this review.", "type": "string" }, + "startedAtMs": { + "description": "Unix timestamp (in milliseconds) when this review started.", + "format": "int64", + "type": "integer" + }, "targetItemId": { "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", "type": [ @@ -592,6 +597,7 @@ "action", "review", "reviewId", + "startedAtMs", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts index ca2d0b0aa..0e9100836 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts @@ -8,6 +8,9 @@ import type { NetworkApprovalContext } from "./NetworkApprovalContext"; import type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; export type CommandExecutionRequestApprovalParams = {threadId: string, turnId: string, itemId: string, /** + * Unix timestamp (in milliseconds) when this approval request started. + */ +startedAtMs: number, /** * Unique identifier for this specific approval callback. * * For regular shell/unified_exec approvals, this is null. diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FileChangeRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FileChangeRequestApprovalParams.ts index c514ed621..2db7be9ec 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FileChangeRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FileChangeRequestApprovalParams.ts @@ -3,6 +3,10 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. export type FileChangeRequestApprovalParams = { threadId: string, turnId: string, itemId: string, +/** + * Unix timestamp (in milliseconds) when this approval request started. + */ +startedAtMs: number, /** * Optional explanatory reason (e.g. request for extra write access). */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts index 5b162cf4b..32d12be60 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts @@ -10,6 +10,14 @@ import type { GuardianApprovalReviewAction } from "./GuardianApprovalReviewActio * shape is expected to change soon. */ export type ItemGuardianApprovalReviewCompletedNotification = { threadId: string, turnId: string, +/** + * Unix timestamp (in milliseconds) when this review started. + */ +startedAtMs: number, +/** + * Unix timestamp (in milliseconds) when this review completed. + */ +completedAtMs: number, /** * Stable identifier for this review. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts index 81ba2cdeb..92d34fdeb 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts @@ -9,6 +9,10 @@ import type { GuardianApprovalReviewAction } from "./GuardianApprovalReviewActio * shape is expected to change soon. */ export type ItemGuardianApprovalReviewStartedNotification = { threadId: string, turnId: string, +/** + * Unix timestamp (in milliseconds) when this review started. + */ +startedAtMs: number, /** * Stable identifier for this review. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalParams.ts index 308670a80..509f60923 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalParams.ts @@ -4,4 +4,8 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; import type { RequestPermissionProfile } from "./RequestPermissionProfile"; -export type PermissionsRequestApprovalParams = { threadId: string, turnId: string, itemId: string, cwd: AbsolutePathBuf, reason: string | null, permissions: RequestPermissionProfile, }; +export type PermissionsRequestApprovalParams = { threadId: string, turnId: string, itemId: string, +/** + * Unix timestamp (in milliseconds) when this approval request started. + */ +startedAtMs: number, cwd: AbsolutePathBuf, reason: string | null, permissions: RequestPermissionProfile, }; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index e72e3c14c..bf0b3c87b 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -2967,6 +2967,7 @@ mod tests { thread_id: "thr_123".to_string(), turn_id: "turn_123".to_string(), item_id: "call_123".to_string(), + started_at_ms: 0, approval_id: None, reason: None, network_approval_context: None, diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders.rs b/codex-rs/app-server-protocol/src/protocol/item_builders.rs index 69ba331ce..17e0f9aef 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders.rs @@ -243,6 +243,7 @@ pub fn guardian_auto_approval_review_notification( thread_id: conversation_id.to_string(), turn_id, review_id: assessment.id.clone(), + started_at_ms: assessment.started_at_ms, target_item_id: assessment.target_item_id.clone(), review, action, @@ -258,6 +259,10 @@ pub fn guardian_auto_approval_review_notification( thread_id: conversation_id.to_string(), turn_id, review_id: assessment.id.clone(), + started_at_ms: assessment.started_at_ms, + completed_at_ms: assessment + .completed_at_ms + .unwrap_or(assessment.started_at_ms), target_item_id: assessment.target_item_id.clone(), decision_source: assessment .decision_source diff --git a/codex-rs/app-server-protocol/src/protocol/thread_history.rs b/codex-rs/app-server-protocol/src/protocol/thread_history.rs index 1f45180c9..1121d3a35 100644 --- a/codex-rs/app-server-protocol/src/protocol/thread_history.rs +++ b/codex-rs/app-server-protocol/src/protocol/thread_history.rs @@ -2143,6 +2143,8 @@ mod tests { id: "review-guardian-exec".into(), target_item_id: Some("guardian-exec".into()), turn_id: "turn-1".into(), + started_at_ms: 1_000, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -2160,6 +2162,8 @@ mod tests { id: "review-guardian-exec".into(), target_item_id: Some("guardian-exec".into()), turn_id: "turn-1".into(), + started_at_ms: 1_000, + completed_at_ms: Some(1_042), status: GuardianAssessmentStatus::Denied, risk_level: Some(codex_protocol::protocol::GuardianRiskLevel::High), user_authorization: Some(codex_protocol::protocol::GuardianUserAuthorization::Low), @@ -2222,6 +2226,8 @@ mod tests { id: "review-guardian-execve".into(), target_item_id: Some("guardian-execve".into()), turn_id: "turn-1".into(), + started_at_ms: 2_000, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -2525,6 +2531,7 @@ mod tests { EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "patch-call".into(), turn_id: turn_id.to_string(), + started_at_ms: 0, changes: [( PathBuf::from("README.md"), codex_protocol::protocol::FileChange::Add { diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 2c3a926c9..0e22c4859 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -1073,6 +1073,9 @@ pub struct ItemStartedNotification { pub struct ItemGuardianApprovalReviewStartedNotification { pub thread_id: String, pub turn_id: String, + /// Unix timestamp (in milliseconds) when this review started. + #[ts(type = "number")] + pub started_at_ms: i64, /// Stable identifier for this review. pub review_id: String, /// Identifier for the reviewed item or tool call when one exists. @@ -1099,6 +1102,12 @@ pub struct ItemGuardianApprovalReviewStartedNotification { pub struct ItemGuardianApprovalReviewCompletedNotification { pub thread_id: String, pub turn_id: String, + /// Unix timestamp (in milliseconds) when this review started. + #[ts(type = "number")] + pub started_at_ms: i64, + /// Unix timestamp (in milliseconds) when this review completed. + #[ts(type = "number")] + pub completed_at_ms: i64, /// Stable identifier for this review. pub review_id: String, /// Identifier for the reviewed item or tool call when one exists. @@ -1248,6 +1257,9 @@ pub struct CommandExecutionRequestApprovalParams { pub thread_id: String, pub turn_id: String, pub item_id: String, + /// Unix timestamp (in milliseconds) when this approval request started. + #[ts(type = "number")] + pub started_at_ms: i64, /// Unique identifier for this specific approval callback. /// /// For regular shell/unified_exec approvals, this is null. @@ -1321,6 +1333,9 @@ pub struct FileChangeRequestApprovalParams { pub thread_id: String, pub turn_id: String, pub item_id: String, + /// Unix timestamp (in milliseconds) when this approval request started. + #[ts(type = "number")] + pub started_at_ms: i64, /// Optional explanatory reason (e.g. request for extra write access). #[ts(optional = nullable)] pub reason: Option, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs index 8ce47e58c..86614a6ae 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs @@ -826,6 +826,9 @@ pub struct PermissionsRequestApprovalParams { pub thread_id: String, pub turn_id: String, pub item_id: String, + /// Unix timestamp (in milliseconds) when this approval request started. + #[ts(type = "number")] + pub started_at_ms: i64, pub cwd: AbsolutePathBuf, pub reason: Option, pub permissions: RequestPermissionProfile, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index d6e49279c..47925b16c 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -277,6 +277,7 @@ fn command_execution_request_approval_rejects_relative_additional_permission_pat "threadId": "thr_123", "turnId": "turn_123", "itemId": "call_123", + "startedAtMs": 1, "command": "cat file", "cwd": absolute_path_string("tmp"), "commandActions": null, @@ -317,6 +318,7 @@ fn permissions_request_approval_uses_request_permission_profile() { "threadId": "thr_123", "turnId": "turn_123", "itemId": "call_123", + "startedAtMs": 1, "cwd": absolute_path_string("repo"), "reason": "Select a workspace root", "permissions": { @@ -379,6 +381,7 @@ fn permissions_request_approval_rejects_macos_permissions() { "threadId": "thr_123", "turnId": "turn_123", "itemId": "call_123", + "startedAtMs": 1, "cwd": absolute_path_string("repo"), "reason": "Select a workspace root", "permissions": { diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index edea431c6..e67f6e02f 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -1945,6 +1945,7 @@ impl CodexClient { thread_id, turn_id, item_id, + started_at_ms: _, approval_id, reason, network_approval_context, @@ -2020,6 +2021,7 @@ impl CodexClient { thread_id, turn_id, item_id, + started_at_ms: _, reason, grant_root, } = params; diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 64215566e..1f2f289b0 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -511,6 +511,7 @@ pub(crate) async fn apply_bespoke_event_handling( thread_id: conversation_id.to_string(), turn_id: event.turn_id.clone(), item_id: item_id.clone(), + started_at_ms: event.started_at_ms, reason: event.reason.clone(), grant_root: event.grant_root.clone(), }; @@ -542,6 +543,7 @@ pub(crate) async fn apply_bespoke_event_handling( call_id, approval_id, turn_id, + started_at_ms, command, cwd, reason, @@ -615,6 +617,7 @@ pub(crate) async fn apply_bespoke_event_handling( thread_id: conversation_id.to_string(), turn_id: turn_id.clone(), item_id: call_id.clone(), + started_at_ms, approval_id: approval_id.clone(), reason, network_approval_context, @@ -764,6 +767,7 @@ pub(crate) async fn apply_bespoke_event_handling( thread_id: conversation_id.to_string(), turn_id: request.turn_id.clone(), item_id: request.call_id.clone(), + started_at_ms: request.started_at_ms, cwd: request_cwd.clone(), reason: request.reason, permissions: request.permissions.into(), @@ -2249,6 +2253,9 @@ mod tests { id: format!("review-{id}"), target_item_id: Some(id.to_string()), turn_id: turn_id.to_string(), + started_at_ms: 1_000, + completed_at_ms: (!matches!(status, GuardianAssessmentStatus::InProgress)) + .then_some(1_042), status, risk_level, user_authorization, @@ -2313,6 +2320,8 @@ mod tests { id: "review-1".to_string(), target_item_id: Some("item-1".to_string()), turn_id: String::new(), + started_at_ms: 1_000, + completed_at_ms: None, status: codex_protocol::protocol::GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -2326,6 +2335,7 @@ mod tests { ServerNotification::ItemGuardianApprovalReviewStarted(payload) => { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-event"); + assert_eq!(payload.started_at_ms, 1_000); assert_eq!(payload.review_id, "review-1"); assert_eq!(payload.target_item_id.as_deref(), Some("item-1")); assert_eq!( @@ -2356,6 +2366,8 @@ mod tests { id: "review-2".to_string(), target_item_id: Some("item-2".to_string()), turn_id: "turn-from-assessment".to_string(), + started_at_ms: 1_000, + completed_at_ms: Some(1_042), status: codex_protocol::protocol::GuardianAssessmentStatus::Denied, risk_level: Some(codex_protocol::protocol::GuardianRiskLevel::High), user_authorization: Some(codex_protocol::protocol::GuardianUserAuthorization::Low), @@ -2371,6 +2383,8 @@ mod tests { ServerNotification::ItemGuardianApprovalReviewCompleted(payload) => { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-assessment"); + assert_eq!(payload.started_at_ms, 1_000); + assert_eq!(payload.completed_at_ms, 1_042); assert_eq!(payload.review_id, "review-2"); assert_eq!(payload.target_item_id.as_deref(), Some("item-2")); assert_eq!(payload.decision_source, AutoReviewDecisionSource::Agent); @@ -2406,6 +2420,8 @@ mod tests { id: "review-3".to_string(), target_item_id: None, turn_id: "turn-from-assessment".to_string(), + started_at_ms: 1_000, + completed_at_ms: Some(1_042), status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, diff --git a/codex-rs/app-server/src/outgoing_message.rs b/codex-rs/app-server/src/outgoing_message.rs index a7420f8c7..cbe196cd9 100644 --- a/codex-rs/app-server/src/outgoing_message.rs +++ b/codex-rs/app-server/src/outgoing_message.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::atomic::AtomicI64; use std::sync::atomic::Ordering; +use std::time::SystemTime; +use std::time::UNIX_EPOCH; use codex_analytics::AnalyticsEventsClient; use codex_app_server_protocol::ClientResponsePayload; @@ -357,8 +359,10 @@ impl OutgoingMessageSender { match entry { Some((id, entry)) => { + let completed_at_ms = now_unix_timestamp_ms(); if let Ok(response) = entry.request.response_from_result(result.clone()) { - self.analytics_events_client.track_server_response(response); + self.analytics_events_client + .track_server_response(completed_at_ms, response); } if let Err(err) = entry.callback.send(Ok(result)) { warn!("could not notify callback for {id:?} due to: {err:?}"); @@ -648,6 +652,15 @@ impl OutgoingMessageSender { } } +fn now_unix_timestamp_ms() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_millis() + .try_into() + .unwrap_or_default() +} + #[cfg(test)] mod tests { use std::time::Duration; @@ -903,6 +916,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "item-1".to_string(), + started_at_ms: 0, approval_id: None, reason: None, network_approval_context: None, @@ -1195,6 +1209,7 @@ mod tests { thread_id: thread_id.to_string(), turn_id: "turn-1".to_string(), item_id: "call-2".to_string(), + started_at_ms: 0, reason: None, grant_root: None, }, diff --git a/codex-rs/app-server/src/transport_tests.rs b/codex-rs/app-server/src/transport_tests.rs index 1600b8be8..5790e46a1 100644 --- a/codex-rs/app-server/src/transport_tests.rs +++ b/codex-rs/app-server/src/transport_tests.rs @@ -258,6 +258,7 @@ async fn command_execution_request_approval_strips_additional_permissions_withou thread_id: "thr_123".to_string(), turn_id: "turn_123".to_string(), item_id: "call_123".to_string(), + started_at_ms: 0, approval_id: None, reason: Some("Need extra read access".to_string()), network_approval_context: None, @@ -322,6 +323,7 @@ async fn command_execution_request_approval_keeps_additional_permissions_with_ca thread_id: "thr_123".to_string(), turn_id: "turn_123".to_string(), item_id: "call_123".to_string(), + started_at_ms: 0, approval_id: None, reason: Some("Need extra read access".to_string()), network_approval_context: None, diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 84224ea2d..ecd392e3e 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -225,6 +225,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() { RequestPermissionsEvent { call_id: request_call_id, turn_id: "child-turn-1".to_string(), + started_at_ms: 0, reason: Some("need access".to_string()), permissions: RequestPermissionProfile { network: Some(NetworkPermissions { @@ -313,6 +314,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f call_id: "command-item-1".to_string(), approval_id: Some("callback-approval-1".to_string()), turn_id: "child-turn-1".to_string(), + started_at_ms: 0, command: vec!["rm".to_string(), "-rf".to_string(), "tmp".to_string()], cwd: test_path_buf("/tmp").abs(), reason: Some("unsafe subcommand".to_string()), diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 850d84dd2..bba2167ce 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -23,6 +23,7 @@ use tokio_util::sync::CancellationToken; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::turn_timing::now_unix_timestamp_ms; use super::GUARDIAN_REVIEW_TIMEOUT; use super::GUARDIAN_REVIEWER_NAME; @@ -251,6 +252,7 @@ async fn run_guardian_review( guardian_reviewed_action(&request), GUARDIAN_REVIEW_TIMEOUT.as_millis() as u64, ); + let started_at_ms = review_tracking.started_at_ms.try_into().unwrap_or_default(); session .send_event( turn.as_ref(), @@ -258,6 +260,8 @@ async fn run_guardian_review( id: review_id.clone(), target_item_id: target_item_id.clone(), turn_id: assessment_turn_id.clone(), + started_at_ms, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -289,6 +293,8 @@ async fn run_guardian_review( id: review_id, target_item_id, turn_id: assessment_turn_id.clone(), + started_at_ms, + completed_at_ms: Some(now_unix_timestamp_ms()), status: GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, @@ -372,6 +378,8 @@ async fn run_guardian_review( id: review_id, target_item_id, turn_id: assessment_turn_id.clone(), + started_at_ms, + completed_at_ms: Some(now_unix_timestamp_ms()), status: GuardianAssessmentStatus::TimedOut, risk_level: None, user_authorization: None, @@ -402,6 +410,8 @@ async fn run_guardian_review( id: review_id, target_item_id, turn_id: assessment_turn_id.clone(), + started_at_ms, + completed_at_ms: Some(now_unix_timestamp_ms()), status: GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, @@ -495,6 +505,8 @@ async fn run_guardian_review( id: review_id, target_item_id, turn_id: assessment_turn_id.clone(), + started_at_ms, + completed_at_ms: Some(now_unix_timestamp_ms()), status, risk_level: Some(assessment.risk_level), user_authorization: Some(assessment.user_authorization), diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 6d4b27542..89c12aaf8 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -1956,6 +1956,7 @@ impl Session { call_id, approval_id, turn_id: turn_context.sub_id.clone(), + started_at_ms: now_unix_timestamp_ms(), command, cwd, reason, @@ -2002,6 +2003,7 @@ impl Session { let event = EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id, turn_id: turn_context.sub_id.clone(), + started_at_ms: now_unix_timestamp_ms(), changes, reason, grant_root, @@ -2165,6 +2167,7 @@ impl Session { let event = EventMsg::RequestPermissions(RequestPermissionsEvent { call_id: call_id.clone(), turn_id: turn_context.sub_id.clone(), + started_at_ms: now_unix_timestamp_ms(), reason: args.reason, permissions: requested_permissions, cwd: Some(cwd), diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index b462022db..4e2d5c08e 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -222,6 +222,7 @@ async fn run_codex_tool_session_inner( let approval_id = ev.effective_approval_id(); let ExecApprovalRequestEvent { turn_id: _, + started_at_ms: _, command, cwd, call_id, @@ -278,6 +279,7 @@ async fn run_codex_tool_session_inner( EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id, turn_id: _, + started_at_ms: _, reason, grant_root, changes, diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 73283e3eb..ace096359 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -187,6 +187,12 @@ pub struct GuardianAssessmentEvent { /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] pub turn_id: String, + #[serde(default)] + #[ts(type = "number")] + pub started_at_ms: i64, + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional, type = "number")] + pub completed_at_ms: Option, pub status: GuardianAssessmentStatus, /// Coarse risk label. Omitted while the assessment is in progress. #[serde(default, skip_serializing_if = "Option::is_none")] @@ -223,6 +229,8 @@ pub struct ExecApprovalRequestEvent { /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] pub turn_id: String, + #[ts(type = "number")] + pub started_at_ms: i64, /// The command to be executed. pub command: Vec, /// The command's working directory. @@ -370,6 +378,8 @@ pub struct ApplyPatchApprovalRequestEvent { /// Uses `#[serde(default)]` for backwards compatibility with older senders. #[serde(default)] pub turn_id: String, + #[ts(type = "number")] + pub started_at_ms: i64, pub changes: HashMap, /// Optional explanatory reason (e.g. request for extra write access). #[serde(skip_serializing_if = "Option::is_none")] diff --git a/codex-rs/protocol/src/request_permissions.rs b/codex-rs/protocol/src/request_permissions.rs index 6c7b699da..be6b88ef5 100644 --- a/codex-rs/protocol/src/request_permissions.rs +++ b/codex-rs/protocol/src/request_permissions.rs @@ -71,6 +71,8 @@ pub struct RequestPermissionsEvent { /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] pub turn_id: String, + #[ts(type = "number")] + pub started_at_ms: i64, #[serde(skip_serializing_if = "Option::is_none")] pub reason: Option, pub permissions: RequestPermissionProfile, diff --git a/codex-rs/rollout/src/recorder_tests.rs b/codex-rs/rollout/src/recorder_tests.rs index 505cd5992..bf51241f5 100644 --- a/codex-rs/rollout/src/recorder_tests.rs +++ b/codex-rs/rollout/src/recorder_tests.rs @@ -216,6 +216,68 @@ async fn load_rollout_items_skips_legacy_ghost_snapshot_lines() -> std::io::Resu Ok(()) } +#[tokio::test] +async fn load_rollout_items_preserves_legacy_guardian_assessment_lines() -> std::io::Result<()> { + let home = TempDir::new().expect("temp dir"); + let rollout_path = home.path().join("rollout.jsonl"); + let mut file = File::create(&rollout_path)?; + let thread_id = ThreadId::new(); + let ts = "2025-01-03T12:00:00Z"; + + writeln!( + file, + "{}", + serde_json::json!({ + "timestamp": ts, + "type": "session_meta", + "payload": { + "id": thread_id, + "timestamp": ts, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version", + "source": "cli", + "model_provider": "test-provider", + }, + }) + )?; + writeln!( + file, + "{}", + serde_json::json!({ + "timestamp": ts, + "type": "event_msg", + "payload": { + "type": "guardian_assessment", + "id": "guardian-1", + "turn_id": "turn-1", + "status": "in_progress", + "action": { + "type": "command", + "source": "shell", + "command": "rm -rf /tmp/guardian", + "cwd": if cfg!(windows) { r"C:\tmp" } else { "/tmp" }, + }, + }, + }) + )?; + + let (items, loaded_thread_id, parse_errors) = + RolloutRecorder::load_rollout_items(&rollout_path).await?; + + assert_eq!(loaded_thread_id, Some(thread_id)); + assert_eq!(parse_errors, 0); + assert_eq!(items.len(), 2); + let RolloutItem::EventMsg(EventMsg::GuardianAssessment(assessment)) = &items[1] else { + panic!("expected guardian assessment rollout item"); + }; + assert_eq!(assessment.id, "guardian-1"); + assert_eq!(assessment.turn_id, "turn-1"); + assert_eq!(assessment.started_at_ms, 0); + + Ok(()) +} + #[tokio::test] async fn load_rollout_items_filters_legacy_ghost_snapshots_from_compaction_history() -> std::io::Result<()> { diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 4b587b0fc..dce87f367 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -429,6 +429,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "call-1".to_string(), + started_at_ms: 0, approval_id: Some("approval-1".to_string()), reason: None, network_approval_context: None, @@ -481,6 +482,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "perm-1".to_string(), + started_at_ms: 0, cwd: absolute_path(if cfg!(windows) { r"C:\tmp" } else { "/tmp" }), reason: None, permissions: serde_json::from_value(json!({ @@ -686,6 +688,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "patch-1".to_string(), + started_at_ms: 0, reason: None, grant_root: None, }, @@ -715,6 +718,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "call-1".to_string(), + started_at_ms: 0, approval_id: Some("approval-1".to_string()), reason: None, network_approval_context: None, diff --git a/codex-rs/tui/src/app/pending_interactive_replay.rs b/codex-rs/tui/src/app/pending_interactive_replay.rs index 671e41461..1a21d4df5 100644 --- a/codex-rs/tui/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui/src/app/pending_interactive_replay.rs @@ -612,6 +612,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: turn_id.to_string(), item_id: call_id.to_string(), + started_at_ms: 0, approval_id: approval_id.map(str::to_string), reason: None, network_approval_context: None, @@ -633,6 +634,7 @@ mod tests { thread_id: "thread-1".to_string(), turn_id: turn_id.to_string(), item_id: call_id.to_string(), + started_at_ms: 0, reason: None, grant_root: None, }, diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 5ab85887a..eacb6d505 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -2659,6 +2659,7 @@ async fn inactive_thread_file_change_approval_recovers_buffered_changes() { thread_id: thread_id.to_string(), turn_id: "turn-approval".to_string(), item_id: "patch-approval".to_string(), + started_at_ms: 0, reason: Some("command failed; retry without sandbox?".to_string()), grant_root: None, }, @@ -2709,6 +2710,7 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions( thread_id: thread_id.to_string(), turn_id: "turn-approval".to_string(), item_id: "call-approval".to_string(), + started_at_ms: 0, cwd: test_absolute_path("/tmp"), reason: Some("Need access to .git".to_string()), permissions: codex_app_server_protocol::RequestPermissionProfile { @@ -4259,6 +4261,7 @@ fn exec_approval_request( thread_id: thread_id.to_string(), turn_id: turn_id.to_string(), item_id: item_id.to_string(), + started_at_ms: 0, approval_id: approval_id.map(str::to_string), reason: Some("needs approval".to_string()), network_approval_context: None, diff --git a/codex-rs/tui/src/app/thread_events.rs b/codex-rs/tui/src/app/thread_events.rs index 56477c18c..431bf5f80 100644 --- a/codex-rs/tui/src/app/thread_events.rs +++ b/codex-rs/tui/src/app/thread_events.rs @@ -465,6 +465,7 @@ mod tests { thread_id: thread_id.to_string(), turn_id: turn_id.to_string(), item_id: item_id.to_string(), + started_at_ms: 0, approval_id: approval_id.map(str::to_string), reason: Some("needs approval".to_string()), network_approval_context: None, diff --git a/codex-rs/tui/src/auto_review_denials.rs b/codex-rs/tui/src/auto_review_denials.rs index e51e071e2..149a60f04 100644 --- a/codex-rs/tui/src/auto_review_denials.rs +++ b/codex-rs/tui/src/auto_review_denials.rs @@ -88,6 +88,8 @@ mod tests { id: format!("review-{id}"), target_item_id: None, turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::Denied, risk_level: None, user_authorization: None, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index adaba76b2..35d523aa5 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1637,6 +1637,7 @@ fn request_permissions_from_params( RequestPermissionsEvent { turn_id: params.turn_id, call_id: params.item_id, + started_at_ms: params.started_at_ms, reason: params.reason, permissions: params.permissions.into(), cwd: Some(params.cwd), @@ -6374,8 +6375,9 @@ impl ChatWidget { self.on_guardian_review_notification( notification.review_id, notification.turn_id, + notification.started_at_ms, notification.review, - /*decision_source*/ None, + /*completion*/ None, notification.action, ); } @@ -6383,8 +6385,9 @@ impl ChatWidget { self.on_guardian_review_notification( notification.review_id, notification.turn_id, + notification.started_at_ms, notification.review, - Some(notification.decision_source), + Some((notification.completed_at_ms, notification.decision_source)), notification.action, ); } @@ -6567,14 +6570,24 @@ impl ChatWidget { &mut self, id: String, turn_id: String, + started_at_ms: i64, review: codex_app_server_protocol::GuardianApprovalReview, - decision_source: Option, + completion: Option<(i64, codex_app_server_protocol::AutoReviewDecisionSource)>, action: GuardianApprovalReviewAction, ) { + let (completed_at_ms, decision_source) = match completion { + Some((completed_at_ms, decision_source)) => { + (Some(completed_at_ms), Some(decision_source)) + } + None => (None, None), + }; + self.on_guardian_assessment(GuardianAssessmentEvent { id, target_item_id: None, turn_id, + started_at_ms, + completed_at_ms, status: match review.status { codex_app_server_protocol::GuardianApprovalReviewStatus::InProgress => { GuardianAssessmentStatus::InProgress diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 93e2a38c1..85c8fc6c0 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -55,6 +55,7 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "item-1".to_string(), + started_at_ms: 0, approval_id: Some("approval-1".to_string()), reason: None, network_approval_context: None, @@ -93,6 +94,7 @@ fn app_server_exec_approval_request_preserves_permissions_context() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "item-1".to_string(), + started_at_ms: 0, approval_id: Some("approval-1".to_string()), reason: None, network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { @@ -156,6 +158,7 @@ fn app_server_request_permissions_preserves_file_system_permissions() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "item-1".to_string(), + started_at_ms: 0, cwd: cwd.clone(), reason: Some("Select a workspace root".to_string()), permissions: codex_app_server_protocol::RequestPermissionProfile { diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index b1949d780..0045e7d12 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -54,6 +54,7 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), item_id: "item-1".to_string(), + started_at_ms: 0, approval_id: Some("approval-1".to_string()), reason: None, network_approval_context: None, diff --git a/codex-rs/tui/src/chatwidget/tests/guardian.rs b/codex-rs/tui/src/chatwidget/tests/guardian.rs index c51b3f668..7cdc9f760 100644 --- a/codex-rs/tui/src/chatwidget/tests/guardian.rs +++ b/codex-rs/tui/src/chatwidget/tests/guardian.rs @@ -6,6 +6,8 @@ fn auto_review_denial_event() -> GuardianAssessmentEvent { id: "auto-review-recent-1".into(), target_item_id: Some("target-auto-review-recent-1".into()), turn_id: "turn-recent-1".into(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::Denied, risk_level: Some(GuardianRiskLevel::High), user_authorization: Some(GuardianUserAuthorization::Low), @@ -73,6 +75,8 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -85,6 +89,8 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::Denied, risk_level: Some(GuardianRiskLevel::High), user_authorization: Some(GuardianUserAuthorization::Low), @@ -127,6 +133,8 @@ async fn guardian_approved_exec_renders_approved_request() { id: "thread:child-thread:guardian-1".into(), target_item_id: Some("guardian-approved-target".into()), turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::Approved, risk_level: Some(GuardianRiskLevel::Low), user_authorization: Some(GuardianUserAuthorization::High), @@ -183,6 +191,8 @@ async fn guardian_approved_request_permissions_renders_request_summary() { id: "guardian-request-permissions".into(), target_item_id: None, turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -205,6 +215,8 @@ async fn guardian_approved_request_permissions_renders_request_summary() { id: "guardian-request-permissions".into(), target_item_id: None, turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::Approved, risk_level: Some(GuardianRiskLevel::Low), user_authorization: Some(GuardianUserAuthorization::High), @@ -253,6 +265,8 @@ async fn guardian_timed_out_exec_renders_warning_and_timed_out_request() { id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -265,6 +279,8 @@ async fn guardian_timed_out_exec_renders_warning_and_timed_out_request() { id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), turn_id: "turn-1".into(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::TimedOut, risk_level: None, user_authorization: None, @@ -315,6 +331,7 @@ async fn app_server_guardian_review_started_sets_review_status() { ItemGuardianApprovalReviewStartedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), + started_at_ms: 0, review_id: "guardian-1".to_string(), target_item_id: Some("guardian-target-1".to_string()), review: GuardianApprovalReview { @@ -356,6 +373,7 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { ItemGuardianApprovalReviewStartedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), + started_at_ms: 0, review_id: "guardian-1".to_string(), target_item_id: Some("guardian-target-1".to_string()), review: GuardianApprovalReview { @@ -375,6 +393,8 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { ItemGuardianApprovalReviewCompletedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: 1, review_id: "guardian-1".to_string(), target_item_id: Some("guardian-target-1".to_string()), decision_source: AppServerGuardianApprovalReviewDecisionSource::Agent, @@ -431,6 +451,7 @@ async fn app_server_guardian_review_timed_out_renders_timed_out_request_snapshot ItemGuardianApprovalReviewStartedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), + started_at_ms: 0, review_id: "guardian-1".to_string(), target_item_id: Some("guardian-target-1".to_string()), review: GuardianApprovalReview { @@ -450,6 +471,8 @@ async fn app_server_guardian_review_timed_out_renders_timed_out_request_snapshot ItemGuardianApprovalReviewCompletedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: 1, review_id: "guardian-1".to_string(), target_item_id: Some("guardian-target-1".to_string()), decision_source: AppServerGuardianApprovalReviewDecisionSource::Agent, @@ -506,6 +529,8 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() { id: id.to_string(), target_item_id: Some(format!("{id}-target")), turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -535,6 +560,8 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "guardian-1".to_string(), target_item_id: Some("guardian-1-target".to_string()), turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -550,6 +577,8 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "guardian-2".to_string(), target_item_id: Some("guardian-2-target".to_string()), turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: None, status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, @@ -565,6 +594,8 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "guardian-1".to_string(), target_item_id: Some("guardian-1-target".to_string()), turn_id: "turn-1".to_string(), + started_at_ms: 0, + completed_at_ms: Some(1), status: GuardianAssessmentStatus::Denied, risk_level: Some(GuardianRiskLevel::High), user_authorization: Some(GuardianUserAuthorization::Low),