diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index 948eae0437..7df78f497d 100644 --- a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs +++ b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs @@ -5807,7 +5807,7 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet // Get latest session, restoring from persistence on demand so every entry // point can use the same start_dialog_turn flow. A loaded session must keep // the same storage identity as this invocation. - let session = match loaded_session { + let mut session = match loaded_session { Some(session) => { if let Some(restore) = requested_restore.as_ref() { self.session_manager.ensure_session_storage_path( @@ -5905,9 +5905,14 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet &session_id, &effective_agent_type, primary_agent_binding.route_owner, - primary_route_key, + primary_route_key.clone(), ) .await?; + // The manager owns a different Session clone. Keep this turn's + // admission snapshot aligned with the binding changed above. + session.agent_type = effective_agent_type.clone(); + session.config.agent_route_owner = primary_agent_binding.route_owner; + session.config.agent_route_key = primary_route_key; } debug!( @@ -14817,9 +14822,9 @@ mod tests { session_storage_workspace_locator, turn_review_manifest_for_agent, validate_required_lineage_turns_settled, ActiveSubagentExecution, BackgroundSubagentWaitMode, ContextCompactionOutcome, ConversationCoordinator, - InterruptedTurnIntentState, ManualCompactionCommitGate, SessionMemoryMode, - SessionReferenceLocator, SessionRelationshipKind, SubagentExecutionRequest, - TEST_AGENT_MODEL_DEFAULTS, + DialogSubmissionPolicy, DialogTriggerSource, InterruptedTurnIntentState, + ManualCompactionCommitGate, SessionMemoryMode, SessionReferenceLocator, + SessionRelationshipKind, SubagentExecutionRequest, TEST_AGENT_MODEL_DEFAULTS, }; use crate::agentic::agents::ExternalSubagentModelBinding; use crate::agentic::coordination::coordination_store::{ @@ -14840,7 +14845,7 @@ mod tests { use crate::agentic::session::{ compression::{CompressionConfig, ContextCompressor}, PromptCachePolicy, SessionContextStore, SessionManager, SessionManagerConfig, - SystemPromptCacheIdentity, UserContextCacheIdentity, + SystemPromptCacheIdentity, UserContextCacheIdentity, TEST_MODEL_RESOLUTION_AI_CONFIG, }; use crate::agentic::skill_agent_snapshot::SkillSnapshotEntry; use crate::agentic::tools::framework::{ @@ -17191,6 +17196,66 @@ mod tests { } } + #[tokio::test] + async fn review_fixer_turn_is_admitted_after_updating_a_deep_review_session_binding() { + let (coordinator, session_manager) = test_coordinator(); + let workspace = tempfile::tempdir().expect("review workspace"); + let workspace_path = workspace.path().to_string_lossy().into_owned(); + let session = session_manager + .create_session( + "Deep review remediation".to_string(), + "DeepReview".to_string(), + SessionConfig { + workspace_path: Some(workspace_path.clone()), + model_id: Some("review-model".to_string()), + enable_tools: true, + ..Default::default() + }, + ) + .await + .expect("DeepReview session should be created"); + let ai_config = AIConfig { + models: vec![AIModelConfig { + id: "review-model".to_string(), + name: "Review model".to_string(), + provider: "openai".to_string(), + model_name: "test-model".to_string(), + enabled: true, + ..AIModelConfig::default() + }], + ..AIConfig::default() + }; + let fix_turn_id = "review-fix-turn"; + TEST_MODEL_RESOLUTION_AI_CONFIG + .scope( + ai_config, + coordinator.start_dialog_turn( + session.session_id.clone(), + "fix selected findings".to_string(), + Some("fix selected findings".to_string()), + Some(fix_turn_id.to_string()), + "ReviewFixer".to_string(), + Some(workspace_path), + None, + None, + DialogSubmissionPolicy::for_source(DialogTriggerSource::DesktopApi), + None, + ), + ) + .await + .expect("ReviewFixer turn should pass admission after the intentional binding update"); + + let updated = session_manager + .get_session(&session.session_id) + .expect("review session should remain loaded"); + assert_eq!(updated.agent_type, "ReviewFixer"); + assert_eq!(session_manager.get_turn_count(&session.session_id), 1); + + let _ = coordinator + .cancel_dialog_turn(&session.session_id, fix_turn_id) + .await; + } + #[tokio::test] async fn assistant_bootstrap_checks_runtime_ownership_before_files_or_attach() { let ownership_root = tempfile::tempdir().expect("ownership root"); 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 855fd7a848..c76271448c 100644 --- a/src/crates/assembly/core/src/agentic/session/session_manager.rs +++ b/src/crates/assembly/core/src/agentic/session/session_manager.rs @@ -128,8 +128,17 @@ pub(crate) struct TurnAdmissionSessionFacts { model_id: Option, reasoning_preset: Option, permission_mode: Option, + max_context_tokens: usize, agent_type: String, + agent_route_owner: SessionAgentRouteOwner, + agent_route_key: Option, enable_tools: bool, + workspace_path: Option, + project_workspace_path: Option, + execution_target: Option, + workspace_id: Option, + remote_connection_id: Option, + remote_ssh_host: Option, } impl TurnAdmissionSessionFacts { @@ -138,8 +147,17 @@ impl TurnAdmissionSessionFacts { model_id: session.config.model_id.clone(), reasoning_preset: session.config.reasoning_preset.clone(), permission_mode: session.config.permission_mode, + max_context_tokens: session.config.max_context_tokens, agent_type: session.agent_type.clone(), + agent_route_owner: session.config.agent_route_owner, + agent_route_key: session.config.agent_route_key.clone(), enable_tools: session.config.enable_tools, + workspace_path: session.config.workspace_path.clone(), + project_workspace_path: session.config.project_workspace_path.clone(), + execution_target: session.config.execution_target.clone(), + workspace_id: session.config.workspace_id.clone(), + remote_connection_id: session.config.remote_connection_id.clone(), + remote_ssh_host: session.config.remote_ssh_host.clone(), } } @@ -152,8 +170,17 @@ impl TurnAdmissionSessionFacts { self.model_id == session.config.model_id && self.reasoning_preset == session.config.reasoning_preset && self.permission_mode == session.config.permission_mode + && self.max_context_tokens == session.config.max_context_tokens && self.agent_type == session.agent_type + && self.agent_route_owner == session.config.agent_route_owner + && self.agent_route_key == session.config.agent_route_key && self.enable_tools == session.config.enable_tools + && self.workspace_path == session.config.workspace_path + && self.project_workspace_path == session.config.project_workspace_path + && self.execution_target == session.config.execution_target + && self.workspace_id == session.config.workspace_id + && self.remote_connection_id == session.config.remote_connection_id + && self.remote_ssh_host == session.config.remote_ssh_host } } @@ -7240,9 +7267,10 @@ impl SessionManager { } /// Persist a Turn only if the execution-affecting Session settings still - /// match the snapshot used to resolve its model, permission, prompt, and - /// reasoning metadata. The validation and Turn append share one Session - /// mutation lock, so a concurrent settings write must retry admission. + /// match the snapshot used to resolve its model, permission, agent route, + /// workspace, prompt, and reasoning metadata. The validation and Turn + /// append share one Session mutation lock, so a concurrent settings write + /// must retry admission. #[allow(clippy::too_many_arguments)] pub(crate) async fn start_dialog_turn_with_prepended_messages_if_session_matches( &self, @@ -10684,6 +10712,210 @@ mod tests { assert_eq!(manager.get_turn_count(&session.session_id), 0); } + #[tokio::test] + async fn dialog_turn_admission_rejects_a_concurrent_context_window_change() { + 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( + "Turn admission context window CAS".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().to_string()), + model_id: Some("model-original".to_string()), + max_context_tokens: 128_128, + ..SessionConfig::default() + }, + ) + .await + .expect("session should create"); + let expected = TurnAdmissionSessionFacts::from_session(&session); + TEST_MODEL_RESOLUTION_AI_CONFIG + .scope( + ServiceAIConfig { + models: vec![test_model("model-original", 256_000)], + ..Default::default() + }, + manager.update_session_model_selection(&session.session_id, "model-original", None), + ) + .await + .expect("same-model context window refresh should succeed"); + + let error = manager + .start_dialog_turn_with_prepended_messages_if_session_matches( + &session.session_id, + "agentic".to_string(), + "must reject stale context window".to_string(), + Some("turn-admission-context-window-race".to_string()), + None, + Vec::new(), + None, + &expected, + ) + .await + .expect_err("a concurrent context window update must invalidate admission"); + + assert!(error.to_string().contains("changed during turn admission")); + assert_eq!(manager.get_turn_count(&session.session_id), 0); + } + + #[tokio::test] + async fn dialog_turn_admission_rejects_a_concurrent_agent_route_owner_change() { + 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( + "Turn admission route CAS".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().to_string()), + ..SessionConfig::default() + }, + ) + .await + .expect("session should create"); + let expected = TurnAdmissionSessionFacts::from_session(&session); + manager + .update_session_agent_binding( + &session.session_id, + "agentic", + SessionAgentRouteOwner::External, + None, + ) + .await + .expect("same-name route owner update should succeed"); + + let error = manager + .start_dialog_turn_with_prepended_messages_if_session_matches( + &session.session_id, + "agentic".to_string(), + "must reject stale route owner".to_string(), + Some("turn-admission-route-race".to_string()), + None, + Vec::new(), + None, + &expected, + ) + .await + .expect_err("a concurrent route owner update must invalidate admission"); + + assert!(error.to_string().contains("changed during turn admission")); + assert_eq!(manager.get_turn_count(&session.session_id), 0); + } + + #[tokio::test] + async fn dialog_turn_admission_rejects_a_concurrent_agent_route_key_change() { + 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( + "Turn admission route key CAS".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().to_string()), + agent_route_key: Some("local:agentic:v1".to_string()), + ..SessionConfig::default() + }, + ) + .await + .expect("session should create"); + let expected = TurnAdmissionSessionFacts::from_session(&session); + manager + .update_session_agent_binding( + &session.session_id, + "agentic", + SessionAgentRouteOwner::Local, + Some("local:agentic:v2".to_string()), + ) + .await + .expect("same-owner route key update should succeed"); + + let error = manager + .start_dialog_turn_with_prepended_messages_if_session_matches( + &session.session_id, + "agentic".to_string(), + "must reject stale route key".to_string(), + Some("turn-admission-route-key-race".to_string()), + None, + Vec::new(), + None, + &expected, + ) + .await + .expect_err("a concurrent route key update must invalidate admission"); + + assert!(error.to_string().contains("changed during turn admission")); + assert_eq!(manager.get_turn_count(&session.session_id), 0); + } + + #[tokio::test] + async fn dialog_turn_admission_rejects_a_concurrent_execution_binding_change() { + let workspace = TestWorkspace::new(); + let persistence_manager = Arc::new( + PersistenceManager::new(workspace.path_manager()).expect("persistence manager"), + ); + let manager = test_manager(persistence_manager); + let original_workspace = workspace.path().to_string_lossy().to_string(); + let session = manager + .create_session( + "Turn admission workspace CAS".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(original_workspace.clone()), + project_workspace_path: Some(original_workspace.clone()), + execution_target: Some(SessionExecutionTarget::local( + original_workspace.clone(), + )), + workspace_id: Some("workspace-original".to_string()), + ..SessionConfig::default() + }, + ) + .await + .expect("session should create"); + let expected = TurnAdmissionSessionFacts::from_session(&session); + let rebound_workspace = workspace.path().join("managed-worktree"); + manager + .update_session_execution_binding( + &session.session_id, + SessionExecutionBindingUpdate { + workspace_path: rebound_workspace.to_string_lossy().to_string(), + project_workspace_path: original_workspace, + workspace_id: Some("workspace-rebound".to_string()), + execution_target: SessionExecutionTarget::local( + rebound_workspace.to_string_lossy().to_string(), + ), + }, + ) + .await + .expect("execution binding update should succeed before the first turn"); + + let error = manager + .start_dialog_turn_with_prepended_messages_if_session_matches( + &session.session_id, + "agentic".to_string(), + "must reject stale workspace binding".to_string(), + Some("turn-admission-workspace-race".to_string()), + None, + Vec::new(), + None, + &expected, + ) + .await + .expect_err("a concurrent execution binding update must invalidate admission"); + + assert!(error.to_string().contains("changed during turn admission")); + assert_eq!(manager.get_turn_count(&session.session_id), 0); + } + #[tokio::test] async fn recovery_persistence_failure_keeps_memory_and_disk_interrupted() { let workspace = TestWorkspace::new(); diff --git a/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs b/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs index 7245c3ffb8..7e8e0a21b9 100644 --- a/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs +++ b/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs @@ -2674,6 +2674,7 @@ mod tests { } } + #[cfg(feature = "opencode-plugin-host")] #[test] fn plugin_after_presentation_prefers_the_plugin_result_contract() { let result = ModelToolResult { diff --git a/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.test.tsx b/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.test.tsx index 32bd9aec88..0b206e2f0e 100644 --- a/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.test.tsx +++ b/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.test.tsx @@ -68,8 +68,19 @@ vi.mock('@bitfun/ui', () => ({ IconButton: ({ children, ...props }: React.ButtonHTMLAttributes) => ( ), - Field: ({ label, children }: React.PropsWithChildren<{ label: string }>) => ( - + Field: ({ + label, + children, + }: React.PropsWithChildren<{ label?: string }>) => ( + ), Input: ({ leading, diff --git a/src/web-ui/src/flow_chat/components/ChatInput.tsx b/src/web-ui/src/flow_chat/components/ChatInput.tsx index 83a1a42f17..089bdbfff0 100644 --- a/src/web-ui/src/flow_chat/components/ChatInput.tsx +++ b/src/web-ui/src/flow_chat/components/ChatInput.tsx @@ -649,14 +649,16 @@ export const ChatInput: React.FC = ({ inputState.value.trim() ); const currentReviewActivity = useSessionReviewActivity(currentSessionId); - // A blocked turn is answered from the composer, so the request the runtime is - // waiting on is composer state like any other part of the next turn. + // The primary composer owns only the active primary session's requests. + // Direct child-session requests are answered in BtwSessionPanel, even while + // this composer is targeting that child, so the same request never has two + // actionable surfaces. Delegated requests remain owned by the parent. const { - activeBatch: activePermissionBatch, - requests: pendingPermissionRequests, + ownedActiveBatch: activePermissionBatch, + ownedRequests: pendingPermissionRequests, respond: respondPermission, respondBatch: respondPermissionBatch, - } = usePermissionRequests(effectiveTargetSessionId || undefined); + } = usePermissionRequests(currentSessionId || undefined); const sessionMachine = useSessionStateMachine(effectiveTargetSessionId); const activePermissionTurnId = sessionMachine?.currentState === SessionExecutionState.PROCESSING diff --git a/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.scss b/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.scss index d7f174c33f..69dd5ba4c5 100644 --- a/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.scss +++ b/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.scss @@ -1,6 +1,6 @@ -// The approval band sits in the composer stack, directly above the capsule. -// It borrows the capsule's width and radius so it reads as the composer having -// grown a row, not as a dialog that happens to be nearby. +// The approval band normally sits in the composer stack, directly above the +// capsule. Embedded child-session panels reuse the same compact surface when +// no child composer exists. .bitfun-chat-input-approval { display: flex; flex-direction: column; diff --git a/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.tsx b/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.tsx index a266a683fa..0321c6a504 100644 --- a/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.tsx +++ b/src/web-ui/src/flow_chat/components/ChatInputApprovalBand.tsx @@ -1,12 +1,14 @@ /** - * The runtime asking to proceed, answered from inside the composer. + * The compact surface for answering a runtime permission request. * * This used to be a card floating over the transcript, positioned by measuring * the composer's height. It covered the very output the reader needed in order * to decide, and it carried its own textarea for the rejection reason while a * perfectly good one sat directly underneath it. So the band lives in the * composer stack instead: the request reads directly above the text field that - * answers it, and the reason is whatever the reader has typed there. + * answers it, and the reason is whatever the reader has typed there. Embedded + * child-session panels also reuse the band because they have no composer of + * their own; those surfaces intentionally omit the optional typed reason. */ import React, { useState } from 'react'; diff --git a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStripLayout.test.ts b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStripLayout.test.ts index f9267a3261..be92dfa341 100644 --- a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStripLayout.test.ts +++ b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStripLayout.test.ts @@ -476,6 +476,20 @@ describe('composer context track layout', () => { expect(band).not.toContain('position: fixed'); }); + it('keeps direct child approvals in the child panel while delegated requests stay with the parent', () => { + const chatInput = readLocalFile('ChatInput.tsx'); + const childPanel = readLocalFile('btw/BtwSessionPanel.tsx'); + + expect(chatInput).toContain('ownedActiveBatch: activePermissionBatch'); + expect(chatInput).toContain('ownedRequests: pendingPermissionRequests'); + expect(chatInput).toContain('usePermissionRequests(currentSessionId || undefined)'); + expect(chatInput).not.toContain( + 'usePermissionRequests(effectiveTargetSessionId || undefined)', + ); + expect(childPanel).toContain('ownedActiveBatch: activePermissionBatch'); + expect(childPanel).toContain(' { const band = readLocalFile('ChatInputApprovalBand.tsx'); diff --git a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx index 7f37afdb4f..7edbc06f19 100644 --- a/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx +++ b/src/web-ui/src/flow_chat/components/btw/BtwSessionPanel.review-action.test.tsx @@ -7,11 +7,21 @@ import { BtwSessionPanel } from './BtwSessionPanel'; import { useReviewActionBarStore } from '../../store/deepReviewActionBarStore'; import { loadPersistedReviewState } from '../../services/ReviewActionBarPersistenceService'; import type { FlowChatState, Session } from '../../types/flow-chat'; +import type { PermissionRequest } from '@/infrastructure/api/service-api/AgentAPI'; const panelMocks = vi.hoisted(() => ({ cancelSession: vi.fn(), hydrateSessionHistoryForDetail: vi.fn(), notificationError: vi.fn(), + permissionRequests: [] as PermissionRequest[], + ownedPermissionRequests: [] as PermissionRequest[], + ownedActivePermissionBatch: undefined as { + sessionId: string; + roundId: string; + requests: PermissionRequest[]; + } | undefined, + respondPermission: vi.fn(() => Promise.resolve()), + respondPermissionBatch: vi.fn(() => Promise.resolve()), virtualItems: [] as unknown[], })); @@ -54,6 +64,37 @@ vi.mock('../modern/useExploreGroupState', () => ({ }), })); +vi.mock('../modern/usePermissionRequests', () => ({ + usePermissionRequests: () => ({ + requests: panelMocks.permissionRequests, + activeBatch: undefined, + ownedRequests: panelMocks.ownedPermissionRequests, + ownedActiveBatch: panelMocks.ownedActivePermissionBatch, + respond: panelMocks.respondPermission, + respondBatch: panelMocks.respondPermissionBatch, + }), +})); + +vi.mock('../ChatInputApprovalBand', () => ({ + ChatInputApprovalBand: ({ + requests, + totalPendingCount, + onRespond, + }: { + requests: PermissionRequest[]; + totalPendingCount: number; + onRespond: (requestId: string, reply: 'once') => Promise; + }) => ( +