Skip to content

Commit db62263

Browse files
Copilotdata-douserCopilot
authored
Fix invalid JSON Schema for query_results_cache_retrieve (#263)
* Initial plan * Fix invalid JSON Schema for query_results_cache_retrieve (use z.object for lineRange/resultIndices) Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/950558d1-9e5d-4eec-bdd3-0668c904dd1f Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * Add server tool-schema-validation.test.ts Adds tests to generically avoid regressions due to invalid schema for any MCP tool. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nathan Randall <70299490+data-douser@users.noreply.github.com> * Address PR review feedback --------- Signed-off-by: Nathan Randall <70299490+data-douser@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> Co-authored-by: Nathan Randall <data-douser@github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent cffb3f8 commit db62263

10 files changed

Lines changed: 431 additions & 26 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ release cadence.
1414

1515
_Changes on `main` since the latest tagged release that have not yet been included in a stable release._
1616

17+
### Fixed
18+
19+
- **`query_results_cache_retrieve` rejected by GitHub Copilot Chat (HTTP 400 invalid schema)** — The `lineRange` and `resultIndices` parameters were defined with `z.tuple([...])`, which the MCP SDK serialized to a bare-array JSON Schema value (e.g. `[{"type":"integer"}, {"type":"integer"}]`). GitHub Copilot Chat enforces strict JSON Schema validation and rejected the entire `ql-mcp` server with `"... is not of type 'object', 'boolean'"`. Both parameters now use `z.object({ start, end })` so they serialize to a valid `type: "object"` JSON Schema. Tool callers must now pass `{ "lineRange": { "start": 1, "end": 10 } }` instead of `{ "lineRange": [1, 10] }`. ([#263](https://github.com/advanced-security/codeql-development-mcp-server/pull/263))
20+
1721
## [v2.25.2] — 2026-04-15
1822

1923
### Highlights

‎client/integration-tests/primitives/tools/query_results_cache_retrieve/retrieve_with_subset/after/monitoring-state.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@
22
"toolName": "query_results_cache_retrieve",
33
"parameters": {
44
"cacheKey": "nonexistent-key-for-test",
5-
"maxLines": 50
5+
"maxLines": 50,
6+
"lineRange": { "start": 1, "end": 10 }
67
},
78
"success": true,
89
"description": "Successfully handled cache retrieve for non-existent key"

‎client/integration-tests/primitives/tools/query_results_cache_retrieve/retrieve_with_subset/before/monitoring-state.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@
22
"toolName": "query_results_cache_retrieve",
33
"parameters": {
44
"cacheKey": "nonexistent-key-for-test",
5-
"maxLines": 50
5+
"maxLines": 50,
6+
"lineRange": { "start": 1, "end": 10 }
67
},
78
"expectedSuccess": true,
89
"description": "Test query_results_cache_retrieve returns appropriate message for missing cache key"

‎client/integration-tests/primitives/tools/query_results_cache_retrieve/retrieve_with_subset/test-config.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
"toolName": "query_results_cache_retrieve",
33
"arguments": {
44
"cacheKey": "nonexistent-key-for-test",
5-
"maxLines": 50
5+
"maxLines": 50,
6+
"lineRange": { "start": 1, "end": 10 }
67
}
78
}

‎extensions/vscode/test/suite/mcp-tool-e2e.integration.test.ts‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,82 @@ suite('MCP Annotation & Audit Tool Integration Tests', () => {
471471
const retrieveText = (retrieveResult.content as Array<{ type: string; text: string }>)[0]?.text ?? '';
472472
assert.ok(retrieveText.includes('No cached result'), `Should handle missing key. Got: ${retrieveText}`);
473473

474+
// Step 5: Retrieve with the new object-form lineRange and resultIndices
475+
// parameters (regression: these used to be tuples, which serialized to an
476+
// invalid JSON Schema and broke GitHub Copilot Chat with HTTP 400).
477+
const retrieveWithSubsetResult = await client.callTool({
478+
name: 'query_results_cache_retrieve',
479+
arguments: {
480+
cacheKey: 'nonexistent-test-key',
481+
lineRange: { start: 1, end: 10 },
482+
resultIndices: { start: 0, end: 5 },
483+
},
484+
});
485+
assert.ok(
486+
!retrieveWithSubsetResult.isError,
487+
`query_results_cache_retrieve with subset should succeed. Got: ${JSON.stringify(retrieveWithSubsetResult.content)}`,
488+
);
489+
474490
console.log('[mcp-annotation-e2e] Query results cache test passed');
475491
});
492+
493+
/**
494+
* Regression test for the bug reported in the issue:
495+
* "Fix invalid schema for query_results_cache_retrieve tool use in
496+
* VS Code Copilot Chat".
497+
*
498+
* The GitHub Copilot Chat backend rejects MCP tools whose JSON Schema for
499+
* any input parameter is not an object or boolean. The original
500+
* `query_results_cache_retrieve` tool defined `lineRange` / `resultIndices`
501+
* with `z.tuple([...])`, which serialized to a bare-array schema value and
502+
* caused HTTP 400 responses from Copilot.
503+
*
504+
* This test enforces — at the live wire-protocol level — that EVERY tool's
505+
* `inputSchema` is itself an object schema and that every property's schema
506+
* value is also an object or boolean (never an array).
507+
*/
508+
test('Every tool inputSchema is a valid strict JSON Schema (no array-valued schemas)', async function () {
509+
this.timeout(15_000);
510+
511+
const response = await client.listTools();
512+
assert.ok(response.tools && response.tools.length > 0, 'Server should list tools');
513+
514+
const offending: string[] = [];
515+
for (const tool of response.tools) {
516+
const inputSchema = tool.inputSchema as
517+
| { type?: string; properties?: Record<string, unknown> }
518+
| undefined;
519+
520+
assert.ok(inputSchema, `Tool ${tool.name} should have an inputSchema`);
521+
assert.ok(
522+
typeof inputSchema === 'object' && !Array.isArray(inputSchema),
523+
`Tool ${tool.name} inputSchema must be a JSON Schema object, got: ${JSON.stringify(inputSchema)}`,
524+
);
525+
526+
const properties = inputSchema.properties ?? {};
527+
for (const [propName, propSchema] of Object.entries(properties)) {
528+
const isValid =
529+
typeof propSchema === 'boolean' ||
530+
(typeof propSchema === 'object' && propSchema !== null && !Array.isArray(propSchema));
531+
if (!isValid) {
532+
offending.push(`${tool.name}.${propName} = ${JSON.stringify(propSchema)}`);
533+
}
534+
}
535+
}
536+
537+
assert.deepStrictEqual(
538+
offending,
539+
[],
540+
`The following tool input properties have invalid (non-object/boolean) JSON Schema values, ` +
541+
`which causes GitHub Copilot Chat to reject the server with HTTP 400:\n ${offending.join('\n ')}`,
542+
);
543+
544+
// Spot-check the originally-broken tool/properties.
545+
const retrieveTool = response.tools.find(t => t.name === 'query_results_cache_retrieve');
546+
assert.ok(retrieveTool, 'query_results_cache_retrieve must be present');
547+
const props = (retrieveTool.inputSchema as { properties?: Record<string, { type?: string }> })
548+
.properties ?? {};
549+
assert.strictEqual(props.lineRange?.type, 'object', 'lineRange must serialize as an object schema');
550+
assert.strictEqual(props.resultIndices?.type, 'object', 'resultIndices must serialize as an object schema');
551+
});
476552
});

