QUALITY-544: Lock NLD for block/image/file-attached agent-view entry#9366
QUALITY-544: Lock NLD for block/image/file-attached agent-view entry#9366evelyn-with-warp wants to merge 13 commits intomasterfrom
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I'm re-reviewing this pull request in response to a review request. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR locks agent-view input to AI mode when image attachment is in progress or pending image/block/selection context exists, and wires the input model to the context model so NLD can be bypassed for those cases.
Concerns
- The new image-processing in-progress counter can leak when the processing future is aborted during submit, leaving
has_locking_attachment()true until a broader context reset.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // `note_image_attachment_completed` in the spawn callback below. | ||
| if let Some(context_model) = &self.context_model { | ||
| context_model.update(ctx, |context_model, ctx| { | ||
| context_model.note_image_attachment_started(ctx); |
There was a problem hiding this comment.
abort_attached_images_future_handle on submit, note_image_attachment_completed never runs and pending_image_attachments_in_progress stays nonzero; pair the abort path with a decrement or ensure completion always runs.
There was a problem hiding this comment.
Overview
This PR adds a pending image-attachment counter and uses it, plus existing pending context, to force-lock agent input to AI mode while images are being attached and to bypass NLD for image-driven agent-view entry.
Concerns
- The new counter is not decremented when image processing is aborted, which can leave
has_locking_attachment()true after a submit/abort race and keep input force-locked until a broader context reset. - Security pass: no security-specific findings in the inlined diff.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
1 similar comment
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR adds a pending image-attachment counter to the blocklist AI context model, wires the input model to consult that locking context, and reorders image attach flows so AI mode is locked before NLD can run. It also adds unit coverage for the new context-model locking predicates and counter behavior.
Concerns
- No blocking correctness or security concerns found in the inlined diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
@cla-bot check |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
The cla-bot has been summoned, and re-checked this pull request! |
When the user pastes or drags-and-drops an image into a terminal that has NLD on with autodetection enabled, entering agent view should lock the input to AI mode for the duration of the attach. Previously, two leak sites would unlock the input and let the classifier flip the buffer back to shell mode after the entry: 1. The `EnteredAgentView` subscriber in `BlocklistAIInputModel` would set `is_locked: !is_ai_autodetection_enabled` even for `ImageAdded` origins. 2. `set_input_mode_agent`'s `should_unlock` branch (which is intended for the `!`-shell-toggle clear path in fullscreen agent view with empty buffer) would re-unlock immediately after the image-add path entered agent view. This change introduces a single `has_locking_attachment()` predicate on `BlocklistAIContextModel` and uses it to gate both leak sites. It also adds an image-attachment-in-progress counter so the predicate is true during the async file-read + resize/encode pipeline, before the final `ImageContext` is appended to `pending_attachments`. Changes: - Add `pending_image_attachments_in_progress: usize` and `has_locking_attachment()` / `note_image_attachment_started()` / `note_image_attachment_completed()` to `BlocklistAIContextModel`. - Wire a handle to `BlocklistAIContextModel` into `BlocklistAIInputModel` via a `set_ai_context_model` setter, called from `TerminalView::new` after both models exist. - Add an `origin_requires_locked_ai` helper and force-lock to AI in the `EnteredAgentView` subscriber when the origin is `ImageAdded` or when `has_locking_attachment()` is true. - Defense in depth: gate `BlocklistAIInputModel::is_autodetection_enabled_for_current_context` on `!has_locking_attachment()`. - Increment / decrement the in-progress counter synchronously in `read_and_process_images_async` and `process_and_attach_images_as_ai_context` so callers see `has_locking_attachment() == true` for the entire attach pipeline. - Reorder `handle_pasted_or_dragdropped_image_filepaths` and `process_and_attach_clipboard_image` so the editor's image dispatch runs before `set_input_mode_agent`, ensuring the counter is already incremented when the lock decision is made. - Gate `set_input_mode_agent`'s `should_unlock` on `!has_locking_attachment()`. Co-Authored-By: Oz <oz-agent@warp.dev>
Cover `has_locking_attachment` (default false; true on pending block ids,
selected text, or pending image; ignores file-only attachments; true while
the in-progress image-attach counter is non-zero) and the
`note_image_attachment_started` / `note_image_attachment_completed` counter
mechanics including saturating decrement at zero and multiple concurrent
in-flight pipelines.
To keep the test fixture small, add `#[cfg(test)]` `new_for_test`
constructors on `BlocklistAIContextModel` and `AmbientAgentViewModel` that
skip subscriptions/singleton lookups (`BlocklistAIHistoryModel`,
`LLMPreferences`, `CloudModel`, `UpdateManager`, `AppExecutionMode`). This
follows the existing `new_for_test` convention used by `Sessions`,
`TerminalModel`, `AuthManager`, `BlocklistAIHistoryModel`, etc.
Tests live in `app/src/ai/blocklist/context_model_test.rs` per the repo's
`${filename}_tests.rs` / `mod_test.rs` convention.
Co-Authored-By: Oz <oz-agent@warp.dev>
Switch from ctx.spawn to ctx.spawn_abortable in process_and_attach_images_as_ai_context so pending_image_attachments_in_progress is decremented on every exit path: - on_resolve (normal completion): unchanged - on_resolve early-return when handle was taken between value production and dispatch: now decrements before returning instead of leaking - on_abort (future aborted before producing a value, e.g. submit_ai_query calling abort_attached_images_future_handle while processing was in flight): new explicit handler that decrements Previously ctx.spawn's default no-op on_abort callback meant that aborting the spawned future left pending_image_attachments_in_progress nonzero, keeping has_locking_attachment() true and force-locking the input into AI mode until an unrelated context reset. Co-Authored-By: Oz <oz-agent@warp.dev>
evelyn-with-warp
force-pushed
the
evelyn/quality-544
branch
from
00cfdeb to
d20d68e
Compare
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
| pub fn has_locking_attachment(&self) -> bool { | ||
| self.pending_image_attachments_in_progress > 0 | ||
| || !self.pending_context_block_ids.is_empty() | ||
| || self.pending_context_selected_text.is_some() |
There was a problem hiding this comment.
L378-379 are independent of attaching images. This is a future-proof change; after merging this, we will work on lock in NLD for attach block context
evelyn-with-warp
changed the title
Selected text is no longer treated as a locking attachment because users can select shell command text (e.g. to copy a previously-run command), and forcing the input into AI mode in that case would be wrong. File attachments are now treated as locking attachments — when a user explicitly attaches a file as context, that's an unambiguous signal that the next query is intended for the agent. Updated unit tests and surrounding doc comments accordingly. Co-Authored-By: Oz <oz-agent@warp.dev>
| codex_notifications = [] | ||
| cloud_mode_setup_v2 = ["cloud_mode"] | ||
| cloud_mode_input_v2 = ["cloud_mode"] | ||
| lock_interactions_to_nl_mode = [] |
There was a problem hiding this comment.
We can get rid of feature flag, like we spoke about offline
| /// decremented when it completes (or fails). It contributes to `has_locking_attachment` so the | ||
| /// input can be force-locked to AI mode for the entire duration of the attach, not only after | ||
| /// the actual `ImageContext` is appended. | ||
| pending_image_attachments_in_progress: usize, |
There was a problem hiding this comment.
Let's try to make use of pending_attachments -- by the time the user types, the attachment should be durable
There was a problem hiding this comment.
if user types after this, then has_locking_attachment will take effect; Tested on a 3 MB image locally and it was fine; so remove the async image reading tracking for limiting complexity
| if !FeatureFlag::LockInteractionsToNLMode.is_enabled() { | ||
| return false; | ||
| } | ||
| matches!(origin, AgentViewEntryOrigin::ImageAdded) |
There was a problem hiding this comment.
Should this also match on an equivalent event for non-image files?
There was a problem hiding this comment.
I removed origin_requires_locked_ai in this PR, and will added back for locking RefineDiff
The design choice is both image/file attachments should be handled in has_locking_attachment
| }, | ||
| ctx, | ||
| ); | ||
| } else if origin_requires_locked_ai(origin) || me.has_locking_attachment(ctx) { |
There was a problem hiding this comment.
now, no; removed origin_requires_locked_ai;
we will need both when supporting RefineDiff
evelyn-with-warp
force-pushed
the
evelyn/quality-544
branch
from
ad0cb60 to
ed724bb
Compare
… on fileattachment, will add it back for RefineDiff
| #[cfg(test)] | ||
| impl BlocklistAIContextModel { |
There was a problem hiding this comment.
nit: let's move this to the test mod (i.e. context_model_test.rs)
|
|
||
| let num_paths = paths_to_process.len(); | ||
|
|
||
| // Dispatch the async read + attach pipeline. The resulting `ImageContext` will be |
There was a problem hiding this comment.
Overview
This PR adds a context-model predicate for locking AI input when pending blocks or attachments exist, wires that predicate into agent-view entry and autodetection checks, and adds unit coverage for the new predicate.
Concerns
- Image attachment paths still have an async gap: the lock decision now runs after spawning image processing, but pending attachments are not appended until the spawned processing completes, so the new predicate is false during the decision. This affects both pasted/drag-dropped image file paths and direct clipboard image data.
- Security pass: no security-specific concerns found.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR adds a has_locking_attachment predicate and uses it to suppress NLD / force AI mode when pending blocks, files, or images are attached.
Concerns
- The new guard prevents future classifier runs, but it does not actively lock an input that is already in unlocked AI/autodetect mode when attachments are appended later. Async image processing and non-image file attachment can add pending context after Agent View entry or
set_input_mode_agenthas already evaluated the predicate.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // 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) { |
There was a problem hiding this comment.
ProcessNonImageFiles add pending attachments after Agent View entry / set_input_mode_agent can run, so the input can remain is_locked: false with a locking attachment. Actively set InputConfig { input_type: InputType::AI, is_locked: true } when pending context becomes locking.
evelyn-with-warp
force-pushed
the
evelyn/quality-544
branch
from
85ab4f0 to
2c77ae0
Compare
There was a problem hiding this comment.
Overview
This PR adds pending context checks so block, file, and image attachments can bypass NLD and keep Agent mode selected. The general direction is sound, but the image attachment paths still run the new lock logic before images are added to pending context.
Concerns
- Dragged/pasted image file paths call
set_input_mode_agentimmediately after starting async image processing, beforepending_attachmentscontains the image, so NLD can remain unlocked. - Clipboard image data has the same ordering problem because image resize/append happens in a spawned completion callback.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR wires pending context into input-mode locking so agent-view entry and autodetection bypass NLD when blocks or attachments are present. It also adds focused unit coverage for the new context-model predicate.
Concerns
has_locking_attachmentonly observes attachments after they are appended topending_attachments, but image attachments are appended after async file read/resize work completes. That leaves the entry/set-input paths able to seefalseand leave the input unlocked while an image is in progress.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR adds BlocklistAIContextModel::has_locking_attachment, wires the AI input model to the context model, and adds tests for locking on pending files, images, and blocks.
Concerns
- Image attachments that are still processing are not represented in
pending_attachmentsyet, but the new input-mode updates are called immediately after spawning async image processing. Pasted/drag-dropped and clipboard images can therefore leave empty fullscreen agent view unlocked and allow NLD before the attachment is appended. - Security pass: no security-specific findings.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let is_buffer_empty = self.buffer_text(ctx).is_empty(); | ||
| let in_active_agent_view = self.agent_view_controller.as_ref(ctx).is_active(); | ||
| if is_buffer_empty || in_active_agent_view { | ||
| self.set_input_mode_agent(true, ctx); |
There was a problem hiding this comment.
read_and_process_images_async only spawns the image read; at this point pending_attachments is still empty, so set_input_mode_agent can still unlock empty fullscreen agent view and NLD can run while the image is in progress. Lock before spawning, track the in-progress state in has_locking_attachment, or lock from the async completion before user input can be classified.
| }); | ||
|
|
||
| // Switch to AI mode with block-level lock, unless already AI-mode-locked | ||
| if !self.is_locked_in_ai_mode(ctx) { |
There was a problem hiding this comment.
process_and_attach_images_as_ai_context appends pending images from its spawned completion callback, so this lock check also runs before has_locking_attachment can see the clipboard image; keep clipboard image addition locked while processing.
evelyn-with-warp
changed the title
There was a problem hiding this comment.
Overview
This PR adds a pending-context check so Agent View entry and AI-mode toggles can force AI mode when blocks, images, or files are attached.
Concerns
- The new pending-attachment check misses the async image attachment window, so image paste/drag-drop can still enter Agent View unlocked before the image is appended to pending context.
- The new autodetection guard does not stop existing unlock paths from calling
enable_autodetection, so pending attachments/blocks can still lose the lock even though classification is later suppressed.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| /// 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() |
There was a problem hiding this comment.
set_input_mode_agent before append_pending_images runs asynchronously, so pending_attachments is empty during both lock decisions and the input can remain unlocked. Include an in-progress image signal here or force-lock AgentViewEntryOrigin::ImageAdded.
| // 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) { |
There was a problem hiding this comment.
set_input_mode_natural_language_detection call enable_autodetection when should_run_input_autodetection returns false, so Esc/NLD toggles with a pending block or file can still clear the lock. Gate those unlock paths on has_locking_attachment or separate the "is already autodetecting" check from the "classification is allowed" check.
2 participants
Description
QUALITY-544: Entering agent view with
imagefile attachedThe main change is adding fn
has_locking_attachment, which includesimagesfiles, blocks and we will lock Agent mode when locking attachment is detectedTesting
Cargo commands with presubmit
Loom videos
ls: https://www.loom.com/share/06774662033d45a99369ceb781759e71Agent Mode
Changelog Entries for Stable
CHANGELOG-BUG-FIX: if user attaches an image in block input we should lock in agent mode, without running the NLD classifier to remove uncertainty
Reviewer
@szgupta @evelyn-with-warp (For testing Github Slack app)