Skip to content

Commit 965b5fb

Browse files
osortegaCopilot
andcommitted
Address review on sandbox write permission rendering
Translate the write permission's invocationMessage, not just the confirmation message. AgentSessionApprovalModel reads invocationMessage for non-terminal approvals, so the sessions list showed the host's generic "Edit file" while the confirmation card showed the file. Both now derive from the same value. Normalize a Windows host path before formatting the link. URI.file only rewrites separators when the client runs Windows, so a Windows host paired with a non-Windows client collapsed C:\repo\file.ts into a single-segment basename and an unresolvable URI. Revert the chatToolConfirmationSubPart change. It surfaced the raw input for plain-string confirmations, but that editor is writable while agent-host approval dispatch never sends editedToolInput, so edits were silently discarded. Translating the message removes the need for it, and this no longer touches shared rendering code. Also stop emitting the raw input for writes: it repeated the file the message already names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6359562 commit 965b5fb

3 files changed

Lines changed: 55 additions & 30 deletions

File tree

‎src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2334,12 +2334,11 @@ export function toolCallStateToInvocation(tc: ToolCallState, subAgentInvocationI
23342334
};
23352335
} else if (getToolKind(tc) === 'terminal' && getInlineToolInput(tc.toolInput)) {
23362336
toolSpecificData = buildTerminalToolSpecificData(tc, sessionResource);
2337-
} else {
2338-
const writePermission = remoteWritePermission(tc);
2337+
} else if (!remoteWriteInvocationMessage(tc)) {
2338+
// A write's only argument is the file the message already names,
2339+
// so there is nothing left for the raw input view to add.
23392340
const toolInput = getInlineToolInput(tc.toolInput);
2340-
if (writePermission) {
2341-
toolSpecificData = { kind: 'input', rawInput: writePermission.rawInput };
2342-
} else if (toolInput) {
2341+
if (toolInput) {
23432342
let rawInput: unknown;
23442343
try { rawInput = JSON.parse(toolInput); } catch { rawInput = { input: toolInput }; }
23452344
toolSpecificData = { kind: 'input', rawInput };
@@ -2348,7 +2347,7 @@ export function toolCallStateToInvocation(tc: ToolCallState, subAgentInvocationI
23482347

23492348
return new ChatToolInvocation(
23502349
{
2351-
invocationMessage: stringOrMarkdownToString(tc.invocationMessage, connectionAuthority),
2350+
invocationMessage: stringOrMarkdownToString(remoteWriteInvocationMessage(tc) ?? tc.invocationMessage, connectionAuthority),
23522351
confirmationMessages,
23532352
presentation: ToolInvocationPresentation.HiddenAfterComplete,
23542353
toolSpecificData,
@@ -2415,19 +2414,25 @@ export function toolCallStateToInvocation(tc: ToolCallState, subAgentInvocationI
24152414
}
24162415

24172416
/**
2418-
* A remote host names a write's target file in `_meta` and sends a plain-text
2419-
* message, where a local host sends the file link and a `{ path }` input.
2420-
* Restates the remote request in that same shape so both render identically.
2417+
* The invocation message for a remote host's write permission.
2418+
*
2419+
* A remote host names the target file in `_meta` and sends a plain-text
2420+
* message, where a local host sends the shared `Edit <file link>` message.
2421+
* Restating it in that form is what gives both hosts the same file pill, in
2422+
* the confirmation card and anywhere else the message is summarized.
24212423
*
2422-
* The link is left as a plain `file:` URI, exactly as a local host emits it;
2423-
* {@link stringOrMarkdownToString} rewrites it to address the remote host.
2424+
* The path is normalized the same way {@link parseAbsoluteFileLinkTarget}
2425+
* normalizes a remote link: `URI.file` only rewrites separators when the
2426+
* *client* runs Windows, so a Windows host paired with a non-Windows client
2427+
* would otherwise keep `C:\repo\file.ts` verbatim and yield a single-segment
2428+
* basename. The link itself stays a plain `file:` URI, exactly as a local host
2429+
* emits it; {@link stringOrMarkdownToString} rewrites it to address the host.
24242430
*/
2425-
function remoteWritePermission(tc: ToolCallPendingConfirmationState): { readonly message: StringOrMarkdown; readonly rawInput: { readonly path: string } } | undefined {
2431+
function remoteWriteInvocationMessage(tc: ToolCallPendingConfirmationState): StringOrMarkdown | undefined {
24262432
const { kind, fileName } = readAgentPermissionRequestMeta(tc);
2427-
if (kind !== AgentPermissionRequestKind.Write || !fileName) {
2428-
return undefined;
2429-
}
2430-
return { message: getEditFileMessage(fileName), rawInput: { path: fileName } };
2433+
return kind === AgentPermissionRequestKind.Write && fileName
2434+
? getEditFileMessage(fileName, path => win32.isAbsolute(path) ? path.replaceAll('\\', '/') : path)
2435+
: undefined;
24312436
}
24322437

24332438
export function toolCallConfirmationMessages(tc: ToolCallPendingConfirmationState, connectionAuthority: string): IToolConfirmationMessages {
@@ -2448,7 +2453,7 @@ export function toolCallConfirmationMessages(tc: ToolCallPendingConfirmationStat
24482453
: stringOrMarkdownToString(tc.confirmationTitle, connectionAuthority) ?? tc.displayName,
24492454
message: isViewUnreviewedCommentsTool(tc.toolName)
24502455
? localize('agentFeedback.reviewMessage', "Choose which comments to reveal to the agent. Unchecked comments stay hidden.")
2451-
: stringOrMarkdownToString(remoteWritePermission(tc)?.message ?? tc.invocationMessage, connectionAuthority),
2456+
: stringOrMarkdownToString(remoteWriteInvocationMessage(tc) ?? tc.invocationMessage, connectionAuthority),
24522457
approvalReason,
24532458
...(tc.options ? { customOptions: tc.options } : {}),
24542459
};

‎src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,8 @@ export class ToolConfirmationSubPart extends AbstractToolConfirmationSubPart {
157157
}
158158
const { message, disclaimer } = state.confirmationMessages!;
159159
const toolInvocation = this.toolInvocation as IChatToolInvocation;
160-
const hasRenderableInput = toolInvocation.toolSpecificData?.kind === 'input'
161-
&& !!toolInvocation.toolSpecificData.rawInput
162-
&& !isEmptyObject(toolInvocation.toolSpecificData.rawInput);
163-
164-
// A plain-string message alone needs no surrounding structure, but the
165-
// input editor below does — skipping it would hide what is being
166-
// approved.
167-
if (typeof message === 'string' && !disclaimer && !hasRenderableInput) {
160+
161+
if (typeof message === 'string' && !disclaimer) {
168162
return message;
169163
} else {
170164
const codeBlockRenderOptions: ICodeBlockRenderOptions = {

‎src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1615,8 +1615,9 @@ suite('stateToProgressAdapter', () => {
16151615

16161616
test('restates a remote host write permission in the local shape', () => {
16171617
// A remote host names the file in `_meta` and sends a plain-text
1618-
// message; a local host sends the shared `Edit <link>` message and a
1619-
// `{ path }` input. Both must render through the same path.
1618+
// message; a local host sends the shared `Edit <link>` message.
1619+
// Both the confirmation card and the sessions list read
1620+
// `invocationMessage`, so it must carry the file too.
16201621
const tc: ToolCallPendingConfirmationState = {
16211622
toolCallId: 'tc-write',
16221623
toolName: 'str_replace_editor',
@@ -1638,16 +1639,42 @@ suite('stateToProgressAdapter', () => {
16381639
: undefined;
16391640

16401641
assert.deepStrictEqual({
1642+
invocationMessage: typeof invocation.invocationMessage === 'string' ? invocation.invocationMessage : invocation.invocationMessage?.value,
16411643
markdown: typeof message === 'string' ? message : message?.value,
16421644
toolSpecificData: invocation.toolSpecificData,
16431645
}, {
16441646
// Link rewriting drops the label so the renderer shows a file
16451647
// widget; a local host's message collapses to the same form.
1648+
invocationMessage: 'Edit [](file:///workspaces/simple-server/index.js)',
16461649
markdown: 'Edit [](file:///workspaces/simple-server/index.js)',
1647-
toolSpecificData: { kind: 'input', rawInput: { path: '/workspaces/simple-server/index.js' } },
1650+
// The message names the file, so raw input would only repeat it.
1651+
toolSpecificData: undefined,
16481652
});
16491653
});
16501654

1655+
test('normalizes a Windows host path regardless of the client platform', () => {
1656+
// `URI.file` only rewrites separators when the *client* runs
1657+
// Windows, so a Windows host paired with a POSIX client would
1658+
// otherwise yield a single-segment basename.
1659+
const tc: ToolCallPendingConfirmationState = {
1660+
toolCallId: 'tc-write-win',
1661+
toolName: 'str_replace_editor',
1662+
displayName: 'Edit',
1663+
invocationMessage: 'Edit file',
1664+
status: ToolCallStatus.PendingConfirmation,
1665+
toolInput: 'C:\\repo\\src\\index.ts',
1666+
_meta: { requestId: 'req-w2', promptRequest: { kind: 'write', fileName: 'C:\\repo\\src\\index.ts' } },
1667+
};
1668+
1669+
const invocation = toolCallStateToInvocation(tc);
1670+
const message = invocation.invocationMessage;
1671+
1672+
assert.strictEqual(
1673+
typeof message === 'string' ? message : message?.value,
1674+
'Edit [](file:///c%3A/repo/src/index.ts)',
1675+
);
1676+
});
1677+
16511678
test('keeps the host message for a write permission that names no file', () => {
16521679
const tc: ToolCallPendingConfirmationState = {
16531680
toolCallId: 'tc-write-nofile',
@@ -1656,7 +1683,7 @@ suite('stateToProgressAdapter', () => {
16561683
invocationMessage: 'Edit file',
16571684
status: ToolCallStatus.PendingConfirmation,
16581685
toolInput: '{"path":"/tmp/a.txt"}',
1659-
_meta: { requestId: 'req-w2', promptRequest: { kind: 'write' } },
1686+
_meta: { requestId: 'req-w3', promptRequest: { kind: 'write' } },
16601687
};
16611688

16621689
const invocation = toolCallStateToInvocation(tc);
@@ -1678,7 +1705,6 @@ suite('stateToProgressAdapter', () => {
16781705
const tc = createToolCallState({
16791706
_meta: { toolKind: 'subagent', subagentDescription: 'Review code', subagentAgentName: 'code-reviewer' },
16801707
});
1681-
16821708
const invocation = toolCallStateToInvocation(tc);
16831709
assert.ok(invocation.toolSpecificData);
16841710
assert.strictEqual(invocation.toolSpecificData.kind, 'subagent');

0 commit comments

Comments
 (0)