From 41b3fb330c50ad29cf56614fb36dcec7b3c1304a Mon Sep 17 00:00:00 2001 From: guantw Date: Tue, 1 Sep 2026 10:17:34 +0800 Subject: [PATCH 1/3] 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 that code into localized, actionable copy in the review panel and route user-initiated retries through the existing trust recovery flow. Add backend and frontend coverage for trust classification and localized error handling. --- .../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 | 50 ++++-- .../shared/services/gitTrustService.test.ts | 13 ++ .../src/shared/services/gitTrustService.ts | 8 + .../src/tools/git/services/GitService.ts | 26 +--- 10 files changed, 381 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 db2803be54..9ed14017cd 100644 --- a/src/apps/desktop/src/api/review_platform_api.rs +++ b/src/apps/desktop/src/api/review_platform_api.rs @@ -2,10 +2,10 @@ use crate::api::app_state::AppState; use bitfun_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 log::error; use serde::Deserialize; @@ -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 77baf499f4..16f9e49fb4 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 bitfun_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, @@ -98,7 +99,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 12b19f9e31..6153d5c804 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 68b2221d35..b671e926b0 100644 --- a/src/crates/services/services-integrations/src/lib.rs +++ b/src/crates/services/services-integrations/src/lib.rs @@ -48,6 +48,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 472f65c963..595ba34a1f 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, @@ -63,6 +66,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}")] @@ -83,6 +91,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 { @@ -4371,7 +4411,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 { @@ -7468,6 +7508,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 3c2b247863..00463511e5 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 @@ -44,6 +44,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, @@ -143,6 +147,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); @@ -763,7 +772,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()); @@ -815,9 +827,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; @@ -836,7 +856,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()); @@ -890,7 +910,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); } @@ -968,7 +988,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); @@ -1379,7 +1399,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; @@ -1918,7 +1938,11 @@ export const ReviewPlatformPanel: React.FC = ({ size="xs" variant="ghost" tooltip="Refresh" - onClick={() => void loadSnapshot(listRemoteId, { force: true, page: currentPageIndex + 1 })} + onClick={() => void loadSnapshot(listRemoteId, { + force: true, + page: currentPageIndex + 1, + userInitiated: true, + })} isLoading={loading} > @@ -1982,7 +2006,11 @@ export const ReviewPlatformPanel: React.FC = ({
{error} -
@@ -2111,7 +2139,7 @@ export const ReviewPlatformPanel: React.FC = ({ className="review-platform__panel-button" size="small" variant="secondary" - onClick={() => void loadSnapshot(undefined, { force: true })} + onClick={() => void loadSnapshot(undefined, { force: true, userInitiated: true })} > Retry diff --git a/src/web-ui/src/shared/services/gitTrustService.test.ts b/src/web-ui/src/shared/services/gitTrustService.test.ts index 4a882640d4..80206e843e 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 383c36b7f1..8b4b1055fa 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 c1b6cd21669a6200e4a93d01f78c8131fa24c507 Mon Sep 17 00:00:00 2001 From: guantw Date: Tue, 1 Sep 2026 15:18:16 +0800 Subject: [PATCH 2/3] 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 and cover formatting, 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 260158d204..2848abfbfc 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: 600; font-size: var(--bf-appearance-token-tool-card-action-font-size); color: var(--bf-appearance-token-color-text-primary); } &__error-message { - margin-left: auto; + grid-column: 2; + min-width: 0; font-size: var(--bf-appearance-token-tool-card-action-font-size); color: var(--bf-appearance-token-color-error); - 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 0e02b3af5b..73d7a89fea 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 @@ -291,6 +291,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 7410fe65fb..b2acfce2f2 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 8d5acc46b6..838ebf9f6b 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,10 @@ import { isTauriRuntime } from '@/infrastructure/runtime'; import { useSettingsStore } from '@/app/scenes/settings/settingsStore'; import { useSceneStore } from '@/app/stores/sceneStore'; import type { ConfigTab } from '@/app/scenes/settings/settingsConfig'; -import { formatElapsedTime } from './actionBarFormatting'; +import { + classifyReviewActionErrorMessage, + formatElapsedTime, +} from './actionBarFormatting'; import { CapacityQueueNotice } from './CapacityQueueNotice'; import { DecisionExecutionGate } from './DecisionExecutionGate'; import { buildInterruptionDiagnostics } from './interruptionDiagnostics'; @@ -808,6 +811,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; @@ -903,7 +921,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 f4600cc318..1611d53a16 100644 --- a/src/web-ui/src/locales/en-US/flow-chat.json +++ b/src/web-ui/src/locales/en-US/flow-chat.json @@ -1017,6 +1017,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 b2aee4aff1..00acf4b602 100644 --- a/src/web-ui/src/locales/zh-CN/flow-chat.json +++ b/src/web-ui/src/locales/zh-CN/flow-chat.json @@ -1017,6 +1017,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 402324084b..1b756a5d59 100644 --- a/src/web-ui/src/locales/zh-TW/flow-chat.json +++ b/src/web-ui/src/locales/zh-TW/flow-chat.json @@ -1017,6 +1017,8 @@ "fixRunning": "正在修復...", "fixCompleted": "修復完成", "fixFailed": "修復失敗", + "actionStartFailed": "無法開始此次操作。", + "actionStartFailedWithReason": "無法開始此次操作：{{reason}}", "fixTimeout": "修復超時", "fixInterrupted": "修復已中斷，最多有 {{count}} 個已選項目可能仍需處理。", "reviewWaitingCapacity": "審核正在等待容量", From d2fc7f4afeffbe3dda1d9dacc2c40d675a7432fe Mon Sep 17 00:00:00 2001 From: guantw Date: Tue, 1 Sep 2026 15:18:06 +0800 Subject: [PATCH 3/3] 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 cover desktop path routing, target resolution, service handling, and API forwarding with regression tests. --- src/apps/desktop/src/api/commands.rs | 9 +- src/apps/desktop/src/api/path_target.rs | 8 +- .../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 + 11 files changed, 257 insertions(+), 32 deletions(-) diff --git a/src/apps/desktop/src/api/commands.rs b/src/apps/desktop/src/api/commands.rs index d2771ca008..cf5e31c892 100644 --- a/src/apps/desktop/src/api/commands.rs +++ b/src/apps/desktop/src/api/commands.rs @@ -519,6 +519,8 @@ pub struct ResetWorkspacePersonaFilesRequest { #[derive(Debug, Deserialize)] pub struct CheckPathExistsRequest { pub path: String, + #[serde(default, rename = "remoteConnectionId")] + pub remote_connection_id: Option, } #[derive(Debug, Deserialize)] @@ -2774,7 +2776,12 @@ pub async fn check_path_exists( state: State<'_, AppState>, request: CheckPathExistsRequest, ) -> Result { - path_exists(&state, &request.path).await + path_exists( + &state, + &request.path, + request.remote_connection_id.as_deref(), + ) + .await } #[tauri::command] diff --git a/src/apps/desktop/src/api/path_target.rs b/src/apps/desktop/src/api/path_target.rs index e224e3420b..8afbdd5cef 100644 --- a/src/apps/desktop/src/api/path_target.rs +++ b/src/apps/desktop/src/api/path_target.rs @@ -351,8 +351,12 @@ pub async fn write_text_file( } } -pub async fn path_exists(app_state: &AppState, raw_path: &str) -> Result { - match resolve_desktop_path_target(app_state, raw_path, None).await? { +pub async fn path_exists( + app_state: &AppState, + raw_path: &str, + preferred_remote_connection_id: Option<&str>, +) -> Result { + match resolve_desktop_path_target(app_state, raw_path, preferred_remote_connection_id).await? { DesktopPathTarget::Local { resolved_path, .. } => Ok(resolved_path.exists()), DesktopPathTarget::Remote { requested_path, 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', + '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/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 cde9927173..008bc0fed1 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', + '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/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 e7a37bd40a..52b85ed2e3 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 @@ -64,4 +64,19 @@ describe('SystemAPI', () => { request: { enabled: true }, }); }); + + 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 66acbbf776..665c7f5302 100644 --- a/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/SystemAPI.ts @@ -118,13 +118,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 1611d53a16..1c6e340140 100644 --- a/src/web-ui/src/locales/en-US/flow-chat.json +++ b/src/web-ui/src/locales/en-US/flow-chat.json @@ -1190,6 +1190,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 00acf4b602..05550de1dd 100644 --- a/src/web-ui/src/locales/zh-CN/flow-chat.json +++ b/src/web-ui/src/locales/zh-CN/flow-chat.json @@ -1190,6 +1190,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 1b756a5d59..db8822a2bf 100644 --- a/src/web-ui/src/locales/zh-TW/flow-chat.json +++ b/src/web-ui/src/locales/zh-TW/flow-chat.json @@ -1190,6 +1190,7 @@ "unresolvedGitRange": "無法解析要求的 Git 範圍，請檢查引用或範圍後重試", "emptyWorkspace": "目前工作區沒有可審核的變更", "emptyExplicitScope": "要求的檔案或目錄沒有工作區變更", + "missingExplicitScope": "要求的檔案或目錄不存在於目前工作區中", "unresolvedTarget": "無法將審核目標準備為有界證據，請開啟對應工作區或縮小目標後重試", "repositoryUntrusted": "該目錄屬於其他使用者，Git 拒絕讀取此儲存庫。請在提示中信任該目錄後重新啟動審核", "uncertain": "審核要求可能已在執行。工作階段已保留，可安全檢查或重試",