Skip to content

Commit d0427c6

Browse files
committed
fix: return inline errors for invalid prompt inputs
The MCP SDK validates prompt arguments before calling handlers. When validation fails (e.g. unsupported language like "rust"), the SDK throws McpError(-32602) with a raw JSON dump — poor UX for VS Code slash commands. Server-side changes (server/src/prompts/workflow-prompts.ts): - Add toPermissiveShape() to widen z.enum() to z.string() in registered schemas so the SDK never rejects user input at the protocol layer - Add createSafePromptHandler() to wrap all 11 prompt handlers with: (1) strict Zod validation that returns user-friendly inline errors (2) exception recovery that catches handler crashes gracefully - Add formatValidationError() to convert ZodError into actionable markdown with field names, received values, and valid options listed Integration tests (extensions/vscode/test/suite/mcp-prompt-e2e.integration.test.ts): - Expand from 4 to 15 e2e tests covering all prompts with file-path params - Add test for invalid language returning inline error (not protocol error) - Add test for path traversal detection - Add tests for all-optional prompts with empty args Unit tests (server/test/src/prompts/workflow-prompts.test.ts): - Add 17 new tests for toPermissiveShape, formatValidationError, createSafePromptHandler, and handler-level invalid argument handling - Update schema-to-registration consistency tests for permissive shapes
1 parent fd1ec8a commit d0427c6

5 files changed

Lines changed: 1518 additions & 740 deletions

File tree

extensions/vscode/test/suite/mcp-prompt-e2e.integration.test.ts

Lines changed: 269 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
* - A relative or nonexistent `queryPath` triggers a cryptic -32001 error
1212
* - Invalid paths propagate silently into the LLM context without any warning
1313
* - Path traversal attempts are not detected
14+
* - Invalid enum values (e.g. unsupported language) crash with raw MCP errors
1415
*/
1516

1617
import * as assert from 'assert';
@@ -46,6 +47,17 @@ function resolveServerPath(): string {
4647
}
4748
}
4849

50+
/**
51+
* Extract the text content from the first message in a prompt result.
52+
*/
53+
function getFirstMessageText(result: Awaited<ReturnType<Client['getPrompt']>>): string {
54+
assert.ok(result.messages, 'Prompt should return messages');
55+
assert.ok(result.messages.length > 0, 'Prompt should return at least one message');
56+
const content = result.messages[0]?.content as unknown as { type: string; text: string };
57+
assert.ok(content?.text, 'First message should have text content');
58+
return content.text;
59+
}
60+
4961
suite('MCP Prompt Error Handling Integration Tests', () => {
5062
let client: Client;
5163
let transport: StdioClientTransport;
@@ -82,6 +94,10 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
8294
try { if (transport) await transport.close(); } catch { /* best-effort */ }
8395
});
8496

97+
// ─────────────────────────────────────────────────────────────────────
98+
// Prompt discovery
99+
// ─────────────────────────────────────────────────────────────────────
100+
85101
test('Server should list prompts including explain_codeql_query', async function () {
86102
this.timeout(15_000);
87103

@@ -98,6 +114,10 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
98114
console.log(`[mcp-prompt-e2e] Server provides ${response.prompts.length} prompts`);
99115
});
100116

117+
// ─────────────────────────────────────────────────────────────────────
118+
// explain_codeql_query — path handling
119+
// ─────────────────────────────────────────────────────────────────────
120+
101121
test('explain_codeql_query with nonexistent relative path should return warning, not throw', async function () {
102122
this.timeout(15_000);
103123

@@ -111,17 +131,12 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
111131
},
112132
});
113133

114-
// The prompt MUST return messages — not throw a protocol error.
115-
assert.ok(result.messages, 'Prompt should return messages');
116-
assert.ok(result.messages.length > 0, 'Prompt should return at least one message');
117-
118-
const text = result.messages[0]?.content as unknown as { type: string; text: string };
119-
assert.ok(text?.text, 'First message should have text content');
134+
const text = getFirstMessageText(result);
120135

