mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix(mcp): treat a non-string uid as omitted instead of throwing (#3354)
Review follow-up on #3373. `query.uid?.trim()` called `.trim()` on a client-supplied value, and the MCP envelope is not type-validated, so `context({uid: 42})` threw a TypeError that `context()` does not catch. Before #3373 the same input ended as a structured not_found. A non-string uid now counts as omitted, which matches how normalizeToolParams already treats a non-string `target_uid`. The impact and trace not-found messages print a trimmed uid only when it is a non-blank string, so a strict adapter sending " " sees the name it searched for instead of `' '`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
533af3dafd
commit
765d84d18d
2 changed files with 102 additions and 4 deletions
|
|
@ -4434,7 +4434,9 @@ export class LocalBackend {
|
|||
const { name, include_content } = query;
|
||||
// A blank uid is an omitted optional field (strict adapters send " "/""),
|
||||
// not a lookup key — fall through to the name instead of `not_found`.
|
||||
const uid = query.uid?.trim();
|
||||
// The MCP envelope is not type-validated: a non-string uid is also treated
|
||||
// as omitted, matching normalizeToolParams' impact target_uid check.
|
||||
const uid = typeof query.uid === 'string' ? query.uid.trim() : undefined;
|
||||
const selectClause = `n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine${include_content ? ', n.content AS content' : ''}`;
|
||||
|
||||
// Direct UID — zero-ambiguity path.
|
||||
|
|
@ -6790,7 +6792,11 @@ export class LocalBackend {
|
|||
if (fromOutcome.kind === 'not_found') {
|
||||
return {
|
||||
status: 'not_found',
|
||||
error: `Source symbol '${params.from_uid ?? params.from}' not found.`,
|
||||
error: `Source symbol '${
|
||||
typeof params.from_uid === 'string' && params.from_uid.trim()
|
||||
? params.from_uid.trim()
|
||||
: params.from
|
||||
}' not found.`,
|
||||
suggestion: 'Check the symbol name or use --from-uid for zero-ambiguity.',
|
||||
};
|
||||
}
|
||||
|
|
@ -6817,7 +6823,11 @@ export class LocalBackend {
|
|||
if (toOutcome.kind === 'not_found') {
|
||||
return {
|
||||
status: 'not_found',
|
||||
error: `Target symbol '${params.to_uid ?? params.to}' not found.`,
|
||||
error: `Target symbol '${
|
||||
typeof params.to_uid === 'string' && params.to_uid.trim()
|
||||
? params.to_uid.trim()
|
||||
: params.to
|
||||
}' not found.`,
|
||||
suggestion: 'Check the symbol name or use --to-uid for zero-ambiguity.',
|
||||
};
|
||||
}
|
||||
|
|
@ -7197,7 +7207,10 @@ export class LocalBackend {
|
|||
);
|
||||
|
||||
if (outcome.kind === 'not_found') {
|
||||
const missing = params.target_uid?.trim() || target;
|
||||
const missing =
|
||||
typeof params.target_uid === 'string' && params.target_uid.trim()
|
||||
? params.target_uid.trim()
|
||||
: target;
|
||||
// not_found = no resolved symbol, so the envelope keeps the partial-but-
|
||||
// typed target (typed PdgImpactTarget — there is no id/type/filePath yet).
|
||||
const notFoundTarget: PdgImpactTarget = { name: target };
|
||||
|
|
|
|||
|
|
@ -654,6 +654,91 @@ describe('LocalBackend.callTool', () => {
|
|||
expect(boundParams).toContainEqual(expect.objectContaining({ symName: 'validate' }));
|
||||
});
|
||||
|
||||
it('treats an empty target_uid like a whitespace one and resolves the name (#3354)', async () => {
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
const result = await backend.callTool('impact', {
|
||||
target: 'validate',
|
||||
target_uid: '',
|
||||
direction: 'upstream',
|
||||
});
|
||||
|
||||
expect(result.error).toBe("Target 'validate' not found");
|
||||
const boundParams = (executeParameterized as any).mock.calls.map((c: unknown[]) => c[2]);
|
||||
expect(boundParams).not.toContainEqual(expect.objectContaining({ uid: expect.anything() }));
|
||||
expect(boundParams).toContainEqual(expect.objectContaining({ symName: 'validate' }));
|
||||
});
|
||||
|
||||
it('treats a non-string impact target_uid as omitted instead of throwing (#3354)', async () => {
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
const result = await backend.callTool('impact', {
|
||||
target: 'validate',
|
||||
target_uid: 42,
|
||||
direction: 'upstream',
|
||||
});
|
||||
|
||||
expect(result.error).toBe("Target 'validate' not found");
|
||||
const boundParams = (executeParameterized as any).mock.calls.map((c: unknown[]) => c[2]);
|
||||
expect(boundParams).not.toContainEqual(expect.objectContaining({ uid: expect.anything() }));
|
||||
});
|
||||
|
||||
it.each([[42], [true], [{ id: 'Function:src/auth.ts:validate' }], [['x']]])(
|
||||
'treats a non-string context uid %j as omitted instead of throwing (#3354)',
|
||||
async (uid) => {
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
const result = await backend.callTool('context', { name: 'validate', uid });
|
||||
|
||||
expect(result).toEqual({ error: "Symbol 'validate' not found" });
|
||||
const boundParams = (executeParameterized as any).mock.calls.map((c: unknown[]) => c[2]);
|
||||
expect(boundParams).not.toContainEqual(expect.objectContaining({ uid: expect.anything() }));
|
||||
},
|
||||
);
|
||||
|
||||
it('names the unresolved trace source, not a blank from_uid (#3354)', async () => {
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
const result = await backend.callTool('trace', {
|
||||
from: 'missingSource',
|
||||
from_uid: ' ',
|
||||
to: 'validate',
|
||||
});
|
||||
|
||||
expect(result).toMatchObject({
|
||||
status: 'not_found',
|
||||
error: "Source symbol 'missingSource' not found.",
|
||||
});
|
||||
});
|
||||
|
||||
it('names the unresolved trace target, not a blank to_uid (#3354)', async () => {
|
||||
const sourceRow = {
|
||||
id: 'Function:src/a.ts:start',
|
||||
name: 'start',
|
||||
type: 'Function',
|
||||
filePath: 'src/a.ts',
|
||||
startLine: 1,
|
||||
endLine: 2,
|
||||
};
|
||||
(executeParameterized as any).mockImplementation(
|
||||
async (_path: string, _query: string, bound: Record<string, unknown> | undefined) =>
|
||||
bound?.uid === sourceRow.id ? [sourceRow] : [],
|
||||
);
|
||||
|
||||
const result = await backend.callTool('trace', {
|
||||
from_uid: sourceRow.id,
|
||||
to: 'missingTarget',
|
||||
to_uid: ' ',
|
||||
});
|
||||
(executeParameterized as any).mockReset();
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
expect(result).toMatchObject({
|
||||
status: 'not_found',
|
||||
error: "Target symbol 'missingTarget' not found.",
|
||||
});
|
||||
});
|
||||
|
||||
it('normalizes impact aliases before @group forwarding', async () => {
|
||||
resolveAtMemberMock.mockResolvedValue({ ok: true, repoPath: '/tmp/test-project' });
|
||||
const groupImpactSpy = vi
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue