From be410c2d8a970920c69a07b9b136d0c8dd891692 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Mon, 27 Jul 2026 22:58:55 -0700 Subject: [PATCH 1/2] fix(worktree): handle historical session toggles --- src/apps/cli/src/chat_state.rs | 19 ++ src/apps/cli/src/modes/chat/worktree.rs | 38 ++-- src/apps/desktop/src/api/worktree_api.rs | 5 +- .../src/agentic/session/session_manager.rs | 153 ++++++++++++-- .../assembly/core/src/service/worktree/mod.rs | 24 ++- .../src/service/worktree/session_binding.rs | 187 ++++++++++++++---- .../src/flow_chat/components/ChatInput.tsx | 41 +++- .../ChatInputWorkspaceStrip.test.tsx | 27 +++ .../components/ChatInputWorkspaceStrip.tsx | 5 +- .../flow_chat/utils/sessionWorktree.test.ts | 56 ++++++ .../src/flow_chat/utils/sessionWorktree.ts | 34 ++++ .../api/service-api/ApiClient.test.ts | 26 +++ .../api/service-api/ApiClient.ts | 28 ++- .../api/service-api/WorktreeAPI.test.ts | 45 +++++ .../api/service-api/WorktreeAPI.ts | 76 +++++-- 15 files changed, 674 insertions(+), 90 deletions(-) create mode 100644 src/web-ui/src/flow_chat/utils/sessionWorktree.test.ts create mode 100644 src/web-ui/src/flow_chat/utils/sessionWorktree.ts diff --git a/src/apps/cli/src/chat_state.rs b/src/apps/cli/src/chat_state.rs index 7766143323..d261c41f57 100644 --- a/src/apps/cli/src/chat_state.rs +++ b/src/apps/cli/src/chat_state.rs @@ -419,6 +419,10 @@ impl ChatState { .is_some() } + pub(crate) fn has_conversation_history(&self) -> bool { + self.metadata.message_count > 0 + } + pub(crate) fn set_worktree_control_available(&mut self, available: bool) { self.worktree_control_available = available; } @@ -1402,6 +1406,21 @@ mod tests { ); } + #[test] + fn worktree_binding_history_ignores_local_system_messages() { + let mut state = ChatState::new( + "session-1".to_string(), + "Session".to_string(), + "agentic".to_string(), + Some("/tmp/project".to_string()), + ); + state.add_system_message("Worktree: off".to_string()); + assert!(!state.has_conversation_history()); + + state.metadata.message_count = 1; + assert!(state.has_conversation_history()); + } + #[test] fn workspace_context_prefers_managed_worktree_branch_or_detached_commit() { let mut state = ChatState::new( diff --git a/src/apps/cli/src/modes/chat/worktree.rs b/src/apps/cli/src/modes/chat/worktree.rs index 5fcb5604e5..73e4dd3667 100644 --- a/src/apps/cli/src/modes/chat/worktree.rs +++ b/src/apps/cli/src/modes/chat/worktree.rs @@ -70,6 +70,23 @@ impl ChatMode { chat_state: &mut ChatState, rt_handle: &tokio::runtime::Handle, ) -> Result> { + let command = match parse_worktree_command(arguments) { + Ok(command) => command, + Err(usage) => { + chat_view.set_status(Some(usage.clone())); + chat_state.add_system_message(usage); + return Ok(None); + } + }; + + self.refresh_workspace_git_status(chat_state, rt_handle); + if command == WorktreeCommand::Status { + let message = Self::worktree_status_message(chat_state); + chat_view.set_status(Some(chat_state.workspace_context_label())); + chat_state.add_system_message(message); + return Ok(None); + } + let action = action_by_id("toggle_worktree", ActionContext::Chat) .expect("Worktree action must remain registered"); let state = ActionState::chat(chat_state.is_processing, false); @@ -85,20 +102,11 @@ impl ChatMode { chat_state.add_system_message(message); return Ok(None); } - - let command = match parse_worktree_command(arguments) { - Ok(command) => command, - Err(usage) => { - chat_view.set_status(Some(usage.clone())); - chat_state.add_system_message(usage); - return Ok(None); - } - }; - - self.refresh_workspace_git_status(chat_state, rt_handle); - if command == WorktreeCommand::Status { - let message = Self::worktree_status_message(chat_state); - chat_view.set_status(Some(chat_state.workspace_context_label())); + if chat_state.has_conversation_history() { + let message = + "Worktree isolation can only be changed before the session's first message" + .to_string(); + chat_view.set_status(Some(message.clone())); chat_state.add_system_message(message); return Ok(None); } @@ -114,11 +122,13 @@ impl ChatMode { "Disabling worktree isolation...".to_string() })); + let project_workspace_path = chat_state.project_workspace_path().map(str::to_string); let result = tokio::task::block_in_place(|| { rt_handle.block_on(WorktreeService::bind_session( WorktreeSessionBindingRequest { request_id: uuid::Uuid::new_v4().to_string(), session_id: chat_state.core_session_id.clone(), + project_workspace_path, enabled, }, )) diff --git a/src/apps/desktop/src/api/worktree_api.rs b/src/apps/desktop/src/api/worktree_api.rs index b4c0bdcfc2..59d3a1c479 100644 --- a/src/apps/desktop/src/api/worktree_api.rs +++ b/src/apps/desktop/src/api/worktree_api.rs @@ -68,8 +68,9 @@ pub async fn worktree_remove( WorktreeService::remove(request).await } -/// Toggle worktree isolation for a single session. The project path is derived -/// from the session itself, so remote checks live in the product layer. +/// Toggle worktree isolation for a single session. The optional project path +/// lets the product layer locate view-only persisted sessions; remote checks +/// and repository resolution remain in that shared layer. #[tauri::command] pub async fn worktree_bind_session( request: WorktreeSessionBindingRequest, diff --git a/src/crates/assembly/core/src/agentic/session/session_manager.rs b/src/crates/assembly/core/src/agentic/session/session_manager.rs index 415887c166..4f16d27f29 100644 --- a/src/crates/assembly/core/src/agentic/session/session_manager.rs +++ b/src/crates/assembly/core/src/agentic/session/session_manager.rs @@ -42,6 +42,7 @@ use crate::service::workspace::{get_global_workspace_service, WorkspaceInfo, Wor use crate::util::errors::{BitFunError, BitFunResult}; use crate::util::sanitize_plain_model_output; use crate::util::timing::elapsed_ms_u64; +use bitfun_core_types::SessionExecutionTarget; pub use bitfun_runtime_ports::SessionViewRestoreTiming; use bitfun_runtime_ports::{SessionStoragePathRequest, SessionStorePort}; use bitfun_services_core::session::{ @@ -50,7 +51,6 @@ use bitfun_services_core::session::{ set_deep_review_run_manifest, set_review_target_evidence, set_session_relationship, SessionStorageLayout, }; -use bitfun_core_types::SessionExecutionTarget; use dashmap::{mapref::entry::Entry, DashMap}; use log::{debug, error, info, warn}; use serde::{Deserialize, Serialize}; @@ -168,6 +168,20 @@ pub struct SessionExecutionBindingUpdate { pub execution_target: SessionExecutionTarget, } +/// Stable failure categories for atomically moving a session execution root. +/// +/// Worktree lifecycle maps these categories to its public structured error +/// contract without having to inspect human-readable `BitFunError` messages. +#[derive(Debug, thiserror::Error)] +pub enum SessionExecutionBindingError { + #[error("{0}")] + Busy(String), + #[error("{0}")] + NotFound(String), + #[error(transparent)] + Internal(#[from] BitFunError), +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum SessionResourceCleanupPolicy { BestEffort, @@ -3497,24 +3511,42 @@ impl SessionManager { &self, session_id: &str, binding: SessionExecutionBindingUpdate, - ) -> BitFunResult<()> { + ) -> Result<(), SessionExecutionBindingError> { // Mirrors update_session_model_id: an evicted session must be restored - // from its recorded storage path before the mutation permit is taken. + // before the mutation permit is taken. View-only historical restores do + // not populate the storage-path index, so use the owning project path as + // the stable fallback locator. if !self.sessions.contains_key(session_id) && self.config.enable_persistence { let session_storage_path = self .session_storage_path_index .get(session_id) .map(|entry| entry.value().path.clone()); - if let Some(session_storage_path) = session_storage_path { - let _ = self - .restore_session_from_storage_path(&session_storage_path, session_id) - .await; + let restore_result = if let Some(session_storage_path) = session_storage_path { + self.restore_session_from_storage_path(&session_storage_path, session_id) + .await + } else { + self.restore_session(Path::new(&binding.project_workspace_path), session_id) + .await + }; + if let Err(restore_error) = restore_result { + return match restore_error { + BitFunError::NotFound(message) => { + Err(SessionExecutionBindingError::NotFound(message)) + } + other => Err(SessionExecutionBindingError::Internal(other)), + }; } } let _mutation_guard = self.acquire_session_mutation(session_id).await?; if let Some(mut session) = self.sessions.get_mut(session_id) { + if !session.dialog_turn_ids.is_empty() || !matches!(session.state, SessionState::Idle) { + return Err(SessionExecutionBindingError::Busy( + "Worktree isolation can only be changed before the session's first message" + .to_string(), + )); + } session.config.workspace_path = Some(binding.workspace_path.clone()); session.config.project_workspace_path = Some(binding.project_workspace_path.clone()); session.config.execution_target = Some(binding.execution_target.clone()); @@ -3522,9 +3554,8 @@ impl SessionManager { session.updated_at = SystemTime::now(); session.last_activity_at = SystemTime::now(); } else { - return Err(BitFunError::NotFound(format!( - "Session not found: {}", - session_id + return Err(SessionExecutionBindingError::NotFound(format!( + "Session not found: {session_id}" ))); } @@ -6819,8 +6850,8 @@ impl SessionManager { #[cfg(test)] mod tests { use super::{ - should_auto_migrate_session_model, CoreSessionStorePort, SessionManager, - SessionManagerConfig, + should_auto_migrate_session_model, CoreSessionStorePort, SessionExecutionBindingError, + SessionExecutionBindingUpdate, SessionManager, SessionManagerConfig, }; use crate::agentic::core::{ CompressionState, Message, MessageContent, MessageRole, ProcessingPhase, Session, @@ -6841,6 +6872,7 @@ mod tests { SessionRelationship, SessionRelationshipKind, ToolCallData, ToolItemData, ToolResultData, TurnStatus, UserMessageData, }; + use bitfun_core_types::SessionExecutionTarget; use bitfun_runtime_ports::SessionStoragePathRequest; use dashmap::{try_result::TryResult, DashMap}; use serde_json::json; @@ -7053,6 +7085,103 @@ mod tests { ) } + #[tokio::test] + async fn execution_binding_rejects_a_session_after_its_first_turn() { + let manager = in_memory_test_manager(); + let workspace = TestWorkspace::new(); + let session = manager + .create_session( + "Binding race".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().to_string()), + ..SessionConfig::default() + }, + ) + .await + .expect("session should be created"); + manager + .sessions + .get_mut(&session.session_id) + .expect("session should remain loaded") + .dialog_turn_ids + .push("turn-1".to_string()); + + let error = manager + .update_session_execution_binding( + &session.session_id, + SessionExecutionBindingUpdate { + workspace_path: "/tmp/worktree".to_string(), + project_workspace_path: workspace.path().to_string_lossy().to_string(), + workspace_id: None, + execution_target: SessionExecutionTarget::local("/tmp/worktree".to_string()), + }, + ) + .await + .expect_err("a non-empty session must not move"); + + assert!(matches!(error, SessionExecutionBindingError::Busy(_))); + assert_eq!( + manager + .get_session(&session.session_id) + .and_then(|session| session.config.workspace_path), + Some(workspace.path().to_string_lossy().to_string()) + ); + } + + #[tokio::test] + async fn execution_binding_restores_a_view_only_empty_session_from_its_project() { + let workspace = TestWorkspace::new(); + let persistence_manager = Arc::new( + PersistenceManager::new(workspace.path_manager()).expect("persistence manager"), + ); + let manager = test_manager(persistence_manager); + let session = manager + .create_session( + "View-only binding".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().to_string()), + project_workspace_path: Some(workspace.path().to_string_lossy().to_string()), + ..SessionConfig::default() + }, + ) + .await + .expect("session should be created"); + assert!(manager + .unload_session_from_memory(&session.session_id) + .await + .expect("session should unload")); + manager + .session_storage_path_index + .remove(&session.session_id); + + let target_path = workspace.path().join("managed-worktree"); + manager + .update_session_execution_binding( + &session.session_id, + SessionExecutionBindingUpdate { + workspace_path: target_path.to_string_lossy().to_string(), + project_workspace_path: workspace.path().to_string_lossy().to_string(), + workspace_id: Some("workspace-2".to_string()), + execution_target: SessionExecutionTarget::local( + target_path.to_string_lossy().to_string(), + ), + }, + ) + .await + .expect("view-only session should restore and rebind"); + + let restored = manager + .get_session(&session.session_id) + .expect("session should be loaded after rebinding"); + assert_eq!( + restored.config.workspace_path.as_deref(), + Some(target_path.to_string_lossy().as_ref()) + ); + assert_eq!(restored.config.workspace_id.as_deref(), Some("workspace-2")); + } + #[tokio::test] async fn unloading_a_session_releases_capacity_without_deleting_persistence() { let workspace = TestWorkspace::new(); diff --git a/src/crates/assembly/core/src/service/worktree/mod.rs b/src/crates/assembly/core/src/service/worktree/mod.rs index ce5ba337d7..aa12c346f9 100644 --- a/src/crates/assembly/core/src/service/worktree/mod.rs +++ b/src/crates/assembly/core/src/service/worktree/mod.rs @@ -1174,10 +1174,11 @@ fn resolve_managed_root( path_manager: &PathManager, ) -> Result { let configured = settings.root_path.trim(); - if configured.is_empty() || configured == "~/.bitfun/worktrees" { + let portable_configured = configured.replace('\\', "/"); + if portable_configured.is_empty() || portable_configured == "~/.bitfun/worktrees" { return Ok(path_manager.worktrees_root()); } - if configured == "~" { + if portable_configured == "~" { return dirs::home_dir().ok_or_else(|| { error( WorktreeErrorCode::InvalidPath, @@ -1185,7 +1186,7 @@ fn resolve_managed_root( ) }); } - if let Some(suffix) = configured.strip_prefix("~/") { + if let Some(suffix) = portable_configured.strip_prefix("~/") { return dirs::home_dir() .map(|home| home.join(suffix)) .ok_or_else(|| { @@ -1199,7 +1200,7 @@ fn resolve_managed_root( if !path.is_absolute() { return Err(error( WorktreeErrorCode::InvalidPath, - "Worktree root must be an absolute path or start with ~/", + "Worktree root must be an absolute path or start with ~/ (or ~\\ on Windows)", )); } Ok(path) @@ -1486,6 +1487,21 @@ mod tests { assert!(resolve_managed_root(&settings, &path_manager).is_err()); } + #[test] + fn windows_style_default_root_uses_the_managed_path_contract() { + let user_root = std::env::temp_dir().join("bitfun-worktree-root-test"); + let path_manager = PathManager::with_user_root_for_tests(user_root); + let settings = WorktreeSettings { + root_path: r"~\.bitfun\worktrees".to_string(), + ..WorktreeSettings::default() + }; + + assert_eq!( + resolve_managed_root(&settings, &path_manager).unwrap(), + path_manager.worktrees_root() + ); + } + #[test] fn request_ids_map_to_stable_session_ids() { let first = WorktreeService::session_id_for_request("request-123").unwrap(); diff --git a/src/crates/assembly/core/src/service/worktree/session_binding.rs b/src/crates/assembly/core/src/service/worktree/session_binding.rs index 1a98eee8b2..5af0c6c87b 100644 --- a/src/crates/assembly/core/src/service/worktree/session_binding.rs +++ b/src/crates/assembly/core/src/service/worktree/session_binding.rs @@ -10,7 +10,7 @@ //! directory underneath it would silently invalidate the history. use crate::agentic::coordination::get_global_coordinator; -use crate::agentic::session::SessionExecutionBindingUpdate; +use crate::agentic::session::{SessionExecutionBindingError, SessionExecutionBindingUpdate}; use crate::service::remote_ssh::lookup_remote_connection; use crate::service::workspace::get_global_workspace_service; use crate::service::worktree::{ @@ -27,6 +27,9 @@ use std::path::Path; pub struct WorktreeSessionBindingRequest { pub request_id: String, pub session_id: String, + /// Stable owner path used to locate view-only or evicted persisted sessions. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub project_workspace_path: Option, /// `true` moves the session into a managed worktree, `false` back to the project. pub enabled: bool, } @@ -59,47 +62,109 @@ fn error(code: WorktreeErrorCode, message: impl Into) -> WorktreeError { } } -async fn load_binding_context(session_id: &str) -> Result { +async fn load_binding_context( + request: &WorktreeSessionBindingRequest, +) -> Result { let coordinator = get_global_coordinator().ok_or_else(|| { error( WorktreeErrorCode::IoFailed, "Session coordinator is not initialized", ) })?; - let session = coordinator - .get_session_manager() - .get_session(session_id) - .ok_or_else(|| { + let session_manager = coordinator.get_session_manager(); + let session = session_manager.get_session(&request.session_id); + + let (workspace_path, project_workspace_path, execution_target) = if let Some(session) = session + { + if !session.dialog_turn_ids.is_empty() { + return Err(error( + WorktreeErrorCode::WorktreeBusy, + "Worktree isolation can only be changed before the session's first message", + )); + } + if !matches!(session.state, crate::agentic::core::SessionState::Idle) { + return Err(error( + WorktreeErrorCode::WorktreeBusy, + "Worktree isolation cannot be changed while the session is processing", + )); + } + if session.config.remote_connection_id.is_some() { + return Err(error( + WorktreeErrorCode::RemoteUnsupported, + "Managed worktrees are not supported for remote SSH workspaces yet", + )); + } + + let workspace_path = session.config.workspace_path.clone().ok_or_else(|| { error( - WorktreeErrorCode::WorktreeNotFound, - format!("Session not found: {session_id}"), + WorktreeErrorCode::InvalidPath, + "Session is not bound to a workspace", ) })?; + let project_workspace_path = session + .config + .project_workspace_path + .clone() + .unwrap_or_else(|| workspace_path.clone()); + let execution_target = session + .config + .execution_target + .clone() + .unwrap_or_else(|| SessionExecutionTarget::local(workspace_path.clone())); + (workspace_path, project_workspace_path, execution_target) + } else { + let project_workspace_path = request + .project_workspace_path + .as_deref() + .map(str::trim) + .filter(|path| !path.is_empty()) + .ok_or_else(|| { + error( + WorktreeErrorCode::WorktreeNotFound, + format!( + "Session not found: {}. The project workspace path is required to restore historical sessions", + request.session_id + ), + ) + })? + .to_string(); + let metadata = session_manager + .load_session_metadata(Path::new(&project_workspace_path), &request.session_id) + .await + .map_err(|metadata_error| { + error( + WorktreeErrorCode::IoFailed, + format!("Failed to load session metadata: {metadata_error}"), + ) + })? + .ok_or_else(|| { + error( + WorktreeErrorCode::WorktreeNotFound, + format!("Session not found: {}", request.session_id), + ) + })?; + if metadata.turn_count > 0 { + return Err(error( + WorktreeErrorCode::WorktreeBusy, + "Worktree isolation can only be changed before the session's first message", + )); + } - if !session.dialog_turn_ids.is_empty() { - return Err(error( - WorktreeErrorCode::WorktreeBusy, - "Worktree isolation can only be changed before the session's first message", - )); - } - if session.config.remote_connection_id.is_some() { - return Err(error( - WorktreeErrorCode::RemoteUnsupported, - "Managed worktrees are not supported for remote SSH workspaces yet", - )); - } + let workspace_path = metadata + .workspace_path + .clone() + .unwrap_or_else(|| project_workspace_path.clone()); + let persisted_project_path = metadata + .project_workspace_path + .clone() + .unwrap_or(project_workspace_path); + let execution_target = metadata + .execution_target + .clone() + .unwrap_or_else(|| SessionExecutionTarget::local(workspace_path.clone())); + (workspace_path, persisted_project_path, execution_target) + }; - let workspace_path = session.config.workspace_path.clone().ok_or_else(|| { - error( - WorktreeErrorCode::InvalidPath, - "Session is not bound to a workspace", - ) - })?; - let project_workspace_path = session - .config - .project_workspace_path - .clone() - .unwrap_or_else(|| workspace_path.clone()); if lookup_remote_connection(&project_workspace_path) .await .is_some() @@ -110,11 +175,12 @@ async fn load_binding_context(session_id: &str) -> Result { + error(WorktreeErrorCode::WorktreeBusy, message) + } + SessionExecutionBindingError::NotFound(message) => { + error(WorktreeErrorCode::WorktreeNotFound, message) + } + SessionExecutionBindingError::Internal(internal) => error( WorktreeErrorCode::IoFailed, - format!("Failed to rebind session workspace: {session_error}"), - ) + format!("Failed to rebind session workspace: {internal}"), + ), })?; Ok(WorktreeSessionBindingResult { @@ -179,7 +251,7 @@ impl WorktreeService { pub async fn bind_session( request: WorktreeSessionBindingRequest, ) -> Result { - let context = load_binding_context(&request.session_id).await?; + let context = load_binding_context(&request).await?; let is_worktree = context.execution_target.worktree_id.is_some(); if request.enabled == is_worktree { @@ -308,3 +380,36 @@ impl WorktreeService { Ok(result) } } + +#[cfg(test)] +mod tests { + use super::WorktreeSessionBindingRequest; + + #[test] + fn binding_request_keeps_legacy_callers_compatible() { + let request: WorktreeSessionBindingRequest = serde_json::from_value(serde_json::json!({ + "requestId": "request-1", + "sessionId": "session-1", + "enabled": true + })) + .expect("legacy request should deserialize"); + + assert_eq!(request.project_workspace_path, None); + } + + #[test] + fn binding_request_uses_a_cross_platform_project_locator() { + let request: WorktreeSessionBindingRequest = serde_json::from_value(serde_json::json!({ + "requestId": "request-2", + "sessionId": "session-2", + "projectWorkspacePath": "D:\\workspace\\BitFun", + "enabled": false + })) + .expect("request should deserialize"); + + assert_eq!( + request.project_workspace_path.as_deref(), + Some(r"D:\workspace\BitFun") + ); + } +} diff --git a/src/web-ui/src/flow_chat/components/ChatInput.tsx b/src/web-ui/src/flow_chat/components/ChatInput.tsx index 10838ab461..cdc09b55b2 100644 --- a/src/web-ui/src/flow_chat/components/ChatInput.tsx +++ b/src/web-ui/src/flow_chat/components/ChatInput.tsx @@ -81,6 +81,10 @@ import { import { isReviewSlashCommand } from '../deep-review/launch/commandParser'; import { createLogger } from '@/shared/utils/logger'; import { isSamePath } from '@/shared/utils/pathUtils'; +import { + isSessionWorktreeBindingLocked, + sessionWorktreeBindingSubscriptionKey, +} from '../utils/sessionWorktree'; import { isTauriRuntime } from '@/infrastructure/runtime'; import { Tooltip, IconButton, confirmDanger, confirmWarning } from '@/component-library'; import { PendingQueuePanel } from './PendingQueuePanel'; @@ -833,6 +837,15 @@ export const ChatInput: React.FC = ({ || workspacePath || '' ).trim(); + const sessionProjectWorkspacePath = ( + (!hasRegisteredWorkspace && ( + effectiveTargetSession?.config.projectWorkspacePath + || effectiveTargetSession?.projectWorkspacePath + || effectiveTargetSession?.workspacePath + )) + || workspacePath + || '' + ).trim(); const workspacePathRef = useRef(sessionBoundWorkspacePath); workspacePathRef.current = sessionBoundWorkspacePath; const { openedWorkspaces } = useWorkspaceContext(); @@ -1031,7 +1044,7 @@ export const ChatInput: React.FC = ({ `${s.remoteConnectionId ?? ''}|${s.remoteSshHost ?? ''}|${s.lastSubmittedMode ?? ''}|` + `${s.currentAcpContextUsage?.used ?? ''}|${s.currentAcpContextUsage?.size ?? ''}|` + `${s.currentTokenUsage?.inputTokens ?? ''}|${s.maxContextTokens ?? ''}|` + - `${s.needsUserAttention ? '1':'0'}` + `${s.needsUserAttention ? '1':'0'}|${sessionWorktreeBindingSubscriptionKey(s)}` ); } } @@ -1900,17 +1913,37 @@ export const ChatInput: React.FC = ({ if (effectiveTargetSession.remoteConnectionId) return undefined; if (isSubagentInputTarget || isAcpTargetSession) return undefined; - const locked = effectiveTargetSession.dialogTurns.length > 0 - || effectiveTargetSession.status === 'active'; + const locked = isSessionWorktreeBindingLocked( + effectiveTargetSession, + !!derivedState?.isProcessing, + ); return { locked, onChange: async (enabled: boolean) => { + const latestSession = FlowChatStore.getInstance() + .getState() + .sessions + .get(effectiveTargetSessionId); + if ( + !latestSession + || isSessionWorktreeBindingLocked(latestSession, false) + ) { + notificationService.error(tWorktrees('strip.toggleLocked')); + return; + } + const latestProjectWorkspacePath = ( + latestSession.config.projectWorkspacePath + || latestSession.projectWorkspacePath + || latestSession.workspacePath + || sessionProjectWorkspacePath + ).trim(); try { const result = await worktreeAPI.bindSession( effectiveTargetSessionId, enabled, globalThis.crypto?.randomUUID?.() ?? `worktree-${Date.now()}`, + latestProjectWorkspacePath, ); FlowChatStore.getInstance().updateSessionExecutionTarget(effectiveTargetSessionId, { workspacePath: result.workspacePath, @@ -1936,8 +1969,10 @@ export const ChatInput: React.FC = ({ }, [ effectiveTargetSession, effectiveTargetSessionId, + derivedState?.isProcessing, isAcpTargetSession, isSubagentInputTarget, + sessionProjectWorkspacePath, tWorktrees, ]); diff --git a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.test.tsx b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.test.tsx index aa65fac3e5..b8deeed0e2 100644 --- a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.test.tsx +++ b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.test.tsx @@ -183,6 +183,33 @@ describe('ChatInputWorkspaceStrip git refresh behavior', () => { expect(onChange).toHaveBeenCalledWith(true); }); + it('coalesces repeated clicks while a worktree transition is pending', async () => { + let finishTransition!: () => void; + const onChange = vi.fn(() => new Promise(resolve => { + finishTransition = resolve; + })); + await act(async () => { + root.render( + + ); + }); + + const toggle = container.querySelector('[data-testid="chat-input-worktree-toggle"]'); + act(() => { + toggle?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + toggle?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); + + expect(onChange).toHaveBeenCalledOnce(); + await act(async () => { + finishTransition(); + }); + }); + it('shows the toggle as on inside a worktree and asks to turn it off', async () => { const onChange = vi.fn(async () => undefined); await act(async () => { diff --git a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.tsx b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.tsx index f908fd7e3d..2144809f24 100644 --- a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.tsx +++ b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStrip.tsx @@ -85,6 +85,7 @@ export const ChatInputWorkspaceStrip: React.FC = ( const permissionRootRef = useRef(null); const [permissionMenuOpen, setPermissionMenuOpen] = useState(false); const [worktreePending, setWorktreePending] = useState(false); + const worktreePendingRef = useRef(false); const trimmedPath = repositoryPath.trim(); const label = workspaceLabel.trim(); @@ -196,11 +197,13 @@ export const ChatInputWorkspaceStrip: React.FC = ( const showPermissionLabel = permissionMode !== 'acp'; const handleWorktreeToggle = () => { - if (!worktreeControl || worktreeToggleDisabled) { + if (!worktreeControl || worktreeToggleDisabled || worktreePendingRef.current) { return; } + worktreePendingRef.current = true; setWorktreePending(true); void Promise.resolve(worktreeControl.onChange(!isWorktree)).finally(() => { + worktreePendingRef.current = false; setWorktreePending(false); }); }; diff --git a/src/web-ui/src/flow_chat/utils/sessionWorktree.test.ts b/src/web-ui/src/flow_chat/utils/sessionWorktree.test.ts new file mode 100644 index 0000000000..f7296daf0c --- /dev/null +++ b/src/web-ui/src/flow_chat/utils/sessionWorktree.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from 'vitest'; +import type { Session } from '../types/flow-chat'; +import { + isSessionWorktreeBindingLocked, + sessionWorktreeBindingSubscriptionKey, +} from './sessionWorktree'; + +function session(overrides: Partial = {}): Session { + return { + sessionId: 'session-1', + dialogTurns: [], + status: 'active', + config: { + executionTarget: { + kind: 'local', + rootPath: '/repo', + }, + }, + createdAt: 0, + lastActiveAt: 0, + error: null, + sessionKind: 'normal', + ...overrides, + }; +} + +describe('session worktree control', () => { + it('does not treat the selected session status as runtime processing', () => { + expect(isSessionWorktreeBindingLocked(session({ status: 'active' }), false)).toBe(false); + expect(isSessionWorktreeBindingLocked(session(), true)).toBe(true); + }); + + it('locks metadata-only history before its dialog turns are hydrated', () => { + expect(isSessionWorktreeBindingLocked(session({ totalTurnCount: 1 }), false)).toBe(true); + }); + + it('invalidates the composer subscription after hydrate and rebind', () => { + const initial = sessionWorktreeBindingSubscriptionKey(session()); + const hydrated = sessionWorktreeBindingSubscriptionKey(session({ totalTurnCount: 1 })); + const rebound = sessionWorktreeBindingSubscriptionKey(session({ + workspacePath: '/worktrees/wt-1', + projectWorkspacePath: '/repo', + config: { + projectWorkspacePath: '/repo', + executionTarget: { + kind: 'managedWorktree', + worktreeId: 'wt-1', + rootPath: '/worktrees/wt-1', + }, + }, + })); + + expect(hydrated).not.toBe(initial); + expect(rebound).not.toBe(initial); + }); +}); diff --git a/src/web-ui/src/flow_chat/utils/sessionWorktree.ts b/src/web-ui/src/flow_chat/utils/sessionWorktree.ts new file mode 100644 index 0000000000..5d5518a8a9 --- /dev/null +++ b/src/web-ui/src/flow_chat/utils/sessionWorktree.ts @@ -0,0 +1,34 @@ +import type { Session } from '../types/flow-chat'; + +type SessionWorktreeFacts = Pick< + Session, + 'dialogTurns' | 'totalTurnCount' | 'workspaceId' | 'workspacePath' | 'projectWorkspacePath' | 'config' +>; + +export function isSessionWorktreeBindingLocked( + session: Pick, + isProcessing: boolean, +): boolean { + return session.dialogTurns.length > 0 + || (session.totalTurnCount ?? 0) > 0 + || isProcessing; +} + +/** + * Fields read by the composer that can change after a historical-session + * hydrate or a worktree transition. Including them in the store selector keeps + * the toggle state and project locator from using a stale session snapshot. + */ +export function sessionWorktreeBindingSubscriptionKey(session: SessionWorktreeFacts): string { + return [ + session.dialogTurns.length, + session.totalTurnCount ?? '', + session.workspaceId ?? '', + session.workspacePath ?? '', + session.projectWorkspacePath ?? '', + session.config.projectWorkspacePath ?? '', + session.config.executionTarget?.kind ?? '', + session.config.executionTarget?.worktreeId ?? '', + session.config.executionTarget?.rootPath ?? '', + ].join('|'); +} diff --git a/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts b/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts index f57730dad9..7457be3f6d 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts @@ -122,6 +122,32 @@ describe('ApiClient startup trace classification', () => { }); }); + it('uses the message from plain structured Tauri errors', async () => { + const transportError = { + code: 'worktree_not_found', + message: 'Session not found: history-1', + }; + adapterMocks.request.mockRejectedValueOnce(transportError); + const client = new ApiClient({ enableLogging: false, retries: 0 }); + + const error = await client.invoke('worktree_bind_session', { + request: { sessionId: 'history-1', enabled: true }, + }).catch((caught: unknown) => caught as { + code: string; + message: string; + details?: { originalError?: unknown }; + }); + + expect(error).toMatchObject({ + code: 'COMMAND_FAILED', + message: 'Session not found: history-1', + details: { + originalError: transportError, + }, + }); + expect(error.message).not.toBe('[object Object]'); + }); + it('keeps message-only transport errors parseable by domain adapters', async () => { const encoded = JSON.stringify({ code: 'stale_revision', diff --git a/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts b/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts index 1a6da78d55..50bf3e8966 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts @@ -34,6 +34,30 @@ function shouldEstimateApiPayloadBytes(): boolean { return globalThis.__BITFUN_PERF_TRACE_ENABLED__ === true; } +function transportErrorMessage(error: unknown): string { + if (error instanceof Error) { + return error.message; + } + if (typeof error === 'string') { + return error; + } + if (error && typeof error === 'object') { + const record = error as Record; + for (const key of ['message', 'detail', 'error']) { + const value = record[key]; + if (typeof value === 'string' && value.trim()) { + return value; + } + } + try { + return JSON.stringify(error) || 'Unknown command error'; + } catch { + return 'Unknown command error'; + } + } + return String(error); +} + function apiErrorCause(error: unknown): unknown { if (!(error instanceof Error)) { return error; @@ -63,7 +87,7 @@ function isOptionalConfigNotFoundCommand(config: TauriCommandConfig, error: unkn return false; } - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = transportErrorMessage(error); const normalized = errorMessage.toLowerCase(); return normalized.includes('not found') && normalized.includes('config path'); } @@ -421,7 +445,7 @@ export class ApiClient implements IApiClient { timestamp: new Date() }; } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = transportErrorMessage(error); const isExpectedError = errorMessage.includes('not found') || diff --git a/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.test.ts index 75aa1fd0a7..97f98f079e 100644 --- a/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.test.ts @@ -51,6 +51,51 @@ describe('WorktreeAPI', () => { } satisfies Partial); }); + it('unwraps domain errors nested by ApiClient command handling', async () => { + const transportError = Object.assign(new Error('Session not found: history-1'), { + code: 'COMMAND_FAILED', + details: { + originalError: { + code: 'worktree_not_found', + message: 'Session not found: history-1', + }, + }, + }); + invokeMock.mockRejectedValue(transportError); + + await expect( + api.bindSession('history-1', true, 'request-3', 'D:\\workspace\\BitFun'), + ).rejects.toMatchObject({ + name: 'WorktreeCommandError', + code: 'worktree_not_found', + message: 'Session not found: history-1', + } satisfies Partial); + }); + + it('sends the project locator when binding a historical session', async () => { + invokeMock.mockResolvedValue({ + sessionId: 'history-1', + workspacePath: '/worktrees/wt-1', + projectWorkspacePath: '/repo', + executionTarget: { + kind: 'managedWorktree', + worktreeId: 'wt-1', + rootPath: '/worktrees/wt-1', + }, + }); + + await api.bindSession('history-1', true, 'request-4', '/repo'); + + expect(invokeMock).toHaveBeenCalledWith('worktree_bind_session', { + request: { + sessionId: 'history-1', + enabled: true, + requestId: 'request-4', + projectWorkspacePath: '/repo', + }, + }); + }); + it('subscribes to event-driven worktree updates', () => { const unsubscribe = vi.fn(); const callback = vi.fn(); diff --git a/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.ts b/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.ts index 32d917b704..22b1ae4ac1 100644 --- a/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/WorktreeAPI.ts @@ -114,14 +114,48 @@ export interface WorktreeSessionBindingResult { retainedWorktreePath?: string; } +const WORKTREE_ERROR_CODES = new Set([ + 'remote_unsupported', + 'not_git_repository', + 'unborn_repo', + 'invalid_base_ref', + 'worktree_not_found', + 'worktree_busy', + 'worktree_locked', + 'dirty_worktree', + 'unpublished_commits', + 'copy_conflict', + 'invalid_path', + 'branch_exists', + 'request_conflict', + 'rollback_incomplete', + 'git_failed', + 'io_failed', +]); + +function isWorktreeErrorCode(value: unknown): value is WorktreeErrorCode { + return typeof value === 'string' && WORKTREE_ERROR_CODES.has(value as WorktreeErrorCode); +} + +function fallbackErrorMessage(error: unknown): string { + if (error instanceof Error) return error.message; + if (error && typeof error === 'object') { + const message = (error as { message?: unknown }).message; + if (typeof message === 'string' && message.trim()) return message; + try { + return JSON.stringify(error) || 'Unknown worktree command error'; + } catch { + return 'Unknown worktree command error'; + } + } + return String(error); +} + export function toWorktreeCommandError(error: unknown): WorktreeCommandError { const candidates: unknown[] = [error]; - if (error instanceof Error) { - const enriched = error as Error & { data?: unknown; cause?: unknown }; - candidates.push(enriched.data, enriched.cause, error.message); - } - for (const candidate of candidates) { - let value = candidate; + const visited = new Set(); + while (candidates.length > 0) { + let value = candidates.shift(); if (typeof value === 'string') { try { value = JSON.parse(value); @@ -130,17 +164,31 @@ export function toWorktreeCommandError(error: unknown): WorktreeCommandError { } } if (value && typeof value === 'object') { - const payload = value as Partial; - if (typeof payload.code === 'string' && typeof payload.message === 'string') { + if (visited.has(value)) continue; + visited.add(value); + const payload = value as Partial & Record; + if (isWorktreeErrorCode(payload.code) && typeof payload.message === 'string') { return new WorktreeCommandError( - payload.code as WorktreeErrorCode, + payload.code, payload.message, payload.recoveryPath, ); } + const details = payload.details; + candidates.push( + payload.data, + payload.cause, + payload.originalError, + payload.error, + payload.message, + details && typeof details === 'object' + ? (details as Record).originalError + : undefined, + details, + ); } } - return new WorktreeCommandError('git_failed', error instanceof Error ? error.message : String(error)); + return new WorktreeCommandError('git_failed', fallbackErrorMessage(error)); } async function invokeWorktree(command: string, request: unknown): Promise { @@ -220,8 +268,14 @@ export class WorktreeAPI { sessionId: string, enabled: boolean, requestId: string, + projectWorkspacePath: string, ): Promise { - return invokeWorktree('worktree_bind_session', { sessionId, enabled, requestId }); + return invokeWorktree('worktree_bind_session', { + sessionId, + enabled, + requestId, + projectWorkspacePath, + }); } onChanged(callback: (event: WorktreeChangedEvent) => void): () => void { From 1bf5460aea130831c3a83db4286048c78b02ac5d Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Mon, 27 Jul 2026 23:31:48 -0700 Subject: [PATCH 2/2] fix(worktree): serialize session binding transitions --- .../src/service/worktree/session_binding.rs | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/crates/assembly/core/src/service/worktree/session_binding.rs b/src/crates/assembly/core/src/service/worktree/session_binding.rs index 5af0c6c87b..598a685f50 100644 --- a/src/crates/assembly/core/src/service/worktree/session_binding.rs +++ b/src/crates/assembly/core/src/service/worktree/session_binding.rs @@ -10,6 +10,7 @@ //! directory underneath it would silently invalidate the history. use crate::agentic::coordination::get_global_coordinator; +use crate::agentic::keyed_lock::KeyedAsyncLock; use crate::agentic::session::{SessionExecutionBindingError, SessionExecutionBindingUpdate}; use crate::service::remote_ssh::lookup_remote_connection; use crate::service::workspace::get_global_workspace_service; @@ -21,6 +22,16 @@ use bitfun_core_types::{ }; use serde::{Deserialize, Serialize}; use std::path::Path; +use std::sync::LazyLock; + +/// Serializes the complete Git-create/rebind/release transition for one session. +/// +/// The SessionManager mutation lock closes the race with turn start, but it is +/// intentionally held only around the final session mutation. A separate lock +/// is needed here so concurrent adapters in one product runtime cannot both +/// preflight the same empty session, create different worktrees, and then +/// overwrite each other's binding. +static SESSION_BINDING_LOCKS: LazyLock = LazyLock::new(KeyedAsyncLock::default); #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -251,6 +262,9 @@ impl WorktreeService { pub async fn bind_session( request: WorktreeSessionBindingRequest, ) -> Result { + bitfun_core_types::validate_session_id(&request.session_id) + .map_err(|message| error(WorktreeErrorCode::InvalidPath, message))?; + let _binding_guard = SESSION_BINDING_LOCKS.lock(&request.session_id).await; let context = load_binding_context(&request).await?; let is_worktree = context.execution_target.worktree_id.is_some(); @@ -383,7 +397,8 @@ impl WorktreeService { #[cfg(test)] mod tests { - use super::WorktreeSessionBindingRequest; + use super::{WorktreeSessionBindingRequest, SESSION_BINDING_LOCKS}; + use std::time::Duration; #[test] fn binding_request_keeps_legacy_callers_compatible() { @@ -412,4 +427,28 @@ mod tests { Some(r"D:\workspace\BitFun") ); } + + #[tokio::test] + async fn binding_transitions_for_the_same_session_are_serialized() { + let session_id = format!("binding-lock-{}", uuid::Uuid::new_v4()); + let first = SESSION_BINDING_LOCKS.lock(&session_id).await; + + assert!( + tokio::time::timeout( + Duration::from_millis(20), + SESSION_BINDING_LOCKS.lock(&session_id), + ) + .await + .is_err(), + "a second transition must wait for the first" + ); + + drop(first); + tokio::time::timeout( + Duration::from_secs(1), + SESSION_BINDING_LOCKS.lock(&session_id), + ) + .await + .expect("the next transition should proceed after release"); + } }