Skip to content

Commit 69febb2

Browse files
axiomofjoyclaude
andcommitted
fix(pxi): stop re-flushing tool outputs the server already holds
The eager flush re-posted client tool outputs the server had already persisted — on a seeded transcript every resolved output is durable by definition, yet the flush sent them again on the next evaluation. Track the IDs the server holds and skip them. The set is fed only from sound sources: the transcript the server sent, and the persistedToolOutputIds the acknowledgement itself names. Reading them off the client's live copy of a message would mark outputs the server never received, and a wrongly marked output is never flushed again. This is a pre-existing defect, independent of mutation approval; it is stacked on that branch only because it was found while building it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9ac7b75 commit 69febb2

12 files changed

Lines changed: 424 additions & 10 deletions

File tree

js/app/src/agent/chat/__tests__/toolOutputFlush.test.ts

Lines changed: 136 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { createClientToolTimingRecorder } from "@phoenix/agent/chat/clientToolTimings";
22
import { flushToolOutputs } from "@phoenix/agent/chat/toolOutputFlush";
3+
import { createTranscriptPersistenceCoordinator } from "@phoenix/agent/chat/transcriptPersistence";
34
import type { AgentUIMessage } from "@phoenix/agent/chat/types";
45

56
const FLUSH_URL = "/v1/agent_sessions/session-1/tool_outputs";
@@ -74,8 +75,9 @@ describe("flushToolOutputs", () => {
7475
expect(body.toolOutputs[0].toolCallId).toBe("call-1");
7576
});
7677