121136
// The response should contain a user-friendly warning about the invalid path.
122137
assert.ok(
123-
text.text.includes('does not exist'),
124-
`Response should warn that the path does not exist. Got:\n${text.text.slice(0, 500)}`,
138+
text.includes('does not exist'),
139+
`Response should warn that the path does not exist. Got:\n${text.slice(0, 500)}`,
125140
);
126141

127142
console.log('[mcp-prompt-e2e] explain_codeql_query correctly returned warning for nonexistent path');
@@ -143,21 +158,59 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
143158
},
144159
});
145160

146-
assert.ok(result.messages, 'Prompt should return messages');
147-
assert.ok(result.messages.length > 0, 'Prompt should return at least one message');
148-
149-
const text = result.messages[0]?.content as unknown as { type: string; text: string };
150-
assert.ok(text?.text, 'First message should have text content');
161+
const text = getFirstMessageText(result);
151162

152163
// With a valid existing path, there should be no warning.
153164
assert.ok(
154-
!text.text.includes('does not exist'),
155-
`Response should NOT contain a "does not exist" warning for valid path. Got:\n${text.text.slice(0, 500)}`,
165+
!text.includes('does not exist'),
166+
`Response should NOT contain a "does not exist" warning for valid path. Got:\n${text.slice(0, 500)}`,
156167
);
157168

158169
console.log('[mcp-prompt-e2e] explain_codeql_query returned clean response for valid path');
159170
});
160171

172+
// ─────────────────────────────────────────────────────────────────────
173+
// explain_codeql_query — invalid language should return error, not crash
174+
// ─────────────────────────────────────────────────────────────────────
175+
176+
test('explain_codeql_query with invalid language should return user-friendly error', async function () {
177+
this.timeout(15_000);
178+
179+
// VS Code slash command might let a user type an invalid language value.
180+
// The server should return a helpful error message rather than a raw
181+
// MCP protocol error (-32602).
182+
try {
183+
const result = await client.getPrompt({
184+
name: 'explain_codeql_query',
185+
arguments: {
186+
queryPath: '/some/query.ql',
187+
language: 'rust',
188+
},
189+
});
190+
191+
// If the server returns messages instead of throwing, the error info
192+
// should be embedded in the response text.
193+
const text = getFirstMessageText(result);
194+
assert.ok(
195+
text.includes('Invalid') || text.includes('invalid') || text.includes('not supported'),
196+
`Response should indicate the language is invalid. Got:\n${text.slice(0, 500)}`,
197+
);
198+
console.log('[mcp-prompt-e2e] explain_codeql_query returned inline error for invalid language');
199+
} catch (error: unknown) {
200+
// If the SDK throws, verify the error message is user-friendly.
201+
const msg = error instanceof Error ? error.message : String(error);
202+
assert.ok(
203+
msg.includes('Invalid') || msg.includes('invalid') || msg.includes('language'),
204+
`Error should mention invalid argument. Got: ${msg.slice(0, 500)}`,
205+
);
206+
console.log(`[mcp-prompt-e2e] explain_codeql_query threw for invalid language: ${msg.slice(0, 200)}`);
207+
}
208+
});
209+
210+
// ─────────────────────────────────────────────────────────────────────
211+
// document_codeql_query — path handling and invalid args
212+
// ─────────────────────────────────────────────────────────────────────
213+
161214
test('document_codeql_query with nonexistent path should return warning, not throw', async function () {
162215
this.timeout(15_000);
163216

@@ -169,15 +222,210 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
169222
},
170223
});
171224

