Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
32 changes: 29 additions & 3 deletions gitnexus/src/mcp/local/local-backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,16 +154,30 @@ const TOOL_STRING_ALIASES: Readonly<Record<string, readonly StringAliasDefinitio
context: [{ canonical: 'file_path', aliases: ['file'] }],
};

interface RequiredStringGroup {
keys: readonly string[];
error: string;
}

const TOOL_REQUIRED_STRING_GROUPS: Readonly<Record<string, RequiredStringGroup>> = {
impact: {
keys: ['target'],
error: 'impact requires at least one of target, name, or symbol.',
},
api_impact: {
keys: ['route', 'file'],
error: 'Either "route" or "file" is required for api_impact.',
},
};

function normalizeToolParams(
method: string,
params: unknown,
): { params: Record<string, unknown> } | { error: string } {
const input = params && typeof params === 'object' ? (params as Record<string, unknown>) : {};
const definitions = TOOL_STRING_ALIASES[method];
if (!definitions) return { params: input };

const normalized = { ...input };
for (const { canonical, aliases } of definitions) {
for (const { canonical, aliases } of definitions ?? []) {
const keys = [canonical, ...aliases];
const supplied = keys.flatMap((key) => {
const value = input[key];
Expand All @@ -181,6 +195,18 @@ function normalizeToolParams(
for (const alias of aliases) delete normalized[alias];
if (supplied.length > 0) normalized[canonical] = supplied[0].value;
}

const requiredGroup = TOOL_REQUIRED_STRING_GROUPS[method];
if (
requiredGroup &&
!requiredGroup.keys.some((key) => {
const value = normalized[key];
return typeof value === 'string' && value.trim().length > 0;
})
) {
return { error: requiredGroup.error };
}

return { params: normalized };
}

Expand Down
13 changes: 2 additions & 11 deletions gitnexus/src/mcp/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@ export interface ToolDefinition {
}
>;
required: string[];
/**
* JSON-Schema `anyOf` for cross-property constraints `required` cannot express
* — e.g. "at least one of route/file". Forwarded verbatim to clients by the
* server's ListTools handler, so MCP clients see the constraint.
*/
anyOf?: Array<{ required: string[] }>;
};
}

Expand Down Expand Up @@ -444,6 +438,7 @@ Each edit is tagged with confidence:
name: 'impact',
description: `Analyze the blast radius of changing a code symbol.
Returns affected symbols grouped by depth, plus risk assessment, affected execution flows, and affected modules.
Requires at least one of "target", "name", or "symbol"; aliases are normalized and conflicting values are rejected before repository resolution.

MODE (opt-in): "callgraph" (default) walks symbol→symbol edges (CALLS/IMPORTS/EXTENDS/IMPLEMENTS) — inter-procedural, the established comparator/default behavior. "pdg" requires an index built with \`gitnexus analyze --pdg\` and returns one unified PDG-facing result: statement-level control/data dependence from the persisted PDG plus inter-procedural symbol reach. The explicit interprocedural surface is interproceduralByDepth/pdgInterprocedural; byDepth remains the compatibility symbol bucket. pdg remains incompatible with crossDepth and @group targets; relationTypes/minConfidence filter the inter-symbol reach.

Expand Down Expand Up @@ -615,7 +610,6 @@ SERVICE: optional monorepo path prefix (case-sensitive path segments). When "rep
},
},
required: ['direction'],
anyOf: [{ required: ['target'] }, { required: ['name'] }, { required: ['symbol'] }],
},
},
{
Expand Down Expand Up @@ -784,7 +778,7 @@ Returns routes that have both detected response keys AND consumers. Shows top-le
name: 'api_impact',
description: `Pre-change impact report for an API route handler.

WHEN TO USE: BEFORE modifying any API route handler. Shows what consumers depend on, what response fields they access, what middleware protects the route, and what execution flows it triggers. Requires at least "route" or "file" parameter.
WHEN TO USE: BEFORE modifying any API route handler. Shows what consumers depend on, what response fields they access, what middleware protects the route, and what execution flows it triggers. Requires at least "route" or "file" parameter; the requirement is enforced before repository resolution.

Risk levels: LOW (0-3 consumers), MEDIUM (4-9 or any mismatches), HIGH (10+ consumers or mismatches with 4+ consumers). Mismatches with confidence "low" indicate the consumer file fetches multiple routes — property attribution is approximate.

Expand All @@ -803,9 +797,6 @@ Response shape is keyed on how many routes match, not on the data: exactly one m
repo: { type: 'string', description: 'Repository name or path.' },
},
required: [],
// Exactly one lookup key is needed, but either works (route wins if both
// are passed) — so the structural constraint is "at least one of route/file".
anyOf: [{ required: ['route'] }, { required: ['file'] }],
},
},
{
Expand Down
7 changes: 6 additions & 1 deletion gitnexus/test/integration/mcp/server-startup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ describe('MCP server end-to-end startup', () => {
const toolsResponse = (await server.nextMessage()) as {
jsonrpc: string;
id: number;
result?: { tools: Array<{ name: string }> };
result?: { tools: Array<{ name: string; inputSchema: Record<string, unknown> }> };
};

expect(toolsResponse.id).toBe(2);
Expand All @@ -252,6 +252,11 @@ describe('MCP server end-to-end startup', () => {
for (const t of expectedTools) {
expect(toolNames).toContain(t);
}
for (const tool of toolsResponse.result!.tools) {
expect(tool.inputSchema).not.toHaveProperty('anyOf');
expect(tool.inputSchema).not.toHaveProperty('oneOf');
expect(tool.inputSchema).not.toHaveProperty('allOf');
}

// tools/call list_repos — proves the paginated { repositories, pagination }
// shape survives the real request → backend.callTool → JSON.stringify →
Expand Down
17 changes: 17 additions & 0 deletions gitnexus/test/unit/calltool-dispatch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,23 @@ describe('LocalBackend.callTool', () => {
expect(impactSpy.mock.calls[0][1]).toMatchObject({ target: 'validate' });
});

it.each([
['impact', { direction: 'upstream' }, /target.*name.*symbol/i],
['impact', { target: ' ', direction: 'upstream' }, /target.*name.*symbol/i],
['api_impact', {}, /route.*file/i],
['api_impact', { route: ' ', file: '' }, /route.*file/i],
])(
'rejects missing %s lookup keys before repository resolution',
async (method, params, error) => {
const resolveSpy = vi.spyOn(backend, 'resolveRepo');

const result = await backend.callTool(method, params);

expect(result.error).toMatch(error);
expect(resolveSpy).not.toHaveBeenCalled();
},
);

it.each([
['impact', { target: 'validate', name: 'login', direction: 'upstream' }],
['impact', { name: 'validate', symbol: 'login', direction: 'upstream' }],
Expand Down
28 changes: 28 additions & 0 deletions gitnexus/test/unit/mcp-http-transport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import type { AddressInfo } from 'net';
import { describe, it, expect, vi, afterEach } from 'vitest';
import express from 'express';
import type { Request, Response, NextFunction } from 'express';
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';
import {
createAuthMiddleware,
createStreamableHttpHandler,
Expand Down Expand Up @@ -269,6 +271,32 @@ describe('startMcpHttpServer', () => {
expect(JSON.parse(body)).toEqual({ status: 'ok' });
});

it('advertises provider-compatible schemas over Streamable HTTP', async () => {
const backend = createMockBackend();
const { handler, cleanup } = createStreamableHttpHandler(backend as never);
const app = express();
app.use(express.json());
app.all('/mcp', (req, res) => void handler(req, res));
const { port, close } = await listen(app);
const client = new Client({ name: 'http-schema-test', version: '0.0.0' });
const transport = new StreamableHTTPClientTransport(new URL(`http://127.0.0.1:${port}/mcp`));

try {
await client.connect(transport);
const response = await client.listTools();
expect(response.tools.length).toBeGreaterThan(0);
for (const tool of response.tools) {
expect(tool.inputSchema).not.toHaveProperty('anyOf');
expect(tool.inputSchema).not.toHaveProperty('oneOf');
expect(tool.inputSchema).not.toHaveProperty('allOf');
}
} finally {
await client.close();
await close();
await cleanup();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
});

it('POST /mcp without auth token returns 401 when --auth-token is configured', async () => {
const { port, server, cleanup } = await startOnFreePort('supersecret');
servers.push({ server, cleanup });
Expand Down
3 changes: 3 additions & 0 deletions gitnexus/test/unit/mcp-read-only.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ describe('MCP read-only mode', () => {
expect(response.tools.map((tool) => tool.name).sort()).toEqual(READ_ONLY_TOOLS);
for (const tool of response.tools) {
expect(tool.description).not.toMatch(/GROUP MODE|CROSS-REPO|@<groupName>/);
expect(tool.inputSchema).not.toHaveProperty('anyOf');
expect(tool.inputSchema).not.toHaveProperty('oneOf');
expect(tool.inputSchema).not.toHaveProperty('allOf');
const properties = tool.inputSchema.properties as Record<
string,
{ description?: string } | undefined
Expand Down
5 changes: 5 additions & 0 deletions gitnexus/test/unit/mcp-repository-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,11 @@ describe('MCP repository policy', () => {
await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]);

const tools = await client.listTools();
for (const tool of tools.tools) {
expect(tool.inputSchema).not.toHaveProperty('anyOf');
expect(tool.inputSchema).not.toHaveProperty('oneOf');
expect(tool.inputSchema).not.toHaveProperty('allOf');
}
expect(tools.tools.map((tool) => tool.name)).not.toContain('group_list');
expect(tools.tools.map((tool) => tool.name)).not.toContain('group_sync');
for (const tool of tools.tools) {
Expand Down
3 changes: 3 additions & 0 deletions gitnexus/test/unit/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ describe('createMCPServer', () => {
for (const tool of response.tools) {
const definition = GITNEXUS_TOOLS.find((t) => t.name === tool.name)!;
expect(tool.annotations).toEqual(definition.annotations);
expect(tool.inputSchema).not.toHaveProperty('anyOf');
expect(tool.inputSchema).not.toHaveProperty('oneOf');
expect(tool.inputSchema).not.toHaveProperty('allOf');
}
} finally {
await client.close();
Expand Down
26 changes: 15 additions & 11 deletions gitnexus/test/unit/tools.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ describe('GITNEXUS_TOOLS', () => {
}
});

it('advertises provider-compatible root schemas without combinators', () => {
for (const tool of GITNEXUS_TOOLS) {
expect(tool.inputSchema).not.toHaveProperty('anyOf');
expect(tool.inputSchema).not.toHaveProperty('oneOf');
expect(tool.inputSchema).not.toHaveProperty('allOf');
}
});

it('each tool exposes all MCP safety annotations', () => {
for (const tool of GITNEXUS_TOOLS) {
expect(typeof tool.annotations.readOnlyHint).toBe('boolean');
Expand Down Expand Up @@ -144,14 +152,12 @@ describe('GITNEXUS_TOOLS', () => {
expect(contextTool.inputSchema.properties.file).toMatchObject({ type: 'string' });
});

it('api_impact tool expresses the route-or-file requirement via anyOf (#2308)', () => {
it('api_impact describes its runtime-enforced route-or-file requirement', () => {
const apiImpactTool = GITNEXUS_TOOLS.find((t) => t.name === 'api_impact')!;
expect(apiImpactTool.inputSchema.anyOf).toEqual([
{ required: ['route'] },
{ required: ['file'] },
]);
// route/file stay optional in `required` (anyOf carries the cross-field rule)
expect(apiImpactTool.inputSchema.required).toEqual([]);
expect(apiImpactTool.inputSchema.properties).toHaveProperty('route');
expect(apiImpactTool.inputSchema.properties).toHaveProperty('file');
expect(apiImpactTool.description).toContain('Requires at least "route" or "file" parameter');
});

it('impact tool requires direction and accepts target, name, or symbol', () => {
Expand All @@ -160,11 +166,9 @@ describe('GITNEXUS_TOOLS', () => {
expect(impactTool.inputSchema.required).not.toContain('target');
expect(impactTool.inputSchema.properties.name).toMatchObject({ type: 'string' });
expect(impactTool.inputSchema.properties.symbol).toMatchObject({ type: 'string' });
expect(impactTool.inputSchema.anyOf).toEqual([
{ required: ['target'] },
{ required: ['name'] },
{ required: ['symbol'] },
]);
expect(impactTool.description).toContain(
'Requires at least one of "target", "name", or "symbol"',
);
});

it('impact tool advertises the PDG-only `line` statement anchor (integer, min 0, not required)', () => {
Expand Down
Loading