diff --git a/codex-rs/core/src/guardian/decision.rs b/codex-rs/core/src/guardian/decision.rs index 3e35b4ad59..ff5a25d90d 100644 --- a/codex-rs/core/src/guardian/decision.rs +++ b/codex-rs/core/src/guardian/decision.rs @@ -52,17 +52,45 @@ pub(crate) fn decide_approval( ) -> futures::future::BoxFuture<'static, Option> { let context: GuardianReviewContext = context.into(); let request: ReviewAction = request.into(); - // Invalidate before scheduling: cancellation may finalize the turn before - // the review future is first polled. Replace this boundary with attribution. - if context + // Bind the invocation before scheduling. Async review cleanup can outlive + // the decision, but must not extend the time blocking this tool. + let review_timing = if context .turn() .config .features .enabled(codex_features::Feature::CriticalPathTiming) { - context.turn().turn_timing_state.invalidate_critical_path(); - } + // Network display identity differs from its owning tool invocation. Keep + // timing correlation separate from the reviewer's existing provenance. + let timing_call_id = match &request.request { + Ok(GuardianApprovalRequest::NetworkAccess { trigger, .. }) => { + trigger.as_ref().map(|trigger| trigger.call_id.as_str()) + } + Ok( + GuardianApprovalRequest::ExecCommand { .. } + | GuardianApprovalRequest::WriteStdin { .. } + | GuardianApprovalRequest::ApplyPatch { .. } + | GuardianApprovalRequest::McpToolCall { .. } + | GuardianApprovalRequest::RequestPermissions { .. }, + ) + | Err(_) => request.tool_call_id.as_deref(), + }; + let timing = timing_call_id.and_then(|call_id| { + context + .turn() + .turn_timing_state + .direct_tool_timings + .start_review_for_turn(call_id, &context.turn().sub_id) + }); + if timing.is_none() { + context.turn().turn_timing_state.invalidate_critical_path(); + } + timing + } else { + None + }; Box::pin(async move { + let _review_timing = review_timing; let (_, history_reset) = session.history_reset().await; let cancellation = match options.external_cancel.take() { Some(external) => crate::exec::cancel_when_either(external, history_reset.clone()), diff --git a/codex-rs/core/src/guardian/decision_timing_tests.rs b/codex-rs/core/src/guardian/decision_timing_tests.rs index 712b595d8b..8c2a3ec1f4 100644 --- a/codex-rs/core/src/guardian/decision_timing_tests.rs +++ b/codex-rs/core/src/guardian/decision_timing_tests.rs @@ -1,15 +1,32 @@ #![cfg(test)] -//! Keeps uninstrumented review work out of complete critical-path captures. +//! Verifies review timing binds to its invocation before scheduling and respects the capture gate. +use codex_direct_tool_timing::ToolBatchTiming; +use codex_utils_critical_path::CriticalPath; +use codex_utils_critical_path::PathDurations; +use core_test_support::PathBufExt; +use core_test_support::test_path_buf; use pretty_assertions::assert_eq; use std::sync::Arc; +use std::time::Duration; use std::time::Instant; -#[test_case::test_case(true; "capture_enabled")] -#[test_case::test_case(false; "capture_disabled")] +#[test_case::test_case(false, false, true; "drop_before_poll")] +#[test_case::test_case(false, true, true; "cancelled_decision_returns")] +#[test_case::test_case(true, false, true; "uncorrelated_network")] +#[test_case::test_case(false, false, false; "runtime_metrics_does_not_start_review_timing")] +#[test_case::test_case(true, false, false; "runtime_metrics_does_not_invalidate_review_timing")] #[tokio::test] -async fn uncaptured_review_invalidates_before_polling(critical_path_timing: bool) { +async fn decision_binds_before_scheduling_and_does_not_follow_reused_ids( + network: bool, + await_decision: bool, + critical_path_timing: bool, +) { let (session, mut turn) = crate::session::tests::make_session_and_context().await; + Arc::make_mut(&mut turn.config) + .features + .enable(codex_features::Feature::RuntimeMetrics) + .unwrap(); Arc::make_mut(&mut turn.config) .features .set_enabled( @@ -17,29 +34,54 @@ async fn uncaptured_review_invalidates_before_polling(critical_path_timing: bool critical_path_timing, ) .unwrap(); + let session = Arc::new(session); let turn = Arc::new(turn); turn.turn_timing_state.enable_critical_path(); turn.turn_timing_state .mark_turn_started(Instant::now()) .await; - assert!( - turn.turn_timing_state - .snapshot_path(Instant::now()) - .is_some() + let timings = &turn.turn_timing_state.direct_tool_timings; + let batch = ToolBatchTiming::default(); + let original_started_at = Instant::now(); + let original = timings.start_turn_direct_call( + "call", + &turn.sub_id, + batch.clone(), + original_started_at, + Some(CriticalPath::default()), ); - let cancellation = tokio_util::sync::CancellationToken::new(); - cancellation.cancel(); - let decision = crate::guardian::decide_approval( - Arc::new(session), - &turn, - "review".to_string(), + let request = if network { + crate::guardian::GuardianApprovalRequest::NetworkAccess { + id: "network-review".to_string(), + turn_id: turn.sub_id.clone(), + environment_id: "local".to_string(), + target: "example.com:443".to_string(), + host: "example.com".to_string(), + protocol: codex_protocol::approvals::NetworkApprovalProtocol::Https, + port: 443, + trigger: None, + } + } else { crate::guardian::GuardianApprovalRequest::ApplyPatch { id: "call".to_string(), environment_id: "local".to_string(), cwd: codex_utils_path_uri::PathUri::parse("file:///tmp").unwrap(), files: Vec::new(), patch: String::new(), - }, + } + }; + assert!( + timings + .start_review_for_turn("call", "another-turn") + .is_none() + ); + let cancellation = tokio_util::sync::CancellationToken::new(); + cancellation.cancel(); + let future = crate::guardian::decide_approval( + Arc::clone(&session), + &turn, + "review".to_string(), + request, crate::tools::sandboxing::ApprovalRequestReasons::default(), crate::guardian::GuardianReviewOptions { require_guardian: true, @@ -49,15 +91,130 @@ async fn uncaptured_review_invalidates_before_polling(critical_path_timing: bool require_synchronous_review: false, }, ); - // No polling or worker scheduling is needed to protect a finalizing turn. assert_eq!( turn.turn_timing_state .snapshot_path(Instant::now()) .is_none(), - critical_path_timing + network && critical_path_timing ); + if await_decision { + assert_eq!( + future.await, + Some(codex_protocol::protocol::ReviewDecision::Abort) + ); + let completed = original.finish().unwrap(); + let durations = completed + .path + .unwrap() + .durations(original_started_at, completed.ended_at); + assert!(durations.auto_review > Duration::ZERO); + return; + } + // Finishing before polling observes the review started synchronously by decide_approval. + let completed = original.finish().unwrap(); + let durations = completed + .path + .unwrap() + .durations(original_started_at, completed.ended_at); assert_eq!( - decision.await, - Some(codex_protocol::protocol::ReviewDecision::Abort) + durations.auto_review > Duration::ZERO, + !network && critical_path_timing + ); + drop(original); + let replacement_started_at = Instant::now(); + let replacement = timings.start_turn_direct_call( + "call", + &turn.sub_id, + batch, + replacement_started_at, + Some(CriticalPath::default()), + ); + drop(future); + let completed = replacement.finish().unwrap(); + assert_eq!( + completed + .path + .unwrap() + .durations(replacement_started_at, completed.ended_at), + PathDurations { + tools: completed.ended_at.duration_since(replacement_started_at), + ..PathDurations::default() + } ); } + +#[test_case::test_case(true; "owned")] +#[test_case::test_case(false; "ownerless")] +#[tokio::test] +async fn network_review_timing_uses_trigger_owner(owned: bool) { + let (session, mut turn, _rx) = crate::session::tests::make_session_and_context_with_rx().await; + Arc::make_mut(&mut Arc::get_mut(&mut turn).expect("unique test turn").config) + .features + .enable(codex_features::Feature::CriticalPathTiming) + .expect("critical-path capture is configurable"); + let timing = &turn.turn_timing_state; + let start = Instant::now(); + timing.mark_turn_started(start).await; + timing.enable_critical_path(); + let batch = ToolBatchTiming::default(); + let work = turn + .turn_timing_state + .direct_tool_timings + .start_turn_direct_call( + "network-tool", + &turn.sub_id, + batch.clone(), + start, + timing.snapshot_path(start), + ); + let blocking = timing.begin_tool_blocking(Some(batch)); + let request = super::GuardianApprovalRequest::NetworkAccess { + id: "network-review".to_string(), + turn_id: turn.sub_id.clone(), + environment_id: "local".to_string(), + target: "https://example.com:443".to_string(), + host: "example.com".to_string(), + protocol: codex_protocol::approvals::NetworkApprovalProtocol::Https, + port: 443, + trigger: owned.then(|| super::GuardianNetworkAccessTrigger { + call_id: "network-tool".to_string(), + tool_name: "shell".to_string(), + command: vec!["curl".to_string()], + cwd: test_path_buf("/repo").abs().into(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + tty: None, + }), + }; + // Binding happens before scheduling, so this also covers cancellation before polling. + let decision = super::decide_approval( + Arc::clone(&session), + &turn, + "network-review".to_string(), + request, + crate::tools::sandboxing::ApprovalRequestReasons { + approval: None, + retry: None, + }, + super::GuardianReviewOptions { + require_guardian: true, + plugin_attribution_override: None, + approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, + external_cancel: None, + require_synchronous_review: false, + }, + ); + drop(decision); + drop(work); + drop(blocking); + let end = Instant::now(); + let path = timing.snapshot_path(end); + if owned { + let totals = path.expect("owned network capture").durations(start, end); + assert!(totals.auto_review > Duration::ZERO); + assert_eq!(totals.unknown, Duration::ZERO); + } else { + assert_eq!(path, None); + } +} diff --git a/codex-rs/core/src/session/elicitation_holders_tests.rs b/codex-rs/core/src/session/elicitation_holders_tests.rs index 2d86b74c62..7137805511 100644 --- a/codex-rs/core/src/session/elicitation_holders_tests.rs +++ b/codex-rs/core/src/session/elicitation_holders_tests.rs @@ -1,6 +1,10 @@ +use codex_direct_tool_timing::ToolBatchTiming; +use codex_direct_tool_timing::ToolTimingGuard; use codex_features::Feature; +use codex_utils_critical_path::CriticalPath; use std::collections::HashMap; use std::sync::Arc; +use std::time::Duration; use std::time::Instant; use codex_core_plugins::PluginCommandAttribution; @@ -20,25 +24,32 @@ use super::tests::make_session_and_context_with_rx; use crate::session::step_context::StepContext; use crate::state::ActiveTurn; -async fn start_timing(turn: &mut Arc, enabled: bool) { +fn start_timing( + turn: &mut Arc, + enabled: bool, +) -> (Arc, Instant) { Arc::make_mut(&mut Arc::get_mut(turn).expect("unique test turn").config) .features .set_enabled(Feature::CriticalPathTiming, enabled) .unwrap(); - // Populate state independently to verify disabled capture does not invalidate it. - turn.turn_timing_state.enable_critical_path(); - turn.turn_timing_state - .mark_turn_started(Instant::now()) - .await; + let start = Instant::now(); + let owner = turn + .turn_timing_state + .direct_tool_timings + .start_turn_direct_call( + "call-1", + &turn.sub_id, + ToolBatchTiming::default(), + start, + Some(CriticalPath::default()), + ); + (owner, start) } -fn assert_uncaptured_wait(turn: &super::turn_context::TurnContext, enabled: bool) { - assert_eq!( - turn.turn_timing_state - .snapshot_path(Instant::now()) - .is_none(), - enabled - ); +fn assert_user_wait(owner: &ToolTimingGuard, start: Instant, enabled: bool) { + let completed = owner.finish().unwrap(); + let totals = completed.path.unwrap().durations(start, completed.ended_at); + assert_eq!(totals.user_wait > Duration::ZERO, enabled); } async fn wait_until_held(pause_state: &mut watch::Receiver) { @@ -55,12 +66,13 @@ async fn wait_until_released(pause_state: &mut watch::Receiver) { .expect("elicitation service should remain available"); } -#[test_case::test_case(true; "capture_on")] -#[test_case::test_case(false; "capture_off")] +#[test_case::test_case(true, "call-1"; "capture_on")] +#[test_case::test_case(false, "call-1"; "capture_off")] +#[test_case::test_case(true, "network-prompt"; "distinct_network_prompt_identity")] #[tokio::test] -async fn command_approval_holds_an_elicitation_until_response(capture: bool) { +async fn command_approval_holds_an_elicitation_until_response(capture: bool, prompt_id: &str) { let (session, mut turn_context, events) = make_session_and_context_with_rx().await; - start_timing(&mut turn_context, capture).await; + let (timing, timing_start) = start_timing(&mut turn_context, capture); *session.active_turn.lock().await = Some(ActiveTurn::default()); let mut pause_state = session.subscribe_elicitation_pause_state(); #[allow(deprecated)] @@ -71,6 +83,7 @@ async fn command_approval_holds_an_elicitation_until_response(capture: bool) { }; let request = tokio::spawn({ + let prompt_id = prompt_id.to_owned(); let session = session.clone(); let turn_context = turn_context.clone(); async move { @@ -80,7 +93,8 @@ async fn command_approval_holds_an_elicitation_until_response(capture: bool) { ExecApprovalKind::Command, crate::guardian::GuardianReviewContext::from(turn_context.clone()) .model_context(), - "call-1".to_string(), + prompt_id, + Some("call-1"), /*approval_id*/ None, /*environment_id*/ None, vec!["echo".to_string()], @@ -104,17 +118,19 @@ async fn command_approval_holds_an_elicitation_until_response(capture: bool) { assert_eq!(event.script_path.as_deref(), Some("scripts/run.py")); wait_until_held(&mut pause_state).await; session - .notify_approval("call-1", ReviewDecision::Approved) + .notify_approval(prompt_id, ReviewDecision::Approved) .await; request.await.expect("approval task"); wait_until_released(&mut pause_state).await; - assert_uncaptured_wait(&turn_context, capture); + assert_user_wait(&timing, timing_start, capture); } +#[test_case::test_case(true; "capture_on")] +#[test_case::test_case(false; "capture_off")] #[tokio::test] -async fn patch_approval_holds_an_elicitation_until_response() { +async fn patch_approval_holds_an_elicitation_until_response(capture: bool) { let (session, mut turn_context, events) = make_session_and_context_with_rx().await; - start_timing(&mut turn_context, /*enabled*/ true).await; + let (timing, timing_start) = start_timing(&mut turn_context, capture); *session.active_turn.lock().await = Some(ActiveTurn::default()); let mut pause_state = session.subscribe_elicitation_pause_state(); @@ -141,13 +157,15 @@ async fn patch_approval_holds_an_elicitation_until_response() { .await; request.await.expect("approval task"); wait_until_released(&mut pause_state).await; - assert_uncaptured_wait(&turn_context, /*enabled*/ true); + assert_user_wait(&timing, timing_start, capture); } +#[test_case::test_case(true; "capture_on")] +#[test_case::test_case(false; "capture_off")] #[tokio::test] -async fn permission_request_holds_an_elicitation_until_response() { +async fn permission_request_holds_an_elicitation_until_response(capture: bool) { let (session, mut turn_context, events) = make_session_and_context_with_rx().await; - start_timing(&mut turn_context, /*enabled*/ true).await; + let (timing, timing_start) = start_timing(&mut turn_context, capture); *session.active_turn.lock().await = Some(ActiveTurn::default()); let mut pause_state = session.subscribe_elicitation_pause_state(); @@ -190,16 +208,21 @@ async fn permission_request_holds_an_elicitation_until_response() { .await; request.await.expect("permission request task"); wait_until_released(&mut pause_state).await; - assert_uncaptured_wait(&turn_context, /*enabled*/ true); + assert_user_wait(&timing, timing_start, capture); } -#[test_case::test_case(true, false; "blocking")] -#[test_case::test_case(false, false; "nonblocking_ui_label")] -#[test_case::test_case(true, true; "cancelled")] +#[test_case::test_case(true, true, false; "blocking")] +#[test_case::test_case(true, false, false; "nonblocking_ui_label")] +#[test_case::test_case(true, true, true; "cancelled")] +#[test_case::test_case(false, true, false; "capture_off")] #[tokio::test] -async fn request_user_input_holds_an_elicitation_until_response(blocking: bool, cancel: bool) { +async fn request_user_input_holds_an_elicitation_until_response( + capture: bool, + blocking: bool, + cancel: bool, +) { let (session, mut turn_context, events) = make_session_and_context_with_rx().await; - start_timing(&mut turn_context, /*enabled*/ true).await; + let (timing, timing_start) = start_timing(&mut turn_context, capture); *session.active_turn.lock().await = Some(ActiveTurn::default()); let mut pause_state = session.subscribe_elicitation_pause_state(); @@ -228,7 +251,7 @@ async fn request_user_input_holds_an_elicitation_until_response(blocking: bool, request.abort(); assert!(matches!(request.await, Err(error) if error.is_cancelled())); wait_until_released(&mut pause_state).await; - assert_uncaptured_wait(&turn_context, /*enabled*/ true); + assert_user_wait(&timing, timing_start, capture); return; } let response = RequestUserInputResponse { @@ -240,5 +263,41 @@ async fn request_user_input_holds_an_elicitation_until_response(blocking: bool, request.await.expect("request user input task"); wait_until_released(&mut pause_state).await; - assert_uncaptured_wait(&turn_context, /*enabled*/ true); + assert_user_wait(&timing, timing_start, capture); +} + +#[test_case::test_case(true; "enabled_unowned_wait_is_incomplete")] +#[test_case::test_case(false; "disabled_unowned_wait_is_unchanged")] +#[tokio::test] +async fn unowned_human_wait_does_not_claim_a_complete_path(capture: bool) { + let (session, mut turn_context, _rx) = make_session_and_context_with_rx().await; + let (_tool_timing, _) = start_timing(&mut turn_context, capture); + turn_context.turn_timing_state.enable_critical_path(); + turn_context + .turn_timing_state + .mark_turn_started(Instant::now()) + .await; + assert!( + turn_context + .turn_timing_state + .snapshot_path(Instant::now()) + .is_some() + ); + assert!( + session + .user_wait_timing_owner(&turn_context, Some("unknown-call")) + .is_none() + ); + assert!( + session + .start_user_wait_timing(&turn_context, /*owner*/ None) + .is_none() + ); + assert_eq!( + turn_context + .turn_timing_state + .snapshot_path(Instant::now()) + .is_none(), + capture + ); } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 1e70c9a7e4..6aab70c2ca 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2998,15 +2998,42 @@ impl Session { .await; } - fn invalidate_uncaptured_user_wait(&self, turn_context: &TurnContext) { - // Replace this boundary with owner-scoped timing when user waits are connected. - if turn_context + fn user_wait_timing_owner( + &self, + turn_context: &TurnContext, + call_id: Option<&str>, + ) -> Option> { + if !turn_context .config .features .enabled(Feature::CriticalPathTiming) + { + return None; + } + // Retain the invocation identity before request preparation can suspend. + call_id.and_then(|call_id| { + turn_context + .turn_timing_state + .direct_tool_timings + .owner_for_turn(call_id, &turn_context.sub_id) + }) + } + + fn start_user_wait_timing( + &self, + turn_context: &TurnContext, + owner: Option<&codex_direct_tool_timing::ToolTimingGuard>, + ) -> Option { + let timing = owner.and_then(codex_direct_tool_timing::ToolTimingGuard::start_user_wait); + if timing.is_none() + && turn_context + .config + .features + .enabled(Feature::CriticalPathTiming) { turn_context.turn_timing_state.invalidate_critical_path(); } + timing } /// Emit an exec approval request event and await the user's decision. @@ -3030,6 +3057,7 @@ impl Session { kind: ExecApprovalKind, model_context: ModelInvocationContext, call_id: String, + timing_call_id: Option<&str>, approval_id: Option, environment_id: Option, command: Vec, @@ -3041,6 +3069,7 @@ impl Session { available_decisions: Option>, plugin_attribution_override: Option, ) -> ReviewDecision { + let user_wait_owner = self.user_wait_timing_owner(turn_context, timing_call_id); let _elicitation = self.services.elicitations.register(); // command-level approvals use `call_id`. // `approval_id` identifies subcommand callbacks and stdin writes. @@ -3111,7 +3140,7 @@ impl Session { available_decisions: Some(available_decisions), parsed_cmd, }); - self.invalidate_uncaptured_user_wait(turn_context); + let _user_wait = self.start_user_wait_timing(turn_context, user_wait_owner.as_deref()); self.send_event(turn_context, event).await; rx_approve.await.unwrap_or(ReviewDecision::Abort) } @@ -3128,6 +3157,7 @@ impl Session { reason: Option, grant_root: Option, ) -> ReviewDecision { + let user_wait_owner = self.user_wait_timing_owner(turn_context, Some(&call_id)); let _elicitation = self.services.elicitations.register(); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); @@ -3154,7 +3184,7 @@ impl Session { reason, grant_root, }); - self.invalidate_uncaptured_user_wait(turn_context); + let _user_wait = self.start_user_wait_timing(turn_context, user_wait_owner.as_deref()); self.send_event(turn_context, event).await; rx_approve.await.unwrap_or(ReviewDecision::Abort) } @@ -3172,6 +3202,7 @@ impl Session { cancellation_token: CancellationToken, ) -> Option { let turn_context = &step_context.turn; + let user_wait_owner = self.user_wait_timing_owner(turn_context, Some(&call_id)); let approval_policy = step_context.settings.approval_policy(); let Some(environment) = step_context .environments @@ -3320,11 +3351,12 @@ impl Session { permissions: requested_permissions, cwd: Some(cwd.clone().into()), }); - self.invalidate_uncaptured_user_wait(turn_context); + let _user_wait = self.start_user_wait_timing(turn_context, user_wait_owner.as_deref()); self.send_event(turn_context.as_ref(), event).await; tokio::select! { biased; _ = cancellation_token.cancelled() => { + drop(_user_wait); let mut active = self.active_turn.lock().await; if let Some(at) = active.as_mut() { let mut ts = at.turn_state.lock().await; @@ -3346,6 +3378,7 @@ impl Session { call_id: String, args: RequestUserInputArgs, ) -> Option { + let user_wait_owner = self.user_wait_timing_owner(turn_context, Some(&call_id)); let _elicitation = self.services.elicitations.register(); let sub_id = turn_context.sub_id.clone(); let (tx_response, rx_response) = oneshot::channel(); @@ -3374,7 +3407,7 @@ impl Session { turn_context .turn_metadata_state .mark_user_input_requested_during_turn(); - self.invalidate_uncaptured_user_wait(turn_context); + let _user_wait = self.start_user_wait_timing(turn_context, user_wait_owner.as_deref()); self.send_event(turn_context, event).await; rx_response.await.ok() } diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 22823beb6a..e7e846c439 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -752,6 +752,7 @@ impl Session { ExecApprovalKind::Command, ctx.review_context.model_context(), ctx.call_id.clone(), + Some(&ctx.call_id), /*approval_id*/ None, Some(environment_id.clone()), command.clone(), @@ -782,6 +783,7 @@ impl Session { ExecApprovalKind::WriteStdin, ctx.review_context.model_context(), id.clone(), + Some(&ctx.call_id), Some(approval_id.clone()), Some(environment_id.clone()), vec![ @@ -852,6 +854,7 @@ impl Session { environment_id, command, cwd, + trigger, .. } => { self.request_command_approval( @@ -859,6 +862,7 @@ impl Session { ExecApprovalKind::Command, ctx.review_context.model_context(), ctx.call_id.clone(), + trigger.as_ref().map(|trigger| trigger.call_id.as_str()), /*approval_id*/ None, Some(environment_id.clone()), command.clone(), diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index e53d538054..bd970ce712 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -160,7 +160,10 @@ async fn approval_review_contributor_skips_existing_guardian_model_call() -> Res let observe = codex_core::test_support::observe_turn_timing(&test.codex) .await .expect("active turn"); - let now = std::time::Instant::now(); + let review_start = std::time::Instant::now(); + tokio::task::yield_now().await; + let review_end = std::time::Instant::now(); + assert!(review_end > review_start); reviewer.release.add_permits(/* n */ 1); loop { @@ -176,7 +179,16 @@ async fn approval_review_contributor_skips_existing_guardian_model_call() -> Res } } - assert_eq!(observe(now, now), None); + assert_eq!( + observe(review_start, review_end), + Some([ + Duration::ZERO, + review_end.duration_since(review_start), + Duration::ZERO, + Duration::ZERO, + Duration::ZERO, + ]) + ); let requests = responses.requests(); assert_eq!(requests.len(), 2); assert!(