mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
feat(mcp): normalize impact and context parameter aliases (#2462)
This commit is contained in:
commit
f49ca472fa
5 changed files with 265 additions and 23 deletions
|
|
@ -142,6 +142,61 @@ function resolveAliasString(canonical: unknown, legacy: unknown): string | undef
|
|||
return undefined;
|
||||
}
|
||||
|
||||
interface StringAliasDefinition {
|
||||
canonical: string;
|
||||
aliases: readonly string[];
|
||||
}
|
||||
|
||||
const TOOL_STRING_ALIASES: Readonly<Record<string, readonly StringAliasDefinition[]>> = {
|
||||
impact: [{ canonical: 'target', aliases: ['name', 'symbol'] }],
|
||||
context: [{ canonical: 'file_path', aliases: ['file'] }],
|
||||
};
|
||||
|
||||
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) {
|
||||
const keys = [canonical, ...aliases];
|
||||
const supplied: Array<{ key: string; value: string }> = [];
|
||||
for (const key of keys) {
|
||||
if (!Object.prototype.hasOwnProperty.call(input, key)) continue;
|
||||
const value = input[key];
|
||||
// Internal CLI callers materialize omitted optional flags as undefined.
|
||||
if (value === undefined) continue;
|
||||
if (typeof value !== 'string' || !value.trim()) {
|
||||
return { error: `MCP parameter ${method}.${key} must be a non-empty string.` };
|
||||
}
|
||||
supplied.push({ key, value: value.trim() });
|
||||
}
|
||||
const distinctValues = new Set(supplied.map(({ value }) => value));
|
||||
if (distinctValues.size > 1) {
|
||||
return {
|
||||
error: `Conflicting MCP parameters for ${method}.${canonical}: ${supplied
|
||||
.map(({ key }) => key)
|
||||
.join(', ')} must agree.`,
|
||||
};
|
||||
}
|
||||
|
||||
for (const alias of aliases) delete normalized[alias];
|
||||
if (supplied.length > 0) normalized[canonical] = supplied[0].value;
|
||||
}
|
||||
|
||||
if (
|
||||
method === 'impact' &&
|
||||
typeof normalized.target !== 'string' &&
|
||||
(typeof normalized.target_uid !== 'string' || !normalized.target_uid.trim())
|
||||
) {
|
||||
return { error: 'MCP impact requires target, name, symbol, or target_uid.' };
|
||||
}
|
||||
return { params: normalized };
|
||||
}
|
||||
|
||||
// AI context generation is CLI-only (gitnexus analyze)
|
||||
// import { generateAIContextFiles } from '../../cli/ai-context.js';
|
||||
|
||||
|
|
@ -1661,7 +1716,9 @@ export class LocalBackend {
|
|||
return this.handleGroupTool(method, params || {});
|
||||
}
|
||||
|
||||
const p = params && typeof params === 'object' ? (params as Record<string, unknown>) : {};
|
||||
const normalized = normalizeToolParams(method, params);
|
||||
if ('error' in normalized) return { error: normalized.error };
|
||||
const p = normalized.params;
|
||||
|
||||
// #2175: Claude Code drops a tool-call argument named exactly "query", so the
|
||||
// query/cypher tools advertise "search_query"/"statement" while still accepting the
|
||||
|
|
@ -1682,47 +1739,52 @@ export class LocalBackend {
|
|||
|
||||
// Resolve repo from optional param (re-reads registry on miss). An optional
|
||||
// `branch` param scopes the resolved handle to that branch's index (#2106).
|
||||
const repoParams = params as { repo?: string; branch?: string } | undefined;
|
||||
const repo = await this.resolveRepo(repoParams?.repo, repoParams?.branch);
|
||||
const repo = await this.resolveRepo(
|
||||
p.repo as string | undefined,
|
||||
p.branch as string | undefined,
|
||||
);
|
||||
|
||||
switch (method) {
|
||||
case 'query':
|
||||
return this.query(repo, params);
|
||||
return this.query(repo, p);
|
||||
case 'cypher': {
|
||||
const raw = await this.cypher(repo, params);
|
||||
const raw = await this.cypher(repo, p);
|
||||
return this.formatCypherAsMarkdown(raw);
|
||||
}
|
||||
case 'context':
|
||||
return this.context(repo, params);
|
||||
return this.context(repo, p);
|
||||
case 'explain':
|
||||
return this.explain(repo, params);
|
||||
return this.explain(repo, p);
|
||||
case 'pdg_query':
|
||||
return this.pdgQuery(repo, params);
|
||||
return this.pdgQuery(repo, p);
|
||||
case 'impact':
|
||||
return this.impact(repo, params);
|
||||
return this.impact(repo, p as unknown as ImpactParams);
|
||||
case 'detect_changes':
|
||||
return this.detectChanges(repo, params);
|
||||
return this.detectChanges(repo, p);
|
||||
case 'check':
|
||||
return this.check(repo, params);
|
||||
return this.check(repo, p);
|
||||
case 'rename':
|
||||
return this.rename(repo, params);
|
||||
return this.rename(repo, p as unknown as Parameters<LocalBackend['rename']>[1]);
|
||||
// Legacy aliases for backwards compatibility
|
||||
case 'search':
|
||||
return this.query(repo, params);
|
||||
return this.query(repo, p);
|
||||
case 'explore':
|
||||
return this.context(repo, { name: params?.name, ...params });
|
||||
return this.context(repo, {
|
||||
name: typeof p.name === 'string' ? p.name : undefined,
|
||||
...p,
|
||||
});
|
||||
case 'overview':
|
||||
return this.overview(repo, params);
|
||||
return this.overview(repo, p);
|
||||
case 'route_map':
|
||||
return this.routeMap(repo, params);
|
||||
return this.routeMap(repo, p);
|
||||
case 'shape_check':
|
||||
return this.shapeCheck(repo, params);
|
||||
return this.shapeCheck(repo, p);
|
||||
case 'tool_map':
|
||||
return this.toolMap(repo, params);
|
||||
return this.toolMap(repo, p);
|
||||
case 'api_impact':
|
||||
return this.apiImpact(repo, params);
|
||||
return this.apiImpact(repo, p);
|
||||
case 'trace':
|
||||
return this.trace(repo, params);
|
||||
return this.trace(repo, p);
|
||||
default:
|
||||
throw new Error(`Unknown tool: ${method}`);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -297,6 +297,10 @@ SERVICE: optional monorepo path prefix (case-sensitive path segments). When "rep
|
|||
description: 'Direct symbol UID from prior tool results (zero-ambiguity lookup)',
|
||||
},
|
||||
file_path: { type: 'string', description: 'File path to disambiguate common names' },
|
||||
file: {
|
||||
type: 'string',
|
||||
description: 'Compatibility alias for file_path; values must agree when both are present',
|
||||
},
|
||||
kind: {
|
||||
type: 'string',
|
||||
description:
|
||||
|
|
@ -461,6 +465,14 @@ SERVICE: optional monorepo path prefix (case-sensitive path segments). When "rep
|
|||
type: 'object',
|
||||
properties: {
|
||||
target: { type: 'string', description: 'Name of function, class, or file to analyze' },
|
||||
name: {
|
||||
type: 'string',
|
||||
description: 'Compatibility alias for target; all supplied target aliases must agree',
|
||||
},
|
||||
symbol: {
|
||||
type: 'string',
|
||||
description: 'Compatibility alias for target; all supplied target aliases must agree',
|
||||
},
|
||||
target_uid: {
|
||||
type: 'string',
|
||||
description:
|
||||
|
|
@ -578,7 +590,7 @@ SERVICE: optional monorepo path prefix (case-sensitive path segments). When "rep
|
|||
maximum: 3600000,
|
||||
},
|
||||
},
|
||||
required: ['target', 'direction'],
|
||||
required: ['direction'],
|
||||
},
|
||||
},
|
||||
{
|
||||
|
|
|
|||
|
|
@ -102,6 +102,31 @@ withTestLbugDB(
|
|||
expect(depNames).toContain('login');
|
||||
});
|
||||
|
||||
it.each(['name', 'symbol'] as const)(
|
||||
'impact tool resolves the %s compatibility alias against a real index',
|
||||
async (alias) => {
|
||||
const result = await backend.callTool('impact', {
|
||||
[alias]: 'validate',
|
||||
direction: 'upstream',
|
||||
});
|
||||
expect(result).not.toHaveProperty('error');
|
||||
expect(result.target?.name).toBe('validate');
|
||||
const directDeps = result.byDepth[1] || result.byDepth['1'] || [];
|
||||
expect(directDeps.map((d: any) => d.name)).toContain('login');
|
||||
},
|
||||
);
|
||||
|
||||
it('context tool resolves the file compatibility alias against a real index', async () => {
|
||||
const result = await backend.callTool('context', {
|
||||
name: 'authenticate',
|
||||
file: 'src/base.ts',
|
||||
});
|
||||
expect(result).not.toHaveProperty('error');
|
||||
expect(result.status).toBe('found');
|
||||
expect(result.symbol?.name).toBe('authenticate');
|
||||
expect(result.symbol?.filePath).toBe('src/base.ts');
|
||||
});
|
||||
|
||||
it('query tool returns results for keyword search', async () => {
|
||||
const result = await backend.callTool('query', { query: 'login' });
|
||||
expect(result).not.toHaveProperty('error');
|
||||
|
|
|
|||
|
|
@ -327,6 +327,138 @@ describe('LocalBackend.callTool', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it.each(['name', 'symbol'] as const)(
|
||||
'normalizes impact.%s to target once before local dispatch',
|
||||
async (alias) => {
|
||||
const impactSpy = vi
|
||||
.spyOn(backend as any, 'impact')
|
||||
.mockResolvedValue({ status: 'normalized' });
|
||||
|
||||
const result = await backend.callTool('impact', {
|
||||
[alias]: ' validate ',
|
||||
direction: 'upstream',
|
||||
});
|
||||
|
||||
expect(result).toEqual({ status: 'normalized' });
|
||||
const dispatched = impactSpy.mock.calls[0][1] as Record<string, unknown>;
|
||||
expect(dispatched.target).toBe('validate');
|
||||
expect(dispatched).not.toHaveProperty('name');
|
||||
expect(dispatched).not.toHaveProperty('symbol');
|
||||
},
|
||||
);
|
||||
|
||||
it('normalizes context.file to file_path once before local dispatch', async () => {
|
||||
const contextSpy = vi
|
||||
.spyOn(backend as any, 'context')
|
||||
.mockResolvedValue({ status: 'normalized' });
|
||||
|
||||
const result = await backend.callTool('context', {
|
||||
name: 'validate',
|
||||
file: ' src/auth.ts ',
|
||||
});
|
||||
|
||||
expect(result).toEqual({ status: 'normalized' });
|
||||
const dispatched = contextSpy.mock.calls[0][1] as Record<string, unknown>;
|
||||
expect(dispatched.file_path).toBe('src/auth.ts');
|
||||
expect(dispatched).not.toHaveProperty('file');
|
||||
});
|
||||
|
||||
it('treats undefined optional alias keys from CLI callers as absent', async () => {
|
||||
const contextSpy = vi
|
||||
.spyOn(backend as any, 'context')
|
||||
.mockResolvedValue({ status: 'normalized' });
|
||||
|
||||
const result = await backend.callTool('context', {
|
||||
name: 'validate',
|
||||
file_path: undefined,
|
||||
file: undefined,
|
||||
});
|
||||
|
||||
expect(result).toEqual({ status: 'normalized' });
|
||||
expect(contextSpy.mock.calls[0][1]).toMatchObject({ name: 'validate' });
|
||||
});
|
||||
|
||||
it('allows agreeing canonical and alias values after trimming', async () => {
|
||||
const impactSpy = vi
|
||||
.spyOn(backend as any, 'impact')
|
||||
.mockResolvedValue({ status: 'normalized' });
|
||||
|
||||
await backend.callTool('impact', {
|
||||
target: 'validate',
|
||||
name: ' validate ',
|
||||
symbol: 'validate',
|
||||
direction: 'upstream',
|
||||
});
|
||||
|
||||
expect(impactSpy.mock.calls[0][1]).toMatchObject({ target: 'validate' });
|
||||
});
|
||||
|
||||
it.each([
|
||||
['impact', { target: 'validate', name: 'login', direction: 'upstream' }],
|
||||
['impact', { name: 'validate', symbol: 'login', direction: 'upstream' }],
|
||||
['context', { name: 'validate', file_path: 'src/auth.ts', file: 'src/login.ts' }],
|
||||
])('rejects conflicting %s aliases before repository resolution', async (method, params) => {
|
||||
const resolveSpy = vi.spyOn(backend, 'resolveRepo');
|
||||
|
||||
const result = await backend.callTool(method, params);
|
||||
|
||||
expect(result.error).toMatch(/conflicting mcp parameters/i);
|
||||
expect(resolveSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
['impact', { target: '', direction: 'upstream' }],
|
||||
['impact', { name: 42, direction: 'upstream' }],
|
||||
['context', { name: 'validate', file: ' ' }],
|
||||
['context', { name: 'validate', file: null }],
|
||||
])('rejects invalid %s aliases before repository resolution', async (method, params) => {
|
||||
const resolveSpy = vi.spyOn(backend, 'resolveRepo');
|
||||
|
||||
const result = await backend.callTool(method, params);
|
||||
|
||||
expect(result.error).toMatch(/non-empty string/i);
|
||||
expect(resolveSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects a missing impact target before repository resolution', async () => {
|
||||
const resolveSpy = vi.spyOn(backend, 'resolveRepo');
|
||||
|
||||
const result = await backend.callTool('impact', { direction: 'upstream' });
|
||||
|
||||
expect(result.error).toMatch(/requires target, name, symbol, or target_uid/i);
|
||||
expect(resolveSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('preserves target_uid-only impact dispatch', async () => {
|
||||
const impactSpy = vi
|
||||
.spyOn(backend as any, 'impact')
|
||||
.mockResolvedValue({ status: 'normalized' });
|
||||
|
||||
await backend.callTool('impact', {
|
||||
target_uid: 'Function:src/auth.ts:validate',
|
||||
direction: 'upstream',
|
||||
});
|
||||
|
||||
expect(impactSpy.mock.calls[0][1]).toMatchObject({
|
||||
target_uid: 'Function:src/auth.ts:validate',
|
||||
});
|
||||
});
|
||||
|
||||
it('normalizes impact aliases before @group forwarding', async () => {
|
||||
resolveAtMemberMock.mockResolvedValue({ ok: true, repoPath: '/tmp/test-project' });
|
||||
const groupImpactSpy = vi
|
||||
.spyOn(backend.getGroupService(), 'groupImpact')
|
||||
.mockResolvedValue({ status: 'normalized' } as any);
|
||||
|
||||
await backend.callTool('impact', {
|
||||
symbol: 'validate',
|
||||
direction: 'upstream',
|
||||
repo: '@grp',
|
||||
});
|
||||
|
||||
expect(groupImpactSpy.mock.calls[0][0]).toMatchObject({ target: 'validate' });
|
||||
});
|
||||
|
||||
it('dispatches query tool', async () => {
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
const result = await backend.callTool('query', { query: 'auth' });
|
||||
|
|
|
|||
|
|
@ -128,6 +128,12 @@ describe('GITNEXUS_TOOLS', () => {
|
|||
expect(contextTool.inputSchema.required).toEqual([]);
|
||||
});
|
||||
|
||||
it('context tool advertises file as a compatibility alias for file_path', () => {
|
||||
const contextTool = GITNEXUS_TOOLS.find((t) => t.name === 'context')!;
|
||||
expect(contextTool.inputSchema.properties.file_path).toBeDefined();
|
||||
expect(contextTool.inputSchema.properties.file).toMatchObject({ type: 'string' });
|
||||
});
|
||||
|
||||
it('api_impact tool expresses the route-or-file requirement via anyOf (#2308)', () => {
|
||||
const apiImpactTool = GITNEXUS_TOOLS.find((t) => t.name === 'api_impact')!;
|
||||
expect(apiImpactTool.inputSchema.anyOf).toEqual([
|
||||
|
|
@ -138,10 +144,15 @@ describe('GITNEXUS_TOOLS', () => {
|
|||
expect(apiImpactTool.inputSchema.required).toEqual([]);
|
||||
});
|
||||
|
||||
it('impact tool requires target and direction', () => {
|
||||
it('impact tool requires direction and advertises target, name, or symbol without combinators', () => {
|
||||
const impactTool = GITNEXUS_TOOLS.find((t) => t.name === 'impact')!;
|
||||
expect(impactTool.inputSchema.required).toContain('target');
|
||||
expect(impactTool.inputSchema.required).toContain('direction');
|
||||
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).not.toHaveProperty('anyOf');
|
||||
expect(impactTool.inputSchema).not.toHaveProperty('oneOf');
|
||||
expect(impactTool.inputSchema).not.toHaveProperty('allOf');
|
||||
});
|
||||
|
||||
it('impact tool advertises the PDG-only `line` statement anchor (integer, min 0, not required)', () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue