Skip to content

Commit 0ac5303

Browse files
authored
fix(acp): stop persisting in-progress tool snapshots (#437)
## Summary 🤖 Generated with [Nori](https://www.npmjs.com/package/nori-ai) - stop recording ACP `ToolSnapshot` events with `phase == InProgress` in transcripts while preserving live TUI delivery - tighten the runaway-search regression test to verify the live exploring cell still renders and transcripts keep only durable snapshots - leave the local Markdown notes/docs out of the commit as requested ## Test Plan - [x] `cargo test -p codex-acp` - [x] `cargo build --bin nori` - [x] `cargo test -p tui-pty-e2e` - [x] `just fmt` - [x] `just fix -p codex-acp` - [x] `just fix -p tui-pty-e2e` - [x] tmux smoke test launching `./target/debug/nori --agent elizacp --skip-trust-directory`, sending `hello`, and verifying the `›` prompt returned Share Nori with your team: https://www.npmjs.com/package/nori-skillsets
1 parent 812900e commit 0ac5303

8 files changed

Lines changed: 409 additions & 0 deletions

File tree

codex-rs/Cargo.lock

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

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,7 @@ pub(crate) async fn emit_client_event(
354354
.send(BackendEvent::Client(client_event.clone()))
355355
.await;
356356
if let Some(recorder) = transcript_recorder
357+
&& transcript::should_record_client_event(&client_event)
357358
&& let Err(e) = recorder.record_client_event(&client_event).await
358359
{
359360
warn!("Failed to record normalized client event: {e}");

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,14 @@ use super::*;
55
/// rejection is the real guard — but large summaries are worth logging.
66
const TRANSCRIPT_SUMMARY_WARN_CHARS: usize = 200_000;
77

8+
pub(crate) fn should_record_client_event(event: &nori_protocol::ClientEvent) -> bool {
9+
!matches!(
10+
event,
11+
nori_protocol::ClientEvent::ToolSnapshot(snapshot)
12+
if snapshot.phase == nori_protocol::ToolPhase::InProgress
13+
)
14+
}
15+
816
/// Convert a loaded transcript into normalized replay events suitable for ACP
917
/// session resume. The replay stream is intentionally static: it reconstructs
1018
/// user/assistant history and completed normalized artifacts without reviving
@@ -314,4 +322,43 @@ mod tests {
314322
]
315323
);
316324
}
325+
326+
#[test]
327+
fn transcript_recording_skips_in_progress_tool_snapshots() {
328+
let in_progress = nori_protocol::ClientEvent::ToolSnapshot(nori_protocol::ToolSnapshot {
329+
call_id: "tool-1".into(),
330+
title: "Search repo".into(),
331+
kind: nori_protocol::ToolKind::Search,
332+
phase: nori_protocol::ToolPhase::InProgress,
333+
locations: vec![],
334+
invocation: Some(nori_protocol::Invocation::Search {
335+
query: Some("needle".into()),
336+
path: Some(PathBuf::from("/repo")),
337+
}),
338+
artifacts: vec![nori_protocol::Artifact::Text {
339+
text: "streaming output".into(),
340+
}],
341+
raw_input: None,
342+
raw_output: None,
343+
owner_request_id: None,
344+
});
345+
let pending = nori_protocol::ClientEvent::ToolSnapshot(nori_protocol::ToolSnapshot {
346+
phase: nori_protocol::ToolPhase::Pending,
347+
..match &in_progress {
348+
nori_protocol::ClientEvent::ToolSnapshot(snapshot) => snapshot.clone(),
349+
_ => unreachable!(),
350+
}
351+
});
352+
let completed = nori_protocol::ClientEvent::ToolSnapshot(nori_protocol::ToolSnapshot {
353+
phase: nori_protocol::ToolPhase::Completed,
354+
..match &in_progress {
355+
nori_protocol::ClientEvent::ToolSnapshot(snapshot) => snapshot.clone(),
356+
_ => unreachable!(),
357+
}
358+
});
359+
360+
assert!(!should_record_client_event(&in_progress));
361+
assert!(should_record_client_event(&pending));
362+
assert!(should_record_client_event(&completed));
363+
}
317364
}

