Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ describe('ToolCallEmitter', () => {
_meta: { toolName: 'edit_file', provenance: 'builtin' },
}),
);
expect(sendUpdateSpy.mock.calls[0][0].rawOutput).toEqual({
fileName: '/test/file.ts',
originalContent: 'old content',
newContent: 'new content',
});
});

it('should not replay truncated session previews as full diffs', async () => {
Expand Down Expand Up @@ -266,6 +271,7 @@ describe('ToolCallEmitter', () => {
_meta: { toolName: 'edit_file', provenance: 'builtin' },
}),
);
expect(sendUpdateSpy.mock.calls[0][0].rawOutput).toBeUndefined();
Comment thread
doudouOUC marked this conversation as resolved.
});

it('should transform message parts to content', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,10 @@ export class ToolCallEmitter extends BaseEmitter {
};

// Add rawOutput from resultDisplay
if (params.resultDisplay !== undefined) {
if (
params.resultDisplay !== undefined &&
!this.isTruncatedSessionDiffDisplay(params.resultDisplay)
) {
(update as Record<string, unknown>)['rawOutput'] = params.resultDisplay;
}

Expand Down Expand Up @@ -353,7 +356,7 @@ export class ToolCallEmitter extends BaseEmitter {

// Check if this is a diff display (edit tool result)
if ('fileName' in obj && 'newContent' in obj) {
if (obj['truncatedForSession'] === true) {
if (this.isTruncatedSessionDiffDisplay(resultDisplay)) {
return {
type: 'content',
content: {
Expand All @@ -374,6 +377,17 @@ export class ToolCallEmitter extends BaseEmitter {
return null;
}

private isTruncatedSessionDiffDisplay(resultDisplay: unknown): boolean {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The same three-condition check (truncatedForSession === true && 'fileName' in obj && 'newContent' in obj) is duplicated here as isTruncatedSessionDiffDisplay and in ToolGroup.tsx:113 as isTruncatedSessionDiff. They live in separate packages (packages/cli vs packages/web-shell) with different names and share no type or constant.

If the FileDiff shape ever changes (e.g., truncatedForSession is renamed, or a new truncation signal is added), both sites must be updated in lockstep. Silent divergence risk: one side strips rawOutput but the other still renders the diff — the exact bug this PR fixes.

Consider extracting a shared predicate into packages/core (alongside the FileDiff type in tools.ts) and importing it in both consumers.

— qwen3.7-max via Qwen Code /review

if (!resultDisplay || typeof resultDisplay !== 'object') return false;

const obj = resultDisplay as Record<string, unknown>;
return (
obj['truncatedForSession'] === true &&
'fileName' in obj &&
'newContent' in obj
);
}

/**
* Transforms Part[] to ToolCallContent[].
* Extracts text from functionResponse parts and text parts.
Expand Down
17 changes: 15 additions & 2 deletions packages/web-shell/client/adapters/transcriptToMessages.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2328,6 +2328,12 @@ describe('transcriptBlocksToDaemonMessages', () => {
const tool =
messages[0].role === 'tool_group' ? messages[0].tools[0] : undefined;
expect(tool?.rawOutput).toBeUndefined();
expect(tool?.content).toEqual([
{
type: 'content',
content: { type: 'text', text: 'rendered elsewhere' },
},
]);
});

it('mergeToolCall updates fields from completion block', () => {
Expand Down Expand Up @@ -2582,7 +2588,7 @@ describe('transcriptBlocksToDaemonMessages', () => {
expect(execTool?.subContent).toBe('Running...');
});

it('does not pass content, locations, or preview to DaemonMessageToolCall', () => {
it('passes content but not locations or preview to DaemonMessageToolCall', () => {
const messages = transcriptBlocksToDaemonMessages([
toolBlock('t1', 'tc1', 'completed', 1, {
toolName: 'Edit',
Expand All @@ -2603,7 +2609,14 @@ describe('transcriptBlocksToDaemonMessages', () => {
messages[0].role === 'tool_group' ? messages[0].tools[0] : undefined;
expect(tool).toBeDefined();
expect(tool?.callId).toBe('tc1');
expect('content' in tool!).toBe(false);
expect(tool?.content).toEqual([
{
type: 'diff',
path: '/path/file.ts',
oldText: 'old',
newText: 'new',
},
]);
expect('locations' in tool!).toBe(false);
expect('preview' in tool!).toBe(false);
});
Expand Down
56 changes: 56 additions & 0 deletions packages/web-shell/client/adapters/transcriptToMessages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
import type {
DaemonMessage,
DaemonMessageToolCall,
DaemonMessageToolCallContent,
DaemonMessageToolCallStatus,
DaemonMessageToolKind,
DaemonMessageTodoItem,
Expand Down Expand Up @@ -645,6 +646,7 @@ function daemonToolBlockToToolCall(
): DaemonMessageToolCall {
const rawOutput = getToolRawOutput(block);
const isBackgroundAgent = isBackgroundAgentBlock(block, rawOutput);
const content = normalizeToolContent(block.content);
const statusMap: Record<string, DaemonMessageToolCallStatus> = {
running: 'in_progress',
pending: 'pending',
Expand Down Expand Up @@ -676,6 +678,7 @@ function daemonToolBlockToToolCall(
parentToolCallId: block.parentToolCallId,
startTime: block.createdAt,
endTime: isComplete && !isBackgroundAgent ? block.updatedAt : undefined,
...(content ? { content } : {}),
};
}

Expand Down Expand Up @@ -813,6 +816,59 @@ function getToolRawOutput(block: DaemonToolTranscriptBlock): unknown {
};
}

function normalizeToolContent(
value: unknown,
): DaemonMessageToolCallContent[] | undefined {
if (!Array.isArray(value)) return undefined;

const content = value.flatMap((entry): DaemonMessageToolCallContent[] => {
const item = getRecord(entry);
if (!item) return [];

const type = item['type'];
if (type === 'content') {
const body = getRecord(item['content']);
if (!body || typeof body['type'] !== 'string') return [];
return [
{
type: 'content',
content: { ...body, type: body['type'] },
},
];
}

if (type === 'diff') {
const newText = item['newText'];
if (typeof newText !== 'string') return [];

const path = item['path'];
const oldText = item['oldText'];
return [
{
type: 'diff',
...(typeof path === 'string' ? { path } : {}),
...(typeof oldText === 'string' ? { oldText } : {}),
newText,
},
];
}

if (type === 'terminal') {
const terminalId = item['terminalId'];
return [
{
type: 'terminal',
...(typeof terminalId === 'string' ? { terminalId } : {}),
},
];
}

return [];
});

return content.length > 0 ? content : undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] normalizeToolContent silently drops entries whose type doesn't match 'content', 'diff', or 'terminal', and also drops malformed entries (null body, missing type string). No console.warn, no log, no metric.

Since block.content arrives over the SSE wire with no compile-time shape guarantee, any daemon regression or MCP server producing non-standard content shapes will cause tool results to vanish from the web-shell with no observable signal — the hardest kind of failure to debug in production.

Consider adding a console.warn for the fallthrough and malformed-entry paths:

if (typeof type === 'string') {
  console.warn(`normalizeToolContent: unknown content type '${type}' dropped`);
}
return [];

— qwen3.7-max via Qwen Code /review

}

function isAskUserQuestionBlock(block: DaemonToolTranscriptBlock): boolean {
if (!block.toolName) return false;
const normalized = block.toolName.toLowerCase();
Expand Down
118 changes: 118 additions & 0 deletions packages/web-shell/client/components/messages/ToolGroup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ function makeShellTool(
};
}

function makeEditTool(overrides: Partial<ACPToolCall>): ACPToolCall {
return {
callId: 'call-edit-1',
toolName: 'edit',
status: 'completed',
...overrides,
};
}

function renderTool(tool: ACPToolCall): HTMLElement {
const container = document.createElement('div');
document.body.appendChild(container);
Expand Down Expand Up @@ -104,6 +113,14 @@ function click(el: Element): void {
});
}

function expandTool(container: HTMLElement): void {
const chevron = [...container.querySelectorAll('span')].find(
(s) => s.textContent === '▸',
);
expect(chevron).toBeTruthy();
click(chevron!.parentElement!);
}

describe('shell tool output expand toggle', () => {
it('shows short output in full without an expand button', () => {
const container = renderShellTool('one\ntwo\nthree');
Expand Down Expand Up @@ -347,3 +364,104 @@ describe('auto-collapse on finish', () => {
expect(container.textContent).toContain('▾');
});
});

describe('edit raw diff rendering', () => {
it('ignores truncated session rawOutput diffs', () => {
const fullDiff = '--- a/file.ts\n+++ b/file.ts\n@@ -1 +1 @@\n-old\n+new';

const normal = renderTool(
makeEditTool({
rawOutput: {
fileDiff: fullDiff,
},
}),
);
expandTool(normal);
expect(normal.textContent).toContain('old');
expect(normal.textContent).toContain('new');

const truncated = renderTool(
makeEditTool({
rawOutput: {
fileName: '/test/file.ts',
newContent: 'preview only',
fileDiff: fullDiff,
truncatedForSession: true,
},
}),
);
expect(truncated.textContent).not.toContain('old');
expect(truncated.textContent).not.toContain('new');
});

it('shows preview and suppresses truncated rawOutput diffs', () => {
const fullDiff = '--- a/file.ts\n+++ b/file.ts\n@@ -1 +1 @@\n-old\n+new';
const preview =
'Full diff omitted from saved session history for /test/file.ts.';
const container = renderTool(
makeEditTool({
content: [
{
type: 'content',
content: { type: 'text', text: preview },
},
],
rawOutput: {
fileName: '/test/file.ts',
newContent: 'preview only',
fileDiff: fullDiff,
truncatedForSession: true,
},
}),
);

expandTool(container);
expect(container.textContent).toContain(preview);
expect(container.textContent).not.toContain('old');
expect(container.textContent).not.toContain('new');
});

it('renders truncated session preview text when no diff is available', () => {
const preview =
'Full diff omitted from saved session history for /test/file.ts.';
const container = renderTool(
makeEditTool({
content: [
{
type: 'content',
content: { type: 'text', text: preview },
},
],
}),
);

expandTool(container);
expect(container.textContent).toContain(preview);
});

it('expands write preview text when no diff is available', () => {
const preview =
'Full diff omitted from saved session history for /test/file.ts.';
const container = renderTool(
makeEditTool({
toolName: 'write',
content: [
{
type: 'content',
content: { type: 'text', text: preview },
},
],
}),
);

expect(container.querySelector('pre')).toBeNull();

const writeLabel = Array.from(container.querySelectorAll('span')).find(
(el) => el.textContent === 'WriteFile',
);
expect(writeLabel).toBeDefined();
click(writeLabel!);

expect(container.querySelector('pre')?.textContent).toBe(preview);
});
});
Loading
Loading