Skip to content

fix: send only stdout delta in Bash terminal output to prevent replay #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
william0wang merged 1 commit into from
Jul 10, 2026
Merged

fix: send only stdout delta in Bash terminal output to prevent replay #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 38 additions & 29 deletions src/handlers/dispatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,21 +131,23 @@ function dispatchToolCallUpdate(
* Bash terminal update — the 2-notification split (matches acp-agent.ts:5061-5094
* and the Python bridge's _dispatch_event). Zed correlates by terminal_id, so
* the two notifications MUST be separate:
* ① terminal_output — pure data, no status/content. Sent on BOTH progress and
* result states (whenever there's NEW data) so live output streams.
* ① terminal_output — pure data, no status/content. Streams live output.
* ② terminal_exit — terminal state only: status + content[type:terminal] +
* _meta.terminal_exit (with exitCode) + rawOutput text fallback. Sent ONLY
* on completed/failed.
* _meta.terminal_exit (with exitCode). Sent ONLY on completed/failed.
*
* Merging them into one notification causes Zed to clear the content once the
* turn completes (the original bug); splitting keeps the output visible.
*
* DELTA DEDUP: zcode's `stdoutTail`/result `content` is a *snapshot* of the
* accumulated stdout tail, but ACP `terminal_output.data` is *append* semantics
* — each value is written to the terminal as new bytes. Without dedup, every
* progress event replays the whole tail, and the result event replays it again,
* so long-running commands show their output N times. We track the last-sent
* snapshot per callId and emit only the suffix beyond it.
* OUTPUT DEDUP (two distinct hazards, both fixed here):
* - Progress replay: zcode's `stdoutTail` is a CUMULATIVE snapshot of the tail,
* but terminal_output.data is APPEND semantics. Without diffing, each progress
* event replays the whole tail → N× duplication. We track the last-sent
* snapshot per callId and emit only the suffix beyond it.
* - Result replay: zcode's result payload re-sends the COMPLETE output. Once
* progress has streamed it, emitting it again would duplicate every line.
* So on completed/failed we SKIP terminal_output IF any progress already
* streamed data for this callId; for short commands (scheduled → result, no
* progress) nothing was sent yet, so we emit the full output once.
*/
async function dispatchTerminalUpdate(
server: ZcodeAcpServer,
Expand All @@ -154,29 +156,41 @@ async function dispatchTerminalUpdate(
ev: Extract<InternalEvent, { kind: "ToolCallUpdate" }>,
toolName: string,
): Promise<void> {
// ① terminal_output (pure data, append semantics) — progress and result both
// emit it, but only the delta beyond the last-sent snapshot.
// Extract the textual output from whichever payload field carries it.
let termData: unknown = ev.rawOutput ?? ev.rawResult;
if (termData && typeof termData === "object" && !Array.isArray(termData)) {
termData = (termData as Record<string, unknown>)["content"] ?? "";
}
const full = termData ? String(termData) : "";
const lastSent = server.terminalSentData.get(ev.callId) ?? "";
// Only the suffix past the previously-sent snapshot is new output. Guard
// against a non-monotonic snapshot (tail rotated) by falling back to the full
// value when the previous snapshot isn't a prefix of the current one.
const delta = lastSent && full.startsWith(lastSent) ? full.slice(lastSent.length) : full;
if (delta) {
server.terminalSentData.set(ev.callId, full);
await sendSessionUpdate(cx, acpSid, {
sessionUpdate: "tool_call_update",
toolCallId: ev.callId,
_meta: { terminal_output: { terminal_id: ev.callId, data: delta } },
});

// ① terminal_output (pure data, append semantics).
// - progress (in_progress): zcode's stdoutTail is a CUMULATIVE snapshot of
// the tail, but terminal_output.data is APPEND semantics, so we diff and
// emit only the suffix beyond the last-sent snapshot.
// - result (completed/failed): the full output was already streamed during
// progress, and zcode's result re-sends the complete tail → skip, UNLESS
// no progress ever fired (short command: scheduled → result). In that
// case nothing was streamed yet, so emit the full output once.
const isResult = ev.status === "completed" || ev.status === "failed";
const alreadyStreamed = server.terminalSentData.has(ev.callId);
if (full && !(isResult && alreadyStreamed)) {
const delta = lastSent && full.startsWith(lastSent) ? full.slice(lastSent.length) : full;
if (delta) {
server.terminalSentData.set(ev.callId, full);
await sendSessionUpdate(cx, acpSid, {
sessionUpdate: "tool_call_update",
toolCallId: ev.callId,
_meta: { terminal_output: { terminal_id: ev.callId, data: delta } },
});
}
}
Comment on lines +175 to 187

@gemini-code-assist gemini-code-assist Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Correctness Bug: Final Output Truncation / Loss

By skipping the terminal_output notification entirely when isResult && alreadyStreamed is true, any new output appended to the stdout tail after the last progress event but before the command completed/failed will be completely lost.

Since terminal_exit no longer carries rawOutput (which is correct to prevent duplication in Zed), there is no other way for this final output delta to reach the client.

Solution

We do not need to skip terminal_output entirely. The existing delta/deduplication logic (const delta = lastSent && full.startsWith(lastSent) ? full.slice(lastSent.length) : full;) already perfectly handles deduplication:

  • If there is no new output, delta will be empty ("") and nothing will be sent.
  • If there is new output, only the new suffix will be sent.

We should remove the !(isResult && alreadyStreamed) guard and simply process the delta normally.

  const isResult = ev.status === "completed" || ev.status === "failed";
  if (full) {
    const delta = lastSent && full.startsWith(lastSent) ? full.slice(lastSent.length) : full;
    if (delta) {
      server.terminalSentData.set(ev.callId, full);
      await sendSessionUpdate(cx, acpSid, {
        sessionUpdate: "tool_call_update",
        toolCallId: ev.callId,
        _meta: { terminal_output: { terminal_id: ev.callId, data: delta } },
      });
    }
  }


// ② terminal_exit (terminal state) — only on completed/failed.
if (ev.status === "completed" || ev.status === "failed") {
// Deliberately omits rawOutput: the output was already streamed via
// terminal_output.data above, and Zed renders BOTH the terminal buffer and
// the rawOutput fallback — including rawOutput here duplicates every line.
if (isResult) {
const exitCode = extractExitCode(ev.rawResult, ev.status === "failed");
const exitUpdate: acp.SessionUpdate = {
sessionUpdate: "tool_call_update",
Expand All @@ -188,11 +202,6 @@ async function dispatchTerminalUpdate(
terminal_exit: { terminal_id: ev.callId, exit_code: exitCode, signal: null },
},
};
// rawOutput gives Zed a text fallback outside the terminal render path.
// This carries the FULL output (not the delta) so a non-terminal render
// fallback shows everything once; the terminal_output stream already
// appended it above via the dedup delta.
if (ev.output !== undefined) exitUpdate.rawOutput = ev.output;
await sendSessionUpdate(cx, acpSid, exitUpdate);
// Terminal session ended — clear the snapshot so a future tool reusing this
// callId (shouldn't happen, but defensively) starts fresh.
Expand Down
11 changes: 6 additions & 5 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,12 @@ export class ZcodeAcpServer {
/** Per-session model cache for configOptions model dropdown. */
readonly modelCache = new Map<string, string>();
/**
* Per-Bash-callId cumulative stdout snapshot already streamed via
* terminal_output. zcode's `stdoutTail` is a *snapshot* of the tail of the
* accumulated stdout, but ACP `terminal_output.data` is *append* semantics —
* so we must diff each new snapshot against the last-sent one and emit only
* the suffix to avoid replaying the whole output on every progress event.
* Per-Bash-callId stdout snapshot already streamed via terminal_output. Used
* by dispatchTerminalUpdate for two dedup guards:
* - progress: diff cumulative stdoutTail snapshots, emit only the suffix.
* - result: if present, the output was already streamed → skip replay.
* Presence of a key also signals "progress fired for this call" (absent =
* short command with no progress, so the result emits output once).
*/
readonly terminalSentData = new Map<string, string>();
/** Monotonic id counter; base 10_000_000 to avoid collisions with zcode-originated ids. */
Expand Down
45 changes: 33 additions & 12 deletions tests/dispatch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ describe("dispatchEvent", () => {
expect(sent[0]).toMatchObject({
_meta: { terminal_output: { terminal_id: "c1", data: "done" } },
});
// ② terminal_exit (terminal state)
// ② terminal_exit (terminal state) — no rawOutput: the output was already
// streamed via terminal_output, and including rawOutput makes Zed render
// it a second time (the duplication bug).
expect(sent[1]).toMatchObject({
sessionUpdate: "tool_call_update",
toolCallId: "c1",
Expand All @@ -157,13 +159,13 @@ describe("dispatchEvent", () => {
claudeCode: { toolName: "Bash" },
terminal_exit: { terminal_id: "c1", exit_code: 0, signal: null },
},
rawOutput: "done",
});
expect((sent[1] as { rawOutput?: string }).rawOutput).toBeUndefined();
});

it("Bash result after streamed progress emits only the unsent tail", async () => {
// Progress already streamed "line1\nline2\n"; the result carries the full
// "line1\nline2\nline3\n". Only "line3\n" should be appended.
it("Bash result after streamed progress skips terminal_output (no replay)", async () => {
// Progress already streamed the full output; zcode's result re-sends the
// complete tail. terminal_output must NOT fire again — only terminal_exit.
const { cx, sent } = mockContext();
const server = makeServer(true);
const base: Partial<Extract<InternalEvent, { kind: "ToolCallUpdate" }>> = {
Expand All @@ -175,7 +177,7 @@ describe("dispatchEvent", () => {
server,
cx,
SID,
{ ...base, status: "in_progress", rawOutput: "line1\nline2\n" },
{ ...base, status: "in_progress", rawOutput: "line1\nline2\nline3\n" },
CHUNK,
);
await dispatchEvent(
Expand All @@ -191,15 +193,34 @@ describe("dispatchEvent", () => {
},
CHUNK,
);
// progress (1) + terminal_output delta (1) + terminal_exit (1)
expect(sent).toHaveLength(3);
// progress terminal_output (1) + result terminal_exit (1). No second data.
expect(sent).toHaveLength(2);
expect(sent[0]).toMatchObject({
_meta: { terminal_output: { data: "line1\nline2\n" } },
_meta: { terminal_output: { data: "line1\nline2\nline3\n" } },
});
expect(sent[1]).toMatchObject({
_meta: { terminal_output: { data: "line3\n" } },
expect(sent[1]).toMatchObject({ status: "completed" });
});

it("Bash result with no prior progress emits full output once", async () => {
// Short command: scheduled → result, no progress event. Nothing was streamed
// yet, so the result must emit terminal_output once so output is visible.
const { cx, sent } = mockContext();
const server = makeServer(true);
const ev: InternalEvent = {
kind: "ToolCallUpdate",
callId: "c1",
tool: "Bash",
status: "completed",
rawOutput: "quick result\n",
output: "quick result\n",
rawResult: { success: true, content: "quick result\n", perf: { exitCode: 0 } },
};
await dispatchEvent(server, cx, SID, ev, CHUNK);
expect(sent).toHaveLength(2);
expect(sent[0]).toMatchObject({
_meta: { terminal_output: { data: "quick result\n" } },
});
expect(sent[2]).toMatchObject({ status: "completed" });
expect(sent[1]).toMatchObject({ status: "completed" });
});

it("Bash failed extracts exit code 1 when no perf.exitCode", async () => {
Expand Down
Loading