Skip to content

Commit 0e5c2a0

Browse files
committed
fix: remove partial agent messages on error in ACP conversation
Previously, when Send() failed, only empty agent messages were removed. If the agent had streamed partial content before the error, that partial message would incorrectly remain in the conversation. Now we remove the agent message on error regardless of whether it has content, ensuring the conversation state stays consistent.
1 parent 35f4f5f commit 0e5c2a0

2 files changed

Lines changed: 51 additions & 3 deletions

File tree

x/acpio/acp_conversation.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,9 +199,8 @@ func (c *ACPConversation) executePrompt(messageParts []st.MessagePart) {
199199

200200
if err != nil {
201201
c.logger.Error("ACPConversation message failed", "error", err)
202-
// Remove the empty streaming message on error
203-
if len(c.messages) > 0 && c.messages[len(c.messages)-1].Role == st.ConversationRoleAgent &&
204-
c.messages[len(c.messages)-1].Message == "" {
202+
// Remove the agent's streaming message on error (may be empty or partial)
203+
if len(c.messages) > 0 && c.messages[len(c.messages)-1].Role == st.ConversationRoleAgent {
205204
c.messages = c.messages[:len(c.messages)-1]
206205
}
207206
messages := slices.Clone(c.messages)

x/acpio/acp_conversation_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,3 +423,52 @@ func Test_Messages_AreCopied(t *testing.T) {
423423
// Unblock the write to let the test complete cleanly
424424
close(done)
425425
}
426+
427+
func Test_ErrorRemovesPartialMessage(t *testing.T) {
428+
mClock := quartz.NewMock(t)
429+
mock := newMockAgentIO()
430+
// Block the write so we can simulate partial content before error
431+
started, done := mock.BlockWrite()
432+
433+
conv := acpio.NewACPConversation(mock, nil, nil, nil, mClock)
434+
conv.Start(context.Background())
435+
436+
// Send a message
437+
err := conv.Send(screentracker.MessagePartText{Content: "test"})
438+
require.NoError(t, err)
439+
440+
// Wait for write to start
441+
<-started
442+
443+
// Should have user message + placeholder agent message
444+
messages := conv.Messages()
445+
require.Len(t, messages, 2)
446+
assert.Equal(t, screentracker.ConversationRoleUser, messages[0].Role)
447+
assert.Equal(t, screentracker.ConversationRoleAgent, messages[1].Role)
448+
449+
// Simulate the agent streaming partial content before the error
450+
mock.SimulateChunks("partial ", "response ", "content")
451+
452+
// Verify partial content is in the agent message
453+
messages = conv.Messages()
454+
require.Len(t, messages, 2)
455+
assert.Equal(t, "partial response content", messages[1].Message)
456+
457+
// Now configure the mock to return an error and unblock
458+
mock.mu.Lock()
459+
mock.writeErr = assert.AnError
460+
mock.mu.Unlock()
461+
close(done)
462+
463+
// Wait for the conversation to stabilize after the error
464+
require.Eventually(t, func() bool {
465+
return conv.Status() == screentracker.ConversationStatusStable
466+
}, 100*time.Millisecond, 5*time.Millisecond, "status should return to stable")
467+
468+
// The partial agent message should be removed on error.
469+
// Only the user message should remain.
470+
messages = conv.Messages()
471+
require.Len(t, messages, 1, "partial agent message should be removed on error")
472+
assert.Equal(t, screentracker.ConversationRoleUser, messages[0].Role)
473+
assert.Equal(t, "test", messages[0].Message)
474+
}

0 commit comments

Comments
 (0)