fix(extractors): address PR #817 review — ambiguous symbol pick + contract id casing

Copilot + Claude review on PR #817 flagged two follow-up bugs on top of
the F1/F2/F3 fixes:

1. http-route-extractor: ambiguous multi-verb case left handlerName null
   but still ran the CONTAINS DB query. pickSymbolUid(syms, null) then
   silently picked pool[0] — reintroducing handler mis-attribution via
   a different route than the .find() bug F2 fixed. Now gates symbol
   enrichment on an ambiguousCandidates flag so the file-basename
   fallback wins instead.

2. manifest-extractor: buildContractId passed raw user casing through
   for the explicit-method form, so get::/api/orders and
   GET::/api/orders produced different contract ids even though
   parseHttpContract upper-cases during lookup. Now reuses
   parseHttpContract + normalizeRoutePath to canonicalize both method
   and path, so logically equivalent manifest inputs share a contract
   id (and share a manifestSymbolUid fallback).

Adds one regression test per bug: lowercase vs uppercase manifest
contract ids must match, and ambiguous multi-verb with CONTAINS rows
must not silently attach a real handler or call the CONTAINS query
at all. 75 tests pass across the three extractor suites.
This commit is contained in:
Gergo Magyar 2026-04-14 07:42:32 +01:00
parent 138470c83d
commit 6b68fa0252
4 changed files with 94 additions and 7 deletions

View file

