mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-25 21:41:12 -07:00
cleanup
cleanup tasks
- remove redundant tool alias; simplify core tool error paths and data structures
- update default compact tool success summary text ("Returned")
- refactor ToolGroupMessage logic & border transitions across Compact, Standard, and Subagent output types
feat(ui): make result summary the *diff toggle target* and refine styling in dense output
- Makes the entire result summary (action word + stats) the clickable trigger for diff visibility.
- Styles the action word (e.g., "Accepted") with an underline to indicate interactivity.
- Removes the redundant "[click to show/hide details]" text to clean up the UI.
- Updates tests and snapshots to reflect the new interaction model.
fix(core): derive and preserve *diffstat* info for rejected edit operations
- Adds getDiffStatFromPatch utility to derive numeric stats from unified diff strings.
- Updates ToolEditConfirmationDetails to include an optional diffStat property.
- Modifies StateManager and CoreToolScheduler to populate diffStat when an edit/write is cancelled.
- Ensures the UI can display rich information (e.g., "+5, -2") for rejected operations.
- Adds a core test to verify correct stats derivation on tool rejection.
This commit is contained in:
@@ -278,6 +278,16 @@ describe('DenseToolMessage', () => {
|
||||
filePath: '/path/to/styles.scss',
|
||||
originalContent: 'old line',
|
||||
newContent: 'new line',
|
||||
diffStat: {
|
||||
user_added_lines: 1,
|
||||
user_removed_lines: 1,
|
||||
user_added_chars: 0,
|
||||
user_removed_chars: 0,
|
||||
model_added_lines: 0,
|
||||
model_removed_lines: 0,
|
||||
model_added_chars: 0,
|
||||
model_removed_chars: 0,
|
||||
},
|
||||
};
|
||||
const { lastFrame, waitUntilReady } = renderWithProviders(
|
||||
<DenseToolMessage
|
||||
@@ -290,8 +300,7 @@ describe('DenseToolMessage', () => {
|
||||
await waitUntilReady();
|
||||
const output = lastFrame();
|
||||
expect(output).toContain('Edit');
|
||||
expect(output).toContain('styles.scss');
|
||||
expect(output).toContain('→ Failed');
|
||||
expect(output).toContain('styles.scss → Failed (+1, -1)');
|
||||
expect(output).toMatchSnapshot();
|
||||
});
|
||||
|
||||
@@ -386,7 +395,7 @@ describe('DenseToolMessage', () => {
|
||||
);
|
||||
await waitUntilReady();
|
||||
const output = lastFrame();
|
||||
expect(output).toContain('→ Output received');
|
||||
expect(output).toContain('→ Returned (possible empty result)');
|
||||
expect(output).toMatchSnapshot();
|
||||
});
|
||||
|
||||
@@ -452,7 +461,7 @@ describe('DenseToolMessage', () => {
|
||||
);
|
||||
await waitUntilReady();
|
||||
const output = lastFrame();
|
||||
expect(output).toContain('[click here to show details]');
|
||||
expect(output).toContain('Accepted');
|
||||
expect(output).not.toContain('new line');
|
||||
expect(output).toMatchSnapshot();
|
||||
});
|
||||
@@ -468,12 +477,12 @@ describe('DenseToolMessage', () => {
|
||||
);
|
||||
await waitUntilReady();
|
||||
const output = lastFrame();
|
||||
expect(output).not.toContain('[click here to show details]');
|
||||
expect(output).toContain('Accepted');
|
||||
expect(output).toContain('new line');
|
||||
expect(output).toMatchSnapshot();
|
||||
});
|
||||
|
||||
it('shows diff content after clicking [click here to show details]', async () => {
|
||||
it('shows diff content after clicking summary', async () => {
|
||||
const { lastFrame, waitUntilReady } = renderWithProviders(
|
||||
<DenseToolMessage
|
||||
{...defaultProps}
|
||||
@@ -486,18 +495,6 @@ describe('DenseToolMessage', () => {
|
||||
|
||||
// Verify it's hidden initially
|
||||
expect(lastFrame()).not.toContain('new line');
|
||||
|
||||
// Click [click here to show details]. We simulate a click.
|
||||
// The toggle button is at the end of the summary line.
|
||||
// Instead of precise coordinates, we can try to click everywhere or mock the click handler.
|
||||
// But since we are using ink-testing-library, we can't easily "click" by text.
|
||||
// However, we can verify that the state change works if we trigger the toggle.
|
||||
|
||||
// Actually, I can't easily simulate a click on a specific component by text in ink-testing-library
|
||||
// without knowing exact coordinates.
|
||||
// But I can verify that it RERENDERS with the diff if I can trigger it.
|
||||
|
||||
// For now, verifying the initial state and the non-alt-buffer state is already a good start.
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -42,8 +42,11 @@ interface DenseToolMessageProps extends IndividualToolCallDisplay {
|
||||
}
|
||||
|
||||
interface ViewParts {
|
||||
// brief description of action
|
||||
description?: React.ReactNode;
|
||||
// result summary or status text
|
||||
summary?: React.ReactNode;
|
||||
// detailed output, e.g. diff or command output
|
||||
payload?: React.ReactNode;
|
||||
}
|
||||
|
||||
@@ -91,6 +94,7 @@ function getFileOpData(
|
||||
resultDisplay: unknown,
|
||||
terminalWidth?: number,
|
||||
availableTerminalHeight?: number,
|
||||
isClickable?: boolean,
|
||||
): ViewParts {
|
||||
const added =
|
||||
(diff.diffStat?.model_added_lines ?? 0) +
|
||||
@@ -121,30 +125,34 @@ function getFileOpData(
|
||||
</Text>
|
||||
</Box>
|
||||
);
|
||||
let decision = '';
|
||||
let decisionColor = theme.text.secondary;
|
||||
let resultSummary = '';
|
||||
let resultColor = theme.text.secondary;
|
||||
|
||||
if (status === ToolCallStatus.Confirming) {
|
||||
decision = 'Confirming';
|
||||
resultSummary = 'Confirming';
|
||||
} else if (
|
||||
status === ToolCallStatus.Success ||
|
||||
status === ToolCallStatus.Executing
|
||||
) {
|
||||
decision = 'Accepted';
|
||||
decisionColor = theme.text.accent;
|
||||
resultSummary = 'Accepted';
|
||||
resultColor = theme.text.accent;
|
||||
} else if (status === ToolCallStatus.Canceled) {
|
||||
decision = 'Rejected';
|
||||
decisionColor = theme.status.error;
|
||||
resultSummary = 'Rejected';
|
||||
resultColor = theme.status.error;
|
||||
} else if (status === ToolCallStatus.Error) {
|
||||
decision = typeof resultDisplay === 'string' ? resultDisplay : 'Failed';
|
||||
decisionColor = theme.status.error;
|
||||
resultSummary =
|
||||
typeof resultDisplay === 'string' ? resultDisplay : 'Failed';
|
||||
resultColor = theme.status.error;
|
||||
}
|
||||
|
||||
const summary = (
|
||||
<Box flexDirection="row">
|
||||
{decision && (
|
||||
<Text color={decisionColor} wrap="truncate-end">
|
||||
→ {decision.replace(/\n/g, ' ')}
|
||||
{resultSummary && (
|
||||
<Text color={resultColor} wrap="truncate-end">
|
||||
→{' '}
|
||||
<Text underline={isClickable}>
|
||||
{resultSummary.replace(/\n/g, ' ')}
|
||||
</Text>
|
||||
</Text>
|
||||
)}
|
||||
{showDiffStat && (
|
||||
@@ -217,19 +225,19 @@ function getListDirectoryData(
|
||||
result: ListDirectoryResult,
|
||||
originalDescription?: string,
|
||||
): ViewParts {
|
||||
const summary = <Text color={theme.text.accent}>→ {result.summary}</Text>;
|
||||
const description = originalDescription ? (
|
||||
<Text color={theme.text.secondary} wrap="truncate-end">
|
||||
{originalDescription}
|
||||
</Text>
|
||||
) : undefined;
|
||||
// For directory listings, we want NO payload in dense mode as per request
|
||||
const summary = <Text color={theme.text.accent}>→ {result.summary}</Text>;
|
||||
|
||||
// For directory listings, we want NO payload in dense mode
|
||||
return { description, summary, payload: undefined };
|
||||
}
|
||||
|
||||
function getListResultData(
|
||||
result: ListDirectoryResult | ReadManyFilesResult,
|
||||
_toolName: string,
|
||||
originalDescription?: string,
|
||||
): ViewParts {
|
||||
// Use 'include' to determine if this is a ReadManyFilesResult
|
||||
@@ -293,7 +301,7 @@ function getGenericSuccessData(
|
||||
} else {
|
||||
summary = (
|
||||
<Text color={theme.text.accent} wrap="wrap">
|
||||
→ Output received
|
||||
→ Returned (possible empty result)
|
||||
</Text>
|
||||
);
|
||||
}
|
||||
@@ -381,10 +389,11 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
|
||||
resultDisplay,
|
||||
terminalWidth,
|
||||
availableTerminalHeight,
|
||||
isAlternateBuffer,
|
||||
);
|
||||
}
|
||||
if (isListResult(resultDisplay)) {
|
||||
return getListResultData(resultDisplay, name, originalDescription);
|
||||
return getListResultData(resultDisplay, originalDescription);
|
||||
}
|
||||
|
||||
if (isGrepResult(resultDisplay)) {
|
||||
@@ -430,10 +439,10 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
|
||||
diff,
|
||||
mappedStatus,
|
||||
resultDisplay,
|
||||
name,
|
||||
terminalWidth,
|
||||
availableTerminalHeight,
|
||||
originalDescription,
|
||||
isAlternateBuffer,
|
||||
]);
|
||||
|
||||
const { description, summary } = viewParts;
|
||||
@@ -519,17 +528,15 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
|
||||
{description}
|
||||
</Box>
|
||||
{summary && (
|
||||
<Box key="tool-summary" marginLeft={1} flexGrow={0}>
|
||||
<Box
|
||||
key="tool-summary"
|
||||
ref={isAlternateBuffer && diff ? toggleRef : undefined}
|
||||
marginLeft={1}
|
||||
flexGrow={0}
|
||||
>
|
||||
{summary}
|
||||
</Box>
|
||||
)}
|
||||
{isAlternateBuffer && diff && (
|
||||
<Box key="tool-toggle" ref={toggleRef} marginLeft={1} flexGrow={1}>
|
||||
<Text color={theme.text.link} dimColor>
|
||||
[click here to {isExpanded ? 'hide' : 'show'} details]
|
||||
</Text>
|
||||
</Box>
|
||||
)}
|
||||
</Box>
|
||||
|
||||
{showPayload && isAlternateBuffer && diffLines.length > 0 && (
|
||||
|
||||
@@ -126,9 +126,10 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
const isCompactModeEnabled = settings.merged.ui?.compactToolOutput === true;
|
||||
|
||||
// Filter out tool calls that should be hidden (e.g. in-progress Ask User, or Plan Mode operations).
|
||||
const toolCalls = useMemo(
|
||||
const visibleToolCalls = useMemo(
|
||||
() =>
|
||||
allToolCalls.filter((t) => {
|
||||
// Hide internal errors unless full verbosity
|
||||
if (
|
||||
isLowErrorVerbosity &&
|
||||
t.status === CoreToolCallStatus.Error &&
|
||||
@@ -136,19 +137,34 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
// Standard hiding logic (e.g. Plan Mode internal edits)
|
||||
if (
|
||||
shouldHideToolCall({
|
||||
displayName: t.name,
|
||||
status: t.status,
|
||||
approvalMode: t.approvalMode,
|
||||
hasResultDisplay: !!t.resultDisplay,
|
||||
parentCallId: t.parentCallId,
|
||||
})
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return !shouldHideToolCall({
|
||||
displayName: t.name,
|
||||
status: t.status,
|
||||
approvalMode: t.approvalMode,
|
||||
hasResultDisplay: !!t.resultDisplay,
|
||||
parentCallId: t.parentCallId,
|
||||
});
|
||||
// We HIDE tools that are still in pre-execution states (Confirming, Pending)
|
||||
// from the History log. They live in the Global Queue or wait for their turn.
|
||||
// Only show tools that are actually running or finished.
|
||||
const displayStatus = mapCoreStatusToDisplayStatus(t.status);
|
||||
|
||||
// We hide Confirming tools from the history log because they are
|
||||
// currently being rendered in the interactive ToolConfirmationQueue.
|
||||
// We show everything else, including Pending (waiting to run) and
|
||||
// Canceled (rejected by user), to ensure the history is complete
|
||||
// and to avoid tools "vanishing" after approval.
|
||||
return displayStatus !== ToolCallStatus.Confirming;
|
||||
}),
|
||||
[allToolCalls, isLowErrorVerbosity],
|
||||
);
|
||||
|
||||
const config = useConfig();
|
||||
const {
|
||||
activePtyId,
|
||||
embeddedShellFocused,
|
||||
@@ -156,6 +172,8 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
pendingHistoryItems,
|
||||
} = useUIState();
|
||||
|
||||
const config = useConfig();
|
||||
|
||||
const { borderColor, borderDimColor } = useMemo(
|
||||
() =>
|
||||
getToolGroupBorderAppearance(
|
||||
@@ -174,26 +192,6 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
],
|
||||
);
|
||||
|
||||
// We HIDE tools that are still in pre-execution states (Confirming, Pending)
|
||||
// from the History log. They live in the Global Queue or wait for their turn.
|
||||
// Only show tools that are actually running or finished.
|
||||
// We explicitly exclude Pending and Confirming to ensure they only
|
||||
// appear in the Global Queue until they are approved and start executing.
|
||||
const visibleToolCalls = useMemo(
|
||||
() =>
|
||||
toolCalls.filter((t) => {
|
||||
const displayStatus = mapCoreStatusToDisplayStatus(t.status);
|
||||
// We hide Confirming tools from the history log because they are
|
||||
// currently being rendered in the interactive ToolConfirmationQueue.
|
||||
// We show everything else, including Pending (waiting to run) and
|
||||
// Canceled (rejected by user), to ensure the history is complete
|
||||
// and to avoid tools "vanishing" after approval.
|
||||
return displayStatus !== ToolCallStatus.Confirming;
|
||||
}),
|
||||
|
||||
[toolCalls],
|
||||
);
|
||||
|
||||
const groupedTools = useMemo(() => {
|
||||
const groups: Array<
|
||||
IndividualToolCallDisplay | IndividualToolCallDisplay[]
|
||||
@@ -218,11 +216,21 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
for (let i = 0; i < groupedTools.length; i++) {
|
||||
const group = groupedTools[i];
|
||||
const isFirst = i === 0;
|
||||
const prevGroup = i > 0 ? groupedTools[i - 1] : null;
|
||||
const prevIsCompact =
|
||||
prevGroup &&
|
||||
!Array.isArray(prevGroup) &&
|
||||
isCompactTool(prevGroup, isCompactModeEnabled);
|
||||
|
||||
if (Array.isArray(group)) {
|
||||
// Agent group
|
||||
height += 1; // Header
|
||||
height += group.length; // 1 line per agent
|
||||
const isFirstProp = isFirst
|
||||
? (borderTopOverride ?? true)
|
||||
: prevIsCompact;
|
||||
if (isFirstProp) height += 1; // Top border
|
||||
|
||||
// Spacing logic
|
||||
if (isFirst) {
|
||||
height += (borderTopOverride ?? true) ? 1 : 0;
|
||||
@@ -230,14 +238,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
height += 1; // marginTop
|
||||
}
|
||||
} else {
|
||||
const tool = group;
|
||||
const isCompact = isCompactTool(tool, isCompactModeEnabled);
|
||||
const prevGroup = i > 0 ? groupedTools[i - 1] : null;
|
||||
const prevIsCompact =
|
||||
prevGroup &&
|
||||
!Array.isArray(prevGroup) &&
|
||||
isCompactTool(prevGroup, isCompactModeEnabled);
|
||||
|
||||
const isCompact = isCompactTool(group, isCompactModeEnabled);
|
||||
if (isCompact) {
|
||||
height += 1; // Base height for compact tool
|
||||
// Spacing logic (matching marginTop)
|
||||
@@ -341,11 +342,26 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
!Array.isArray(nextGroup) &&
|
||||
isCompactTool(nextGroup, isCompactModeEnabled);
|
||||
|
||||
if (Array.isArray(group)) {
|
||||
// Subagent group behaves like a standard tool for borders
|
||||
const marginTop = isFirst ? (borderTopOverride ?? true ? 1 : 0) : 1;
|
||||
const isFirstProp = !!(isFirst ? (borderTopOverride ?? true) : prevIsCompact);
|
||||
const isAgentGroup = Array.isArray(group);
|
||||
const isCompact =
|
||||
!isAgentGroup && isCompactTool(group, isCompactModeEnabled);
|
||||
|
||||
let marginTop = 0;
|
||||
if (isFirst) {
|
||||
marginTop = (borderTopOverride ?? true) ? 1 : 0;
|
||||
} else if (isCompact && prevIsCompact) {
|
||||
marginTop = 0;
|
||||
} else {
|
||||
marginTop = 1;
|
||||
}
|
||||
|
||||
const isFirstProp = !!(isFirst
|
||||
? (borderTopOverride ?? true)
|
||||
: prevIsCompact);
|
||||
|
||||
const showClosingBorder = !isCompact && (nextIsCompact || isLast);
|
||||
|
||||
if (isAgentGroup) {
|
||||
return (
|
||||
<Box
|
||||
key={group[0].callId}
|
||||
@@ -362,7 +378,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
isFirst={isFirstProp}
|
||||
isExpandable={isExpandable}
|
||||
/>
|
||||
{(nextIsCompact || isLast) && (
|
||||
{showClosingBorder && (
|
||||
<Box
|
||||
height={0}
|
||||
width={contentWidth}
|
||||
@@ -380,41 +396,13 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
}
|
||||
|
||||
const tool = group;
|
||||
const isCompact = isCompactTool(tool, isCompactModeEnabled);
|
||||
const isShellToolCall = isShellTool(tool.name);
|
||||
|
||||
let marginTop = 0;
|
||||
if (isFirst) {
|
||||
marginTop = (borderTopOverride ?? true) ? 1 : 0;
|
||||
} else if (isCompact && prevIsCompact) {
|
||||
marginTop = 0;
|
||||
// } else if (!isCompact && prevIsCompact) {
|
||||
// marginTop = 1;
|
||||
} else {
|
||||
marginTop = 1;
|
||||
}
|
||||
|
||||
let createTopBorder = true;
|
||||
if (isCompact) {
|
||||
createTopBorder = false;
|
||||
}
|
||||
// } else if (isFirst) {
|
||||
// createTopBorder = borderTopOverride ?? true;
|
||||
// } else {
|
||||
// createTopBorder = !!prevIsCompact;
|
||||
// }
|
||||
|
||||
const commonProps = {
|
||||
...tool,
|
||||
availableTerminalHeight: availableTerminalHeightPerToolMessage,
|
||||
terminalWidth: contentWidth,
|
||||
emphasis: 'medium' as const,
|
||||
// isFirst: !!(isCompact
|
||||
// ? false
|
||||
// : isFirst
|
||||
// ? (borderTopOverride ?? true)
|
||||
// : prevIsCompact),
|
||||
isFirst: createTopBorder,
|
||||
isFirst: isCompact ? false : isFirstProp,
|
||||
borderColor,
|
||||
borderDimColor,
|
||||
isExpandable,
|
||||
@@ -456,7 +444,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
||||
</Box>
|
||||
)}
|
||||
</Box>
|
||||
{!isCompact && (nextIsCompact || isLast) && (
|
||||
{showClosingBorder && (
|
||||
<Box
|
||||
height={0}
|
||||
width={contentWidth}
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
|
||||
|
||||
exports[`DenseToolMessage > Toggleable Diff View (Alternate Buffer) > hides diff content by default when in alternate buffer mode 1`] = `
|
||||
" ✓ test-tool test.ts → Accepted [click here to show details]
|
||||
" ✓ test-tool test.ts → Accepted
|
||||
"
|
||||
`;
|
||||
|
||||
@@ -32,7 +32,7 @@ exports[`DenseToolMessage > renders correctly for Edit tool using confirmationDe
|
||||
`;
|
||||
|
||||
exports[`DenseToolMessage > renders correctly for Errored Edit tool 1`] = `
|
||||
" x Edit styles.scss → Failed [click here to show details]
|
||||
" x Edit styles.scss → Failed (+1, -1)
|
||||
"
|
||||
`;
|
||||
|
||||
@@ -119,7 +119,7 @@ exports[`DenseToolMessage > renders generic failure message for error status wit
|
||||
`;
|
||||
|
||||
exports[`DenseToolMessage > renders generic output message for unknown object results 1`] = `
|
||||
" ✓ test-tool Test description → Output received
|
||||
" ✓ test-tool Test description → Returned (possible empty result)
|
||||
"
|
||||
`;
|
||||
|
||||
|
||||
@@ -23,6 +23,7 @@ import {
|
||||
getToolSuggestion,
|
||||
isToolCallResponseInfo,
|
||||
} from '../utils/tool-utils.js';
|
||||
import { getDiffStatFromPatch } from '../tools/diffOptions.js';
|
||||
import type { ToolConfirmationRequest } from '../confirmation-bus/types.js';
|
||||
import { MessageBusType } from '../confirmation-bus/types.js';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
@@ -312,6 +313,12 @@ export class CoreToolScheduler {
|
||||
waitingCall.confirmationDetails.originalContent,
|
||||
newContent: waitingCall.confirmationDetails.newContent,
|
||||
filePath: waitingCall.confirmationDetails.filePath,
|
||||
// Derive stats from the patch if they aren't already present
|
||||
diffStat:
|
||||
waitingCall.confirmationDetails.diffStat ??
|
||||
getDiffStatFromPatch(
|
||||
waitingCall.confirmationDetails.fileDiff,
|
||||
),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -22,6 +22,7 @@ import {
|
||||
ToolConfirmationOutcome,
|
||||
type AnyDeclarativeTool,
|
||||
type AnyToolInvocation,
|
||||
type FileDiff,
|
||||
} from '../tools/tools.js';
|
||||
import { MessageBusType } from '../confirmation-bus/types.js';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
@@ -359,7 +360,7 @@ describe('SchedulerStateManager', () => {
|
||||
expect(active.confirmationDetails).toEqual(details);
|
||||
});
|
||||
|
||||
it('should preserve diff when cancelling an edit tool call', () => {
|
||||
it('should preserve diff and derive stats when cancelling an edit tool call', () => {
|
||||
const call = createValidatingCall();
|
||||
stateManager.enqueue([call]);
|
||||
stateManager.dequeue();
|
||||
@@ -369,9 +370,9 @@ describe('SchedulerStateManager', () => {
|
||||
title: 'Edit',
|
||||
fileName: 'test.txt',
|
||||
filePath: '/path/to/test.txt',
|
||||
fileDiff: 'diff',
|
||||
originalContent: 'old',
|
||||
newContent: 'new',
|
||||
fileDiff: '@@ -1,1 +1,1 @@\n-old line\n+new line',
|
||||
originalContent: 'old line',
|
||||
newContent: 'new line',
|
||||
onConfirm: vi.fn(),
|
||||
};
|
||||
|
||||
@@ -389,13 +390,14 @@ describe('SchedulerStateManager', () => {
|
||||
|
||||
const completed = stateManager.completedBatch[0] as CancelledToolCall;
|
||||
expect(completed.status).toBe(CoreToolCallStatus.Cancelled);
|
||||
expect(completed.response.resultDisplay).toEqual({
|
||||
fileDiff: 'diff',
|
||||
fileName: 'test.txt',
|
||||
filePath: '/path/to/test.txt',
|
||||
originalContent: 'old',
|
||||
newContent: 'new',
|
||||
});
|
||||
const result = completed.response.resultDisplay as FileDiff;
|
||||
expect(result.fileDiff).toBe(details.fileDiff);
|
||||
expect(result.diffStat).toEqual(
|
||||
expect.objectContaining({
|
||||
model_added_lines: 1,
|
||||
model_removed_lines: 1,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should ignore status updates for non-existent callIds', () => {
|
||||
|
||||
@@ -32,6 +32,7 @@ import {
|
||||
type SerializableConfirmationDetails,
|
||||
} from '../confirmation-bus/types.js';
|
||||
import { isToolCallResponseInfo } from '../utils/tool-utils.js';
|
||||
import { getDiffStatFromPatch } from '../tools/diffOptions.js';
|
||||
|
||||
/**
|
||||
* Handler for terminal tool calls.
|
||||
@@ -473,6 +474,8 @@ export class SchedulerStateManager {
|
||||
filePath: details.filePath,
|
||||
originalContent: details.originalContent,
|
||||
newContent: details.newContent,
|
||||
// Derive stats from the patch if they aren't already present
|
||||
diffStat: details.diffStat ?? getDiffStatFromPatch(details.fileDiff),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -76,3 +76,39 @@ export function getDiffStat(
|
||||
user_removed_chars: userStats.removedChars,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Extracts line and character stats from a unified diff patch string.
|
||||
* This is useful for reconstructing stats for rejected or errored operations
|
||||
* where the full strings may no longer be easily accessible.
|
||||
*/
|
||||
export function getDiffStatFromPatch(patch: string): DiffStat {
|
||||
let addedLines = 0;
|
||||
let removedLines = 0;
|
||||
let addedChars = 0;
|
||||
let removedChars = 0;
|
||||
|
||||
const lines = patch.split('\n');
|
||||
for (const line of lines) {
|
||||
// Only count lines that are additions or removals,
|
||||
// excluding the diff headers (--- and +++) and metadata (\)
|
||||
if (line.startsWith('+') && !line.startsWith('+++')) {
|
||||
addedLines++;
|
||||
addedChars += line.length - 1;
|
||||
} else if (line.startsWith('-') && !line.startsWith('---')) {
|
||||
removedLines++;
|
||||
removedChars += line.length - 1;
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
model_added_lines: addedLines,
|
||||
model_removed_lines: removedLines,
|
||||
model_added_chars: addedChars,
|
||||
model_removed_chars: removedChars,
|
||||
user_added_lines: 0,
|
||||
user_removed_lines: 0,
|
||||
user_added_chars: 0,
|
||||
user_removed_chars: 0,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -162,9 +162,7 @@ describe('LSTool', () => {
|
||||
const result = await invocation.execute(abortSignal);
|
||||
|
||||
expect(result.llmContent).toBe(`Directory ${emptyDir} is empty.`);
|
||||
expect(result.returnDisplay).toEqual(
|
||||
expect.objectContaining({ summary: 'Directory is empty.' }),
|
||||
);
|
||||
expect(result.returnDisplay).toBe('Directory is empty.');
|
||||
});
|
||||
|
||||
it('should respect ignore patterns', async () => {
|
||||
@@ -226,9 +224,7 @@ describe('LSTool', () => {
|
||||
const result = await invocation.execute(abortSignal);
|
||||
|
||||
expect(result.llmContent).toContain('Path is not a directory');
|
||||
expect(result.returnDisplay).toEqual(
|
||||
expect.objectContaining({ summary: 'Error: Path is not a directory.' }),
|
||||
);
|
||||
expect(result.returnDisplay).toBe('Error: Path is not a directory.');
|
||||
expect(result.error?.type).toBe(ToolErrorType.PATH_IS_NOT_A_DIRECTORY);
|
||||
});
|
||||
|
||||
@@ -238,11 +234,7 @@ describe('LSTool', () => {
|
||||
const result = await invocation.execute(abortSignal);
|
||||
|
||||
expect(result.llmContent).toContain('Error listing directory');
|
||||
expect(result.returnDisplay).toEqual(
|
||||
expect.objectContaining({
|
||||
summary: 'Error: Failed to list directory.',
|
||||
}),
|
||||
);
|
||||
expect(result.returnDisplay).toBe('Error: Failed to list directory.');
|
||||
expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR);
|
||||
});
|
||||
|
||||
@@ -282,11 +274,7 @@ describe('LSTool', () => {
|
||||
|
||||
expect(result.llmContent).toContain('Error listing directory');
|
||||
expect(result.llmContent).toContain('permission denied');
|
||||
expect(result.returnDisplay).toEqual(
|
||||
expect.objectContaining({
|
||||
summary: 'Error: Failed to list directory.',
|
||||
}),
|
||||
);
|
||||
expect(result.returnDisplay).toBe('Error: Failed to list directory.');
|
||||
expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR);
|
||||
});
|
||||
|
||||
|
||||
@@ -143,10 +143,7 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
|
||||
): ToolResult {
|
||||
return {
|
||||
llmContent,
|
||||
// Return an object with summary for dense output support
|
||||
returnDisplay: {
|
||||
summary: `Error: ${returnDisplay}`,
|
||||
},
|
||||
returnDisplay: `Error: ${returnDisplay}`,
|
||||
error: {
|
||||
message: llmContent,
|
||||
type,
|
||||
@@ -171,9 +168,7 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
|
||||
if (validationError) {
|
||||
return {
|
||||
llmContent: validationError,
|
||||
returnDisplay: {
|
||||
summary: 'Path not in workspace.',
|
||||
},
|
||||
returnDisplay: 'Path not in workspace.',
|
||||
error: {
|
||||
message: validationError,
|
||||
type: ToolErrorType.PATH_NOT_IN_WORKSPACE,
|
||||
@@ -205,9 +200,7 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
|
||||
// Changed error message to be more neutral for LLM
|
||||
return {
|
||||
llmContent: `Directory ${resolvedDirPath} is empty.`,
|
||||
returnDisplay: {
|
||||
summary: `Directory is empty.`,
|
||||
},
|
||||
returnDisplay: `Directory is empty.`,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -273,9 +273,7 @@ ${finalExclusionPatternsForDescription
|
||||
const errorMessage = `Error during file search: ${getErrorMessage(error)}`;
|
||||
return {
|
||||
llmContent: errorMessage,
|
||||
returnDisplay: {
|
||||
summary: `Error: ${getErrorMessage(error)}`,
|
||||
},
|
||||
returnDisplay: `Error: ${getErrorMessage(error)}`,
|
||||
error: {
|
||||
message: errorMessage,
|
||||
type: ToolErrorType.READ_MANY_FILES_SEARCH_ERROR,
|
||||
|
||||
@@ -150,8 +150,6 @@ export {
|
||||
SKILL_PARAM_NAME,
|
||||
};
|
||||
|
||||
export const LS_TOOL_NAME_LEGACY = 'list_directory'; // Just to be safe if anything used the old exported name directly
|
||||
|
||||
export const EDIT_TOOL_NAMES = new Set([EDIT_TOOL_NAME, WRITE_FILE_TOOL_NAME]);
|
||||
|
||||
/**
|
||||
|
||||
@@ -910,6 +910,7 @@ export interface ToolEditConfirmationDetails {
|
||||
originalContent: string | null;
|
||||
newContent: string;
|
||||
isModifying?: boolean;
|
||||
diffStat?: DiffStat;
|
||||
ideConfirmation?: Promise<DiffUpdateResult>;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user