fix(agent): drop rawOutput for non-MCP tool results#2626
Open
K3N4Y wants to merge 3 commits into
Open
Conversation
Standard tool results (Read, Bash, ...) render from `content`, so also copying the full result into `rawOutput` duplicated large payloads on their way to the renderer. Only MCP tools consume rawOutput (App bridge replay), so emit it solely when an MCP tool result is present.
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/agent/src/adapters/claude/conversion/sdk-to-acp.test.ts:310-341
**Prefer parameterised tests here**
The two `it(...)` blocks share identical scaffolding — `registerToolUse`, clearing `updates`, calling `handleUserAssistantMessage`, and calling `toolUpdate` — differing only in the tool name, the presence of `tool_use_result`, and the expected `rawOutput` shape. The team's coding standard asks for parameterised tests wherever the same logical flow is exercised with varied inputs. An `it.each` table that passes `toolName`, `toolUseResult`, and `expectedRawOutput` (nullable) would express this once, making future cases (e.g. a third tool type) a single table row rather than a copied block.
Reviews (1): Last reviewed commit: "fix(agent): drop rawOutput for non-MCP t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+310
to
+341
| await registerToolUse(context, "tool_read", "Read", { | ||
| file_path: "/test/a.ts", | ||
| }); | ||
| updates.length = 0; | ||
| await handleUserAssistantMessage( | ||
| userToolResult("tool_read", "line one\nline two"), | ||
| context, | ||
| ); | ||
| const update = toolUpdate(updates); | ||
| expect(update?.status).toBe("completed"); | ||
| expect(update && "rawOutput" in update).toBe(false); | ||
| }); | ||
|
|
||
| it("forwards rawOutput for MCP tool results consumed by the App bridge", async () => { | ||
| const { context, updates } = createHandlerContext(); | ||
| await registerToolUse(context, "tool_mcp", "mcp__srv__do"); | ||
| updates.length = 0; | ||
| await handleUserAssistantMessage( | ||
| userToolResult("tool_mcp", "ignored", { | ||
| content: [{ type: "text", text: "mcp payload" }], | ||
| structuredContent: { ok: true }, | ||
| }), | ||
| context, | ||
| ); | ||
| const update = toolUpdate(updates); | ||
| expect(update?.status).toBe("completed"); | ||
| expect(update?.rawOutput).toEqual({ | ||
| content: [{ type: "text", text: "mcp payload" }], | ||
| structuredContent: { ok: true }, | ||
| isError: false, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
The two it(...) blocks share identical scaffolding — registerToolUse, clearing updates, calling handleUserAssistantMessage, and calling toolUpdate — differing only in the tool name, the presence of tool_use_result, and the expected rawOutput shape. The team's coding standard asks for parameterised tests wherever the same logical flow is exercised with varied inputs. An it.each table that passes toolName, toolUseResult, and expectedRawOutput (nullable) would express this once, making future cases (e.g. a third tool type) a single table row rather than a copied block.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/conversion/sdk-to-acp.test.ts
Line: 310-341
Comment:
**Prefer parameterised tests here**
The two `it(...)` blocks share identical scaffolding — `registerToolUse`, clearing `updates`, calling `handleUserAssistantMessage`, and calling `toolUpdate` — differing only in the tool name, the presence of `tool_use_result`, and the expected `rawOutput` shape. The team's coding standard asks for parameterised tests wherever the same logical flow is exercised with varied inputs. An `it.each` table that passes `toolName`, `toolUseResult`, and `expectedRawOutput` (nullable) would express this once, making future cases (e.g. a third tool type) a single table row rather than a copied block.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Standard tool results (Read, Bash, ...) render from
content, so also copying the full result intorawOutputduplicated large payloads on their way to the renderer. Only MCP tools consume rawOutput (App bridge replay), so emit it solely when an MCP tool result is present.Problem
When PostHog Code runs standard tools like Read or Bash, the tool result payload was being duplicated: once in content (where the renderer actually reads it) and again in rawOutput. For large file reads or substantial bash output, this doubled the serialized payload sent through the renderer pipeline for no reason. Only MCP tools consume rawOutput — the App bridge replays it to display structured MCP results.
Changes
sdk-to-acp.ts: Conditionally include rawOutput in tool_call_update only when an MCP tool result is present (ctx.mcpToolUseResult). Standard tools now emit status and metadata without the redundant rawOutput payload.
sdk-to-acp.test.ts: Added two tests covering both branches:
This is a pure optimization — no behavioral change for consumers.
How did you test this?
Ran pnpm --filter @posthog/agent test — all 690 tests pass, including the 2 new test cases
Ran pnpm biome lint on both changed files — 0 issues
Manually verified the MCP path: rawOutput still carries content, structuredContent, and isError for App bridge replay
Automatic notifications