From 97558bd3c0def88d09506d73343ecf36c697a032 Mon Sep 17 00:00:00 2001 From: limityan Date: Sun, 26 Jul 2026 16:06:32 +0800 Subject: [PATCH 1/2] docs(review): define detail recovery states --- docs/architecture/review-lifecycle.md | 111 ++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/docs/architecture/review-lifecycle.md b/docs/architecture/review-lifecycle.md index 76e2e952e8..69802e30f6 100644 --- a/docs/architecture/review-lifecycle.md +++ b/docs/architecture/review-lifecycle.md @@ -326,6 +326,109 @@ questions and real states without Skill names, agent ids, packet ids, or budgets A child with no loaded transcript renders preparing, loading, or load-failed state rather than a title over an empty body. +### Execution detail and recovery projection + +Execution outcome and transcript availability are separate facts. A Review can +still be running while history loading fails, and a completed Review can be +visible before its transcript is hydrated. Card and detail surfaces therefore +derive one presentation from existing session and Task facts instead of keeping +a second UI-owned lifecycle. + +```mermaid +flowchart LR + Metadata["Session metadata"] --> Content["Content state"] + Live["Live session"] --> Status["Execution state"] + Turn["Last turn"] --> Status + Task["Task result"] --> Status + Content --> View["Review view"] + Status --> View + View --> Card["Card"] + View --> Detail["Detail"] + View --> Actions["Actions"] +``` + +Content state has four user-visible outcomes: + +| Source fact | Detail body | +|---|---| +| new child with no turn yet | preparing | +| metadata only, or history hydration in progress | loading | +| hydrated transcript | transcript and result | +| history hydration failed | load failed with a load-only retry | + +Reloading content never starts or continues model execution. A load failure is +not presented as a Review failure, and an empty transcript is never rendered as +an unexplained blank pane. + +Execution state is derived in this order: + +1. A live `Processing` session is running. Live state wins over the persisted + idle form while a runtime still owns the turn. +2. A structured Task result such as `partial_timeout` or `cancelled` preserves + the more precise parent-visible outcome. This matters because a child with + partial output may have a completed persisted turn even though the bounded + reviewer execution timed out. +3. The latest child turn supplies completed, error, or cancelled state. +4. A restored idle session with an unfinished latest turn is interrupted; it is + not running and is not complete. +5. Parent Task status is a compatibility fallback only when the linked child or + its persisted facts are unavailable. + +These are projection rules, not a new persisted enum or a second state machine. +They produce the following plain-language behavior: + +| Execution outcome | Presentation | Allowed action | +|---|---|---| +| active turn | running | stop | +| completed turn | completed | inspect result | +| timeout with output | timed out, partial result kept | inspect partial result | +| timeout without output | timed out | return to the owning Review | +| model or provider failure | could not complete | return to the owning Review | +| user-confirmed cancellation | stopped | inspect retained output | +| runtime lost with an unfinished turn | interrupted | return to the owning Review; continue there when available | +| incomplete legacy facts | unable to confirm | reload details; do not retry automatically | + +An individual focused check never exposes a direct rerun action. The owning +Review remains responsible for bounded retries and coverage decisions, so the +UI cannot bypass its retry budget or duplicate work. Continuation is available +only for an interrupted, nonterminal Review at the Review-session level. It +reuses that session and appends a turn without creating a second logical launch. +Retry after a terminal timeout or failure remains an explicit new revision. +Opening details, restoring a window, or restarting the application must not +resubmit the original request. + +Stopping has a confirmation boundary: + +```mermaid +stateDiagram-v2 + [*] --> Running + Running --> Stopping: user stops + Stopping --> Stopped: cancellation confirmed + Stopping --> Running: cancellation not confirmed +``` + +`Stopping` is transient UI intent. The UI settles the turn as stopped only after +the runtime accepts cancellation. If cancellation cannot be confirmed, it +reloads the authoritative state and says that stopping could not be confirmed; +it must not claim success or launch replacement work. + +Application restore follows runtime ownership rather than the last visible card: + +```mermaid +flowchart TD + Restore["Restore"] --> LiveOwner{"Runtime active?"} + LiveOwner -->|Yes| Running["Running"] + LiveOwner -->|No| LastTurn{"Turn finished?"} + LastTurn -->|Yes| Terminal["Saved outcome"] + LastTurn -->|No| Interrupted["Interrupted"] + LastTurn -->|Unknown| Unknown["Unable to confirm"] +``` + +Persisted processing state is not revived after an application restart. If a +remote or still-running host remains authoritative, its live state is shown; +otherwise an unfinished turn is interrupted and waits for explicit user intent. +Partial transcript content remains inspectable in either case. + The pull-request surface continues to use exact provider identity and verified base/head freshness. A stale record offers “Review current version” and creates another revision of that record. Cached pull-request overview data is not @@ -403,6 +506,14 @@ evidence: - launch does not force-open execution detail; - no Review state renders as an unexplained blank pane; +- execution status and transcript-loading status remain independent; +- opening or reloading detail performs no model call and creates no child turn; +- a partial timeout keeps partial output and remains visibly distinct from a + successful completion; +- user cancellation is shown as stopped only after cancellation is confirmed; +- application restart never silently revives or duplicates an unfinished turn; +- an interrupted Review can continue only through explicit Review-level intent; +- an individual focused check cannot bypass the owning Review's retry policy; - metadata-only restore shows a useful bounded summary; - stale pull-request revisions cannot be presented as current; - re-review preserves one record and creates a distinct revision; From 3bc9055abdcbf33580b3cc8f81418898104e7384 Mon Sep 17 00:00:00 2001 From: limityan Date: Sun, 26 Jul 2026 18:58:32 +0800 Subject: [PATCH 2/2] fix(review): restore focused check details safely --- docs/architecture/review-lifecycle.md | 21 + src/apps/desktop/src/api/agentic_api.rs | 44 +- src/apps/server/src/rpc_dispatcher.rs | 7 +- .../src/agentic/coordination/coordinator.rs | 25 + .../task/launch_review_agent.rs | 9 +- .../tools/implementations/task/tests.rs | 15 + src/crates/contracts/events/src/agentic.rs | 8 + .../events/src/frontend_projection.rs | 7 + .../src/deep_review/focused_assignment.rs | 257 +++++++- .../agent-runtime/src/deep_review/mod.rs | 4 +- .../tests/remote_connect_contracts.rs | 1 + .../components/panels/base/FlexiblePanel.tsx | 2 + .../TaskDetailPanel/TaskDetailPanel.tsx | 6 +- .../BtwSessionPanel.review-action.test.tsx | 604 +++++++++++++++++- .../components/btw/BtwSessionPanel.scss | 41 ++ .../components/btw/BtwSessionPanel.tsx | 208 ++++-- .../components/modern/ExportImageButton.tsx | 3 + .../components/modern/FlowChatContext.tsx | 2 + .../components/modern/ModelRoundItem.tsx | 9 +- .../modern/ModernFlowChatContainer.tsx | 13 +- .../src/flow_chat/services/FlowChatManager.ts | 16 +- .../src/flow_chat/services/btwSessionPane.ts | 59 +- .../EventHandlerModule.test.ts | 44 ++ .../flow-chat-manager/EventHandlerModule.ts | 18 +- .../flow-chat-manager/SessionModule.test.ts | 111 ++++ .../flow-chat-manager/SessionModule.ts | 101 ++- .../services/flow-chat-manager/index.ts | 1 + .../services/flow-chat-manager/types.ts | 14 + .../flow_chat/services/openBtwSession.test.ts | 118 +++- .../src/flow_chat/store/FlowChatStore.ts | 33 +- .../flow_chat/tool-cards/TaskToolDisplay.scss | 21 + .../tool-cards/TaskToolDisplay.test.tsx | 449 ++++++++++++- .../flow_chat/tool-cards/TaskToolDisplay.tsx | 199 ++++-- src/web-ui/src/flow_chat/types/flow-chat.ts | 3 + .../utils/dialogTurnStability.test.ts | 7 + .../flow_chat/utils/dialogTurnStability.ts | 17 +- .../flow_chat/utils/reviewDetailState.test.ts | 199 ++++++ .../src/flow_chat/utils/reviewDetailState.ts | 121 ++++ .../flow_chat/utils/reviewSessionStop.test.ts | 51 -- .../src/flow_chat/utils/reviewSessionStop.ts | 19 - .../src/flow_chat/utils/reviewTaskOutcome.ts | 72 +++ .../api/service-api/AgentAPI.test.ts | 27 + .../api/service-api/AgentAPI.ts | 11 +- src/web-ui/src/locales/en-US/flow-chat.json | 23 +- src/web-ui/src/locales/zh-CN/flow-chat.json | 23 +- src/web-ui/src/locales/zh-TW/flow-chat.json | 23 +- .../src/shared/services/review-team/types.ts | 4 + 47 files changed, 2816 insertions(+), 254 deletions(-) create mode 100644 src/web-ui/src/flow_chat/utils/reviewDetailState.test.ts create mode 100644 src/web-ui/src/flow_chat/utils/reviewDetailState.ts delete mode 100644 src/web-ui/src/flow_chat/utils/reviewSessionStop.test.ts delete mode 100644 src/web-ui/src/flow_chat/utils/reviewSessionStop.ts create mode 100644 src/web-ui/src/flow_chat/utils/reviewTaskOutcome.ts diff --git a/docs/architecture/review-lifecycle.md b/docs/architecture/review-lifecycle.md index 69802e30f6..4288c2b806 100644 --- a/docs/architecture/review-lifecycle.md +++ b/docs/architecture/review-lifecycle.md @@ -326,6 +326,27 @@ questions and real states without Skill names, agent ids, packet ids, or budgets A child with no loaded transcript renders preparing, loading, or load-failed state rather than a title over an empty body. +Focused-check titles cross one narrow public boundary: + +```mermaid +flowchart LR + Assignment["Focused question"] --> Admission["Runtime admission"] + Admission --> Label["Public label"] + Label --> Card["Card"] + Label --> Detail["Detail"] + Assignment -. "internal fields" .-> Execution["Execution only"] +``` + +The label is short, plain-language metadata stored in the admitted child +manifest. Cards and detail tabs read only that label; they never derive titles +from the model prompt, capability key, path scope, or other launch arguments. +Missing or unsafe labels use a generic localized title and never block the +check. Linking projects only the admitted public label; the existing manifest +is persisted for recovery. Both representations come from the same admitted +assignment, without sending internal manifest fields through the UI event. +Session reconstruction re-admits that label through the same runtime function +before restored metadata is persisted. + ### Execution detail and recovery projection Execution outcome and transcript availability are separate facts. A Review can diff --git a/src/apps/desktop/src/api/agentic_api.rs b/src/apps/desktop/src/api/agentic_api.rs index 7eb30f1cc3..c2bcfdc37b 100644 --- a/src/apps/desktop/src/api/agentic_api.rs +++ b/src/apps/desktop/src/api/agentic_api.rs @@ -14,6 +14,7 @@ use crate::runtime::{ DesktopRuntimeContext, DesktopSessionApplicationError, DesktopSessionScopeRequest, }; use crate::startup_trace::DesktopStartupTrace; +use bitfun_agent_runtime::deep_review::sanitize_focused_review_public_metadata; use bitfun_agent_runtime::sdk::{ AgentDialogTurnRequest, AgentInputAttachment, AgentSessionModelUpdateRequest, AgentSubmissionSource, AgentTurnCancellationRequest, PermissionAuditRecord, PermissionGrant, @@ -685,6 +686,19 @@ pub struct CancelSessionRequest { pub session_id: String, } +fn sanitize_create_session_review_metadata(request: &mut CreateSessionRequest) { + if let Some(manifest) = request.deep_review_run_manifest.as_mut() { + sanitize_focused_review_public_metadata(manifest); + } +} + +#[derive(Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct CancelSessionResponse { + pub cancelled: bool, + pub dialog_turn_id: Option, +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct CancelToolRequest { @@ -1184,11 +1198,12 @@ pub struct GenerateSessionTitleRequest { pub async fn create_session( coordinator: State<'_, Arc>, app_state: State<'_, AppState>, - request: CreateSessionRequest, + mut request: CreateSessionRequest, ) -> Result { fn norm_conn(s: Option) -> Option { s.map(|x| x.trim().to_string()).filter(|x| !x.is_empty()) } + sanitize_create_session_review_metadata(&mut request); let wp = request.workspace_path.clone(); let remote_conn = norm_conn(request.remote_connection_id.clone()).or_else(|| { request @@ -2159,8 +2174,8 @@ pub async fn control_deep_review_queue( pub async fn cancel_session( coordinator: State<'_, Arc>, request: CancelSessionRequest, -) -> Result<(), String> { - coordinator +) -> Result { + let dialog_turn_id = coordinator .cancel_active_turn_for_session(&request.session_id, std::time::Duration::from_secs(5)) .await .map_err(|e| { @@ -2172,7 +2187,10 @@ pub async fn cancel_session( format!("Failed to cancel session: {}", e) })?; - Ok(()) + Ok(CancelSessionResponse { + cancelled: dialog_turn_id.is_some(), + dialog_turn_id, + }) } #[derive(Debug, Deserialize)] @@ -3044,6 +3062,24 @@ mod tests { } } + #[test] + fn create_session_recovery_sanitizes_focused_review_public_metadata() { + let mut request = idempotent_create_request(); + request.deep_review_run_manifest = Some(json!({ + "reviewMode": "deep", + "focusedAssignment": { + "displayLabel": "Review Worker packet 7", + "question": "Could this contract break callers?" + } + })); + + sanitize_create_session_review_metadata(&mut request); + + let assignment = &request.deep_review_run_manifest.as_ref().unwrap()["focusedAssignment"]; + assert!(assignment.get("displayLabel").is_none()); + assert_eq!(assignment["question"], "Could this contract break callers?"); + } + #[test] fn existing_create_session_retry_returns_the_matching_session() { let request = idempotent_create_request(); diff --git a/src/apps/server/src/rpc_dispatcher.rs b/src/apps/server/src/rpc_dispatcher.rs index 08875a1bf8..2335103fd9 100644 --- a/src/apps/server/src/rpc_dispatcher.rs +++ b/src/apps/server/src/rpc_dispatcher.rs @@ -474,12 +474,15 @@ pub async fn dispatch( "cancel_session" => { let request = extract_request(¶ms)?; let session_id = get_string(&request, "sessionId")?; - state + let dialog_turn_id = state .coordinator .cancel_active_turn_for_session(&session_id, Duration::from_secs(5)) .await .map_err(|e| anyhow!("{}", e))?; - Ok(serde_json::Value::Null) + Ok(serde_json::json!({ + "cancelled": dialog_turn_id.is_some(), + "dialogTurnId": dialog_turn_id, + })) } "get_session_messages" => { let request = params.get("request").unwrap_or(¶ms); diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index 018e56bd4c..b0c235a07f 100644 --- a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs +++ b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs @@ -65,6 +65,7 @@ use crate::service::workspace::{ }; use crate::service_agent_runtime::CoreServiceAgentRuntime; use crate::util::errors::{BitFunError, BitFunResult}; +use bitfun_agent_runtime::deep_review::FocusedReviewAssignment; use bitfun_agent_runtime::output_surface::{ supports_inline_markdown_images_for_source, TOOL_CONTEXT_INLINE_MARKDOWN_IMAGE_DISPLAY_KEY, }; @@ -5432,6 +5433,17 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet external_generation_lease: _external_generation_lease, } = request; let prepared_target_session_id = target_session_id.clone(); + let deep_review_run_manifest = context + .get("deep_review_run_manifest") + .and_then(|raw| serde_json::from_str::(raw).ok()); + let focused_review_display_label = deep_review_run_manifest + .as_ref() + .and_then(|manifest| { + FocusedReviewAssignment::from_manifest(manifest) + .ok() + .flatten() + }) + .and_then(|assignment| assignment.display_label().map(str::to_string)); let continuation_policy = session_config.continuation_policy; let requested_timeout_seconds = timeout_seconds.filter(|seconds| *seconds > 0); @@ -5649,6 +5661,18 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet .await; return Err(error); } + if let Some(manifest) = deep_review_run_manifest.as_ref() { + if let Err(error) = self + .session_manager + .set_session_deep_review_run_manifest(&session_id, Some(manifest.clone())) + .await + { + warn!( + "Failed to persist Review manifest for linked subagent session: session_id={}, error={}", + session_id, error + ); + } + } if let Some(source_session_id) = prompt_cache_source_session_id.as_deref() { self.session_manager .seed_forked_edit_constraints(source_session_id, &session_id) @@ -5761,6 +5785,7 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet .session_manager .get_session(&session_id) .and_then(|session| session.config.model_id.clone()), + focused_review_display_label: focused_review_display_label.clone(), }) .await; } diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs b/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs index edabc3f819..6c8988037f 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs @@ -65,8 +65,13 @@ impl LaunchReviewAgentTool { }, "focused_assignment": { "type": "object", - "description": "A target-bound question for ReviewWorker. Required for adaptive non-packet checks; managed packets may attach a question without repeating their packet file scope.", + "description": "A target-bound question for ReviewWorker. Required for adaptive non-packet checks; managed packets may attach a question without repeating their packet file scope. A safe display_label may be shown after runtime validation.", "properties": { + "display_label": { + "type": "string", + "maxLength": 80, + "description": "Optional short plain-language label for the concern, using at most eight words. Do not include internal coordination values such as agent or skill names, packet IDs, file paths, or model IDs. Unsafe labels are ignored and never block the check." + }, "question": { "type": "string" }, "independent_value": { "type": "string" }, "target_fingerprint": { "type": "string" }, @@ -210,7 +215,7 @@ Built-in review agent types: - `ReviewWorker`: one read-only worker whose bounded prompt supplies the dynamic review lens, concrete question, file or packet scope, and expected evidence. It may cover a narrow specialist uncertainty or a managed file packet, but must not widen its assignment. - `ReviewJudge`: final quality-inspector pass after reviewer outputs are available. -The capability catalog below contains short descriptions only. For an adaptive ReviewWorker call, copy the selected key and fingerprint into `focused_assignment`; full guidance is loaded only after runtime admission. Outside a manifest-declared work-packet plan, do not split files, launch routine parallel coverage, or repeat the primary review. +The capability catalog below contains short descriptions only. For an adaptive ReviewWorker call, copy the selected key and fingerprint into `focused_assignment`; full guidance is loaded only after runtime admission. When possible, give each focused assignment a short plain-language `display_label` that describes the user-visible concern without internal coordination values such as agent or skill names, packet IDs, file paths, or model IDs. The runtime ignores an unsafe label without blocking the check. Outside a manifest-declared work-packet plan, do not split files, launch routine parallel coverage, or repeat the primary review. For a managed packet, pass its exact manifest `packet_id` in the top-level `packet_id` field. Runtime rejects missing or unknown managed packet ids. diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/task/tests.rs b/src/crates/assembly/core/src/agentic/tools/implementations/task/tests.rs index 19c55ad18e..ba45632c5a 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/task/tests.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/task/tests.rs @@ -285,6 +285,21 @@ async fn launch_review_agent_schema_exposes_retry_without_agent_or_fork_controls assert_eq!(schema["properties"]["retry_coverage"]["type"], "object"); assert_eq!(schema["properties"]["packet_id"]["type"], "string"); assert_eq!(schema["properties"]["focused_assignment"]["type"], "object"); + assert_eq!( + schema["properties"]["focused_assignment"]["properties"]["display_label"]["type"], + "string" + ); + let display_label_description = schema["properties"]["focused_assignment"]["properties"] + ["display_label"]["description"] + .as_str() + .expect("display_label description should be a string"); + assert!(display_label_description.contains("file paths")); + assert!(!display_label_description.contains("packet, path, model")); + assert!(!schema["properties"]["focused_assignment"]["required"] + .as_array() + .unwrap() + .iter() + .any(|value| value.as_str() == Some("display_label"))); assert!(schema["properties"].get("fork_context").is_none()); assert!(schema["properties"].get("agent_id").is_none()); assert!(schema["properties"].get("run_in_background").is_none()); diff --git a/src/crates/contracts/events/src/agentic.rs b/src/crates/contracts/events/src/agentic.rs index ba4392d1de..184658b1e5 100644 --- a/src/crates/contracts/events/src/agentic.rs +++ b/src/crates/contracts/events/src/agentic.rs @@ -136,6 +136,9 @@ pub enum AgenticEvent { /// Resolved model selector stored on the child session. #[serde(skip_serializing_if = "Option::is_none")] model_id: Option, + /// Runtime-admitted public label for a focused Review child. + #[serde(skip_serializing_if = "Option::is_none")] + focused_review_display_label: Option, }, DialogTurnCompleted { @@ -930,6 +933,7 @@ mod tests { parent_tool_call_id: "tool-1".to_string(), agent_type: Some("GeneralPurpose".to_string()), model_id: Some("fast".to_string()), + focused_review_display_label: Some("Authentication boundary".to_string()), }; assert_eq!(event.session_id(), Some("child-session")); @@ -944,5 +948,9 @@ mod tests { assert_eq!(serialized["parent_tool_call_id"], "tool-1"); assert_eq!(serialized["agent_type"], "GeneralPurpose"); assert_eq!(serialized["model_id"], "fast"); + assert_eq!( + serialized["focused_review_display_label"], + "Authentication boundary" + ); } } diff --git a/src/crates/contracts/events/src/frontend_projection.rs b/src/crates/contracts/events/src/frontend_projection.rs index 6bdedf7a95..dd95254f53 100644 --- a/src/crates/contracts/events/src/frontend_projection.rs +++ b/src/crates/contracts/events/src/frontend_projection.rs @@ -98,6 +98,7 @@ pub fn project_agentic_frontend_event(event: AgenticEvent) -> Option Some(AgenticFrontendEvent::new( "agentic://subagent-session-linked", json!({ @@ -108,6 +109,7 @@ pub fn project_agentic_frontend_event(event: AgenticEvent) -> Option, question: String, independent_value: String, target_fingerprint: String, @@ -61,6 +67,7 @@ impl FocusedReviewAssignment { let expected_evidence = bounded_string(object.get("expected_evidence"), "expected_evidence")?; let capability_key = bounded_string(object.get("capability_key"), "capability_key")?; + let display_label = parse_public_display_label(object.get("display_label")); let capability_fingerprint = bounded_string( object.get("capability_fingerprint"), "capability_fingerprint", @@ -108,6 +115,7 @@ impl FocusedReviewAssignment { let question_id = derive_question_id(&question, &target_fingerprint); Ok(Self { question_id, + display_label, question, independent_value, target_fingerprint, @@ -125,12 +133,16 @@ impl FocusedReviewAssignment { else { return Ok(None); }; - let assignment = serde_json::from_value::(raw.clone()).map_err(|_| { + let mut assignment = serde_json::from_value::(raw.clone()).map_err(|_| { violation( "focused_review_assignment_invalid", "focusedAssignment is malformed", ) })?; + assignment.display_label = assignment + .display_label + .as_deref() + .and_then(sanitize_public_display_label); let evidence = ReviewTargetEvidence::from_manifest(manifest) .map_err(|error| violation("focused_review_target_invalid", error.to_string()))? .ok_or_else(|| { @@ -163,6 +175,10 @@ impl FocusedReviewAssignment { &self.question_id } + pub fn display_label(&self) -> Option<&str> { + self.display_label.as_deref() + } + pub fn question(&self) -> &str { &self.question } @@ -264,6 +280,118 @@ fn bounded_string( }) } +fn parse_public_display_label(raw: Option<&Value>) -> Option { + raw.and_then(Value::as_str) + .and_then(sanitize_public_display_label) +} + +fn sanitize_public_display_label(value: &str) -> Option { + let normalized = value.split_whitespace().collect::>().join(" "); + let words = normalized.split_whitespace().collect::>(); + let is_plain_label = (2..=PUBLIC_DISPLAY_LABEL_CHAR_LIMIT) + .contains(&normalized.chars().count()) + && !words.is_empty() + && words.len() <= PUBLIC_DISPLAY_LABEL_WORD_LIMIT + && normalized.chars().all(|character| { + character.is_alphanumeric() + || character.is_whitespace() + || matches!( + character, + '-' | '.' | ',' | '\'' | '’' | '&' | '+' | '(' | ')' + ) + }); + let terms = normalized + .split(|character: char| !character.is_alphanumeric()) + .filter(|term| !term.is_empty()) + .collect::>(); + let contains_internal_term = terms.iter().any(|term| { + INTERNAL_DISPLAY_LABEL_TERMS + .iter() + .any(|reserved| term.eq_ignore_ascii_case(reserved)) + }) || terms.windows(2).any(|window| { + (window[0].eq_ignore_ascii_case("review") + && (window[1].eq_ignore_ascii_case("worker") + || window[1].eq_ignore_ascii_case("judge"))) + || (window[0].eq_ignore_ascii_case("packet") + && window[1] + .chars() + .all(|character| character.is_ascii_digit())) + }) || terms.windows(3).any(|window| { + window[0].eq_ignore_ascii_case("launch") + && window[1].eq_ignore_ascii_case("review") + && window[2].eq_ignore_ascii_case("agent") + }); + let contains_structured_identifier = terms.iter().any(|term| { + let mut previous_was_lowercase = false; + let mut lowercase_to_uppercase_transitions = 0; + for character in term.chars() { + if previous_was_lowercase && character.is_ascii_uppercase() { + lowercase_to_uppercase_transitions += 1; + } + previous_was_lowercase = character.is_ascii_lowercase(); + } + let long_hex = + term.len() >= 16 && term.chars().all(|character| character.is_ascii_hexdigit()); + lowercase_to_uppercase_transitions >= 2 || long_hex + }) || contains_uuid(&normalized); + + (is_plain_label + && !terms.is_empty() + && !contains_internal_term + && !contains_structured_identifier) + .then_some(normalized) +} + +/// Re-admits the only focused-check field that crosses from a persisted run +/// manifest into product UI. Other execution metadata is preserved verbatim. +pub fn sanitize_focused_review_public_metadata(manifest: &mut Value) { + let assignment_key = if manifest.get("focusedAssignment").is_some() { + "focusedAssignment" + } else { + "focused_assignment" + }; + let Some(assignment) = manifest + .get_mut(assignment_key) + .and_then(Value::as_object_mut) + else { + return; + }; + + for key in ["displayLabel", "display_label"] { + if !assignment.contains_key(key) { + continue; + } + let admitted = assignment + .get(key) + .and_then(Value::as_str) + .and_then(sanitize_public_display_label); + if let Some(label) = admitted { + assignment.insert(key.to_string(), Value::String(label)); + } else { + assignment.remove(key); + } + } +} + +fn contains_uuid(value: &str) -> bool { + value + .split(|character: char| !(character.is_ascii_hexdigit() || character == '-')) + .filter(|token| !token.is_empty()) + .any(|token| { + let segments = token.split('-').collect::>(); + segments.len() == 5 + && segments + .iter() + .zip([8, 4, 4, 4, 12]) + .all(|(segment, expected_len)| { + segment.len() == expected_len + && segment + .chars() + .all(|character| character.is_ascii_hexdigit()) + }) + }) +} + fn parse_path_array(raw: &Value) -> Result, DeepReviewPolicyViolation> { let paths = raw .as_array() @@ -389,6 +517,7 @@ mod tests { let assignment = FocusedReviewAssignment::from_input( &manifest(), &json!({ + "display_label": "Rename discovery boundary", "question": "Could the rename break module discovery?", "independent_value": "The primary review found an unresolved rename boundary.", "target_fingerprint": "target-12345678", @@ -401,6 +530,14 @@ mod tests { ) .expect("assignment should be valid"); + assert_eq!( + assignment.display_label(), + Some("Rename discovery boundary") + ); + assert_eq!( + assignment.to_value()["displayLabel"], + "Rename discovery boundary" + ); assert_eq!(assignment.allowed_changed_paths(), &["src/new.rs"]); let evidence = ReviewTargetEvidence::from_manifest(&manifest()) .expect("evidence should parse") @@ -424,6 +561,7 @@ mod tests { let error = FocusedReviewAssignment::from_input( &manifest(), &json!({ + "display_label": "Missing path boundary", "question": "Is this safe?", "independent_value": "The primary review needs independent evidence.", "target_fingerprint": "target-12345678", @@ -449,6 +587,7 @@ mod tests { let assignment = FocusedReviewAssignment::from_input( &manifest, &json!({ + "display_label": "Exact path handling", "question": "Is this path handled exactly?", "independent_value": "The path boundary needs independent evidence.", "target_fingerprint": "target-12345678", @@ -474,6 +613,7 @@ mod tests { let assignment = FocusedReviewAssignment::from_input( &manifest, &json!({ + "display_label": "Renamed caller boundary", "question": "Could the renamed implementation break callers?", "independent_value": "The renamed implementation needs isolated evidence.", "target_fingerprint": "target-12345678", @@ -499,6 +639,7 @@ mod tests { fn question_identity_is_stable_across_disjoint_target_scopes() { let input_for = |path: &str, capability: &str| { json!({ + "display_label": "Caller contract boundary", "question": "Could this contract break callers?", "independent_value": "The same question needs evidence from disjoint packets.", "target_fingerprint": "target-12345678", @@ -523,4 +664,118 @@ mod tests { assert_eq!(first.question_id(), second.question_id()); } + + #[test] + fn assignment_drops_internal_coordination_from_the_optional_display_label() { + for display_label in [ + "ReviewWorker packet-7", + "Review Worker boundary", + "Review-Judge result", + "Launch Review Agent check", + "Focused packet-7 boundary", + "CodeReviewTesting", + "abcdef0123456789", + "Check 550e8400-e29b-41d4-a716-446655440000", + "Check g550e8400-e29b-41d4-a716-446655440000z", + "--", + ] { + let assignment = FocusedReviewAssignment::from_input( + &manifest(), + &json!({ + "display_label": display_label, + "question": "Could this contract break callers?", + "independent_value": "The primary review needs independent evidence.", + "target_fingerprint": "target-12345678", + "allowed_changed_paths": ["src/new.rs"], + "expected_evidence": "A concrete call path.", + "capability_key": "builtin::review-worker", + "capability_fingerprint": "capability-12345678" + }), + None, + ) + .expect("an optional display label must not block the focused check"); + + assert_eq!(assignment.display_label(), None, "label: {display_label}"); + } + } + + #[test] + fn assignment_accepts_plain_domain_language_in_the_display_label() { + for display_label in [ + "Cross-workspace OAuth 2.0 boundary", + "OAuth token refresh", + "Path traversal boundary", + "Data model migration", + "Testing concern", + "iOS authentication", + "GitHub OAuth", + "OpenAI client timeout", + "GraphQL schema", + ] { + let assignment = FocusedReviewAssignment::from_input( + &manifest(), + &json!({ + "display_label": display_label, + "question": "Could this contract break callers?", + "independent_value": "The primary review needs independent evidence.", + "target_fingerprint": "target-12345678", + "allowed_changed_paths": ["src/new.rs"], + "expected_evidence": "A concrete call path.", + "capability_key": "skill:project::custom::code-review-testing", + "capability_fingerprint": "capability-12345678" + }), + None, + ) + .expect("plain domain language should be accepted"); + + assert_eq!(assignment.display_label(), Some(display_label)); + } + } + + #[test] + fn restored_assignment_drops_an_unsafe_persisted_display_label() { + let mut child_manifest = manifest(); + let assignment = FocusedReviewAssignment::from_input( + &child_manifest, + &json!({ + "display_label": "Authentication boundary", + "question": "Could this contract break callers?", + "independent_value": "The primary review needs independent evidence.", + "target_fingerprint": "target-12345678", + "allowed_changed_paths": ["src/new.rs"], + "expected_evidence": "A concrete call path.", + "capability_key": "builtin::review-worker", + "capability_fingerprint": "capability-12345678" + }), + None, + ) + .expect("assignment should be valid"); + child_manifest["focusedAssignment"] = assignment.to_value(); + child_manifest["focusedAssignment"]["displayLabel"] = json!("ReviewWorker packet 7"); + + let restored = FocusedReviewAssignment::from_manifest(&child_manifest) + .expect("restored assignment should remain usable") + .expect("focused assignment should exist"); + + assert_eq!(restored.display_label(), None); + } + + #[test] + fn public_metadata_sanitizer_preserves_manifest_and_drops_only_an_unsafe_label() { + let mut manifest = json!({ + "reviewMode": "deep", + "focusedAssignment": { + "displayLabel": "Review Worker packet 7", + "question": "Could this contract break callers?" + } + }); + + sanitize_focused_review_public_metadata(&mut manifest); + + assert!(manifest["focusedAssignment"].get("displayLabel").is_none()); + assert_eq!( + manifest["focusedAssignment"]["question"], + "Could this contract break callers?" + ); + } } diff --git a/src/crates/execution/agent-runtime/src/deep_review/mod.rs b/src/crates/execution/agent-runtime/src/deep_review/mod.rs index 199c0c2cb9..8ea6099d5f 100644 --- a/src/crates/execution/agent-runtime/src/deep_review/mod.rs +++ b/src/crates/execution/agent-runtime/src/deep_review/mod.rs @@ -39,8 +39,8 @@ pub use execution_policy::{ DeepReviewStrategyLevel, DeepReviewSubagentRole, }; pub use focused_assignment::{ - adaptive_review_max_focused_calls, is_adaptive_review_manifest, FocusedReviewAssignment, - FocusedReviewPathAccess, + adaptive_review_max_focused_calls, is_adaptive_review_manifest, + sanitize_focused_review_public_metadata, FocusedReviewAssignment, FocusedReviewPathAccess, }; pub use incremental_cache::DeepReviewIncrementalCache; pub use manifest::DeepReviewRunManifestGate; diff --git a/src/crates/services/services-integrations/tests/remote_connect_contracts.rs b/src/crates/services/services-integrations/tests/remote_connect_contracts.rs index 65bc878758..28dfca9036 100644 --- a/src/crates/services/services-integrations/tests/remote_connect_contracts.rs +++ b/src/crates/services/services-integrations/tests/remote_connect_contracts.rs @@ -2308,6 +2308,7 @@ fn remote_connect_tracker_keeps_subagent_items_out_of_parent_accumulators() { parent_tool_call_id: "task-1".to_string(), agent_type: None, model_id: None, + focused_review_display_label: None, }); tracker.handle_agentic_event(&AgenticEvent::TextChunk { session_id: "child-session".to_string(), diff --git a/src/web-ui/src/app/components/panels/base/FlexiblePanel.tsx b/src/web-ui/src/app/components/panels/base/FlexiblePanel.tsx index 229da7ba44..9855fb3af6 100644 --- a/src/web-ui/src/app/components/panels/base/FlexiblePanel.tsx +++ b/src/web-ui/src/app/components/panels/base/FlexiblePanel.tsx @@ -792,6 +792,8 @@ const FlexiblePanel: React.FC = memo(({ childSessionId={content.data?.childSessionId} parentSessionId={content.data?.parentSessionId} workspacePath={content.data?.workspacePath || workspacePath} + viewKind={content.data?.viewKind} + displayTitle={content.data?.displayTitle} /> ); diff --git a/src/web-ui/src/flow_chat/components/TaskDetailPanel/TaskDetailPanel.tsx b/src/web-ui/src/flow_chat/components/TaskDetailPanel/TaskDetailPanel.tsx index 60b980b431..6143f9ca93 100644 --- a/src/web-ui/src/flow_chat/components/TaskDetailPanel/TaskDetailPanel.tsx +++ b/src/web-ui/src/flow_chat/components/TaskDetailPanel/TaskDetailPanel.tsx @@ -462,7 +462,11 @@ export const TaskDetailPanel: React.FC = ({ data }) => { setStopError(null); try { - await agentAPI.cancelSession(subagentSessionId); + const result = await agentAPI.cancelSession(subagentSessionId); + if (!result.cancelled) { + setStopError(t('toolCards.taskDetailPanel.stopSubagentFailed')); + setStoppingSubagent(false); + } } catch (error) { const message = error instanceof Error ? error.message 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 05457f7500..51f3ea71c6 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 @@ -8,6 +8,13 @@ import { useReviewActionBarStore } from '../../store/deepReviewActionBarStore'; import { loadPersistedReviewState } from '../../services/ReviewActionBarPersistenceService'; import type { FlowChatState, Session } from '../../types/flow-chat'; +const panelMocks = vi.hoisted(() => ({ + cancelSession: vi.fn(), + hydrateSessionHistoryForDetail: vi.fn(), + notificationError: vi.fn(), + virtualItems: [] as unknown[], +})); + let flowChatState: FlowChatState; const translate = (_key: string, options?: Record & { defaultValue?: string }) => ( options?.defaultValue ?? _key @@ -23,9 +30,19 @@ vi.mock('react-i18next', () => ({ }), })); -vi.mock('../modern/VirtualItemRenderer', () => ({ - VirtualItemRenderer: () =>
, -})); +vi.mock('../modern/VirtualItemRenderer', async () => { + const ReactModule = await import('react'); + const { useFlowChatContext } = await import('../modern/FlowChatContext'); + return { + VirtualItemRenderer: () => { + const { allowTranscriptExport } = useFlowChatContext(); + return ReactModule.createElement('div', { + 'data-testid': 'virtual-item-renderer', + 'data-allow-transcript-export': String(allowTranscriptExport), + }); + }, + }; +}); vi.mock('../modern/ProcessingIndicator', () => ({ ProcessingIndicator: () =>
, @@ -58,11 +75,26 @@ vi.mock('@/component-library', () => ({ IconButton: ({ children, onClick, + disabled, + className, + 'data-testid': testId, + 'aria-label': ariaLabel, }: { children: React.ReactNode; onClick?: () => void; + disabled?: boolean; + className?: string; + 'data-testid'?: string; + 'aria-label'?: string; }) => ( - ), @@ -80,7 +112,7 @@ vi.mock('@/shared/utils/tabUtils', () => ({ vi.mock('@/infrastructure/api', () => ({ agentAPI: { - cancelSession: vi.fn(), + cancelSession: (...args: unknown[]) => panelMocks.cancelSession(...args), }, })); @@ -92,7 +124,7 @@ vi.mock('@/infrastructure/event-bus', () => ({ vi.mock('@/shared/notification-system', () => ({ notificationService: { - error: vi.fn(), + error: (...args: unknown[]) => panelMocks.notificationError(...args), }, })); @@ -110,22 +142,23 @@ vi.mock('../../store/FlowChatStore', () => ({ getInstance: () => ({ getState: () => flowChatState, subscribe: () => () => {}, - loadSessionHistory: vi.fn(), }), }, flowChatStore: { getState: () => flowChatState, subscribe: () => () => {}, - loadSessionHistory: vi.fn(), }, })); -vi.mock('../../store/modernFlowChatStore', () => ({ - sessionToVirtualItems: () => [], +vi.mock('../../services/FlowChatManager', () => ({ + flowChatManager: { + hydrateSessionHistoryForDetail: (...args: unknown[]) => + panelMocks.hydrateSessionHistoryForDetail(...args), + }, })); -vi.mock('../../utils/reviewSessionStop', () => ({ - settleStoppedReviewSessionState: vi.fn(), +vi.mock('../../store/modernFlowChatStore', () => ({ + sessionToVirtualItems: () => panelMocks.virtualItems, })); vi.mock('../../services/ReviewActionBarPersistenceService', () => ({ @@ -184,6 +217,24 @@ function createReviewSession(): Session { } as Session; } +function createEmptyReviewCheckSession(overrides: Partial = {}): Session { + return { + sessionId: 'review-check-child', + title: 'Internal reviewer title', + dialogTurns: [], + status: 'idle', + config: {}, + createdAt: 1, + lastActiveAt: 1, + error: null, + sessionKind: 'subagent', + parentSessionId: 'parent-session', + workspacePath: 'D:/workspace/project', + historyState: 'ready', + ...overrides, + } as Session; +} + function createCompletedDeepReviewWithoutResult(): Session { const childSession = createReviewSession(); return { @@ -402,6 +453,11 @@ describe('BtwSessionPanel review action bar integration', () => { beforeEach(() => { (globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; useReviewActionBarStore.getState().reset(); + panelMocks.cancelSession.mockReset(); + panelMocks.hydrateSessionHistoryForDetail.mockReset(); + panelMocks.hydrateSessionHistoryForDetail.mockResolvedValue(undefined); + panelMocks.notificationError.mockReset(); + panelMocks.virtualItems = []; container = document.createElement('div'); document.body.appendChild(container); root = createRoot(container); @@ -437,6 +493,530 @@ describe('BtwSessionPanel review action bar integration', () => { useReviewActionBarStore.getState().reset(); }); + it('shows a Review-check loading state instead of an empty thread', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + isHistorical: true, + historyState: 'metadata-only', + })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('toolCards.taskTool.reviewCoverageLabel'); + expect(container.textContent).toContain('childSession.reviewDetail.loading'); + expect(container.textContent).not.toContain('session.empty'); + }); + + it('does not start a second Review-check history load while hydration is in progress', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + isHistorical: true, + historyState: 'hydrating', + })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(panelMocks.hydrateSessionHistoryForDetail).not.toHaveBeenCalled(); + expect(container.textContent).toContain('childSession.reviewDetail.loading'); + }); + + it('offers a load-only retry after Review-check history hydration fails', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + isHistorical: true, + historyState: 'failed', + })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + panelMocks.hydrateSessionHistoryForDetail.mockClear(); + + const retryButton = Array.from(container.querySelectorAll('button')) + .find((button) => button.textContent?.includes('childSession.reviewDetail.retryLoad')); + expect(retryButton).toBeTruthy(); + + await act(async () => { + retryButton!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); + + expect(panelMocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith('review-check-child'); + expect(panelMocks.cancelSession).not.toHaveBeenCalled(); + }); + + it('uses the parent workspace when retrying a legacy child without saved location', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + workspacePath: undefined, + isHistorical: true, + historyState: 'failed', + })], + ['parent-session', { + ...flowChatState.sessions.get('parent-session')!, + workspacePath: 'D:/workspace/parent', + remoteConnectionId: 'remote-current', + remoteSshHost: 'host-current', + }], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + const retryButton = Array.from(container.querySelectorAll('button')) + .find((button) => button.textContent?.includes('childSession.reviewDetail.retryLoad')); + expect(retryButton).toBeTruthy(); + + await act(async () => { + retryButton!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); + + expect(panelMocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith( + 'review-check-child', + { + workspacePath: 'D:/workspace/parent', + remoteConnectionId: 'remote-current', + remoteSshHost: 'host-current', + }, + ); + }); + + it('offers a load-only retry when legacy Review-check details are unavailable', async () => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ historyState: 'ready' })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('childSession.reviewDetail.unavailable'); + expect(container.textContent).toContain('childSession.reviewDetail.retryLoad'); + expect(panelMocks.hydrateSessionHistoryForDetail).not.toHaveBeenCalled(); + }); + + it('uses the parent Task partial-timeout outcome for empty Review-check details', async () => { + const reviewCheck = createEmptyReviewCheckSession({ + parentToolCallId: 'review-task-call', + dialogTurns: [{ + id: 'turn-completed', + sessionId: 'review-check-child', + userMessage: { id: 'user-completed', content: 'internal launch prompt', timestamp: 1 }, + modelRounds: [], + status: 'completed', + startTime: 1, + }], + }); + const parentSession = { + ...flowChatState.sessions.get('parent-session')!, + dialogTurns: [{ + id: 'parent-turn', + sessionId: 'parent-session', + userMessage: { id: 'parent-user', content: 'review', timestamp: 1 }, + modelRounds: [{ + id: 'parent-round', + index: 0, + isStreaming: false, + isComplete: true, + status: 'completed', + startTime: 1, + items: [{ + id: 'review-task', + type: 'tool', + timestamp: 1, + status: 'completed', + toolName: 'LaunchReviewAgent', + toolCall: { id: 'review-task-call', input: {} }, + toolResult: { + success: true, + result: { status: 'partial_timeout', partial_output: 'private details' }, + }, + }], + }], + status: 'completed', + startTime: 1, + }], + } as Session; + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', reviewCheck], + ['parent-session', parentSession], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('childSession.reviewDetail.partialTimedOut'); + expect(container.textContent).toContain('Checking a specific concern'); + expect(container.textContent).not.toContain('Internal reviewer title'); + expect(container.querySelector('[data-testid="btw-session-panel-origin-button"]')).toBeTruthy(); + }); + + it('disables transcript export actions for the filtered Review-check projection', async () => { + panelMocks.virtualItems = [{ type: 'model-round', turnId: 'turn-1' }]; + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + dialogTurns: [{ + id: 'turn-1', + sessionId: 'review-check-child', + userMessage: { id: 'internal-user', content: 'internal launch prompt', timestamp: 1 }, + modelRounds: [], + status: 'completed', + startTime: 1, + }], + })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.querySelector('[data-testid="virtual-item-renderer"]') + ?.getAttribute('data-allow-transcript-export')).toBe('false'); + }); + + it.each([ + { + name: 'stopped', + session: createEmptyReviewCheckSession({ + dialogTurns: [{ + id: 'turn-stopped', + sessionId: 'review-check-child', + userMessage: { id: 'user-stopped', content: 'review', timestamp: 1 }, + modelRounds: [], + status: 'cancelled', + startTime: 1, + }], + }), + expectedKey: 'childSession.reviewDetail.stopped', + }, + { + name: 'interrupted', + session: createEmptyReviewCheckSession({ + dialogTurns: [{ + id: 'turn-interrupted', + sessionId: 'review-check-child', + userMessage: { id: 'user-interrupted', content: 'review', timestamp: 1 }, + modelRounds: [{ + id: 'round-interrupted', + index: 0, + isStreaming: false, + isComplete: true, + status: 'cancelled', + startTime: 1, + items: [{ + id: 'tool-interrupted', + type: 'tool', + toolName: 'read_file', + timestamp: 1, + status: 'cancelled', + interruptionReason: 'app_restart', + }], + }], + status: 'cancelled', + startTime: 1, + }], + }), + expectedKey: 'childSession.reviewDetail.interrupted', + }, + { + name: 'timed out', + session: createEmptyReviewCheckSession({ + dialogTurns: [{ + id: 'turn-timeout', + sessionId: 'review-check-child', + userMessage: { id: 'user-timeout', content: 'review', timestamp: 1 }, + modelRounds: [], + status: 'error', + startTime: 1, + errorDetail: { category: 'timeout', rawMessage: 'private timeout detail' }, + }], + }), + expectedKey: 'childSession.reviewDetail.timedOut', + }, + { + name: 'model access failed', + session: createEmptyReviewCheckSession({ + dialogTurns: [{ + id: 'turn-failed', + sessionId: 'review-check-child', + userMessage: { id: 'user-failed', content: 'review', timestamp: 1 }, + modelRounds: [], + status: 'error', + startTime: 1, + error: 'private provider error', + }], + }), + expectedKey: 'childSession.reviewDetail.failed', + }, + ])('shows a safe empty state when a Review check is $name', async ({ session, expectedKey }) => { + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', session], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain(expectedKey); + expect(container.textContent).not.toContain('private provider error'); + expect(container.textContent).not.toContain('private timeout detail'); + expect(container.textContent).not.toContain('session.empty'); + }); + + it('refreshes a stopped Review check only after cancellation is confirmed', async () => { + let resolveCancel!: (result: { cancelled: boolean; dialogTurnId?: string }) => void; + const cancelRequest = new Promise<{ cancelled: boolean; dialogTurnId?: string }>((resolve) => { + resolveCancel = resolve; + }); + panelMocks.cancelSession.mockReturnValueOnce(cancelRequest); + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + status: 'running', + dialogTurns: [{ + id: 'turn-running', + sessionId: 'review-check-child', + userMessage: { id: 'user-running', content: 'review', timestamp: 1 }, + modelRounds: [], + status: 'processing', + startTime: 1, + }], + })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + const stopButton = container.querySelector( + '[data-testid="btw-session-panel-stop-review"]', + ); + expect(stopButton).toBeTruthy(); + expect(stopButton?.getAttribute('aria-label')).toBe('toolCards.taskDetailPanel.stopReviewWork'); + + await act(async () => { + stopButton!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(panelMocks.cancelSession).toHaveBeenCalledWith('review-check-child'); + expect(panelMocks.hydrateSessionHistoryForDetail).not.toHaveBeenCalled(); + expect(container.querySelector('[data-testid="btw-session-panel-stop-spinner"]')).toBeTruthy(); + + await act(async () => { + resolveCancel({ cancelled: true, dialogTurnId: 'turn-running' }); + await cancelRequest; + }); + + expect(panelMocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith('review-check-child'); + }); + + it('keeps a Review check running locally when cancellation cannot be confirmed', async () => { + panelMocks.cancelSession.mockResolvedValueOnce({ cancelled: false, dialogTurnId: null }); + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', createEmptyReviewCheckSession({ + status: 'running', + dialogTurns: [{ + id: 'turn-running', + sessionId: 'review-check-child', + userMessage: { id: 'user-running', content: 'review', timestamp: 1 }, + modelRounds: [], + status: 'processing', + startTime: 1, + }], + })], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + + await act(async () => { + root.render( + , + ); + }); + + const stopButton = container.querySelector( + '[data-testid="btw-session-panel-stop-review"]', + ); + expect(stopButton).toBeTruthy(); + + await act(async () => { + stopButton!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(panelMocks.notificationError).toHaveBeenCalledWith( + 'toolCards.taskDetailPanel.stopReviewWorkFailed', + ); + expect(panelMocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith('review-check-child'); + }); + + it('does not report a stop failure when the check completed before cancellation arrived', async () => { + panelMocks.cancelSession.mockResolvedValueOnce({ cancelled: false, dialogTurnId: null }); + const runningChild = createEmptyReviewCheckSession({ + status: 'running', + dialogTurns: [{ + id: 'turn-running', + sessionId: 'review-check-child', + userMessage: { id: 'user-running', content: 'review', timestamp: 1 }, + modelRounds: [], + status: 'processing', + startTime: 1, + }], + }); + flowChatState = { + ...flowChatState, + sessions: new Map([ + ['review-check-child', runningChild], + ['parent-session', flowChatState.sessions.get('parent-session')!], + ]), + } as FlowChatState; + panelMocks.hydrateSessionHistoryForDetail.mockImplementationOnce(async () => { + flowChatState = { + ...flowChatState, + sessions: new Map(flowChatState.sessions).set('review-check-child', { + ...runningChild, + dialogTurns: [{ + ...runningChild.dialogTurns[0], + status: 'completed', + endTime: 2, + }], + }), + } as FlowChatState; + }); + + await act(async () => { + root.render( + , + ); + }); + + await act(async () => { + container.querySelector('[data-testid="btw-session-panel-stop-review"]')! + .dispatchEvent(new MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(panelMocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith('review-check-child'); + expect(panelMocks.notificationError).not.toHaveBeenCalled(); + }); + it('shows the completed Deep Review action bar even when the report has no remediation items', async () => { await act(async () => { root.render( diff --git a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.scss b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.scss index d35589f65e..d5a67621c0 100644 --- a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.scss +++ b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.scss @@ -164,12 +164,41 @@ &__empty-state { display: flex; + flex-direction: column; align-items: center; justify-content: center; + gap: 8px; min-height: 120px; + padding: 24px 16px; + box-sizing: border-box; color: var(--color-text-muted); font-size: var(--tool-card-action-font-size); text-align: center; + + &--with-content { + min-height: 0; + padding: 10px 16px; + border-bottom: 1px solid var(--border-subtle); + } + } + + &__stop-spinner { + animation: btw-session-panel-stop-spin 0.9s linear infinite; + } + + &__empty-retry { + padding: 5px 10px; + border: 1px solid var(--border-base); + border-radius: 6px; + background: var(--color-bg-secondary); + color: var(--color-text-secondary); + font: inherit; + cursor: pointer; + + &:hover { + background: var(--element-bg-hover); + color: var(--color-text-primary); + } } &__scroll-to-bottom { @@ -235,3 +264,15 @@ flex-shrink: 0; } } + +@keyframes btw-session-panel-stop-spin { + to { + transform: rotate(360deg); + } +} + +@media (prefers-reduced-motion: reduce) { + .btw-session-panel__stop-spinner { + animation: none; + } +} 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 002b53e132..253ad75ef6 100644 --- a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.tsx +++ b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.tsx @@ -1,7 +1,7 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; import {useTranslation} from 'react-i18next'; import path from 'path-browserify'; -import {CornerUpLeft, Link2, Square, Sparkles} from 'lucide-react'; +import {CornerUpLeft, Link2, Loader2, Square, Sparkles} from 'lucide-react'; import {FlowChatContext} from '../modern/FlowChatContext'; import {VirtualItemRenderer} from '../modern/VirtualItemRenderer'; import {ProcessingIndicator} from '../modern/ProcessingIndicator'; @@ -23,7 +23,17 @@ import {agentAPI} from '@/infrastructure/api'; import {globalEventBus} from '@/infrastructure/event-bus'; import {notificationService} from '@/shared/notification-system'; import {createLogger} from '@/shared/utils/logger'; -import {settleStoppedReviewSessionState} from '../../utils/reviewSessionStop'; +import { + deriveReviewDetailProjection, + filterReviewDetailItems, + type ReviewDetailContentState, + type ReviewDetailExecutionState, +} from '../../utils/reviewDetailState'; +import {findReviewTaskOutcome} from '../../utils/reviewTaskOutcome'; +import { + loadBtwSessionHistory, + type BtwSessionViewKind, +} from '../../services/btwSessionPane'; import {findLatestCodeReviewResult, findLatestCodeReviewResultState} from '../../utils/reviewSessionSummary'; import { deriveDeepReviewInterruption, @@ -72,6 +82,8 @@ export interface BtwSessionPanelProps { childSessionId?: string; parentSessionId?: string; workspacePath?: string; + viewKind?: BtwSessionViewKind; + displayTitle?: string; } const PANEL_CONFIG: FlowChatConfig = { @@ -89,6 +101,20 @@ const log = createLogger('BtwSessionPanel'); const REVIEW_ACTION_BOTTOM_BLANK_SPACE_PX = 96; const EMPTY_ACTION_ID_SET = new Set(); const EMPTY_REMEDIATION_ITEMS: ReturnType = []; +const REVIEW_DETAIL_CONTENT_STATE_KEYS: Record = { + loading: 'childSession.reviewDetail.loading', + 'load-failed': 'childSession.reviewDetail.loadFailed', + unavailable: 'childSession.reviewDetail.unavailable', +}; +const REVIEW_DETAIL_EXECUTION_STATE_KEYS: Record = { + preparing: 'childSession.reviewDetail.preparing', + 'completed-empty': 'childSession.reviewDetail.completedEmpty', + 'partial-timeout': 'childSession.reviewDetail.partialTimedOut', + stopped: 'childSession.reviewDetail.stopped', + interrupted: 'childSession.reviewDetail.interrupted', + 'timed-out': 'childSession.reviewDetail.timedOut', + failed: 'childSession.reviewDetail.failed', +}; const isActiveReviewTurnStatus = (status?: DialogTurn['status']) => status === 'pending' || @@ -115,6 +141,8 @@ export const BtwSessionPanel: React.FC = ({ childSessionId, parentSessionId, workspacePath, + viewKind, + displayTitle, }) => { const { t } = useTranslation('flow-chat'); const [flowChatState, setFlowChatState] = useState(() => flowChatStore.getState()); @@ -138,7 +166,9 @@ export const BtwSessionPanel: React.FC = ({ childRelationship.kind === 'subagent' ? childRelationship.kind : 'btw'; - const childBadgeLabel = t(`childSession.kinds.${childKind}.short`, { + const childBadgeLabel = viewKind === 'review-check' + ? t('toolCards.taskTool.reviewCoverageLabel') + : t(`childSession.kinds.${childKind}.short`, { defaultValue: childKind === 'deep_review' ? 'Strict' : childKind === 'review' @@ -148,7 +178,7 @@ export const BtwSessionPanel: React.FC = ({ : childKind === 'miniapp' ? 'MiniApp' : t('btw.shortLabel'), - }); + }); const childTitleFallback = t(`childSession.kinds.${childKind}.title`, { defaultValue: t('btw.threadLabel'), }); @@ -156,7 +186,16 @@ export const BtwSessionPanel: React.FC = ({ defaultValue: t('btw.origin'), }); const showOriginMeta = childKind !== 'miniapp' && childKind !== 'subagent'; - const virtualItems = useMemo(() => sessionToVirtualItems(childSession ?? null), [childSession]); + const sessionVirtualItems = useMemo( + () => sessionToVirtualItems(childSession ?? null), + [childSession], + ); + const virtualItems = useMemo( + () => viewKind === 'review-check' + ? filterReviewDetailItems(sessionVirtualItems) + : sessionVirtualItems, + [sessionVirtualItems, viewKind], + ); const { exploreGroupStates, onExploreGroupToggle, @@ -164,29 +203,58 @@ export const BtwSessionPanel: React.FC = ({ onExpandAllInTurn, onCollapseGroup, } = useExploreGroupState(virtualItems); + const isReviewDetail = viewKind === 'review-check' || childKind === 'review' || childKind === 'deep_review'; + const reviewTaskOutcome = viewKind === 'review-check' + ? findReviewTaskOutcome(parentSession, childSession?.parentToolCallId) + : null; + const reviewDetailProjection = isReviewDetail + ? deriveReviewDetailProjection(childSession, virtualItems.length > 0, reviewTaskOutcome) + : null; + const reviewDetailNotices = reviewDetailProjection + ? [ + ...(reviewDetailProjection.execution + ? [{ + state: reviewDetailProjection.execution, + key: REVIEW_DETAIL_EXECUTION_STATE_KEYS[reviewDetailProjection.execution], + }] + : []), + ...(reviewDetailProjection.content + ? [{ + state: reviewDetailProjection.content, + key: REVIEW_DETAIL_CONTENT_STATE_KEYS[reviewDetailProjection.content], + }] + : []), + ] + : []; + const canRetryReviewDetailLoad = virtualItems.length === 0 && Boolean( + reviewDetailProjection?.content === 'load-failed' || + reviewDetailProjection?.content === 'unavailable' || + reviewDetailProjection?.execution === 'partial-timeout' + ); // Load history for historical sessions that have not yet had their turns loaded. - const isLoadingRef = useRef(false); - useEffect(() => { + const loadChildHistory = useCallback(async () => { if (!childSessionId || !childSession) return; - if (!childSession.isHistorical) return; - if (isLoadingRef.current) return; - const path = workspacePath ?? childSession.workspacePath; + const path = workspacePath ?? childSession.workspacePath ?? parentSession?.workspacePath; if (!path) return; - isLoadingRef.current = true; - flowChatStore.loadSessionHistory( + await loadBtwSessionHistory({ childSessionId, - path, - undefined, - childSession.remoteConnectionId, - childSession.remoteSshHost, - { includeInternal: childSession.sessionKind === 'subagent' }, - ).finally(() => { - isLoadingRef.current = false; + ...(!childSession.workspacePath + ? { + workspacePath: path, + remoteConnectionId: childSession.remoteConnectionId || parentSession?.remoteConnectionId, + remoteSshHost: childSession.remoteSshHost || parentSession?.remoteSshHost, + } + : {}), }); - }, [childSessionId, childSession, workspacePath]); + }, [childSessionId, childSession, parentSession, workspacePath]); + + useEffect(() => { + if (!childSession?.isHistorical || childSession.historyState !== 'metadata-only') return; + void loadChildHistory().catch(() => undefined); + }, [childSession?.historyState, childSession?.isHistorical, loadChildHistory]); const updateScrollAffordance = useCallback(() => { const container = scrollContainerRef.current; @@ -280,6 +348,7 @@ export const BtwSessionPanel: React.FC = ({ sessionId: childSessionId, activeSessionOverride: childSession ?? null, allowUserMessageEdit: false, + allowTranscriptExport: viewKind !== 'review-check', config: PANEL_CONFIG, exploreGroupStates, onExploreGroupToggle, @@ -296,6 +365,7 @@ export const BtwSessionPanel: React.FC = ({ onExpandGroup, onExpandAllInTurn, onCollapseGroup, + viewKind, ]); const lastDialogTurn = childSession?.dialogTurns[childSession.dialogTurns.length - 1]; @@ -347,7 +417,7 @@ export const BtwSessionPanel: React.FC = ({ }, [isTurnProcessing, lastItem, isContentGrowing]); const canStopReviewSession = - (childKind === 'review' || childKind === 'deep_review') && + (viewKind === 'review-check' || childKind === 'review' || childKind === 'deep_review') && isTurnProcessing && !stoppingReview; @@ -365,7 +435,7 @@ export const BtwSessionPanel: React.FC = ({ const actionBarLastSubmittedAction = actionBarState?.lastSubmittedAction ?? null; const isDeepReview = childKind === 'deep_review'; const isReviewSession = childKind === 'review' || childKind === 'deep_review'; - const canReturnToParentSession = isReviewSession && Boolean(parentSessionId); + const canReturnToParentSession = (viewKind === 'review-check' || isReviewSession) && Boolean(parentSessionId); const btwOrigin = childSession?.btwOrigin; const showReviewActionBar = isReviewSession && @@ -861,19 +931,42 @@ export const BtwSessionPanel: React.FC = ({ } setStoppingReview(true); + const reportUnconfirmedStop = async () => { + await loadChildHistory().catch(() => undefined); + const latestSession = flowChatStore.getState().sessions.get(childSessionId); + const latestTurn = latestSession?.dialogTurns[latestSession.dialogTurns.length - 1]; + if (isActiveReviewTurnStatus(latestTurn?.status)) { + notificationService.error( + t(viewKind === 'review-check' + ? 'toolCards.taskDetailPanel.stopReviewWorkFailed' + : 'childSession.stopReviewFailed'), + ); + } + }; try { - const cancelRequest = agentAPI.cancelSession(childSessionId); - await settleStoppedReviewSessionState(childSessionId); - await cancelRequest; + const result = await agentAPI.cancelSession(childSessionId); + if (!result.cancelled) { + await reportUnconfirmedStop(); + } else { + await loadChildHistory().catch(() => undefined); + } } catch (error) { log.error('Failed to stop review session', { childSessionId, error }); - notificationService.error( - t('childSession.stopReviewFailed'), - ); + await reportUnconfirmedStop(); } finally { setStoppingReview(false); } - }, [childSessionId, stoppingReview, isTurnProcessing, t]); + }, [childSessionId, stoppingReview, isTurnProcessing, loadChildHistory, t, viewKind]); + + const stopReviewLabel = viewKind === 'review-check' + ? t('toolCards.taskDetailPanel.stopReviewWork') + : t('childSession.stopReview'); + const stoppingReviewLabel = viewKind === 'review-check' + ? t('toolCards.taskDetailPanel.stoppingReviewWork') + : t('childSession.stoppingReview'); + const returnToParentLabel = viewKind === 'review-check' + ? t('childSession.backToReview') + : t('btw.backToParent'); const handleReturnToParentSession = useCallback(() => { const resolvedParentSessionId = btwOrigin?.parentSessionId || parentSessionId; @@ -914,7 +1007,11 @@ export const BtwSessionPanel: React.FC = ({ {childBadgeLabel}
- {resolveSessionTitle(childSession, childTitleFallback)} + + {displayTitle?.trim() || (viewKind === 'review-check' + ? childBadgeLabel + : resolveSessionTitle(childSession, childTitleFallback))} +
{showOriginMeta && ( @@ -924,7 +1021,7 @@ export const BtwSessionPanel: React.FC = ({ {resolveSessionTitle(parentSession, t('btw.parent'))}
)} - {(childKind === 'review' || childKind === 'deep_review') && ( + {(viewKind === 'review-check' || childKind === 'review' || childKind === 'deep_review') && ( = ({ onClick={() => void handleStopReviewSession()} disabled={!canStopReviewSession} tooltip={stoppingReview - ? t('childSession.stoppingReview') - : t('childSession.stopReview')} + ? stoppingReviewLabel + : stopReviewLabel} aria-label={stoppingReview - ? t('childSession.stoppingReview') - : t('childSession.stopReview')} + ? stoppingReviewLabel + : stopReviewLabel} data-testid="btw-session-panel-stop-review" > - + {stoppingReview ? ( + + ) : ( + + )} )} {canReturnToParentSession && ( @@ -948,8 +1053,8 @@ export const BtwSessionPanel: React.FC = ({ variant="ghost" size="xs" onClick={handleReturnToParentSession} - tooltip={backTooltip} - aria-label={t('btw.backToParent')} + tooltip={viewKind === 'review-check' ? returnToParentLabel : backTooltip} + aria-label={returnToParentLabel} data-testid="btw-session-panel-origin-button" > @@ -963,8 +1068,33 @@ export const BtwSessionPanel: React.FC = ({ className="btw-session-panel__body" style={reviewActionBottomPadding > 0 ? { paddingBottom: `${reviewActionBottomPadding}px` } : undefined} > + {isReviewDetail && reviewDetailNotices.length > 0 && ( +
0 ? ' btw-session-panel__empty-state--with-content' : ''}`} + role={reviewDetailNotices.some(({ state }) => + state === 'load-failed' || state === 'failed' || state === 'timed-out') + ? 'alert' + : 'status'} + aria-live="polite" + > + {reviewDetailNotices.map(({ state, key }) => ( + {t(key, { label: childBadgeLabel })} + ))} + {canRetryReviewDetailLoad && ( + + )} +
+ )} {virtualItems.length === 0 ? ( -
{t('session.empty')}
+ !isReviewDetail || reviewDetailNotices.length === 0 ? ( +
{t('session.empty')}
+ ) : null ) : ( <> {virtualItems.map((item, index) => ( diff --git a/src/web-ui/src/flow_chat/components/modern/ExportImageButton.tsx b/src/web-ui/src/flow_chat/components/modern/ExportImageButton.tsx index df349e0530..0802cac5f7 100644 --- a/src/web-ui/src/flow_chat/components/modern/ExportImageButton.tsx +++ b/src/web-ui/src/flow_chat/components/modern/ExportImageButton.tsx @@ -541,6 +541,9 @@ export const ExportImageButton: React.FC = ({ className={`model-round-item__action-btn model-round-item__export-btn ${className}`} onClick={handleExport} disabled={isExporting} + aria-label={isExporting + ? i18nService.t('flow-chat:exportImage.exporting') + : i18nService.t('flow-chat:exportImage.exportToImage')} > {isExporting ? : } diff --git a/src/web-ui/src/flow_chat/components/modern/FlowChatContext.tsx b/src/web-ui/src/flow_chat/components/modern/FlowChatContext.tsx index ad8e3ae9a6..995c151738 100644 --- a/src/web-ui/src/flow_chat/components/modern/FlowChatContext.tsx +++ b/src/web-ui/src/flow_chat/components/modern/FlowChatContext.tsx @@ -27,6 +27,8 @@ export interface FlowChatContextValue { activeSessionOverride?: Session | null; allowUserMessageRollback?: boolean; allowUserMessageEdit?: boolean; + /** Hides transcript actions when the visible projection omits internal turn input. */ + allowTranscriptExport?: boolean; // Config config?: FlowChatConfig; diff --git a/src/web-ui/src/flow_chat/components/modern/ModelRoundItem.tsx b/src/web-ui/src/flow_chat/components/modern/ModelRoundItem.tsx index d3ad05ba1d..4542aff06f 100644 --- a/src/web-ui/src/flow_chat/components/modern/ModelRoundItem.tsx +++ b/src/web-ui/src/flow_chat/components/modern/ModelRoundItem.tsx @@ -366,7 +366,7 @@ export const ModelRoundItem = React.memo( }) => { const { t } = useTranslation('flow-chat'); const { formatDate, formatNumber } = useI18n('flow-chat'); - const { sessionId } = useFlowChatContext(); + const { sessionId, allowTranscriptExport = true } = useFlowChatContext(); const typewriterRevealGate = useCreateTypewriterRevealGate(); // Capture mount-time streaming state once: history rounds may fade in, // but a round that started as streaming must never replay fadeIn when it @@ -877,7 +877,7 @@ export const ModelRoundItem = React.memo( -
+ {allowTranscriptExport &&
)} -
+
} - + {allowTranscriptExport && } )} diff --git a/src/web-ui/src/flow_chat/components/modern/ModernFlowChatContainer.tsx b/src/web-ui/src/flow_chat/components/modern/ModernFlowChatContainer.tsx index d48a6ec848..8f615b70e7 100644 --- a/src/web-ui/src/flow_chat/components/modern/ModernFlowChatContainer.tsx +++ b/src/web-ui/src/flow_chat/components/modern/ModernFlowChatContainer.tsx @@ -1264,7 +1264,18 @@ export const ModernFlowChatContainer: React.FC = ( } try { - await agentAPI.cancelSession(subagent.sessionId); + const result = await agentAPI.cancelSession(subagent.sessionId); + if (!result.cancelled) { + setStoppingBackgroundSubagentIds((previous) => { + const next = new Set(previous); + next.delete(subagent.sessionId); + return next; + }); + notificationService.error( + t('flowChatHeader.backgroundSubagentStopFailed'), + { duration: 5000 }, + ); + } } catch (_error) { setStoppingBackgroundSubagentIds((previous) => { const next = new Set(previous); diff --git a/src/web-ui/src/flow_chat/services/FlowChatManager.ts b/src/web-ui/src/flow_chat/services/FlowChatManager.ts index 6ceff7faa9..0ca9c4f200 100644 --- a/src/web-ui/src/flow_chat/services/FlowChatManager.ts +++ b/src/web-ui/src/flow_chat/services/FlowChatManager.ts @@ -23,11 +23,17 @@ import { } from '../utils/sessionOrdering'; import { resolveSessionRelationship } from '../utils/sessionMetadata'; -import type { FlowChatContext, SessionConfig, DialogTurn } from './flow-chat-manager/types'; +import type { + FlowChatContext, + SessionConfig, + DialogTurn, + SessionHistoryHydrationLocation, +} from './flow-chat-manager/types'; import { saveAllInProgressTurns, immediateSaveDialogTurn, createChatSession as createChatSessionModule, + hydrateSessionHistoryForDetail as hydrateSessionHistoryForDetailModule, preloadHistoricalSessionForOpen as preloadHistoricalSessionForOpenModule, switchChatSession as switchChatSessionModule, deleteChatSession as deleteChatSessionModule, @@ -75,6 +81,7 @@ export class FlowChatManager { }), pendingTurnCompletions: new Map(), pendingHistoryLoads: new Map(), + pendingHistoryLoadCapabilities: new Map(), pendingContextRestores: new Map(), contentBuffers: new Map(), activeTextItems: new Map(), @@ -494,6 +501,13 @@ export class FlowChatManager { preloadHistoricalSessionForOpenModule(this.context, sessionId); } + async hydrateSessionHistoryForDetail( + sessionId: string, + location?: SessionHistoryHydrationLocation, + ): Promise { + await hydrateSessionHistoryForDetailModule(this.context, sessionId, location); + } + async deleteChatSession(sessionId: string): Promise { return deleteChatSessionModule(this.context, sessionId); } diff --git a/src/web-ui/src/flow_chat/services/btwSessionPane.ts b/src/web-ui/src/flow_chat/services/btwSessionPane.ts index 7f27767d07..5634e6ba7d 100644 --- a/src/web-ui/src/flow_chat/services/btwSessionPane.ts +++ b/src/web-ui/src/flow_chat/services/btwSessionPane.ts @@ -5,13 +5,18 @@ import { useAgentCanvasStore } from '@/app/components/panels/content-canvas/stor import type { CanvasTab } from '@/app/components/panels/content-canvas/types'; import { flowChatStore } from '../store/FlowChatStore'; import { resolveSessionTitle } from '../utils/sessionTitle'; +import { flowChatManager } from './FlowChatManager'; export const BTW_SESSION_PANEL_TYPE = 'btw-session' as const; +export type BtwSessionViewKind = 'review-check'; + export interface BtwSessionPanelData { childSessionId: string; parentSessionId: string; workspacePath?: string; + viewKind?: BtwSessionViewKind; + displayTitle?: string; } export interface BtwSessionPanelMetadata { @@ -35,6 +40,13 @@ export interface EnsureBtwSessionAvailableParams { includeInternal?: boolean; } +export interface LoadBtwSessionHistoryParams { + childSessionId: string; + workspacePath?: string; + remoteConnectionId?: string; + remoteSshHost?: string; +} + type AgentCanvasState = ReturnType; export const getBtwSessionDuplicateKey = (childSessionId: string) => `btw-session-${childSessionId}`; @@ -90,14 +102,18 @@ const requestRightPanelExpansion = (): void => { export const buildBtwSessionPanelContent = ( childSessionId: string, parentSessionId: string, - workspacePath?: string + workspacePath?: string, + viewKind?: BtwSessionViewKind, + displayTitle?: string, ): PanelContent => ({ type: BTW_SESSION_PANEL_TYPE, - title: resolveBtwSessionTitle(childSessionId), + title: displayTitle?.trim() || resolveBtwSessionTitle(childSessionId), data: { childSessionId, parentSessionId, workspacePath, + ...(viewKind ? { viewKind } : {}), + ...(displayTitle?.trim() ? { displayTitle: displayTitle.trim() } : {}), } satisfies BtwSessionPanelData, metadata: { duplicateCheckKey: getBtwSessionDuplicateKey(childSessionId), @@ -132,6 +148,21 @@ export const selectActiveBtwSessionTab = (state: AgentCanvasState): CanvasTab | return activeTab; }; +export async function loadBtwSessionHistory(params: LoadBtwSessionHistoryParams): Promise { + const location = params.workspacePath + ? { + workspacePath: params.workspacePath, + remoteConnectionId: params.remoteConnectionId, + remoteSshHost: params.remoteSshHost, + } + : undefined; + if (location) { + await flowChatManager.hydrateSessionHistoryForDetail(params.childSessionId, location); + } else { + await flowChatManager.hydrateSessionHistoryForDetail(params.childSessionId); + } +} + export function ensureBtwSessionAvailable(params: EnsureBtwSessionAvailableParams): void { const existingSession = flowChatStore.getState().sessions.get(params.childSessionId); const parentSession = flowChatStore.getState().sessions.get(params.parentSessionId); @@ -192,14 +223,16 @@ export function ensureBtwSessionAvailable(params: EnsureBtwSessionAvailableParam return; } - void flowChatStore.loadSessionHistory( - params.childSessionId, - workspacePath, - undefined, - resolvedRemoteConnectionId, - resolvedRemoteSshHost, - { includeInternal: params.includeInternal }, - ); + void loadBtwSessionHistory({ + childSessionId: params.childSessionId, + ...(!sessionToHydrate?.workspacePath + ? { + workspacePath, + remoteConnectionId: resolvedRemoteConnectionId, + remoteSshHost: resolvedRemoteSshHost, + } + : {}), + }).catch(() => undefined); } export function openBtwSessionInAuxPane(params: { @@ -215,13 +248,16 @@ export function openBtwSessionInAuxPane(params: { remoteConnectionId?: string; remoteSshHost?: string; includeInternal?: boolean; + viewKind?: BtwSessionViewKind; }): void { ensureBtwSessionAvailable(params); const content = buildBtwSessionPanelContent( params.childSessionId, params.parentSessionId, - params.workspacePath + params.workspacePath, + params.viewKind, + params.sessionTitle, ); const duplicateCheckKey = content.metadata?.duplicateCheckKey; @@ -232,6 +268,7 @@ export function openBtwSessionInAuxPane(params: { if (params.expand !== false && isRightPanelCollapsed()) { requestRightPanelExpansion(); } + canvasStore.updateTabContent(existing.tab.id, existing.groupId, content); canvasStore.switchToTab(existing.tab.id, existing.groupId); clearSessionUnreadCompletionAfterRender(params.childSessionId); return; 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 760322f1ab..79a0255c00 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 @@ -137,6 +137,50 @@ describe('subagent parent helpers', () => { toolCallId: 'task-2', }); }); + + it('projects a linked Review manifest into the live child session', () => { + const task = makeTaskTool('task-review'); + FlowChatStore.getInstance().setState(() => ({ + sessions: new Map([[ + 'parent-session', + { + sessionId: 'parent-session', + title: 'Parent Session', + dialogTurns: [{ + id: 'parent-turn', + sessionId: 'parent-session', + userMessage: { id: 'user-1', content: 'Review', timestamp: 900 }, + modelRounds: [makeRound('round-1', [task])], + status: 'processing', + startTime: 900, + }], + status: 'idle', + config: { agentType: 'CodeReview' }, + createdAt: 800, + lastActiveAt: 1000, + error: null, + sessionKind: 'normal', + workspacePath: 'D:\\workspace\\repo', + } as Session, + ]]), + activeSessionId: 'parent-session', + })); + __test_only__.handleSubagentSessionLinked( + { currentWorkspacePath: 'D:\\workspace\\repo' } as FlowChatContext, + { + sessionId: 'review-child', + parentSessionId: 'parent-session', + parentDialogTurnId: 'parent-turn', + parentToolCallId: 'task-review', + agentType: 'ReviewWorker', + focusedReviewDisplayLabel: 'Authentication boundary', + }, + ); + + expect( + FlowChatStore.getInstance().getState().sessions.get('review-child')?.focusedReviewDisplayLabel, + ).toBe('Authentication boundary'); + }); }); describe('shouldProcessEvent', () => { 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 bf5ca9933f..2d1ac4f33f 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 @@ -153,6 +153,7 @@ export const __test_only__ = { mergeParamsPartialEventData, findSubagentParentInfoByRound, handleDialogTurnFailed, + handleSubagentSessionLinked, }; function shouldMarkUnreadCompletion(sessionId: string): boolean { @@ -396,6 +397,7 @@ function ensureSubagentSession( subagentSessionId: string, event?: Record, explicitSubagentType?: string, + focusedReviewDisplayLabel?: SubagentSessionLinkedEvent['focusedReviewDisplayLabel'], ): void { const store = FlowChatStore.getInstance(); const existing = store.getState().sessions.get(subagentSessionId); @@ -414,6 +416,7 @@ function ensureSubagentSession( subagentType: subagentType || undefined, }); } + store.updateSessionFocusedReviewDisplayLabel(subagentSessionId, focusedReviewDisplayLabel); return; } @@ -438,6 +441,7 @@ function ensureSubagentSession( ? parentTurnIndex + 1 : undefined, }, + focusedReviewDisplayLabel, }, parentSession?.remoteConnectionId || extractEventRemoteConnectionId(event), parentSession?.remoteSshHost || extractEventRemoteSshHost(event), @@ -487,6 +491,11 @@ function handleSubagentSessionLinked( event?.subagentDialogTurnId ?? (event as any)?.subagent_dialog_turn_id; const agentType = event?.agentType ?? (event as any)?.agent_type; const modelId = event?.modelId ?? (event as any)?.model_id; + const rawFocusedReviewDisplayLabel = event?.focusedReviewDisplayLabel + ?? (event as any)?.focused_review_display_label; + const focusedReviewDisplayLabel = typeof rawFocusedReviewDisplayLabel === 'string' + ? rawFocusedReviewDisplayLabel + : undefined; if (!childSessionId || !parentSessionId || !parentDialogTurnId || !parentToolCallId) { log.warn('SubagentSessionLinked missing required fields', { event }); @@ -500,7 +509,14 @@ function handleSubagentSessionLinked( }; attachSubagentSessionToParentTool(parentInfo, childSessionId, subagentDialogTurnId); - ensureSubagentSession(context, parentInfo, childSessionId, event as Record, agentType); + ensureSubagentSession( + context, + parentInfo, + childSessionId, + event as Record, + agentType, + focusedReviewDisplayLabel, + ); if (typeof modelId === 'string' && modelId.trim()) { FlowChatStore.getInstance().updateSessionModelName(childSessionId, modelId.trim()); } diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.test.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.test.ts index 654898fadd..a844f4d48f 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.test.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.test.ts @@ -4,6 +4,7 @@ import { createChatSession, deleteChatSession, ensureBackendSession, + hydrateSessionHistoryForDetail, preloadHistoricalSessionForOpen, retryCreateBackendSession, resolveAgentTypeForSessionCreation, @@ -614,6 +615,116 @@ describe('SessionModule historical session coordination', () => { await load.promise; }); + it('deduplicates concurrent detail hydration and includes internal child output', async () => { + const load = createDeferred(); + const { context, flowChatStore } = createContext(createSession({ + isHistorical: false, + historyState: 'new', + sessionKind: 'subagent', + })); + flowChatStore.loadSessionHistory.mockReturnValueOnce(load.promise); + + const first = hydrateSessionHistoryForDetail(context, 'history-1'); + const second = hydrateSessionHistoryForDetail(context, 'history-1'); + await Promise.resolve(); + + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledTimes(1); + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledWith( + 'history-1', + 'D:/workspace/BitFun', + undefined, + undefined, + undefined, + { includeInternal: true, deferFullHistoryUntilActive: false }, + ); + + load.resolve(); + await Promise.all([first, second]); + }); + + it('uses the owning panel scope when a legacy child is missing its workspace location', async () => { + const { context, flowChatStore } = createContext(createSession({ + workspacePath: undefined, + remoteConnectionId: undefined, + remoteSshHost: undefined, + sessionKind: 'subagent', + })); + + await hydrateSessionHistoryForDetail(context, 'history-1', { + workspacePath: 'D:/workspace/BitFun', + remoteConnectionId: 'remote-current', + remoteSshHost: 'host-current', + }); + + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledWith( + 'history-1', + 'D:/workspace/BitFun', + undefined, + 'remote-current', + 'host-current', + { includeInternal: true, deferFullHistoryUntilActive: false }, + ); + }); + + it('retries with a stronger location after a reused weak hydrate fails', async () => { + const { context, flowChatStore } = createContext(createSession({ + workspacePath: undefined, + remoteConnectionId: undefined, + remoteSshHost: undefined, + sessionKind: 'subagent', + })); + + const weakHydrate = hydrateSessionHistoryForDetail(context, 'history-1'); + const strongHydrate = hydrateSessionHistoryForDetail(context, 'history-1', { + workspacePath: 'D:/workspace/BitFun', + remoteConnectionId: 'remote-current', + remoteSshHost: 'host-current', + }); + + await expect(weakHydrate).rejects.toThrow('Workspace path is required'); + await expect(strongHydrate).resolves.toBeUndefined(); + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledTimes(1); + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledWith( + 'history-1', + 'D:/workspace/BitFun', + undefined, + 'remote-current', + 'host-current', + { includeInternal: true, deferFullHistoryUntilActive: false }, + ); + }); + + it('upgrades a weaker in-flight preload before showing subagent details', async () => { + const preload = createDeferred(); + const { context, flowChatStore } = createContext(createSession({ + sessionKind: 'subagent', + })); + context.pendingHistoryLoads.set('history-other', Promise.resolve()); + flowChatStore.loadSessionHistory + .mockReturnValueOnce(preload.promise) + .mockResolvedValueOnce(undefined); + + preloadHistoricalSessionForOpen(context, 'history-1'); + await Promise.resolve(); + const detailHydrate = hydrateSessionHistoryForDetail(context, 'history-1'); + + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledTimes(1); + + preload.resolve(); + await detailHydrate; + + expect(flowChatStore.loadSessionHistory).toHaveBeenCalledTimes(2); + expect(flowChatStore.loadSessionHistory).toHaveBeenNthCalledWith( + 2, + 'history-1', + 'D:/workspace/BitFun', + undefined, + undefined, + undefined, + { includeInternal: true, deferFullHistoryUntilActive: false }, + ); + }); + it('retries a reused preload that stale-skipped after explicit activation', async () => { const stalePreload = createDeferred(); const retryLoad = createDeferred(); diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.ts index fe600480ed..0f4cd7d7d4 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/SessionModule.ts @@ -16,7 +16,11 @@ import { isPeerDeviceModeActive } from '@/infrastructure/peer-device/peerModeFla import { normalizeRemoteWorkspacePath } from '@/shared/utils/pathUtils'; import { WorkspaceKind, type WorkspaceInfo } from '@/shared/types'; import type { AIModelConfig, AgentModelDefaultsConfig, DefaultModelsConfig } from '@/infrastructure/config/types'; -import type { FlowChatContext, SessionConfig } from './types'; +import type { + FlowChatContext, + SessionConfig, + SessionHistoryHydrationLocation, +} from './types'; import type { Session } from '../../types/flow-chat'; import { touchSessionActivity, cleanupSaveState } from './PersistenceModule'; import { cleanupSessionBuffers } from './TextChunkModule'; @@ -43,6 +47,16 @@ import { const log = createLogger('SessionModule'); const pendingSessionCreations = new Map>(); + +const getHydrationLocationKey = ( + location: SessionHistoryHydrationLocation | undefined, +): string => location?.workspacePath + ? JSON.stringify([ + location.workspacePath, + location.remoteConnectionId ?? '', + location.remoteSshHost ?? '', + ]) + : ''; export const SESSION_ACTIVITY_TOUCH_DELAY_MS = 350; let latestSwitchRequestId = 0; let pendingActivityTouchTimer: ReturnType | null = null; @@ -143,16 +157,47 @@ async function hydrateHistoricalSession( options?: { isRetryStillRelevant?: () => boolean; retryActiveStaleReuse?: boolean; + allowNonHistorical?: boolean; + includeInternal?: boolean; + deferFullHistoryUntilActive?: boolean; + location?: SessionHistoryHydrationLocation; }, ): Promise { const existing = context.pendingHistoryLoads.get(sessionId); if (existing) { + const existingCapabilities = context.pendingHistoryLoadCapabilities?.get(sessionId); + const requestedLocationKey = getHydrationLocationKey(options?.location); + const requiresStrongerHydrate = + (options?.includeInternal === true && existingCapabilities?.includeInternal !== true) || + (options?.deferFullHistoryUntilActive === false && + existingCapabilities?.deferFullHistoryUntilActive !== false) || + (Boolean(requestedLocationKey) && existingCapabilities?.locationKey !== requestedLocationKey); startupTrace.markPhase('historical_session_hydrate_reused'); recordHistorySessionDiagnosticEvent(sessionId, 'hydrate_reused_pending', { notifyOnError, retryActiveStaleReuse: options?.retryActiveStaleReuse === true, }); - await existing; + let existingFailed = false; + let existingError: unknown; + try { + await existing; + } catch (error) { + existingFailed = true; + existingError = error; + } + if (requiresStrongerHydrate) { + if (context.pendingHistoryLoads.get(sessionId) === existing) { + context.pendingHistoryLoads.delete(sessionId); + } + if (context.pendingHistoryLoadCapabilities?.get(sessionId)?.promise === existing) { + context.pendingHistoryLoadCapabilities.delete(sessionId); + } + await hydrateHistoricalSession(context, sessionId, notifyOnError, options); + return; + } + if (existingFailed) { + throw existingError; + } const retryStillRelevant = options?.isRetryStillRelevant?.() !== false; const shouldRetryActiveStale = shouldRetryActiveStaleHydrate(context, sessionId); recordHistorySessionDiagnosticEvent(sessionId, 'hydrate_reused_settled', { @@ -184,19 +229,25 @@ async function hydrateHistoricalSession( const loadPromise = (async () => { const session = context.flowChatStore.getState().sessions.get(sessionId); - if (!session?.isHistorical) { + if (!session || (!session.isHistorical && options?.allowNonHistorical !== true)) { recordHistorySessionDiagnosticEvent(sessionId, 'hydrate_request_skipped', { reason: session ? 'not_historical' : 'missing_session', }); return; } - const workspacePath = requireSessionWorkspacePath(session.workspacePath, sessionId); - const remote = isRemoteTraceContext(session.remoteConnectionId, session.remoteSshHost); + const workspacePath = requireSessionWorkspacePath( + session.workspacePath || options?.location?.workspacePath, + sessionId, + ); + const storedConnectionId = options?.location?.remoteConnectionId || session.remoteConnectionId; + const storedSshHost = options?.location?.remoteSshHost || session.remoteSshHost; + const remote = isRemoteTraceContext(storedConnectionId, storedSshHost); + const deferFullHistoryUntilActive = options?.deferFullHistoryUntilActive ?? true; markHistorySessionHydratePending(sessionId, { notifyOnError, remote, - deferFullHistoryUntilActive: true, + deferFullHistoryUntilActive, }); startupTrace.markPhase('historical_session_hydrate_request', { remote }); recordHistorySessionDiagnosticEvent(sessionId, 'hydrate_request_started', { @@ -210,13 +261,13 @@ async function hydrateHistoricalSession( // remoteConnectionId becomes stale; the active workspace always // carries the up-to-date connection_id. const effectiveConnectionId = resolveEffectiveConnectionId( - session.remoteConnectionId, - session.remoteSshHost, + storedConnectionId, + storedSshHost, workspacePath ); const effectiveSshHost = resolveEffectiveSshHost( - session.remoteSshHost, - session.remoteConnectionId, + storedSshHost, + storedConnectionId, workspacePath ); @@ -226,11 +277,22 @@ async function hydrateHistoricalSession( undefined, effectiveConnectionId, effectiveSshHost, - { deferFullHistoryUntilActive: true }, + { + includeInternal: options?.includeInternal, + deferFullHistoryUntilActive, + }, ); })(); context.pendingHistoryLoads.set(sessionId, loadPromise); + const pendingHistoryLoadCapabilities = + context.pendingHistoryLoadCapabilities ??= new Map(); + pendingHistoryLoadCapabilities.set(sessionId, { + promise: loadPromise, + includeInternal: options?.includeInternal === true, + deferFullHistoryUntilActive: options?.deferFullHistoryUntilActive ?? true, + locationKey: getHydrationLocationKey(options?.location), + }); try { await loadPromise; @@ -257,9 +319,26 @@ async function hydrateHistoricalSession( if (context.pendingHistoryLoads.get(sessionId) === loadPromise) { context.pendingHistoryLoads.delete(sessionId); } + if (context.pendingHistoryLoadCapabilities?.get(sessionId)?.promise === loadPromise) { + context.pendingHistoryLoadCapabilities.delete(sessionId); + } } } +export async function hydrateSessionHistoryForDetail( + context: FlowChatContext, + sessionId: string, + location?: SessionHistoryHydrationLocation, +): Promise { + const session = context.flowChatStore.getState().sessions.get(sessionId); + await hydrateHistoricalSession(context, sessionId, false, { + allowNonHistorical: true, + includeInternal: session?.sessionKind === 'subagent', + deferFullHistoryUntilActive: false, + location, + }); +} + function shouldHydrateHistoricalSessionBeforeSwitch(session: Session | undefined): session is Session { if (session?.isHistorical !== true) { return false; diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/index.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/index.ts index 8e0ec79417..3e299424da 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/index.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/index.ts @@ -32,6 +32,7 @@ export { getModelMaxTokens, resolveAgentTypeForSessionCreation, createChatSession, + hydrateSessionHistoryForDetail, preloadHistoricalSessionForOpen, switchChatSession, deleteChatSession, diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/types.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/types.ts index 2ee4734cdf..b3b317927b 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/types.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/types.ts @@ -23,6 +23,13 @@ export interface FlowChatContext { }>; /** In-flight historical session hydration: sessionId -> promise */ pendingHistoryLoads: Map>; + /** Capabilities of each in-flight hydrate, used to avoid reusing a weaker preload. */ + pendingHistoryLoadCapabilities?: Map; + includeInternal: boolean; + deferFullHistoryUntilActive: boolean; + locationKey: string; + }>; /** In-flight backend context restore for view-restored historical sessions. */ pendingContextRestores?: Map>; /** Content buffers: sessionId -> (roundId -> content) */ @@ -54,6 +61,13 @@ export interface FlowChatContext { currentWorkspacePath: string | null; } +/** Current owner scope used only when a restored child lacks saved location metadata. */ +export interface SessionHistoryHydrationLocation { + workspacePath?: string; + remoteConnectionId?: string; + remoteSshHost?: string; +} + /** * Tool event handling options. */ diff --git a/src/web-ui/src/flow_chat/services/openBtwSession.test.ts b/src/web-ui/src/flow_chat/services/openBtwSession.test.ts index 3098741ba4..b9c9a895d9 100644 --- a/src/web-ui/src/flow_chat/services/openBtwSession.test.ts +++ b/src/web-ui/src/flow_chat/services/openBtwSession.test.ts @@ -6,10 +6,11 @@ const mocks = vi.hoisted(() => ({ createTab: vi.fn(), clearSessionUnreadCompletion: vi.fn(), findTabByMetadata: vi.fn(), + updateTabContent: vi.fn(), switchToTab: vi.fn(), closeTab: vi.fn(), addExternalSession: vi.fn(), - loadSessionHistory: vi.fn(), + hydrateSessionHistoryForDetail: vi.fn(() => Promise.resolve()), updateSessionRelationship: vi.fn(), switchChatSession: vi.fn(), syncSessionToModernStore: vi.fn(), @@ -74,6 +75,7 @@ vi.mock('@/app/components/panels/content-canvas/stores', () => ({ secondaryGroup: { activeTabId: null, tabs: [] }, tertiaryGroup: { activeTabId: null, tabs: [] }, findTabByMetadata: (...args: unknown[]) => mocks.findTabByMetadata(...args), + updateTabContent: (...args: unknown[]) => mocks.updateTabContent(...args), switchToTab: (...args: unknown[]) => mocks.switchToTab(...args), closeTab: (...args: unknown[]) => mocks.closeTab(...args), }), @@ -88,8 +90,6 @@ vi.mock('../store/FlowChatStore', () => ({ }), addExternalSession: (...args: unknown[]) => mocks.addExternalSession(...args), - loadSessionHistory: (...args: unknown[]) => - mocks.loadSessionHistory(...args), updateSessionRelationship: (...args: unknown[]) => mocks.updateSessionRelationship(...args), clearSessionUnreadCompletion: (...args: unknown[]) => @@ -100,6 +100,8 @@ vi.mock('../store/FlowChatStore', () => ({ vi.mock('./FlowChatManager', () => ({ flowChatManager: { switchChatSession: (...args: unknown[]) => mocks.switchChatSession(...args), + hydrateSessionHistoryForDetail: (...args: unknown[]) => + mocks.hydrateSessionHistoryForDetail(...args), }, })); @@ -115,10 +117,11 @@ describe('openBtwSessionInAuxPane', () => { mocks.createTab.mockClear(); mocks.clearSessionUnreadCompletion.mockClear(); mocks.findTabByMetadata.mockReset(); + mocks.updateTabContent.mockClear(); mocks.switchToTab.mockClear(); mocks.closeTab.mockClear(); mocks.addExternalSession.mockClear(); - mocks.loadSessionHistory.mockClear(); + mocks.hydrateSessionHistoryForDetail.mockClear(); mocks.updateSessionRelationship.mockClear(); mocks.switchChatSession.mockReset(); mocks.syncSessionToModernStore.mockClear(); @@ -165,6 +168,42 @@ describe('openBtwSessionInAuxPane', () => { expect(mocks.clearSessionUnreadCompletion).toHaveBeenCalledWith('review-child'); }); + it('carries Review-check presentation without changing the child session kind', () => { + sessions.set('parent-session', { + sessionId: 'parent-session', + workspacePath: 'D:\\workspace\\repo', + mode: 'DeepReview', + }); + + openBtwSessionInAuxPane({ + childSessionId: 'review-check-child', + parentSessionId: 'parent-session', + workspacePath: 'D:\\workspace\\repo', + sessionKind: 'subagent', + viewKind: 'review-check', + includeInternal: true, + expand: false, + }); + + expect(mocks.createTab).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + childSessionId: 'review-check-child', + viewKind: 'review-check', + }), + }), + ); + expect(mocks.addExternalSession).toHaveBeenCalledWith( + 'review-check-child', + expect.any(String), + 'DeepReview', + 'D:\\workspace\\repo', + expect.objectContaining({ sessionKind: 'subagent' }), + undefined, + undefined, + ); + }); + it('switches to an existing aux pane tab without expanding the right panel again', () => { const dispatchEvent = stubWindowForPanelExpansion(false); mocks.findTabByMetadata.mockReturnValue({ @@ -176,11 +215,23 @@ describe('openBtwSessionInAuxPane', () => { childSessionId: 'review-child', parentSessionId: 'parent-session', workspacePath: 'D:\\workspace\\repo', + viewKind: 'review-check', + sessionTitle: 'Checking authentication', }); expect(mocks.findTabByMetadata).toHaveBeenCalledWith({ duplicateCheckKey: 'btw-session-review-child', }); + expect(mocks.updateTabContent).toHaveBeenCalledWith( + 'existing-review-tab', + 'secondary', + expect.objectContaining({ + data: expect.objectContaining({ + viewKind: 'review-check', + displayTitle: 'Checking authentication', + }), + }), + ); expect(mocks.switchToTab).toHaveBeenCalledWith('existing-review-tab', 'secondary'); expect(mocks.createTab).not.toHaveBeenCalled(); expect(dispatchEvent).not.toHaveBeenCalledWith( @@ -238,13 +289,13 @@ describe('openBtwSessionInAuxPane', () => { 'remote-1', 'host-1', ); - expect(mocks.loadSessionHistory).toHaveBeenCalledWith( + expect(mocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith( 'subagent-child', - 'D:\\workspace\\repo', - undefined, - 'remote-1', - 'host-1', - { includeInternal: true }, + { + workspacePath: 'D:\\workspace\\repo', + remoteConnectionId: 'remote-1', + remoteSshHost: 'host-1', + }, ); }); @@ -283,13 +334,37 @@ describe('openBtwSessionInAuxPane', () => { parentToolCallId: 'call-1', }), ); - expect(mocks.loadSessionHistory).toHaveBeenCalledWith( + expect(mocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith('subagent-child'); + }); + + it('passes the parent location when a legacy child has no saved workspace scope', () => { + sessions.set('parent-session', { + sessionId: 'parent-session', + workspacePath: 'D:\\workspace\\repo', + mode: 'agentic', + remoteConnectionId: 'remote-current', + remoteSshHost: 'host-current', + }); + sessions.set('subagent-child', { + sessionId: 'subagent-child', + sessionKind: 'subagent', + isHistorical: true, + historyState: 'metadata-only', + }); + + ensureBtwSessionAvailable({ + childSessionId: 'subagent-child', + parentSessionId: 'parent-session', + sessionKind: 'subagent', + }); + + expect(mocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith( 'subagent-child', - 'D:\\workspace\\repo', - undefined, - 'remote-1', - 'host-1', - { includeInternal: true }, + { + workspacePath: 'D:\\workspace\\repo', + remoteConnectionId: 'remote-current', + remoteSshHost: 'host-current', + }, ); }); @@ -321,14 +396,7 @@ describe('openBtwSessionInAuxPane', () => { }); expect(mocks.addExternalSession).not.toHaveBeenCalled(); - expect(mocks.loadSessionHistory).toHaveBeenCalledWith( - 'subagent-child', - 'D:\\workspace\\repo', - undefined, - 'remote-1', - 'host-1', - { includeInternal: true }, - ); + expect(mocks.hydrateSessionHistoryForDetail).toHaveBeenCalledWith('subagent-child'); }); it('does not hydrate an existing live subagent with in-memory turns just to fill missing model selection', () => { @@ -376,7 +444,7 @@ describe('openBtwSessionInAuxPane', () => { parentToolCallId: 'call-1', }), ); - expect(mocks.loadSessionHistory).not.toHaveBeenCalled(); + expect(mocks.hydrateSessionHistoryForDetail).not.toHaveBeenCalled(); }); }); diff --git a/src/web-ui/src/flow_chat/store/FlowChatStore.ts b/src/web-ui/src/flow_chat/store/FlowChatStore.ts index 7f09ec3621..6484ffe50a 100644 --- a/src/web-ui/src/flow_chat/store/FlowChatStore.ts +++ b/src/web-ui/src/flow_chat/store/FlowChatStore.ts @@ -54,6 +54,7 @@ import { normalizeRecoveredTextStatus, normalizeRecoveredThinkingStatus, normalizeRecoveredToolStatus, + normalizeRecoveredTurnFinishReason, normalizeRecoveredTurnStatus, settleInterruptedDialogTurn, } from '../utils/dialogTurnStability'; @@ -1839,6 +1840,7 @@ export class FlowChatStore { isTransient?: boolean; agentBackedTransient?: boolean; deepReviewRunManifest?: Session['deepReviewRunManifest']; + focusedReviewDisplayLabel?: Session['focusedReviewDisplayLabel']; reviewTargetEvidence?: Session['reviewTargetEvidence']; reviewTargetFilePaths?: Session['reviewTargetFilePaths']; }, @@ -1885,6 +1887,7 @@ export class FlowChatStore { btwThreads: [], btwOrigin: relationship.btwOrigin, deepReviewRunManifest: meta?.deepReviewRunManifest, + focusedReviewDisplayLabel: meta?.focusedReviewDisplayLabel, reviewTargetEvidence: meta?.reviewTargetEvidence, reviewTargetFilePaths: meta?.reviewTargetFilePaths, isTransient: meta?.isTransient ?? false, @@ -2097,6 +2100,21 @@ export class FlowChatStore { }); } + public updateSessionFocusedReviewDisplayLabel( + sessionId: string, + focusedReviewDisplayLabel: Session['focusedReviewDisplayLabel'], + ): void { + if (!focusedReviewDisplayLabel) return; + this.setState(prev => { + const session = prev.sessions.get(sessionId); + if (!session || session.focusedReviewDisplayLabel === focusedReviewDisplayLabel) return prev; + + const newSessions = new Map(prev.sessions); + newSessions.set(sessionId, { ...session, focusedReviewDisplayLabel }); + return { ...prev, sessions: newSessions }; + }); + } + /** * Update session relationship metadata (parent/child grouping, kind, etc.). * This is UI-only and does not affect backend behavior directly. @@ -4888,6 +4906,12 @@ export class FlowChatStore { const normalizedTurnStatus = isLiveTurn ? normalizeLiveTurnStatus(turn.status) : normalizeRecoveredTurnStatus(turn.status, { error: undefined }); + const persistedFinishReason = + typeof turn.finishReason === 'string' + ? turn.finishReason + : typeof turn.finish_reason === 'string' + ? turn.finish_reason + : undefined; const rawTokenUsage = turn.tokenUsage ?? turn.token_usage; return { @@ -5029,12 +5053,9 @@ export class FlowChatStore { }), timestamp: turn.timestamp, status: normalizedTurnStatus, - finishReason: - typeof turn.finishReason === 'string' - ? turn.finishReason - : typeof turn.finish_reason === 'string' - ? turn.finish_reason - : undefined, + finishReason: isLiveTurn + ? persistedFinishReason + : normalizeRecoveredTurnFinishReason(turn.status, persistedFinishReason), hasFinalResponse: typeof turn.hasFinalResponse === 'boolean' ? turn.hasFinalResponse diff --git a/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.scss b/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.scss index ba8925ad2d..70774c71a3 100644 --- a/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.scss +++ b/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.scss @@ -239,6 +239,27 @@ background: rgba(var(--task-failed-badge-rgb), 0.15); } + .task-review-outcome { + display: inline-flex; + align-items: center; + padding: 0.1rem var(--flowchat-inline-gap); + border-radius: 3px; + font-size: var(--flowchat-font-size-xxs); + font-weight: 500; + flex-shrink: 0; + color: var(--color-text-muted); + background: var(--element-bg-soft); + + &--partial-timeout { + color: var(--color-warning); + background: var(--color-warning-bg); + } + + &--timed-out { + color: var(--color-error); + } + } + .task-subagent-stop-button { width: 22px; height: 22px; diff --git a/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.test.tsx b/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.test.tsx index bb1c5287fb..0f62e7d386 100644 --- a/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.test.tsx +++ b/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.test.tsx @@ -7,6 +7,7 @@ import type { FlowToolItem, ToolCardConfig } from '../types/flow-chat'; const mocks = vi.hoisted(() => ({ openBtwSessionInAuxPane: vi.fn(), + loadBtwSessionHistory: vi.fn(() => Promise.resolve()), cancelSession: vi.fn(), notificationError: vi.fn(), flowChatListeners: new Set<() => void>(), @@ -44,6 +45,15 @@ vi.mock('react-i18next', () => { if (key === 'toolCards.taskTool.reviewCheckUnavailable') { return 'This check could not be completed. The main review can continue.'; } + if (key === 'toolCards.taskTool.reviewPartialTimeout') { + return 'Timed out after returning partial details'; + } + if (key === 'toolCards.taskTool.reviewTimedOut') { + return 'Timed out'; + } + if (key === 'toolCards.taskTool.reviewStopped') { + return 'Stopped'; + } if (key === 'toolCards.taskTool.cancelSession') { return `Cancel session: ${options?.sessionId}`; } @@ -84,16 +94,19 @@ vi.mock('@/shared/services/reviewTeamService', () => ({ vi.mock('./ToolTimeoutIndicator', () => ({ ToolTimeoutIndicator: ({ + isRunning, completedStatus, completedDurationMs, completedFailureReason, }: { + isRunning?: boolean; completedStatus?: string; completedDurationMs?: number; completedFailureReason?: string; }) => ( ({ vi.mock('../services/btwSessionPane', () => ({ openBtwSessionInAuxPane: (...args: unknown[]) => mocks.openBtwSessionInAuxPane(...args), + loadBtwSessionHistory: (...args: unknown[]) => mocks.loadBtwSessionHistory(...args), })); vi.mock('@/infrastructure/api/service-api/AgentAPI', () => ({ @@ -155,6 +169,27 @@ vi.mock('../store/FlowChatStore', () => ({ startTime: 1000, }], }], + ['review-session-focused', { + sessionId: 'review-session-focused', + mode: 'CodeReview', + config: { agentType: 'CodeReview', modelName: 'fast' }, + focusedReviewDisplayLabel: 'Authentication boundary', + dialogTurns: [{ + id: 'review-turn-focused', + status: 'processing', + startTime: 1000, + }], + }], + ['review-session-unsafe-label', { + sessionId: 'review-session-unsafe-label', + mode: 'CodeReview', + config: { agentType: 'CodeReview', modelName: 'fast' }, + dialogTurns: [{ + id: 'review-turn-unsafe-label', + status: 'processing', + startTime: 1000, + }], + }], ['review-session-error', { sessionId: 'review-session-error', mode: 'CodeReview', @@ -178,6 +213,17 @@ vi.mock('../store/FlowChatStore', () => ({ endTime: 1800, }], }], + ['review-session-completed', { + sessionId: 'review-session-completed', + mode: 'CodeReview', + config: { agentType: 'CodeReview', modelName: 'fast' }, + dialogTurns: [{ + id: 'review-turn-completed', + status: 'completed', + startTime: 1000, + endTime: 1800, + }], + }], ['review-session-dynamic', { sessionId: 'review-session-dynamic', mode: 'CodeReview', @@ -431,10 +477,11 @@ describeWithJsdom('TaskToolDisplay', () => { expect(container.textContent).not.toContain('managed-review:batch-1-of-4'); }); - it('shows safe additional-check progress without projecting model-controlled identifiers', async () => { + it('shows the admitted public label without projecting model-controlled identifiers', async () => { const toolItem: FlowToolItem = { ...reviewTaskItem('running', 'ReviewWorker'), toolName: 'LaunchReviewAgent', + subagentSessionId: 'review-session-focused', toolCall: { id: 'launch-review-call-focused', input: { @@ -457,7 +504,8 @@ describeWithJsdom('TaskToolDisplay', () => { ); }); - expect(container.textContent).toContain('Checking a specific concern'); + expect(container.textContent).toContain('Authentication boundary'); + expect(container.textContent).not.toContain('Check boundary'); expect(container.textContent).not.toMatch(/\bagent\b/i); expect(container.textContent).not.toContain('ReviewWorker'); expect(container.textContent).not.toContain('packet-7'); @@ -466,6 +514,43 @@ describeWithJsdom('TaskToolDisplay', () => { expect(container.textContent).not.toContain('src/internal.ts'); }); + it('falls back to a generic title when a focused-check description contains structured identifiers', async () => { + const toolItem: FlowToolItem = { + ...reviewTaskItem('running', 'ReviewWorker'), + toolName: 'LaunchReviewAgent', + subagentSessionId: 'review-session-unsafe-label', + toolCall: { + id: 'launch-review-call-internal-label', + input: { + description: 'ReviewWorker skill:private packet-7 src/auth.ts', + prompt: 'Internal worker prompt', + subagent_type: 'ReviewWorker', + focused_assignment: { + question: 'Is authentication enforced?', + independent_value: 'Independent validation', + target_fingerprint: 'fingerprint', + expected_evidence: 'Authentication checks', + capability_key: 'skill:private', + capability_fingerprint: 'internal-fingerprint', + allowed_changed_paths: ['src/auth.ts'], + }, + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('Checking a specific concern'); + expect(container.textContent).not.toContain('ReviewWorker'); + expect(container.textContent).not.toContain('skill:private'); + expect(container.textContent).not.toContain('packet-7'); + expect(container.textContent).not.toContain('src/auth.ts'); + }); + it('hides internal additional-check failure details', async () => { const toolItem: FlowToolItem = { ...reviewTaskItem('error', 'ReviewWorker'), @@ -503,6 +588,155 @@ describeWithJsdom('TaskToolDisplay', () => { expect(container.textContent).not.toContain('src/private.ts'); }); + it('shows partial-timeout Review results without exposing partial output', async () => { + const toolItem: FlowToolItem = { + ...reviewTaskItem('completed', 'ReviewWorker'), + toolName: 'LaunchReviewAgent', + toolResult: { + success: true, + result: { + duration: 31_000, + status: 'partial_timeout', + partial_output: 'private partial findings from src/private.ts', + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('Timed out after returning partial details'); + expect(container.textContent).not.toContain('private partial findings'); + expect(container.textContent).not.toContain('src/private.ts'); + expect(container.querySelector('.task-failed-badge')).toBeNull(); + }); + + it('shows a safe timeout outcome for a Review check', async () => { + const toolItem: FlowToolItem = { + ...reviewTaskItem('error', 'ReviewWorker'), + toolName: 'LaunchReviewAgent', + toolResult: { + success: false, + result: null, + error: 'provider timeout while reading src/private.ts', + duration_ms: 30_000, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('Timed out'); + expect(container.textContent).not.toContain('provider timeout'); + expect(container.textContent).not.toContain('src/private.ts'); + expect(container.querySelector('[data-completed-failure-reason="Timed out"]')).toBeTruthy(); + expect(container.querySelector('.task-failed-badge')).toBeNull(); + }); + + it('shows a stopped outcome for a cancelled Review check', async () => { + const toolItem: FlowToolItem = { + ...reviewTaskItem('completed', 'ReviewWorker'), + toolName: 'LaunchReviewAgent', + toolResult: { + success: true, + result: { + duration: 2_000, + status: 'cancelled', + reason: 'private cancellation detail', + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.textContent).toContain('Stopped'); + expect(container.textContent).not.toContain('private cancellation detail'); + expect(container.querySelector('.task-failed-badge')).toBeNull(); + }); + + it.each([ + { status: 'partial_timeout', label: 'Timed out after returning partial details' }, + { status: 'timed_out', label: 'Timed out' }, + { status: 'cancelled', label: 'Stopped' }, + ])('prefers a live child over a stale parent $status outcome', async ({ status, label }) => { + const toolItem: FlowToolItem = { + ...reviewTaskItem('completed', 'ReviewWorker', 'Check authentication'), + toolName: 'LaunchReviewAgent', + subagentSessionId: 'review-session-running', + toolCall: { + id: 'launch-review-live-child', + input: { + description: 'Check authentication', + prompt: 'Internal worker prompt', + subagent_type: 'ReviewWorker', + focused_assignment: { question: 'Is authentication enforced?' }, + }, + }, + toolResult: { + success: status === 'partial_timeout', + result: { status, duration: 30_000 }, + error: status === 'timed_out' ? 'request timed out' : undefined, + duration_ms: 30_000, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.querySelector('[data-testid="cube-loading"]')).toBeTruthy(); + expect(container.querySelector('.task-subagent-stop-button')).toBeTruthy(); + expect(container.querySelector('.task-review-outcome')).toBeNull(); + expect(container.querySelector('.task-failed-badge')).toBeNull(); + expect(container.textContent).not.toContain(label); + const indicator = container.querySelector('[data-testid="tool-timeout-indicator"]'); + expect(indicator?.getAttribute('data-completed-status')).toBeNull(); + expect(indicator?.getAttribute('data-completed-duration')).toBeNull(); + }); + + it.each([ + { childSessionId: 'review-session-completed' }, + { childSessionId: 'review-session-error' }, + { childSessionId: 'review-session-cancelled' }, + ])('does not keep running controls after child $childSessionId is terminal', async ({ childSessionId }) => { + const toolItem: FlowToolItem = { + ...reviewTaskItem('running', 'ReviewWorker', 'Check authentication'), + subagentSessionId: childSessionId, + toolCall: { + id: `task-${childSessionId}`, + input: { + description: 'Check authentication', + prompt: 'Internal worker prompt', + subagent_type: 'ReviewWorker', + packet_id: 'reviewer:security:group-1-of-1', + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.querySelector('[data-testid="cube-loading"]')).toBeNull(); + expect(container.querySelector('.task-subagent-stop-button')).toBeNull(); + expect(container.querySelector('[data-testid="tool-timeout-indicator"]') + ?.getAttribute('data-is-running')).toBe('false'); + }); + it('shows a background review as running while its child session is still processing', async () => { const toolItem: FlowToolItem = { ...reviewTaskItem('completed', 'CodeReview', 'Review CLI app layer diff'), @@ -791,7 +1025,7 @@ describeWithJsdom('TaskToolDisplay', () => { ); }); - it('keeps historical fixed-reviewer tasks in the Deep Review coverage view', async () => { + it('opens historical fixed-reviewer details in the real child session', async () => { const toolItem: FlowToolItem = { ...reviewTaskItem('completed', 'ReviewSecurity', 'Review authentication changes'), subagentSessionId: 'legacy-review-security-session', @@ -814,10 +1048,20 @@ describeWithJsdom('TaskToolDisplay', () => { openButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); }); - expect(mocks.openBtwSessionInAuxPane).not.toHaveBeenCalled(); + expect(mocks.openBtwSessionInAuxPane).toHaveBeenCalledWith( + expect.objectContaining({ + childSessionId: 'legacy-review-security-session', + parentSessionId: 'deep-review-parent-session', + sessionKind: 'subagent', + agentType: 'ReviewSecurity', + subagentType: 'ReviewSecurity', + viewKind: 'review-check', + includeInternal: true, + }), + ); }); - it('keeps a historical packetless ReviewJudge task in the coverage view', async () => { + it('opens a historical packetless ReviewJudge in the real child session', async () => { const toolItem: FlowToolItem = { ...reviewTaskItem('completed', 'ReviewJudge', 'Validate disputed findings'), subagentSessionId: 'legacy-review-judge-session', @@ -838,7 +1082,17 @@ describeWithJsdom('TaskToolDisplay', () => { openButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); }); - expect(mocks.openBtwSessionInAuxPane).not.toHaveBeenCalled(); + expect(mocks.openBtwSessionInAuxPane).toHaveBeenCalledWith( + expect.objectContaining({ + childSessionId: 'legacy-review-judge-session', + parentSessionId: 'deep-review-parent-session', + sessionKind: 'subagent', + agentType: 'ReviewJudge', + subagentType: 'ReviewJudge', + viewKind: 'review-check', + includeInternal: true, + }), + ); }); it('does not apply the historical reviewer fallback outside Deep Review', async () => { @@ -946,7 +1200,7 @@ describeWithJsdom('TaskToolDisplay', () => { }); it('stops a running foreground subagent from the task header', async () => { - mocks.cancelSession.mockResolvedValueOnce(undefined); + mocks.cancelSession.mockResolvedValueOnce({ cancelled: true, dialogTurnId: 'turn-1' }); const toolItem: FlowToolItem = { ...reviewTaskItem('running', 'Explore', 'Investigate task card behavior'), @@ -973,6 +1227,187 @@ describeWithJsdom('TaskToolDisplay', () => { expect(mocks.cancelSession).toHaveBeenCalledWith('subagent-session-1'); }); + it.each(['preparing', 'streaming'] as const)( + 'keeps ordinary foreground work stoppable while it is %s', + async (status) => { + const toolItem: FlowToolItem = { + ...reviewTaskItem(status, 'Explore', 'Investigate task state'), + subagentSessionId: 'subagent-session-1', + }; + + await act(async () => { + root.render( + , + ); + }); + + expect(container.querySelector('[data-testid="cube-loading"]')).toBeTruthy(); + expect(container.querySelector('.task-subagent-stop-button')).toBeTruthy(); + expect(container.querySelector('[data-testid="tool-timeout-indicator"]') + ?.getAttribute('data-is-running')).toBe('true'); + }, + ); + + it('clears ordinary foreground stopping state when cancellation is not confirmed', async () => { + mocks.cancelSession.mockResolvedValueOnce({ cancelled: false, dialogTurnId: null }); + const toolItem: FlowToolItem = { + ...reviewTaskItem('running', 'Explore', 'Investigate task state'), + subagentSessionId: 'subagent-session-1', + }; + + await act(async () => { + root.render( + , + ); + }); + + const stopButton = container.querySelector('.task-subagent-stop-button')!; + await act(async () => { + stopButton.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(stopButton.disabled).toBe(false); + expect(mocks.notificationError).toHaveBeenCalledWith( + 'toolCards.taskDetailPanel.stopSubagentFailed', + { duration: 5000 }, + ); + }); + + it('refreshes a stopped Review check only after cancellation is confirmed', async () => { + let resolveCancel!: (result: { cancelled: boolean; dialogTurnId?: string }) => void; + const cancelRequest = new Promise<{ cancelled: boolean; dialogTurnId?: string }>((resolve) => { + resolveCancel = resolve; + }); + mocks.cancelSession.mockReturnValueOnce(cancelRequest); + + const toolItem: FlowToolItem = { + ...reviewTaskItem('running', 'ReviewWorker', 'Check authentication changes'), + subagentSessionId: 'review-session-running', + toolCall: { + id: 'task-call-review-stop', + input: { + description: 'Check authentication changes', + prompt: 'Review the authentication changes', + subagent_type: 'ReviewWorker', + packet_id: 'reviewer:security:group-1-of-1', + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + const stopButton = container.querySelector('.task-subagent-stop-button'); + expect(stopButton).toBeTruthy(); + + await act(async () => { + stopButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(mocks.cancelSession).toHaveBeenCalledWith('review-session-running'); + expect(mocks.loadBtwSessionHistory).not.toHaveBeenCalled(); + + await act(async () => { + resolveCancel({ cancelled: true, dialogTurnId: 'turn-running' }); + await cancelRequest; + }); + + expect(mocks.loadBtwSessionHistory).toHaveBeenCalledWith({ + childSessionId: 'review-session-running', + workspacePath: 'D:\\workspace\\repo', + remoteConnectionId: 'remote-1', + remoteSshHost: 'host-1', + }); + }); + + it('keeps a Review check running locally when cancellation cannot be confirmed', async () => { + mocks.cancelSession.mockResolvedValueOnce({ cancelled: false, dialogTurnId: null }); + const toolItem: FlowToolItem = { + ...reviewTaskItem('running', 'ReviewWorker', 'Check authentication changes'), + subagentSessionId: 'review-session-running', + toolCall: { + id: 'task-call-review-stop-failed', + input: { + description: 'Check authentication changes', + prompt: 'Review the authentication changes', + subagent_type: 'ReviewWorker', + packet_id: 'reviewer:security:group-1-of-1', + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + const stopButton = container.querySelector('.task-subagent-stop-button'); + expect(stopButton).toBeTruthy(); + + await act(async () => { + stopButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(mocks.notificationError).toHaveBeenCalledWith( + 'toolCards.taskDetailPanel.stopReviewWorkFailed', + { duration: 5000 }, + ); + expect(mocks.loadBtwSessionHistory).toHaveBeenCalledWith({ + childSessionId: 'review-session-running', + workspacePath: 'D:\\workspace\\repo', + remoteConnectionId: 'remote-1', + remoteSshHost: 'host-1', + }); + }); + + it('does not report a stop failure when the Review check already completed', async () => { + mocks.cancelSession.mockResolvedValueOnce({ cancelled: false, dialogTurnId: null }); + mocks.loadBtwSessionHistory.mockImplementationOnce(async () => { + mocks.dynamicReviewTurn.status = 'completed'; + mocks.dynamicReviewTurn.endTime = 2200; + }); + const toolItem: FlowToolItem = { + ...reviewTaskItem('running', 'ReviewWorker', 'Check authentication changes'), + subagentSessionId: 'review-session-dynamic', + toolCall: { + id: 'task-call-review-stop-race', + input: { + description: 'Check authentication changes', + prompt: 'Review the authentication changes', + subagent_type: 'ReviewWorker', + packet_id: 'reviewer:security:group-1-of-1', + }, + }, + }; + + await act(async () => { + root.render( + , + ); + }); + + await act(async () => { + container.querySelector('.task-subagent-stop-button')! + .dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); + await Promise.resolve(); + }); + + expect(mocks.loadBtwSessionHistory).toHaveBeenCalledWith({ + childSessionId: 'review-session-dynamic', + workspacePath: 'D:\\workspace\\repo', + remoteConnectionId: 'remote-1', + remoteSshHost: 'host-1', + }); + expect(mocks.notificationError).not.toHaveBeenCalled(); + }); + it('does not show the foreground stop button for background subagents', async () => { const toolItem: FlowToolItem = { ...reviewTaskItem('running', 'Explore', 'Investigate background behavior'), diff --git a/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.tsx b/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.tsx index 3e9ed24f33..b8698cb9a0 100644 --- a/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.tsx +++ b/src/web-ui/src/flow_chat/tool-cards/TaskToolDisplay.tsx @@ -33,10 +33,11 @@ import { useToolCardHeightContract } from './useToolCardHeightContract'; import { ToolTimeoutIndicator } from './ToolTimeoutIndicator'; import { getReviewerContextBySubagentId } from '@/shared/services/reviewTeamService'; import type { ReviewerContext } from '@/shared/services/reviewTeamService'; -import { openBtwSessionInAuxPane } from '../services/btwSessionPane'; +import { loadBtwSessionHistory, openBtwSessionInAuxPane } from '../services/btwSessionPane'; import { flowChatStore } from '../store/FlowChatStore'; import { useSessionGoalModeActive } from '../hooks/useSessionGoalModeActive'; import { deriveSubagentExecutionStatus } from '../utils/subagentProjection'; +import { deriveReviewTaskOutcome } from '../utils/reviewTaskOutcome'; import { agentAPI } from '@/infrastructure/api/service-api/AgentAPI'; import { notificationService } from '@/shared/notification-system/services/NotificationService'; import './TaskToolDisplay.scss'; @@ -180,6 +181,8 @@ function readLinkedSubagentSnapshot(sessionId: string): string { session?.mode ?? '', session?.config?.agentType ?? '', session?.config?.modelName ?? '', + session?.focusedReviewDisplayLabel ?? '', + session?.deepReviewRunManifest?.focusedAssignment?.displayLabel ?? '', turn?.id ?? '', turn?.status ?? '', turn?.startTime ?? null, @@ -189,6 +192,17 @@ function readLinkedSubagentSnapshot(sessionId: string): string { ]); } +function readFocusedReviewDisplayLabel(sessionId: string): string { + if (!sessionId) { + return ''; + } + const session = flowChatStore.getState().sessions.get(sessionId); + return readStringValue( + session?.focusedReviewDisplayLabel + ?? session?.deepReviewRunManifest?.focusedAssignment?.displayLabel, + ); +} + const LEGACY_DEEP_REVIEWER_TYPES = new Set([ 'ReviewBusinessLogic', 'ReviewPerformance', @@ -355,6 +369,27 @@ export const TaskToolDisplay: React.FC = ({ const linkedSubagentSession = linkedSubagentSessionId ? flowChatStore.getState().sessions.get(linkedSubagentSessionId) : undefined; + const loadLinkedReviewHistory = useCallback(async () => { + if (!linkedSubagentSessionId) { + return; + } + const sessions = flowChatStore.getState().sessions; + const latestChild = sessions.get(linkedSubagentSessionId); + const latestParent = sessionId ? sessions.get(sessionId) : undefined; + const fallbackWorkspacePath = latestChild?.workspacePath + ? undefined + : latestParent?.workspacePath; + await loadBtwSessionHistory({ + childSessionId: linkedSubagentSessionId, + ...(fallbackWorkspacePath + ? { + workspacePath: fallbackWorkspacePath, + remoteConnectionId: latestParent?.remoteConnectionId, + remoteSshHost: latestParent?.remoteSshHost, + } + : {}), + }); + }, [linkedSubagentSessionId, sessionId]); const getTaskInput = () => { if (!toolCall?.input) return null; @@ -388,9 +423,12 @@ export const TaskToolDisplay: React.FC = ({ || readStringValue(toolCall.input.packetId); const isFocusedReview = hasFocusedReviewAssignment(toolCall.input) || (toolItem.toolName?.toLowerCase() === 'launchreviewagent' && !packetId); + const focusedDisplayLabel = isFocusedReview + ? readFocusedReviewDisplayLabel(linkedSubagentSessionId) + : ''; return { description: isFocusedReview - ? t('toolCards.taskTool.reviewFocusedDescription') + ? focusedDisplayLabel || t('toolCards.taskTool.reviewFocusedDescription') : t('toolCards.taskTool.reviewCoverageDescription'), prompt: 'Not provided', agentType: t('toolCards.taskTool.reviewCoverageLabel'), @@ -453,38 +491,65 @@ export const TaskToolDisplay: React.FC = ({ const linkedSubagentTurn = linkedSubagentSession?.dialogTurns?.[ linkedSubagentSession.dialogTurns.length - 1 ]; - const backgroundSubagentStatus = isBackgroundTask + const projectedSubagentStatus = isBackgroundTask || isReviewCoverageTask ? deriveSubagentExecutionStatus(linkedSubagentTurn) : null; - const backgroundSubagentIsRunning = backgroundSubagentStatus === 'running'; - const isCancelledResult = readTaskWasCancelled(status, toolResult); - const displayStatus = isCancelledResult + const projectedSubagentIsRunning = projectedSubagentStatus === 'running'; + const reviewTaskOutcome = isReviewCoverageTask && !projectedSubagentIsRunning + ? deriveReviewTaskOutcome(toolItem) + : null; + const isReviewPartialTimeout = reviewTaskOutcome === 'partial-timeout'; + const rawTaskErrorMessage = readTaskErrorMessage(toolResult); + const isReviewTimeout = reviewTaskOutcome === 'timed-out'; + const isCancelledResult = !projectedSubagentIsRunning && ( + readTaskWasCancelled(status, toolResult) || reviewTaskOutcome === 'stopped' + ); + const displayStatus = projectedSubagentIsRunning + ? 'running' + : isCancelledResult ? 'cancelled' - : backgroundSubagentStatus ?? status; - const isFailed = + : projectedSubagentStatus ?? status; + const effectiveIsRunning = projectedSubagentStatus == null + ? isRunning + : projectedSubagentIsRunning; + const isFailed = !projectedSubagentIsRunning && ( displayStatus === 'error' || ( - !isCancelledResult && - (status === 'error' || - (toolResult != null && - 'success' in toolResult && - toolResult.success === false))); - const backgroundSubagentDurationMs = isBackgroundTask && + !isCancelledResult && + (status === 'error' || + (toolResult != null && + 'success' in toolResult && + toolResult.success === false)) + ) + ); + const hasFailedOutcome = isFailed || isReviewTimeout; + const projectedSubagentDurationMs = (isBackgroundTask || isReviewCoverageTask) && linkedSubagentTurn?.endTime != null && linkedSubagentTurn.startTime != null ? Math.max(0, linkedSubagentTurn.endTime - linkedSubagentTurn.startTime) : undefined; - const taskDurationMs = isBackgroundTask - ? backgroundSubagentDurationMs + const taskDurationMs = projectedSubagentIsRunning + ? undefined + : isBackgroundTask || isReviewCoverageTask + ? projectedSubagentDurationMs ?? readTaskDurationMs(toolResult) : readTaskDurationMs(toolResult); const taskErrorMessage = displayStatus === 'error' - ? linkedSubagentTurn?.error || readTaskErrorMessage(toolResult) - : readTaskErrorMessage(toolResult); - const visibleTaskErrorMessage = isReviewCoverageTask && taskErrorMessage - ? t('toolCards.taskTool.reviewCheckUnavailable') + ? linkedSubagentTurn?.error || rawTaskErrorMessage + : rawTaskErrorMessage; + const visibleTaskErrorMessage = isReviewTimeout + ? t('toolCards.taskTool.reviewTimedOut') + : isReviewCoverageTask && taskErrorMessage + ? t('toolCards.taskTool.reviewCheckUnavailable') : taskErrorMessage; + const reviewOutcome = isReviewPartialTimeout + ? { key: 'toolCards.taskTool.reviewPartialTimeout', kind: 'partial-timeout' } + : isReviewTimeout + ? { key: 'toolCards.taskTool.reviewTimedOut', kind: 'timed-out' } + : isReviewCoverageTask && isCancelledResult + ? { key: 'toolCards.taskTool.reviewStopped', kind: 'stopped' } + : null; const completedDurationStatus = isCancelledResult || displayStatus === 'cancelled' ? 'cancelled' - : isFailed + : hasFailedOutcome ? 'error' : status === 'cancelled' || status === 'rejected' ? 'cancelled' @@ -505,18 +570,23 @@ export const TaskToolDisplay: React.FC = ({ || Boolean(resolvedSubagentModel) || isRunning ); + const stableSubagentType = readTaskSubagentType(toolCall?.input) + || readStringValue(linkedSubagentSession?.subagentType); + const stableAgentType = readStringValue(linkedSubagentSession?.mode) + || readStringValue(linkedSubagentSession?.config?.agentType) + || stableSubagentType; const canStopSyncSubagent = - isTaskTool && - isRunning && + (isTaskTool || isReviewCoverageTask) && + effectiveIsRunning && !isCancelAction && !isBackgroundTask && Boolean(linkedSubagentSessionId); useEffect(() => { - if (!isRunning || !linkedSubagentSessionId) { + if (!effectiveIsRunning || !linkedSubagentSessionId) { setIsStoppingSubagent(false); } - }, [isRunning, linkedSubagentSessionId]); + }, [effectiveIsRunning, linkedSubagentSessionId]); const handleStopSyncSubagent = useCallback(async (event: React.MouseEvent) => { event.stopPropagation(); @@ -525,15 +595,40 @@ export const TaskToolDisplay: React.FC = ({ } setIsStoppingSubagent(true); - try { - await agentAPI.cancelSession(linkedSubagentSessionId); - } catch (_error) { + const reportStopFailure = async () => { + if (isReviewCoverageTask) { + await loadLinkedReviewHistory().catch(() => undefined); + const latestSession = flowChatStore.getState().sessions.get(linkedSubagentSessionId); + const latestTurn = latestSession?.dialogTurns[latestSession.dialogTurns.length - 1]; + if (deriveSubagentExecutionStatus(latestTurn) !== 'running') { + setIsStoppingSubagent(false); + return; + } + } setIsStoppingSubagent(false); - notificationService.error(t('toolCards.taskDetailPanel.stopSubagentFailed'), { + notificationService.error(t(isReviewCoverageTask + ? 'toolCards.taskDetailPanel.stopReviewWorkFailed' + : 'toolCards.taskDetailPanel.stopSubagentFailed'), { duration: 5000, }); + }; + try { + const result = await agentAPI.cancelSession(linkedSubagentSessionId); + if (isReviewCoverageTask && !result.cancelled) { + await reportStopFailure(); + } else if (isReviewCoverageTask) { + await loadLinkedReviewHistory().catch(() => undefined); + setIsStoppingSubagent(false); + } else if (!result.cancelled) { + setIsStoppingSubagent(false); + notificationService.error(t('toolCards.taskDetailPanel.stopSubagentFailed'), { + duration: 5000, + }); + } + } catch (_error) { + await reportStopFailure(); } - }, [isStoppingSubagent, linkedSubagentSessionId, t]); + }, [isReviewCoverageTask, isStoppingSubagent, linkedSubagentSessionId, loadLinkedReviewHistory, t]); const handleCardClick = useCallback((e: React.MouseEvent) => { const target = e.target as HTMLElement; @@ -552,7 +647,7 @@ export const TaskToolDisplay: React.FC = ({ }, [isExpanded, updateCardExpandedState]); const showHeaderExpandHint = !isCancelAction && ( - isFailed || + hasFailedOutcome || hasInterruptionNote || hasRealPrompt || needsConfirmation || @@ -595,7 +690,7 @@ export const TaskToolDisplay: React.FC = ({ return; } - if (linkedSubagentSessionId && sessionId && !isReviewCoverageTask) { + if (linkedSubagentSessionId && sessionId) { const parentSession = flowChatStore.getState().sessions.get(sessionId); openBtwSessionInAuxPane({ childSessionId: linkedSubagentSessionId, @@ -603,12 +698,13 @@ export const TaskToolDisplay: React.FC = ({ workspacePath: parentSession?.workspacePath, sessionKind: 'subagent', sessionTitle: taskHeaderLine, - agentType: taskInput?.agentType, + agentType: stableAgentType || undefined, parentToolCallId: toolCall?.id || toolItem.id, - subagentType: taskInput?.agentType, + subagentType: stableSubagentType || undefined, remoteConnectionId: parentSession?.remoteConnectionId, remoteSshHost: parentSession?.remoteSshHost, includeInternal: true, + ...(isReviewCoverageTask ? { viewKind: 'review-check' as const } : {}), }); return; } @@ -626,7 +722,7 @@ export const TaskToolDisplay: React.FC = ({ window.dispatchEvent(new CustomEvent('agent-create-tab', { detail: tabInfo })); } }, - [isCancelAction, isReviewCoverageTask, linkedSubagentSessionId, onOpenInPanel, sessionId, taskInput, toolCall?.id, toolItem, taskHeaderLine], + [isCancelAction, isReviewCoverageTask, linkedSubagentSessionId, onOpenInPanel, sessionId, stableAgentType, stableSubagentType, taskInput, toolCall?.id, toolItem, taskHeaderLine], ); const renderToolIcon = () => { @@ -637,7 +733,7 @@ export const TaskToolDisplay: React.FC = ({
= ({
-
+
{showSubagentExecModel && resolvedSubagentModel ? ( <> @@ -660,7 +756,7 @@ export const TaskToolDisplay: React.FC = ({
0 ? toolCall.timeout_seconds * 1000 @@ -673,11 +769,20 @@ export const TaskToolDisplay: React.FC = ({ defaultTimeoutDisabled={defaultTimeoutDisabled} completedDurationMs={taskDurationMs} completedStatus={completedDurationStatus} - completedFailureReason={isFailed ? visibleTaskErrorMessage ?? undefined : undefined} + completedFailureReason={hasFailedOutcome ? visibleTaskErrorMessage ?? undefined : undefined} /> - {isFailed && ( + {hasFailedOutcome && !reviewOutcome && ( {t('toolCards.taskTool.failed')} )} + {reviewOutcome && ( + + {t(reviewOutcome.key)} + + )} {canStopSyncSubagent && (