codex-rs/mock-acp-agent/docs.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ Used by `@/codex-rs/tui-pty-e2e/` for end-to-end integration testing. The mock a
3232

3333
**Stuck Tool Calls (No Completion)**: The `MOCK_AGENT_STUCK_TOOL_CALLS` env var triggers a scenario where 3 Read tool calls are sent with `Pending` status but never receive completion updates. After a short delay the agent sends its final text response and ends the turn. This reproduces the frozen-display bug where incomplete ExecCells fill the viewport and block `insert_history_lines()` from rendering the agent's text. The fix under test is `finalize_active_cell_as_failed()` in `@/codex-rs/tui/src/chatwidget.rs`.
3434

35+
**Runaway Search Snapshot Amplification**: The `MOCK_AGENT_RUNAWAY_SEARCH` env var triggers a deterministic Search tool-call stream that repeatedly emits `InProgress` updates for the **same** `call_id` while the text artifact grows cumulatively on every update. Tunables:
36+
- `MOCK_AGENT_RUNAWAY_SEARCH_UPDATES` -- number of `ToolCallUpdate(InProgress)` events to emit
37+
- `MOCK_AGENT_RUNAWAY_SEARCH_LINES_PER_UPDATE` -- how many search-result lines to append per update
38+
- `MOCK_AGENT_RUNAWAY_SEARCH_LINE_LEN` -- target width for each generated result line
39+
- `MOCK_AGENT_RUNAWAY_SEARCH_DELAY_MS` -- delay between updates
40+
- `MOCK_AGENT_RUNAWAY_SEARCH_SKIP_COMPLETION` -- if set, do not send a final `Completed` update
41+
- `MOCK_AGENT_RUNAWAY_SEARCH_SKIP_FINAL_TEXT` -- if set, do not send a final text chunk
42+
43+
Used by `@/codex-rs/tui-pty-e2e/tests/acp_runaway_search.rs` to reproduce the current ACP backend bug where one streaming search is normalized and recorded as many full snapshots, eventually crashing `nori` under constrained memory.
44+
3545
**Race Condition Simulation**: The `MOCK_AGENT_TOOL_CALLS_DURING_FINAL_STREAM` env var triggers a scenario that reproduces the timing where tool call completions arrive while the final text response is streaming. This is structured in phases:
3646
1. Tool calls that complete before text streaming starts (rendered normally)
3747
2. Text streaming begins (activates the TUI's stream_controller)

codex-rs/mock-acp-agent/src/main.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
//! Mock ACP agent for testing nori-cli
22
3+
mod runaway_search;
4+
35
use std::cell::Cell;
46
use std::path::PathBuf;
57
use std::sync::Arc;
@@ -331,6 +333,10 @@ impl acp::Agent for MockAgent {
331333
return Ok(acp::PromptResponse::new(acp::StopReason::EndTurn));
332334
}
333335

336+
if std::env::var("MOCK_AGENT_RUNAWAY_SEARCH").is_ok() {
337+
return runaway_search::run(self, session_id).await;
338+
}
339+
334340
// Reproduce the orphan tool cell bug caused by cascade deferral.
335341
//
336342
// The bug sequence:
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
use agent_client_protocol::{self as acp};
2+
use serde_json::json;
3+
use tokio::time::Duration;
4+
use tokio::time::sleep;
5+
6+
use crate::MockAgent;
7+
8+
const RUNAWAY_TITLE: &str = "Search runaway-pattern in runaway-search-fixture";
9+
const RUNAWAY_CALL_ID: &str = "runaway-search-001";
10+
11+
pub(crate) async fn run(
12+
agent: &MockAgent,
13+
session_id: acp::SessionId,
14+
) -> Result<acp::PromptResponse, acp::Error> {
15+
let updates = env_usize("MOCK_AGENT_RUNAWAY_SEARCH_UPDATES", 60);
16+
let lines_per_update = env_usize("MOCK_AGENT_RUNAWAY_SEARCH_LINES_PER_UPDATE", 25);
17+
let line_len = env_usize("MOCK_AGENT_RUNAWAY_SEARCH_LINE_LEN", 96);
18+
let delay_ms = env_u64("MOCK_AGENT_RUNAWAY_SEARCH_DELAY_MS", 2);
19+
let skip_completion = std::env::var("MOCK_AGENT_RUNAWAY_SEARCH_SKIP_COMPLETION").is_ok();
20+
let skip_final_text = std::env::var("MOCK_AGENT_RUNAWAY_SEARCH_SKIP_FINAL_TEXT").is_ok();
21+
22+
eprintln!(
23+
"Mock agent: sending runaway search stream updates={updates} lines_per_update={lines_per_update} line_len={line_len} delay_ms={delay_ms}"
24+
);
25+
26+
let call_id = acp::ToolCallId::new(RUNAWAY_CALL_ID);
27+
agent
28+
.send_tool_call(
29+
session_id.clone(),
30+
acp::ToolCall::new(call_id.clone(), RUNAWAY_TITLE)
31+
.kind(acp::ToolKind::Search)
32+
.status(acp::ToolCallStatus::Pending)
33+
.raw_input(json!({
34+
"pattern": "runaway-pattern",
35+
"path": "runaway-search-fixture",
36+
})),
37+
)
38+
.await?;
39+
40+
let padding = "x".repeat(line_len);
41+
let mut cumulative_output = String::new();
42+
43+
for update_index in 0..updates {
44+
if agent.cancel_requested.get() {
45+
return Ok(acp::PromptResponse::new(acp::StopReason::Cancelled));
46+
}
47+
48+
for line_index in 0..lines_per_update {
49+
let prefix = format!(
50+
"/repo/runaway-search-fixture/src/path_{update_index:04}_{line_index:04}.rs:{}: runaway-pattern ",
51+
update_index * lines_per_update + line_index + 1
52+
);
53+
cumulative_output.push_str(&prefix);
54+
let padding_len = line_len.saturating_sub(prefix.len());
55+
cumulative_output.push_str(&padding[..padding_len]);
56+
cumulative_output.push('\n');
57+
}
58+
59+
agent
60+
.send_tool_call_update(
61+
session_id.clone(),
62+
acp::ToolCallUpdate::new(
63+
call_id.clone(),
64+
acp::ToolCallUpdateFields::new()
65+
.status(acp::ToolCallStatus::InProgress)
66+
.content(vec![acp::ToolCallContent::Content(acp::Content::new(
67+
acp::ContentBlock::Text(acp::TextContent::new(
68+
cumulative_output.clone(),
69+
)),
70+
))]),
71+
),
72+
)
73+
.await?;
74+
75+
if delay_ms > 0 {
76+
sleep(Duration::from_millis(delay_ms)).await;
77+
}
78+
}
79+
80+
if !skip_completion {
81+
agent
82+
.send_tool_call_update(
83+
session_id.clone(),
84+
acp::ToolCallUpdate::new(
85+
call_id,
86+
acp::ToolCallUpdateFields::new().status(acp::ToolCallStatus::Completed),
87+
),
88+
)
89+
.await?;
90+
}
91+
92+
if !skip_final_text {
93+
agent
94+
.send_text_chunk(session_id, "Runaway search scenario complete.")
95+
.await?;
96+
}
97+
98+
Ok(acp::PromptResponse::new(acp::StopReason::EndTurn))
99+
}
100+
101+
fn env_usize(name: &str, default: usize) -> usize {
102+
std::env::var(name)
103+
.ok()
104+
.and_then(|value| value.parse::<usize>().ok())
105+
.unwrap_or(default)
106+
}
107+
108+
fn env_u64(name: &str, default: u64) -> u64 {
109+
std::env::var(name)
110+
.ok()
111+
.and_then(|value| value.parse::<u64>().ok())
112+
.unwrap_or(default)
113+
}

codex-rs/tui-pty-e2e/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,5 @@ libc = "0.2"
1818
[dev-dependencies]
1919
tempfile = "3"
2020
regex = "1"
21+
pretty_assertions = { workspace = true }
22+
serde_json = { workspace = true }

0 commit comments

Comments
 (0)