Fix streaming file edit lifecycle
This commit is contained in:
@@ -136,6 +136,7 @@ Key invariants:
|
|||||||
- Direct-provider remote telemetry records requested, started, retry-scheduled, and finished model-turn phases with explicit `llm_finished` state; root `provider_run_finished` records distinguish clean completion from failure or cancellation and mark the response stream terminal
|
- Direct-provider remote telemetry records requested, started, retry-scheduled, and finished model-turn phases with explicit `llm_finished` state; root `provider_run_finished` records distinguish clean completion from failure or cancellation and mark the response stream terminal
|
||||||
- `use_rig` and provider selection may choose request/transport details but must never choose lifecycle ownership
|
- `use_rig` and provider selection may choose request/transport details but must never choose lifecycle ownership
|
||||||
- Direct-provider output may be projected through `ResponseStream`, but provider progress must not depend on response-stream result draining or `AfterStreamFinished`
|
- Direct-provider output may be projected through `ResponseStream`, but provider progress must not depend on response-stream result draining or `AfterStreamFinished`
|
||||||
|
- Direct-provider `RequestFileEdits` views must register from streaming output before provider-run completion; preprocessing results must survive delayed view registration, and `NotReady` retries must remain automatic rather than emitting a synthetic user permission decision
|
||||||
- A clean direct-provider `ProviderRunOutcome::Completed` explicitly finalizes the conversation as `Success` after terminal output projection, even if earlier turns added tool actions; child-completion waits rely on that status
|
- A clean direct-provider `ProviderRunOutcome::Completed` explicitly finalizes the conversation as `Success` after terminal output projection, even if earlier turns added tool actions; child-completion waits rely on that status
|
||||||
- Provider actions and results must correlate by `(conversation_id, run_id, epoch, call_id)`; stale or duplicate callbacks must not advance a run
|
- Provider actions and results must correlate by `(conversation_id, run_id, epoch, call_id)`; stale or duplicate callbacks must not advance a run
|
||||||
- Active provider runs must checkpoint before external work, persist without credentials, normalize unsafe restored states, and reconcile command state before continuing
|
- Active provider runs must checkpoint before external work, persist without credentials, normalize unsafe restored states, and reconcile command state before continuing
|
||||||
|
|||||||
@@ -190,6 +190,54 @@ type ProviderActionCorrelation = (
|
|||||||
ProviderToolExecutionRef,
|
ProviderToolExecutionRef,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
type ActionExecutionKey = (AIConversationId, AIAgentActionId);
|
||||||
|
|
||||||
|
#[derive(Default)]
|
||||||
|
struct NotReadyActionTracker {
|
||||||
|
actions: HashSet<ActionExecutionKey>,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl NotReadyActionTracker {
|
||||||
|
fn should_retry(&self, conversation_id: AIConversationId, action_id: &AIAgentActionId) -> bool {
|
||||||
|
self.actions.contains(&(conversation_id, action_id.clone()))
|
||||||
|
}
|
||||||
|
|
||||||
|
fn update_after_attempt(
|
||||||
|
&mut self,
|
||||||
|
conversation_id: AIConversationId,
|
||||||
|
action_id: AIAgentActionId,
|
||||||
|
reason: NotExecutedReason,
|
||||||
|
initiator: ActionExecutionInitiator,
|
||||||
|
) {
|
||||||
|
let key = (conversation_id, action_id);
|
||||||
|
match (reason, initiator) {
|
||||||
|
(NotExecutedReason::NotReady, ActionExecutionInitiator::Automatic) => {
|
||||||
|
self.actions.insert(key);
|
||||||
|
}
|
||||||
|
(NotExecutedReason::NotReady, ActionExecutionInitiator::User)
|
||||||
|
| (NotExecutedReason::NeedsConfirmation | NotExecutedReason::WaitingOnSharer, _) => {
|
||||||
|
self.actions.remove(&key);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn clear(&mut self, conversation_id: AIConversationId, action_id: &AIAgentActionId) {
|
||||||
|
self.actions.remove(&(conversation_id, action_id.clone()));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[derive(Clone, Copy)]
|
||||||
|
enum ActionExecutionInitiator {
|
||||||
|
Automatic,
|
||||||
|
User,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl ActionExecutionInitiator {
|
||||||
|
fn is_user_initiated(self) -> bool {
|
||||||
|
matches!(self, Self::User)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
impl RunningActions {
|
impl RunningActions {
|
||||||
fn new(phase: RunningActionPhase, action_id: AIAgentActionId) -> Self {
|
fn new(phase: RunningActionPhase, action_id: AIAgentActionId) -> Self {
|
||||||
Self {
|
Self {
|
||||||
@@ -678,6 +726,9 @@ pub struct BlocklistAIActionModel {
|
|||||||
/// Permission-card rejections whose cancelled action result must not emit a second provider event.
|
/// Permission-card rejections whose cancelled action result must not emit a second provider event.
|
||||||
denied_permissions: HashSet<(AIConversationId, AIAgentActionId)>,
|
denied_permissions: HashSet<(AIConversationId, AIAgentActionId)>,
|
||||||
|
|
||||||
|
/// Actions parked because their executor-specific UI or state was not ready yet.
|
||||||
|
not_ready_actions: NotReadyActionTracker,
|
||||||
|
|
||||||
/// Durable provider work identity for actions owned by an active provider run.
|
/// Durable provider work identity for actions owned by an active provider run.
|
||||||
provider_tool_executions:
|
provider_tool_executions:
|
||||||
HashMap<(AIConversationId, AIAgentActionId), ProviderToolExecutionRef>,
|
HashMap<(AIConversationId, AIAgentActionId), ProviderToolExecutionRef>,
|
||||||
@@ -771,6 +822,7 @@ impl BlocklistAIActionModel {
|
|||||||
running_actions: Default::default(),
|
running_actions: Default::default(),
|
||||||
action_order: Default::default(),
|
action_order: Default::default(),
|
||||||
denied_permissions: Default::default(),
|
denied_permissions: Default::default(),
|
||||||
|
not_ready_actions: Default::default(),
|
||||||
provider_tool_executions: Default::default(),
|
provider_tool_executions: Default::default(),
|
||||||
terminal_view_id,
|
terminal_view_id,
|
||||||
pending_preprocessed_actions: Default::default(),
|
pending_preprocessed_actions: Default::default(),
|
||||||
@@ -975,9 +1027,12 @@ impl BlocklistAIActionModel {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
let Some(result) =
|
let Some(result) = self.start_pending_action_by_id(
|
||||||
self.start_pending_action_by_id(&front_action.id, conversation_id, false, ctx)
|
&front_action.id,
|
||||||
else {
|
conversation_id,
|
||||||
|
ActionExecutionInitiator::Automatic,
|
||||||
|
ctx,
|
||||||
|
) else {
|
||||||
log::info!("[tool-debug] try_to_execute_available_actions: start_pending_action_by_id returned None (blocked)");
|
log::info!("[tool-debug] try_to_execute_available_actions: start_pending_action_by_id returned None (blocked)");
|
||||||
return;
|
return;
|
||||||
};
|
};
|
||||||
@@ -1290,7 +1345,12 @@ impl BlocklistAIActionModel {
|
|||||||
};
|
};
|
||||||
|
|
||||||
if self
|
if self
|
||||||
.start_pending_action_by_id(&pending_action_id, conversation_id, true, ctx)
|
.start_pending_action_by_id(
|
||||||
|
&pending_action_id,
|
||||||
|
conversation_id,
|
||||||
|
ActionExecutionInitiator::User,
|
||||||
|
ctx,
|
||||||
|
)
|
||||||
.is_some_and(|result| matches!(result, StartedAction::Sync))
|
.is_some_and(|result| matches!(result, StartedAction::Sync))
|
||||||
{
|
{
|
||||||
self.try_to_execute_available_actions(conversation_id, ctx);
|
self.try_to_execute_available_actions(conversation_id, ctx);
|
||||||
@@ -1305,7 +1365,39 @@ impl BlocklistAIActionModel {
|
|||||||
ctx: &mut ModelContext<Self>,
|
ctx: &mut ModelContext<Self>,
|
||||||
) {
|
) {
|
||||||
if self
|
if self
|
||||||
.start_pending_action_by_id(action_id, conversation_id, true, ctx)
|
.start_pending_action_by_id(
|
||||||
|
action_id,
|
||||||
|
conversation_id,
|
||||||
|
ActionExecutionInitiator::User,
|
||||||
|
ctx,
|
||||||
|
)
|
||||||
|
.is_some_and(|result| matches!(result, StartedAction::Sync))
|
||||||
|
{
|
||||||
|
self.try_to_execute_available_actions(conversation_id, ctx);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Retries an action only when a prior automatic attempt found its executor not ready.
|
||||||
|
pub(super) fn retry_not_ready_action(
|
||||||
|
&mut self,
|
||||||
|
action_id: &AIAgentActionId,
|
||||||
|
conversation_id: AIConversationId,
|
||||||
|
ctx: &mut ModelContext<Self>,
|
||||||
|
) {
|
||||||
|
if !self
|
||||||
|
.not_ready_actions
|
||||||
|
.should_retry(conversation_id, action_id)
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if self
|
||||||
|
.start_pending_action_by_id(
|
||||||
|
action_id,
|
||||||
|
conversation_id,
|
||||||
|
ActionExecutionInitiator::Automatic,
|
||||||
|
ctx,
|
||||||
|
)
|
||||||
.is_some_and(|result| matches!(result, StartedAction::Sync))
|
.is_some_and(|result| matches!(result, StartedAction::Sync))
|
||||||
{
|
{
|
||||||
self.try_to_execute_available_actions(conversation_id, ctx);
|
self.try_to_execute_available_actions(conversation_id, ctx);
|
||||||
@@ -1339,7 +1431,8 @@ impl BlocklistAIActionModel {
|
|||||||
conversation_id: AIConversationId,
|
conversation_id: AIConversationId,
|
||||||
ctx: &mut ModelContext<Self>,
|
ctx: &mut ModelContext<Self>,
|
||||||
) {
|
) {
|
||||||
if reason.needs_confirmation() {
|
match reason {
|
||||||
|
NotExecutedReason::NeedsConfirmation => {
|
||||||
#[cfg(not(target_family = "wasm"))]
|
#[cfg(not(target_family = "wasm"))]
|
||||||
log_tool_event(
|
log_tool_event(
|
||||||
ctx,
|
ctx,
|
||||||
@@ -1384,6 +1477,24 @@ impl BlocklistAIActionModel {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
NotExecutedReason::NotReady => {
|
||||||
|
#[cfg(not(target_family = "wasm"))]
|
||||||
|
log_tool_event(
|
||||||
|
ctx,
|
||||||
|
RemoteLogLevel::Warn,
|
||||||
|
"Tool execution deferred",
|
||||||
|
serde_json::json!({
|
||||||
|
"event": "tool_execution_not_ready",
|
||||||
|
"conversation_id": conversation_id.to_string(),
|
||||||
|
"action_id": action.id.to_string(),
|
||||||
|
"task_id": action.task_id.to_string(),
|
||||||
|
"tool_name": action_tool_name(action),
|
||||||
|
"permission_kind": format!("{:?}", permission_kind_for_action(&action.action)),
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
NotExecutedReason::WaitingOnSharer => {}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn action_phase_for_action(
|
fn action_phase_for_action(
|
||||||
@@ -1415,9 +1526,10 @@ impl BlocklistAIActionModel {
|
|||||||
&mut self,
|
&mut self,
|
||||||
action_id: &AIAgentActionId,
|
action_id: &AIAgentActionId,
|
||||||
conversation_id: AIConversationId,
|
conversation_id: AIConversationId,
|
||||||
is_user_initiated: bool,
|
initiator: ActionExecutionInitiator,
|
||||||
ctx: &mut ModelContext<Self>,
|
ctx: &mut ModelContext<Self>,
|
||||||
) -> Option<StartedAction> {
|
) -> Option<StartedAction> {
|
||||||
|
let is_user_initiated = initiator.is_user_initiated();
|
||||||
if is_user_initiated && self.running_actions.contains_key(&conversation_id) {
|
if is_user_initiated && self.running_actions.contains_key(&conversation_id) {
|
||||||
// User-driven approvals still execute one action at a time so that interactive
|
// User-driven approvals still execute one action at a time so that interactive
|
||||||
// confirmations do not overlap in the UI.
|
// confirmations do not overlap in the UI.
|
||||||
@@ -1476,6 +1588,7 @@ impl BlocklistAIActionModel {
|
|||||||
|
|
||||||
match execute_result {
|
match execute_result {
|
||||||
TryExecuteResult::ExecutedAsync => {
|
TryExecuteResult::ExecutedAsync => {
|
||||||
|
self.not_ready_actions.clear(conversation_id, &action_id);
|
||||||
#[cfg(not(target_family = "wasm"))]
|
#[cfg(not(target_family = "wasm"))]
|
||||||
log_tool_event(
|
log_tool_event(
|
||||||
ctx,
|
ctx,
|
||||||
@@ -1494,6 +1607,7 @@ impl BlocklistAIActionModel {
|
|||||||
Some(StartedAction::Async { phase })
|
Some(StartedAction::Async { phase })
|
||||||
}
|
}
|
||||||
TryExecuteResult::ExecutedSync => {
|
TryExecuteResult::ExecutedSync => {
|
||||||
|
self.not_ready_actions.clear(conversation_id, &action_id);
|
||||||
#[cfg(not(target_family = "wasm"))]
|
#[cfg(not(target_family = "wasm"))]
|
||||||
log_tool_event(
|
log_tool_event(
|
||||||
ctx,
|
ctx,
|
||||||
@@ -1511,6 +1625,12 @@ impl BlocklistAIActionModel {
|
|||||||
Some(StartedAction::Sync)
|
Some(StartedAction::Sync)
|
||||||
}
|
}
|
||||||
TryExecuteResult::NotExecuted { reason, action } => {
|
TryExecuteResult::NotExecuted { reason, action } => {
|
||||||
|
self.not_ready_actions.update_after_attempt(
|
||||||
|
conversation_id,
|
||||||
|
action_id,
|
||||||
|
reason,
|
||||||
|
initiator,
|
||||||
|
);
|
||||||
self.pending_actions
|
self.pending_actions
|
||||||
.entry(conversation_id)
|
.entry(conversation_id)
|
||||||
.or_default()
|
.or_default()
|
||||||
@@ -2047,6 +2167,8 @@ impl BlocklistAIActionModel {
|
|||||||
std::mem::discriminant(&action_result.result),
|
std::mem::discriminant(&action_result.result),
|
||||||
cancellation_reason
|
cancellation_reason
|
||||||
);
|
);
|
||||||
|
self.not_ready_actions
|
||||||
|
.clear(conversation_id, &action_result.id);
|
||||||
let should_remove_entry =
|
let should_remove_entry =
|
||||||
self.running_actions
|
self.running_actions
|
||||||
.get_mut(&conversation_id)
|
.get_mut(&conversation_id)
|
||||||
|
|||||||
@@ -231,12 +231,6 @@ pub enum NotExecutedReason {
|
|||||||
WaitingOnSharer,
|
WaitingOnSharer,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl NotExecutedReason {
|
|
||||||
pub fn needs_confirmation(&self) -> bool {
|
|
||||||
matches!(self, Self::NeedsConfirmation)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Result type for `BlocklistAIActionExecutor::try_to_execute_action`.
|
/// Result type for `BlocklistAIActionExecutor::try_to_execute_action`.
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub(super) enum TryExecuteResult {
|
pub(super) enum TryExecuteResult {
|
||||||
|
|||||||
@@ -42,10 +42,34 @@ use crate::terminal::model::session::SessionType;
|
|||||||
use crate::{safe_warn, BlocklistAIHistoryModel};
|
use crate::{safe_warn, BlocklistAIHistoryModel};
|
||||||
const APPLY_DIFF_RESULT_CONTEXT_LINES: usize = 10;
|
const APPLY_DIFF_RESULT_CONTEXT_LINES: usize = 10;
|
||||||
|
|
||||||
|
type AppliedDiffs = (Vec<FileDiff>, DiffSessionType);
|
||||||
|
|
||||||
|
#[derive(Default)]
|
||||||
|
struct PendingAppliedDiffs {
|
||||||
|
by_action: HashMap<AIAgentActionId, AppliedDiffs>,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl PendingAppliedDiffs {
|
||||||
|
fn buffer(
|
||||||
|
&mut self,
|
||||||
|
action_id: AIAgentActionId,
|
||||||
|
diffs: Vec<FileDiff>,
|
||||||
|
diff_session_type: DiffSessionType,
|
||||||
|
) {
|
||||||
|
self.by_action.insert(action_id, (diffs, diff_session_type));
|
||||||
|
}
|
||||||
|
|
||||||
|
fn take(&mut self, action_id: &AIAgentActionId) -> Option<AppliedDiffs> {
|
||||||
|
self.by_action.remove(action_id)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
pub struct RequestFileEditsExecutor {
|
pub struct RequestFileEditsExecutor {
|
||||||
active_session: ModelHandle<ActiveSession>,
|
active_session: ModelHandle<ActiveSession>,
|
||||||
apply_diff_model: ModelHandle<ApplyDiffModel>,
|
apply_diff_model: ModelHandle<ApplyDiffModel>,
|
||||||
diff_views: HashMap<AIAgentActionId, ViewHandle<CodeDiffView>>,
|
diff_views: HashMap<AIAgentActionId, ViewHandle<CodeDiffView>>,
|
||||||
|
/// Successfully applied diffs that completed before their view was registered.
|
||||||
|
pending_applied_diffs: PendingAppliedDiffs,
|
||||||
/// Set of action IDs where diff application failed.
|
/// Set of action IDs where diff application failed.
|
||||||
diff_application_failures: HashMap<AIAgentActionId, Vec1<DiffApplicationError>>,
|
diff_application_failures: HashMap<AIAgentActionId, Vec1<DiffApplicationError>>,
|
||||||
terminal_view_id: EntityId,
|
terminal_view_id: EntityId,
|
||||||
@@ -62,6 +86,7 @@ impl RequestFileEditsExecutor {
|
|||||||
active_session,
|
active_session,
|
||||||
apply_diff_model,
|
apply_diff_model,
|
||||||
diff_views: HashMap::new(),
|
diff_views: HashMap::new(),
|
||||||
|
pending_applied_diffs: PendingAppliedDiffs::default(),
|
||||||
diff_application_failures: HashMap::new(),
|
diff_application_failures: HashMap::new(),
|
||||||
terminal_view_id,
|
terminal_view_id,
|
||||||
}
|
}
|
||||||
@@ -117,15 +142,18 @@ impl RequestFileEditsExecutor {
|
|||||||
.is_allowed()
|
.is_allowed()
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Registers a diff view to handle a RequestFileEdits action.
|
/// Registers a diff view to handle a RequestFileEdits action and applies any diffs that
|
||||||
/// Note this MUST be called before `execute` or `preprocess_action` is invoked in
|
/// finished preprocessing before the UI observed the action.
|
||||||
/// order for the necessary state to be set to handle the action.
|
|
||||||
pub fn register_requested_edits(
|
pub fn register_requested_edits(
|
||||||
&mut self,
|
&mut self,
|
||||||
action_id: &AIAgentActionId,
|
action_id: &AIAgentActionId,
|
||||||
view: &ViewHandle<CodeDiffView>,
|
view: &ViewHandle<CodeDiffView>,
|
||||||
|
ctx: &mut ModelContext<Self>,
|
||||||
) {
|
) {
|
||||||
self.diff_views.insert(action_id.clone(), view.clone());
|
self.diff_views.insert(action_id.clone(), view.clone());
|
||||||
|
if let Some((diffs, diff_session_type)) = self.pending_applied_diffs.take(action_id) {
|
||||||
|
Self::apply_diffs_to_view(view, diffs, diff_session_type, ctx);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(super) fn execute(
|
pub(super) fn execute(
|
||||||
@@ -322,55 +350,25 @@ impl RequestFileEditsExecutor {
|
|||||||
tx: oneshot::Sender<()>,
|
tx: oneshot::Sender<()>,
|
||||||
ctx: &mut ModelContext<Self>,
|
ctx: &mut ModelContext<Self>,
|
||||||
) {
|
) {
|
||||||
tx.send(()).ok();
|
match applied_diffs {
|
||||||
|
Ok(applied_diffs) if !applied_diffs.is_empty() => {
|
||||||
let Some(diff_view) = self.diff_views.get(&id) else {
|
|
||||||
log::warn!(
|
|
||||||
"Tried to apply diffs for a RequestFileEdits action without a corresponding diff view"
|
|
||||||
);
|
|
||||||
return;
|
|
||||||
};
|
|
||||||
|
|
||||||
let applied_diffs = match applied_diffs {
|
|
||||||
Ok(diffs) if !diffs.is_empty() => diffs,
|
|
||||||
Ok(_) => {
|
|
||||||
// We didn't generate any diffs--consider this a failure.
|
|
||||||
log::warn!("No diffs generated");
|
|
||||||
self.diff_application_failures
|
|
||||||
.insert(id, vec1![DiffApplicationError::EmptyDiff]);
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
Err(err) => {
|
|
||||||
safe_warn!(
|
|
||||||
safe: ("Failed to generate diffs"),
|
|
||||||
full: ("Failed to generate diffs {err:?}")
|
|
||||||
);
|
|
||||||
self.diff_application_failures.insert(id, err);
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
let current_working_directory = self
|
let current_working_directory = self
|
||||||
.active_session
|
.active_session
|
||||||
.as_ref(ctx)
|
.as_ref(ctx)
|
||||||
.current_working_directory()
|
.current_working_directory()
|
||||||
.cloned();
|
.cloned();
|
||||||
|
|
||||||
let shell_launch_data = self.active_session.as_ref(ctx).shell_launch_data(ctx);
|
let shell_launch_data = self.active_session.as_ref(ctx).shell_launch_data(ctx);
|
||||||
|
let diffs = applied_diffs
|
||||||
let mut diffs = Vec::with_capacity(applied_diffs.len());
|
.into_iter()
|
||||||
for diff in applied_diffs {
|
.map(|diff| {
|
||||||
let path = host_native_absolute_path(
|
let path = host_native_absolute_path(
|
||||||
diff.file_name.as_str(),
|
diff.file_name.as_str(),
|
||||||
&shell_launch_data,
|
&shell_launch_data,
|
||||||
¤t_working_directory,
|
¤t_working_directory,
|
||||||
);
|
);
|
||||||
let file_diff = FileDiff::new(diff.original_content, path, diff.diff_type);
|
FileDiff::new(diff.original_content, path, diff.diff_type)
|
||||||
diffs.push(file_diff);
|
})
|
||||||
}
|
.collect();
|
||||||
|
|
||||||
// Set the session type on the diff view so save/delete/create routes
|
|
||||||
// through the correct FileModel backend.
|
|
||||||
let diff_session_type = match self.active_session.as_ref(ctx).session_type(ctx) {
|
let diff_session_type = match self.active_session.as_ref(ctx).session_type(ctx) {
|
||||||
Some(SessionType::WarpifiedRemote {
|
Some(SessionType::WarpifiedRemote {
|
||||||
host_id: Some(host_id),
|
host_id: Some(host_id),
|
||||||
@@ -378,6 +376,36 @@ impl RequestFileEditsExecutor {
|
|||||||
_ => DiffSessionType::Local,
|
_ => DiffSessionType::Local,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
if let Some(diff_view) = self.diff_views.get(&id).cloned() {
|
||||||
|
Self::apply_diffs_to_view(&diff_view, diffs, diff_session_type, ctx);
|
||||||
|
} else {
|
||||||
|
self.pending_applied_diffs
|
||||||
|
.buffer(id, diffs, diff_session_type);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Ok(_) => {
|
||||||
|
log::warn!("No diffs generated");
|
||||||
|
self.diff_application_failures
|
||||||
|
.insert(id, vec1![DiffApplicationError::EmptyDiff]);
|
||||||
|
}
|
||||||
|
Err(err) => {
|
||||||
|
safe_warn!(
|
||||||
|
safe: ("Failed to generate diffs"),
|
||||||
|
full: ("Failed to generate diffs {err:?}")
|
||||||
|
);
|
||||||
|
self.diff_application_failures.insert(id, err);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
tx.send(()).ok();
|
||||||
|
}
|
||||||
|
|
||||||
|
fn apply_diffs_to_view(
|
||||||
|
diff_view: &ViewHandle<CodeDiffView>,
|
||||||
|
diffs: Vec<FileDiff>,
|
||||||
|
diff_session_type: DiffSessionType,
|
||||||
|
ctx: &mut ModelContext<Self>,
|
||||||
|
) {
|
||||||
diff_view.update(ctx, |diff_view, ctx| {
|
diff_view.update(ctx, |diff_view, ctx| {
|
||||||
diff_view.set_diff_session_type(diff_session_type);
|
diff_view.set_diff_session_type(diff_session_type);
|
||||||
diff_view.set_candidate_diffs(diffs, ctx);
|
diff_view.set_candidate_diffs(diffs, ctx);
|
||||||
|
|||||||
@@ -2,8 +2,36 @@ use std::collections::HashMap;
|
|||||||
|
|
||||||
use ai::agent::action_result::AnyFileContent;
|
use ai::agent::action_result::AnyFileContent;
|
||||||
use ai::agent::FileLocations;
|
use ai::agent::FileLocations;
|
||||||
|
use ai::diff_validation::DiffType;
|
||||||
|
|
||||||
use super::updated_file_contexts_from_editor_buffers;
|
use super::{
|
||||||
|
updated_file_contexts_from_editor_buffers, AIAgentActionId, DiffSessionType, FileDiff,
|
||||||
|
PendingAppliedDiffs,
|
||||||
|
};
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn applied_diffs_survive_until_delayed_view_registration() {
|
||||||
|
let action_id = AIAgentActionId::from("file-edit".to_string());
|
||||||
|
let mut pending = PendingAppliedDiffs::default();
|
||||||
|
pending.buffer(
|
||||||
|
action_id.clone(),
|
||||||
|
vec![FileDiff::new(
|
||||||
|
"before".to_string(),
|
||||||
|
"/workspace/src/main.rs".to_string(),
|
||||||
|
DiffType::update(vec![], None),
|
||||||
|
)],
|
||||||
|
DiffSessionType::Local,
|
||||||
|
);
|
||||||
|
|
||||||
|
let (diffs, session_type) = pending
|
||||||
|
.take(&action_id)
|
||||||
|
.expect("buffered diffs should remain available for registration");
|
||||||
|
assert_eq!(diffs.len(), 1);
|
||||||
|
assert_eq!(diffs[0].base.content, "before");
|
||||||
|
assert_eq!(diffs[0].base.file_path, "/workspace/src/main.rs");
|
||||||
|
assert!(matches!(session_type, DiffSessionType::Local));
|
||||||
|
assert!(pending.take(&action_id).is_none());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn updated_file_contexts_from_editor_buffers_returns_changed_lines_with_context() {
|
fn updated_file_contexts_from_editor_buffers_returns_changed_lines_with_context() {
|
||||||
|
|||||||
@@ -140,6 +140,47 @@ fn phased_scheduling_stops_at_serial_barrier_and_resumes_afterward() {
|
|||||||
assert_eq!(count_startable_actions_for_pass(&actions[3..]), 2);
|
assert_eq!(count_startable_actions_for_pass(&actions[3..]), 2);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn automatic_retries_only_target_actions_deferred_as_not_ready() {
|
||||||
|
let conversation_id = AIConversationId::new();
|
||||||
|
let action_id = AIAgentActionId::from("file-edit".to_string());
|
||||||
|
let mut tracker = NotReadyActionTracker::default();
|
||||||
|
|
||||||
|
tracker.update_after_attempt(
|
||||||
|
conversation_id,
|
||||||
|
action_id.clone(),
|
||||||
|
NotExecutedReason::NotReady,
|
||||||
|
ActionExecutionInitiator::Automatic,
|
||||||
|
);
|
||||||
|
assert!(tracker.should_retry(conversation_id, &action_id));
|
||||||
|
assert!(!ActionExecutionInitiator::Automatic.is_user_initiated());
|
||||||
|
|
||||||
|
tracker.update_after_attempt(
|
||||||
|
conversation_id,
|
||||||
|
action_id.clone(),
|
||||||
|
NotExecutedReason::NotReady,
|
||||||
|
ActionExecutionInitiator::User,
|
||||||
|
);
|
||||||
|
assert!(!tracker.should_retry(conversation_id, &action_id));
|
||||||
|
|
||||||
|
tracker.update_after_attempt(
|
||||||
|
conversation_id,
|
||||||
|
action_id.clone(),
|
||||||
|
NotExecutedReason::NeedsConfirmation,
|
||||||
|
ActionExecutionInitiator::Automatic,
|
||||||
|
);
|
||||||
|
assert!(!tracker.should_retry(conversation_id, &action_id));
|
||||||
|
|
||||||
|
tracker.update_after_attempt(
|
||||||
|
conversation_id,
|
||||||
|
action_id.clone(),
|
||||||
|
NotExecutedReason::WaitingOnSharer,
|
||||||
|
ActionExecutionInitiator::Automatic,
|
||||||
|
);
|
||||||
|
assert!(!tracker.should_retry(conversation_id, &action_id));
|
||||||
|
assert!(ActionExecutionInitiator::User.is_user_initiated());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn finished_results_stay_in_original_action_order() {
|
fn finished_results_stay_in_original_action_order() {
|
||||||
let action_order = HashMap::from([
|
let action_order = HashMap::from([
|
||||||
|
|||||||
@@ -2196,6 +2196,19 @@ impl AIBlock {
|
|||||||
}
|
}
|
||||||
|
|
||||||
match action {
|
match action {
|
||||||
|
AIAgentAction {
|
||||||
|
id: action_id,
|
||||||
|
action: AIAgentActionType::RequestFileEdits { title, file_edits },
|
||||||
|
..
|
||||||
|
} => {
|
||||||
|
self.ensure_requested_edit_view(
|
||||||
|
action_id,
|
||||||
|
title,
|
||||||
|
file_edits.clone(),
|
||||||
|
output.server_output_id.clone(),
|
||||||
|
ctx,
|
||||||
|
);
|
||||||
|
}
|
||||||
AIAgentAction {
|
AIAgentAction {
|
||||||
id: action_id,
|
id: action_id,
|
||||||
action:
|
action:
|
||||||
@@ -2761,7 +2774,7 @@ impl AIBlock {
|
|||||||
},
|
},
|
||||||
..
|
..
|
||||||
} => {
|
} => {
|
||||||
self.handle_requested_edit_complete(
|
self.ensure_requested_edit_view(
|
||||||
id,
|
id,
|
||||||
title,
|
title,
|
||||||
file_edits.clone(),
|
file_edits.clone(),
|
||||||
@@ -3281,7 +3294,7 @@ impl AIBlock {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
fn handle_requested_edit_complete(
|
fn ensure_requested_edit_view(
|
||||||
&mut self,
|
&mut self,
|
||||||
action_id: &AIAgentActionId,
|
action_id: &AIAgentActionId,
|
||||||
title: &Option<String>,
|
title: &Option<String>,
|
||||||
@@ -3289,6 +3302,10 @@ impl AIBlock {
|
|||||||
server_output_id: Option<ServerOutputId>,
|
server_output_id: Option<ServerOutputId>,
|
||||||
ctx: &mut ViewContext<Self>,
|
ctx: &mut ViewContext<Self>,
|
||||||
) {
|
) {
|
||||||
|
if self.requested_edits.contains_key(action_id) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
let identifiers = AIIdentifiers {
|
let identifiers = AIIdentifiers {
|
||||||
client_conversation_id: Some(self.client_ids.conversation_id),
|
client_conversation_id: Some(self.client_ids.conversation_id),
|
||||||
client_exchange_id: Some(self.client_ids.client_exchange_id),
|
client_exchange_id: Some(self.client_ids.client_exchange_id),
|
||||||
@@ -3344,14 +3361,6 @@ impl AIBlock {
|
|||||||
ctx,
|
ctx,
|
||||||
)
|
)
|
||||||
});
|
});
|
||||||
let executor = self
|
|
||||||
.action_model
|
|
||||||
.as_ref(ctx)
|
|
||||||
.request_file_edits_executor(ctx);
|
|
||||||
executor.update(ctx, |executor, _| {
|
|
||||||
executor.register_requested_edits(action_id, &view);
|
|
||||||
});
|
|
||||||
|
|
||||||
// If the diff is being viewed in a shared session (read-only mode), populate diffs from the payload.
|
// If the diff is being viewed in a shared session (read-only mode), populate diffs from the payload.
|
||||||
if self.action_model.as_ref(ctx).is_view_only() {
|
if self.action_model.as_ref(ctx).is_view_only() {
|
||||||
let active_session = self.active_session.as_ref(ctx);
|
let active_session = self.active_session.as_ref(ctx);
|
||||||
@@ -3508,7 +3517,17 @@ impl AIBlock {
|
|||||||
});
|
});
|
||||||
|
|
||||||
self.requested_edits
|
self.requested_edits
|
||||||
.insert(action_id.clone(), RequestedEdit::new(view));
|
.insert(action_id.clone(), RequestedEdit::new(view.clone()));
|
||||||
|
let executor = self
|
||||||
|
.action_model
|
||||||
|
.as_ref(ctx)
|
||||||
|
.request_file_edits_executor(ctx);
|
||||||
|
executor.update(ctx, |executor, ctx| {
|
||||||
|
executor.register_requested_edits(action_id, &view, ctx);
|
||||||
|
});
|
||||||
|
self.action_model.update(ctx, |action_model, ctx| {
|
||||||
|
action_model.retry_not_ready_action(action_id, self.client_ids.conversation_id, ctx);
|
||||||
|
});
|
||||||
|
|
||||||
if self.model.request_type(ctx).is_passive() {
|
if self.model.request_type(ctx).is_passive() {
|
||||||
BlocklistAIHistoryModel::handle(ctx).update(ctx, |history, ctx| {
|
BlocklistAIHistoryModel::handle(ctx).update(ctx, |history, ctx| {
|
||||||
|
|||||||
@@ -136,6 +136,10 @@ use crate::{AIAgentTodoList, FeatureFlag};
|
|||||||
|
|
||||||
const BLOCKED_ACTION_MESSAGE_FOR_UPLOADING_ARTIFACT: &str = "Grant access to upload this artifact?";
|
const BLOCKED_ACTION_MESSAGE_FOR_UPLOADING_ARTIFACT: &str = "Grant access to upload this artifact?";
|
||||||
|
|
||||||
|
fn should_render_requested_edit(action_status: Option<&AIActionStatus>) -> bool {
|
||||||
|
!action_status.is_some_and(AIActionStatus::is_preprocessing)
|
||||||
|
}
|
||||||
|
|
||||||
/// Data required to render the AI block output component.
|
/// Data required to render the AI block output component.
|
||||||
#[derive(Copy, Clone)]
|
#[derive(Copy, Clone)]
|
||||||
pub(crate) struct Props<'a> {
|
pub(crate) struct Props<'a> {
|
||||||
@@ -564,10 +568,7 @@ pub(super) fn render(props: Props, app: &AppContext) -> Box<dyn Element> {
|
|||||||
let action_status =
|
let action_status =
|
||||||
props.action_model.as_ref(app).get_action_status(id);
|
props.action_model.as_ref(app).get_action_status(id);
|
||||||
|
|
||||||
let is_preprocessing = action_status
|
if should_render_requested_edit(action_status.as_ref()) {
|
||||||
.clone()
|
|
||||||
.is_some_and(|status| status.is_preprocessing());
|
|
||||||
if !is_preprocessing && !status.is_streaming() {
|
|
||||||
if let Some(requested_edit) = props.requested_edits.get(id) {
|
if let Some(requested_edit) = props.requested_edits.get(id) {
|
||||||
// Don't render the requested edit if the diffs are empty for passive code diffs.
|
// Don't render the requested edit if the diffs are empty for passive code diffs.
|
||||||
if request_type.is_passive_code_diff()
|
if request_type.is_passive_code_diff()
|
||||||
|
|||||||
@@ -11,12 +11,23 @@ use watcher::HomeDirectoryWatcher;
|
|||||||
|
|
||||||
use super::{
|
use super::{
|
||||||
format_upload_artifact_text, parsed_skill_for_common_locations, read_skill_display_text,
|
format_upload_artifact_text, parsed_skill_for_common_locations, read_skill_display_text,
|
||||||
|
should_render_requested_edit,
|
||||||
};
|
};
|
||||||
use crate::ai::agent::UploadArtifactResult;
|
use crate::ai::agent::UploadArtifactResult;
|
||||||
|
use crate::ai::blocklist::action_model::AIActionStatus;
|
||||||
use crate::ai::skills::SkillManager;
|
use crate::ai::skills::SkillManager;
|
||||||
use crate::settings::AISettings;
|
use crate::settings::AISettings;
|
||||||
use crate::warp_managed_paths_watcher::WarpManagedPathsWatcher;
|
use crate::warp_managed_paths_watcher::WarpManagedPathsWatcher;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn requested_edits_render_as_soon_as_preprocessing_finishes() {
|
||||||
|
assert!(!should_render_requested_edit(Some(
|
||||||
|
&AIActionStatus::Preprocessing
|
||||||
|
)));
|
||||||
|
assert!(should_render_requested_edit(Some(&AIActionStatus::Blocked)));
|
||||||
|
assert!(should_render_requested_edit(None));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn format_upload_artifact_text_includes_request_details() {
|
fn format_upload_artifact_text_includes_request_details() {
|
||||||
let request = UploadArtifactRequest {
|
let request = UploadArtifactRequest {
|
||||||
|
|||||||
@@ -366,7 +366,7 @@ pub enum CodeDiffState {
|
|||||||
/// The diff is received, but is queued for interaction behind another action.
|
/// The diff is received, but is queued for interaction behind another action.
|
||||||
Queued,
|
Queued,
|
||||||
/// The user is reviewing (and possibly editing) the code diff.
|
/// The user is reviewing (and possibly editing) the code diff.
|
||||||
/// Unlike requested commands, a [`CodeDiffView`] is only created upon stream completion.
|
/// The view is created as soon as the requested edit is present in streaming output.
|
||||||
WaitingForUser,
|
WaitingForUser,
|
||||||
/// If the payload is some, the code diff was accepted but the individual file changes have not
|
/// If the payload is some, the code diff was accepted but the individual file changes have not
|
||||||
/// been fully computed and saved yet. We cache the accepted diff state to collect unified diffs
|
/// been fully computed and saved yet. We cache the accepted diff state to collect unified diffs
|
||||||
|
|||||||
@@ -82,6 +82,86 @@ fn restored_provider_projection_skips_stream_initialization() {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn provider_followup_turn_starts_a_distinct_text_message() {
|
||||||
|
let mut projector = ProviderRunResponseProjector::new(RuntimeResponseConfig {
|
||||||
|
task_id: "task".to_owned(),
|
||||||
|
conversation_id: "conversation".to_owned(),
|
||||||
|
needs_create_task: false,
|
||||||
|
user_query: None,
|
||||||
|
model_id: "model".to_owned(),
|
||||||
|
max_context_tokens: Some(1_000),
|
||||||
|
capabilities: RuntimeCapabilities::provider(),
|
||||||
|
empty_output_message: None,
|
||||||
|
});
|
||||||
|
let first_work_id = galaxy_agent_core::ExternalWorkId {
|
||||||
|
run_id: galaxy_agent_core::ProviderRunId::new("run"),
|
||||||
|
epoch: galaxy_agent_core::RunEpoch::new(1),
|
||||||
|
};
|
||||||
|
projector
|
||||||
|
.project(ProviderRunProjection::ModelTurnStarted {
|
||||||
|
work_id: first_work_id.clone(),
|
||||||
|
profile: galaxy_agent_core::ProviderRequestProfile::new("base"),
|
||||||
|
runtime_id: "runtime".to_owned(),
|
||||||
|
model_id: "model".to_owned(),
|
||||||
|
runtime_request_id: "request-1".to_owned(),
|
||||||
|
retry_attempt: 0,
|
||||||
|
elapsed_ms: 1,
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
let first_text = projector
|
||||||
|
.project(ProviderRunProjection::ModelEvent {
|
||||||
|
work_id: first_work_id,
|
||||||
|
event: AgentEvent::TextDelta {
|
||||||
|
text: "before tool".to_owned(),
|
||||||
|
},
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
let Some(response_event::Type::ClientActions(first_actions)) = &first_text[0].r#type else {
|
||||||
|
panic!("expected first text action");
|
||||||
|
};
|
||||||
|
let Some(client_action::Action::AddMessagesToTask(first_add)) =
|
||||||
|
&first_actions.actions[0].action
|
||||||
|
else {
|
||||||
|
panic!("first turn should add a text message");
|
||||||
|
};
|
||||||
|
let first_message_id = first_add.messages[0].id.clone();
|
||||||
|
|
||||||
|
let second_work_id = galaxy_agent_core::ExternalWorkId {
|
||||||
|
run_id: galaxy_agent_core::ProviderRunId::new("run"),
|
||||||
|
epoch: galaxy_agent_core::RunEpoch::new(2),
|
||||||
|
};
|
||||||
|
projector
|
||||||
|
.project(ProviderRunProjection::ModelTurnStarted {
|
||||||
|
work_id: second_work_id.clone(),
|
||||||
|
profile: galaxy_agent_core::ProviderRequestProfile::new("base"),
|
||||||
|
runtime_id: "runtime".to_owned(),
|
||||||
|
model_id: "model".to_owned(),
|
||||||
|
runtime_request_id: "request-2".to_owned(),
|
||||||
|
retry_attempt: 0,
|
||||||
|
elapsed_ms: 1,
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
let second_text = projector
|
||||||
|
.project(ProviderRunProjection::ModelEvent {
|
||||||
|
work_id: second_work_id,
|
||||||
|
event: AgentEvent::TextDelta {
|
||||||
|
text: "after tool".to_owned(),
|
||||||
|
},
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
let Some(response_event::Type::ClientActions(second_actions)) = &second_text[0].r#type else {
|
||||||
|
panic!("expected follow-up text action");
|
||||||
|
};
|
||||||
|
let Some(client_action::Action::AddMessagesToTask(second_add)) =
|
||||||
|
&second_actions.actions[0].action
|
||||||
|
else {
|
||||||
|
panic!("follow-up turn should add a text message");
|
||||||
|
};
|
||||||
|
|
||||||
|
assert_ne!(first_message_id, second_add.messages[0].id);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn provider_and_session_runtimes_share_text_translation() {
|
fn provider_and_session_runtimes_share_text_translation() {
|
||||||
for mut translator in [provider_translator(), session_translator()] {
|
for mut translator in [provider_translator(), session_translator()] {
|
||||||
|
|||||||
Reference in New Issue
Block a user