Skip to content

Commit 198313b

Browse files
Rank the no-account copy as the buttons rank it, and harden its links
Read the sentence in the order the routes are weighted: GitHub sign-in leads, as the primary button; the provider sign-in follows; reload and docs trail, being the copy's only links rather than buttons. Reload and docs become their own sentences — kept as trailing clauses they would have fallen under the "if you already set up Claude elsewhere" conditional, which does not scope docs. Addresses review feedback: build both `command:` hrefs through `createCommandUri` instead of by hand (`encodeURIComponent` leaves `)` alone, so an agent id containing one closed the markdown link destination early), and escape the host-supplied display name and sign-in provider before interpolating them into markdown this banner trusts for two commands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent e223404 commit 198313b

2 files changed

Lines changed: 45 additions & 33 deletions

File tree

src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSdkSetupNotification.ts

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import { Disposable, DisposableStore } from '../../../../../../base/common/lifecycle.js';
77
import { Event } from '../../../../../../base/common/event.js';
8-
import { IMarkdownString, MarkdownString } from '../../../../../../base/common/htmlContent.js';
8+
import { createCommandUri, escapeMarkdownSyntaxTokens, IMarkdownString, MarkdownString } from '../../../../../../base/common/htmlContent.js';
99
import { localize } from '../../../../../../nls.js';
1010
import { AgentHostAllowSignedOutWhenUsableSettingId, IAgentHostService } from '../../../../../../platform/agentHost/common/agentService.js';
1111
import { LOCAL_AGENT_HOST_SCHEME_PREFIX } from '../../../../../../platform/agentHost/common/agentHostConnectionsService.js';
@@ -85,14 +85,6 @@ export function getAgentSdkSetupStateToReport(previous: AgentSdkSetupState | und
8585

8686
// #region Banner
8787

88-
/**
89-
* A `command:` href carrying the agent id, so a link in the copy takes the same
90-
* route a button would — funnel step and URL validation included.
91-
*/
92-
function setupCommandLink(commandId: string, agent: string): string {
93-
return `command:${commandId}?${encodeURIComponent(JSON.stringify(agent))}`;
94-
}
95-
9688
/** Trusted for the commands its links address, and nothing else. */
9789
function setupMarkdown(value: string): MarkdownString {
9890
return new MarkdownString(value, { isTrusted: { enabledCommands: [AGENT_SDK_SETUP_OPEN_DOCS_COMMAND_ID, AGENT_SDK_SETUP_RELOAD_COMMAND_ID] } });
@@ -101,29 +93,32 @@ function setupMarkdown(value: string): MarkdownString {
10193
/**
10294
* The "no account" second line: one whole sentence per combination of routes,
10395
* never assembled from localized fragments, because clause order is not stable
104-
* across languages. The GitHub clause is unconditional — every agent behind this
105-
* banner reaches models through our Copilot proxy once signed in, which is
106-
* workbench knowledge rather than something an agent could declare.
107-
*
108-
* Reload leads every variant: setup finished in a terminal has no completion
109-
* signal, so the user who has already done it should not read the routes at all.
110-
* Docs are a link inside the sentence that already explains them rather than a
111-
* third button competing with the two routes that actually sign you in.
96+
* across languages. The GitHub clause is unconditional and leads: it is the
97+
* primary button, and reaching models through our Copilot proxy is workbench
98+
* knowledge rather than something an agent could declare. Reload and docs trail
99+
* the sign-in routes, ranked as the copy's only links rather than buttons.
112100
*/
113101
function noAccountDescription(setup: IAgentSdkSetupInfo, displayName: string): IMarkdownString {
114-
const provider = setup.signInProviderName;
115-
const reload = setupCommandLink(AGENT_SDK_SETUP_RELOAD_COMMAND_ID, setup.agent);
116-
const docs = setup.setupDocsUrl ? setupCommandLink(AGENT_SDK_SETUP_OPEN_DOCS_COMMAND_ID, setup.agent) : undefined;
102+
// Both nouns are the host's, and this string is trusted for two commands, so
103+
// they are escaped rather than interpolated raw: `[]()` in a name would
104+
// otherwise synthesize a link to either one.
105+
const name = escapeMarkdownSyntaxTokens(displayName);
106+
const provider = setup.signInProviderName && escapeMarkdownSyntaxTokens(setup.signInProviderName);
107+
// `command:` hrefs, so a link in the copy takes the same route a button would —
108+
// funnel step and URL validation included. Both carry the agent id and nothing
109+
// else: the docs command resolves the URL from the agent's own declaration.
110+
const reload = createCommandUri(AGENT_SDK_SETUP_RELOAD_COMMAND_ID, setup.agent).toString();
111+
const docs = setup.setupDocsUrl ? createCommandUri(AGENT_SDK_SETUP_OPEN_DOCS_COMMAND_ID, setup.agent).toString() : undefined;
117112
if (provider && docs) {
118-
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription.all', "If you already set up {0} elsewhere, [reload {0} configuration]({1}). Sign in to GitHub to use GitHub Copilot models, sign in to {2} to use your {2} subscription, or [learn more]({3}) for other ways to set up {0}.", displayName, reload, provider, docs));
113+
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription.all', "Sign in to GitHub to use GitHub Copilot models, or sign in to {2} to use your {2} subscription. If you already set up {0} elsewhere, [reload {0} configuration]({1}). [Learn more]({3}) about other ways to set up {0}.", name, reload, provider, docs));
119114
}
120115
if (provider) {
121-
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription.signIn', "If you already set up {0} elsewhere, [reload {0} configuration]({1}). Sign in to GitHub to use GitHub Copilot models, or sign in to {2} to use your {2} subscription.", displayName, reload, provider));
116+
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription.signIn', "Sign in to GitHub to use GitHub Copilot models, or sign in to {2} to use your {2} subscription. If you already set up {0} elsewhere, [reload {0} configuration]({1}).", name, reload, provider));
122117
}
123118
if (docs) {
124-
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription.docs', "If you already set up {0} elsewhere, [reload {0} configuration]({1}). Sign in to GitHub to use GitHub Copilot models, or [learn more]({2}) for other ways to set up {0}.", displayName, reload, docs));
119+
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription.docs', "Sign in to GitHub to use GitHub Copilot models. If you already set up {0} elsewhere, [reload {0} configuration]({1}). [Learn more]({2}) about other ways to set up {0}.", name, reload, docs));
125120
}
126-
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription', "If you already set up {0} elsewhere, [reload {0} configuration]({1}). Sign in to GitHub to use GitHub Copilot models.", displayName, reload));
121+
return setupMarkdown(localize('agentHost.sdkSetup.noAccountDescription', "Sign in to GitHub to use GitHub Copilot models. If you already set up {0} elsewhere, [reload {0} configuration]({1}).", name, reload));
127122
}
128123

129124
/**

src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostSdkSetupNotification.test.ts

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -116,34 +116,51 @@ suite('Agent SDK setup banner', () => {
116116
assert.deepStrictEqual(notification.actions.map(action => action.label), ['Sign in to ChatGPT', 'Sign in to GitHub']);
117117
});
118118

119-
test('the routes named in the copy are the ones the agent declared, and reload leads all four', () => {
119+
test('the routes named in the copy are the ones the agent declared, ranked as the buttons rank them', () => {
120120
// One whole sentence per combination rather than joined clauses, since a
121121
// translator reorders them freely. GitHub appears in all four: every agent
122122
// behind this banner reaches models through our proxy once signed in.
123123
const noAccount = (setup: Omit<IAgentSdkSetupInfo, 'agent' | 'download'>) => {
124124
const description = createAgentSdkSetupNotification({ agent: 'claude', download: 'ready', ...setup }, 'Claude', 'noAccount')?.description;
125125
return typeof description === 'string' ? description : description?.value;
126126
};
127-
// The agent id, like every button carries — the command resolves the URL
128-
// from the agent's own declaration rather than trusting the banner's copy.
129-
const docs = `command:${AGENT_SDK_SETUP_OPEN_DOCS_COMMAND_ID}?%22claude%22`;
127+
// Leads every variant, as the primary button does.
128+
const gitHub = 'Sign in to GitHub to use GitHub Copilot models';
130129
// Unconditional: setup finished in a terminal has no completion signal, so
131130
// every agent needs the "look again" route whatever else it declares.
132-
const reload = `If you already set up Claude elsewhere, [reload Claude configuration](command:${AGENT_SDK_SETUP_RELOAD_COMMAND_ID}?%22claude%22).`;
131+
const reload = `If you already set up Claude elsewhere, [reload Claude configuration](command:${AGENT_SDK_SETUP_RELOAD_COMMAND_ID}?%255B%2522claude%2522%255D).`;
132+
// The agent id, like every button carries — the command resolves the URL
133+
// from the agent's own declaration rather than trusting the banner's copy.
134+
const docs = `[Learn more](command:${AGENT_SDK_SETUP_OPEN_DOCS_COMMAND_ID}?%255B%2522claude%2522%255D) about other ways to set up Claude.`;
133135

134136
assert.deepStrictEqual({
135137
gitHubOnly: noAccount({}),
136138
docs: noAccount({ setupDocsUrl: 'https://example.test/claude' }),
137139
signIn: noAccount({ signInProviderName: 'ChatGPT' }),
138140
both: noAccount({ setupDocsUrl: 'https://example.test/claude', signInProviderName: 'ChatGPT' }),
139141
}, {
140-
gitHubOnly: `${reload} Sign in to GitHub to use GitHub Copilot models.`,
141-
docs: `${reload} Sign in to GitHub to use GitHub Copilot models, or [learn more](${docs}) for other ways to set up Claude.`,
142-
signIn: `${reload} Sign in to GitHub to use GitHub Copilot models, or sign in to ChatGPT to use your ChatGPT subscription.`,
143-
both: `${reload} Sign in to GitHub to use GitHub Copilot models, sign in to ChatGPT to use your ChatGPT subscription, or [learn more](${docs}) for other ways to set up Claude.`,
142+
gitHubOnly: `${gitHub}. ${reload}`,
143+
docs: `${gitHub}. ${reload} ${docs}`,
144+
signIn: `${gitHub}, or sign in to ChatGPT to use your ChatGPT subscription. ${reload}`,
145+
both: `${gitHub}, or sign in to ChatGPT to use your ChatGPT subscription. ${reload} ${docs}`,
144146
});
145147
});
146148

149+
test('a name carrying markdown is escaped, so the host cannot forge a third link', () => {
150+
// Both nouns arrive from the host, and this description is trusted for two
151+
// commands — an unescaped `[]()` in either would render as a link to one of
152+
// them instead of as the name.
153+
const description = createAgentSdkSetupNotification(
154+
{ agent: 'claude', download: 'ready', setupDocsUrl: 'https://example.test/claude', signInProviderName: 'Chat[G]PT' },
155+
'Claude [x](command:evil)',
156+
'noAccount',
157+
)?.description;
158+
const name = 'Claude \\[x\\]\\(command:evil\\)';
159+
160+
assert.strictEqual(typeof description === 'string' ? description : description?.value,
161+
`Sign in to GitHub to use GitHub Copilot models, or sign in to Chat\\[G\\]PT to use your Chat\\[G\\]PT subscription. If you already set up ${name} elsewhere, [reload ${name} configuration](command:${AGENT_SDK_SETUP_RELOAD_COMMAND_ID}?%255B%2522claude%2522%255D). [Learn more](command:${AGENT_SDK_SETUP_OPEN_DOCS_COMMAND_ID}?%255B%2522claude%2522%255D) about other ways to set up ${name}.`);
162+
});
163+
147164
test('the copy is trusted for its own two commands alone, so its links render and reach nothing else', () => {
148165
// Untrusted markdown renders a `command:` link as inert text, which would
149166
// leave both routes with no affordance at all now that neither has a button.

0 commit comments

Comments
 (0)