@ -256,15 +256,19 @@ export class HttpRouteExtractor implements ContractExtractor {
(d) => normalizeHttpPath(d.path) === normalizedRoute,
);
let match: (typeof candidates)[number] | undefined;
const ambiguousCandidates = !method && candidates.length > 1;
if (method) {
match = candidates.find((d) => d.method === method);
} else if (candidates.length === 1) {
match = candidates[0];
}
// else: multiple candidates + unknown method → leave match
// undefined so handlerName stays null (file-basename fallback
// via pickSymbolUid) and method stays at the conservative 'GET'
// default set below.
// undefined so handlerName stays null and skip symbol
// enrichment below, keeping the file-basename fallback instead
// of letting pickSymbolUid silently pick the first Function /
// Method in the file (which reintroduces the mis-attribution
// we were trying to avoid). Method stays at the conservative
// 'GET' default set below.
if (match) {
if (!method) method = match.method;
handlerName = match.name;
@ -278,7 +282,7 @@ export class HttpRouteExtractor implements ContractExtractor {
let symbolName = path.basename(filePath) || 'handler';
let symPath = filePath;
const fileId = row.fileId ?? row[0];
if (fileId) {
if (fileId && !ambiguousCandidates) {
try {
const syms = await db(CONTAINS_QUERY, { fileId });
if (syms.length > 0) {

View file

@ -284,8 +284,15 @@ export class ManifestExtractor {
private buildContractId(type: ContractType, contract: string): string {
switch (type) {
case 'http': {
if (/^[A-Za-z]+::/.test(contract)) return `http::${contract}`;
return `http::*::${contract}`;
// Canonicalize method casing and path separators so logically
// equivalent inputs (`get::/api/orders` vs `GET::/api/orders`,
// or trailing-slash variants) produce the same contractId and
// matching `manifestSymbolUid` fallback. Without this, raw
// user casing leaks into cross-impact join keys and fragments
// matches across repos.
const { method, path: rawPath } = parseHttpContract(contract);
const normalizedPath = normalizeRoutePath(rawPath);
return method ? `http::${method}::${normalizedPath}` : `http::*::${normalizedPath}`;
}
case 'grpc':
return `grpc::${contract}`;

View file

@ -202,6 +202,49 @@ describe('HttpRouteExtractor — graph-assisted multi-verb disambiguation', () =
expect(out[0].symbolName).toBe('routes.ts');
});
// ── Provider: multi-verb + CONTAINS rows → must still refuse to guess ──
it('provider: ambiguous multi-verb skips CONTAINS enrichment (no silent pool[0] pick)', async () => {
// Regression test for Copilot's review on PR #817. Before the fix,
// the ambiguous-case code path left `handlerName` null but still ran
// the CONTAINS DB query, and `pickSymbolUid(syms, null)` silently
// picked pool[0] — reintroducing handler mis-attribution via a
// different route than `.find()`.
FILE_DETECTIONS.set('routes.ts', [
detection('provider', 'GET', '/api/orders', 'listOrders'),
detection('provider', 'POST', '/api/orders', 'createOrder'),
]);
const db = vi.fn(async (query: string) => {
if (query.includes('HANDLES_ROUTE')) {
return [
{
fileId: 'f1',
filePath: 'routes.ts',
routePath: '/api/orders',
routeSource: 'unknown-reason',
},
];
}
if (query.includes('CONTAINS')) return containsFor(['listOrders', 'createOrder']);
return [];
});
const out = await new HttpRouteExtractor().extract(db, '/repo', {
name: 'r',
url: 'r',
} as never);
expect(out).toHaveLength(1);
// Ambiguous → do not attribute to any real handler in the file.
expect(out[0].symbolName).not.toBe('listOrders');
expect(out[0].symbolName).not.toBe('createOrder');
expect(out[0].symbolUid).toBe('');
expect(out[0].symbolName).toBe('routes.ts');
expect(out[0].meta.method).toBe('GET');
// CONTAINS query must have been skipped entirely under ambiguity.
const calls = db.mock.calls.map(([q]) => q as string);
expect(calls.some((q) => q.includes('CONTAINS'))).toBe(false);
});
// ── Provider: three-verb method known ──────────────────────────────
it('provider: three verbs at same path with method known still matches correctly', async () => {
FILE_DETECTIONS.set('routes.ts', [

View file

@ -423,7 +423,9 @@ describe('ManifestExtractor', () => {
expect(seenParam).toBe('/');
// No match → synthetic uid, no crash.
const provider = result.contracts.find((c) => c.role === 'provider');
expect(provider?.symbolUid).toBe('manifest::orders-svc::http::GET::');
// buildContractId canonicalizes the empty path to `/` so contract ids
// match regardless of trailing-slash variants in the manifest input.
expect(provider?.symbolUid).toBe('manifest::orders-svc::http::GET::/');
});
it('treats empty method portion (::/api/orders) as a bare path', async () => {
@ -545,6 +547,37 @@ describe('ManifestExtractor', () => {
expect(provider?.contractId).toBe('http::GET::/api/orders');
});
it('canonicalizes method casing so get::/api/orders and GET::/api/orders share a contractId', async () => {
// Regression for Copilot's review on PR #817: without canonicalization,
// `buildContractId` passed raw casing through (`http::get::/api/orders`)
// while `parseHttpContract` upper-cased during lookup, fragmenting
// cross-impact joins between providers and consumers that happened to
// use different casing conventions in their group.yaml.
const lower = await extractor.extractFromManifest([
{
from: 'gateway',
to: 'orders-svc',
type: 'http',
contract: 'get::/api/orders',
role: 'consumer',
},
]);
const upper = await extractor.extractFromManifest([
{
from: 'gateway',
to: 'orders-svc',
type: 'http',
contract: 'GET::/api/orders',
role: 'consumer',
},
]);
const lowerContractId = lower.contracts.find((c) => c.role === 'provider')?.contractId;
const upperContractId = upper.contracts.find((c) => c.role === 'provider')?.contractId;
expect(lowerContractId).toBe('http::GET::/api/orders');
expect(upperContractId).toBe('http::GET::/api/orders');
expect(lowerContractId).toBe(upperContractId);
});
it('returns empty for no links', async () => {
const result = await extractor.extractFromManifest([]);
expect(result.contracts).toHaveLength(0);