From e255bdacf7c324734f9fac7d4603ef3822dac2c1 Mon Sep 17 00:00:00 2001 From: guantw Date: Thu, 27 Aug 2026 22:38:48 +0800 Subject: [PATCH 1/2] fix(session): keep turn admission bindings consistent DeepReview remediation changes a manager-owned session binding before the turn is admitted. The admission snapshot must track the effective route owner and all execution-affecting session bindings so stale configuration cannot start a turn. Synchronize the turn-local snapshot after the ReviewFixer binding update and reject concurrent model, route, context-window, and workspace binding changes. Add regression coverage for the successful remediation path and admission races. --- .../src/agentic/coordination/coordinator.rs | 74 ++++++- .../src/agentic/session/session_manager.rs | 186 +++++++++++++++++- .../btw/DeepReviewActionBar.test.tsx | 1 + 3 files changed, 253 insertions(+), 8 deletions(-) diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index eb04906da3..6e0fd84ea3 100644 --- a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs +++ b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs @@ -5786,7 +5786,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( @@ -5884,6 +5884,10 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet primary_agent_binding.route_owner, ) .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; } debug!( @@ -14772,9 +14776,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::{ @@ -14795,7 +14799,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::{ @@ -17135,6 +17139,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 99dbdf535a..9e0d679e6e 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,16 @@ pub(crate) struct TurnAdmissionSessionFacts { model_id: Option, reasoning_preset: Option, permission_mode: Option, + max_context_tokens: usize, agent_type: String, + agent_route_owner: SessionAgentRouteOwner, 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 +146,16 @@ 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, 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 +168,16 @@ 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.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 } } @@ -7223,9 +7247,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, @@ -10667,6 +10692,161 @@ 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, + ) + .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_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/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx b/src/web-ui/src/flow_chat/components/btw/DeepReviewActionBar.test.tsx index 31a9ecedad..00783aafd4 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 @@ -121,6 +121,7 @@ vi.mock('@/infrastructure/api/service-api/AgentAPI', () => ({ vi.mock('@/infrastructure/runtime', () => ({ isTauriRuntime: () => true, + isOpenHarmonyRuntime: () => false, })); vi.mock('@/infrastructure/event-bus', () => ({ From 0c980282450081296470fcb205d13f5bd434917d Mon Sep 17 00:00:00 2001 From: guantw Date: Fri, 28 Aug 2026 20:35:30 +0800 Subject: [PATCH 2/2] fix(flow-chat): route permission requests to a single owner Adapt #2595 to the Explore composer architecture. The primary composer handles only requests owned by the primary session, while embedded BTW and review panels expose direct child-session requests through the shared approval band. Delegated requests remain actionable only from the parent surface. Repair stale design-system test mocks and normalize a cross-platform stylesheet fixture inherited from the latest Explore base so the full frontend suite passes on Linux and Windows checkouts. --- .../ssh-remote/SSHConnectionDialog.test.tsx | 14 +++ .../src/flow_chat/components/ChatInput.tsx | 12 +- .../components/ChatInputApprovalBand.scss | 6 +- .../components/ChatInputApprovalBand.tsx | 6 +- .../ChatInputWorkspaceStripLayout.test.ts | 14 +++ .../BtwSessionPanel.review-action.test.tsx | 117 ++++++++++++++++++ .../components/btw/BtwSessionPanel.scss | 11 ++ .../components/btw/BtwSessionPanel.tsx | 33 ++++- .../modern/permissionRequestRouting.test.ts | 39 ++++++ .../modern/permissionRequestRouting.ts | 56 ++++++++- .../modern/usePermissionRequests.test.tsx | 22 ++++ .../modern/usePermissionRequests.ts | 22 +++- .../AppearanceMarketDialog.test.tsx | 1 + .../common/ConfigPageLayout.test.tsx | 22 ++-- 14 files changed, 347 insertions(+), 28 deletions(-) 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 80fa8ed87d..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,6 +68,20 @@ vi.mock('@bitfun/ui', () => ({ IconButton: ({ children, ...props }: React.ButtonHTMLAttributes) => ( ), + Field: ({ + label, + children, + }: React.PropsWithChildren<{ label?: string }>) => ( + + ), Input: ({ leading, trailing, diff --git a/src/web-ui/src/flow_chat/components/ChatInput.tsx b/src/web-ui/src/flow_chat/components/ChatInput.tsx index 53a6af875f..334da3c29b 100644 --- a/src/web-ui/src/flow_chat/components/ChatInput.tsx +++ b/src/web-ui/src/flow_chat/components/ChatInput.tsx @@ -661,14 +661,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 23af37c950..c7cc680ed3 100644 --- a/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStripLayout.test.ts +++ b/src/web-ui/src/flow_chat/components/ChatInputWorkspaceStripLayout.test.ts @@ -378,6 +378,20 @@ describe('status 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; + }) => ( +