From e1b068004af0bbc066097ce4e41d2619c2b32869 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Sat, 25 Apr 2026 13:55:49 -0700 Subject: [PATCH 1/2] Render copilot skill invocations as a tool call The Copilot SDK now exposes a skill tool that the model uses to load SKILL.md files. Until now this surfaced in the agent host as the default tool UI, which doesn't fit. This change does three things: - Hide the raw `skill` tool call (added to HIDDEN_TOOL_NAMES). - Synthesize a `tool_start`/`tool_complete` pair from the SDK's `skill.invoked` lifecycle event so we can show 'Reading [name]' with a link to the skill file. Wired in both the live event path and the history-replay path so reconnect renders identically. - Filter out the synthetic `user.message` events the SDK sometimes injects with skill content (data.source !== 'user'). Client side, the chat adapter now tags markdown links pointing at a SKILL.md file with `?vscodeLinkType=skill` so they render as the inline skill pill instead of a plain markdown anchor. Doing this on the client (rather than at the agent host) keeps the agent host host-agnostic and also upgrades older sessions and other providers that emit SKILL.md links. The link rewriter also now preserves the original link text rather than always collapsing to `[](newHref)`, so a skill named 'plan' shows up as 'Reading [plan]' rather than 'Reading [SKILL.md]'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../node/copilot/copilotAgentSession.ts | 13 ++- .../node/copilot/copilotToolDisplay.ts | 87 ++++++++++++++++++- .../node/copilot/mapSessionEvents.ts | 45 +++++++++- .../test/node/copilotToolDisplay.test.ts | 48 +++++++++- .../test/node/mapSessionEvents.test.ts | 62 +++++++++++++ .../agentHost/stateToProgressAdapter.ts | 37 ++++++-- .../stateToProgressAdapter.test.ts | 12 +-- 7 files changed, 286 insertions(+), 18 deletions(-) diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts index 08684c105778e..a4a546174d5a3 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts @@ -24,7 +24,7 @@ import type { FileEdit, ToolDefinition } from '../../common/state/protocol/state import { SessionInputAnswerState, SessionInputAnswerValueKind, SessionInputQuestionKind, SessionInputResponseKind, ToolResultContentType, type PendingMessage, type SessionInputAnswer, type SessionInputRequest, type ToolCallResult, type ToolResultContent } from '../../common/state/sessionState.js'; import { CopilotSessionWrapper } from './copilotSessionWrapper.js'; import type { ShellManager } from './copilotShellTools.js'; -import { getEditFilePath, getInvocationMessage, getPastTenseMessage, getPermissionDisplay, getShellLanguage, getSubagentMetadata, getToolDisplayName, getToolInputString, getToolKind, isEditTool, isHiddenTool, isShellTool, tryStringify, type ITypedPermissionRequest } from './copilotToolDisplay.js'; +import { getEditFilePath, getInvocationMessage, getPastTenseMessage, getPermissionDisplay, getShellLanguage, getSubagentMetadata, getToolDisplayName, getToolInputString, getToolKind, isEditTool, isHiddenTool, isShellTool, synthesizeSkillToolEvents, tryStringify, type ITypedPermissionRequest } from './copilotToolDisplay.js'; import { FileEditTracker } from './fileEditTracker.js'; import { mapSessionEvents } from './mapSessionEvents.js'; import { buildPendingEditContentUri } from './pendingEditContentStore.js'; @@ -793,6 +793,17 @@ export class CopilotAgentSession extends Disposable { this._onDidSessionProgress.fire({ session, type: 'idle' }); })); + // The SDK emits a `skill` tool call (which we hide) and a richer + // `skill.invoked` event with the resolved SKILL.md path. Synthesize a + // tool-start/complete pair from the latter so the UI can render a + // clickable file link, matching the `view`-tool display style. + this._register(wrapper.onSkillInvoked(e => { + this._logService.info(`[Copilot:${sessionId}] Skill invoked: ${e.data.name} (${e.data.path})`); + const { start, complete } = synthesizeSkillToolEvents(session, e.data, e.id); + this._onDidSessionProgress.fire(start); + this._onDidSessionProgress.fire(complete); + })); + this._register(wrapper.onSubagentStarted(e => { this._logService.info(`[Copilot:${sessionId}] Subagent started: toolCallId=${e.data.toolCallId}, agent=${e.data.agentName}`); this._onDidSessionProgress.fire({ diff --git a/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts b/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts index a1428617afff2..341eac7a41a60 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts @@ -8,7 +8,7 @@ import { hasKey } from '../../../../base/common/types.js'; import { URI } from '../../../../base/common/uri.js'; import { appendEscapedMarkdownInlineCode } from '../../../../base/common/htmlContent.js'; import { localize } from '../../../../nls.js'; -import type { IAgentToolReadyEvent } from '../../common/agentService.js'; +import type { IAgentToolCompleteEvent, IAgentToolReadyEvent, IAgentToolStartEvent } from '../../common/agentService.js'; import { stripRedundantCdPrefix } from '../../common/commandLineHelpers.js'; import { StringOrMarkdown } from '../../common/state/protocol/state.js'; import { basename } from '../../../../base/common/resources.js'; @@ -55,6 +55,7 @@ const enum CopilotToolName { WebFetch = 'web_fetch', AskUser = 'ask_user', ReportIntent = 'report_intent', + Skill = 'skill', } /** Parameters for the `bash` / `powershell` shell tools. */ @@ -170,9 +171,16 @@ const SUBAGENT_TOOL_NAMES: ReadonlySet = new Set([ /** * Tools that should not be shown to the user. These are internal tools * used by the CLI for its own purposes (e.g., reporting intent to the model). + * + * `skill` is hidden because the SDK already emits a richer `skill.invoked` + * lifecycle event with the resolved skill file path; the agent session + * synthesizes a tool-start/complete pair from that event so the UI can + * render a clickable file link instead of just the skill name. See + * {@link synthesizeSkillToolEvents}. */ const HIDDEN_TOOL_NAMES: ReadonlySet = new Set([ CopilotToolName.ReportIntent, + CopilotToolName.Skill, ]); /** @@ -398,6 +406,83 @@ export function getPastTenseMessage(toolName: string, displayName: string, param } } +// ============================================================================= +// Skill event synthesis +// +// The Copilot SDK emits a `skill` tool call (which we hide) and, separately, a +// `skill.invoked` lifecycle event with the resolved skill file path. We turn +// the latter into a synthesized tool-start/complete pair so clients can render +// a clickable file link to the SKILL.md the agent loaded -- matching the +// existing `view`-tool display style. Live and replay paths share this helper +// so they stay in lock-step (see also the mirrored-pair gotcha for tool-call +// display in this file). +// ============================================================================= + +/** Subset of the SDK's `skill.invoked` payload that the synth helper needs. */ +export interface ICopilotSkillInvokedData { + readonly name: string; + readonly path?: string; + readonly description?: string; +} + +/** + * Builds a stable synthetic tool call id for a `skill.invoked` event so + * reconnect/replay produces the same id as the original live emit. + */ +export function getSkillSyntheticToolCallId(eventId: string | undefined, data: ICopilotSkillInvokedData): string { + if (eventId) { + return `synth-skill-${eventId}`; + } + if (data.path) { + return `synth-skill-${data.path}`; + } + return `synth-skill-${data.name}`; +} + +/** + * Synthesizes the `tool_start` and `tool_complete` agent progress events that + * represent a successful `skill.invoked` lifecycle event. Used by both the + * live session handler and the history-replay mapper so the two paths render + * identically. + */ +export function synthesizeSkillToolEvents( + session: URI, + data: ICopilotSkillInvokedData, + eventId: string | undefined, +): { start: IAgentToolStartEvent; complete: IAgentToolCompleteEvent } { + const toolCallId = getSkillSyntheticToolCallId(eventId, data); + const displayName = localize('toolName.skill', "Read Skill"); + // Use the skill name as the link text rather than the basename: every skill + // file is named SKILL.md, so `Reading [plan]` reads better than the + // always-identical `Reading [SKILL.md]`. The client may further upgrade this + // link to a rich pill based on the `SKILL.md` basename. + const skillLink = data.path ? `[${data.name}](${URI.file(data.path)})` : undefined; + const invocationMessage: StringOrMarkdown = skillLink + ? md(localize('toolInvoke.skill', "Reading skill {0}", skillLink)) + : localize('toolInvoke.skillName', "Reading skill {0}", data.name); + const pastTenseMessage: StringOrMarkdown = skillLink + ? md(localize('toolComplete.skill', "Read skill {0}", skillLink)) + : localize('toolComplete.skillName', "Read skill {0}", data.name); + const start: IAgentToolStartEvent = { + session, + type: 'tool_start', + toolCallId, + toolName: CopilotToolName.Skill, + displayName, + invocationMessage, + }; + const complete: IAgentToolCompleteEvent = { + session, + type: 'tool_complete', + toolCallId, + result: { + success: true, + pastTenseMessage, + }, + }; + return { start, complete }; +} + export function getToolInputString(toolName: string, parameters: Record | undefined, rawArguments: string | undefined): string | undefined { if (!parameters && !rawArguments) { return undefined; diff --git a/src/vs/platform/agentHost/node/copilot/mapSessionEvents.ts b/src/vs/platform/agentHost/node/copilot/mapSessionEvents.ts index 551ff04b11346..6e224c3428ab0 100644 --- a/src/vs/platform/agentHost/node/copilot/mapSessionEvents.ts +++ b/src/vs/platform/agentHost/node/copilot/mapSessionEvents.ts @@ -8,7 +8,7 @@ import { IAgentMessageEvent, IAgentSubagentStartedEvent, IAgentToolCompleteEvent import { stripRedundantCdPrefix } from '../../common/commandLineHelpers.js'; import { IFileEditRecord, ISessionDatabase } from '../../common/sessionDataService.js'; import { ToolResultContentType, type ToolResultContent } from '../../common/state/sessionState.js'; -import { getInvocationMessage, getPastTenseMessage, getShellLanguage, getSubagentMetadata, getToolDisplayName, getToolInputString, getToolKind, isEditTool, isHiddenTool } from './copilotToolDisplay.js'; +import { getInvocationMessage, getPastTenseMessage, getShellLanguage, getSubagentMetadata, getToolDisplayName, getToolInputString, getToolKind, isEditTool, isHiddenTool, synthesizeSkillToolEvents } from './copilotToolDisplay.js'; import { buildSessionDbUri } from './fileEditTracker.js'; function tryStringify(value: unknown): string | undefined { @@ -58,11 +58,45 @@ export interface ISessionEventMessage { reasoningText?: string; encryptedContent?: string; parentToolCallId?: string; + /** + * Origin of this message. The SDK sets this to a non-`'user'` value + * (e.g. `'skill-pdf'`) for messages it injects on behalf of a skill or + * other internal mechanism. We filter those out so they don't render + * as user turns. + */ + source?: string; }; } +/** Minimal event shape for `skill.invoked`, used to synthesize a tool-style render. */ +export interface ISessionEventSkillInvoked { + type: 'skill.invoked'; + id?: string; + data: { + name: string; + path?: string; + description?: string; + }; +} + +/** + * Returns true if the event is a SDK-injected `user.message` that should not + * be shown to the user (e.g. skill-content injection). + * + * The SDK marks these via a non-`'user'` `source` field. Older sessions + * persisted before `source` existed will not be filtered; that is accepted + * leakage rather than guessed-at content sniffing. + */ +export function isSyntheticUserMessage(event: ISessionEvent): boolean { + if (event.type !== 'user.message') { + return false; + } + const source = (event as ISessionEventMessage).data?.source; + return !!source && source.toLowerCase() !== 'user'; +} + /** Minimal event shape for session history mapping. */ -export type ISessionEvent = ISessionEventToolStart | ISessionEventToolComplete | ISessionEventMessage | ISessionEventSubagentStarted | { type: string; data?: unknown }; +export type ISessionEvent = ISessionEventToolStart | ISessionEventToolComplete | ISessionEventMessage | ISessionEventSubagentStarted | ISessionEventSkillInvoked | { type: string; data?: unknown }; export interface ISessionEventSubagentStarted { type: 'subagent.started'; @@ -143,6 +177,9 @@ export async function mapSessionEvents( // Second pass: build result events for (const e of events) { if (e.type === 'assistant.message' || e.type === 'user.message') { + if (isSyntheticUserMessage(e)) { + continue; + } const d = (e as ISessionEventMessage).data; result.push({ session, @@ -253,6 +290,10 @@ export async function mapSessionEvents( agentDisplayName: d.agentDisplayName, agentDescription: d.agentDescription, }); + } else if (e.type === 'skill.invoked') { + const skillEvent = e as ISessionEventSkillInvoked; + const { start, complete } = synthesizeSkillToolEvents(session, skillEvent.data, skillEvent.id); + result.push(start, complete); } } return result; diff --git a/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts b/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts index 86d2c10c713ff..880da888258d9 100644 --- a/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts @@ -6,7 +6,7 @@ import assert from 'assert'; import { URI } from '../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { getInvocationMessage, getPastTenseMessage, getPermissionDisplay, getShellLanguage, getToolInputString, getToolKind, type ITypedPermissionRequest } from '../../node/copilot/copilotToolDisplay.js'; +import { getInvocationMessage, getPastTenseMessage, getPermissionDisplay, getShellLanguage, getToolInputString, getToolKind, isHiddenTool, synthesizeSkillToolEvents, type ITypedPermissionRequest } from '../../node/copilot/copilotToolDisplay.js'; suite('getPermissionDisplay — cd-prefix stripping', () => { @@ -293,3 +293,49 @@ suite('copilotToolDisplay — write_/read_ shell tools', () => { }); }); }); + +suite('skill events', () => { + + ensureNoDisposablesAreLeakedInTestSuite(); + + const session = URI.parse('agent://copilot/test'); + + test('hides the raw `skill` tool call and synthesizes a tool-start/complete pair from `skill.invoked`', () => { + const withPath = synthesizeSkillToolEvents( + session, + { name: 'plan', path: '/abs/repo/skills/plan/SKILL.md' }, + 'evt-123', + ); + const noPath = synthesizeSkillToolEvents( + session, + { name: 'plan' }, + undefined, + ); + + assert.deepStrictEqual({ + skillIsHidden: isHiddenTool('skill'), + withPathToolCallId: withPath.start.toolCallId, + withPathSameIdOnComplete: withPath.start.toolCallId === withPath.complete.toolCallId, + withPathToolName: withPath.start.toolName, + withPathDisplayName: withPath.start.displayName, + withPathInvocation: withPath.start.invocationMessage, + withPathPastTense: withPath.complete.result.pastTenseMessage, + withPathSuccess: withPath.complete.result.success, + noPathToolCallId: noPath.start.toolCallId, + noPathInvocation: noPath.start.invocationMessage, + noPathPastTense: noPath.complete.result.pastTenseMessage, + }, { + skillIsHidden: true, + withPathToolCallId: 'synth-skill-evt-123', + withPathSameIdOnComplete: true, + withPathToolName: 'skill', + withPathDisplayName: 'Read Skill', + withPathInvocation: { markdown: 'Reading [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + withPathPastTense: { markdown: 'Read [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + withPathSuccess: true, + noPathToolCallId: 'synth-skill-plan', + noPathInvocation: 'Reading skill plan', + noPathPastTense: 'Read skill plan', + }); + }); +}); diff --git a/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts b/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts index 046fde01db6fc..a0b7bd349c236 100644 --- a/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts +++ b/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts @@ -254,6 +254,68 @@ suite('mapSessionEvents', () => { }); }); + // ---- Skill events --------------------------------------------------- + + suite('skill events', () => { + + test('synthesizes tool start/complete from skill.invoked and filters synthetic skill-injected user messages', async () => { + const events: ISessionEvent[] = [ + { + type: 'tool.execution_start', + data: { toolCallId: 'tc-skill', toolName: 'skill', arguments: { skill: 'plan' } }, + }, + { + type: 'tool.execution_complete', + data: { toolCallId: 'tc-skill', success: true }, + }, + { + type: 'skill.invoked', + id: 'evt-42', + data: { name: 'plan', path: '/abs/repo/skills/plan/SKILL.md' }, + }, + { + type: 'user.message', + data: { messageId: 'msg-skill', content: '', source: 'skill-plan' }, + }, + { + type: 'assistant.message', + data: { messageId: 'msg-1', content: 'ok' }, + }, + ]; + + const result = await mapSessionEvents(session, undefined, events); + + assert.deepStrictEqual({ + count: result.length, + types: result.map(r => r.type), + skillStart: result[0], + skillComplete: result[1], + assistantRole: (result[2] as { role: string }).role, + }, { + count: 3, + types: ['tool_start', 'tool_complete', 'message'], + skillStart: { + session, + type: 'tool_start', + toolCallId: 'synth-skill-evt-42', + toolName: 'skill', + displayName: 'Read Skill', + invocationMessage: { markdown: 'Reading [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + }, + skillComplete: { + session, + type: 'tool_complete', + toolCallId: 'synth-skill-evt-42', + result: { + success: true, + pastTenseMessage: { markdown: 'Read [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + }, + }, + assistantRole: 'assistant', + }); + }); + }); + // ---- cd-prefix rewriting -------------------------------------------- suite('cd-prefix rewriting', () => { diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts index d29664a24bd0c..cad110fc14e55 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts @@ -411,11 +411,13 @@ export function rewriteMarkdownLinks(markdown: string, connectionAuthority: stri * or returns `undefined` if the token should be left alone (external * scheme or unparseable URI). * - * The output collapses to the canonical inline form `[](newHref)` (or - * `![](newHref)` for images) — the chat renderer has richer handling for - * empty-text agent-host links, so preserving the original label isn't - * useful. This also means autolinks (``) and reference-style links - * (`[text][ref]`) are normalized into the inline form. + * The output is the canonical inline form `[text](newHref)` (or + * `![text](newHref)` for images). When the original link has no display + * text the chat renderer falls back to its file-widget rendering, but a + * non-empty label (e.g. a skill name) is preserved so the renderer shows + * it instead of the URI's basename. This also means autolinks (``) + * and reference-style links (`[text][ref]`) are normalized into the + * inline form. */ function rewriteLinkTokenRaw(token: Tokens.Link | Tokens.Image, connectionAuthority: string): string | undefined { let parsed: URI; @@ -428,9 +430,30 @@ function rewriteLinkTokenRaw(token: Tokens.Link | Tokens.Image, connectionAuthor if (!scheme || EXTERNAL_LINK_SCHEMES.has(scheme)) { return undefined; } - const newHref = toAgentHostUri(parsed, connectionAuthority).toString(); + let agentHostUri = toAgentHostUri(parsed, connectionAuthority); + // VS-Code-specific: links pointing at a `SKILL.md` file are rendered as a + // rich skill pill rather than a plain markdown link. The chat renderer's + // inline anchor widget keys off the `vscodeLinkType` query parameter (see + // `chatInlineAnchorWidget.ts`), so we tag the URI here on the client side + // rather than at the agent host. We do this whether or not the link came + // in pre-tagged so older sessions and other agent providers also benefit. + if (isSkillFileUri(parsed) && !agentHostUri.query.includes('vscodeLinkType=')) { + const existing = agentHostUri.query; + agentHostUri = agentHostUri.with({ query: existing ? `${existing}&vscodeLinkType=skill` : 'vscodeLinkType=skill' }); + } const prefix = token.type === 'image' ? '![' : '['; - return `${prefix}](${newHref})`; + const text = token.text ?? ''; + return `${prefix}${text}](${agentHostUri.toString()})`; +} + +/** + * Returns true when the URI's basename is `SKILL.md` (case-insensitive). + * Used to tag skill links so the chat renderer shows the rich skill pill + * instead of a plain markdown anchor. + */ +function isSkillFileUri(uri: URI): boolean { + const name = basename(uri); + return name.toLowerCase() === 'skill.md'; } /** diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts index dcd44ed099333..1888763ae14ff 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts @@ -244,8 +244,8 @@ suite('stateToProgressAdapter', () => { if (response.type !== 'response') { return; } const part = response.parts[0] as IChatMarkdownContent; assert.deepStrictEqual(part.content.value, - 'See [](vscode-agent-host://my-host/file/-/a/b.ts), ' + - '[](vscode-agent-host://my-host/agenthost-content/-/s/x), ' + + 'See [local](vscode-agent-host://my-host/file/-/a/b.ts), ' + + '[content](vscode-agent-host://my-host/agenthost-content/-/s/x), ' + '[external](https://example.com) and ' + '[rel](./foo.md).' ); @@ -270,8 +270,8 @@ suite('stateToProgressAdapter', () => { assert.strictEqual(response.type, 'response'); if (response.type !== 'response') { return; } const value = (response.parts[0] as IChatMarkdownContent).content.value; - assert.ok(value.includes('[](vscode-agent-host://my-host/file/-/a.ts)')); - assert.ok(value.includes('[](vscode-agent-host://my-host/file/-/c.ts)')); + assert.ok(value.includes('[real](vscode-agent-host://my-host/file/-/a.ts)')); + assert.ok(value.includes('[another](vscode-agent-host://my-host/file/-/c.ts)')); // The link inside the fenced code block must NOT be rewritten. assert.ok(value.includes('[fake](file:///b.ts)')); assert.ok(!value.includes('[fake](vscode-agent-host')); @@ -289,7 +289,7 @@ suite('stateToProgressAdapter', () => { if (response.type !== 'response') { return; } const value = (response.parts[0] as IChatMarkdownContent).content.value; assert.strictEqual(value, - 'Real [](vscode-agent-host://my-host/file/-/a.ts) and literal `[two](file:///b.ts)` here.' + 'Real [one](vscode-agent-host://my-host/file/-/a.ts) and literal `[two](file:///b.ts)` here.' ); }); @@ -415,7 +415,7 @@ suite('stateToProgressAdapter', () => { assert.ok(invocation.pastTenseMessage); assert.strictEqual(typeof invocation.pastTenseMessage, 'object'); const value = (invocation.pastTenseMessage as { value: string }).value; - assert.strictEqual(value, 'Read [](vscode-agent-host://ssh__macbook-air/file/-/path/to/foo.ts)'); + assert.strictEqual(value, 'Read [foo.ts](vscode-agent-host://ssh__macbook-air/file/-/path/to/foo.ts)'); }); test('finalizes terminal tool with output and exit code', () => { From 89433a4490f804019db1a686825ce24e38890279 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Sat, 25 Apr 2026 14:32:37 -0700 Subject: [PATCH 2/2] Address review comments and fix CI snapshots - Add escapeMarkdownLinkLabel helper in htmlContent.ts: only escapes '\' and ']' so link labels rendered without re-parsing markdown (e.g. the chat skill pill) don't show literal backslashes for safe characters like '-' or '.'. - Use it in synthesizeSkillToolEvents and stateToProgressAdapter's rewriteLinkTokenRaw instead of the over-eager escapeMarkdownSyntaxTokens. - Hash the path/name fallback in getSkillSyntheticToolCallId so the synthesized toolCallId never contains '/' (which breaks ChatResponseResource.createUri paths). - Update copilotToolDisplay/mapSessionEvents test snapshots to match the actual emitted strings ('Reading skill [plan](...)' / 'Read skill [plan](...)') and the new hash-based fallback id. - Add htmlContent unit tests for escapeMarkdownLinkLabel. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/vs/base/common/htmlContent.ts | 12 ++++++++ src/vs/base/test/common/htmlContent.test.ts | 23 ++++++++++++++- .../node/copilot/copilotToolDisplay.ts | 29 ++++++++++++------- .../test/node/copilotToolDisplay.test.ts | 6 ++-- .../test/node/mapSessionEvents.test.ts | 4 +-- .../agentHost/stateToProgressAdapter.ts | 8 +++-- 6 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/vs/base/common/htmlContent.ts b/src/vs/base/common/htmlContent.ts index a9b07f6e0b83e..31acfa88384dc 100644 --- a/src/vs/base/common/htmlContent.ts +++ b/src/vs/base/common/htmlContent.ts @@ -155,6 +155,18 @@ export function escapeMarkdownSyntaxTokens(text: string): string { return text.replace(/[\\`*_{}[\]()#+\-!~]/g, '\\$&'); // CodeQL [SM02383] Backslash is escaped in the character class } +/** + * Escapes only the characters that would break out of markdown link text + * (`[label](url)`) syntax: `\` and `]`. Use this when the escaped string is + * displayed as the visible label of a link, since renderers that extract the + * link text without re-parsing markdown (e.g. the chat inline anchor / skill + * pill) would otherwise show full `escapeMarkdownSyntaxTokens` backslashes + * (`\-`, `\.`, ...) verbatim. + */ +export function escapeMarkdownLinkLabel(text: string): string { + return text.replace(/[\\\]]/g, '\\$&'); +} + /** * @see https://github.com/microsoft/vscode/issues/193746 */ diff --git a/src/vs/base/test/common/htmlContent.test.ts b/src/vs/base/test/common/htmlContent.test.ts index 82a84971a523c..e3894cd19dc05 100644 --- a/src/vs/base/test/common/htmlContent.test.ts +++ b/src/vs/base/test/common/htmlContent.test.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import assert from 'assert'; -import { appendEscapedMarkdownInlineCode } from '../../common/htmlContent.js'; +import { appendEscapedMarkdownInlineCode, escapeMarkdownLinkLabel } from '../../common/htmlContent.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from './utils.js'; suite('htmlContent', () => { @@ -37,4 +37,25 @@ suite('htmlContent', () => { assert.strictEqual(appendEscapedMarkdownInlineCode('``'), '``` `` ```'); }); }); + + suite('escapeMarkdownLinkLabel', () => { + test('passes plain text through unchanged', () => { + assert.strictEqual(escapeMarkdownLinkLabel('hello'), 'hello'); + assert.strictEqual(escapeMarkdownLinkLabel(''), ''); + assert.strictEqual(escapeMarkdownLinkLabel('heap-snapshot-analysis'), 'heap-snapshot-analysis'); + assert.strictEqual(escapeMarkdownLinkLabel('foo.bar_baz'), 'foo.bar_baz'); + }); + + test('escapes only `\\` and `]`', () => { + assert.strictEqual(escapeMarkdownLinkLabel('a]b'), 'a\\]b'); + assert.strictEqual(escapeMarkdownLinkLabel('a\\b'), 'a\\\\b'); + assert.strictEqual(escapeMarkdownLinkLabel(']]'), '\\]\\]'); + }); + + test('does not escape characters that are safe in link text', () => { + // these would be escaped by escapeMarkdownSyntaxTokens but must + // pass through here since they render literally inside `[...]`. + assert.strictEqual(escapeMarkdownLinkLabel('a*b_c#d-e.f!g~h+i(j)k{l}m'), 'a*b_c#d-e.f!g~h+i(j)k{l}m'); + }); + }); }); diff --git a/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts b/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts index 341eac7a41a60..2d061529b7601 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts @@ -6,7 +6,8 @@ import type { PermissionRequest } from '@github/copilot-sdk'; import { hasKey } from '../../../../base/common/types.js'; import { URI } from '../../../../base/common/uri.js'; -import { appendEscapedMarkdownInlineCode } from '../../../../base/common/htmlContent.js'; +import { appendEscapedMarkdownInlineCode, escapeMarkdownLinkLabel } from '../../../../base/common/htmlContent.js'; +import { hash } from '../../../../base/common/hash.js'; import { localize } from '../../../../nls.js'; import type { IAgentToolCompleteEvent, IAgentToolReadyEvent, IAgentToolStartEvent } from '../../common/agentService.js'; import { stripRedundantCdPrefix } from '../../common/commandLineHelpers.js'; @@ -427,16 +428,17 @@ export interface ICopilotSkillInvokedData { /** * Builds a stable synthetic tool call id for a `skill.invoked` event so - * reconnect/replay produces the same id as the original live emit. + * reconnect/replay produces the same id as the original live emit. The id + * is used unencoded as a path segment (e.g. by `ChatResponseResource.createUri`), + * so it must not contain characters like `/` -- we hash any fallback values + * that could carry filesystem paths or arbitrary text. */ export function getSkillSyntheticToolCallId(eventId: string | undefined, data: ICopilotSkillInvokedData): string { if (eventId) { return `synth-skill-${eventId}`; } - if (data.path) { - return `synth-skill-${data.path}`; - } - return `synth-skill-${data.name}`; + const seed = data.path ?? data.name; + return `synth-skill-${hash(seed).toString(16)}`; } /** @@ -453,10 +455,17 @@ export function synthesizeSkillToolEvents( const toolCallId = getSkillSyntheticToolCallId(eventId, data); const displayName = localize('toolName.skill', "Read Skill"); // Use the skill name as the link text rather than the basename: every skill - // file is named SKILL.md, so `Reading [plan]` reads better than the - // always-identical `Reading [SKILL.md]`. The client may further upgrade this - // link to a rich pill based on the `SKILL.md` basename. - const skillLink = data.path ? `[${data.name}](${URI.file(data.path)})` : undefined; + // file is named SKILL.md, so `Reading skill [plan]` reads better than the + // always-identical `Reading skill [SKILL.md]`. The client may further upgrade + // this link to a rich pill based on the `SKILL.md` basename. Skill names and + // paths come from the SDK / agent host and are escaped to prevent markdown + // injection from a malicious skill author. + // Escape only the characters that would break out of markdown link text + // syntax (`\` and `]`); a full markdown escape would leave visible + // backslashes in renderers (like the skill pill) that extract link text + // without re-parsing markdown. + const escapedName = escapeMarkdownLinkLabel(data.name); + const skillLink = data.path ? `[${escapedName}](${URI.file(data.path)})` : undefined; const invocationMessage: StringOrMarkdown = skillLink ? md(localize('toolInvoke.skill', "Reading skill {0}", skillLink)) : localize('toolInvoke.skillName', "Reading skill {0}", data.name); diff --git a/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts b/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts index 880da888258d9..07d9471f38bca 100644 --- a/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts @@ -330,10 +330,10 @@ suite('skill events', () => { withPathSameIdOnComplete: true, withPathToolName: 'skill', withPathDisplayName: 'Read Skill', - withPathInvocation: { markdown: 'Reading [plan](file:///abs/repo/skills/plan/SKILL.md)' }, - withPathPastTense: { markdown: 'Read [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + withPathInvocation: { markdown: 'Reading skill [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + withPathPastTense: { markdown: 'Read skill [plan](file:///abs/repo/skills/plan/SKILL.md)' }, withPathSuccess: true, - noPathToolCallId: 'synth-skill-plan', + noPathToolCallId: 'synth-skill-2108d652', noPathInvocation: 'Reading skill plan', noPathPastTense: 'Read skill plan', }); diff --git a/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts b/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts index a0b7bd349c236..d5105061035fb 100644 --- a/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts +++ b/src/vs/platform/agentHost/test/node/mapSessionEvents.test.ts @@ -300,7 +300,7 @@ suite('mapSessionEvents', () => { toolCallId: 'synth-skill-evt-42', toolName: 'skill', displayName: 'Read Skill', - invocationMessage: { markdown: 'Reading [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + invocationMessage: { markdown: 'Reading skill [plan](file:///abs/repo/skills/plan/SKILL.md)' }, }, skillComplete: { session, @@ -308,7 +308,7 @@ suite('mapSessionEvents', () => { toolCallId: 'synth-skill-evt-42', result: { success: true, - pastTenseMessage: { markdown: 'Read [plan](file:///abs/repo/skills/plan/SKILL.md)' }, + pastTenseMessage: { markdown: 'Read skill [plan](file:///abs/repo/skills/plan/SKILL.md)' }, }, }, assistantRole: 'assistant', diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts index cad110fc14e55..0234eacc1e399 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { IMarkdownString, MarkdownString } from '../../../../../../base/common/htmlContent.js'; +import { escapeMarkdownLinkLabel, IMarkdownString, MarkdownString } from '../../../../../../base/common/htmlContent.js'; import { marked, type Token, type Tokens, type TokensList } from '../../../../../../base/common/marked/marked.js'; import { URI } from '../../../../../../base/common/uri.js'; import { ToolCallStatus, TurnState, ResponsePartKind, getToolFileEdits, getToolOutputText, getToolSubagentContent, type ActiveTurn, type ICompletedToolCall, type ToolCallState, type Turn, FileEditKind, ToolResultContentType, type ToolResultContent } from '../../../../../../platform/agentHost/common/state/sessionState.js'; @@ -442,7 +442,11 @@ function rewriteLinkTokenRaw(token: Tokens.Link | Tokens.Image, connectionAuthor agentHostUri = agentHostUri.with({ query: existing ? `${existing}&vscodeLinkType=skill` : 'vscodeLinkType=skill' }); } const prefix = token.type === 'image' ? '![' : '['; - const text = token.text ?? ''; + // Escape only the characters that would break out of markdown link text + // syntax (`\` and `]`). A full markdown escape would leave visible + // backslashes in renderers (like the skill pill) that extract link text + // without re-parsing markdown. + const text = escapeMarkdownLinkLabel(token.text ?? ''); return `${prefix}${text}](${agentHostUri.toString()})`; }