Skip to content

Commit e294ff9

Browse files
authored
fix: omit tool-call narration from review output (#475)
## Summary - drop streamed assistant narration that occurs before tool calls from persisted review output - apply the same trailing-text rule across codex, claude, gemini, and opencode review parsers - add anonymized regression tests based on recent log shapes so tool-call chatter does not leak into reviews ## Testing - go fmt ./... - go vet ./... - go test ./...
1 parent 6ff0f2c commit e294ff9

14 files changed

Lines changed: 223 additions & 51 deletions

internal/agent/claude.go

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,9 @@ type claudeStreamMessage struct {
211211
} `json:"error,omitempty"`
212212
}
213213

214-
// extractContentText extracts text from a Claude message content field.
215-
// Content can be a plain string or an array of content blocks
216-
// (e.g. [{"type":"text","text":"..."}]).
214+
// extractContentText extracts only the text that appears after the last tool-use
215+
// block in a Claude message content field. Content can be a plain string or an
216+
// array of content blocks (e.g. [{"type":"text","text":"..."}]).
217217
func extractContentText(raw json.RawMessage) string {
218218
if len(raw) == 0 {
219219
return ""
@@ -229,13 +229,16 @@ func extractContentText(raw json.RawMessage) string {
229229
Text string `json:"text"`
230230
}
231231
if json.Unmarshal(raw, &blocks) == nil {
232-
var texts []string
232+
texts := newTrailingReviewText()
233233
for _, b := range blocks {
234-
if b.Type == "text" && b.Text != "" {
235-
texts = append(texts, b.Text)
234+
switch b.Type {
235+
case "text":
236+
texts.Add(b.Text)
237+
case "tool_use", "tool_result":
238+
texts.ResetAfterTool()
236239
}
237240
}
238-
return strings.Join(texts, "\n")
241+
return texts.Join("\n")
239242
}
240243
return ""
241244
}
@@ -248,7 +251,7 @@ func parseStreamJSON(r io.Reader, output io.Writer) (string, error) {
248251
br := bufio.NewReader(r)
249252

250253
var lastResult string
251-
var assistantMessages []string
254+
assistantMessages := newTrailingReviewText()
252255
var errorMessages []string
253256
var validEventsParsed bool
254257

@@ -274,9 +277,14 @@ func parseStreamJSON(r io.Reader, output io.Writer) (string, error) {
274277
// Content can be a string or an array of content blocks.
275278
if msg.Type == "assistant" {
276279
if text := extractContentText(msg.Message.Content); text != "" {
277-
assistantMessages = append(assistantMessages, text)
280+
assistantMessages.Add(text)
278281
}
279282
}
283+
if msg.Type == "tool_use" || msg.Type == "tool_result" {
284+
// Only the trailing post-tool assistant segment is treated
285+
// as the review body.
286+
assistantMessages.ResetAfterTool()
287+
}
280288

281289
// The final result message contains the summary.
282290
// When is_error is set, the result event signals a
@@ -315,7 +323,7 @@ func parseStreamJSON(r io.Reader, output io.Writer) (string, error) {
315323
}
316324

317325
// Build partial output for error context
318-
partial := strings.Join(assistantMessages, "\n")
326+
partial := assistantMessages.Join("\n")
319327

320328
// If error events were received but we got no result, report them with any partial output
321329
if len(errorMessages) > 0 && lastResult == "" {
@@ -326,8 +334,8 @@ func parseStreamJSON(r io.Reader, output io.Writer) (string, error) {
326334
if lastResult != "" {
327335
return lastResult, nil
328336
}
329-
if len(assistantMessages) > 0 {
330-
return strings.Join(assistantMessages, "\n"), nil
337+
if result := assistantMessages.Join("\n"); result != "" {
338+
return result, nil
331339
}
332340

333341
return "", nil

internal/agent/claude_test.go

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,26 @@ still not json
193193
`,
194194
expectedResult: "First block\nSecond block",
195195
},
196+
{
197+
name: "AssistantFallbackDropsNarrationBeforeToolUse",
198+
input: `{"type":"system","subtype":"init"}
199+
{"type":"assistant","message":{"content":"I am checking the relevant files first."}}
200+
{"type":"tool_use","name":"Read","input":{"file_path":"redacted.go"}}
201+
{"type":"tool_result","content":"redacted"}
202+
{"type":"assistant","message":{"content":"## Review Findings\n- **Severity**: Low; **Problem**: Final finding."}}
203+
`,
204+
expectedResult: "## Review Findings\n- **Severity**: Low; **Problem**: Final finding.",
205+
},
206+
{
207+
name: "AssistantFallbackPrefersFinalPostToolSegment",
208+
input: `{"type":"system","subtype":"init"}
209+
{"type":"assistant","message":{"content":"## Review Findings\n- **Severity**: Low; **Problem**: Earlier provisional finding."}}
210+
{"type":"tool_use","name":"Read","input":{"file_path":"redacted.go"}}
211+
{"type":"tool_result","content":"redacted"}
212+
{"type":"assistant","message":{"content":"## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding."}}
213+
`,
214+
expectedResult: "## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding.",
215+
},
196216
{
197217
name: "ContentBlockArrayWithToolUse",
198218
input: `{"type":"system","subtype":"init"}
@@ -280,7 +300,7 @@ func TestExtractContentText(t *testing.T) {
280300
{
281301
name: "MixedBlockTypes",
282302
raw: `[{"type":"text","text":"Analysis"},{"type":"tool_use","name":"Read"},{"type":"text","text":"Done"}]`,
283-
want: "Analysis\nDone",
303+
want: "Done",
284304
},
285305
{
286306
name: "EmptyArray",

internal/agent/codex.go

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,20 @@ func isCodexEventType(eventType string) bool {
268268
strings.HasPrefix(eventType, "item.")
269269
}
270270

271+
func isCodexToolEvent(ev codexEvent) bool {
272+
if ev.Type != "item.started" &&
273+
ev.Type != "item.updated" &&
274+
ev.Type != "item.completed" {
275+
return false
276+
}
277+
switch ev.Item.Type {
278+
case "command_execution", "file_change":
279+
return true
280+
default:
281+
return false
282+
}
283+
}
284+
271285
func codexFailureEventError(ev codexEvent) error {
272286
switch ev.Type {
273287
case "turn.failed":
@@ -298,8 +312,7 @@ func (a *CodexAgent) parseStreamJSON(r io.Reader, sw *syncWriter) (string, error
298312
br := bufio.NewReader(r)
299313

300314
var validEventsParsed bool
301-
var agentMessages []string
302-
messageIndexByID := make(map[string]int)
315+
agentMessages := newTrailingReviewText()
303316
var streamFailure error
304317

305318
for {
@@ -324,19 +337,18 @@ func (a *CodexAgent) parseStreamJSON(r io.Reader, sw *syncWriter) (string, error
324337
streamFailure = codexFailureEventError(ev)
325338
}
326339

340+
if isCodexToolEvent(ev) {
341+
// The persisted review is defined as the assistant text
342+
// after the last tool event in the stream.
343+
agentMessages.ResetAfterTool()
344+
}
345+
327346
// Collect agent_message text from completed/updated items.
328347
// For messages with IDs, keep only the latest text per ID to avoid duplicates
329348
// from incremental updates while preserving first-seen order.
330349
if (ev.Type == "item.completed" || ev.Type == "item.updated") &&
331350
ev.Item.Type == "agent_message" && ev.Item.Text != "" {
332-
if ev.Item.ID == "" {
333-
agentMessages = append(agentMessages, ev.Item.Text)
334-
} else if idx, ok := messageIndexByID[ev.Item.ID]; ok {
335-
agentMessages[idx] = ev.Item.Text
336-
} else {
337-
messageIndexByID[ev.Item.ID] = len(agentMessages)
338-
agentMessages = append(agentMessages, ev.Item.Text)
339-
}
351+
agentMessages.AddWithID(ev.Item.ID, ev.Item.Text)
340352
}
341353
}
342354
}
@@ -355,8 +367,8 @@ func (a *CodexAgent) parseStreamJSON(r io.Reader, sw *syncWriter) (string, error
355367
return "", streamFailure
356368
}
357369

358-
if len(agentMessages) > 0 {
359-
return strings.Join(agentMessages, "\n"), nil
370+
if result := agentMessages.Join("\n"); result != "" {
371+
return result, nil
360372
}
361373

362374
return "", nil

internal/agent/codex_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,32 @@ func TestCodexParseStreamJSON(t *testing.T) {
179179
),
180180
want: "Done reviewing.",
181181
},
182+
{
183+
name: "DropsNarrationBeforeToolCalls",
184+
input: buildStream(
185+
jsonThreadStarted,
186+
jsonTurnStarted,
187+
`{"type":"item.completed","item":{"id":"msg1","type":"agent_message","text":"Checking the relevant files before I write the review."}}`,
188+
`{"type":"item.started","item":{"id":"cmd1","type":"command_execution","command":"sh -lc sed ..."}}`,
189+
`{"type":"item.completed","item":{"id":"cmd1","type":"command_execution","command":"sh -lc sed ...","exit_code":0}}`,
190+
`{"type":"item.completed","item":{"id":"msg2","type":"agent_message","text":"## Review Findings\n- **Severity**: Low; **Problem**: Final finding."}}`,
191+
jsonTurnCompleted,
192+
),
193+
want: "## Review Findings\n- **Severity**: Low; **Problem**: Final finding.",
194+
},
195+
{
196+
name: "PrefersFinalPostToolSegment",
197+
input: buildStream(
198+
jsonThreadStarted,
199+
jsonTurnStarted,
200+
`{"type":"item.completed","item":{"id":"msg1","type":"agent_message","text":"## Review Findings\n- **Severity**: Low; **Problem**: Earlier provisional finding."}}`,
201+
`{"type":"item.started","item":{"id":"cmd1","type":"command_execution","command":"sh -lc rg ..."}}`,
202+
`{"type":"item.completed","item":{"id":"cmd1","type":"command_execution","command":"sh -lc rg ...","exit_code":0}}`,
203+
`{"type":"item.completed","item":{"id":"msg2","type":"agent_message","text":"## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding."}}`,
204+
jsonTurnCompleted,
205+
),
206+
want: "## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding.",
207+
},
182208
{
183209
name: "StreamsToWriter",
184210
input: buildStream(`{"type":"item.completed","item":{"type":"agent_message","text":"hello"}}`),

internal/agent/gemini.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func (a *GeminiAgent) parseStreamJSON(r io.Reader, sw *syncWriter) (parseResult,
193193
br := bufio.NewReader(r)
194194

195195
var lastResult string
196-
var assistantMessages []string
196+
assistantMessages := newTrailingReviewText()
197197
var validEventsParsed bool
198198

199199
for {
@@ -217,11 +217,16 @@ func (a *GeminiAgent) parseStreamJSON(r io.Reader, sw *syncWriter) (parseResult,
217217
// Collect assistant messages for the result
218218
// Gemini format: type="message", role="assistant", content at top level
219219
if msg.Type == "message" && msg.Role == "assistant" && msg.Content != "" {
220-
assistantMessages = append(assistantMessages, msg.Content)
220+
assistantMessages.Add(msg.Content)
221221
}
222222
// Claude Code format: type="assistant", message.content nested
223223
if msg.Type == "assistant" && msg.Message.Content != "" {
224-
assistantMessages = append(assistantMessages, msg.Message.Content)
224+
assistantMessages.Add(msg.Message.Content)
225+
}
226+
if msg.Type == "tool" || msg.Type == "tool_result" {
227+
// Treat pre-tool assistant text as provisional; only the
228+
// trailing post-tool segment becomes review output.
229+
assistantMessages.ResetAfterTool()
225230
}
226231

227232
// The final result message contains the summary
@@ -245,8 +250,8 @@ func (a *GeminiAgent) parseStreamJSON(r io.Reader, sw *syncWriter) (parseResult,
245250
if lastResult != "" {
246251
return parseResult{result: lastResult}, nil
247252
}
248-
if len(assistantMessages) > 0 {
249-
return parseResult{result: strings.Join(assistantMessages, "\n")}, nil
253+
if result := assistantMessages.Join("\n"); result != "" {
254+
return parseResult{result: result}, nil
250255
}
251256

252257
return parseResult{}, nil

internal/agent/gemini_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,34 @@ func TestGeminiParseStreamJSON(t *testing.T) {
166166
}
167167
},
168168
},
169+
{
170+
name: "AssistantFallbackDropsNarrationBeforeToolUse",
171+
input: `{"type":"system","subtype":"init"}
172+
{"type":"assistant","message":{"content":"I am checking the relevant files first."}}
173+
{"type":"tool","name":"Read"}
174+
{"type":"assistant","message":{"content":"## Review Findings\n- **Severity**: Low; **Problem**: Final finding."}}
175+
`,
176+
assertResult: func(t *testing.T, res string) {
177+
want := "## Review Findings\n- **Severity**: Low; **Problem**: Final finding."
178+
if res != want {
179+
t.Errorf("expected result %q, got %q", want, res)
180+
}
181+
},
182+
},
183+
{
184+
name: "AssistantFallbackPrefersFinalPostToolSegment",
185+
input: `{"type":"system","subtype":"init"}
186+
{"type":"assistant","message":{"content":"## Review Findings\n- **Severity**: Low; **Problem**: Earlier provisional finding."}}
187+
{"type":"tool","name":"Read"}
188+
{"type":"assistant","message":{"content":"## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding."}}
189+
`,
190+
assertResult: func(t *testing.T, res string) {
191+
want := "## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding."
192+
if res != want {
193+
t.Errorf("expected result %q, got %q", want, res)
194+
}
195+
},
196+
},
169197
{
170198
name: "NoValidEvents",
171199
input: `not json at all

internal/agent/opencode.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,12 @@ func parseOpenCodeJSON(
172172
) (string, error) {
173173
br := bufio.NewReader(r)
174174

175-
var textParts []string
175+
textParts := newTrailingReviewText()
176176

177177
for {
178178
line, err := br.ReadString('\n')
179179
if err != nil && err != io.EOF {
180-
return strings.Join(textParts, ""), fmt.Errorf(
180+
return textParts.Join(""), fmt.Errorf(
181181
"read stream: %w", err,
182182
)
183183
}
@@ -193,11 +193,13 @@ func parseOpenCodeJSON(
193193
ev.Part != nil {
194194
var part opencodePart
195195
if json.Unmarshal(ev.Part, &part) == nil &&
196-
part.Type == "text" && part.Text != "" {
197-
textParts = append(
198-
textParts,
199-
stripTerminalControls(part.Text),
200-
)
196+
part.Type != "" {
197+
if part.Type == "tool" {
198+
textParts.ResetAfterTool()
199+
}
200+
if part.Type == "text" && part.Text != "" {
201+
textParts.Add(stripTerminalControls(part.Text))
202+
}
201203
}
202204
}
203205
}
@@ -207,7 +209,7 @@ func parseOpenCodeJSON(
207209
}
208210
}
209211

210-
return strings.Join(textParts, ""), nil
212+
return textParts.Join(""), nil
211213
}
212214

213215
// ansiPattern matches ANSI CSI and OSC escape sequences.

internal/agent/opencode_test.go

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -131,13 +131,30 @@ func TestOpenCodeReviewParsesJSONStream(t *testing.T) {
131131

132132
result, _, _ := runMockOpenCodeReview(t, "", "prompt", stdoutLines)
133133

134-
assertContains(t, result, "**Review:** Fix the typo.")
135-
assertContains(t, result, " Done.")
134+
assertEqual(t, result, " Done.")
136135
// Tool events should not appear in the result text
137136
assertNotContains(t, result, "Read")
138137
assertNotContains(t, result, "file_path")
139138
}
140139

140+
func TestOpenCodeReviewPrefersFinalPostToolSegment(t *testing.T) {
141+
t.Parallel()
142+
skipIfWindows(t)
143+
144+
stdoutLines := []string{
145+
makeTextEvent("## Review Findings\n- **Severity**: Low; **Problem**: Earlier provisional finding."),
146+
makeOpenCodeEvent("tool", map[string]any{
147+
"type": "tool",
148+
"tool": "Read",
149+
}),
150+
makeTextEvent("## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding."),
151+
}
152+
153+
result, _, _ := runMockOpenCodeReview(t, "", "prompt", stdoutLines)
154+
155+
assertEqual(t, result, "## Review Findings\n- **Severity**: Medium; **Problem**: Final persisted finding.")
156+
}
157+
141158
func TestOpenCodeReviewStreamsToOutput(t *testing.T) {
142159
t.Parallel()
143160
skipIfWindows(t)
@@ -214,8 +231,8 @@ func TestParseOpenCodeJSON(t *testing.T) {
214231

215232
lines := strings.Join([]string{
216233
makeTextEvent("Part one."),
217-
makeOpenCodeEvent("reasoning", map[string]any{
218-
"type": "reasoning", "text": "thinking...",
234+
makeOpenCodeEvent("tool", map[string]any{
235+
"type": "tool", "tool": "Read",
219236
}),
220237
makeTextEvent(" Part two."),
221238
}, "\n") + "\n"
@@ -228,7 +245,7 @@ func TestParseOpenCodeJSON(t *testing.T) {
228245
t.Fatalf("parseOpenCodeJSON: %v", err)
229246
}
230247

231-
assertEqual(t, result, "Part one. Part two.")
248+
assertEqual(t, result, " Part two.")
232249

233250
// All raw lines should be written to output
234251
out := outputBuf.String()

0 commit comments

Comments
 (0)