Skip to content

Commit d986128

Browse files
author
俊良
committed
fix(cli): preserve advisor continuation order
1 parent 524b0fb commit d986128

10 files changed

Lines changed: 351 additions & 19 deletions

File tree

‎packages/cli/src/ui/components/messages/AdvisorMessage.tsx‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import React from 'react';
88
import { Box, Text } from 'ink';
9+
import type { AdvisorReviewDisplay } from '@qwen-code/qwen-code-core';
910
import { Colors } from '../../colors.js';
1011
import { MarkdownDisplay } from '../../utils/MarkdownDisplay.js';
1112
import { useTerminalSize } from '../../hooks/useTerminalSize.js';
@@ -19,9 +20,55 @@ export interface AdvisorDisplayProps {
1920
containerWidth?: number;
2021
}
2122

23+
export interface AdvisorReviewCardProps {
24+
review: AdvisorReviewDisplay;
25+
containerWidth: number;
26+
}
27+
2228
// border(1)*2 + paddingX(1)*2 = 4
2329
const ADVISOR_SELF_CHROME = 4;
2430

31+
const ADVISOR_REVIEW_SECTIONS: ReadonlyArray<{
32+
title: string;
33+
field: keyof Omit<AdvisorReviewDisplay, 'type'>;
34+
}> = [
35+
{ title: 'Verdict', field: 'verdict' },
36+
{ title: 'Risks', field: 'risks' },
37+
{ title: 'Missing evidence', field: 'missingEvidence' },
38+
{ title: 'Recommendation', field: 'recommendation' },
39+
];
40+
41+
export const AdvisorReviewCard: React.FC<AdvisorReviewCardProps> = ({
42+
review,
43+
containerWidth,
44+
}) => {
45+
const contentWidth = Math.max(2, containerWidth - ADVISOR_SELF_CHROME);
46+
47+
return (
48+
<Box
49+
flexDirection="column"
50+
borderStyle="round"
51+
borderColor={Colors.AccentCyan}
52+
paddingX={1}
53+
width="100%"
54+
>
55+
<Text color={Colors.AccentCyan} bold>
56+
Advisor feedback
57+
</Text>
58+
{ADVISOR_REVIEW_SECTIONS.map(({ title, field }) => (
59+
<Box key={field} flexDirection="column" marginTop={1}>
60+
<Text bold>{title}</Text>
61+
<MarkdownDisplay
62+
text={review[field]}
63+
isPending={false}
64+
contentWidth={contentWidth}
65+
/>
66+
</Box>
67+
))}
68+
</Box>
69+
);
70+
};
71+
2572
const AdvisorMessageInternal: React.FC<AdvisorDisplayProps> = ({
2673
text,
2774
model,

‎packages/cli/src/ui/components/messages/ToolMessage.test.tsx‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -802,6 +802,33 @@ describe('<ToolMessage />', () => {
802802
expect(lastFrame()).toMatch(/MockDiff:--- a\/file\.txt/);
803803
});
804804

805+
it('renders structured Advisor feedback instead of stringified JSON', () => {
806+
const { lastFrame } = renderWithContext(
807+
<ToolMessage
808+
{...baseProps}
809+
name="advisor"
810+
description="Consult Advisor"
811+
resultDisplay={{
812+
type: 'advisor_review',
813+
verdict: 'Check the edge case.',
814+
risks: 'Retries may be missing.',
815+
missingEvidence: 'No failing test output.',
816+
recommendation: 'Add a regression test.',
817+
}}
818+
/>,
819+
StreamingState.Idle,
820+
);
821+
822+
const output = lastFrame();
823+
expect(output).toContain('Advisor feedback');
824+
expect(output).toContain('Verdict');
825+
expect(output).toContain('Risks');
826+
expect(output).toContain('Missing evidence');
827+
expect(output).toContain('Recommendation');
828+
expect(output).toContain('MockMarkdown:Check the edge case.');
829+
expect(output).not.toContain('"advisor_review"');
830+
});
831+
805832
it('diff results are not collapsed for completed collapsible tools (bypass shouldCollapseResult)', () => {
806833
const diffResult = {
807834
fileDiff: '--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new',

‎packages/cli/src/ui/components/messages/ToolMessage.tsx‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,15 @@ import type {
2020
PlanResultDisplay,
2121
AnsiOutput,
2222
AnsiOutputDisplay,
23+
AdvisorReviewDisplay,
2324
Config,
2425
McpToolProgressData,
2526
FileDiff,
2627
TerminalImageDisplay,
2728
} from '@qwen-code/qwen-code-core';
2829
import {
2930
formatVisionBridgeNoticeDisplay,
31+
isAdvisorReviewDisplay,
3032
isTerminalImageDisplay,
3133
isVisionBridgeNoticeDisplay,
3234
ToolNames,
@@ -59,6 +61,7 @@ import {
5961
import { ToolElapsedTime } from '../shared/ToolElapsedTime.js';
6062
import { TerminalImage } from '../TerminalImage.js';
6163
import { formatInlineImageOverflow } from '../../utils/inline-image-parts.js';
64+
import { AdvisorReviewCard } from './AdvisorMessage.js';
6265

6366
// Names that resolve to the agent tool: the canonical name plus whatever
6467
// legacy request aliases core's migration map declares (e.g. 'task').
@@ -173,6 +176,7 @@ type DisplayRendererResult =
173176
| { type: 'none' }
174177
| { type: 'todo'; data: TodoResultDisplay }
175178
| { type: 'plan'; data: PlanResultDisplay }
179+
| { type: 'advisor'; data: AdvisorReviewDisplay }
176180
| { type: 'string'; data: string }
177181
| { type: 'diff'; data: { fileDiff: string; fileName: string } }
178182
| { type: 'task'; data: AgentResultDisplay }
@@ -194,6 +198,13 @@ const useResultDisplayRenderer = (
194198
return { type: 'image', data: resultDisplay };
195199
}
196200

201+
if (isAdvisorReviewDisplay(resultDisplay)) {
202+
return {
203+
type: 'advisor',
204+
data: resultDisplay,
205+
};
206+
}
207+
197208
// Check for TodoResultDisplay
198209
if (
199210
typeof resultDisplay === 'object' &&
@@ -938,6 +949,12 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
938949
childWidth={innerWidth}
939950
/>
940951
)}
952+
{effectiveDisplayRenderer.type === 'advisor' && (
953+
<AdvisorReviewCard
954+
review={effectiveDisplayRenderer.data}
955+
containerWidth={innerWidth}
956+
/>
957+
)}
941958
{effectiveDisplayRenderer.type === 'task' && config && (
942959
<SubagentExecutionRenderer
943960
data={effectiveDisplayRenderer.data}

‎packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,50 @@ describe('reduceDaemonEventToTuiUpdates', () => {
162162
]);
163163
});
164164

165+
it('preserves a sanitized Advisor review as structured output', () => {
166+
const updates = reduceDaemonEventToTuiUpdates({
167+
id: 1,
168+
v: 1,
169+
type: 'session_update',
170+
data: {
171+
sessionId: 'session-1',
172+
update: {
173+
sessionUpdate: 'tool_call_update',
174+
toolCallId: 'tool-advisor',
175+
kind: 'advisor',
176+
title: 'Consult Advisor',
177+
status: 'completed',
178+
rawOutput: {
179+
type: 'advisor_review',
180+
verdict: 'Check\x1b]0;bad\x07 the edge case.',
181+
risks: 'Retries may be missing.',
182+
missingEvidence: 'No failing test output.',
183+
recommendation: 'Add a regression test.',
184+
},
185+
},
186+
},
187+
});
188+
189+
expect(updates).toMatchObject([
190+
{
191+
type: 'tool_group_update',
192+
item: {
193+
tools: [
194+
{
195+
resultDisplay: {
196+
type: 'advisor_review',
197+
verdict: 'Check the edge case.',
198+
risks: 'Retries may be missing.',
199+
missingEvidence: 'No failing test output.',
200+
recommendation: 'Add a regression test.',
201+
},
202+
},
203+
],
204+
},
205+
},
206+
]);
207+
});
208+
165209
it('maps assistant, tool, model, and disconnect daemon events while suppressing thought history', () => {
166210
expect(
167211
reduceDaemonEventToTuiUpdates({

‎packages/cli/src/ui/daemon/daemon-tui-adapter.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type {
1111
} from '@agentclientprotocol/sdk';
1212
import {
1313
createDebugLogger,
14+
isAdvisorReviewDisplay,
1415
isVisionBridgeNoticeDisplay,
1516
} from '@qwen-code/qwen-code-core';
1617
import {
@@ -276,6 +277,11 @@ function formatToolResultDisplay(
276277
value,
277278
) as IndividualToolCallDisplay['resultDisplay'];
278279
}
280+
if (isAdvisorReviewDisplay(value)) {
281+
return sanitizeDaemonValue(
282+
value,
283+
) as IndividualToolCallDisplay['resultDisplay'];
284+
}
279285
if (
280286
isRecord(value) &&
281287
(typeof value['fileDiff'] === 'string' ||

‎packages/cli/src/ui/hooks/useGeminiStream.test.tsx‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5155,6 +5155,137 @@ describe('useGeminiStream', () => {
51555155
]);
51565156
});
51575157

5158+
it('commits streamed text before scheduling a tool continuation', async () => {
5159+
const toolRequest = {
5160+
callId: 'advisor-call',
5161+
name: 'advisor',
5162+
args: {},
5163+
isClientInitiated: false,
5164+
prompt_id: 'prompt-advisor',
5165+
};
5166+
mockSendMessageStream.mockReturnValueOnce(
5167+
(async function* () {
5168+
yield {
5169+
type: ServerGeminiEventType.Content,
5170+
value: 'I will ask the advisor before continuing.',
5171+
};
5172+
yield {
5173+
type: ServerGeminiEventType.ToolCallRequest,
5174+
value: toolRequest,
5175+
};
5176+
})(),
5177+
);
5178+
5179+
const { result } = renderTestHook();
5180+
5181+
await act(async () => {
5182+
await result.current.submitQuery('review this change');
5183+
});
5184+
5185+
const textCommitIndex = mockAddItem.mock.calls.findIndex(
5186+
([item]) =>
5187+
item.type === 'gemini' &&
5188+
item.text === 'I will ask the advisor before continuing.',
5189+
);
5190+
const scheduleOrder = mockScheduleToolCalls.mock.invocationCallOrder[0];
5191+
5192+
expect(textCommitIndex).toBeGreaterThanOrEqual(0);
5193+
expect(scheduleOrder).toBeDefined();
5194+
expect(mockAddItem.mock.invocationCallOrder[textCommitIndex]).toBeLessThan(
5195+
scheduleOrder!,
5196+
);
5197+
});
5198+
5199+
it('keeps a completed tool ahead of its streaming continuation', async () => {
5200+
const toolRequest = {
5201+
callId: 'advisor-continuation',
5202+
name: 'advisor',
5203+
args: {},
5204+
isClientInitiated: false,
5205+
prompt_id: 'prompt-advisor-continuation',
5206+
};
5207+
let releaseContinuation!: () => void;
5208+
const continuationStarted = new Promise<void>((resolve) => {
5209+
releaseContinuation = resolve;
5210+
});
5211+
mockSendMessageStream
5212+
.mockReturnValueOnce(
5213+
(async function* () {
5214+
yield {
5215+
type: ServerGeminiEventType.ToolCallRequest,
5216+
value: toolRequest,
5217+
};
5218+
})(),
5219+
)
5220+
.mockReturnValueOnce(
5221+
(async function* () {
5222+
yield {
5223+
type: ServerGeminiEventType.Content,
5224+
value: 'Here is the final answer.',
5225+
};
5226+
await continuationStarted;
5227+
})(),
5228+
);
5229+
const completedTool = {
5230+
request: toolRequest,
5231+
status: 'success',
5232+
responseSubmittedToGemini: false,
5233+
response: {
5234+
callId: toolRequest.callId,
5235+
responseParts: [
5236+
{
5237+
functionResponse: {
5238+
id: toolRequest.callId,
5239+
name: toolRequest.name,
5240+
response: { output: 'advisor feedback' },
5241+
},
5242+
},
5243+
],
5244+
resultDisplay: 'advisor feedback',
5245+
error: undefined,
5246+
errorType: undefined,
5247+
},
5248+
tool: {
5249+
name: 'advisor',
5250+
displayName: 'Advisor',
5251+
description: 'Consult Advisor',
5252+
build: vi.fn(),
5253+
},
5254+
invocation: {
5255+
getDescription: () => 'Consult Advisor',
5256+
},
5257+
} as unknown as TrackedCompletedToolCall;
5258+
5259+
const { result } = renderTestHook();
5260+
5261+
await act(async () => {
5262+
await result.current.submitQuery('review this change');
5263+
});
5264+
5265+
const onComplete = mockUseReactToolScheduler.mock.calls.at(-1)?.[0] as
5266+
| ((tools: TrackedCompletedToolCall[]) => Promise<void>)
5267+
| undefined;
5268+
let completionPromise: Promise<void> | undefined;
5269+
act(() => {
5270+
completionPromise = onComplete?.([completedTool]);
5271+
});
5272+
5273+
await waitFor(() => {
5274+
expect(mockSendMessageStream).toHaveBeenCalledTimes(2);
5275+
expect(
5276+
result.current.pendingHistoryItems.map((item) => item.type),
5277+
).toEqual(['tool_group', 'gemini']);
5278+
});
5279+
5280+
await act(async () => {
5281+
releaseContinuation();
5282+
await completionPromise;
5283+
});
5284+
5285+
const committedTypes = mockAddItem.mock.calls.map(([item]) => item.type);
5286+
expect(committedTypes.slice(-2)).toEqual(['tool_group', 'gemini']);
5287+
});
5288+
51585289
it('drops a late tool result whose callId is already paired in chat.history (Race A dedup)', async () => {
51595290
// Race A repro: the chat-internal repair pass already synthesized a
51605291
// functionResponse for this callId on the Retry push (because the

0 commit comments

Comments
 (0)