Skip to content

Commit 959dedf

Browse files
authored
fix(acp): Handle ACP cancel-tail prompt ordering (#442)
## Summary - absorb repeated empty ACP `end_turn` responses after a cancelled turn before admitting the next logical prompt - add a deterministic mock ACP scenario plus ACP and TUI PTY regressions for the cancel-tail ordering bug - include the investigation report and evidence captured for the cancellation-ordering behavior ## Test Plan - [x] `cargo test -p mock-acp-agent` - [x] `cargo test -p codex-acp` - [x] `cargo build --bin nori && cargo test -p tui-pty-e2e`
1 parent e94aa02 commit 959dedf

24 files changed

Lines changed: 1872 additions & 17 deletions

codex-rs/Cargo.lock

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

codex-rs/acp/docs.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,8 @@ When `Op::Interrupt` fires, the ACP backend now only submits `InboundEvent::Canc
818818
- `SessionPhaseChanged(Idle)` and `PromptCompleted { stop_reason, last_agent_message }` are emitted only when that prompt response is reduced
819819
- queued follow-up prompts remain in the reducer-owned outbound queue until an eligible drain point (`stop_reason: end_turn`)
820820

821+
`SacpConnection::prompt()` also carries a small amount of session-local transport state so cancellation tails can be absorbed without widening the public phase model. If the previous prompt ended with `Cancelled`, the next prompt request may receive one or more immediate empty `end_turn` responses before the agent starts working on the user's real follow-up prompt. The connection layer now treats those empty terminal responses as stale cancel-tail cleanup and retries the same ACP prompt request until either streamed updates arrive or a non-stale stop reason is observed. That keeps the reducer contract unchanged: it still only sees the final logical completion for the user-facing prompt turn.
822+
821823
This removes the old synthetic interrupt-abort fast-path that treated cancel as immediate idle. The TUI now renders ACP interrupt state from reducer-owned phase/completion projections instead of inferring prompt ownership from interrupt timing.
822824

823825
**Tool Classification System:**

codex-rs/acp/src/backend/session_reducer.rs

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ use nori_protocol::session_runtime::SessionPhase;
1818
use nori_protocol::session_runtime::SessionRuntime;
1919
use nori_protocol::session_runtime::TranscriptMessage;
2020
use nori_protocol::session_runtime::TranscriptRole;
21+
use tracing::debug;
2122

2223
/// Everything that can affect [`SessionRuntime`] state.
2324
#[derive(Debug)]
@@ -40,6 +41,32 @@ pub enum InboundEvent {
4041
LoadSubmit { request_id: String },
4142
}
4243

44+
pub(super) fn inbound_event_kind(event: &InboundEvent) -> &'static str {
45+
match event {
46+
InboundEvent::Notification(update) => crate::connection::session_update_kind(update),
47+
InboundEvent::PromptResponse { .. } => "prompt_response",
48+
InboundEvent::PromptFailed => "prompt_failed",
49+
InboundEvent::LoadResponse => "load_response",
50+
InboundEvent::PermissionRequest { .. } => "permission_request",
51+
InboundEvent::PromptSubmit(_) => "prompt_submit",
52+
InboundEvent::CancelSubmit => "cancel_submit",
53+
InboundEvent::LoadSubmit { .. } => "load_submit",
54+
}
55+
}
56+
57+
pub(super) fn session_phase_label(phase: &SessionPhase) -> &'static str {
58+
match phase {
59+
SessionPhase::Idle => "idle",
60+
SessionPhase::Loading { .. } => "loading",
61+
SessionPhase::Prompt {
62+
cancelling: true, ..
63+
} => "cancelling",
64+
SessionPhase::Prompt {
65+
cancelling: false, ..
66+
} => "prompt",
67+
}
68+
}
69+
4370
/// Side effects the caller must execute after reduction.
4471
#[derive(Debug, PartialEq)]
4572
pub enum SideEffect {
@@ -116,6 +143,12 @@ fn reduce_prompt_submit(
116143
) {
117144
if runtime.phase != SessionPhase::Idle {
118145
runtime.queue.push_back(prompt);
146+
debug!(
147+
target: "acp_event_flow",
148+
phase = session_phase_label(&runtime.phase),
149+
queue_len = runtime.queue.len(),
150+
"Queued prompt while another session request is active"
151+
);
119152
out.events.push(ClientEvent::QueueChanged(QueueChanged {
120153
prompts: queued_prompt_texts(runtime),
121154
}));
@@ -126,6 +159,7 @@ fn reduce_prompt_submit(
126159
}
127160

128161
fn start_prompt(runtime: &mut SessionRuntime, prompt: QueuedPrompt, out: &mut ReduceOutput) {
162+
let phase_before = session_phase_label(&runtime.phase);
129163
let request_id = new_request_id();
130164

131165
// Build ACP content blocks from the queued prompt.
@@ -154,6 +188,15 @@ fn start_prompt(runtime: &mut SessionRuntime, prompt: QueuedPrompt, out: &mut Re
154188
});
155189
}
156190

191+
debug!(
192+
target: "acp_event_flow",
193+
request_id = %request_id,
194+
prompt_kind = ?prompt.kind,
195+
phase_before,
196+
queue_len = runtime.queue.len(),
197+
"Reducer started prompt and emitted session/prompt side effect"
198+
);
199+
157200
out.events
158201
.push(ClientEvent::SessionPhaseChanged(runtime.phase_view()));
159202
out.side_effects.push(SideEffect::SendPrompt {
@@ -198,6 +241,20 @@ fn reduce_cancel_submit(runtime: &mut SessionRuntime, out: &mut ReduceOutput) {
198241
}
199242
}
200243

244+
debug!(
245+
target: "acp_event_flow",
246+
request_id = %owner_id,
247+
pending_permission_requests = runtime
248+
.active
249+
.as_ref()
250+
.map_or(0, |active| active.pending_permission_requests.len()),
251+
tool_calls = runtime
252+
.active
253+
.as_ref()
254+
.map_or(0, |active| active.tool_call_ids.len()),
255+
"Reducer marked the active prompt as cancelling"
256+
);
257+
201258
out.events
202259
.push(ClientEvent::SessionPhaseChanged(runtime.phase_view()));
203260
out.side_effects.push(SideEffect::SendCancel);
@@ -213,6 +270,22 @@ fn reduce_prompt_response(
213270
stop_reason: acp::StopReason,
214271
out: &mut ReduceOutput,
215272
) {
273+
let active_request_id = runtime
274+
.active
275+
.as_ref()
276+
.map(|active| active.request_id.clone())
277+
.unwrap_or_else(|| "<none>".to_string());
278+
let phase_before = session_phase_label(&runtime.phase);
279+
let queue_len_before = runtime.queue.len();
280+
debug!(
281+
target: "acp_event_flow",
282+
active_request_id,
283+
phase_before,
284+
queue_len_before,
285+
?stop_reason,
286+
"Reducer received prompt response"
287+
);
288+
216289
if !matches!(runtime.phase, SessionPhase::Prompt { .. }) {
217290
out.events.push(ClientEvent::Warning(WarningInfo {
218291
message: "Received prompt response while not in Prompt phase".to_string(),
@@ -225,6 +298,15 @@ fn reduce_prompt_response(
225298

226299
runtime.phase = SessionPhase::Idle;
227300

301+
debug!(
302+
target: "acp_event_flow",
303+
active_request_id,
304+
?stop_reason,
305+
should_drain_queue,
306+
queue_len_after_finalize = runtime.queue.len(),
307+
"Reducer finalized prompt response"
308+
);
309+
228310
out.events
229311
.push(ClientEvent::SessionPhaseChanged(runtime.phase_view()));
230312
out.events
@@ -242,6 +324,18 @@ fn reduce_prompt_response(
242324
}
243325

244326
fn reduce_prompt_failed(runtime: &mut SessionRuntime, out: &mut ReduceOutput) {
327+
let active_request_id = runtime
328+
.active
329+
.as_ref()
330+
.map(|active| active.request_id.clone())
331+
.unwrap_or_else(|| "<none>".to_string());
332+
debug!(
333+
target: "acp_event_flow",
334+
active_request_id,
335+
phase = session_phase_label(&runtime.phase),
336+
"Reducer received prompt failure"
337+
);
338+
245339
if !matches!(runtime.phase, SessionPhase::Prompt { .. }) {
246340
out.events.push(ClientEvent::Warning(WarningInfo {
247341
message: "Received prompt failure while not in Prompt phase".to_string(),
@@ -308,6 +402,18 @@ fn reduce_notification(
308402
normalizer: &mut ClientEventNormalizer,
309403
out: &mut ReduceOutput,
310404
) {
405+
debug!(
406+
target: "acp_event_flow",
407+
update_kind = crate::connection::session_update_kind(&update),
408+
phase = session_phase_label(&runtime.phase),
409+
active_request_id = runtime
410+
.active
411+
.as_ref()
412+
.map(|active| active.request_id.as_str())
413+
.unwrap_or("<none>"),
414+
"Reducer received session/update"
415+
);
416+
311417
// Session metadata updates are accepted in any phase.
312418
if is_session_metadata_update(&update) {
313419
reduce_metadata_update(runtime, &update, normalizer, out);

codex-rs/acp/src/backend/session_reducer/tests.rs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ use pretty_assertions::assert_eq;
1111

1212
use super::InboundEvent;
1313
use super::SideEffect;
14+
use super::inbound_event_kind;
1415
use super::reduce;
16+
use super::session_phase_label;
1517

1618
fn new_runtime() -> SessionRuntime {
1719
SessionRuntime::new()
@@ -125,6 +127,41 @@ fn prompt_response_transitions_to_idle() {
125127
)));
126128
}
127129

130+
#[test]
131+
fn inbound_event_kind_labels_prompt_response() {
132+
assert_eq!(
133+
inbound_event_kind(&InboundEvent::PromptResponse {
134+
stop_reason: acp::StopReason::Cancelled,
135+
}),
136+
"prompt_response"
137+
);
138+
}
139+
140+
#[test]
141+
fn session_phase_label_labels_known_phases() {
142+
assert_eq!(session_phase_label(&SessionPhase::Idle), "idle");
143+
assert_eq!(
144+
session_phase_label(&SessionPhase::Prompt {
145+
request_id: "req-1".to_string(),
146+
cancelling: false,
147+
}),
148+
"prompt"
149+
);
150+
assert_eq!(
151+
session_phase_label(&SessionPhase::Prompt {
152+
request_id: "req-1".to_string(),
153+
cancelling: true,
154+
}),
155+
"cancelling"
156+
);
157+
assert_eq!(
158+
session_phase_label(&SessionPhase::Loading {
159+
request_id: "req-2".to_string(),
160+
}),
161+
"loading"
162+
);
163+
}
164+
128165
// =========================================================================
129166
// 2. Cancel semantics
130167
// =========================================================================

0 commit comments

Comments
 (0)