From 40b4e1e469e1f09a41b9d7a24ce793948c717054 Mon Sep 17 00:00:00 2001 From: limityan Date: Tue, 12 May 2026 22:28:26 +0800 Subject: [PATCH] fix(deep-review): stabilize recovery and capacity waits --- .../core/src/agentic/deep_review/budget.rs | 47 +--- .../src/agentic/deep_review/task_adapter.rs | 80 +++++- .../tools/implementations/task_tool.rs | 229 ++++++++++++++++-- .../agents/components/ReviewTeamPage.test.tsx | 39 ++- .../BtwSessionPanel.review-action.test.tsx | 193 +++++++++++++++ .../components/btw/BtwSessionPanel.tsx | 75 +++++- .../btw/DeepReviewActionBar.i18n.test.ts | 6 +- .../components/btw/DeepReviewActionBar.scss | 51 +++- .../btw/DeepReviewActionBar.test.tsx | 99 +++----- .../action-bar/CapacityQueueNotice.test.tsx | 57 ++++- .../action-bar/CapacityQueueNotice.tsx | 29 ++- .../action-bar/DeepReviewActionBar.tsx | 70 ++---- .../action-bar/PartialResultsPanel.test.tsx | 1 + .../action-bar/PartialResultsPanel.tsx | 20 +- .../interruptionDiagnostics.test.ts | 22 ++ .../action-bar/interruptionDiagnostics.ts | 2 +- .../ReviewActionBarPersistenceService.test.ts | 121 ++++++++- .../ReviewActionBarPersistenceService.ts | 39 ++- .../EventHandlerModule.test.ts | 128 ++++++++++ .../flow-chat-manager/EventHandlerModule.ts | 70 ++++++ .../store/deepReviewActionBarStore.ts | 12 + .../tool-cards/ToolTimeoutIndicator.test.tsx | 101 +++++++- .../tool-cards/ToolTimeoutIndicator.tsx | 3 +- .../utils/deepReviewContinuation.test.ts | 88 ++++++- .../flow_chat/utils/deepReviewContinuation.ts | 106 +++++++- .../utils/deepReviewExperience.test.ts | 150 +++++++++++- .../flow_chat/utils/deepReviewExperience.ts | 155 ++++++++++-- .../utils/reviewSessionSummary.test.ts | 70 ++++++ .../flow_chat/utils/reviewSessionSummary.ts | 31 ++- src/web-ui/src/locales/en-US/flow-chat.json | 20 +- src/web-ui/src/locales/zh-CN/flow-chat.json | 20 +- src/web-ui/src/locales/zh-TW/flow-chat.json | 20 +- .../src/shared/services/review-team/index.ts | 18 -- .../services/review-team/promptBlock.ts | 43 +++- .../services/reviewTargetClassifier.test.ts | 20 ++ .../shared/services/reviewTargetClassifier.ts | 12 + .../shared/services/reviewTeamService.test.ts | 74 ++++-- 37 files changed, 1976 insertions(+), 345 deletions(-) diff --git a/src/crates/core/src/agentic/deep_review/budget.rs b/src/crates/core/src/agentic/deep_review/budget.rs index f82e0fc90..14c3cd2e0 100644 --- a/src/crates/core/src/agentic/deep_review/budget.rs +++ b/src/crates/core/src/agentic/deep_review/budget.rs @@ -556,7 +556,7 @@ impl DeepReviewBudgetTracker { parent_dialog_turn_id: &str, max_active_reviewers: usize, launch_batch: u64, - packet_id: Option<&str>, + _packet_id: Option<&str>, ) -> Result>, DeepReviewPolicyViolation> { let now = Instant::now(); let mut budget = self @@ -564,24 +564,6 @@ impl DeepReviewBudgetTracker { .entry(parent_dialog_turn_id.to_string()) .or_insert_with(|| DeepReviewTurnBudget::new(now)); - if let Some((&earliest_active_batch, _)) = - budget.active_reviewer_launch_batches.iter().next() - { - if earliest_active_batch < launch_batch { - let packet_label = packet_id - .map(str::trim) - .filter(|value| !value.is_empty()) - .unwrap_or("unknown"); - return Err(DeepReviewPolicyViolation::new( - "deep_review_launch_batch_blocked", - format!( - "Reviewer packet '{}' is in launch_batch {}, but launch_batch {} still has active reviewer(s). Wait for earlier-batch reviewers to finish, timeout, or be cancelled before launching this packet. If the queue remains blocked, pause or cancel the queued reviewers from the Review Team action bar and retry with a lower max parallel reviewer setting.", - packet_label, launch_batch, earliest_active_batch - ), - )); - } - } - if budget.active_reviewers >= max_active_reviewers { return Ok(None); } @@ -822,31 +804,22 @@ mod tests { use super::*; #[test] - fn launch_batch_admission_blocks_later_batch_while_earlier_batch_is_active() { + fn launch_batch_admission_allows_later_batch_when_reviewer_capacity_is_free() { let tracker = DeepReviewBudgetTracker::default(); - let turn_id = "turn-launch-batch-blocked"; + let turn_id = "turn-launch-batch-fill-free-slot"; let _first_batch = tracker .try_begin_active_reviewer_for_launch_batch(turn_id, 2, 1, Some("packet-a")) .expect("batch admission should not fail") .expect("first reviewer should start"); - let violation = match tracker.try_begin_active_reviewer_for_launch_batch( - turn_id, - 2, - 2, - Some("packet-b"), - ) { - Err(violation) => violation, - Ok(_) => panic!("later launch batch should wait while earlier batch is active"), - }; + let second_batch = tracker + .try_begin_active_reviewer_for_launch_batch(turn_id, 2, 2, Some("packet-b")) + .expect("later batch admission should not fail when reviewer capacity is free"); - assert_eq!(violation.code, "deep_review_launch_batch_blocked"); - assert!(violation.message.contains("packet-b")); - assert!(violation.message.contains("launch_batch 2")); - assert!(violation.message.contains("launch_batch 1")); - assert!(violation - .message - .contains("Wait for earlier-batch reviewers")); + assert!( + second_batch.is_some(), + "later batch should fill a freed reviewer slot instead of waiting for the earlier batch to drain" + ); } #[test] diff --git a/src/crates/core/src/agentic/deep_review/task_adapter.rs b/src/crates/core/src/agentic/deep_review/task_adapter.rs index f9bb17a8c..5c87a7ba9 100644 --- a/src/crates/core/src/agentic/deep_review/task_adapter.rs +++ b/src/crates/core/src/agentic/deep_review/task_adapter.rs @@ -37,6 +37,9 @@ use tokio::time::sleep; const DEEP_REVIEW_QUEUE_POLL_INTERVAL: Duration = Duration::from_millis(10); #[cfg(not(test))] const DEEP_REVIEW_QUEUE_POLL_INTERVAL: Duration = Duration::from_secs(1); +pub(crate) const DEEP_REVIEW_PROVIDER_CAPACITY_MAX_RETRY_ATTEMPTS: usize = 3; +const DEEP_REVIEW_PROVIDER_CAPACITY_BACKOFF_MULTIPLIER: u64 = 3; +const DEEP_REVIEW_PROVIDER_CAPACITY_MAX_BACKOFF_SECONDS: u64 = 600; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum DeepReviewQueueWaitSkipReason { @@ -59,6 +62,7 @@ pub(crate) enum DeepReviewQueueWaitOutcome { pub(crate) enum DeepReviewProviderQueueWaitOutcome { ReadyToRetry { queue_elapsed_ms: u64, + early_capacity_probe: bool, }, Skipped { queue_elapsed_ms: u64, @@ -514,6 +518,39 @@ pub(crate) fn provider_capacity_queue_wait_seconds( .filter(|seconds| *seconds > 0) } +pub(crate) fn provider_capacity_queue_wait_seconds_for_attempt( + decision: &DeepReviewCapacityQueueDecision, + conc_policy: &DeepReviewConcurrencyPolicy, + retry_attempt_index: usize, +) -> Option { + let base_wait_seconds = provider_capacity_queue_wait_seconds(decision, conc_policy)?; + if decision.retry_after_seconds.is_some() { + return Some(base_wait_seconds); + } + + let multiplier = DEEP_REVIEW_PROVIDER_CAPACITY_BACKOFF_MULTIPLIER.saturating_pow( + u32::try_from(retry_attempt_index) + .unwrap_or(u32::MAX) + .min(8), + ); + Some( + base_wait_seconds + .saturating_mul(multiplier) + .min(DEEP_REVIEW_PROVIDER_CAPACITY_MAX_BACKOFF_SECONDS), + ) + .filter(|seconds| *seconds > 0) +} + +fn provider_capacity_wait_can_wake_on_active_reviewer_release( + reason: DeepReviewCapacityQueueReason, +) -> bool { + matches!( + reason, + DeepReviewCapacityQueueReason::ProviderConcurrencyLimit + | DeepReviewCapacityQueueReason::TemporaryOverload + ) +} + pub(crate) fn capacity_skip_result_for_provider_reason( reason: DeepReviewCapacityQueueReason, dialog_turn_id: &str, @@ -707,6 +744,9 @@ pub(crate) async fn wait_for_provider_capacity_retry( let mut queue_timer = QueueWaitTimer::start(Instant::now()); let max_wait = Duration::from_secs(max_wait_seconds); let optional_reviewer_count = is_optional_reviewer.then_some(1); + let initial_active_reviewers = deep_review_active_reviewer_count(dialog_turn_id); + let can_wake_on_active_reviewer_release = + provider_capacity_wait_can_wake_on_active_reviewer_release(reason); record_deep_review_runtime_provider_capacity_queue(dialog_turn_id, reason); @@ -737,7 +777,7 @@ pub(crate) async fn wait_for_provider_capacity_retry( optional_reviewer_count, Some(effective_parallel_instances), queue_elapsed_ms, - conc_policy.max_queue_wait_seconds, + max_wait_seconds, ) .await; return DeepReviewProviderQueueWaitOutcome::Skipped { @@ -764,7 +804,7 @@ pub(crate) async fn wait_for_provider_capacity_retry( optional_reviewer_count, Some(effective_parallel_instances), queue_elapsed_ms, - conc_policy.max_queue_wait_seconds, + max_wait_seconds, ) .await; sleep(DEEP_REVIEW_QUEUE_POLL_INTERVAL).await; @@ -788,10 +828,40 @@ pub(crate) async fn wait_for_provider_capacity_retry( optional_reviewer_count, Some(effective_parallel_instances), queue_elapsed_ms, - conc_policy.max_queue_wait_seconds, + max_wait_seconds, ) .await; - return DeepReviewProviderQueueWaitOutcome::ReadyToRetry { queue_elapsed_ms }; + return DeepReviewProviderQueueWaitOutcome::ReadyToRetry { + queue_elapsed_ms, + early_capacity_probe: false, + }; + } + + if can_wake_on_active_reviewer_release + && initial_active_reviewers > 0 + && active_reviewers < initial_active_reviewers + { + record_deep_review_runtime_queue_wait(dialog_turn_id, queue_elapsed_ms); + clear_deep_review_queue_control_for_tool(dialog_turn_id, tool_id); + emit_queue_state( + session_id, + dialog_turn_id, + tool_id, + subagent_type, + DeepReviewQueueStatus::Running, + Some(reason), + 0, + active_reviewers, + optional_reviewer_count, + Some(effective_parallel_instances), + queue_elapsed_ms, + max_wait_seconds, + ) + .await; + return DeepReviewProviderQueueWaitOutcome::ReadyToRetry { + queue_elapsed_ms, + early_capacity_probe: true, + }; } emit_queue_state( @@ -806,7 +876,7 @@ pub(crate) async fn wait_for_provider_capacity_retry( optional_reviewer_count, Some(effective_parallel_instances), queue_elapsed_ms, - conc_policy.max_queue_wait_seconds, + max_wait_seconds, ) .await; diff --git a/src/crates/core/src/agentic/tools/implementations/task_tool.rs b/src/crates/core/src/agentic/tools/implementations/task_tool.rs index 04108d99d..da5265245 100644 --- a/src/crates/core/src/agentic/tools/implementations/task_tool.rs +++ b/src/crates/core/src/agentic/tools/implementations/task_tool.rs @@ -205,11 +205,16 @@ impl TaskTool { ) } - fn deep_review_provider_capacity_queue_wait_seconds( + fn deep_review_provider_capacity_queue_wait_seconds_for_attempt( decision: &crate::agentic::deep_review_policy::DeepReviewCapacityQueueDecision, conc_policy: &DeepReviewConcurrencyPolicy, + retry_attempt_index: usize, ) -> Option { - deep_review_task_adapter::provider_capacity_queue_wait_seconds(decision, conc_policy) + deep_review_task_adapter::provider_capacity_queue_wait_seconds_for_attempt( + decision, + conc_policy, + retry_attempt_index, + ) } async fn wait_for_deep_review_provider_capacity_retry( @@ -1098,6 +1103,7 @@ impl Tool for TaskTool { let prepared_prompt = prompt; let mut provider_capacity_retry_reason: Option = None; let mut provider_capacity_queue_elapsed_ms = 0_u64; + let mut provider_capacity_retry_attempts = 0_usize; let result = loop { let parent_info = SubagentParentInfo { tool_call_id: tool_call_id.clone(), @@ -1140,7 +1146,9 @@ impl Tool for TaskTool { { drop(deep_review_active_guard.take()); - if provider_capacity_retry_reason.is_some() { + if provider_capacity_retry_attempts + >= deep_review_task_adapter::DEEP_REVIEW_PROVIDER_CAPACITY_MAX_RETRY_ATTEMPTS + { let (data, assistant_message) = Self::deep_review_capacity_skip_result_for_provider_queue_outcome( reason, &dialog_turn_id, @@ -1177,9 +1185,10 @@ impl Tool for TaskTool { } if let Some(max_wait_seconds) = - Self::deep_review_provider_capacity_queue_wait_seconds( + Self::deep_review_provider_capacity_queue_wait_seconds_for_attempt( &decision, conc_policy, + provider_capacity_retry_attempts, ) { match Self::wait_for_deep_review_provider_capacity_retry( @@ -1196,8 +1205,11 @@ impl Tool for TaskTool { { DeepReviewProviderQueueWaitOutcome::ReadyToRetry { queue_elapsed_ms, + early_capacity_probe, } => { - provider_capacity_queue_elapsed_ms = queue_elapsed_ms; + provider_capacity_queue_elapsed_ms = + provider_capacity_queue_elapsed_ms + .saturating_add(queue_elapsed_ms); let effective_parallel_instances = deep_review_effective_parallel_instances( &dialog_turn_id, @@ -1257,6 +1269,11 @@ impl Tool for TaskTool { } } provider_capacity_retry_reason = Some(reason); + if !early_capacity_probe { + provider_capacity_retry_attempts = + provider_capacity_retry_attempts + .saturating_add(1); + } Self::record_deep_review_provider_capacity_retry( &dialog_turn_id, reason, @@ -1267,13 +1284,16 @@ impl Tool for TaskTool { queue_elapsed_ms, skip_reason, } => { + provider_capacity_queue_elapsed_ms = + provider_capacity_queue_elapsed_ms + .saturating_add(queue_elapsed_ms); let (data, assistant_message) = Self::deep_review_capacity_skip_result_for_provider_queue_outcome( reason, &dialog_turn_id, &subagent_type, conc_policy, start_time.elapsed().as_millis(), - queue_elapsed_ms, + provider_capacity_queue_elapsed_ms, Some(skip_reason), ); return Ok(vec![ToolResult::Result { @@ -1405,8 +1425,8 @@ impl Tool for TaskTool { #[cfg(test)] mod tests { use super::TaskTool; - use crate::agentic::agents::{get_agent_registry, Agent, AgentCategory, SubAgentSource}; use crate::agentic::agents::CustomSubagentConfig; + use crate::agentic::agents::{get_agent_registry, Agent, AgentCategory, SubAgentSource}; use crate::agentic::deep_review::task_adapter as deep_review_task_adapter; use crate::agentic::deep_review_policy::{ DeepReviewBudgetTracker, DeepReviewExecutionPolicy, DeepReviewSubagentRole, @@ -1811,19 +1831,23 @@ mod tests { } #[tokio::test] - async fn deep_review_capacity_queue_waits_for_previous_launch_batch_without_lowering_cap() { + async fn deep_review_capacity_queue_starts_later_batch_when_reviewer_capacity_frees() { use crate::agentic::deep_review::task_adapter::DeepReviewLaunchBatchInfo; use crate::agentic::deep_review_policy::{ deep_review_capacity_skip_count, deep_review_effective_parallel_instances, try_begin_deep_review_active_reviewer_for_launch_batch, DeepReviewConcurrencyPolicy, }; - let turn_id = "turn-launch-batch-queue-wait"; - let tool_id = "tool-launch-batch-queue-wait"; - let occupied = + let turn_id = "turn-launch-batch-fill-free-slot"; + let tool_id = "tool-launch-batch-fill-free-slot"; + let occupied_a = try_begin_deep_review_active_reviewer_for_launch_batch(turn_id, 2, 1, Some("packet-a")) .expect("launch batch admission should not fail") .expect("first batch reviewer should start"); + let occupied_b = + try_begin_deep_review_active_reviewer_for_launch_batch(turn_id, 2, 1, Some("packet-b")) + .expect("launch batch admission should not fail") + .expect("second first-batch reviewer should start"); let policy = DeepReviewConcurrencyPolicy { max_parallel_instances: 2, stagger_seconds: 0, @@ -1855,22 +1879,23 @@ mod tests { tokio::time::sleep(tokio::time::Duration::from_millis(30)).await; assert!( !handle.is_finished(), - "later launch batch should wait while an earlier batch is active" + "later launch batch should wait while reviewer capacity is full" ); - drop(occupied); + drop(occupied_a); let outcome = tokio::time::timeout(tokio::time::Duration::from_millis(500), handle) .await - .expect("later launch batch should become ready after earlier batch finishes") + .expect("later launch batch should become ready as soon as reviewer capacity frees") .expect("spawned wait should not panic") .expect("queue wait should resolve"); match outcome { super::DeepReviewQueueWaitOutcome::Ready { .. } => {} super::DeepReviewQueueWaitOutcome::Skipped { .. } => { - panic!("later launch batch should not expire while an earlier batch is active"); + panic!("later launch batch should not expire after reviewer capacity frees"); } } + drop(occupied_b); assert_eq!(deep_review_capacity_skip_count(turn_id), 0); assert_eq!(deep_review_effective_parallel_instances(turn_id, 2), 2); } @@ -2568,11 +2593,40 @@ mod tests { ); assert_eq!( - TaskTool::deep_review_provider_capacity_queue_wait_seconds(&decision, &policy), + TaskTool::deep_review_provider_capacity_queue_wait_seconds_for_attempt( + &decision, &policy, 0, + ), Some(30) ); } + #[test] + fn deep_review_provider_queue_wait_uses_exponential_backoff_attempts() { + use crate::agentic::deep_review_policy::DeepReviewConcurrencyPolicy; + + let policy = DeepReviewConcurrencyPolicy { + max_parallel_instances: 3, + stagger_seconds: 0, + max_queue_wait_seconds: 60, + batch_extras_separately: true, + allow_bounded_auto_retry: false, + auto_retry_elapsed_guard_seconds: 180, + }; + let decision = TaskTool::deep_review_capacity_decision_for_provider_error( + &BitFunError::ai("Provider error: code=429, message=too many concurrent requests"), + ); + + let waits = (0..3) + .map(|attempt| { + TaskTool::deep_review_provider_capacity_queue_wait_seconds_for_attempt( + &decision, &policy, attempt, + ) + }) + .collect::>(); + + assert_eq!(waits, vec![Some(60), Some(180), Some(540)]); + } + #[test] fn deep_review_provider_queue_wait_rejects_fail_fast_errors() { use crate::agentic::deep_review_policy::DeepReviewConcurrencyPolicy; @@ -2590,11 +2644,148 @@ mod tests { ); assert_eq!( - TaskTool::deep_review_provider_capacity_queue_wait_seconds(&decision, &policy), + TaskTool::deep_review_provider_capacity_queue_wait_seconds_for_attempt( + &decision, &policy, 0, + ), None ); } + #[tokio::test] + async fn deep_review_provider_capacity_queue_retries_when_active_reviewer_frees_capacity() { + use crate::agentic::deep_review::task_adapter::DeepReviewProviderQueueWaitOutcome; + use crate::agentic::deep_review_policy::{ + try_begin_deep_review_active_reviewer, DeepReviewCapacityQueueReason, + DeepReviewConcurrencyPolicy, + }; + + let turn_id = "turn-provider-queue-active-release"; + let tool_id = "tool-provider-queue-active-release"; + let occupied = try_begin_deep_review_active_reviewer(turn_id, 2) + .expect("precondition should occupy another reviewer slot"); + let policy = DeepReviewConcurrencyPolicy { + max_parallel_instances: 2, + stagger_seconds: 0, + max_queue_wait_seconds: 60, + batch_extras_separately: true, + allow_bounded_auto_retry: false, + auto_retry_elapsed_guard_seconds: 180, + }; + let turn_id_owned = turn_id.to_string(); + let tool_id_owned = tool_id.to_string(); + + let handle = tokio::spawn(async move { + TaskTool::wait_for_deep_review_provider_capacity_retry( + "session-provider-queue-active-release", + &turn_id_owned, + &tool_id_owned, + "ReviewSecurity", + &policy, + DeepReviewCapacityQueueReason::ProviderConcurrencyLimit, + 60, + false, + ) + .await + }); + + tokio::time::sleep(tokio::time::Duration::from_millis(30)).await; + assert!( + !handle.is_finished(), + "provider queue should keep waiting while no additional reviewer capacity freed" + ); + drop(occupied); + + let outcome = tokio::time::timeout(tokio::time::Duration::from_millis(500), handle) + .await + .expect("provider queue should wake when another active reviewer frees capacity") + .expect("spawned wait should not panic"); + + match outcome { + DeepReviewProviderQueueWaitOutcome::ReadyToRetry { + queue_elapsed_ms, + early_capacity_probe, + } => { + assert!( + queue_elapsed_ms < 500, + "early capacity wake should not wait for the full backoff window" + ); + assert!( + early_capacity_probe, + "active reviewer release should be marked as an early provider capacity probe" + ); + } + DeepReviewProviderQueueWaitOutcome::Skipped { .. } => { + panic!("provider queue should retry after active reviewer capacity frees") + } + } + } + + #[tokio::test] + async fn deep_review_provider_retry_after_wait_ignores_active_reviewer_release() { + use crate::agentic::deep_review::task_adapter::DeepReviewProviderQueueWaitOutcome; + use crate::agentic::deep_review_policy::{ + try_begin_deep_review_active_reviewer, DeepReviewCapacityQueueReason, + DeepReviewConcurrencyPolicy, + }; + + let turn_id = "turn-provider-retry-after-hard-wait"; + let tool_id = "tool-provider-retry-after-hard-wait"; + let occupied = try_begin_deep_review_active_reviewer(turn_id, 2) + .expect("precondition should occupy another reviewer slot"); + let policy = DeepReviewConcurrencyPolicy { + max_parallel_instances: 2, + stagger_seconds: 0, + max_queue_wait_seconds: 1, + batch_extras_separately: true, + allow_bounded_auto_retry: false, + auto_retry_elapsed_guard_seconds: 180, + }; + let turn_id_owned = turn_id.to_string(); + let tool_id_owned = tool_id.to_string(); + + let handle = tokio::spawn(async move { + TaskTool::wait_for_deep_review_provider_capacity_retry( + "session-provider-retry-after-hard-wait", + &turn_id_owned, + &tool_id_owned, + "ReviewSecurity", + &policy, + DeepReviewCapacityQueueReason::RetryAfter, + 1, + false, + ) + .await + }); + + tokio::time::sleep(tokio::time::Duration::from_millis(30)).await; + drop(occupied); + tokio::time::sleep(tokio::time::Duration::from_millis(120)).await; + assert!( + !handle.is_finished(), + "retry-after waits should not be interrupted by local reviewer capacity release" + ); + + let outcome = tokio::time::timeout(tokio::time::Duration::from_millis(1500), handle) + .await + .expect("retry-after wait should eventually finish") + .expect("spawned wait should not panic"); + + match outcome { + DeepReviewProviderQueueWaitOutcome::ReadyToRetry { + early_capacity_probe, + .. + } => { + assert!( + !early_capacity_probe, + "retry-after completion should be a natural cooldown retry" + ); + } + DeepReviewProviderQueueWaitOutcome::Skipped { .. } => { + panic!("retry-after wait should retry after its bounded cooldown") + } + } + } + #[tokio::test] async fn deep_review_provider_capacity_queue_cancel_control_skips_retry() { use crate::agentic::deep_review::task_adapter::{ @@ -2703,7 +2894,9 @@ mod tests { .expect("spawned wait should not panic"); match outcome { - DeepReviewProviderQueueWaitOutcome::ReadyToRetry { queue_elapsed_ms } => { + DeepReviewProviderQueueWaitOutcome::ReadyToRetry { + queue_elapsed_ms, .. + } => { assert!(queue_elapsed_ms >= 900); } DeepReviewProviderQueueWaitOutcome::Skipped { .. } => { diff --git a/src/web-ui/src/app/scenes/agents/components/ReviewTeamPage.test.tsx b/src/web-ui/src/app/scenes/agents/components/ReviewTeamPage.test.tsx index ca6df7df4..66dea73de 100644 --- a/src/web-ui/src/app/scenes/agents/components/ReviewTeamPage.test.tsx +++ b/src/web-ui/src/app/scenes/agents/components/ReviewTeamPage.test.tsx @@ -2,7 +2,7 @@ import React, { act } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createRoot, type Root } from 'react-dom/client'; -const loadDefaultReviewTeam = vi.fn(); +const loadDefaultReviewTeam = vi.hoisted(() => vi.fn()); const notificationFns = vi.hoisted(() => ({ success: vi.fn(), error: vi.fn(), @@ -63,6 +63,11 @@ vi.mock('@/infrastructure/api/service-api/ConfigAPI', () => ({ }, })); +vi.mock('@/infrastructure/config/services/modelConfigs', () => ({ + getModelDisplayName: (model: { name?: string; id?: string; model_name?: string }) => + model.name ?? model.model_name ?? model.id ?? 'model', +})); + vi.mock('@/infrastructure/api/service-api/SubagentAPI', () => ({ SubagentAPI: { listSubagents: vi.fn(async () => []), @@ -84,15 +89,29 @@ vi.mock('@/infrastructure/contexts/WorkspaceContext', () => ({ useCurrentWorkspace: () => ({ workspacePath: 'D:/workspace/project' }), })); -vi.mock('@/shared/services/reviewTeamService', async () => { - const actual = await vi.importActual( - '@/shared/services/reviewTeamService', - ); - return { - ...actual, - loadDefaultReviewTeam, - }; -}); +vi.mock('@/shared/services/reviewTeamService', () => ({ + DEFAULT_REVIEW_TEAM_CONCURRENCY_POLICY: { + maxConcurrentReviewers: 4, + }, + DEFAULT_REVIEW_TEAM_EXECUTION_POLICY: { + reviewerTimeoutSeconds: 300, + judgeTimeoutSeconds: 240, + reviewerFileSplitThreshold: 20, + maxSameRoleInstances: 3, + }, + DEFAULT_REVIEW_TEAM_MODEL: 'fast', + FALLBACK_REVIEW_TEAM_DEFINITION: { + id: 'default-review-team', + name: 'Code Review Team', + members: [], + }, + REVIEW_STRATEGY_DEFINITIONS: { + quick: { label: 'Quick' }, + normal: { label: 'Normal' }, + deep: { label: 'Deep' }, + }, + loadDefaultReviewTeam, +})); let JSDOMCtor: (new ( html?: string, diff --git a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx index 00d296ed4..352a3f93c 100644 --- a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx +++ b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx @@ -183,6 +183,77 @@ function createReviewSession(): Session { } as Session; } +function createCompletedDeepReviewWithoutResult(): Session { + const childSession = createReviewSession(); + return { + ...childSession, + dialogTurns: childSession.dialogTurns.map((turn) => ({ + ...turn, + modelRounds: turn.modelRounds.map((round) => ({ + ...round, + items: [{ + id: 'reviewer-task', + type: 'tool', + timestamp: 2, + status: 'completed', + toolName: 'Task', + toolCall: { + id: 'task-security', + input: { subagent_type: 'ReviewSecurity' }, + }, + toolResult: { + success: true, + result: { + summary: { + overall_assessment: 'Security reviewer found no blockers.', + }, + }, + }, + }], + })), + })), + } as Session; +} + +function createInterruptedDeepReviewWithoutResult(): Session { + const childSession = createCompletedDeepReviewWithoutResult(); + return { + ...childSession, + status: 'error', + error: 'previous execution failed', + dialogTurns: childSession.dialogTurns.map((turn) => ({ + ...turn, + status: 'error', + error: 'previous execution failed', + })), + } as Session; +} + +function createCancelledResumeDeepReview(): Session { + const childSession = createInterruptedDeepReviewWithoutResult(); + return { + ...childSession, + status: 'idle', + error: null, + dialogTurns: [ + ...childSession.dialogTurns, + { + id: 'turn-2', + sessionId: 'deep-review-child', + userMessage: { + id: 'user-2', + content: 'Continue interrupted Deep Review', + timestamp: 2, + }, + modelRounds: [], + status: 'cancelled', + startTime: 2, + timestamp: 2, + }, + ], + } as Session; +} + describe('BtwSessionPanel review action bar integration', () => { let container: HTMLDivElement; let root: Root; @@ -242,4 +313,126 @@ describe('BtwSessionPanel review action bar integration', () => { }); expect(useReviewActionBarStore.getState().remediationItems).toEqual([]); }); + + it('shows a resumable Deep Review action bar when the run completed without a structured report', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['deep-review-child', createCompletedDeepReviewWithoutResult()], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(useReviewActionBarStore.getState()).toMatchObject({ + childSessionId: 'deep-review-child', + phase: 'review_interrupted', + interruption: expect.objectContaining({ + canResume: true, + resultRecoveryReason: 'missing_submit_code_review', + }), + }); + }); + + it('does not restore a stale interruption while a resume request is starting', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['deep-review-child', createInterruptedDeepReviewWithoutResult()], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + const store = useReviewActionBarStore.getState(); + store.showInterruptedActionBar({ + childSessionId: 'deep-review-child', + parentSessionId: 'parent-session', + interruption: { + phase: 'review_interrupted', + childSessionId: 'deep-review-child', + parentSessionId: 'parent-session', + originalTarget: '/DeepReview review latest commit', + errorDetail: { category: 'unknown', rawMessage: 'previous execution failed' }, + canResume: true, + recommendedActions: [], + reviewers: [], + }, + }); + store.setActiveAction('resume', { baselineTurnId: 'turn-1' }); + store.updatePhase('resume_running'); + store.minimize(); + + await act(async () => { + root.render( + , + ); + }); + + expect(useReviewActionBarStore.getState()).toMatchObject({ + childSessionId: 'deep-review-child', + phase: 'resume_running', + minimized: true, + }); + }); + + it('restores the interrupted action bar when a resumed Deep Review is cancelled by the user', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['deep-review-child', createCancelledResumeDeepReview()], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + const store = useReviewActionBarStore.getState(); + store.showInterruptedActionBar({ + childSessionId: 'deep-review-child', + parentSessionId: 'parent-session', + interruption: { + phase: 'review_interrupted', + childSessionId: 'deep-review-child', + parentSessionId: 'parent-session', + originalTarget: '/DeepReview review latest commit', + errorDetail: { category: 'unknown', rawMessage: 'previous execution failed' }, + canResume: true, + recommendedActions: [], + reviewers: [], + }, + }); + store.setActiveAction('resume', { baselineTurnId: 'turn-1' }); + store.updatePhase('resume_running'); + store.minimize(); + + await act(async () => { + root.render( + , + ); + }); + + expect(useReviewActionBarStore.getState()).toMatchObject({ + childSessionId: 'deep-review-child', + phase: 'review_interrupted', + minimized: false, + interruption: expect.objectContaining({ + interruptionReason: 'manual_cancelled', + }), + }); + }); }); diff --git a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.tsx b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.tsx index 727bebd3f..eef3c5104 100644 --- a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.tsx +++ b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.tsx @@ -24,8 +24,12 @@ import {globalEventBus} from '@/infrastructure/event-bus'; import {notificationService} from '@/shared/notification-system'; import {createLogger} from '@/shared/utils/logger'; import {settleStoppedReviewSessionState} from '../../utils/reviewSessionStop'; -import {findLatestCodeReviewResult} from '../../utils/reviewSessionSummary'; -import {deriveDeepReviewInterruption} from '../../utils/deepReviewContinuation'; +import {findLatestCodeReviewResult, findLatestCodeReviewResultState} from '../../utils/reviewSessionSummary'; +import { + deriveDeepReviewInterruption, + deriveDeepReviewResultRecoveryInterruption, + type DeepReviewResultRecoveryReason, +} from '../../utils/deepReviewContinuation'; import {buildReviewRemediationItems, type CodeReviewRemediationData} from '../../utils/codeReviewRemediation'; import {ReviewActionBar} from './DeepReviewActionBar'; import {type ReviewActionMode, type ReviewActionPhase, useReviewActionBarStore} from '../../store/deepReviewActionBarStore'; @@ -384,7 +388,10 @@ export const BtwSessionPanel: React.FC = ({ useEffect(() => { if (!isReviewSession || !childSessionId || !childSession) return; - const latestReviewData = findLatestCodeReviewResult(childSession) as DeepReviewActionData | null; + const latestReviewResultState = findLatestCodeReviewResultState(childSession); + const latestReviewData = latestReviewResultState.status === 'valid' + ? latestReviewResultState.result as DeepReviewActionData + : null; const reviewMode: ReviewActionMode = isDeepReview ? 'deep' : 'standard'; const latestReviewMode = latestReviewData?.review_mode ?? 'standard'; const lastTurn = childSession.dialogTurns[childSession.dialogTurns.length - 1]; @@ -394,8 +401,70 @@ export const BtwSessionPanel: React.FC = ({ const deepReviewInterruption = isDeepReview ? deriveDeepReviewInterruption(childSession) : null; + const resultRecoveryReason: DeepReviewResultRecoveryReason | null = + isDeepReview && isComplete + ? latestReviewResultState.status === 'missing' + ? 'missing_submit_code_review' + : latestReviewResultState.status === 'invalid' + ? 'invalid_submit_code_review' + : latestReviewData && latestReviewMode !== 'deep' + ? 'wrong_review_mode' + : null + : null; + const resultRecoveryInterruption = resultRecoveryReason + ? deriveDeepReviewResultRecoveryInterruption(childSession, resultRecoveryReason) + : null; const store = useReviewActionBarStore.getState(); + const isCurrentResumeRunning = + store.childSessionId === childSessionId && + store.phase === 'resume_running'; + if (isCurrentResumeRunning) { + const resumeTurnHasStarted = + !store.resumeBaselineTurnId || + lastTurn?.id !== store.resumeBaselineTurnId; + + if (!resumeTurnHasStarted) { + return; + } + + if (turnStatus === 'error') { + store.updatePhase('resume_failed', lastTurn?.error ?? childSession.error ?? undefined); + store.restore(); + return; + } + + if (turnStatus === 'cancelled' && deepReviewInterruption) { + store.showInterruptedActionBar({ + childSessionId, + parentSessionId: parentSessionId ?? null, + interruption: deepReviewInterruption, + }); + store.restore(); + return; + } + + if (turnStatus !== 'completed') { + return; + } + } + + if (resultRecoveryInterruption) { + const canShowResultRecovery = + store.childSessionId !== childSessionId || + store.phase === 'idle' || + store.phase === 'review_waiting_capacity' || + store.phase === 'resume_running'; + + if (canShowResultRecovery) { + store.showInterruptedActionBar({ + childSessionId, + parentSessionId: parentSessionId ?? null, + interruption: resultRecoveryInterruption, + }); + } + return; + } if (isDeepReview && (!latestReviewData || latestReviewMode !== 'deep') && deepReviewInterruption) { store.showInterruptedActionBar({ diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts index 2ba113d3e..f24821c66 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts @@ -33,6 +33,8 @@ const REQUIRED_ACTION_BAR_KEYS = [ 'deepReviewActionBar.fixInterrupted', 'deepReviewActionBar.continueFix', 'deepReviewActionBar.skipRemaining', + 'deepReviewActionBar.manualCancel.title', + 'deepReviewActionBar.manualCancel.description', 'deepReviewActionBar.decisionGate.title', 'deepReviewActionBar.decisionGate.description', 'deepReviewActionBar.decisionGate.supplementLabel', @@ -55,7 +57,6 @@ const REQUIRED_ACTION_BAR_KEYS = [ 'deepReviewActionBar.capacityQueue.continueQueue', 'deepReviewActionBar.capacityQueue.cancelQueued', 'deepReviewActionBar.capacityQueue.skipOptionalQueued', - 'deepReviewActionBar.capacityQueue.runSlowerNextTime', 'deepReviewActionBar.capacityQueue.openReviewSettings', 'deepReviewActionBar.capacityQueue.reasons.launchBatchBlocked', 'deepReviewActionBar.capacityQueue.reasonDetails.providerRateLimit', @@ -64,9 +65,6 @@ const REQUIRED_ACTION_BAR_KEYS = [ 'deepReviewActionBar.capacityQueue.reasonDetails.localConcurrencyCap', 'deepReviewActionBar.capacityQueue.reasonDetails.launchBatchBlocked', 'deepReviewActionBar.capacityQueue.reasonDetails.temporaryOverload', - 'deepReviewActionBar.capacityQueue.runSlowerSaved', - 'deepReviewActionBar.capacityQueue.runSlowerFailed', - 'deepReviewActionBar.capacityQueue.runSlowerFailedWithReason', 'deepReviewActionBar.capacityQueue.controlFailed', 'deepReviewActionBar.capacityQueue.controlFailedWithReason', 'deepReviewActionBar.capacityQueue.controlPartiallyFailedWithReason', diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss index 292ae69e5..5c9b133e9 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss @@ -973,29 +973,64 @@ font-weight: 500; } + &__partial-list { + margin: 6px 0 0; + padding-left: 16px; + } + + &__partial-text { + margin: 4px 0; + color: var(--color-text-secondary); + line-height: 1.4; + } + /* Error attribution */ &__attribution { display: flex; - flex-direction: column; - gap: 8px; - padding: 10px 12px; + align-items: flex-start; + gap: 10px; + padding: 9px 12px; border-radius: 8px; font-size: 12px; + border: 1px solid var(--border-subtle, var(--border-base)); + background: color-mix(in srgb, var(--color-bg-secondary) 82%, transparent); + + &::before { + content: ''; + flex: 0 0 auto; + width: 6px; + height: 6px; + margin-top: 6px; + border-radius: 999px; + background: var(--color-text-tertiary); + box-shadow: 0 0 0 3px color-mix(in srgb, var(--color-text-tertiary) 10%, transparent); + } &--warning { - background: color-mix(in srgb, var(--color-warning, #f59e0b) 8%, var(--color-bg-secondary)); - border: 1px solid color-mix(in srgb, var(--color-warning, #f59e0b) 18%, transparent); + background: color-mix(in srgb, var(--color-warning, #f59e0b) 7%, var(--color-bg-secondary)); + border-color: color-mix(in srgb, var(--color-warning, #f59e0b) 18%, var(--border-base)); + + &::before { + background: var(--color-warning, #f59e0b); + box-shadow: 0 0 0 3px color-mix(in srgb, var(--color-warning, #f59e0b) 14%, transparent); + } } &--error { - background: color-mix(in srgb, var(--color-error, #ef4444) 8%, var(--color-bg-secondary)); - border: 1px solid color-mix(in srgb, var(--color-error, #ef4444) 18%, transparent); + background: color-mix(in srgb, var(--color-error, #ef4444) 7%, var(--color-bg-secondary)); + border-color: color-mix(in srgb, var(--color-error, #ef4444) 18%, var(--border-base)); + + &::before { + background: var(--color-error, #ef4444); + box-shadow: 0 0 0 3px color-mix(in srgb, var(--color-error, #ef4444) 14%, transparent); + } } } &__attribution-message { + flex: 1 1 auto; color: var(--color-text-secondary); - line-height: 1.5; + line-height: 1.45; } /* Recovery plan */ diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx index 397caecea..66b1fe59a 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx @@ -7,6 +7,7 @@ const sendMessageMock = vi.hoisted(() => vi.fn()); const eventBusEmitMock = vi.hoisted(() => vi.fn()); const confirmWarningMock = vi.hoisted(() => vi.fn()); const continueDeepReviewSessionMock = vi.hoisted(() => vi.fn()); +const buildErrorAttributionMock = vi.hoisted(() => vi.fn(() => null)); const buildRecoveryPlanMock = vi.hoisted(() => vi.fn(() => ({ willPreserve: ['ReviewSecurity'], willRerun: ['ReviewPerformance'], @@ -14,7 +15,6 @@ const buildRecoveryPlanMock = vi.hoisted(() => vi.fn(() => ({ summaryText: '1 completed reviewer will be preserved; 1 reviewer will be rerun', }))); const controlDeepReviewQueueMock = vi.hoisted(() => vi.fn()); -const lowerDefaultReviewTeamMaxParallelReviewersMock = vi.hoisted(() => vi.fn()); const flowChatSessionsMock = vi.hoisted(() => new Map()); vi.mock('react-i18next', () => ({ @@ -88,10 +88,6 @@ vi.mock('@/infrastructure/api/service-api/AgentAPI', () => ({ }, })); -vi.mock('@/shared/services/reviewTeamService', () => ({ - lowerDefaultReviewTeamMaxParallelReviewers: lowerDefaultReviewTeamMaxParallelReviewersMock, -})); - vi.mock('@/infrastructure/event-bus', () => ({ globalEventBus: { emit: eventBusEmitMock, @@ -133,7 +129,7 @@ vi.mock('../../utils/deepReviewExperience', () => ({ aggregateReviewerProgress: () => [], buildReviewerProgressSummary: () => null, extractPartialReviewData: () => null, - buildErrorAttribution: () => null, + buildErrorAttribution: buildErrorAttributionMock, buildRecoveryPlan: buildRecoveryPlanMock, evaluateDegradationOptions: () => [], })); @@ -192,13 +188,7 @@ describeWithJsdom('DeepReviewActionBar', () => { confirmWarningMock.mockResolvedValue(true); eventBusEmitMock.mockReturnValue(false); continueDeepReviewSessionMock.mockResolvedValue(undefined); - lowerDefaultReviewTeamMaxParallelReviewersMock.mockResolvedValue({ - maxParallelInstances: 1, - maxQueueWaitSeconds: 120, - allowProviderCapacityQueue: true, - allowBoundedAutoRetry: false, - autoRetryElapsedGuardSeconds: 180, - }); + buildErrorAttributionMock.mockReturnValue(null); flowChatSessionsMock.clear(); useReviewActionBarStore.getState().reset(); }); @@ -479,7 +469,7 @@ describeWithJsdom('DeepReviewActionBar', () => { expect(container.textContent).toContain('Reason: provider concurrency limit'); expect(container.textContent).toContain('Waited 12s of 1m 0s'); expect(container.textContent).toContain('Your active session is busy.'); - expect(container.textContent).toContain('Run slower next time'); + expect(container.textContent).not.toContain('Run slower next time'); expect(container.textContent).toContain('Open Review settings'); const pauseButton = Array.from(container.querySelectorAll('button')) @@ -496,17 +486,6 @@ describeWithJsdom('DeepReviewActionBar', () => { }).capacityQueueState.status).toBe('paused_by_user'); expect(container.textContent).toContain('Queue paused'); - const runSlowerButton = Array.from(container.querySelectorAll('button')) - .find((button) => button.textContent?.includes('Run slower next time')); - expect(runSlowerButton).toBeTruthy(); - - await act(async () => { - runSlowerButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); - await Promise.resolve(); - }); - - expect(lowerDefaultReviewTeamMaxParallelReviewersMock).toHaveBeenCalledTimes(1); - const openSettingsButton = Array.from(container.querySelectorAll('button')) .find((button) => button.textContent?.includes('Open Review settings')); expect(openSettingsButton).toBeTruthy(); @@ -682,44 +661,6 @@ describeWithJsdom('DeepReviewActionBar', () => { }).capacityQueueState.status).toBe('queued_for_capacity'); }); - it('shows the settings update reason when run-slower fails', async () => { - const { DeepReviewActionBar } = await import('./DeepReviewActionBar'); - const { notificationService } = await import('@/shared/notification-system'); - lowerDefaultReviewTeamMaxParallelReviewersMock.mockRejectedValueOnce( - new Error('config store unavailable'), - ); - - useReviewActionBarStore.getState().showCapacityQueueBar({ - childSessionId: 'child-session', - parentSessionId: 'parent-session', - capacityQueueState: { - status: 'queued_for_capacity', - reason: 'local_concurrency_cap', - queuedReviewerCount: 1, - }, - }); - - await act(async () => { - root.render(); - }); - - const runSlowerButton = Array.from(container.querySelectorAll('button')) - .find((button) => button.textContent?.includes('Run slower next time')); - expect(runSlowerButton).toBeTruthy(); - - await act(async () => { - runSlowerButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); - await Promise.resolve(); - }); - - expect(notificationService.error).toHaveBeenCalledWith( - expect.stringContaining('config store unavailable'), - ); - expect(notificationService.error).toHaveBeenCalledWith( - expect.stringContaining('Open Review settings'), - ); - }); - it('starts a structured retry turn for explicit incomplete Deep Review slices', async () => { const { DeepReviewActionBar } = await import('./DeepReviewActionBar'); @@ -1011,6 +952,16 @@ describeWithJsdom('DeepReviewActionBar', () => { it('keeps Deep Review interruption actions in one row without a standalone retry or recovery toggle', async () => { const { DeepReviewActionBar } = await import('./DeepReviewActionBar'); + buildErrorAttributionMock.mockReturnValue({ + category: 'network', + title: 'Network issue', + severity: 'warning', + description: 'Please retry later, or check your network and model service status.', + actions: [ + { code: 'retry', labelKey: 'errors:ai.actions.retry' }, + { code: 'copy_diagnostics', labelKey: 'errors:ai.actions.copyDiagnostics' }, + ], + }); useReviewActionBarStore.getState().showInterruptedActionBar({ childSessionId: 'deep-review-session', @@ -1047,12 +998,18 @@ describeWithJsdom('DeepReviewActionBar', () => { expect(buttonTexts.some((text) => text.includes('Copy diagnostics'))).toBe(true); expect(buttonTexts.some((text) => text.includes('Retry'))).toBe(false); expect(buttonTexts.some((text) => text.includes('Show recovery plan'))).toBe(false); + expect(container.querySelectorAll('.deep-review-action-bar__attribution button')).toHaveLength(0); + expect(container.querySelector('.deep-review-action-bar__attribution-actions')).toBeNull(); expect(container.textContent).toContain('1 completed reviewers will be preserved'); expect(container.textContent).toContain('1 reviewers will be rerun'); }); - it('minimizes and disables the continue action after a resume request starts successfully', async () => { + it('minimizes and hides stale interruption controls after a resume request starts successfully', async () => { const { DeepReviewActionBar } = await import('./DeepReviewActionBar'); + let resolveContinuation: (() => void) | null = null; + continueDeepReviewSessionMock.mockReturnValueOnce(new Promise((resolve) => { + resolveContinuation = resolve; + })); useReviewActionBarStore.getState().showInterruptedActionBar({ childSessionId: 'deep-review-session', @@ -1086,9 +1043,17 @@ describeWithJsdom('DeepReviewActionBar', () => { expect(continueDeepReviewSessionMock).toHaveBeenCalledTimes(1); expect(state.phase).toBe('resume_running'); expect(state.minimized).toBe(true); + expect(state.activeAction).toBe('resume'); + expect(container.textContent).toContain('Continuing review'); + expect(container.textContent).not.toContain('Deep review interrupted'); + expect(Array.from(container.querySelectorAll('button')) + .some((button) => button.textContent?.includes('Continue review'))).toBe(false); + expect(Array.from(container.querySelectorAll('button')) + .some((button) => button.textContent?.includes('Copy diagnostics'))).toBe(false); - const restoredContinueButton = Array.from(container.querySelectorAll('button')) - .find((button) => button.textContent?.includes('Continue review')) as HTMLButtonElement | undefined; - expect(restoredContinueButton?.disabled).toBe(true); + await act(async () => { + resolveContinuation?.(); + await Promise.resolve(); + }); }); }); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.test.tsx b/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.test.tsx index 97e99634a..b41759007 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.test.tsx +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.test.tsx @@ -41,7 +41,6 @@ describe('CapacityQueueNotice', () => { onContinueQueue={vi.fn()} onSkipOptionalQueuedReviewers={vi.fn()} onCancelQueuedReviewers={vi.fn()} - onRunSlowerNextTime={vi.fn()} onOpenReviewSettings={vi.fn()} />, ); @@ -53,7 +52,7 @@ describe('CapacityQueueNotice', () => { expect(html).toContain('Waited 12s of 1m 0s'); expect(html).toContain('Pause queue'); expect(html).toContain('Skip optional extras'); - expect(html).toContain('Run slower next time'); + expect(html).not.toContain('Run slower next time'); }); it('renders launch-batch waiting as a concrete queue reason', () => { @@ -72,7 +71,6 @@ describe('CapacityQueueNotice', () => { onContinueQueue={vi.fn()} onSkipOptionalQueuedReviewers={vi.fn()} onCancelQueuedReviewers={vi.fn()} - onRunSlowerNextTime={vi.fn()} onOpenReviewSettings={vi.fn()} />, ); @@ -84,6 +82,56 @@ describe('CapacityQueueNotice', () => { expect(html).not.toContain('Waited 4s of 1m 0s'); }); + it('explains launch-batch waits that outlast the configured queue window', () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain('waited longer than the configured queue window'); + expect(html).toContain('Cancel queued reviewers'); + expect(html).toContain('Open Review settings'); + expect(html).not.toContain('Run slower next time'); + expect(html).not.toContain('Waited 1m 30s of 1m 0s'); + }); + + it('does not show the long launch-batch detail before the queue window is exceeded', () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).not.toContain('waited longer than the configured queue window'); + }); + it('explains active-reviewer waits without presenting max wait as a hard timeout', () => { const html = renderToStaticMarkup( { onContinueQueue={vi.fn()} onSkipOptionalQueuedReviewers={vi.fn()} onCancelQueuedReviewers={vi.fn()} - onRunSlowerNextTime={vi.fn()} onOpenReviewSettings={vi.fn()} />, ); @@ -141,7 +188,6 @@ describe('CapacityQueueNotice', () => { onContinueQueue={vi.fn()} onSkipOptionalQueuedReviewers={vi.fn()} onCancelQueuedReviewers={vi.fn()} - onRunSlowerNextTime={vi.fn()} onOpenReviewSettings={vi.fn()} />, ); @@ -166,7 +212,6 @@ describe('CapacityQueueNotice', () => { onContinueQueue={vi.fn()} onSkipOptionalQueuedReviewers={vi.fn()} onCancelQueuedReviewers={vi.fn()} - onRunSlowerNextTime={vi.fn()} onOpenReviewSettings={vi.fn()} />, ); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.tsx b/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.tsx index 8acfaed12..0ef788037 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.tsx +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.tsx @@ -4,7 +4,6 @@ import { Clock, Pause, Play, - Settings, SkipForward, } from 'lucide-react'; import { Button } from '@/component-library'; @@ -21,7 +20,6 @@ interface CapacityQueueNoticeProps { onContinueQueue: () => void | Promise; onSkipOptionalQueuedReviewers: () => void | Promise; onCancelQueuedReviewers: () => void | Promise; - onRunSlowerNextTime: () => void | Promise; onOpenReviewSettings: () => void | Promise; } @@ -40,11 +38,11 @@ const CAPACITY_QUEUE_REASON_DETAIL_KEYS: Record = { provider_rate_limit: { key: 'deepReviewActionBar.capacityQueue.reasonDetails.providerRateLimit', - defaultValue: 'The model provider is rate-limiting requests. BitFun will wait briefly; reducing Review Team parallel reviewers can help if this repeats.', + defaultValue: 'The model provider is rate-limiting requests. BitFun will wait briefly and continue when capacity returns.', }, provider_concurrency_limit: { key: 'deepReviewActionBar.capacityQueue.reasonDetails.providerConcurrencyLimit', - defaultValue: 'The model provider rejected another concurrent reviewer. BitFun will retry after capacity opens; lower reviewer parallelism if it keeps happening.', + defaultValue: 'The model provider rejected another concurrent reviewer. BitFun will retry after capacity opens.', }, retry_after: { key: 'deepReviewActionBar.capacityQueue.reasonDetails.retryAfter', @@ -102,7 +100,6 @@ export const CapacityQueueNotice: React.FC = ({ onContinueQueue, onSkipOptionalQueuedReviewers, onCancelQueuedReviewers, - onRunSlowerNextTime, onOpenReviewSettings, }) => { const { t } = useTranslation('flow-chat'); @@ -124,6 +121,11 @@ export const CapacityQueueNotice: React.FC = ({ : null; const capacityQueueWaitMode = getCapacityQueueWaitMode(capacityQueueState); const activeReviewerCount = capacityQueueState.activeReviewerCount ?? 0; + const isLongLaunchBatchWait = capacityQueueState.reason === 'launch_batch_blocked' + && activeReviewerCount > 0 + && capacityQueueState.queueElapsedMs !== undefined + && capacityQueueState.maxQueueWaitSeconds !== undefined + && capacityQueueState.queueElapsedMs > capacityQueueState.maxQueueWaitSeconds * 1000; const capacityQueueTitle = capacityQueueState.status === 'paused_by_user' ? t('deepReviewActionBar.capacityQueue.pausedTitle', { defaultValue: 'Queue paused', @@ -207,6 +209,13 @@ export const CapacityQueueNotice: React.FC = ({ {capacityQueueReasonDetail} )} + {isLongLaunchBatchWait && ( + + {t('deepReviewActionBar.capacityQueue.longLaunchBatchWaitDetail', { + defaultValue: 'This reviewer has waited longer than the configured queue window because an earlier reviewer batch is still running. You can keep waiting, pause the queue, cancel queued reviewers, or open Review settings.', + })} + + )} {capacityQueueState.sessionConcurrencyHigh && ( {t('deepReviewActionBar.capacityQueue.sessionBusy', { @@ -326,16 +335,6 @@ export const CapacityQueueNotice: React.FC = ({ )} - - ))} - - )} )} {/* Recovery plan preview */} - {hasInterruption && recoveryPlan && ( + {showInterruptionDetails && recoveryPlan && ( )} {/* Context overflow degradation options */} - {hasInterruption && interruption?.errorDetail?.category === 'context_overflow' && ( + {showInterruptionDetails && interruption?.errorDetail?.category === 'context_overflow' && (
{t('deepReviewActionBar.contextOverflowTitle', { @@ -960,7 +922,7 @@ export const ReviewActionBar: React.FC = () => { isDeepReview={isDeepReview} retryableSliceCount={retryableSlices.length} remediationItemCount={remediationItems.length} - hasInterruption={hasInterruption} + hasInterruption={showInterruptionDetails} partialResultsAvailable={Boolean(partialResults?.hasPartialResults)} activeAction={activeAction} isFixDisabled={isFixDisabled} diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.test.tsx b/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.test.tsx index 004453344..8900e02d8 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.test.tsx +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.test.tsx @@ -61,5 +61,6 @@ describe('PartialResultsPanel', () => { expect(html).toContain('Hide partial results'); expect(html).toContain('1 issues found'); expect(html).toContain('1 remediation items'); + expect(html).toContain('Security completed'); }); }); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.tsx b/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.tsx index d27ed7694..16863c61a 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.tsx +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/PartialResultsPanel.tsx @@ -20,7 +20,8 @@ export const PartialResultsPanel: React.FC = ({ onTogglePartialResults, }) => { const { t } = useTranslation('flow-chat'); - const showSummary = Boolean(progressSummary && progressSummary.completed > 0); + const hasPartialDetails = Boolean(partialResults?.hasPartialResults); + const showSummary = Boolean(progressSummary && progressSummary.completed > 0 && hasPartialDetails); return ( <> @@ -68,6 +69,23 @@ export const PartialResultsPanel: React.FC = ({
)} + {partialResults.completedReviewerSummaries.length > 0 && ( +
+ + {t('deepReviewActionBar.partialReviewerSummaries', { + count: partialResults.completedReviewerSummaries.length, + defaultValue: '{{count}} reviewer summaries', + })} + +
    + {partialResults.completedReviewerSummaries.map((summary, index) => ( +
  • + {summary} +
    • + ))} +
+
+ )} )} diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.test.ts b/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.test.ts index 5b71df682..4b2d6bef5 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.test.ts +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.test.ts @@ -52,4 +52,26 @@ describe('buildInterruptionDiagnostics', () => { expect(diagnostics).toContain('... [truncated]'); expect(diagnostics.length).toBeLessThan(1500); }); + + it('expands terse presenter diagnostics when the interruption has a raw failure message', () => { + const diagnostics = buildInterruptionDiagnostics( + { + category: 'unknown', + rawMessage: 'Conversation execution failed after ReviewSecurity completed.', + }, + { + ...presentation, + category: 'unknown', + titleKey: 'errors:ai.executionFailed', + messageKey: 'errors:ai.genericSuggestion', + diagnostics: 'category=unknown', + }, + t, + ); + + expect(diagnostics).toContain('=== Deep Review Interruption Diagnostics ==='); + expect(diagnostics).toContain('Error type: unknown (unknown)'); + expect(diagnostics).toContain('raw message: Conversation execution failed after ReviewSecurity completed.'); + expect(diagnostics).not.toBe('category=unknown'); + }); }); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.ts b/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.ts index 9b91a8efa..b64b69cf9 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.ts +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/interruptionDiagnostics.ts @@ -14,7 +14,7 @@ export function buildInterruptionDiagnostics( presentation: AiErrorPresentation, t: Translate, ): string { - if (presentation.diagnostics) { + if (presentation.diagnostics && !presentation.diagnostics.trim().startsWith('category=')) { return presentation.diagnostics; } diff --git a/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.test.ts b/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.test.ts index 1e915ddc7..95946ad90 100644 --- a/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.test.ts +++ b/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.test.ts @@ -22,9 +22,47 @@ vi.mock('../store/FlowChatStore', () => ({ }, })); +function createMetadata(overrides: Record = {}) { + return { + sessionId: 'session-1', + sessionName: 'Test Session', + agentType: 'agentic', + modelName: 'auto', + createdAt: 1000, + lastActiveAt: 1000, + turnCount: 1, + messageCount: 1, + toolCallCount: 0, + status: 'active', + tags: [], + ...overrides, + }; +} + +function createStoreSession(overrides: Record = {}) { + return { + sessionId: 'session-1', + title: 'Test Session', + dialogTurns: [], + status: 'idle', + config: { agentType: 'agentic', modelName: 'auto' }, + createdAt: 1000, + lastActiveAt: 1000, + error: null, + sessionKind: 'deep_review', + workspacePath: '/workspace/project', + ...overrides, + }; +} + describe('ReviewActionBarPersistenceService', () => { beforeEach(() => { vi.clearAllMocks(); + (sessionAPI.saveSessionMetadata as any).mockResolvedValue(undefined); + (sessionAPI.loadSessionMetadata as any).mockResolvedValue(undefined); + (flowChatStore.getState as any).mockReturnValue({ + sessions: new Map(), + }); }); afterEach(() => { @@ -79,12 +117,13 @@ describe('ReviewActionBarPersistenceService', () => { }); it('saves metadata with reviewActionState when session exists', async () => { - const mockSession = { - workspacePath: '/workspace/project', - remoteConnectionId: undefined, - remoteSshHost: undefined, - }; + const mockSession = createStoreSession(); + const existingMetadata = createMetadata({ + sessionName: 'Existing Session', + customMetadata: { kind: 'deep_review' }, + }); + (sessionAPI.loadSessionMetadata as any).mockResolvedValue(existingMetadata); (flowChatStore.getState as any).mockReturnValue({ sessions: new Map([['session-1', mockSession]]), }); @@ -111,6 +150,9 @@ describe('ReviewActionBarPersistenceService', () => { expect(sessionAPI.saveSessionMetadata).toHaveBeenCalledTimes(1); const [metadata, workspacePath] = (sessionAPI.saveSessionMetadata as any).mock.calls[0]; expect(metadata.sessionId).toBe('session-1'); + expect(metadata.sessionName).toBe('Existing Session'); + expect(metadata.agentType).toBe('agentic'); + expect(metadata.modelName).toBe('auto'); expect(metadata.reviewActionState).toEqual({ version: 1, phase: 'review_completed', @@ -122,13 +164,64 @@ describe('ReviewActionBarPersistenceService', () => { expect(workspacePath).toBe('/workspace/project'); }); + it('builds complete metadata when no existing metadata is available', async () => { + const mockSession = createStoreSession({ + title: 'Fresh Review', + createdAt: 2000, + lastActiveAt: 2500, + }); + + (sessionAPI.loadSessionMetadata as any).mockResolvedValue(null); + (flowChatStore.getState as any).mockReturnValue({ + sessions: new Map([['session-1', mockSession]]), + }); + + await persistReviewActionState({ + childSessionId: 'session-1', + parentSessionId: null, + reviewMode: 'deep', + phase: 'review_completed', + reviewData: null, + remediationItems: [], + selectedRemediationIds: new Set(), + dismissed: false, + minimized: false, + activeAction: null, + customInstructions: '', + errorMessage: null, + interruption: null, + completedRemediationIds: new Set(), + fixingRemediationIds: new Set(), + remainingFixIds: [], + } as any); + + expect(sessionAPI.saveSessionMetadata).toHaveBeenCalledTimes(1); + const [metadata] = (sessionAPI.saveSessionMetadata as any).mock.calls[0]; + expect(metadata).toMatchObject({ + sessionId: 'session-1', + sessionName: 'Fresh Review', + agentType: 'agentic', + modelName: 'auto', + status: 'active', + reviewActionState: { + version: 1, + phase: 'review_completed', + completedRemediationIds: [], + minimized: false, + customInstructions: '', + persistedAt: expect.any(Number), + }, + }); + expect(Array.isArray(metadata.tags)).toBe(true); + }); + it('passes remote connection info when available', async () => { - const mockSession = { - workspacePath: '/workspace/project', + const mockSession = createStoreSession({ remoteConnectionId: 'remote-1', remoteSshHost: 'ssh-host-1', - }; + }); + (sessionAPI.loadSessionMetadata as any).mockResolvedValue(createMetadata()); (flowChatStore.getState as any).mockReturnValue({ sessions: new Map([['session-1', mockSession]]), }); @@ -161,11 +254,23 @@ describe('ReviewActionBarPersistenceService', () => { describe('clearPersistedReviewState', () => { it('saves metadata with undefined reviewActionState', async () => { + (sessionAPI.loadSessionMetadata as any).mockResolvedValue(createMetadata({ + reviewActionState: { + version: 1, + phase: 'review_completed', + completedRemediationIds: [], + minimized: false, + customInstructions: '', + persistedAt: 1000, + }, + })); + await clearPersistedReviewState('session-1', '/workspace/project'); expect(sessionAPI.saveSessionMetadata).toHaveBeenCalledTimes(1); const [metadata] = (sessionAPI.saveSessionMetadata as any).mock.calls[0]; expect(metadata.sessionId).toBe('session-1'); + expect(metadata.sessionName).toBe('Test Session'); expect(metadata.reviewActionState).toBeUndefined(); }); }); diff --git a/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.ts b/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.ts index 0b8b92f94..bbfe924ff 100644 --- a/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.ts +++ b/src/web-ui/src/flow_chat/services/ReviewActionBarPersistenceService.ts @@ -8,8 +8,9 @@ import { createLogger } from '@/shared/utils/logger'; import { sessionAPI } from '@/infrastructure/api/service-api/SessionAPI'; import { flowChatStore } from '../store/FlowChatStore'; +import { buildSessionMetadata } from '../utils/sessionMetadata'; import type { ReviewActionBarState } from '../store/deepReviewActionBarStore'; -import type { ReviewActionPersistedState } from '@/shared/types/session-history'; +import type { ReviewActionPersistedState, SessionMetadata } from '@/shared/types/session-history'; const log = createLogger('ReviewActionBarPersistence'); @@ -29,11 +30,28 @@ export async function persistReviewActionState(state: ReviewActionBarState): Pro }; try { - await sessionAPI.saveSessionMetadata( - { + let existingMetadata: SessionMetadata | null = null; + try { + existingMetadata = await sessionAPI.loadSessionMetadata( + state.childSessionId, + session.workspacePath, + session.remoteConnectionId, + session.remoteSshHost + ); + } catch (error) { + log.warn('Failed to load session metadata before persisting review action state', { sessionId: state.childSessionId, - reviewActionState: payload, - } as any, + error, + }); + } + + const metadata = { + ...(existingMetadata ?? buildSessionMetadata(session, null)), + reviewActionState: payload, + }; + + await sessionAPI.saveSessionMetadata( + metadata, session.workspacePath, session.remoteConnectionId, session.remoteSshHost @@ -46,11 +64,14 @@ export async function persistReviewActionState(state: ReviewActionBarState): Pro export async function clearPersistedReviewState(sessionId: string, workspacePath: string): Promise { try { + const existingMetadata = await sessionAPI.loadSessionMetadata(sessionId, workspacePath); + if (!existingMetadata) return; + + const metadata = { ...existingMetadata }; + delete metadata.reviewActionState; + await sessionAPI.saveSessionMetadata( - { - sessionId, - reviewActionState: undefined, - } as any, + metadata, workspacePath ); } catch (error) { diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.test.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.test.ts index 7aa0d07bd..a54e6f05e 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.test.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.test.ts @@ -80,9 +80,11 @@ describe('shouldProcessEvent', () => { beforeEach(() => { vi.restoreAllMocks(); + resetFlowChatStore(); }); afterEach(() => { + resetFlowChatStore(); stateMachineManager.clear(); }); @@ -131,6 +133,132 @@ describe('shouldProcessEvent', () => { ).toBe(false); }); + it('recovers active latest-turn data when the state machine was reset to idle', () => { + FlowChatStore.getInstance().setState(() => ({ + sessions: new Map([[ + mockSessionId, + { + sessionId: mockSessionId, + title: 'Test Session', + dialogTurns: [{ + id: mockTurnId, + sessionId: mockSessionId, + userMessage: { + id: 'user-1', + content: 'Continue review', + timestamp: 1000, + }, + modelRounds: [], + status: 'processing', + startTime: 1000, + }], + status: 'idle', + config: { agentType: 'agentic' }, + createdAt: 1000, + lastActiveAt: 1000, + error: null, + sessionKind: 'normal', + } as Session, + ]]), + activeSessionId: mockSessionId, + })); + stateMachineManager.getOrCreate(mockSessionId); + + expect( + shouldProcessEvent(mockSessionId, mockTurnId, 'data', 'ToolEvent'), + ).toBe(true); + expect(stateMachineManager.getCurrentState(mockSessionId)).toBe(SessionExecutionState.PROCESSING); + expect(stateMachineManager.get(mockSessionId)?.getContext().currentDialogTurnId).toBe(mockTurnId); + }); + + it('does not recover idle data for an old non-latest turn', () => { + FlowChatStore.getInstance().setState(() => ({ + sessions: new Map([[ + mockSessionId, + { + sessionId: mockSessionId, + title: 'Test Session', + dialogTurns: [ + { + id: mockTurnId, + sessionId: mockSessionId, + userMessage: { + id: 'user-1', + content: 'Old turn', + timestamp: 1000, + }, + modelRounds: [], + status: 'processing', + startTime: 1000, + }, + { + id: 'newer-turn', + sessionId: mockSessionId, + userMessage: { + id: 'user-2', + content: 'New turn', + timestamp: 2000, + }, + modelRounds: [], + status: 'processing', + startTime: 2000, + }, + ], + status: 'idle', + config: { agentType: 'agentic' }, + createdAt: 1000, + lastActiveAt: 2000, + error: null, + sessionKind: 'normal', + } as Session, + ]]), + activeSessionId: mockSessionId, + })); + stateMachineManager.getOrCreate(mockSessionId); + + expect( + shouldProcessEvent(mockSessionId, mockTurnId, 'data', 'ToolEvent'), + ).toBe(false); + expect(stateMachineManager.getCurrentState(mockSessionId)).toBe(SessionExecutionState.IDLE); + }); + + it('does not recover idle data for a cancelled latest turn', () => { + FlowChatStore.getInstance().setState(() => ({ + sessions: new Map([[ + mockSessionId, + { + sessionId: mockSessionId, + title: 'Test Session', + dialogTurns: [{ + id: mockTurnId, + sessionId: mockSessionId, + userMessage: { + id: 'user-1', + content: 'Cancelled review', + timestamp: 1000, + }, + modelRounds: [], + status: 'cancelled', + startTime: 1000, + }], + status: 'idle', + config: { agentType: 'agentic' }, + createdAt: 1000, + lastActiveAt: 1000, + error: null, + sessionKind: 'normal', + } as Session, + ]]), + activeSessionId: mockSessionId, + })); + stateMachineManager.getOrCreate(mockSessionId); + + expect( + shouldProcessEvent(mockSessionId, mockTurnId, 'data', 'ToolEvent'), + ).toBe(false); + expect(stateMachineManager.getCurrentState(mockSessionId)).toBe(SessionExecutionState.IDLE); + }); + it('returns false for data event when turn ID mismatches', () => { vi.spyOn(stateMachineManager, 'get').mockReturnValue({ getCurrentState: () => SessionExecutionState.PROCESSING, diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.ts index 5d829fc70..1c8a3040e 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/EventHandlerModule.ts @@ -96,6 +96,13 @@ function isStreamingExecutionState(state: SessionExecutionState): boolean { return state === SessionExecutionState.PROCESSING || state === SessionExecutionState.FINISHING; } +const RECOVERABLE_IDLE_TURN_STATUSES = new Set([ + 'pending', + 'image_analyzing', + 'processing', + 'finishing', +]); + export function isAppWindowFocused(): boolean { if (typeof document === 'undefined') { return true; @@ -123,6 +130,59 @@ function logDroppedDataEvent( }); } +function recoverIdleLatestTurnDataEvent( + eventName: string, + sessionId: string, + turnId: string | null, + currentState: SessionExecutionState, + currentDialogTurnId: string | null +): boolean { + if ( + currentState !== SessionExecutionState.IDLE || + !turnId || + currentDialogTurnId + ) { + return false; + } + + const session = FlowChatStore.getInstance().getState().sessions.get(sessionId); + const latestTurn = session?.dialogTurns[session.dialogTurns.length - 1]; + if ( + !latestTurn || + latestTurn.id !== turnId || + !RECOVERABLE_IDLE_TURN_STATUSES.has(latestTurn.status) + ) { + return false; + } + + const machine = stateMachineManager.get(sessionId); + const machineContext = machine?.getContext(); + if (machineContext) { + machineContext.currentDialogTurnId = turnId; + } + + void stateMachineManager + .transition(sessionId, SessionExecutionEvent.START, { + taskId: sessionId, + dialogTurnId: turnId, + }) + .catch(error => { + log.error('State machine transition failed while recovering active data event', { + sessionId, + turnId, + eventName, + error, + }); + }); + + log.debug('Recovered active data event after idle state', { + sessionId, + turnId, + eventName, + }); + return true; +} + function handleDeepReviewQueueStateChanged(event: DeepReviewQueueStateChangedEvent): void { const store = FlowChatStore.getInstance(); const session = store.getState().sessions.get(event.sessionId); @@ -331,6 +391,16 @@ export function shouldProcessEvent( } if (!isStreamingExecutionState(currentState)) { + if (recoverIdleLatestTurnDataEvent( + eventName, + sessionId, + turnId, + currentState, + context.currentDialogTurnId, + )) { + return true; + } + logDroppedDataEvent(eventName, sessionId, turnId, { reason: 'state_not_accepting_data', currentState, diff --git a/src/web-ui/src/flow_chat/store/deepReviewActionBarStore.ts b/src/web-ui/src/flow_chat/store/deepReviewActionBarStore.ts index 791038af8..1df61dd30 100644 --- a/src/web-ui/src/flow_chat/store/deepReviewActionBarStore.ts +++ b/src/web-ui/src/flow_chat/store/deepReviewActionBarStore.ts @@ -121,6 +121,8 @@ export interface ReviewActionBarState { fixingRemediationIds: Set; /** Last dialog turn that existed before the current fix request was submitted */ fixingBaselineTurnId: string | null; + /** Last dialog turn that existed before the current resume request was submitted */ + resumeBaselineTurnId: string | null; /** IDs of items remaining when a fix was interrupted */ remainingFixIds: string[]; /** User's option choice for needs_decision items: map of item id -> option index */ @@ -194,6 +196,7 @@ const initialState = { completedRemediationIds: new Set(), fixingRemediationIds: new Set(), fixingBaselineTurnId: null as string | null, + resumeBaselineTurnId: null as string | null, remainingFixIds: [] as string[], decisionSelections: {} as Record, capacityQueueState: null as DeepReviewCapacityQueueState | null, @@ -334,6 +337,7 @@ export const useReviewActionBarStore = create((set, get) = completedRemediationIds: preservedCompleted, fixingRemediationIds: new Set(), fixingBaselineTurnId: null, + resumeBaselineTurnId: null, remainingFixIds: [], decisionSelections: {}, capacityQueueState: null, @@ -360,6 +364,7 @@ export const useReviewActionBarStore = create((set, get) = completedRemediationIds: new Set(), fixingRemediationIds: new Set(), fixingBaselineTurnId: null, + resumeBaselineTurnId: null, remainingFixIds: [], decisionSelections: {}, capacityQueueState: null, @@ -388,6 +393,7 @@ export const useReviewActionBarStore = create((set, get) = : new Set(), fixingRemediationIds: new Set(), fixingBaselineTurnId: null, + resumeBaselineTurnId: null, remainingFixIds: [], decisionSelections: {}, capacityQueueState: withNormalizedWaitingReviewers(capacityQueueState), @@ -409,6 +415,7 @@ export const useReviewActionBarStore = create((set, get) = completedRemediationIds: nextCompleted, fixingRemediationIds: new Set(), fixingBaselineTurnId: null, + resumeBaselineTurnId: null, remainingFixIds: [], }); } else { @@ -416,6 +423,7 @@ export const useReviewActionBarStore = create((set, get) = phase, errorMessage: errorMessage ?? null, ...(phase !== 'fix_running' ? { fixingBaselineTurnId: null } : {}), + ...(phase !== 'resume_running' ? { resumeBaselineTurnId: null } : {}), }); } }, @@ -502,6 +510,9 @@ export const useReviewActionBarStore = create((set, get) = set({ activeAction: action, lastSubmittedAction: action, + ...(action === 'resume' + ? { resumeBaselineTurnId: options?.baselineTurnId ?? null } + : {}), }); } else { set({ activeAction: action }); @@ -520,6 +531,7 @@ export const useReviewActionBarStore = create((set, get) = phase: 'review_completed', remainingFixIds: [], fixingBaselineTurnId: null, + resumeBaselineTurnId: null, activeAction: null, lastSubmittedAction: null, }), diff --git a/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.test.tsx b/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.test.tsx index 477ec9ed9..8350413ac 100644 --- a/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.test.tsx +++ b/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.test.tsx @@ -1,10 +1,20 @@ import React from 'react'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; import { renderToStaticMarkup } from 'react-dom/server'; import { I18nextProvider, initReactI18next } from 'react-i18next'; import { createInstance, type i18n as I18nInstance } from 'i18next'; -import { beforeAll, describe, expect, it } from 'vitest'; +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { ToolTimeoutIndicator } from './ToolTimeoutIndicator'; +const setSubagentTimeoutMock = vi.hoisted(() => vi.fn()); + +vi.mock('@/infrastructure/api/service-api/AgentAPI', () => ({ + agentAPI: { + setSubagentTimeout: setSubagentTimeoutMock, + }, +})); + let i18n: I18nInstance; beforeAll(async () => { @@ -22,6 +32,8 @@ beforeAll(async () => { failedDurationTooltipWithReason: 'Failed after {{duration}}: {{reason}}', cancelledDurationTooltip: 'Cancelled after {{duration}}', durationTooltip: 'Duration {{duration}}', + disableTooltip: 'Disable timeout', + disableLabel: 'Ignore timeout', }, }, }, @@ -31,15 +43,58 @@ beforeAll(async () => { }); }); -function renderIndicator(element: React.ReactElement): string { - return renderToStaticMarkup( +function withI18n(element: React.ReactElement): React.ReactElement { + return ( {element} - , + ); } +function renderIndicator(element: React.ReactElement): string { + return renderToStaticMarkup(withI18n(element)); +} + describe('ToolTimeoutIndicator', () => { + let dom: { window: Window & typeof globalThis } | null = null; + let container: HTMLDivElement | null = null; + let root: Root | null = null; + + beforeEach(async () => { + const jsdom = await import('jsdom'); + dom = new jsdom.JSDOM('', { + pretendToBeVisual: true, + url: 'http://localhost', + }) as unknown as { window: Window & typeof globalThis }; + + const { window } = dom; + vi.stubGlobal('window', window); + vi.stubGlobal('document', window.document); + vi.stubGlobal('navigator', window.navigator); + vi.stubGlobal('HTMLElement', window.HTMLElement); + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true); + + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + setSubagentTimeoutMock.mockResolvedValue(undefined); + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + } + container?.remove(); + dom?.window.close(); + vi.unstubAllGlobals(); + vi.clearAllMocks(); + root = null; + container = null; + dom = null; + }); + it('uses a success affordance for completed subagent durations', () => { const html = renderIndicator( { expect(html).toContain('Failed after 2.4s: provider timed out'); expect(html).toContain('2.4s'); }); + + it('does not render an ignore-timeout control before the subagent session is known', () => { + const html = renderIndicator( + , + ); + + expect(html).not.toContain('timeout-ignore-btn'); + expect(html).not.toContain('Ignore timeout'); + }); + + it('disables the running subagent timeout when the ignore control is clicked', async () => { + await act(async () => { + root!.render(withI18n( + , + )); + }); + + const button = container!.querySelector('.timeout-ignore-btn'); + expect(button).toBeTruthy(); + + await act(async () => { + button!.dispatchEvent(new dom!.window.MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(setSubagentTimeoutMock).toHaveBeenCalledWith('subagent-session', { type: 'disable' }); + }); }); diff --git a/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.tsx b/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.tsx index 7fb62d829..69c6f7475 100644 --- a/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.tsx +++ b/src/web-ui/src/flow_chat/tool-cards/ToolTimeoutIndicator.tsx @@ -155,6 +155,7 @@ export const ToolTimeoutIndicator: React.FC = ({ if (!isRunning) return null; const hasTimeout = Boolean(timeoutMs && timeoutMs > 0); + const canControlTimeout = showControls && hasTimeout && Boolean(subagentSessionId); const displayRemaining = isTimeoutDisabled ? null : remainingMs; // Determine warning threshold: remaining < 20% of original timeout. @@ -185,7 +186,7 @@ export const ToolTimeoutIndicator: React.FC = ({ )} - {showControls && hasTimeout && ( + {canControlTimeout && (