172-
assert.ok(result.messages, 'Prompt should return messages');
173-
assert.ok(result.messages.length > 0, 'Prompt should return at least one message');
174-
175-
const text = result.messages[0]?.content as unknown as { type: string; text: string };
225+
const text = getFirstMessageText(result);
176226
assert.ok(
177-
text?.text?.includes('does not exist'),
178-
`Response should warn that the path does not exist. Got:\n${(text?.text ?? '').slice(0, 500)}`,
227+
text.includes('does not exist'),
228+
`Response should warn that the path does not exist. Got:\n${text.slice(0, 500)}`,
179229
);
180230

181231
console.log('[mcp-prompt-e2e] document_codeql_query correctly returned warning for nonexistent path');
182232
});
233+
234+
test('document_codeql_query with path traversal should return warning', async function () {
235+
this.timeout(15_000);
236+
237+
const result = await client.getPrompt({
238+
name: 'document_codeql_query',
239+
arguments: {
240+
queryPath: '../../../etc/passwd',
241+
language: 'javascript',
242+
},
243+
});
244+
245+
const text = getFirstMessageText(result);
246+
assert.ok(
247+
text.includes('path traversal') || text.includes('Invalid file path'),
248+
`Response should warn about path traversal. Got:\n${text.slice(0, 500)}`,
249+
);
250+
251+
console.log('[mcp-prompt-e2e] document_codeql_query correctly warned about path traversal');
252+
});
253+
254+
// ─────────────────────────────────────────────────────────────────────
255+
// workshop_creation_workflow — path and parameter handling
256+
// ─────────────────────────────────────────────────────────────────────
257+
258+
test('workshop_creation_workflow with nonexistent queryPath should return warning', async function () {
259+
this.timeout(15_000);
260+
261+
const result = await client.getPrompt({
262+
name: 'workshop_creation_workflow',
263+
arguments: {
264+
queryPath: 'missing/Workshop.ql',
265+
language: 'python',
266+
},
267+
});
268+
269+
const text = getFirstMessageText(result);
270+
assert.ok(
271+
text.includes('does not exist'),
272+
`Response should warn that the path does not exist. Got:\n${text.slice(0, 500)}`,
273+
);
274+
275+
console.log('[mcp-prompt-e2e] workshop_creation_workflow correctly warned for nonexistent path');
276+
});
277+
278+
// ─────────────────────────────────────────────────────────────────────
279+
// tools_query_workflow — database path handling
280+
// ─────────────────────────────────────────────────────────────────────
281+
282+
test('tools_query_workflow with nonexistent database path should return warning', async function () {
283+
this.timeout(15_000);
284+
285+
const result = await client.getPrompt({
286+
name: 'tools_query_workflow',
287+
arguments: {
288+
database: 'nonexistent/db',
289+
language: 'go',
290+
},
291+
});
292+
293+
const text = getFirstMessageText(result);
294+
assert.ok(
295+
text.includes('does not exist'),
296+
`Response should warn that the database path does not exist. Got:\n${text.slice(0, 500)}`,
297+
);
298+
299+
console.log('[mcp-prompt-e2e] tools_query_workflow correctly warned for nonexistent database');
300+
});
301+
302+
// ─────────────────────────────────────────────────────────────────────
303+
// sarif_rank_false_positives — optional sarifPath handling
304+
// ─────────────────────────────────────────────────────────────────────
305+
306+
test('sarif_rank_false_positives with nonexistent sarifPath should return warning', async function () {
307+
this.timeout(15_000);
308+
309+
const result = await client.getPrompt({
310+
name: 'sarif_rank_false_positives',
311+
arguments: {
312+
sarifPath: 'nonexistent/results.sarif',
313+
},
314+
});
315+
316+
const text = getFirstMessageText(result);
317+
assert.ok(
318+
text.includes('does not exist'),
319+
`Response should warn that the SARIF path does not exist. Got:\n${text.slice(0, 500)}`,
320+
);
321+
322+
console.log('[mcp-prompt-e2e] sarif_rank_false_positives correctly warned for nonexistent path');
323+
});
324+
325+
test('sarif_rank_false_positives with no arguments should return content without warning', async function () {
326+
this.timeout(15_000);
327+
328+
const result = await client.getPrompt({
329+
name: 'sarif_rank_false_positives',
330+
arguments: {},
331+
});
332+
333+
const text = getFirstMessageText(result);
334+
// With no file paths provided, there should be no file-not-found warning.
335+
assert.ok(
336+
!text.includes('does not exist'),
337+
`Response should not contain path warnings when no paths given. Got:\n${text.slice(0, 500)}`,
338+
);
339+
340+
console.log('[mcp-prompt-e2e] sarif_rank_false_positives returned clean response with no args');
341+
});
342+
343+
// ─────────────────────────────────────────────────────────────────────
344+
// ql_tdd_advanced — optional database path handling
345+
// ─────────────────────────────────────────────────────────────────────
346+
347+
test('ql_tdd_advanced with nonexistent database should return warning', async function () {
348+
this.timeout(15_000);
349+
350+
const result = await client.getPrompt({
351+
name: 'ql_tdd_advanced',
352+
arguments: {
353+
language: 'javascript',
354+
database: 'nonexistent/db',
355+
},
356+
});
357+
358+
const text = getFirstMessageText(result);
359+
assert.ok(
360+
text.includes('does not exist'),
361+
`Response should warn that the database does not exist. Got:\n${text.slice(0, 500)}`,
362+
);
363+
364+
console.log('[mcp-prompt-e2e] ql_tdd_advanced correctly warned for nonexistent database');
365+
});
366+
367+
// ─────────────────────────────────────────────────────────────────────
368+
// ql_lsp_iterative_development — optional path handling
369+
// ─────────────────────────────────────────────────────────────────────
370+
371+
test('ql_lsp_iterative_development with nonexistent queryPath should return warning', async function () {
372+
this.timeout(15_000);
373+
374+
const result = await client.getPrompt({
375+
name: 'ql_lsp_iterative_development',
376+
arguments: {
377+
queryPath: 'nonexistent/query.ql',
378+
language: 'python',
379+
},
380+
});
381+
382+
const text = getFirstMessageText(result);
383+
assert.ok(
384+
text.includes('does not exist'),
385+
`Response should warn that the query path does not exist. Got:\n${text.slice(0, 500)}`,
386+
);
387+
388+
console.log('[mcp-prompt-e2e] ql_lsp_iterative_development correctly warned for nonexistent path');
389+
});
390+
391+
// ─────────────────────────────────────────────────────────────────────
392+
// All prompts with all-optional params should handle empty args gracefully
393+
// ─────────────────────────────────────────────────────────────────────
394+
395+
test('ql_tdd_basic with empty arguments should return content without errors', async function () {
396+
this.timeout(15_000);
397+
398+
const result = await client.getPrompt({
399+
name: 'ql_tdd_basic',
400+
arguments: {},
401+
});
402+
403+
const text = getFirstMessageText(result);
404+
assert.ok(text.length > 0, 'Should return non-empty content');
405+
assert.ok(
406+
!text.includes('does not exist'),
407+
'Should not contain path warnings with no args',
408+
);
409+
410+
console.log('[mcp-prompt-e2e] ql_tdd_basic returned clean response with empty args');
411+
});
412+
413+
test('run_query_and_summarize_false_positives with nonexistent queryPath should return warning', async function () {
414+
this.timeout(15_000);
415+
416+
const result = await client.getPrompt({
417+
name: 'run_query_and_summarize_false_positives',
418+
arguments: {
419+
queryPath: 'nonexistent/fp-query.ql',
420+
},
421+
});
422+
423+
const text = getFirstMessageText(result);
424+
assert.ok(
425+
text.includes('does not exist'),
426+
`Response should warn that the query path does not exist. Got:\n${text.slice(0, 500)}`,
427+
);
428+
429+
console.log('[mcp-prompt-e2e] run_query_and_summarize_false_positives correctly warned for nonexistent path');
430+
});
183431
});

0 commit comments

Comments
 (0)