‎server/dist/codeql-development-mcp-server.js‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -200831,8 +200831,14 @@ function registerQueryResultsCacheRetrieveTool(server) {
200831200831
"Retrieve cached query results with optional subset selection. Supports line ranges (for graphtext/CSV) and SARIF result indices and file filtering to return only the relevant portion.",
200832200832
{
200833200833
cacheKey: external_exports.string().describe("The cache key of the result to retrieve."),
200834-
lineRange: external_exports.tuple([external_exports.number().int().min(1), external_exports.number().int().min(1)]).refine(([start, end]) => start <= end, { message: "lineRange start must be <= end" }).optional().describe("Line range [start, end] (1-indexed, inclusive). For graphtext/CSV output only."),
200835-
resultIndices: external_exports.tuple([external_exports.number().int().min(0), external_exports.number().int().min(0)]).refine(([start, end]) => start <= end, { message: "resultIndices start must be <= end" }).optional().describe("SARIF result index range [start, end] (0-indexed, inclusive). For SARIF output only."),
200834+
lineRange: external_exports.object({
200835+
start: external_exports.number().int().min(1).describe("First line to include (1-indexed, inclusive)."),
200836+
end: external_exports.number().int().min(1).describe("Last line to include (1-indexed, inclusive).")
200837+
}).refine(({ start, end }) => start <= end, { message: "lineRange.start must be <= lineRange.end" }).optional().describe("Line range {start, end} (1-indexed, inclusive). For graphtext/CSV output only."),
200838+
resultIndices: external_exports.object({
200839+
start: external_exports.number().int().min(0).describe("First SARIF result index to include (0-indexed, inclusive)."),
200840+
end: external_exports.number().int().min(0).describe("Last SARIF result index to include (0-indexed, inclusive).")
200841+
}).refine(({ start, end }) => start <= end, { message: "resultIndices.start must be <= resultIndices.end" }).optional().describe("SARIF result index range {start, end} (0-indexed, inclusive). For SARIF output only."),
200836200842
fileFilter: external_exports.string().optional().describe("For SARIF: only include results whose file path contains this string."),
200837200843
maxLines: external_exports.number().int().positive().optional().describe("Maximum number of lines to return for line-based formats (default: 500)."),
200838200844
maxResults: external_exports.number().int().positive().optional().describe("Maximum number of SARIF results to return (default: 100).")
@@ -200846,7 +200852,7 @@ function registerQueryResultsCacheRetrieveTool(server) {
200846200852
const isSarif = meta.outputFormat.includes("sarif");
200847200853
if (isSarif) {
200848200854
const subset2 = store.getCacheSarifSubset(cacheKey2, {
200849-
resultIndices,
200855+
resultIndices: resultIndices ? [resultIndices.start, resultIndices.end] : void 0,
200850200856
fileFilter,
200851200857
maxResults
200852200858
});
@@ -200877,7 +200883,7 @@ function registerQueryResultsCacheRetrieveTool(server) {
200877200883
};
200878200884
}
200879200885
const subset = store.getCacheContentSubset(cacheKey2, {
200880-
lineRange,
200886+
lineRange: lineRange ? [lineRange.start, lineRange.end] : void 0,
200881200887
maxLines: maxLines ?? 500
200882200888
});
200883200889
if (!subset) {

‎server/dist/codeql-development-mcp-server.js.map‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎server/src/tools/cache-tools.ts‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,21 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
7777
{
7878
cacheKey: z.string().describe('The cache key of the result to retrieve.'),
7979
lineRange: z
80-
.tuple([z.number().int().min(1), z.number().int().min(1)])
81-
.refine(([start, end]) => start <= end, { message: 'lineRange start must be <= end' })
80+
.object({
81+
start: z.number().int().min(1).describe('First line to include (1-indexed, inclusive).'),
82+
end: z.number().int().min(1).describe('Last line to include (1-indexed, inclusive).'),
83+
})
84+
.refine(({ start, end }) => start <= end, { message: 'lineRange.start must be <= lineRange.end' })
8285
.optional()
83-
.describe('Line range [start, end] (1-indexed, inclusive). For graphtext/CSV output only.'),
86+
.describe('Line range {start, end} (1-indexed, inclusive). For graphtext/CSV output only.'),
8487
resultIndices: z
85-
.tuple([z.number().int().min(0), z.number().int().min(0)])
86-
.refine(([start, end]) => start <= end, { message: 'resultIndices start must be <= end' })
88+
.object({
89+
start: z.number().int().min(0).describe('First SARIF result index to include (0-indexed, inclusive).'),
90+
end: z.number().int().min(0).describe('Last SARIF result index to include (0-indexed, inclusive).'),
91+
})
92+
.refine(({ start, end }) => start <= end, { message: 'resultIndices.start must be <= resultIndices.end' })
8793
.optional()
88-
.describe('SARIF result index range [start, end] (0-indexed, inclusive). For SARIF output only.'),
94+
.describe('SARIF result index range {start, end} (0-indexed, inclusive). For SARIF output only.'),
8995
fileFilter: z.string().optional().describe('For SARIF: only include results whose file path contains this string.'),
9096
maxLines: z.number().int().positive().optional().describe('Maximum number of lines to return for line-based formats (default: 500).'),
9197
maxResults: z.number().int().positive().optional().describe('Maximum number of SARIF results to return (default: 100).'),
@@ -103,7 +109,7 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
103109
const isSarif = meta.outputFormat.includes('sarif');
104110
if (isSarif) {
105111
const subset = store.getCacheSarifSubset(cacheKey, {
106-
resultIndices,
112+
resultIndices: resultIndices ? [resultIndices.start, resultIndices.end] : undefined,
107113
fileFilter,
108114
maxResults,
109115
});
@@ -137,7 +143,7 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
137143

138144
// Line-based subset for graphtext, CSV, or any other text format.
139145
const subset = store.getCacheContentSubset(cacheKey, {
140-
lineRange,
146+
lineRange: lineRange ? [lineRange.start, lineRange.end] : undefined,
141147
maxLines: maxLines ?? 500,
142148
});
143149
if (!subset) {

‎server/test/src/tools/cache-tools.test.ts‎

Lines changed: 108 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ describe('Cache Tools', () => {
7474
});
7575
});
7676

77-
it('should validate lineRange as positive integer tuple with start <= end', () => {
77+
it('should validate lineRange as positive integer object with start <= end', () => {
7878
registerCacheTools(mockServer);
7979

8080
const retrieveCall = (mockServer.tool as any).mock.calls.find(
@@ -84,19 +84,22 @@ describe('Cache Tools', () => {
8484
const lineRange = schema.lineRange;
8585

8686
// Valid range
87-
expect(lineRange.safeParse([1, 5]).success).toBe(true);
87+
expect(lineRange.safeParse({ start: 1, end: 5 }).success).toBe(true);
8888

8989
// start > end should fail refinement
90-
expect(lineRange.safeParse([5, 1]).success).toBe(false);
90+
expect(lineRange.safeParse({ start: 5, end: 1 }).success).toBe(false);
9191

9292
// 0 is not >= 1
93-
expect(lineRange.safeParse([0, 5]).success).toBe(false);
93+
expect(lineRange.safeParse({ start: 0, end: 5 }).success).toBe(false);
9494

9595
// Floats should fail
96-
expect(lineRange.safeParse([1.5, 3]).success).toBe(false);
96+
expect(lineRange.safeParse({ start: 1.5, end: 3 }).success).toBe(false);
97+
98+
// Tuple form (legacy) must NOT be accepted — we want a JSON-Schema-clean object.
99+
expect(lineRange.safeParse([1, 5]).success).toBe(false);
97100
});
98101

99-
it('should validate resultIndices as non-negative integer tuple', () => {
102+
it('should validate resultIndices as non-negative integer object', () => {
100103
registerCacheTools(mockServer);
101104

102105
const retrieveCall = (mockServer.tool as any).mock.calls.find(
@@ -106,13 +109,16 @@ describe('Cache Tools', () => {
106109
const resultIndices = schema.resultIndices;
107110

108111
// Valid range
109-
expect(resultIndices.safeParse([0, 5]).success).toBe(true);
112+
expect(resultIndices.safeParse({ start: 0, end: 5 }).success).toBe(true);
110113

111114
// start > end should fail refinement
112-
expect(resultIndices.safeParse([5, 1]).success).toBe(false);
115+
expect(resultIndices.safeParse({ start: 5, end: 1 }).success).toBe(false);
113116

114117
// Negative should fail
115-
expect(resultIndices.safeParse([-1, 3]).success).toBe(false);
118+
expect(resultIndices.safeParse({ start: -1, end: 3 }).success).toBe(false);
119+
120+
// Tuple form (legacy) must NOT be accepted.
121+
expect(resultIndices.safeParse([0, 5]).success).toBe(false);
116122
});
117123

118124
it('should validate maxLines as positive integer', () => {
@@ -145,6 +151,99 @@ describe('Cache Tools', () => {
145151
});
146152
});
147153

154+
describe('JSON Schema serialization (issue: GitHub Copilot Chat strict validation)', () => {
155+
beforeEach(() => {
156+
vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({
157+
storageLocation: testStorageDir,
158+
autoTrackSessions: true,
159+
retentionDays: 90,
160+
includeCallParameters: true,
161+
includeCallResults: true,
162+
maxActiveSessionsPerQuery: 3,
163+
scoringFrequency: 'per_call',
164+
archiveCompletedSessions: true,
165+
enableAnnotationTools: true,
166+
enableRecommendations: true,
167+
enableMonitoringTools: false,
168+
});
169+
});
170+
171+
/**
172+
* Regression test for the bug where `lineRange` and `resultIndices`
173+
* (defined via `z.tuple([...])`) serialized to a bare array as the
174+
* JSON Schema value (e.g. `[{"type":"integer"}, {"type":"integer"}]`),
175+
* which the GitHub Copilot Chat backend rejects with HTTP 400:
176+
* "[...] is not of type 'object', 'boolean'."
177+
*
178+
* Every property's JSON Schema MUST itself be an object (or boolean),
179+
* never an array.
180+
*/
181+
it('produces a strict-JSON-Schema-valid input schema for every cache tool', async () => {
182+
// Use the real McpServer + SDK tool registration path so we exercise
183+
// the same Zod -> JSON-Schema conversion that the live server uses.
184+
const { McpServer: RealMcpServer } = await import('@modelcontextprotocol/sdk/server/mcp.js');
185+
const realServer = new RealMcpServer({ name: 'test', version: '0.0.0' });
186+
registerCacheTools(realServer);
187+
188+
// Internal handle to the registered tools and their JSON-Schema
189+
// converter — this is the same code path used to satisfy
190+
// `tools/list` requests over the wire.
191+
const registered = (realServer as any)._registeredTools as Record<string, { inputSchema?: unknown }>;
192+
const { toJsonSchemaCompat } = await import('@modelcontextprotocol/sdk/server/zod-json-schema-compat.js');
193+
194+
const offendingProps: string[] = [];
195+
for (const [toolName, def] of Object.entries(registered)) {
196+
if (!def.inputSchema) continue;
197+
const jsonSchema = toJsonSchemaCompat(def.inputSchema as never) as {
198+
properties?: Record<string, unknown>;
199+
};
200+
// The top-level schema must be an object schema.
201+
expect(typeof jsonSchema, `${toolName} top-level schema type`).toBe('object');
202+
expect(Array.isArray(jsonSchema), `${toolName} top-level schema must not be an array`).toBe(false);
203+
204+
for (const [propName, propSchema] of Object.entries(jsonSchema.properties ?? {})) {
205+
// Per JSON Schema spec (and OpenAI/Copilot strict validation),
206+
// every property's schema value must be an object or boolean.
207+
const isValid =
208+
typeof propSchema === 'boolean' ||
209+
(typeof propSchema === 'object' && propSchema !== null && !Array.isArray(propSchema));
210+
if (!isValid) {
211+
offendingProps.push(`${toolName}.${propName} = ${JSON.stringify(propSchema)}`);
212+
}
213+
}
214+
}
215+
216+
expect(offendingProps, `Properties with invalid (non-object/boolean) JSON Schema: ${offendingProps.join('; ')}`).toEqual([]);
217+
});
218+
219+
it('serializes lineRange and resultIndices as JSON-Schema object types (not bare arrays)', async () => {
220+
const { McpServer: RealMcpServer } = await import('@modelcontextprotocol/sdk/server/mcp.js');
221+
const realServer = new RealMcpServer({ name: 'test', version: '0.0.0' });
222+
registerCacheTools(realServer);
223+
224+
const registered = (realServer as any)._registeredTools as Record<string, { inputSchema?: unknown }>;
225+
const { toJsonSchemaCompat } = await import('@modelcontextprotocol/sdk/server/zod-json-schema-compat.js');
226+
227+
const retrieveTool = registered['query_results_cache_retrieve'];
228+
expect(retrieveTool, 'query_results_cache_retrieve must be registered').toBeTruthy();
229+
230+
const jsonSchema = toJsonSchemaCompat(retrieveTool.inputSchema as never) as {
231+
properties: Record<string, { type?: string; properties?: Record<string, unknown> }>;
232+
};
233+
234+
for (const propName of ['lineRange', 'resultIndices']) {
235+
const prop = jsonSchema.properties[propName];
236+
expect(prop, `${propName} must be present in JSON Schema`).toBeTruthy();
237+
expect(typeof prop, `${propName} schema must be an object`).toBe('object');
238+
expect(Array.isArray(prop), `${propName} schema must not be a bare array`).toBe(false);
239+
expect(prop.type, `${propName} must be type=object`).toBe('object');
240+
expect(prop.properties, `${propName} must declare start/end sub-properties`).toBeTruthy();
241+
expect(prop.properties).toHaveProperty('start');
242+
expect(prop.properties).toHaveProperty('end');
243+
}
244+
});
245+
});
246+
148247
describe('handler behavior', () => {
149248
beforeEach(async () => {
150249
vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({

0 commit comments

Comments
 (0)