Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ release cadence.

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

### Fixed

- **`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))

## [v2.25.2] — 2026-04-15

### Highlights
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
"toolName": "query_results_cache_retrieve",
"parameters": {
"cacheKey": "nonexistent-key-for-test",
"maxLines": 50
"maxLines": 50,
"lineRange": { "start": 1, "end": 10 }
},
"success": true,
"description": "Successfully handled cache retrieve for non-existent key"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
"toolName": "query_results_cache_retrieve",
"parameters": {
"cacheKey": "nonexistent-key-for-test",
"maxLines": 50
"maxLines": 50,
"lineRange": { "start": 1, "end": 10 }
},
"expectedSuccess": true,
"description": "Test query_results_cache_retrieve returns appropriate message for missing cache key"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"toolName": "query_results_cache_retrieve",
"arguments": {
"cacheKey": "nonexistent-key-for-test",
"maxLines": 50
"maxLines": 50,
"lineRange": { "start": 1, "end": 10 }
}
}
76 changes: 76 additions & 0 deletions extensions/vscode/test/suite/mcp-tool-e2e.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,82 @@ suite('MCP Annotation & Audit Tool Integration Tests', () => {
const retrieveText = (retrieveResult.content as Array<{ type: string; text: string }>)[0]?.text ?? '';
assert.ok(retrieveText.includes('No cached result'), `Should handle missing key. Got: ${retrieveText}`);

// Step 5: Retrieve with the new object-form lineRange and resultIndices
// parameters (regression: these used to be tuples, which serialized to an
// invalid JSON Schema and broke GitHub Copilot Chat with HTTP 400).
const retrieveWithSubsetResult = await client.callTool({
name: 'query_results_cache_retrieve',
arguments: {
cacheKey: 'nonexistent-test-key',
lineRange: { start: 1, end: 10 },
resultIndices: { start: 0, end: 5 },
},
});
assert.ok(
!retrieveWithSubsetResult.isError,
`query_results_cache_retrieve with subset should succeed. Got: ${JSON.stringify(retrieveWithSubsetResult.content)}`,
);

console.log('[mcp-annotation-e2e] Query results cache test passed');
});

/**
* Regression test for the bug reported in the issue:
* "Fix invalid schema for query_results_cache_retrieve tool use in
* VS Code Copilot Chat".
*
* The GitHub Copilot Chat backend rejects MCP tools whose JSON Schema for
* any input parameter is not an object or boolean. The original
* `query_results_cache_retrieve` tool defined `lineRange` / `resultIndices`
* with `z.tuple([...])`, which serialized to a bare-array schema value and
* caused HTTP 400 responses from Copilot.
*
* This test enforces — at the live wire-protocol level — that EVERY tool's
* `inputSchema` is itself an object schema and that every property's schema
* value is also an object or boolean (never an array).
*/
test('Every tool inputSchema is a valid strict JSON Schema (no array-valued schemas)', async function () {
this.timeout(15_000);

const response = await client.listTools();
assert.ok(response.tools && response.tools.length > 0, 'Server should list tools');

const offending: string[] = [];
for (const tool of response.tools) {
const inputSchema = tool.inputSchema as
| { type?: string; properties?: Record<string, unknown> }
| undefined;

assert.ok(inputSchema, `Tool ${tool.name} should have an inputSchema`);
assert.ok(
typeof inputSchema === 'object' && !Array.isArray(inputSchema),
`Tool ${tool.name} inputSchema must be a JSON Schema object, got: ${JSON.stringify(inputSchema)}`,
);

const properties = inputSchema.properties ?? {};
for (const [propName, propSchema] of Object.entries(properties)) {
const isValid =
typeof propSchema === 'boolean' ||
(typeof propSchema === 'object' && propSchema !== null && !Array.isArray(propSchema));
if (!isValid) {
offending.push(`${tool.name}.${propName} = ${JSON.stringify(propSchema)}`);
}
}
}

assert.deepStrictEqual(
offending,
[],
`The following tool input properties have invalid (non-object/boolean) JSON Schema values, ` +
`which causes GitHub Copilot Chat to reject the server with HTTP 400:\n ${offending.join('\n ')}`,
);

// Spot-check the originally-broken tool/properties.
const retrieveTool = response.tools.find(t => t.name === 'query_results_cache_retrieve');
assert.ok(retrieveTool, 'query_results_cache_retrieve must be present');
const props = (retrieveTool.inputSchema as { properties?: Record<string, { type?: string }> })
.properties ?? {};
assert.strictEqual(props.lineRange?.type, 'object', 'lineRange must serialize as an object schema');
assert.strictEqual(props.resultIndices?.type, 'object', 'resultIndices must serialize as an object schema');
});
});
14 changes: 10 additions & 4 deletions server/dist/codeql-development-mcp-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -200831,8 +200831,14 @@ function registerQueryResultsCacheRetrieveTool(server) {
"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.",
{
cacheKey: external_exports.string().describe("The cache key of the result to retrieve."),
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."),
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."),
lineRange: external_exports.object({
start: external_exports.number().int().min(1).describe("First line to include (1-indexed, inclusive)."),
end: external_exports.number().int().min(1).describe("Last line to include (1-indexed, inclusive).")
}).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."),
resultIndices: external_exports.object({
start: external_exports.number().int().min(0).describe("First SARIF result index to include (0-indexed, inclusive)."),
end: external_exports.number().int().min(0).describe("Last SARIF result index to include (0-indexed, inclusive).")
}).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."),
fileFilter: external_exports.string().optional().describe("For SARIF: only include results whose file path contains this string."),
maxLines: external_exports.number().int().positive().optional().describe("Maximum number of lines to return for line-based formats (default: 500)."),
maxResults: external_exports.number().int().positive().optional().describe("Maximum number of SARIF results to return (default: 100).")
Expand All @@ -200846,7 +200852,7 @@ function registerQueryResultsCacheRetrieveTool(server) {
const isSarif = meta.outputFormat.includes("sarif");
if (isSarif) {
const subset2 = store.getCacheSarifSubset(cacheKey2, {
resultIndices,
resultIndices: resultIndices ? [resultIndices.start, resultIndices.end] : void 0,
fileFilter,
maxResults
});
Expand Down Expand Up @@ -200877,7 +200883,7 @@ function registerQueryResultsCacheRetrieveTool(server) {
};
}
const subset = store.getCacheContentSubset(cacheKey2, {
lineRange,
lineRange: lineRange ? [lineRange.start, lineRange.end] : void 0,
maxLines: maxLines ?? 500
});
if (!subset) {
Expand Down
4 changes: 2 additions & 2 deletions server/dist/codeql-development-mcp-server.js.map

Large diffs are not rendered by default.

22 changes: 14 additions & 8 deletions server/src/tools/cache-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,21 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
{
cacheKey: z.string().describe('The cache key of the result to retrieve.'),
lineRange: z
.tuple([z.number().int().min(1), z.number().int().min(1)])
.refine(([start, end]) => start <= end, { message: 'lineRange start must be <= end' })
.object({
start: z.number().int().min(1).describe('First line to include (1-indexed, inclusive).'),
end: z.number().int().min(1).describe('Last line to include (1-indexed, inclusive).'),
})
.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.'),
.describe('Line range {start, end} (1-indexed, inclusive). For graphtext/CSV output only.'),
resultIndices: z
.tuple([z.number().int().min(0), z.number().int().min(0)])
.refine(([start, end]) => start <= end, { message: 'resultIndices start must be <= end' })
.object({
start: z.number().int().min(0).describe('First SARIF result index to include (0-indexed, inclusive).'),
end: z.number().int().min(0).describe('Last SARIF result index to include (0-indexed, inclusive).'),
})
.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.'),
.describe('SARIF result index range {start, end} (0-indexed, inclusive). For SARIF output only.'),
fileFilter: z.string().optional().describe('For SARIF: only include results whose file path contains this string.'),
maxLines: z.number().int().positive().optional().describe('Maximum number of lines to return for line-based formats (default: 500).'),
maxResults: z.number().int().positive().optional().describe('Maximum number of SARIF results to return (default: 100).'),
Expand All @@ -103,7 +109,7 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
const isSarif = meta.outputFormat.includes('sarif');
if (isSarif) {
const subset = store.getCacheSarifSubset(cacheKey, {
resultIndices,
resultIndices: resultIndices ? [resultIndices.start, resultIndices.end] : undefined,
fileFilter,
maxResults,
});
Expand Down Expand Up @@ -137,7 +143,7 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {

// Line-based subset for graphtext, CSV, or any other text format.
const subset = store.getCacheContentSubset(cacheKey, {
lineRange,
lineRange: lineRange ? [lineRange.start, lineRange.end] : undefined,
maxLines: maxLines ?? 500,
});
if (!subset) {
Expand Down
117 changes: 108 additions & 9 deletions server/test/src/tools/cache-tools.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe('Cache Tools', () => {
});
});

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

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

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

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

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

// Floats should fail
expect(lineRange.safeParse([1.5, 3]).success).toBe(false);
expect(lineRange.safeParse({ start: 1.5, end: 3 }).success).toBe(false);

// Tuple form (legacy) must NOT be accepted — we want a JSON-Schema-clean object.
expect(lineRange.safeParse([1, 5]).success).toBe(false);
});

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

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

// Valid range
expect(resultIndices.safeParse([0, 5]).success).toBe(true);
expect(resultIndices.safeParse({ start: 0, end: 5 }).success).toBe(true);

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

// Negative should fail
expect(resultIndices.safeParse([-1, 3]).success).toBe(false);
expect(resultIndices.safeParse({ start: -1, end: 3 }).success).toBe(false);

// Tuple form (legacy) must NOT be accepted.
expect(resultIndices.safeParse([0, 5]).success).toBe(false);
});

it('should validate maxLines as positive integer', () => {
Expand Down Expand Up @@ -145,6 +151,99 @@ describe('Cache Tools', () => {
});
});

describe('JSON Schema serialization (issue: GitHub Copilot Chat strict validation)', () => {
beforeEach(() => {
vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({
storageLocation: testStorageDir,
autoTrackSessions: true,
retentionDays: 90,
includeCallParameters: true,
includeCallResults: true,
maxActiveSessionsPerQuery: 3,
scoringFrequency: 'per_call',
archiveCompletedSessions: true,
enableAnnotationTools: true,
enableRecommendations: true,
enableMonitoringTools: false,
});
});

/**
* Regression test for the bug where `lineRange` and `resultIndices`
* (defined via `z.tuple([...])`) serialized to a bare array as the
* JSON Schema value (e.g. `[{"type":"integer"}, {"type":"integer"}]`),
* which the GitHub Copilot Chat backend rejects with HTTP 400:
* "[...] is not of type 'object', 'boolean'."
*
* Every property's JSON Schema MUST itself be an object (or boolean),
* never an array.
*/
it('produces a strict-JSON-Schema-valid input schema for every cache tool', async () => {
// Use the real McpServer + SDK tool registration path so we exercise
// the same Zod -> JSON-Schema conversion that the live server uses.
const { McpServer: RealMcpServer } = await import('@modelcontextprotocol/sdk/server/mcp.js');
const realServer = new RealMcpServer({ name: 'test', version: '0.0.0' });
registerCacheTools(realServer);

// Internal handle to the registered tools and their JSON-Schema
// converter — this is the same code path used to satisfy
// `tools/list` requests over the wire.
const registered = (realServer as any)._registeredTools as Record<string, { inputSchema?: unknown }>;
const { toJsonSchemaCompat } = await import('@modelcontextprotocol/sdk/server/zod-json-schema-compat.js');

const offendingProps: string[] = [];
for (const [toolName, def] of Object.entries(registered)) {
if (!def.inputSchema) continue;
const jsonSchema = toJsonSchemaCompat(def.inputSchema as never) as {
properties?: Record<string, unknown>;
};
// The top-level schema must be an object schema.
expect(typeof jsonSchema, `${toolName} top-level schema type`).toBe('object');
expect(Array.isArray(jsonSchema), `${toolName} top-level schema must not be an array`).toBe(false);

for (const [propName, propSchema] of Object.entries(jsonSchema.properties ?? {})) {
// Per JSON Schema spec (and OpenAI/Copilot strict validation),
// every property's schema value must be an object or boolean.
const isValid =
typeof propSchema === 'boolean' ||
(typeof propSchema === 'object' && propSchema !== null && !Array.isArray(propSchema));
if (!isValid) {
offendingProps.push(`${toolName}.${propName} = ${JSON.stringify(propSchema)}`);
}
}
}

expect(offendingProps, `Properties with invalid (non-object/boolean) JSON Schema: ${offendingProps.join('; ')}`).toEqual([]);
});

it('serializes lineRange and resultIndices as JSON-Schema object types (not bare arrays)', async () => {
const { McpServer: RealMcpServer } = await import('@modelcontextprotocol/sdk/server/mcp.js');
const realServer = new RealMcpServer({ name: 'test', version: '0.0.0' });
registerCacheTools(realServer);

const registered = (realServer as any)._registeredTools as Record<string, { inputSchema?: unknown }>;
const { toJsonSchemaCompat } = await import('@modelcontextprotocol/sdk/server/zod-json-schema-compat.js');

const retrieveTool = registered['query_results_cache_retrieve'];
expect(retrieveTool, 'query_results_cache_retrieve must be registered').toBeTruthy();

const jsonSchema = toJsonSchemaCompat(retrieveTool.inputSchema as never) as {
properties: Record<string, { type?: string; properties?: Record<string, unknown> }>;
};

for (const propName of ['lineRange', 'resultIndices']) {
const prop = jsonSchema.properties[propName];
expect(prop, `${propName} must be present in JSON Schema`).toBeTruthy();
expect(typeof prop, `${propName} schema must be an object`).toBe('object');
expect(Array.isArray(prop), `${propName} schema must not be a bare array`).toBe(false);
expect(prop.type, `${propName} must be type=object`).toBe('object');
expect(prop.properties, `${propName} must declare start/end sub-properties`).toBeTruthy();
expect(prop.properties).toHaveProperty('start');
expect(prop.properties).toHaveProperty('end');
}
});
});

describe('handler behavior', () => {
beforeEach(async () => {
vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({
Expand Down
Loading
Loading