From 066a44bc9d1a7c12baff57a70e91a3d8df6cf1d6 Mon Sep 17 00:00:00 2001 From: guantw Date: Sun, 6 Sep 2026 15:11:36 +0800 Subject: [PATCH 1/5] fix(review): localize untrusted repository errors Preserve repository ownership failures as a stable error code across Review platform commands and reuse the shared trust classifier. Translate the code into actionable localized copy and route user-initiated retries through the existing trust recovery flow. Cover trust classification and localized error handling with backend and frontend regression tests. --- .../desktop/src/api/review_platform_api.rs | 93 ++++++++--- .../core/src/service/review_platform/mod.rs | 8 +- .../services-integrations/src/git/trust.rs | 147 +++--------------- .../services/services-integrations/src/lib.rs | 3 + .../src/repository_trust.rs | 141 +++++++++++++++++ .../src/review_platform.rs | 78 +++++++++- .../review-platform/ReviewPlatformPanel.tsx | 42 +++-- .../shared/services/gitTrustService.test.ts | 13 ++ .../src/shared/services/gitTrustService.ts | 8 + .../src/tools/git/services/GitService.ts | 26 +--- 10 files changed, 373 insertions(+), 186 deletions(-) create mode 100644 src/crates/services/services-integrations/src/repository_trust.rs diff --git a/src/apps/desktop/src/api/review_platform_api.rs b/src/apps/desktop/src/api/review_platform_api.rs index ec2440630f..a951512c7a 100644 --- a/src/apps/desktop/src/api/review_platform_api.rs +++ b/src/apps/desktop/src/api/review_platform_api.rs @@ -3,10 +3,10 @@ use crate::api::app_state::AppState; use log::error; use openbitfun_core::service::review_platform::{ - ReviewPlatformCiLog, ReviewPlatformDetailSection, ReviewPlatformError, - ReviewPlatformIssueEvidence, ReviewPlatformKind, ReviewPlatformPullRequestDetail, - ReviewPlatformPullRequestDetailPage, ReviewPlatformPullRequestReviewTarget, - ReviewPlatformService, ReviewPlatformWorkspaceSnapshot, + untrusted_repository_error_message, ReviewPlatformCiLog, ReviewPlatformDetailSection, + ReviewPlatformError, ReviewPlatformIssueEvidence, ReviewPlatformKind, + ReviewPlatformPullRequestDetail, ReviewPlatformPullRequestDetailPage, + ReviewPlatformPullRequestReviewTarget, ReviewPlatformService, ReviewPlatformWorkspaceSnapshot, }; use serde::Deserialize; use tauri::State; @@ -88,10 +88,7 @@ pub async fn review_platform_get_workspace_snapshot( "Failed to get review platform workspace snapshot: path={}, remote_id={:?}, error={}", request.repository_path, request.remote_id, error ); - format!( - "Failed to get review platform workspace snapshot: {}", - error - ) + review_platform_command_error("Failed to get review platform workspace snapshot", &error) }) } @@ -104,10 +101,13 @@ pub async fn review_platform_get_workspace_context( .await .map_err(|error| { error!( - "Failed to get review platform workspace context: path={}, remote_id={:?}, error={}", - request.repository_path, request.remote_id, error - ); - format!("Failed to get review platform workspace context: {}", error) + "Failed to get review platform workspace context: path={}, remote_id={:?}, error={}", + request.repository_path, request.remote_id, error + ); + review_platform_command_error( + "Failed to get review platform workspace context", + &error, + ) }) } @@ -130,7 +130,10 @@ pub async fn review_platform_get_pull_request_detail( request.pull_request_id, error ); - format!("Failed to get review platform pull request detail: {}", error) + review_platform_command_error( + "Failed to get review platform pull request detail", + &error, + ) }) } @@ -153,7 +156,7 @@ pub async fn review_platform_get_pull_request_review_target( request.pull_request_id, error ); - format!("Failed to prepare pull request Review target: {}", error) + review_platform_command_error("Failed to prepare pull request Review target", &error) }) } @@ -182,7 +185,7 @@ pub async fn review_platform_get_issue( request.issue_id, safe_error ); - format!("Failed to get provider Issue evidence: {safe_error}") + safe_review_platform_command_error("Failed to get provider Issue evidence", &error) }) } @@ -209,10 +212,26 @@ pub async fn review_platform_get_pull_request_review_target_by_identity( request.pull_request_id, safe_error ); - format!("Failed to prepare pull request Review target: {safe_error}") + safe_review_platform_command_error("Failed to prepare pull request Review target", &error) }) } +fn review_platform_command_error(context: &str, error: &ReviewPlatformError) -> String { + if let Some(repository_path) = error.untrusted_repository_path() { + return untrusted_repository_error_message(repository_path); + } + + format!("{context}: {error}") +} + +fn safe_review_platform_command_error(context: &str, error: &ReviewPlatformError) -> String { + if let Some(repository_path) = error.untrusted_repository_path() { + return untrusted_repository_error_message(repository_path); + } + + format!("{context}: {}", safe_review_platform_error(error)) +} + fn safe_review_platform_error(error: &ReviewPlatformError) -> String { match error { ReviewPlatformError::Http { status, .. } => format!("provider returned HTTP {status}"), @@ -228,6 +247,9 @@ fn safe_review_platform_error(error: &ReviewPlatformError) -> String { "requested Issue is a pull request".to_string() } ReviewPlatformError::InvalidRepository(_) => "invalid repository".to_string(), + ReviewPlatformError::RepositoryUntrusted { .. } => { + "repository ownership is not trusted".to_string() + } ReviewPlatformError::RemoteNotFound(_) => "provider remote was not found".to_string(), ReviewPlatformError::UnsupportedPlatform(_) => "unsupported provider".to_string(), ReviewPlatformError::Api(_) => "provider request was rejected".to_string(), @@ -259,9 +281,9 @@ pub async fn review_platform_get_pull_request_detail_page( request.per_page, error ); - format!( - "Failed to get review platform pull request detail page: {}", - error + review_platform_command_error( + "Failed to get review platform pull request detail page", + &error, ) }) } @@ -288,7 +310,7 @@ pub async fn review_platform_get_pull_request_ci_log( request.ci_item_id, error ); - format!("Failed to get review platform CI log: {}", error) + review_platform_command_error("Failed to get review platform CI log", &error) }) } @@ -351,6 +373,37 @@ mod tests { use super::*; use serde_json::json; + #[test] + fn review_platform_command_errors_preserve_the_repository_trust_code() { + let error = ReviewPlatformError::RepositoryUntrusted { + repository_path: "/srv/shared/repo".to_string(), + detail: "fatal: detected dubious ownership".to_string(), + }; + + assert_eq!( + review_platform_command_error("Failed to load review platform", &error), + "git_repository_untrusted: /srv/shared/repo" + ); + assert_eq!( + safe_review_platform_command_error("Failed to load review platform", &error), + "git_repository_untrusted: /srv/shared/repo" + ); + } + + #[test] + fn review_platform_command_errors_keep_context_for_other_failures() { + let error = ReviewPlatformError::RemoteNotFound("origin".to_string()); + + assert_eq!( + review_platform_command_error("Failed to load review platform", &error), + "Failed to load review platform: Remote not found: origin" + ); + assert_eq!( + safe_review_platform_command_error("Failed to load review platform", &error), + "Failed to load review platform: provider remote was not found" + ); + } + #[test] fn review_platform_request_wire_deserializes_issue_identity_fields() { let request: ReviewPlatformIssueRequest = serde_json::from_value(json!({ diff --git a/src/crates/assembly/core/src/service/review_platform/mod.rs b/src/crates/assembly/core/src/service/review_platform/mod.rs index a7d991be72..12e2fbb8c5 100644 --- a/src/crates/assembly/core/src/service/review_platform/mod.rs +++ b/src/crates/assembly/core/src/service/review_platform/mod.rs @@ -9,8 +9,9 @@ use crate::infrastructure::try_get_path_manager_arc; use std::sync::Arc; pub use openbitfun_services_integrations::review_platform::{ - ReviewAuthSource, ReviewAuthState, ReviewChecks, ReviewDecision, ReviewEvidenceCompleteness, - ReviewFileStatus, ReviewItemState, ReviewPlatformAccount, ReviewPlatformActionResult, + classify_git_command_failure, untrusted_repository_error_message, ReviewAuthSource, + ReviewAuthState, ReviewChecks, ReviewDecision, ReviewEvidenceCompleteness, ReviewFileStatus, + ReviewItemState, ReviewPlatformAccount, ReviewPlatformActionResult, ReviewPlatformApprovalRequest, ReviewPlatformAuthChallenge, ReviewPlatformAuthChallengeState, ReviewPlatformCapabilities, ReviewPlatformCiItem, ReviewPlatformCiLog, ReviewPlatformCommit, ReviewPlatformCreatePullRequestRequest, ReviewPlatformDetailSection, ReviewPlatformError, @@ -105,7 +106,8 @@ impl ReviewPlatformWorkspaceClassifier for CoreReviewPlatformWorkspaceClassifier } else { stderr }; - return Err(ReviewPlatformError::InvalidRepository( + return Err(classify_git_command_failure( + current_dir, message.trim().to_string(), )); } diff --git a/src/crates/services/services-integrations/src/git/trust.rs b/src/crates/services/services-integrations/src/git/trust.rs index 5ea2a1701a..1a8db8cf46 100644 --- a/src/crates/services/services-integrations/src/git/trust.rs +++ b/src/crates/services/services-integrations/src/git/trust.rs @@ -24,14 +24,24 @@ use super::GitError; use serde::{Deserialize, Serialize}; use std::path::Path; -/// Ownership rejection wordings emitted by the Git CLI and by libgit2. -const OWNERSHIP_REJECTION_MARKERS: [&str; 5] = [ - "dubious ownership", - "is owned by someone else", - "is not owned by current user", - "not owned by current user", - "owned by a different user", -]; +pub const REPOSITORY_UNTRUSTED_ERROR_PREFIX: &str = + crate::repository_trust::REPOSITORY_UNTRUSTED_ERROR_PREFIX; + +pub fn is_untrusted_repository_message(message: &str) -> bool { + crate::repository_trust::is_untrusted_repository_message(message) +} + +pub fn normalize_trust_path(raw: &str) -> Option { + crate::repository_trust::normalize_trust_path(raw) +} + +pub fn untrusted_repository_path_from_message(message: &str) -> Option { + crate::repository_trust::untrusted_repository_path_from_message(message) +} + +pub fn untrusted_repository_error_message(repository_path: &str) -> String { + crate::repository_trust::untrusted_repository_error_message(repository_path) +} // Repository-level phrasings only. A bare "does not exist" also matches Git // talking about an object, a ref or a pathspec inside a repository that is @@ -89,11 +99,6 @@ fn contains_any(message: &str, markers: &[&str]) -> bool { markers.iter().any(|marker| lowered.contains(marker)) } -/// Whether a Git/libgit2 diagnostic is an ownership rejection. -pub fn is_untrusted_repository_message(message: &str) -> bool { - contains_any(message, &OWNERSHIP_REJECTION_MARKERS) -} - /// Whether a Git/libgit2 diagnostic says no repository is there. /// /// What is neither this nor [`is_untrusted_repository_message`] is a probe that @@ -105,110 +110,6 @@ pub fn is_missing_repository_message(message: &str) -> bool { contains_any(message, &MISSING_REPOSITORY_MARKERS) } -/// True when the value carries a shape only Windows produces: a `\\server\share` -/// or `\\?\...` prefix, or a `C:\` / `C:/` drive root. -/// -/// This is a shape test on purpose, not `cfg!(windows)`: a Windows desktop -/// driving a remote Linux workspace normalizes that host's paths, and a POSIX -/// host can be handed a Windows path by a peer. -fn looks_like_windows_path(value: &str) -> bool { - if value.starts_with('\\') { - return true; - } - let mut chars = value.chars(); - matches!( - (chars.next(), chars.next(), chars.next()), - (Some(drive), Some(':'), Some('\\' | '/')) if drive.is_ascii_alphabetic() - ) -} - -/// Normalizes a repository path into the shape Git compares `safe.directory` -/// entries against: forward slashes, no extended-length prefix, no trailing -/// separator. -pub fn normalize_trust_path(raw: &str) -> Option { - let trimmed = raw.trim().trim_matches('\'').trim_matches('"').trim(); - if trimmed.is_empty() { - return None; - } - - // Only a Windows-shaped path may have its backslashes rewritten. Backslash - // is an ordinary filename character on POSIX, and rewriting it would turn - // `/srv/we\ird/repo` into a `safe.directory` entry that can never match and - // a manual command naming a directory that does not exist. - let mut value = if looks_like_windows_path(trimmed) { - trimmed.replace('\\', "/") - } else { - trimmed.to_string() - }; - // `\\?\UNC\server\share` is the extended-length spelling of the UNC path - // `\\server\share`. Dropping the whole prefix would leave - // `UNC/server/share`, which Git never matches — and the manual command we - // hand the user would name a path that does not exist. - let unc_prefix = value - .get(..8) - .filter(|prefix| prefix.eq_ignore_ascii_case("//?/UNC/")); - if unc_prefix.is_some() { - value = format!("//{}", &value[8..]); - } else if let Some(stripped) = value.strip_prefix("//?/") { - value = stripped.to_string(); - } - while value.len() > 1 && value.ends_with('/') && !value.ends_with(":/") { - value.pop(); - } - - (!value.is_empty()).then_some(value) -} - -/// Extracts the repository path Git named in an ownership rejection. -/// -/// Both wordings quote the path first, before any remediation hint: -/// `detected dubious ownership in repository at ''` (CLI) and -/// `repository path '' is not owned by current user` (libgit2). -/// -/// Only the line carrying the rejection itself is read. The CLI's advice block -/// repeats the path on later lines, and reading those would make the result -/// depend on how far the prose was truncated. -pub fn untrusted_repository_path_from_message(message: &str) -> Option { - if !is_untrusted_repository_message(message) { - return None; - } - - message - .lines() - .filter(|line| contains_any(line, &OWNERSHIP_REJECTION_MARKERS)) - .find_map(quoted_path_on_line) - // A wording we have not seen may put the path on its own line. Falling - // back to any quoted span still beats losing the path entirely. - .or_else(|| message.lines().find_map(quoted_path_on_line)) -} - -/// Takes the widest quoted span on a line: first quote to last quote of the same -/// kind. -/// -/// Git does not escape quotes inside the path it prints, so stopping at the -/// first closing quote truncates `/srv/a'b/repo` to `/srv/a` — and that -/// truncated path outranks the caller's in `classify_command_failure`, so it is -/// what would be written into the user's global `safe.directory`, never -/// reclaimed, and still not resolve the rejection. -fn quoted_path_on_line(line: &str) -> Option { - for quote in ['\'', '"'] { - let Some(open) = line.find(quote) else { - continue; - }; - let start = open + quote.len_utf8(); - let Some(end) = line.rfind(quote) else { - continue; - }; - if end < start { - continue; - } - if let Some(path) = normalize_trust_path(&line[start..end]) { - return Some(path); - } - } - None -} - /// The command a user can run themselves when the product cannot apply the /// decision (remote workspace, peer host, restricted configuration). /// @@ -280,18 +181,6 @@ fn shell_single_quote(value: &str) -> String { format!("'{}'", value.replace('\'', r"'\''")) } -/// Stable prefix every interface uses to carry an ownership rejection across a -/// command boundary that only transports strings (Tauri `Result`, -/// JSON-RPC error `data`). Frontends branch on this instead of on prose, which -/// is localized by Git itself and differs between the CLI and libgit2. -pub const REPOSITORY_UNTRUSTED_ERROR_PREFIX: &str = "git_repository_untrusted:"; - -/// Boundary error string for an ownership rejection. One producer so the -/// desktop and app-server surfaces cannot drift apart on the wire. -pub fn untrusted_repository_error_message(repository_path: &str) -> String { - format!("{REPOSITORY_UNTRUSTED_ERROR_PREFIX} {repository_path}") -} - fn untrusted_error(repository_path: Option, detail: impl Into) -> GitError { GitError::RepositoryUntrusted { repository_path: repository_path.unwrap_or_default(), diff --git a/src/crates/services/services-integrations/src/lib.rs b/src/crates/services/services-integrations/src/lib.rs index ef0bfd48f8..7ca6e7e521 100644 --- a/src/crates/services/services-integrations/src/lib.rs +++ b/src/crates/services/services-integrations/src/lib.rs @@ -72,6 +72,9 @@ pub mod miniapp_market; #[cfg(feature = "plugin-source")] pub mod plugin_source; +#[cfg(any(feature = "git", feature = "review-platform"))] +mod repository_trust; + #[cfg(feature = "remote-connect")] pub mod remote_connect; diff --git a/src/crates/services/services-integrations/src/repository_trust.rs b/src/crates/services/services-integrations/src/repository_trust.rs new file mode 100644 index 0000000000..5f59daf55e --- /dev/null +++ b/src/crates/services/services-integrations/src/repository_trust.rs @@ -0,0 +1,141 @@ +//! Feature-light Git repository ownership trust contract. +//! +//! Both the full Git service and Review Platform execute Git commands, but +//! Review Platform deliberately remains independently compilable without the +//! heavier `git` feature. Keep the ownership diagnostic classification and +//! stable boundary code here so those two capability slices cannot drift. + +/// Ownership rejection wordings emitted by the Git CLI and by libgit2. +const OWNERSHIP_REJECTION_MARKERS: [&str; 5] = [ + "dubious ownership", + "is owned by someone else", + "is not owned by current user", + "not owned by current user", + "owned by a different user", +]; + +fn contains_any(message: &str, markers: &[&str]) -> bool { + let lowered = message.to_lowercase(); + markers.iter().any(|marker| lowered.contains(marker)) +} + +/// Whether a Git/libgit2 diagnostic is an ownership rejection. +pub(crate) fn is_untrusted_repository_message(message: &str) -> bool { + contains_any(message, &OWNERSHIP_REJECTION_MARKERS) +} + +/// True when the value carries a shape only Windows produces: a `\\server\share` +/// or `\\?\...` prefix, or a `C:\` / `C:/` drive root. +fn looks_like_windows_path(value: &str) -> bool { + if value.starts_with('\\') { + return true; + } + let mut chars = value.chars(); + matches!( + (chars.next(), chars.next(), chars.next()), + (Some(drive), Some(':'), Some('\\' | '/')) if drive.is_ascii_alphabetic() + ) +} + +/// Normalizes a repository path into the shape Git compares `safe.directory` +/// entries against: forward slashes, no extended-length prefix, no trailing +/// separator. +pub(crate) fn normalize_trust_path(raw: &str) -> Option { + let trimmed = raw.trim().trim_matches('\'').trim_matches('"').trim(); + if trimmed.is_empty() { + return None; + } + + // Only a Windows-shaped path may have its backslashes rewritten. Backslash + // is an ordinary filename character on POSIX. + let mut value = if looks_like_windows_path(trimmed) { + trimmed.replace('\\', "/") + } else { + trimmed.to_string() + }; + if value + .get(..8) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case("//?/UNC/")) + { + value = format!("//{}", &value[8..]); + } else if let Some(stripped) = value.strip_prefix("//?/") { + value = stripped.to_string(); + } + while value.len() > 1 && value.ends_with('/') && !value.ends_with(":/") { + value.pop(); + } + + (!value.is_empty()).then_some(value) +} + +/// Extracts the repository path Git named in an ownership rejection. +pub(crate) fn untrusted_repository_path_from_message(message: &str) -> Option { + if !is_untrusted_repository_message(message) { + return None; + } + + message + .lines() + .filter(|line| contains_any(line, &OWNERSHIP_REJECTION_MARKERS)) + .find_map(quoted_path_on_line) + .or_else(|| message.lines().find_map(quoted_path_on_line)) +} + +fn quoted_path_on_line(line: &str) -> Option { + for quote in ['\'', '"'] { + let Some(open) = line.find(quote) else { + continue; + }; + let start = open + quote.len_utf8(); + let Some(end) = line.rfind(quote) else { + continue; + }; + if end < start { + continue; + } + if let Some(path) = normalize_trust_path(&line[start..end]) { + return Some(path); + } + } + None +} + +/// Stable prefix used across string-only Desktop and JSON-RPC boundaries. +pub(crate) const REPOSITORY_UNTRUSTED_ERROR_PREFIX: &str = "git_repository_untrusted:"; + +/// Boundary error string for an ownership rejection. +pub(crate) fn untrusted_repository_error_message(repository_path: &str) -> String { + format!("{REPOSITORY_UNTRUSTED_ERROR_PREFIX} {repository_path}") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn classifies_cli_ownership_rejection_and_preserves_the_path() { + let message = concat!( + "fatal: detected dubious ownership in repository at '/srv/shared/repo'\n", + "To add an exception for this directory, call:\n", + "git config --global --add safe.directory /srv/shared/repo", + ); + + assert!(is_untrusted_repository_message(message)); + assert_eq!( + untrusted_repository_path_from_message(message).as_deref(), + Some("/srv/shared/repo") + ); + } + + #[test] + fn normalizes_windows_paths_without_rewriting_posix_backslashes() { + assert_eq!( + normalize_trust_path("\\\\?\\C:\\work\\repo\\").as_deref(), + Some("C:/work/repo") + ); + assert_eq!( + normalize_trust_path("/srv/we\\ird/repo").as_deref(), + Some("/srv/we\\ird/repo") + ); + } +} diff --git a/src/crates/services/services-integrations/src/review_platform.rs b/src/crates/services/services-integrations/src/review_platform.rs index fc3030a241..d2b7bec237 100644 --- a/src/crates/services/services-integrations/src/review_platform.rs +++ b/src/crates/services/services-integrations/src/review_platform.rs @@ -4,6 +4,9 @@ //! and provider-neutral review-platform response semantics. Concrete HTTP //! transport lives in `review_platform_http`. +use crate::repository_trust::{ + is_untrusted_repository_message, normalize_trust_path, untrusted_repository_path_from_message, +}; use crate::review_platform_http::{ send_json as send_review_json, send_json_response as send_review_json_response, send_json_response_bounded as send_review_json_response_bounded, @@ -64,6 +67,11 @@ static TOKEN_STORE_TEMP_NONCE: std::sync::atomic::AtomicU64 = std::sync::atomic: pub enum ReviewPlatformError { #[error("Invalid repository path: {0}")] InvalidRepository(String), + #[error("Repository ownership is not trusted: {repository_path}")] + RepositoryUntrusted { + repository_path: String, + detail: String, + }, #[error("Remote not found: {0}")] RemoteNotFound(String), #[error("Unsupported review platform: {0}")] @@ -84,6 +92,38 @@ pub enum ReviewPlatformError { TargetIsPullRequest { issue_id: String }, } +impl ReviewPlatformError { + pub fn untrusted_repository_path(&self) -> Option<&str> { + match self { + Self::RepositoryUntrusted { + repository_path, .. + } => Some(repository_path), + _ => None, + } + } +} + +/// Stable boundary error for a repository ownership rejection. +pub fn untrusted_repository_error_message(repository_path: &str) -> String { + crate::repository_trust::untrusted_repository_error_message(repository_path) +} + +/// Classifies a failed Git probe without making Review Platform depend on the +/// full Git capability feature. +pub fn classify_git_command_failure(repository_path: &str, message: String) -> ReviewPlatformError { + if is_untrusted_repository_message(&message) { + let repository_path = untrusted_repository_path_from_message(&message) + .or_else(|| normalize_trust_path(repository_path)) + .unwrap_or_else(|| repository_path.to_string()); + return ReviewPlatformError::RepositoryUntrusted { + repository_path, + detail: message, + }; + } + + ReviewPlatformError::InvalidRepository(message) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum ReviewPlatformKind { @@ -4385,7 +4425,7 @@ async fn execute_git_command( } else { String::from_utf8_lossy(&output.stderr).to_string() }; - Err(ReviewPlatformError::InvalidRepository(message)) + Err(classify_git_command_failure(current_dir, message)) } fn review_evidence_error(error: ReviewPlatformError, resource: &str) -> ReviewPlatformError { @@ -7487,6 +7527,42 @@ mod tests { }; use tokio::fs; + #[test] + fn git_ownership_rejection_keeps_a_stable_repository_trust_contract() { + let detail = concat!( + "fatal: detected dubious ownership in repository at '/srv/shared/repo'\n", + "To add an exception for this directory, call:\n", + "git config --global --add safe.directory /srv/shared/repo", + ); + + let error = classify_git_command_failure("/srv/controller/path", detail.to_string()); + + assert!(matches!( + error, + ReviewPlatformError::RepositoryUntrusted { + ref repository_path, + detail: ref captured_detail, + } if repository_path == "/srv/shared/repo" && captured_detail == detail + )); + assert_eq!(error.untrusted_repository_path(), Some("/srv/shared/repo")); + assert_eq!( + untrusted_repository_error_message("/srv/shared/repo"), + "git_repository_untrusted: /srv/shared/repo" + ); + } + + #[test] + fn ordinary_git_failure_remains_an_invalid_repository_error() { + let error = + classify_git_command_failure("/srv/project", "fatal: not a git repository".to_string()); + + assert!(matches!( + error, + ReviewPlatformError::InvalidRepository(ref detail) + if detail == "fatal: not a git repository" + )); + } + struct AlwaysRemoteWorkspace; #[async_trait::async_trait] diff --git a/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx b/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx index ee21a32f02..feb932b4a6 100644 --- a/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx +++ b/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx @@ -37,6 +37,10 @@ import { findLatestCodeReviewResultState, summarizeCodeReviewResult } from '@/fl import { parsePullRequestUrl, remoteMatchesPullRequestLink } from '@/shared/utils/pullRequestLinks'; import { useContextStore } from '@/shared/stores/contextStore'; import { quickActions } from '@/shared/services/ide-control'; +import { + describeGitTrustFailure, + withGitRepositoryTrustRecovery, +} from '@/shared/services/gitTrustService'; import type { PullRequestContext } from '@/shared/types/context'; import { currentPullRequestReviewStatusText, @@ -136,6 +140,11 @@ const detailPageCache = new Map(); const reviewLaunchesInFlight = new Set(); const EMPTY_REVIEW_THREADS: ReviewPlatformThread[] = []; +function reviewPlatformErrorMessage(error: unknown, fallback: string): string { + return describeGitTrustFailure(error) + ?? (error instanceof Error ? error.message : fallback); +} + function detailPageInfo(pagination: ReviewPlatformPagination, itemCount: number): PageInfo { const pageIndex = Math.max(0, (pagination.page || 1) - 1); const perPage = Math.max(1, pagination.perPage || itemCount || 1); @@ -756,7 +765,10 @@ export const ReviewPlatformPanel: React.FC = ({ [account, snapshot.remotes], ); - const loadSnapshot = useCallback(async (nextRemoteId?: string | null, options?: { force?: boolean; page?: number }) => { + const loadSnapshot = useCallback(async ( + nextRemoteId?: string | null, + options?: { force?: boolean; page?: number; userInitiated?: boolean }, + ) => { const requestSeq = ++snapshotRequestSeq.current; if (!workspacePath) { setSnapshot(emptySnapshot()); @@ -808,9 +820,17 @@ export const ReviewPlatformPanel: React.FC = ({ setLoading(true); setError(null); try { - const next = detailOnly - ? await reviewPlatformAPI.getWorkspaceContext(workspacePath, requestedRemoteId ?? null) - : await reviewPlatformAPI.getWorkspaceSnapshot(workspacePath, requestedRemoteId ?? null, requestedPage, PR_PAGE_SIZE); + const next = await withGitRepositoryTrustRecovery( + () => detailOnly + ? reviewPlatformAPI.getWorkspaceContext(workspacePath, requestedRemoteId ?? null) + : reviewPlatformAPI.getWorkspaceSnapshot( + workspacePath, + requestedRemoteId ?? null, + requestedPage, + PR_PAGE_SIZE, + ), + { userInitiated: options?.userInitiated }, + ); if (snapshotRequestSeq.current !== requestSeq) return; setSnapshot(next); const remoteId = next.selectedRemoteId ?? next.remotes[0]?.id ?? null; @@ -829,7 +849,7 @@ export const ReviewPlatformPanel: React.FC = ({ setSnapshotCacheState('cached'); } catch (err) { if (snapshotRequestSeq.current !== requestSeq) return; - const message = err instanceof Error ? err.message : 'Failed to load pull requests'; + const message = reviewPlatformErrorMessage(err, 'Failed to load pull requests'); setError(message); if (!cached) { setSnapshot(emptySnapshot()); @@ -883,7 +903,7 @@ export const ReviewPlatformPanel: React.FC = ({ } catch (err) { if (detailRequestSeq.current !== requestSeq) return; log.error('Failed to load pull request detail', { pullRequestId, error: err }); - setDetailError(err instanceof Error ? err.message : 'Failed to load pull request details.'); + setDetailError(reviewPlatformErrorMessage(err, 'Failed to load pull request details.')); if (!cached) { setDetail(null); } @@ -961,7 +981,7 @@ export const ReviewPlatformPanel: React.FC = ({ } catch (err) { if (detailSectionRequestSeq.current !== requestSeq) return; log.error('Failed to load pull request detail section', { pullRequestId, section, page, perPage, error: err }); - setDetailError(err instanceof Error ? err.message : 'Failed to load pull request details.'); + setDetailError(reviewPlatformErrorMessage(err, 'Failed to load pull request details.')); } finally { if (detailSectionRequestSeq.current === requestSeq) { setDetailLoading(false); @@ -1372,7 +1392,7 @@ export const ReviewPlatformPanel: React.FC = ({ setCiLogById(prev => ({ ...prev, [item.id]: nextLog })); return nextLog; } catch (err) { - const message = err instanceof Error ? err.message : 'Failed to load CI error log.'; + const message = reviewPlatformErrorMessage(err, 'Failed to load CI error log.'); setCiLogErrorById(prev => ({ ...prev, [item.id]: message })); log.error('Failed to load CI log', { itemId: item.id, error: err }); return null; @@ -1910,7 +1930,7 @@ export const ReviewPlatformPanel: React.FC = ({ aria-label="Refresh" className="review-platform__icon-button" size="sm" - onClick={() => void loadSnapshot(listRemoteId, { force: true, page: currentPageIndex + 1 })} + onClick={() => void loadSnapshot(listRemoteId, { force: true, page: currentPageIndex + 1, userInitiated: true })} loading={loading} icon={} /> @@ -1980,7 +2000,7 @@ export const ReviewPlatformPanel: React.FC = ({
{error} -
@@ -2108,7 +2128,7 @@ export const ReviewPlatformPanel: React.FC = ({ diff --git a/src/web-ui/src/shared/services/gitTrustService.test.ts b/src/web-ui/src/shared/services/gitTrustService.test.ts index 40c85e02cb..ecda6aee2c 100644 --- a/src/web-ui/src/shared/services/gitTrustService.test.ts +++ b/src/web-ui/src/shared/services/gitTrustService.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { TauriCommandError } from '@/infrastructure/api/errors/TauriCommandError'; import { + describeGitTrustFailure, requestGitRepositoryTrust, resetGitTrustDecisions, withGitRepositoryTrustRecovery, @@ -67,6 +68,18 @@ beforeEach(() => { successMock.mockReset(); }); +describe('describeGitTrustFailure', () => { + it('turns the stable repository trust code into localized copy', () => { + expect(describeGitTrustFailure(untrustedError())).toBe( + `panels/git:trust.required|${JSON.stringify({ path: REPOSITORY_PATH })}`, + ); + }); + + it('leaves unrelated failures to the calling surface', () => { + expect(describeGitTrustFailure(new Error('provider unavailable'))).toBeUndefined(); + }); +}); + describe('requestGitRepositoryTrust', () => { it('grants trust only after the user confirms', async () => { confirmWarningMock.mockResolvedValue(true); diff --git a/src/web-ui/src/shared/services/gitTrustService.ts b/src/web-ui/src/shared/services/gitTrustService.ts index 3ddb777c06..4f70b93ae9 100644 --- a/src/web-ui/src/shared/services/gitTrustService.ts +++ b/src/web-ui/src/shared/services/gitTrustService.ts @@ -49,6 +49,14 @@ const promptQuietUntil = new Map(); */ const promptKey = repositoryPathKey; +/** Names an ownership rejection with localized, actionable copy. */ +export function describeGitTrustFailure(failure: unknown): string | undefined { + const repositoryPath = gitRepositoryUntrustedPath(failure); + return repositoryPath + ? i18nService.t('panels/git:trust.required', { path: repositoryPath }) + : undefined; +} + /** Test seam: forgets in-flight prompts and remembered decisions. */ export function resetGitTrustDecisions(): void { inFlightRequests.clear(); diff --git a/src/web-ui/src/tools/git/services/GitService.ts b/src/web-ui/src/tools/git/services/GitService.ts index cc8464a8dc..37c8d49231 100644 --- a/src/web-ui/src/tools/git/services/GitService.ts +++ b/src/web-ui/src/tools/git/services/GitService.ts @@ -3,15 +3,15 @@ */ import { gitAPI } from '@/infrastructure/api'; -import { - gitRepositoryUntrustedPath, - isGitRepositoryUntrustedError, -} from '@/infrastructure/api/errors/TauriCommandError'; +import { isGitRepositoryUntrustedError } from '@/infrastructure/api/errors/TauriCommandError'; import { createLogger } from '@/shared/utils/logger'; import { measureAsync } from '@/shared/utils/timing'; import { i18nService } from '@/infrastructure/i18n'; +import { describeGitTrustFailure } from '@/shared/services/gitTrustService'; import { gitStateManager } from '../state/GitStateManager'; +export { describeGitTrustFailure } from '@/shared/services/gitTrustService'; + const log = createLogger('GitService'); export type { GitRepository, @@ -44,24 +44,6 @@ import { GitLogParams } from '../types'; -/** - * Names the ownership wall behind a failed Git operation, or `undefined` when - * that is not what went wrong. - * - * The rejection reaches a caller as a stable code from two directions: a local - * executor throws it, and a remote one returns it in `result.error`. Neither is - * a sentence. Passing it through puts `git_repository_untrusted: - * /srv/shared/repo` in the panel next to a commit that did not happen, while - * the very same wall on a status read names the repository and points at the - * way out. - */ -export function describeGitTrustFailure(failure: unknown): string | undefined { - const repositoryPath = gitRepositoryUntrustedPath(failure); - return repositoryPath - ? i18nService.t('panels/git:trust.required', { path: repositoryPath }) - : undefined; -} - export class GitService { private static instance: GitService; From 6300dd48bc30eebf61dffa8a7f3ee38dbaf9ebee Mon Sep 17 00:00:00 2001 From: guantw Date: Sun, 6 Sep 2026 15:11:36 +0800 Subject: [PATCH 2/5] fix(review): localize action start failures Translate dialog-turn start failures in the Review action bar while preserving specific backend reasons. Allow multiline error details to wrap without truncation using the current theme tokens. Cover error classification, localization, component wiring, and layout behavior. --- .../btw/DeepReviewActionBar.i18n.test.ts | 2 ++ .../components/btw/DeepReviewActionBar.scss | 15 ++++++----- .../btw/DeepReviewActionBar.test.tsx | 27 +++++++++++++++++++ .../btw/DeepReviewActionBarLayout.test.ts | 16 +++++++++++ .../action-bar/DeepReviewActionBar.tsx | 22 +++++++++++++-- .../action-bar/actionBarFormatting.test.ts | 26 +++++++++++++++++- .../action-bar/actionBarFormatting.ts | 23 ++++++++++++++++ src/web-ui/src/locales/en-US/flow-chat.json | 2 ++ src/web-ui/src/locales/zh-CN/flow-chat.json | 2 ++ src/web-ui/src/locales/zh-TW/flow-chat.json | 2 ++ 10 files changed, 127 insertions(+), 10 deletions(-) diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts index 33eee758fb..85bc550057 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.i18n.test.ts @@ -17,6 +17,8 @@ const REQUIRED_ACTION_BAR_KEYS = [ 'deepReviewActionBar.managedCoverageProgress', 'deepReviewActionBar.managedCoverageDeferred', 'deepReviewActionBar.fixAndReviewRunning', + 'deepReviewActionBar.actionStartFailed', + 'deepReviewActionBar.actionStartFailedWithReason', 'deepReviewActionBar.minimizedStandard', 'deepReviewActionBar.minimizedReviewRunningDeep', 'deepReviewActionBar.minimizedReviewRunningStandard', diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss index 68ead8797b..5073ef2b26 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.scss @@ -126,8 +126,9 @@ /* Status header */ &__status { - display: flex; - align-items: center; + display: grid; + grid-template-columns: auto minmax(0, 1fr); + align-items: start; gap: 6px; padding-right: 136px; } @@ -154,19 +155,19 @@ } &__status-title { + min-width: 0; font-weight: var(--openbitfun-type-label-selected-font-weight); font-size: var(--openbitfun-type-flow-control-font-size); color: var(--openbitfun-color-content-primary); } &__error-message { - margin-left: auto; + grid-column: 2; + min-width: 0; font-size: var(--openbitfun-type-flow-control-font-size); color: var(--openbitfun-color-status-danger-content); - max-width: 50%; - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; + overflow-wrap: anywhere; + white-space: pre-wrap; } /* Remediation section */ diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx index a02c0a25db..a10d37a2cd 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx @@ -292,6 +292,33 @@ describeWithJsdom('DeepReviewActionBar', () => { expect(container.querySelector('[role="status"]')).toBeTruthy(); }); + it('localizes the stable dialog-start prefix without translating provider details', async () => { + const store = useReviewActionBarStore.getState(); + store.showActionBar({ + childSessionId: 'child-session', + parentSessionId: 'parent-session', + reviewData: { + summary: { recommended_action: 'request_changes' }, + remediation_plan: ['Fix the provider failure.'], + }, + phase: 'fix_failed', + }); + store.updatePhase( + 'fix_failed', + 'Failed to start dialog turn: provider quota exhausted', + 'child-session', + ); + + await act(async () => { + root.render(); + }); + + expect(container.textContent).toContain( + 'Unable to start this action: provider quota exhausted', + ); + expect(container.textContent).not.toContain('Failed to start dialog turn:'); + }); + it('keeps remediation in progress after submitting a fix turn', async () => { flowChatSessionsMock.set('child-session', { sessionId: 'child-session', diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBarLayout.test.ts b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBarLayout.test.ts index 4b3a1b3504..aaf2b94992 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBarLayout.test.ts +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBarLayout.test.ts @@ -51,4 +51,20 @@ describe('DeepReviewActionBar layout styles', () => { expect(stylesheet).not.toContain('--deep-review-action-bar-scrollbar-gutter'); expect(actions).not.toContain('calc('); }); + + it('keeps complete review and fix error details readable', () => { + const stylesheet = readActionBarStylesheet(); + const status = extractBlock(stylesheet, '&__status'); + const errorMessage = extractBlock(stylesheet, '&__error-message'); + + expect(status).toContain('display: grid;'); + expect(status).toContain('grid-template-columns: auto minmax(0, 1fr);'); + expect(errorMessage).toContain('grid-column: 2;'); + expect(errorMessage).toContain('overflow-wrap: anywhere;'); + expect(errorMessage).toContain('white-space: pre-wrap;'); + expect(errorMessage).not.toContain('overflow: hidden;'); + expect(errorMessage).not.toContain('text-overflow: ellipsis;'); + expect(errorMessage).not.toContain('white-space: nowrap;'); + expect(errorMessage).not.toContain('max-width: 50%;'); + }); }); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx b/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx index bb387834f6..d378f8f8be 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx @@ -45,7 +45,10 @@ import { isTauriRuntime } from '@/infrastructure/runtime'; import { useSettingsStore } from '@/app/scenes/settings/settingsStore'; import { useSceneStore } from '@/app/stores/sceneStore'; import type { SettingsPageId } from '@/app/scenes/settings/settingsTypes'; -import { formatElapsedTime } from './actionBarFormatting'; +import { + classifyReviewActionErrorMessage, + formatElapsedTime, +} from './actionBarFormatting'; import { CapacityQueueNotice } from './CapacityQueueNotice'; import { DecisionExecutionGate } from './DecisionExecutionGate'; import { buildInterruptionDiagnostics } from './interruptionDiagnostics'; @@ -829,6 +832,21 @@ export const ReviewActionBar: React.FC = ({ childSessionId } }, []); + const displayErrorMessage = useMemo(() => { + if (!errorMessage) return null; + + const presentation = classifyReviewActionErrorMessage(errorMessage); + if (presentation.kind === 'raw') { + return presentation.message; + } + + return presentation.reason + ? t('deepReviewActionBar.actionStartFailedWithReason', { + reason: presentation.reason, + }) + : t('deepReviewActionBar.actionStartFailed'); + }, [errorMessage, t]); + const handleCopyDiagnostics = useCallback(async () => { const detail = interruption?.errorDetail; if (!detail) return; @@ -924,7 +942,7 @@ export const ReviewActionBar: React.FC = ({ childSessionId PhaseIcon={PhaseIcon} phaseIconClass={phaseConfig.iconClass} phaseTitle={phaseTitle} - errorMessage={errorMessage} + errorMessage={displayErrorMessage} minimizeLabel={t('deepReviewActionBar.minimize')} onMinimize={handleMinimize} /> diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts index 1780866cf8..95f0ee8d54 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from 'vitest'; -import { formatElapsedTime } from './actionBarFormatting'; +import { + classifyReviewActionErrorMessage, + formatElapsedTime, +} from './actionBarFormatting'; describe('action bar formatting', () => { it('formats elapsed milliseconds without changing existing labels', () => { @@ -8,4 +11,25 @@ describe('action bar formatting', () => { expect(formatElapsedTime(60_000)).toBe('1m 0s'); expect(formatElapsedTime(125_000)).toBe('2m 5s'); }); + + it('classifies only the stable product-owned dialog-start prefix', () => { + expect(classifyReviewActionErrorMessage( + 'Failed to start dialog turn: provider quota exhausted', + )).toEqual({ + kind: 'start_dialog_turn_failed', + reason: 'provider quota exhausted', + }); + + expect(classifyReviewActionErrorMessage('Failed to start dialog turn:')).toEqual({ + kind: 'start_dialog_turn_failed', + reason: null, + }); + }); + + it('keeps arbitrary backend and provider details as raw display data', () => { + expect(classifyReviewActionErrorMessage('Provider request failed in region us-east')).toEqual({ + kind: 'raw', + message: 'Provider request failed in region us-east', + }); + }); }); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts index 7cc4cef63c..e0d490365a 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts @@ -1,3 +1,26 @@ +const START_DIALOG_TURN_ERROR_PREFIX = 'Failed to start dialog turn:'; + +export type ReviewActionErrorPresentation = + | { kind: 'start_dialog_turn_failed'; reason: string | null } + | { kind: 'raw'; message: string }; + +export function classifyReviewActionErrorMessage(message: string): ReviewActionErrorPresentation { + const normalizedMessage = message.trim(); + + if (normalizedMessage.startsWith(START_DIALOG_TURN_ERROR_PREFIX)) { + const reason = normalizedMessage.slice(START_DIALOG_TURN_ERROR_PREFIX.length).trim(); + return { + kind: 'start_dialog_turn_failed', + reason: reason || null, + }; + } + + return { + kind: 'raw', + message: normalizedMessage, + }; +} + export function formatElapsedTime(ms: number): string { const seconds = Math.floor(ms / 1000); const minutes = Math.floor(seconds / 60); diff --git a/src/web-ui/src/locales/en-US/flow-chat.json b/src/web-ui/src/locales/en-US/flow-chat.json index e83db724eb..a0c8a88b87 100644 --- a/src/web-ui/src/locales/en-US/flow-chat.json +++ b/src/web-ui/src/locales/en-US/flow-chat.json @@ -1093,6 +1093,8 @@ "fixRunning": "Fixing in progress...", "fixCompleted": "Fix completed", "fixFailed": "Fix failed", + "actionStartFailed": "Unable to start this action.", + "actionStartFailedWithReason": "Unable to start this action: {{reason}}", "fixTimeout": "Fix timed out", "fixInterrupted": "Fix was interrupted. Up to {{count}} selected items may still need attention.", "reviewWaitingCapacity": "Review waiting for capacity", diff --git a/src/web-ui/src/locales/zh-CN/flow-chat.json b/src/web-ui/src/locales/zh-CN/flow-chat.json index 4fe5668a48..e7998faf36 100644 --- a/src/web-ui/src/locales/zh-CN/flow-chat.json +++ b/src/web-ui/src/locales/zh-CN/flow-chat.json @@ -1093,6 +1093,8 @@ "fixRunning": "正在修复...", "fixCompleted": "修复完成", "fixFailed": "修复失败", + "actionStartFailed": "无法开始此次操作。", + "actionStartFailedWithReason": "无法开始此次操作：{{reason}}", "fixTimeout": "修复超时", "fixInterrupted": "修复已中断，最多有 {{count}} 个已选项目可能仍需处理。", "reviewWaitingCapacity": "审核正在等待容量", diff --git a/src/web-ui/src/locales/zh-TW/flow-chat.json b/src/web-ui/src/locales/zh-TW/flow-chat.json index fbe1a762c7..c3545d4cb3 100644 --- a/src/web-ui/src/locales/zh-TW/flow-chat.json +++ b/src/web-ui/src/locales/zh-TW/flow-chat.json @@ -1093,6 +1093,8 @@ "fixRunning": "正在修復...", "fixCompleted": "修復完成", "fixFailed": "修復失敗", + "actionStartFailed": "無法開始此次操作。", + "actionStartFailedWithReason": "無法開始此次操作：{{reason}}", "fixTimeout": "修復超時", "fixInterrupted": "修復已中斷，最多有 {{count}} 個已選項目可能仍需處理。", "reviewWaitingCapacity": "審核正在等待容量", From 28210802f067290c5164c8e41f0962ec2080cef7 Mon Sep 17 00:00:00 2001 From: guantw Date: Sun, 6 Sep 2026 15:11:37 +0800 Subject: [PATCH 3/5] fix(review): report missing explicit targets Resolve explicit file and directory scopes against the active local or remote workspace before starting Review. Return localized missing-target feedback and forward the optional remote connection scope through the existing path-existence API. Cover target resolution, service handling, and API forwarding with regression tests. --- .../deep-review/launch/targetResolver.test.ts | 97 +++++++++++++++ .../deep-review/launch/targetResolver.ts | 110 ++++++++++++++---- .../flow_chat/services/ReviewService.test.ts | 33 ++++++ .../src/flow_chat/services/ReviewService.ts | 6 + .../api/service-api/SystemAPI.test.ts | 15 +++ .../api/service-api/SystemAPI.ts | 8 +- src/web-ui/src/locales/en-US/flow-chat.json | 1 + src/web-ui/src/locales/zh-CN/flow-chat.json | 1 + src/web-ui/src/locales/zh-TW/flow-chat.json | 1 + 9 files changed, 243 insertions(+), 29 deletions(-) diff --git a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts index addf12e4fb..dffb7a91fd 100644 --- a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts +++ b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts @@ -14,6 +14,7 @@ const mockGitGetDiff = vi.fn(); const mockGitResolveRevision = vi.fn(); const mockWorkspaceReadFile = vi.fn(); const mockWorkspaceGetFileMetadata = vi.fn(); +const mockSystemCheckPathExists = vi.fn(); vi.mock('@/infrastructure/api', () => ({ gitAPI: { @@ -26,6 +27,9 @@ vi.mock('@/infrastructure/api', () => ({ readFileContent: (...args: any[]) => mockWorkspaceReadFile(...args), getFileMetadata: (...args: any[]) => mockWorkspaceGetFileMetadata(...args), }, + systemAPI: { + checkPathExists: (...args: any[]) => mockSystemCheckPathExists(...args), + }, })); describe('Deep Review target resolver', () => { @@ -53,6 +57,7 @@ describe('Deep Review target resolver', () => { isFile: true, size: 1024, }); + mockSystemCheckPathExists.mockResolvedValue(true); }); it('counts changed lines from unified diff without headers', () => { @@ -109,6 +114,77 @@ describe('Deep Review target resolver', () => { }); }); + it('normalizes an existing changed absolute POSIX target to the workspace', async () => { + mockGitGetStatus.mockResolvedValueOnce({ + staged: [{ path: 'tests/existing.ts', status: 'modified' }], + unstaged: [], + untracked: [], + conflicts: [], + current_branch: 'main', + ahead: 0, + behind: 0, + }); + mockGitGetChangedFiles.mockResolvedValueOnce([ + { path: 'tests/existing.ts', status: 'modified' }, + ]); + mockGitGetDiff.mockResolvedValueOnce('+changed\n'); + + const result = await resolveSlashCommandReviewTarget( + '/storage/Users/currentUser/files/git_code/BitFun/tests/existing.ts', + '/storage/Users/currentUser/files/git_code/BitFun', + ); + + expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ + 'tests/existing.ts', + ]); + expect(result.targetEvidence).toMatchObject({ + source: 'workspace', + completeness: 'complete', + workspaceBinding: 'matching_dirty', + }); + expect(mockSystemCheckPathExists).not.toHaveBeenCalled(); + }); + + it('reports a missing absolute POSIX target', async () => { + mockSystemCheckPathExists.mockResolvedValueOnce(false); + + const result = await resolveSlashCommandReviewTarget( + '/storage/Users/currentUser/files/git_code/BitFun/tests/missing.ts', + '/storage/Users/currentUser/files/git_code/BitFun', + ); + + expect(mockSystemCheckPathExists).toHaveBeenCalledWith( + '/storage/Users/currentUser/files/git_code/BitFun/tests/missing.ts', + undefined, + ); + expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ + 'tests/missing.ts', + ]); + expect(result.targetEvidence).toMatchObject({ + completeness: 'unknown', + limitations: ['explicit_target_path_not_found'], + }); + }); + + it('keeps the existing unchanged-path limitation distinct from a missing path', async () => { + const result = await resolveSlashCommandReviewTarget( + '/storage/Users/currentUser/files/git_code/BitFun/tests/unchanged.ts', + '/storage/Users/currentUser/files/git_code/BitFun', + ); + + expect(mockSystemCheckPathExists).toHaveBeenCalledWith( + '/storage/Users/currentUser/files/git_code/BitFun/tests/unchanged.ts', + undefined, + ); + expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ + 'tests/unchanged.ts', + ]); + expect(result.targetEvidence).toMatchObject({ + completeness: 'unknown', + limitations: ['explicit_file_scope_has_no_workspace_changes'], + }); + }); + it('expands an explicit directory without widening outside it', async () => { mockGitGetStatus.mockResolvedValueOnce({ staged: [{ path: 'src/inside.ts', status: 'modified' }], @@ -302,6 +378,27 @@ describe('Deep Review target resolver', () => { }); }); + it('checks an explicit remote POSIX target without local fallback', async () => { + const result = await resolveSlashCommandReviewTarget( + '/remote/workspace/src/existing.ts', + '/remote/workspace', + 'remote-1', + ); + + expect(mockSystemCheckPathExists).toHaveBeenCalledWith( + '/remote/workspace/src/existing.ts', + 'remote-1', + ); + expect(mockGitGetStatus).not.toHaveBeenCalled(); + expect(mockGitGetChangedFiles).not.toHaveBeenCalled(); + expect(mockGitGetDiff).not.toHaveBeenCalled(); + expect(result.targetEvidence).toMatchObject({ + source: 'workspace', + completeness: 'unknown', + limitations: ['remote_workspace_review_unavailable'], + }); + }); + it('preserves both paths and rename semantics for an edited workspace rename', async () => { mockGitGetStatus.mockResolvedValueOnce({ staged: [{ path: 'src/new-name.ts', status: 'added' }], diff --git a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts index a5c2a9e72a..bbf036632e 100644 --- a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts +++ b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts @@ -1,4 +1,4 @@ -import { gitAPI, workspaceAPI } from '@/infrastructure/api'; +import { gitAPI, systemAPI, workspaceAPI } from '@/infrastructure/api'; import { isGitRepositoryUntrustedError } from '@/infrastructure/api/errors/TauriCommandError'; import type { GitChangedFile, @@ -287,6 +287,34 @@ function workspaceFilePath(workspacePath: string, filePath: string): string { return `${rootPath.replace(/\/+$/, '')}/${normalizePath(filePath, workspacePath)}`; } +async function findMissingWorkspacePaths( + workspacePath: string, + paths: string[], + remoteConnectionId?: string, +): Promise { + const results = await Promise.all([...new Set(paths)].map(async (path) => ({ + path, + exists: await systemAPI.checkPathExists( + workspaceFilePath(workspacePath, path), + remoteConnectionId, + ), + }))); + return results.flatMap(({ path, exists }) => exists ? [] : [path]); +} + +function missingExplicitTarget( + target: ReviewTargetClassification, +): ResolvedDeepReviewTarget { + return { + target, + changeStats: buildUnknownChangeStats(target), + targetEvidence: buildUnknownReviewTargetEvidence( + target, + 'explicit_target_path_not_found', + ), + }; +} + function countTextFileLines(content: string): number { if (!content) { return 0; @@ -604,15 +632,8 @@ export async function resolveSlashCommandReviewTarget( }; } - if (remoteConnectionId) { - return resolveCurrentFileReviewSnapshot( - workspacePath, - target, - remoteConnectionId, - ); - } - - if (!bindTargetToWorkspace(target, workspacePath)) { + const workspaceTarget = bindTargetToWorkspace(target, workspacePath); + if (!workspaceTarget) { return { target, changeStats: buildUnknownChangeStats(target), @@ -624,6 +645,24 @@ export async function resolveSlashCommandReviewTarget( } try { + if (remoteConnectionId) { + const missingPaths = await findMissingWorkspacePaths( + workspacePath, + workspaceTarget.files + .filter((file) => !file.excluded) + .map((file) => file.normalizedPath), + remoteConnectionId, + ); + if (missingPaths.length > 0) { + return missingExplicitTarget(workspaceTarget); + } + return resolveCurrentFileReviewSnapshot( + workspacePath, + workspaceTarget, + remoteConnectionId, + ); + } + const [status, changedFiles] = await Promise.all([ gitAPI.getStatus(workspacePath, 'review_explicit_scope_snapshot'), gitAPI.getChangedFiles(workspacePath, { @@ -641,18 +680,22 @@ export async function resolveSlashCommandReviewTarget( for (const path of collectWorkspaceDiffFilePaths(status)) { candidatePaths.add(normalizePath(path, workspacePath).replace(/\/+$/, '')); } - const requested = explicitFilePaths.map((path) => { - const normalized = normalizePath(path, workspacePath); - const normalizedPath = normalized.replace(/\/+$/, ''); - const exactFileExists = candidatePaths.has(normalizedPath); - const containsChangedFiles = [...candidatePaths].some((candidate) => ( - candidate.startsWith(`${normalizedPath}/`) - )); - return { - path: normalizedPath, - directory: /[\\/]$/.test(path) || (!exactFileExists && containsChangedFiles), - }; - }); + const requested = workspaceTarget.files + .filter((file) => !file.excluded) + .map((file) => { + const normalized = normalizePath(file.normalizedPath, workspacePath); + const normalizedPath = normalized.replace(/\/+$/, ''); + const exactFileExists = candidatePaths.has(normalizedPath); + const containsChangedFiles = [...candidatePaths].some((candidate) => ( + candidate.startsWith(`${normalizedPath}/`) + )); + return { + path: normalizedPath, + directory: + /[\\/]$/.test(file.normalizedPath) || + (!exactFileExists && containsChangedFiles), + }; + }); const matchesRequestedPath = (path: string): boolean => { const normalized = normalizePath(path, workspacePath).replace(/\/+$/, ''); return requested.some((entry) => ( @@ -681,12 +724,29 @@ export async function resolveSlashCommandReviewTarget( represented.add(normalized); } } + const requestedHasScopedChange = (entry: (typeof requested)[number]): boolean => + scopedChanges.some((change) => [change.path, change.oldPath] + .filter((path): path is string => Boolean(path)) + .map((path) => normalizePath(path, workspacePath).replace(/\/+$/, '')) + .some((path) => ( + path === entry.path || + (entry.directory && path.startsWith(`${entry.path}/`)) + ))); + const missingPaths = await findMissingWorkspacePaths( + workspacePath, + requested + .filter((entry) => !requestedHasScopedChange(entry)) + .map((entry) => entry.path), + ); + if (missingPaths.length > 0) { + return missingExplicitTarget(workspaceTarget); + } if (scopedChanges.length === 0) { return { - target, - changeStats: buildUnknownChangeStats(target), + target: workspaceTarget, + changeStats: buildUnknownChangeStats(workspaceTarget), targetEvidence: buildUnknownReviewTargetEvidence( - target, + workspaceTarget, 'explicit_file_scope_has_no_workspace_changes', ), }; diff --git a/src/web-ui/src/flow_chat/services/ReviewService.test.ts b/src/web-ui/src/flow_chat/services/ReviewService.test.ts index f31212105a..1c3ec2c3d8 100644 --- a/src/web-ui/src/flow_chat/services/ReviewService.test.ts +++ b/src/web-ui/src/flow_chat/services/ReviewService.test.ts @@ -556,6 +556,39 @@ describe('ReviewService', () => { )).rejects.toThrow('Remote workspace Review is not supported'); }); + it('reports a missing explicit file before spending reviewer capacity', async () => { + const manifest = runManifest('normal'); + mocks.resolveSlashCommandReviewTarget.mockResolvedValue({ + target: { + ...manifest.target, + source: 'slash_command_explicit_files', + files: [{ + ...manifest.target.files[0], + path: 'tests/missing.ts', + normalizedPath: 'tests/missing.ts', + source: 'slash_command_explicit_files', + status: 'unknown', + }], + }, + changeStats: { fileCount: 1, lineCountSource: 'unknown' }, + targetEvidence: { + ...targetEvidence(), + completeness: 'unknown', + workspaceBinding: 'unavailable', + files: [], + limitations: ['explicit_target_path_not_found'], + }, + }); + + await expect(prepareReviewLaunchFromSlashCommand( + '/review tests/missing.ts', + '/workspace/project', + )).rejects.toMatchObject({ + message: 'The requested file or directory does not exist in the current workspace.', + launchErrorMessageKey: 'deepReviewActionBar.launchError.missingExplicitScope', + }); + }); + it('blocks an empty confirmed workspace snapshot before spending reviewer capacity', async () => { const manifest = runManifest('normal'); mocks.resolveSlashCommandReviewTarget.mockResolvedValue({ diff --git a/src/web-ui/src/flow_chat/services/ReviewService.ts b/src/web-ui/src/flow_chat/services/ReviewService.ts index 955fae91a3..281d6b2431 100644 --- a/src/web-ui/src/flow_chat/services/ReviewService.ts +++ b/src/web-ui/src/flow_chat/services/ReviewService.ts @@ -287,6 +287,12 @@ async function prepareFromResolvedTarget(params: { 'deepReviewActionBar.launchError.unresolvedTarget', ); } + if (params.targetEvidence.limitations.includes('explicit_target_path_not_found')) { + throw reviewTargetError( + 'The requested file or directory does not exist in the current workspace.', + 'deepReviewActionBar.launchError.missingExplicitScope', + ); + } if (params.targetEvidence.limitations.includes('explicit_file_scope_has_no_workspace_changes')) { throw reviewTargetError( 'The requested files or directories contain no workspace changes.', diff --git a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts index 3b4e4aa513..c459cbce24 100644 --- a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts @@ -111,4 +111,19 @@ describe('SystemAPI', () => { await expect(systemAPI.setClipboard('device-code')).rejects.toThrow('Clipboard write failed'); expect(invokeMock).not.toHaveBeenCalled(); }); + + it('checks path existence through an explicit remote workspace scope', async () => { + invokeMock.mockResolvedValueOnce(true); + + await expect(systemAPI.checkPathExists( + '/remote/workspace/src/existing.ts', + 'remote-connection-1', + )).resolves.toBe(true); + expect(invokeMock).toHaveBeenCalledWith('check_path_exists', { + request: { + path: '/remote/workspace/src/existing.ts', + remoteConnectionId: 'remote-connection-1', + }, + }); + }); }); diff --git a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts index 62f68e3658..d47885925f 100644 --- a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts @@ -154,13 +154,13 @@ export class SystemAPI { } - async checkPathExists(path: string): Promise { + async checkPathExists(path: string, remoteConnectionId?: string): Promise { try { - return await api.invoke('check_path_exists', { - request: { path } + return await api.invoke('check_path_exists', { + request: { path, remoteConnectionId } }); } catch (error) { - throw createTauriCommandError('check_path_exists', error, { path }); + throw createTauriCommandError('check_path_exists', error, { path, remoteConnectionId }); } } diff --git a/src/web-ui/src/locales/en-US/flow-chat.json b/src/web-ui/src/locales/en-US/flow-chat.json index a0c8a88b87..79f6315640 100644 --- a/src/web-ui/src/locales/en-US/flow-chat.json +++ b/src/web-ui/src/locales/en-US/flow-chat.json @@ -1266,6 +1266,7 @@ "unresolvedGitRange": "The requested Git range could not be resolved. Check the ref or range and try again.", "emptyWorkspace": "There are no workspace changes to review.", "emptyExplicitScope": "The requested files or directories contain no workspace changes.", + "missingExplicitScope": "The requested file or directory does not exist in the current workspace.", "unresolvedTarget": "The Review target could not be prepared as bounded evidence. Open its workspace or narrow the target and try again.", "repositoryUntrusted": "Git will not read this repository because the folder is owned by another user. Trust the folder when prompted, then start Review again.", "uncertain": "The Review request may already be running. Its session was preserved so you can inspect or retry it safely.", diff --git a/src/web-ui/src/locales/zh-CN/flow-chat.json b/src/web-ui/src/locales/zh-CN/flow-chat.json index e7998faf36..914bbb6451 100644 --- a/src/web-ui/src/locales/zh-CN/flow-chat.json +++ b/src/web-ui/src/locales/zh-CN/flow-chat.json @@ -1266,6 +1266,7 @@ "unresolvedGitRange": "无法解析请求的 Git 范围，请检查引用或范围后重试", "emptyWorkspace": "当前工作区没有可审核的变更", "emptyExplicitScope": "请求的文件或目录没有工作区变更", + "missingExplicitScope": "请求的文件或目录在当前工作区中不存在", "unresolvedTarget": "无法将审核目标准备为有界证据，请打开对应工作区或缩小目标后重试", "repositoryUntrusted": "该目录属于其他用户，Git 拒绝读取此仓库。请在提示中信任该目录后重新启动审核", "uncertain": "审核请求可能已经在运行。会话已保留，可安全检查或重试", diff --git a/src/web-ui/src/locales/zh-TW/flow-chat.json b/src/web-ui/src/locales/zh-TW/flow-chat.json index c3545d4cb3..5671544648 100644 --- a/src/web-ui/src/locales/zh-TW/flow-chat.json +++ b/src/web-ui/src/locales/zh-TW/flow-chat.json @@ -1266,6 +1266,7 @@ "unresolvedGitRange": "無法解析要求的 Git 範圍，請檢查引用或範圍後重試", "emptyWorkspace": "目前工作區沒有可審核的變更", "emptyExplicitScope": "要求的檔案或目錄沒有工作區變更", + "missingExplicitScope": "要求的檔案或目錄不存在於目前工作區中", "unresolvedTarget": "無法將審核目標準備為有界證據，請開啟對應工作區或縮小目標後重試", "repositoryUntrusted": "該目錄屬於其他使用者，Git 拒絕讀取此儲存庫。請在提示中信任該目錄後重新啟動審核", "uncertain": "審核要求可能已在執行。工作階段已保留，可安全檢查或重試", From 0e0fffc43344d36f6a8d11f7f9faff43ace13a97 Mon Sep 17 00:00:00 2001 From: guantw Date: Sun, 6 Sep 2026 15:38:26 +0800 Subject: [PATCH 4/5] fix(review): align error recovery with explicit actions Restrict Review platform trust recovery to explicit refresh and retry actions. Share localized launch error presentation across action headers and notifications while retaining structured keys and backend reasons. Keep unsupported remote Review targets free of path probes, preserve local missing-target and Git evidence handling, and cover the interaction boundaries with regression tests. --- .../ReviewPlatformPanel.trust.test.tsx | 107 ++++++++++++++++++ .../review-platform/ReviewPlatformPanel.tsx | 22 ++-- .../btw/DeepReviewActionBar.test.tsx | 30 +++++ .../action-bar/DeepReviewActionBar.tsx | 28 ++--- .../action-bar/actionBarFormatting.test.ts | 21 ++++ .../action-bar/actionBarFormatting.ts | 28 +++++ .../deep-review/launch/targetResolver.test.ts | 30 +++-- .../deep-review/launch/targetResolver.ts | 12 -- .../api/service-api/SystemAPI.test.ts | 8 +- .../api/service-api/SystemAPI.ts | 6 +- 10 files changed, 233 insertions(+), 59 deletions(-) create mode 100644 src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.trust.test.tsx diff --git a/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.trust.test.tsx b/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.trust.test.tsx new file mode 100644 index 0000000000..50ff711139 --- /dev/null +++ b/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.trust.test.tsx @@ -0,0 +1,107 @@ +import React, { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; +import { ReviewPlatformPanel } from './ReviewPlatformPanel'; +import { resetGitTrustDecisions } from '@/shared/services/gitTrustService'; +import { TauriCommandError } from '@/infrastructure/api/errors/TauriCommandError'; + +const mocks = vi.hoisted(() => ({ + snapshot: vi.fn(), context: vi.fn(), confirm: vi.fn(), trust: vi.fn(), + state: { sessions: new Map(), activeSessionId: null }, +})); +vi.mock('@/infrastructure/api', () => ({ + reviewPlatformAPI: { getWorkspaceSnapshot: mocks.snapshot, getWorkspaceContext: mocks.context }, + systemAPI: {}, + gitAPI: { trustRepository: mocks.trust, getRepositoryTrust: vi.fn() }, +})); +vi.mock('@/infrastructure/confirm-dialog', () => ({ confirmWarning: mocks.confirm })); +vi.mock('@/infrastructure/i18n', () => ({ + i18nService: { t: (key: string) => key }, +})); +vi.mock('@/shared/utils/logger', () => ({ + createLogger: () => ({ error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() }), +})); +vi.mock('@/shared/notification-system', () => ({ + notificationService: { warning: vi.fn(), success: vi.fn(), error: vi.fn() }, +})); +vi.mock('@/flow_chat/store/FlowChatStore', () => ({ + flowChatStore: { getState: () => mocks.state, subscribe: () => () => {} }, +})); +vi.mock('@/flow_chat/components/DeepReviewConsentDialog', () => ({ + useDeepReviewConsent: () => ({ confirmDeepReviewLaunch: vi.fn(), deepReviewConsentDialog: null }), +})); +vi.mock('@/flow_chat/services/ReviewService', () => ({ + launchPreparedReviewSession: vi.fn(), prepareReviewLaunchFromPullRequest: vi.fn(), +})); +vi.mock('@/flow_chat/services/sessionActivation', () => ({ openMainSession: vi.fn() })); +vi.mock('@/flow_chat/services/btwSessionPane', () => ({ openBtwSessionInAuxPane: vi.fn() })); +vi.mock('@/shared/services/ide-control', () => ({ quickActions: {} })); +vi.mock('@/shared/stores/contextStore', () => ({ useContextStore: {} })); +vi.mock('@/infrastructure/markdown', () => ({ MarkdownRenderer: () => null })); +vi.mock('@openbitfun/ui', () => { + const Box = ({ children }: { children?: React.ReactNode }) =>
{children}
; + const Button = ({ children, onClick, disabled, 'aria-label': label }: { + children?: React.ReactNode; onClick?: () => void; disabled?: boolean; 'aria-label'?: string; + }) => ; + return { + Button, IconButton: Button, Icon: () => null, Input: () => null, Combobox: () => null, + Field: Box, ScrollArea: Box, TabGroup: () => null, Tooltip: Box, + Dialog: () => null, DialogBody: Box, DialogClose: Box, DialogHeader: Box, + DialogHeading: Box, DialogTitle: Box, + }; +}); + +let dom: { window: Window & typeof globalThis }; +let root: Root; +let container: HTMLDivElement; +const workspacePath = '/workspace/review-trust-test'; +const error = new TauriCommandError('Command failed', { + command: 'review_platform_get_workspace_snapshot', + originalError: `git_repository_untrusted: ${workspacePath}`, +}); + +beforeEach(async () => { + const { JSDOM } = await import('jsdom'); + dom = new JSDOM('', { url: 'http://localhost' }); + vi.stubGlobal('window', dom.window); + vi.stubGlobal('document', dom.window.document); + vi.stubGlobal('localStorage', dom.window.localStorage); + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true); + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + vi.resetAllMocks(); + resetGitTrustDecisions(); + mocks.snapshot.mockRejectedValue(error); + mocks.context.mockRejectedValue(error); +}); + +afterEach(() => { + act(() => root.unmount()); + dom.window.close(); + vi.unstubAllGlobals(); +}); + +describe('Review platform trust interaction', () => { + it.each([false, true])('does not prompt during automatic loading (detailOnly=%s)', async (detailOnly) => { + await act(async () => root.render()); + expect(detailOnly ? mocks.context : mocks.snapshot).toHaveBeenCalledTimes(1); + expect(mocks.confirm).not.toHaveBeenCalled(); + expect(mocks.trust).not.toHaveBeenCalled(); + expect(container.textContent).toContain('panels/git:trust.required'); + expect(container.textContent).not.toContain('git_repository_untrusted:'); + }); + + it.each([true, false])('asks on Retry and replays only after approval (approved=%s)', async (approved) => { + mocks.confirm.mockResolvedValue(approved); + mocks.trust.mockResolvedValue({ state: 'trusted', repositoryPath: workspacePath }); + await act(async () => root.render()); + const retry = Array.from(container.querySelectorAll('button')).find(button => button.textContent === 'Retry'); + expect(retry).toBeTruthy(); + await act(async () => retry!.click()); + expect(mocks.confirm).toHaveBeenCalledTimes(1); + expect(mocks.trust).toHaveBeenCalledTimes(approved ? 1 : 0); + expect(mocks.snapshot).toHaveBeenCalledTimes(approved ? 3 : 2); + expect(container.textContent).toContain('panels/git:trust.required'); + }); +}); diff --git a/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx b/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx index feb932b4a6..f38316b583 100644 --- a/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx +++ b/src/web-ui/src/app/components/panels/review-platform/ReviewPlatformPanel.tsx @@ -820,17 +820,17 @@ export const ReviewPlatformPanel: React.FC = ({ setLoading(true); setError(null); try { - const next = await withGitRepositoryTrustRecovery( - () => detailOnly - ? reviewPlatformAPI.getWorkspaceContext(workspacePath, requestedRemoteId ?? null) - : reviewPlatformAPI.getWorkspaceSnapshot( - workspacePath, - requestedRemoteId ?? null, - requestedPage, - PR_PAGE_SIZE, - ), - { userInitiated: options?.userInitiated }, - ); + const fetchSnapshot = () => detailOnly + ? reviewPlatformAPI.getWorkspaceContext(workspacePath, requestedRemoteId ?? null) + : reviewPlatformAPI.getWorkspaceSnapshot( + workspacePath, + requestedRemoteId ?? null, + requestedPage, + PR_PAGE_SIZE, + ); + const next = options?.userInitiated + ? await withGitRepositoryTrustRecovery(fetchSnapshot, { userInitiated: true }) + : await fetchSnapshot(); if (snapshotRequestSeq.current !== requestSeq) return; setSnapshot(next); const remoteId = next.selectedRemoteId ?? next.remotes[0]?.id ?? null; diff --git a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx index a10d37a2cd..31bfab13ce 100644 --- a/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx +++ b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx @@ -319,6 +319,36 @@ describeWithJsdom('DeepReviewActionBar', () => { expect(container.textContent).not.toContain('Failed to start dialog turn:'); }); + it.each([ + [new Error('Failed to start dialog turn: provider quota exhausted'), 'Unable to start this action: provider quota exhausted'], + [Object.assign(new Error('Network connection was interrupted before Review could start.'), { + launchErrorMessageKey: 'deepReviewActionBar.launchError.network', + originalMessage: 'Failed to start dialog turn: provider connection closed', + }), 'Network connection interrupted. Review failed to start.\nprovider connection closed'], + ])('shows the same launch error in the header and notification: %s', async (error, message) => { + const { notificationService } = await import('@/shared/notification-system'); + sendMessageMock.mockRejectedValueOnce(error); + useReviewActionBarStore.getState().showActionBar({ + childSessionId: 'child-session', + parentSessionId: 'parent-session', + reviewData: { + summary: { recommended_action: 'request_changes' }, + remediation_plan: ['Fix the provider failure.'], + }, + phase: 'review_completed', + }); + await act(async () => root.render()); + const startFixButton = Array.from(container.querySelectorAll('button')) + .find((button) => button.textContent?.includes('Start fixing')); + expect(startFixButton).toBeTruthy(); + await act(async () => { + startFixButton!.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true })); + }); + expect(notificationService.error).toHaveBeenCalledWith(message, { duration: 5000 }); + expect(container.textContent).toContain(message); + expect(container.textContent).not.toContain('Failed to start dialog turn:'); + }); + it('keeps remediation in progress after submitting a fix turn', async () => { flowChatSessionsMock.set('child-session', { sessionId: 'child-session', diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx b/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx index d378f8f8be..0903dedf8b 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/DeepReviewActionBar.tsx @@ -46,7 +46,7 @@ import { useSettingsStore } from '@/app/scenes/settings/settingsStore'; import { useSceneStore } from '@/app/stores/sceneStore'; import type { SettingsPageId } from '@/app/scenes/settings/settingsTypes'; import { - classifyReviewActionErrorMessage, + getReviewActionErrorMessage, formatElapsedTime, } from './actionBarFormatting'; import { CapacityQueueNotice } from './CapacityQueueNotice'; @@ -519,14 +519,11 @@ export const ReviewActionBar: React.FC = ({ childSessionId log.error('Failed to start review remediation', { childSessionId, reviewMode, error }); const msg = error instanceof Error ? error.message : String(error); const isTimeout = /timeout/i.test(msg); - store.updatePhase(isTimeout ? 'fix_timeout' : 'fix_failed', msg, childSessionId); + const message = getReviewActionErrorMessage(error, t, t('deepReviewActionBar.actionStartFailed')); + store.updatePhase(isTimeout ? 'fix_timeout' : 'fix_failed', message, childSessionId); store.restore(childSessionId ?? undefined); notificationService.error( - error instanceof Error - ? error.message - : t('toolCards.codeReview.reviewFailed', { - error: t('toolCards.codeReview.unknownError'), - }), + message, { duration: 5000 }, ); } finally { @@ -660,7 +657,7 @@ export const ReviewActionBar: React.FC = ({ childSessionId reviewMode, error, }); - const message = normalizeActionErrorMessage(error); + const message = getReviewActionErrorMessage(error, t, t('deepReviewActionBar.actionStartFailed')); notificationService.error(message, { duration: 5000 }); } finally { store.setActiveAction(null, undefined, childSessionId); @@ -724,9 +721,7 @@ export const ReviewActionBar: React.FC = ({ childSessionId store.minimize(childSessionId); } catch (error) { log.error('Failed to start DeepReview retry coverage', { childSessionId, error }); - const message = error instanceof Error - ? error.message - : t('deepReviewActionBar.retryIncompleteFailed'); + const message = getReviewActionErrorMessage(error, t, t('deepReviewActionBar.retryIncompleteFailed')); notificationService.error(message, { duration: 5000 }); } finally { store.setActiveAction(null, undefined, childSessionId); @@ -835,16 +830,7 @@ export const ReviewActionBar: React.FC = ({ childSessionId const displayErrorMessage = useMemo(() => { if (!errorMessage) return null; - const presentation = classifyReviewActionErrorMessage(errorMessage); - if (presentation.kind === 'raw') { - return presentation.message; - } - - return presentation.reason - ? t('deepReviewActionBar.actionStartFailedWithReason', { - reason: presentation.reason, - }) - : t('deepReviewActionBar.actionStartFailed'); + return getReviewActionErrorMessage(errorMessage, t, t('deepReviewActionBar.actionStartFailed')); }, [errorMessage, t]); const handleCopyDiagnostics = useCallback(async () => { diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts index 95f0ee8d54..4fb03818e0 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.test.ts @@ -2,9 +2,30 @@ import { describe, expect, it } from 'vitest'; import { classifyReviewActionErrorMessage, formatElapsedTime, + getReviewActionErrorMessage, } from './actionBarFormatting'; +import { createTestI18nT } from '@/test/i18nTestUtils'; describe('action bar formatting', () => { + const translate = createTestI18nT('flow-chat'); + + it('prefers structured launch copy and preserves the original backend reason', () => { + const error = Object.assign(new Error('Failed to start dialog turn: legacy wrapper'), { + launchErrorMessageKey: 'deepReviewActionBar.launchError.network', + originalMessage: 'Failed to start dialog turn: provider connection closed', + }); + expect(getReviewActionErrorMessage(error, translate, 'fallback')).toBe( + 'Network connection interrupted. Review failed to start.\nprovider connection closed', + ); + }); + + it('supports legacy stored strings, plain errors, and empty failure payloads', () => { + expect(getReviewActionErrorMessage('Failed to start dialog turn: quota exhausted', translate, 'fallback')) + .toBe('Unable to start this action: quota exhausted'); + expect(getReviewActionErrorMessage(new Error('provider detail'), translate, 'fallback')).toBe('provider detail'); + expect(getReviewActionErrorMessage(null, translate, 'fallback')).toBe('fallback'); + }); + it('formats elapsed milliseconds without changing existing labels', () => { expect(formatElapsedTime(999)).toBe('0s'); expect(formatElapsedTime(12_000)).toBe('12s'); diff --git a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts index e0d490365a..490e0746e5 100644 --- a/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts +++ b/src/web-ui/src/flow_chat/deep-review/action-bar/actionBarFormatting.ts @@ -1,3 +1,5 @@ +import { getDeepReviewLaunchErrorMessage, type DeepReviewLaunchError } from '../launch/launchErrors'; + const START_DIALOG_TURN_ERROR_PREFIX = 'Failed to start dialog turn:'; export type ReviewActionErrorPresentation = @@ -30,3 +32,29 @@ export function formatElapsedTime(ms: number): string { } return `${minutes}m ${remainingSeconds}s`; } + +export function getReviewActionErrorMessage( + error: unknown, + translate: (key: string, options?: { defaultValue?: string; reason?: string }) => string, + fallback: string, +): string { + const launchError = error as DeepReviewLaunchError | null | undefined; + if (launchError?.launchErrorMessageKey) { + const message = getDeepReviewLaunchErrorMessage(error, translate, fallback); + const original = launchError.originalMessage?.trim(); + const presentation = original ? classifyReviewActionErrorMessage(original) : null; + const reason = presentation?.kind === 'start_dialog_turn_failed' + ? presentation.reason + : original; + return reason && reason !== message ? `${message}\n${reason}` : message; + } + + const message = error instanceof Error ? error.message : typeof error === 'string' ? error : ''; + if (!message.trim()) return fallback; + + const presentation = classifyReviewActionErrorMessage(message); + if (presentation.kind === 'raw') return presentation.message; + return presentation.reason + ? translate('deepReviewActionBar.actionStartFailedWithReason', { reason: presentation.reason }) + : translate('deepReviewActionBar.actionStartFailed'); +} diff --git a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts index dffb7a91fd..89fcbe91a9 100644 --- a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts +++ b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts @@ -145,6 +145,21 @@ describe('Deep Review target resolver', () => { expect(mockSystemCheckPathExists).not.toHaveBeenCalled(); }); + it.each([ + { path: 'src/old.ts', status: 'deleted', old_path: undefined }, + { path: 'src/new.ts', status: 'renamed', old_path: 'src/old.ts' }, + ])('keeps an explicit old path reviewable from Git evidence: $status', async (change) => { + mockSystemCheckPathExists.mockResolvedValue(false); + mockGitGetChangedFiles.mockResolvedValue([change]); + mockGitGetDiff.mockResolvedValue('-old line\n'); + const result = await resolveSlashCommandReviewTarget('src/old.ts', '/workspace'); + expect(mockSystemCheckPathExists).not.toHaveBeenCalled(); + expect(result.targetEvidence.limitations).not.toContain('explicit_target_path_not_found'); + expect(result.targetEvidence.files).toEqual([ + expect.objectContaining({ path: change.path, status: change.status }), + ]); + }); + it('reports a missing absolute POSIX target', async () => { mockSystemCheckPathExists.mockResolvedValueOnce(false); @@ -155,7 +170,6 @@ describe('Deep Review target resolver', () => { expect(mockSystemCheckPathExists).toHaveBeenCalledWith( '/storage/Users/currentUser/files/git_code/BitFun/tests/missing.ts', - undefined, ); expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ 'tests/missing.ts', @@ -174,7 +188,6 @@ describe('Deep Review target resolver', () => { expect(mockSystemCheckPathExists).toHaveBeenCalledWith( '/storage/Users/currentUser/files/git_code/BitFun/tests/unchanged.ts', - undefined, ); expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ 'tests/unchanged.ts', @@ -378,17 +391,20 @@ describe('Deep Review target resolver', () => { }); }); - it('checks an explicit remote POSIX target without local fallback', async () => { + it.each(['exists', 'missing', 'offline'])('rejects an explicit remote target without probing when the host is %s', async (hostState) => { + mockSystemCheckPathExists.mockImplementation(async () => { + if (hostState === 'offline') throw new Error('remote host disconnected'); + return hostState === 'exists'; + }); const result = await resolveSlashCommandReviewTarget( '/remote/workspace/src/existing.ts', '/remote/workspace', 'remote-1', ); - expect(mockSystemCheckPathExists).toHaveBeenCalledWith( - '/remote/workspace/src/existing.ts', - 'remote-1', - ); + expect(mockSystemCheckPathExists).not.toHaveBeenCalled(); + expect(mockWorkspaceReadFile).not.toHaveBeenCalled(); + expect(mockWorkspaceGetFileMetadata).not.toHaveBeenCalled(); expect(mockGitGetStatus).not.toHaveBeenCalled(); expect(mockGitGetChangedFiles).not.toHaveBeenCalled(); expect(mockGitGetDiff).not.toHaveBeenCalled(); diff --git a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts index bbf036632e..7de04a4bf7 100644 --- a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts +++ b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.ts @@ -290,13 +290,11 @@ function workspaceFilePath(workspacePath: string, filePath: string): string { async function findMissingWorkspacePaths( workspacePath: string, paths: string[], - remoteConnectionId?: string, ): Promise { const results = await Promise.all([...new Set(paths)].map(async (path) => ({ path, exists: await systemAPI.checkPathExists( workspaceFilePath(workspacePath, path), - remoteConnectionId, ), }))); return results.flatMap(({ path, exists }) => exists ? [] : [path]); @@ -646,16 +644,6 @@ export async function resolveSlashCommandReviewTarget( try { if (remoteConnectionId) { - const missingPaths = await findMissingWorkspacePaths( - workspacePath, - workspaceTarget.files - .filter((file) => !file.excluded) - .map((file) => file.normalizedPath), - remoteConnectionId, - ); - if (missingPaths.length > 0) { - return missingExplicitTarget(workspaceTarget); - } return resolveCurrentFileReviewSnapshot( workspacePath, workspaceTarget, diff --git a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts index c459cbce24..71f14a8fae 100644 --- a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.test.ts @@ -112,17 +112,15 @@ describe('SystemAPI', () => { expect(invokeMock).not.toHaveBeenCalled(); }); - it('checks path existence through an explicit remote workspace scope', async () => { + it('checks path existence on the active host', async () => { invokeMock.mockResolvedValueOnce(true); await expect(systemAPI.checkPathExists( - '/remote/workspace/src/existing.ts', - 'remote-connection-1', + '/workspace/src/existing.ts', )).resolves.toBe(true); expect(invokeMock).toHaveBeenCalledWith('check_path_exists', { request: { - path: '/remote/workspace/src/existing.ts', - remoteConnectionId: 'remote-connection-1', + path: '/workspace/src/existing.ts', }, }); }); diff --git a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts index d47885925f..fa144e62d2 100644 --- a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts @@ -154,13 +154,13 @@ export class SystemAPI { } - async checkPathExists(path: string, remoteConnectionId?: string): Promise { + async checkPathExists(path: string): Promise { try { return await api.invoke('check_path_exists', { - request: { path, remoteConnectionId } + request: { path } }); } catch (error) { - throw createTauriCommandError('check_path_exists', error, { path, remoteConnectionId }); + throw createTauriCommandError('check_path_exists', error, { path }); } } From cc91a308531711c731ea4160b095e8d5013ba4fa Mon Sep 17 00:00:00 2001 From: guantw Date: Sun, 6 Sep 2026 16:17:01 +0800 Subject: [PATCH 5/5] test(review): use canonical product name in target fixtures Use OpenBitFun in absolute workspace path fixtures so the target resolver tests comply with the product identity audit. --- .../deep-review/launch/targetResolver.test.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts index 89fcbe91a9..7f4c9e7be4 100644 --- a/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts +++ b/src/web-ui/src/flow_chat/deep-review/launch/targetResolver.test.ts @@ -130,8 +130,8 @@ describe('Deep Review target resolver', () => { mockGitGetDiff.mockResolvedValueOnce('+changed\n'); const result = await resolveSlashCommandReviewTarget( - '/storage/Users/currentUser/files/git_code/BitFun/tests/existing.ts', - '/storage/Users/currentUser/files/git_code/BitFun', + '/storage/Users/currentUser/files/git_code/OpenBitFun/tests/existing.ts', + '/storage/Users/currentUser/files/git_code/OpenBitFun', ); expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ @@ -164,12 +164,12 @@ describe('Deep Review target resolver', () => { mockSystemCheckPathExists.mockResolvedValueOnce(false); const result = await resolveSlashCommandReviewTarget( - '/storage/Users/currentUser/files/git_code/BitFun/tests/missing.ts', - '/storage/Users/currentUser/files/git_code/BitFun', + '/storage/Users/currentUser/files/git_code/OpenBitFun/tests/missing.ts', + '/storage/Users/currentUser/files/git_code/OpenBitFun', ); expect(mockSystemCheckPathExists).toHaveBeenCalledWith( - '/storage/Users/currentUser/files/git_code/BitFun/tests/missing.ts', + '/storage/Users/currentUser/files/git_code/OpenBitFun/tests/missing.ts', ); expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ 'tests/missing.ts', @@ -182,12 +182,12 @@ describe('Deep Review target resolver', () => { it('keeps the existing unchanged-path limitation distinct from a missing path', async () => { const result = await resolveSlashCommandReviewTarget( - '/storage/Users/currentUser/files/git_code/BitFun/tests/unchanged.ts', - '/storage/Users/currentUser/files/git_code/BitFun', + '/storage/Users/currentUser/files/git_code/OpenBitFun/tests/unchanged.ts', + '/storage/Users/currentUser/files/git_code/OpenBitFun', ); expect(mockSystemCheckPathExists).toHaveBeenCalledWith( - '/storage/Users/currentUser/files/git_code/BitFun/tests/unchanged.ts', + '/storage/Users/currentUser/files/git_code/OpenBitFun/tests/unchanged.ts', ); expect(result.target.files.map((file) => file.normalizedPath)).toEqual([ 'tests/unchanged.ts',