mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Make turn diff tracker multi-env aware (#26433)
## Why Turn diffs were tracked as one flat set of absolute paths. In multi-environment turns, local and remote environments can report the same path while representing different filesystems, so a single path key can collapse distinct changes or attribute them to the wrong environment. The environment name is **NOT** included in the generated unified diff. This can come later.
This commit is contained in:
committed by
GitHub
Unverified
parent
da490ba9de
commit
86a1ddd028
@@ -70,16 +70,25 @@ pub(crate) enum ToolEventFailure<'a> {
|
||||
}
|
||||
|
||||
enum TurnDiffTrackerUpdate<'a> {
|
||||
Track(&'a AppliedPatchDelta),
|
||||
Track {
|
||||
environment_id: Option<String>,
|
||||
delta: &'a AppliedPatchDelta,
|
||||
},
|
||||
Invalidate,
|
||||
None,
|
||||
}
|
||||
|
||||
fn tracker_update_for_known_delta(delta: &AppliedPatchDelta) -> TurnDiffTrackerUpdate<'_> {
|
||||
fn tracker_update_for_known_delta<'a>(
|
||||
environment_id: Option<&str>,
|
||||
delta: &'a AppliedPatchDelta,
|
||||
) -> TurnDiffTrackerUpdate<'a> {
|
||||
if delta.is_exact() && delta.is_empty() {
|
||||
TurnDiffTrackerUpdate::None
|
||||
} else {
|
||||
TurnDiffTrackerUpdate::Track(delta)
|
||||
TurnDiffTrackerUpdate::Track {
|
||||
environment_id: environment_id.map(str::to_string),
|
||||
delta,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -120,6 +129,7 @@ pub(crate) enum ToolEmitter {
|
||||
ApplyPatch {
|
||||
changes: HashMap<PathBuf, FileChange>,
|
||||
auto_approved: bool,
|
||||
environment_id: Option<String>,
|
||||
},
|
||||
UnifiedExec {
|
||||
command: Vec<String>,
|
||||
@@ -141,10 +151,15 @@ impl ToolEmitter {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn apply_patch(changes: HashMap<PathBuf, FileChange>, auto_approved: bool) -> Self {
|
||||
pub fn apply_patch_for_environment(
|
||||
changes: HashMap<PathBuf, FileChange>,
|
||||
auto_approved: bool,
|
||||
environment_id: String,
|
||||
) -> Self {
|
||||
Self::ApplyPatch {
|
||||
changes,
|
||||
auto_approved,
|
||||
environment_id: Some(environment_id),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -210,7 +225,11 @@ impl ToolEmitter {
|
||||
.await;
|
||||
}
|
||||
(
|
||||
Self::ApplyPatch { changes, .. },
|
||||
Self::ApplyPatch {
|
||||
changes,
|
||||
environment_id,
|
||||
..
|
||||
},
|
||||
ToolEventStage::Success {
|
||||
output,
|
||||
applied_patch_delta,
|
||||
@@ -222,7 +241,7 @@ impl ToolEmitter {
|
||||
PatchApplyStatus::Failed
|
||||
};
|
||||
let tracker_update = applied_patch_delta
|
||||
.map(tracker_update_for_known_delta)
|
||||
.map(|delta| tracker_update_for_known_delta(environment_id.as_deref(), delta))
|
||||
.unwrap_or(TurnDiffTrackerUpdate::Invalidate);
|
||||
emit_patch_end(
|
||||
ctx,
|
||||
@@ -267,7 +286,11 @@ impl ToolEmitter {
|
||||
.await;
|
||||
}
|
||||
(
|
||||
Self::ApplyPatch { changes, .. },
|
||||
Self::ApplyPatch {
|
||||
changes,
|
||||
environment_id,
|
||||
..
|
||||
},
|
||||
ToolEventStage::Failure(ToolEventFailure::Rejected {
|
||||
message,
|
||||
applied_patch_delta,
|
||||
@@ -280,7 +303,9 @@ impl ToolEmitter {
|
||||
(*message).to_string(),
|
||||
PatchApplyStatus::Declined,
|
||||
applied_patch_delta
|
||||
.map(tracker_update_for_known_delta)
|
||||
.map(|delta| {
|
||||
tracker_update_for_known_delta(environment_id.as_deref(), delta)
|
||||
})
|
||||
.unwrap_or(TurnDiffTrackerUpdate::None),
|
||||
)
|
||||
.await;
|
||||
@@ -565,8 +590,11 @@ async fn emit_patch_end(
|
||||
let mut guard = tracker.lock().await;
|
||||
let previous_diff = guard.get_unified_diff();
|
||||
let tracker_changed = match tracker_update {
|
||||
TurnDiffTrackerUpdate::Track(delta) => {
|
||||
guard.track_delta(delta);
|
||||
TurnDiffTrackerUpdate::Track {
|
||||
environment_id,
|
||||
delta,
|
||||
} => {
|
||||
guard.track_delta(environment_id.as_deref().unwrap_or_default(), delta);
|
||||
true
|
||||
}
|
||||
TurnDiffTrackerUpdate::Invalidate => {
|
||||
@@ -627,14 +655,18 @@ mod tests {
|
||||
.await
|
||||
.expect("apply patch");
|
||||
|
||||
ToolEmitter::apply_patch(HashMap::new(), /*auto_approved*/ false)
|
||||
.finish(
|
||||
ToolEventCtx::new(session.as_ref(), turn.as_ref(), "call-id", Some(&tracker)),
|
||||
out,
|
||||
Some(&delta),
|
||||
)
|
||||
.await
|
||||
.expect_err("failed patch");
|
||||
ToolEmitter::ApplyPatch {
|
||||
changes: HashMap::new(),
|
||||
auto_approved: false,
|
||||
environment_id: None,
|
||||
}
|
||||
.finish(
|
||||
ToolEventCtx::new(session.as_ref(), turn.as_ref(), "call-id", Some(&tracker)),
|
||||
out,
|
||||
Some(&delta),
|
||||
)
|
||||
.await
|
||||
.expect_err("failed patch");
|
||||
|
||||
let completed = rx_event.recv().await.expect("item completed event");
|
||||
assert!(matches!(
|
||||
|
||||
@@ -378,8 +378,11 @@ impl ToolExecutor<ToolInvocation> for ApplyPatchHandler {
|
||||
}
|
||||
InternalApplyPatchInvocation::DelegateToRuntime(apply) => {
|
||||
let changes = convert_apply_patch_to_protocol(&apply.action);
|
||||
let emitter =
|
||||
ToolEmitter::apply_patch(changes.clone(), apply.auto_approved);
|
||||
let emitter = ToolEmitter::apply_patch_for_environment(
|
||||
changes.clone(),
|
||||
apply.auto_approved,
|
||||
turn_environment.environment_id.clone(),
|
||||
);
|
||||
let event_ctx = ToolEventCtx::new(
|
||||
session.as_ref(),
|
||||
turn.as_ref(),
|
||||
@@ -537,7 +540,11 @@ pub(crate) async fn intercept_apply_patch(
|
||||
}
|
||||
InternalApplyPatchInvocation::DelegateToRuntime(apply) => {
|
||||
let changes = convert_apply_patch_to_protocol(&apply.action);
|
||||
let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved);
|
||||
let emitter = ToolEmitter::apply_patch_for_environment(
|
||||
changes.clone(),
|
||||
apply.auto_approved,
|
||||
turn_environment.environment_id.clone(),
|
||||
);
|
||||
let event_ctx = ToolEventCtx::new(
|
||||
session.as_ref(),
|
||||
turn.as_ref(),
|
||||
|
||||
Reference in New Issue
Block a user