77-
it("re-posts every resolved output on each call; the endpoint dedupes", async () => {
78+
it("skips outputs marked synced and marks flushed outputs in place", async () => {
7879
const fetchMock = vi.fn().mockResolvedValue(okResponse());
80+
const syncedToolOutputIds = new Set<string>();
7981

8082
flushToolOutputs({
8183
message: assistantMessage([
@@ -84,6 +86,7 @@ describe("flushToolOutputs", () => {
8486
]),
8587
flushUrl: FLUSH_URL,
8688
fetch: fetchMock,
89+
syncedToolOutputIds,
8790
});
8891
flushToolOutputs({
8992
message: assistantMessage([
@@ -93,6 +96,7 @@ describe("flushToolOutputs", () => {
9396
]),
9497
flushUrl: FLUSH_URL,
9598
fetch: fetchMock,
99+
syncedToolOutputIds,
96100
});
97101
await settle();
98102

@@ -102,7 +106,137 @@ describe("flushToolOutputs", () => {
102106
secondBody.toolOutputs.map(
103107
(toolOutput: { toolCallId: string }) => toolOutput.toolCallId
104108
)
105-
).toEqual(["call-1", "call-2"]);
109+
).toEqual(["call-2"]);
110+
expect(syncedToolOutputIds).toEqual(new Set(["call-1", "call-2"]));
111+
});
112+
113+
it("does not post at all when every resolved output is already synced", async () => {
114+
const fetchMock = vi.fn().mockResolvedValue(okResponse());
115+
116+
flushToolOutputs({
117+
message: assistantMessage([
118+
resolvedToolPart("call-1"),
119+
pendingToolPart("call-2"),
120+
]),
121+
flushUrl: FLUSH_URL,
122+
fetch: fetchMock,
123+
syncedToolOutputIds: new Set(["call-1"]),
124+
});
125+
await settle();
126+
127+
expect(fetchMock).not.toHaveBeenCalled();
128+
});
129+
130+
it("unmarks flushed outputs when the post fails so a retry can re-flush", async () => {
131+
const fetchMock = vi
132+
.fn()
133+
.mockRejectedValueOnce(new Error("offline"))
134+
.mockResolvedValue(okResponse());
135+
const syncedToolOutputIds = new Set<string>();
136+
const message = assistantMessage([
137+
resolvedToolPart("call-1"),
138+
pendingToolPart("call-2"),
139+
]);
140+
141+
flushToolOutputs({
142+
message,
143+
flushUrl: FLUSH_URL,
144+
fetch: fetchMock,
145+
syncedToolOutputIds,
146+
});
147+
await settle();
148+
expect(syncedToolOutputIds.size).toBe(0);
149+
150+
flushToolOutputs({
151+
message,
152+
flushUrl: FLUSH_URL,
153+
fetch: fetchMock,
154+
syncedToolOutputIds,
155+
});
156+
await settle();
157+
158+
expect(fetchMock).toHaveBeenCalledTimes(2);
159+
expect(syncedToolOutputIds).toEqual(new Set(["call-1"]));
160+
});
161+
162+
it("unmarks flushed outputs on a non-2xx response", async () => {
163+
const fetchMock = vi.fn().mockResolvedValue({ ok: false } as Response);
164+
const syncedToolOutputIds = new Set<string>();
165+
166+
flushToolOutputs({
167+
message: assistantMessage([
168+
resolvedToolPart("call-1"),
169+
pendingToolPart("call-2"),
170+
]),
171+
flushUrl: FLUSH_URL,
172+
fetch: fetchMock,
173+
syncedToolOutputIds,
174+
});
175+
await settle();
176+
177+
expect(fetchMock).toHaveBeenCalledTimes(1);
178+
expect(syncedToolOutputIds.size).toBe(0);
179+
});
180+
181+
it("still retries after a failed flush when the ack does not name the output", async () => {
182+
// A failed flush unmarks its IDs so a later flush can retry. The
183+
// transcript-persisted ack used to re-mark them from the client's own copy
184+
// of the message, cancelling that retry for an output the server never
185+
// received. The ack now names only what it wrote, so an output it does not
186+
// name stays flushable.
187+
const fetchMock = vi
188+
.fn()
189+
.mockResolvedValueOnce({ ok: false, status: 500 } as Response)
190+
.mockResolvedValue(okResponse());
191+
const coordinator = createTranscriptPersistenceCoordinator();
192+
const message = assistantMessage([
193+
resolvedToolPart("call-1"),
194+
pendingToolPart("call-2"),
195+
]);
196+
197+
flushToolOutputs({
198+
message,
199+
flushUrl: FLUSH_URL,
200+
fetch: fetchMock,
201+
syncedToolOutputIds: coordinator.syncedToolOutputIds,
202+
});
203+
await settle();
204+
expect(coordinator.syncedToolOutputIds.size).toBe(0);
205+
206+
// The turn persisted without call-1's output, so the ack names nothing.
207+
// Its output resolved client-side after the server took its snapshot.
208+
coordinator.acknowledge({ messageId: "assistant-1" });
209+
coordinator.markToolOutputsPersisted([]);
210+
211+
flushToolOutputs({
212+
message,
213+
flushUrl: FLUSH_URL,
214+
fetch: fetchMock,
215+
syncedToolOutputIds: coordinator.syncedToolOutputIds,
216+
});
217+
await settle();
218+
219+
expect(fetchMock).toHaveBeenCalledTimes(2);
220+
expect(coordinator.syncedToolOutputIds).toEqual(new Set(["call-1"]));
221+
});
222+
223+
it("skips an output the ack named as persisted", async () => {
224+
const fetchMock = vi.fn().mockResolvedValue(okResponse());
225+
const coordinator = createTranscriptPersistenceCoordinator();
226+
227+
coordinator.markToolOutputsPersisted(["call-1"]);
228+
flushToolOutputs({
229+
message: assistantMessage([
230+
resolvedToolPart("call-1"),
231+
pendingToolPart("call-2"),
232+
]),
233+
flushUrl: FLUSH_URL,
234+
fetch: fetchMock,
235+
syncedToolOutputIds: coordinator.syncedToolOutputIds,
236+
});
237+
await settle();
238+
239+
expect(fetchMock).not.toHaveBeenCalled();
106240
});
107241

108242
it("does not flush when every tool call has resolved", async () => {

js/app/src/agent/chat/__tests__/transcriptPersistence.test.ts

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import { describe, expect, it } from "vitest";
22

3-
import { createTranscriptPersistenceCoordinator } from "../transcriptPersistence";
3+
import {
4+
createTranscriptPersistenceCoordinator,
5+
resolvedClientToolOutputIds,
6+
} from "../transcriptPersistence";
7+
import type { AgentUIMessage } from "../types";
48

59
describe("createTranscriptPersistenceCoordinator", () => {
610
it("waits for the matching assistant message to be persisted", async () => {
@@ -66,4 +70,78 @@ describe("createTranscriptPersistenceCoordinator", () => {
6670
coordinator.waitForMessage({ messageId: "assistant-1" })
6771
).resolves.toBe(true);
6872
});
73+
74+
it("marks only the tool outputs it is given", () => {
75+
const coordinator = createTranscriptPersistenceCoordinator();
76+
77+
coordinator.markToolOutputsPersisted(["tool-call-a", "tool-call-b"]);
78+
79+
expect(coordinator.syncedToolOutputIds).toEqual(
80+
new Set(["tool-call-a", "tool-call-b"])
81+
);
82+
});
83+
84+
it("marks nothing when the acknowledgement names no outputs", () => {
85+
const coordinator = createTranscriptPersistenceCoordinator();
86+
87+
// An older server, or a turn that persisted no client outputs. Either way
88+
// the flush must stay free to send them.
89+
coordinator.markToolOutputsPersisted(undefined);
90+
coordinator.markToolOutputsPersisted([]);
91+
92+
expect(coordinator.syncedToolOutputIds.size).toBe(0);
93+
});
94+
});
95+
96+
describe("resolvedClientToolOutputIds", () => {
97+
it("picks resolved client tool outputs off a server-provided message", () => {
98+
const message = {
99+
id: "assistant-1",
100+
role: "assistant",
101+
parts: [
102+
{
103+
type: "tool-edit_prompt",
104+
toolCallId: "tool-call-resolved",
105+
state: "output-available",
106+
input: {},
107+
output: { applied: true },
108+
callProviderMetadata: {
109+
phoenix: { toolExecutionEnvironment: "client" },
110+
},
111+
},
112+
{
113+
type: "tool-edit_prompt",
114+
toolCallId: "tool-call-pending",
115+
state: "input-available",
116+
input: {},
117+
callProviderMetadata: {
118+
phoenix: { toolExecutionEnvironment: "client" },
119+
},
120+
},
121+
{
122+
type: "tool-bash",
123+
toolCallId: "tool-call-server",
124+
state: "output-available",
125+
input: {},
126+
output: "done",
127+
},
128+
],
129+
} as AgentUIMessage;
130+
131+
// Server-executed and still-pending calls are not the client's to flush.
132+
expect(resolvedClientToolOutputIds(message)).toEqual([
133+
"tool-call-resolved",
134+
]);
135+
});
136+
137+
it("ignores non-assistant and missing messages", () => {
138+
expect(resolvedClientToolOutputIds(undefined)).toEqual([]);
139+
expect(
140+
resolvedClientToolOutputIds({
141+
id: "user-1",
142+
role: "user",
143+
parts: [{ type: "text", text: "hello" }],
144+
} as AgentUIMessage)
145+
).toEqual([]);
146+
});
69147
});

js/app/src/agent/chat/createAgentSessionChat.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ import {
1818
} from "@phoenix/agent/chat/rehydratePendingToolCalls";
1919
import { shouldSendAutomaticallyAfterToolOutput } from "@phoenix/agent/chat/shouldSendAutomatically";
2020
import { flushToolOutputs } from "@phoenix/agent/chat/toolOutputFlush";
21-
import { createTranscriptPersistenceCoordinator } from "@phoenix/agent/chat/transcriptPersistence";
21+
import {
22+
createTranscriptPersistenceCoordinator,
23+
resolvedClientToolOutputIds,
24+
} from "@phoenix/agent/chat/transcriptPersistence";
2225
import { createTurnCompletionGate } from "@phoenix/agent/chat/turnCompletion";
2326
import type {
2427
AgentUIMessage,
@@ -111,6 +114,11 @@ export function createAgentSessionChat({
111114
// racing its own in-flight change.
112115
let lastAssertedModelSelection: AgentModelSelection | null = null;
113116
const transcriptPersistence = createTranscriptPersistenceCoordinator();
117+
seedMessages.forEach((message) =>
118+
transcriptPersistence.markToolOutputsPersisted(
119+
resolvedClientToolOutputIds(message)
120+
)
121+
);
114122
const turnCompletionGate = createTurnCompletionGate({
115123
getShouldSendAutomatically: (messages) =>
116124
shouldSendAutomaticallyAfterToolOutput({
@@ -238,6 +246,12 @@ export function createAgentSessionChat({
238246
});
239247
} else if (dataPart.type === "data-transcript-persisted") {
240248
transcriptPersistence.acknowledge(dataPart.data);
249+
// The server names what it wrote. Deriving this from the local copy of
250+
// the message instead would mark outputs that resolved after the
251+
// server's snapshot, suppressing their flush forever.
252+
transcriptPersistence.markToolOutputsPersisted(
253+
dataPart.data.persistedToolOutputIds
254+
);
241255
}
242256
},
243257
sendAutomaticallyWhen: async ({ messages }) => {
@@ -253,6 +267,7 @@ export function createAgentSessionChat({
253267
toolTimings,
254268
locallyInterruptedToolCallIds:
255269
store.getState().locallyInterruptedToolCallIds,
270+
syncedToolOutputIds: transcriptPersistence.syncedToolOutputIds,
256271
});
257272
}
258273
return false;

js/app/src/agent/chat/toolOutputFlush.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export function flushToolOutputs({
2121
fetch: fetchFn,
2222
toolTimings = null,
2323
locallyInterruptedToolCallIds = {},
24+
syncedToolOutputIds = null,
2425
}: {
2526
/** The transcript's trailing assistant message. */
2627
message: AgentUIMessage;
@@ -32,6 +33,8 @@ export function flushToolOutputs({
3233
toolTimings?: ClientToolTimingRecorder | null;
3334
/** Tool calls this client resolved as interrupted; suppresses the flush. */
3435
locallyInterruptedToolCallIds?: LocallyInterruptedToolCallIds;
36+
/** Tool-call IDs the server already holds; skipped, and updated in place. */
37+
syncedToolOutputIds?: Set<string> | null;
3538
}): void {
3639
const enrichedMessage = enrichMessageWithClientToolMetadata({
3740
message,
@@ -41,19 +44,35 @@ export function flushToolOutputs({
4144
const toolOutputs = getFlushableClientToolOutputs({
4245
message: enrichedMessage,
4346
locallyInterruptedToolCallIds,
44-
});
47+
}).filter((part) => !syncedToolOutputIds?.has(part.toolCallId));
4548
if (toolOutputs.length === 0) {
4649
return;
4750
}
4851
const body: SubmitToolOutputsRequestBody = {
4952
toolOutputs,
5053
lastMessageId: message.id,
5154
};
55+
// Marked before the response lands so an overlapping evaluation (each
56+
// approval response re-runs sendAutomaticallyWhen) doesn't double-post.
57+
const flushedToolCallIds = toolOutputs.map((part) => part.toolCallId);
58+
for (const toolCallId of flushedToolCallIds) {
59+
syncedToolOutputIds?.add(toolCallId);
60+
}
5261
void fetchFn(flushUrl, {
5362
method: "POST",
5463
headers: { "Content-Type": "application/json" },
5564
body: JSON.stringify(body),
56-
}).catch(() => {
57-
// Benign: the chat continuation re-carries resolved outputs.
58-
});
65+
})
66+
.then((response) => {
67+
if (!response.ok) {
68+
throw new Error(`Tool output flush failed: ${response.status}`);
69+
}
70+
})
71+
.catch(() => {
72+
// Benign: the chat continuation re-carries resolved outputs. Unmark so
73+
// a later flush may retry before that continuation happens.
74+
for (const toolCallId of flushedToolCallIds) {
75+
syncedToolOutputIds?.delete(toolCallId);
76+
}
77+
});
5978
}

0 commit comments

Comments
 (0)