diff --git a/.gitignore b/.gitignore index ec9c92ea9..d3c5511c4 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,6 @@ desired_behavior.md # Don't include the python cache for bundled skills. __pycache__/ + +# Project notes +.note/ diff --git a/app/src/ai/blocklist/context_model.rs b/app/src/ai/blocklist/context_model.rs index 671b1d466..f06b8189e 100644 --- a/app/src/ai/blocklist/context_model.rs +++ b/app/src/ai/blocklist/context_model.rs @@ -318,6 +318,34 @@ impl BlocklistAIContextModel { } } + /// Test-only constructor that skips every subscription and singleton lookup performed by + /// [`Self::new`], so unit tests can build a [`BlocklistAIContextModel`] without registering + /// `BlocklistAIHistoryModel`, `LLMPreferences`, `ModelEventDispatcher`, `Sessions`, or + /// `AppExecutionMode`. Callers still pass real [`TerminalModel`] and [`AgentViewController`] + /// handles to populate the struct fields, but neither needs to be functional for the + /// methods exercised by these tests. + #[cfg(test)] + pub(crate) fn new_for_test( + terminal_model: Arc>, + terminal_view_id: EntityId, + agent_view_controller: ModelHandle, + ) -> Self { + Self { + terminal_model, + directory_context: Default::default(), + pending_context_block_ids: HashSet::new(), + pending_context_selected_text: None, + pending_attachments: Default::default(), + pending_query_state: PendingQueryState::default(), + terminal_view_id, + agent_view_controller, + pending_inline_diff_hunk_attachments: Default::default(), + pending_document_id: None, + auto_attached_agent_view_user_block_ids: Vec::new(), + queue_next_prompt_enabled: false, + } + } + /// Resets the set of blocks to be included as context to an empty list. /// Also removes any selected text that was to be included as context. pub fn reset_context_to_default(&mut self, ctx: &mut ModelContext) { @@ -329,6 +357,12 @@ impl BlocklistAIContextModel { self.auto_attached_agent_view_user_block_ids.clear(); } + /// Returns `true` if the next AI query has any context that should force the input to be + /// locked in AI mode (skipping NLD): a pending image or file attachment, or a pending block. + pub fn has_locking_attachment(&self) -> bool { + !self.pending_context_block_ids.is_empty() || !self.pending_attachments.is_empty() + } + /// Returns the set `BlockId`s corresponding to blocks to be included as context with the next /// query. pub fn pending_context_block_ids(&self) -> &HashSet { @@ -991,3 +1025,7 @@ pub enum BlocklistAIContextEvent { impl Entity for BlocklistAIContextModel { type Event = BlocklistAIContextEvent; } + +#[cfg(test)] +#[path = "context_model_test.rs"] +mod tests; diff --git a/app/src/ai/blocklist/context_model_test.rs b/app/src/ai/blocklist/context_model_test.rs new file mode 100644 index 000000000..4b22ab0a1 --- /dev/null +++ b/app/src/ai/blocklist/context_model_test.rs @@ -0,0 +1,176 @@ +//! Unit tests for [`BlocklistAIContextModel::has_locking_attachment`]. +//! +//! These tests deliberately bypass the production [`BlocklistAIContextModel::new`] constructor +//! (which subscribes to several singletons) and instead use [`BlocklistAIContextModel::new_for_test`] +//! together with [`super::agent_view::AgentViewController::new`]. That keeps the fixture small +//! enough to focus on the lock logic without standing up `BlocklistAIHistoryModel`, +//! `LLMPreferences`, `CloudModel`, `UpdateManager`, or `AppExecutionMode`. + +use std::sync::Arc; + +use parking_lot::FairMutex; +use warpui::r#async::executor::Background; +use warpui::{App, EntityId, ModelHandle}; + +use super::{BlocklistAIContextModel, PendingAttachment, PendingFile}; +use crate::ai::agent::ImageContext; +use crate::ai::blocklist::agent_view::{AgentViewController, EphemeralMessageModel}; +use crate::terminal::color::{self, Colors}; +use crate::terminal::event_listener::ChannelEventListener; +use crate::terminal::model::test_utils::block_size; +use crate::terminal::model::{BlockId, TerminalModel}; + +impl BlocklistAIContextModel { + pub(crate) fn append_pending_attachments_for_test( + &mut self, + attachments: Vec, + ) { + self.pending_attachments.extend(attachments); + } + + pub(crate) fn insert_pending_block_id_for_test(&mut self, block_id: BlockId) { + self.pending_context_block_ids.insert(block_id); + } + + pub(crate) fn set_pending_selected_text_for_test(&mut self, text: Option) { + self.pending_context_selected_text = text; + } +} + +/// Builds a [`BlocklistAIContextModel`] with stub dependencies. None of the dependencies are +/// exercised by the methods under test; they only need to satisfy the struct's field types. +fn build_test_context_model(app: &mut App) -> ModelHandle { + let terminal_model = Arc::new(FairMutex::new(TerminalModel::new_for_test( + block_size(), + color::List::from(&Colors::default()), + ChannelEventListener::new_for_test(), + Arc::new(Background::default()), + false, /* should_show_bootstrap_block */ + None, /* restored_blocks */ + false, /* honor_ps1 */ + false, /* is_inverted */ + None, /* session_startup_path */ + ))); + let terminal_view_id = EntityId::new(); + + let ephemeral_message_model = app.add_model(|_| EphemeralMessageModel::new()); + let agent_view_controller = app.add_model(|_| { + AgentViewController::new( + terminal_model.clone(), + terminal_view_id, + ephemeral_message_model, + ) + }); + + app.add_model(|_| { + BlocklistAIContextModel::new_for_test( + terminal_model, + terminal_view_id, + agent_view_controller, + ) + }) +} + +fn make_image_attachment(file_name: &str) -> PendingAttachment { + PendingAttachment::Image(ImageContext { + data: String::new(), + mime_type: "image/png".to_owned(), + file_name: file_name.to_owned(), + is_figma: false, + }) +} + +fn make_file_attachment(file_name: &str) -> PendingAttachment { + PendingAttachment::File(PendingFile { + file_name: file_name.to_owned(), + file_path: file_name.into(), + mime_type: "text/plain".to_owned(), + }) +} + +#[test] +fn has_locking_attachment_is_false_for_default_state() { + App::test((), |mut app| async move { + let model = build_test_context_model(&mut app); + + model.read(&app, |m, _| { + assert!(!m.has_locking_attachment()); + }); + }); +} + +#[test] +fn has_locking_attachment_is_true_with_pending_block_id() { + App::test((), |mut app| async move { + let model = build_test_context_model(&mut app); + + model.update(&mut app, |m, _| { + m.insert_pending_block_id_for_test(BlockId::new()); + }); + + model.read(&app, |m, _| assert!(m.has_locking_attachment())); + }); +} + +#[test] +fn has_locking_attachment_is_false_with_only_pending_selected_text() { + // Selected text alone is *not* a locking attachment: the user could be selecting shell + // command text (e.g. to copy a previously-run command), and forcing the input into AI + // mode in that case would be wrong. Only images, files, or blocks should force the lock. + App::test((), |mut app| async move { + let model = build_test_context_model(&mut app); + + model.update(&mut app, |m, _| { + m.set_pending_selected_text_for_test(Some("hello".to_owned())); + }); + + model.read(&app, |m, _| assert!(!m.has_locking_attachment())); + }); +} + +#[test] +fn has_locking_attachment_is_true_with_pending_image_attachment() { + App::test((), |mut app| async move { + let model = build_test_context_model(&mut app); + + model.update(&mut app, |m, _| { + m.append_pending_attachments_for_test(vec![make_image_attachment("a.png")]); + }); + + model.read(&app, |m, _| assert!(m.has_locking_attachment())); + }); +} + +#[test] +fn has_locking_attachment_is_true_with_only_file_attachments() { + // File attachments are locking attachments — the user has explicitly attached a file as + // context, which is unambiguously a signal that the next query is intended for the agent. + App::test((), |mut app| async move { + let model = build_test_context_model(&mut app); + + model.update(&mut app, |m, _| { + m.append_pending_attachments_for_test(vec![ + make_file_attachment("notes.txt"), + make_file_attachment("readme.md"), + ]); + }); + + model.read(&app, |m, _| assert!(m.has_locking_attachment())); + }); +} + +#[test] +fn has_locking_attachment_is_true_with_mixed_image_and_file_attachments() { + App::test((), |mut app| async move { + let model = build_test_context_model(&mut app); + + model.update(&mut app, |m, _| { + m.append_pending_attachments_for_test(vec![ + make_file_attachment("notes.txt"), + make_image_attachment("a.png"), + ]); + }); + + model.read(&app, |m, _| assert!(m.has_locking_attachment())); + }); +} diff --git a/app/src/ai/blocklist/input_model.rs b/app/src/ai/blocklist/input_model.rs index 86182aa69..554e4e9b4 100644 --- a/app/src/ai/blocklist/input_model.rs +++ b/app/src/ai/blocklist/input_model.rs @@ -20,6 +20,7 @@ use warpui::{AppContext, Entity, EntityId, ModelContext, ModelHandle, SingletonE pub use input_classifier::InputType; use super::agent_view::{AgentViewController, AgentViewControllerEvent, AgentViewEntryOrigin}; +use super::context_model::BlocklistAIContextModel; use crate::terminal::cli_agent_sessions::{ CLIAgentInputState, CLIAgentSessionsModel, CLIAgentSessionsModelEvent, }; @@ -148,6 +149,11 @@ pub struct BlocklistAIInputModel { agent_view_controller: ModelHandle, + /// Handle to the per-pane context model. Used to read pending attachments / blocks when + /// deciding whether to force-lock the input to AI mode (see + /// [`BlocklistAIContextModel::has_locking_attachment`]). + ai_context_model: ModelHandle, + terminal_view_id: EntityId, autodetect_abort_handle: Option, @@ -158,6 +164,7 @@ impl BlocklistAIInputModel { pub fn new( model: Arc>, agent_view_controller: ModelHandle, + ai_context_model: ModelHandle, terminal_view_id: EntityId, ctx: &mut ModelContext, ) -> Self { @@ -267,6 +274,19 @@ impl BlocklistAIInputModel { }, ctx, ); + } else if me.has_locking_attachment(ctx) { + // Interaction patterns that should fully bypass NLD on + // entry: image / file attachment in progress / attached, or block + // already in pending context. Force-lock to AI regardless of the + // user's NLD setting so the classifier never gets a chance to drop + // the buffer back to shell. + me.set_input_config_internal( + InputConfig { + input_type: InputType::AI, + is_locked: true, + }, + ctx, + ); } else { let is_autodetection_enabled = AISettings::as_ref(ctx).is_ai_autodetection_enabled(ctx); @@ -320,6 +340,7 @@ impl BlocklistAIInputModel { is_locked: !is_autodetection_enabled, }, agent_view_controller, + ai_context_model, terminal_view_id, last_ai_autodetection_ts: None, last_explicit_input_type_set_at: None, @@ -329,6 +350,11 @@ impl BlocklistAIInputModel { } } + /// Convenience wrapper around `BlocklistAIContextModel::has_locking_attachment`. + fn has_locking_attachment(&self, app: &AppContext) -> bool { + self.ai_context_model.as_ref(app).has_locking_attachment() + } + /// Returns the InputType enum which specifies how we will handle the terminal input. pub fn input_type(&self) -> InputType { self.input_config.input_type @@ -488,6 +514,14 @@ impl BlocklistAIInputModel { return false; } + // Defense in depth: while there is a pending attachment (image / file) or block, + // the classifier must never have a chance to flip the input back to shell mode, even + // per-keystroke. The `EnteredAgentView` subscriber and `set_input_mode_agent` already + // lock at entry; this guard protects the window if any future caller forgets. + if self.has_locking_attachment(app) { + return false; + } + let ai_settings = AISettings::as_ref(app); if FeatureFlag::AgentView.is_enabled() { if self.agent_view_controller.as_ref(app).is_fullscreen() { diff --git a/app/src/terminal/input.rs b/app/src/terminal/input.rs index 4e821aa47..fc73df39e 100644 --- a/app/src/terminal/input.rs +++ b/app/src/terminal/input.rs @@ -13015,10 +13015,16 @@ impl Input { // When AgentView is enabled, reverting to AI mode in an active agent view with an empty // buffer should unlock (re-enable autodetection) - semantically like clearing the "!". + // + // If there is a pending image / file attachment or block, do NOT unlock. The user's + // intent is unambiguously "talk to the agent"; letting the classifier flip the input + // back to shell mode would be a bug. + let has_locking_attachment = self.ai_context_model.as_ref(ctx).has_locking_attachment(); let should_unlock = FeatureFlag::AgentView.is_enabled() && self.agent_view_controller.as_ref(ctx).is_fullscreen() && is_input_buffer_empty - && AISettings::as_ref(ctx).is_ai_autodetection_enabled(ctx); + && AISettings::as_ref(ctx).is_ai_autodetection_enabled(ctx) + && !has_locking_attachment; if should_unlock { self.ai_input_model.update(ctx, |ai_input_model, ctx| { diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index 187d83793..8feb96aec 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -3293,10 +3293,21 @@ impl TerminalView { ctx.notify(); }); + let ai_context_model = ctx.add_model(|ctx| { + BlocklistAIContextModel::new( + sessions.clone(), + &model_events_handle, + model.clone(), + terminal_view_id, + agent_view_controller.clone(), + ctx, + ) + }); let ai_input_model = ctx.add_model(|ctx| { let mut model = BlocklistAIInputModel::new( model.clone(), agent_view_controller.clone(), + ai_context_model.clone(), terminal_view_id, ctx, ); @@ -3322,16 +3333,6 @@ impl TerminalView { ctx, ) }); - let ai_context_model = ctx.add_model(|ctx| { - BlocklistAIContextModel::new( - sessions.clone(), - &model_events_handle, - model.clone(), - terminal_view_id, - agent_view_controller.clone(), - ctx, - ) - }); let ai_controller = ctx.add_model(|ctx| { BlocklistAIController::new( ai_input_model.clone(),