diff --git a/packages/loopover-engine/src/discovery-index-contract.ts b/packages/loopover-engine/src/discovery-index-contract.ts index 6a0b8f06af..c8342ca149 100644 --- a/packages/loopover-engine/src/discovery-index-contract.ts +++ b/packages/loopover-engine/src/discovery-index-contract.ts @@ -174,6 +174,15 @@ export function normalizeDiscoveryIndexRequest(raw: unknown): ParsedDiscoveryInd warnings.push("DiscoveryIndexRequest must be a mapping; falling back to an empty query."); } const source = record ?? {}; + // #9615: surface (never reject) a declared version skew through the existing warnings channel -- the + // same push-one-templated-string-and-continue style as clampLimit and the #6774 candidate cap. An + // absent or non-number declaration stays silent by the tolerant-parser convention: an older/looser + // sender is not an error. + if (typeof source.contractVersion === "number" && source.contractVersion !== DISCOVERY_INDEX_CONTRACT_VERSION) { + warnings.push( + `DiscoveryIndexRequest declared contractVersion ${source.contractVersion}; this build speaks ${DISCOVERY_INDEX_CONTRACT_VERSION}.`, + ); + } const query: DiscoveryIndexQuery = { repos: normalizeStringList(source.repos, normalizeRepoFullName), orgs: normalizeStringList(source.orgs, normalizeOwner), @@ -239,6 +248,15 @@ export function normalizeDiscoveryIndexResponse(raw: unknown): ParsedDiscoveryIn if (!record) { warnings.push("DiscoveryIndexResponse must be a mapping; falling back to an empty candidate list."); } + // #9615: same version-skew warning as the request side -- a server speaking another contract version is + // surfaced instead of being silently relabelled as this build's version. Warning only: nothing below is + // rejected, dropped, or changed by it. + const declaredVersion = record?.contractVersion; + if (typeof declaredVersion === "number" && declaredVersion !== DISCOVERY_INDEX_CONTRACT_VERSION) { + warnings.push( + `DiscoveryIndexResponse declared contractVersion ${declaredVersion}; this build speaks ${DISCOVERY_INDEX_CONTRACT_VERSION}.`, + ); + } const rawCandidates = record && Array.isArray(record.candidates) ? record.candidates : []; // #6774: the request side clamps page size to MAX_PAGE_LIMIT, but this response comes from the OPTIONAL, // only-partially-trusted hosted index (see this module's header). A misbehaving or compromised host could diff --git a/packages/loopover-engine/test/discovery-index-contract-version.test.ts b/packages/loopover-engine/test/discovery-index-contract-version.test.ts new file mode 100644 index 0000000000..7931c2e85a --- /dev/null +++ b/packages/loopover-engine/test/discovery-index-contract-version.test.ts @@ -0,0 +1,50 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { + DISCOVERY_INDEX_CONTRACT_VERSION, + normalizeDiscoveryIndexRequest, + normalizeDiscoveryIndexResponse, +} from "../dist/index.js"; + +// #9615: a declared contract-version skew is surfaced through the tolerant parser's existing warnings +// channel on both wire directions, instead of being silently relabelled as this build's version. + +test("normalizeDiscoveryIndexRequest surfaces a declared contract-version skew as a warning (#9615)", () => { + const parsed = normalizeDiscoveryIndexRequest({ contractVersion: 99, repos: ["a/b"] }); + assert.ok( + parsed.warnings.includes( + `DiscoveryIndexRequest declared contractVersion 99; this build speaks ${DISCOVERY_INDEX_CONTRACT_VERSION}.`, + ), + ); + assert.equal(parsed.request.contractVersion, DISCOVERY_INDEX_CONTRACT_VERSION); + assert.deepEqual(parsed.request.query.repos, ["a/b"]); +}); + +test("normalizeDiscoveryIndexResponse surfaces a declared contract-version skew as a warning (#9615)", () => { + const parsed = normalizeDiscoveryIndexResponse({ contractVersion: 99, candidates: [] }); + assert.ok( + parsed.warnings.includes( + `DiscoveryIndexResponse declared contractVersion 99; this build speaks ${DISCOVERY_INDEX_CONTRACT_VERSION}.`, + ), + ); + assert.equal(parsed.response.contractVersion, DISCOVERY_INDEX_CONTRACT_VERSION); +}); + +test("version warnings stay silent for absent, non-number, and matching declarations (#9615)", () => { + assert.deepEqual(normalizeDiscoveryIndexRequest({ repos: ["a/b"] }).warnings, []); + assert.deepEqual(normalizeDiscoveryIndexRequest({ contractVersion: "1", repos: ["a/b"] }).warnings, []); + assert.deepEqual(normalizeDiscoveryIndexRequest({ contractVersion: 1, repos: ["a/b"] }).warnings, []); + assert.deepEqual(normalizeDiscoveryIndexResponse({ candidates: [] }).warnings, []); + assert.deepEqual(normalizeDiscoveryIndexResponse({ contractVersion: "1", candidates: [] }).warnings, []); + const matching = normalizeDiscoveryIndexResponse({ + contractVersion: 1, + candidates: [{ repoFullName: "owner/repo", issueNumber: 1, title: "x" }], + }); + assert.deepEqual(matching.warnings, []); + assert.equal(matching.response.candidates[0]?.repoFullName, "owner/repo"); + // A non-mapping response takes the optional-chain's undefined arm and stays version-silent too. + assert.deepEqual(normalizeDiscoveryIndexResponse(null).warnings, [ + "DiscoveryIndexResponse must be a mapping; falling back to an empty candidate list.", + ]); +}); diff --git a/packages/loopover-miner/lib/discovery-index-client.ts b/packages/loopover-miner/lib/discovery-index-client.ts index 8c0821d3ee..294b7c9e50 100644 --- a/packages/loopover-miner/lib/discovery-index-client.ts +++ b/packages/loopover-miner/lib/discovery-index-client.ts @@ -100,7 +100,11 @@ export async function queryDiscoveryIndex( { method: "POST", headers: { "content-type": "application/json", ...authHeaders(env) }, - body: JSON.stringify(request.query), + // #9615: send the whole normalized request -- the contract version WITH the query -- flattened to + // one level, because the server parses this body with normalizeDiscoveryIndexRequest, which reads + // the query fields (and the declared contractVersion) off the top level. Nesting the query under a + // `query` key would make the version travel but the query invisible to the server's parse. + body: JSON.stringify({ contractVersion: request.contractVersion, ...request.query }), }, { timeoutMs: options.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS }, ); diff --git a/test/unit/discovery-index-contract.test.ts b/test/unit/discovery-index-contract.test.ts index cabf91f61f..883a0ba6c4 100644 --- a/test/unit/discovery-index-contract.test.ts +++ b/test/unit/discovery-index-contract.test.ts @@ -219,3 +219,32 @@ describe("repo-segment path safety in candidate normalization (#9610)", () => { expect(normalized?.repo).toBe("repo"); }); }); + +describe("contractVersion skew warnings (#9615)", () => { + it("warns with the exact request-side message on a mismatched declared version, still emitting version 1", () => { + const parsed = normalizeDiscoveryIndexRequest({ contractVersion: 99, repos: ["a/b"] }); + expect(parsed.warnings).toContain("DiscoveryIndexRequest declared contractVersion 99; this build speaks 1."); + expect(parsed.request.contractVersion).toBe(1); + expect(parsed.request.query.repos).toEqual(["a/b"]); + }); + + it("warns with the exact response-side message on a mismatched declared version, still emitting version 1", () => { + const parsed = normalizeDiscoveryIndexResponse({ contractVersion: 99, candidates: [] }); + expect(parsed.warnings).toContain("DiscoveryIndexResponse declared contractVersion 99; this build speaks 1."); + expect(parsed.response.contractVersion).toBe(1); + }); + + it("stays silent when the declared version is absent or not a number (tolerant-parser convention)", () => { + expect(normalizeDiscoveryIndexRequest({ repos: ["a/b"] }).warnings).toEqual([]); + expect(normalizeDiscoveryIndexRequest({ contractVersion: "1", repos: ["a/b"] }).warnings).toEqual([]); + expect(normalizeDiscoveryIndexResponse({ candidates: [] }).warnings).toEqual([]); + expect(normalizeDiscoveryIndexResponse({ contractVersion: "1", candidates: [] }).warnings).toEqual([]); + }); + + it("stays silent on a matching declared version and still returns the candidates", () => { + const parsed = normalizeDiscoveryIndexResponse({ contractVersion: 1, candidates: [VALID_CANDIDATE] }); + expect(parsed.warnings).toEqual([]); + expect(parsed.response.candidates.map((candidate) => candidate.repoFullName)).toEqual(["owner/repo"]); + expect(normalizeDiscoveryIndexRequest({ contractVersion: 1, repos: ["a/b"] }).warnings).toEqual([]); + }); +}); diff --git a/test/unit/miner-discovery-index-client.test.ts b/test/unit/miner-discovery-index-client.test.ts index a6f218ccc4..730a515e28 100644 --- a/test/unit/miner-discovery-index-client.test.ts +++ b/test/unit/miner-discovery-index-client.test.ts @@ -9,6 +9,7 @@ vi.mock("../../packages/loopover-miner/lib/logger.js", () => ({ getLogger: () => logSpy, })); +import { normalizeDiscoveryIndexRequest } from "../../packages/loopover-engine/src/index"; import { DISCOVERY_INDEX_URL_FLAG, DISCOVERY_PLANE_FLAG, @@ -71,7 +72,7 @@ describe("queryDiscoveryIndex (#7168)", () => { it("posts the normalized query and returns the normalized response when enabled", async () => { const fetchImpl = vi.fn(async (url: string, init: RequestInit) => { expect(url).toBe("https://discovery.example.internal/v1/discovery-index/query"); - expect(JSON.parse(String(init.body))).toMatchObject({ repos: ["a/b"] }); + expect(JSON.parse(String(init.body))).toMatchObject({ contractVersion: 1, repos: ["a/b"] }); return Response.json({ contractVersion: 1, candidates: [ @@ -96,6 +97,31 @@ describe("queryDiscoveryIndex (#7168)", () => { expect(response.candidates[0]).toMatchObject({ repoFullName: "a/b", issueNumber: 1 }); }); + it("sends the contract version with the query fields at the top level, in a body the server-side parse round-trips (#9615)", async () => { + let sentBody: unknown; + const fetchImpl = vi.fn(async (_url: string, init: RequestInit) => { + sentBody = JSON.parse(String(init.body)); + return Response.json({ contractVersion: 1, candidates: [], nextCursor: null }); + }); + await queryDiscoveryIndex({ repos: ["a/b"], searchTerms: [" help wanted "] }, { env: ENABLED_ENV, fetchImpl }); + + expect(sentBody).toEqual({ + contractVersion: 1, + repos: ["a/b"], + orgs: [], + searchTerms: ["help wanted"], + limit: 50, + cursor: null, + }); + // The server parses this exact body with normalizeDiscoveryIndexRequest (top-level fields, #9615): + // the widened body round-trips to the same normalized query the client computed, with no warning. + const reparsed = normalizeDiscoveryIndexRequest(sentBody); + expect(reparsed.warnings).toEqual([]); + expect(reparsed.request.query).toEqual( + normalizeDiscoveryIndexRequest({ repos: ["a/b"], searchTerms: [" help wanted "] }).request.query, + ); + }); + it("fails open (returns empty) on a non-ok response or a thrown error", async () => { const notOk = await queryDiscoveryIndex({ repos: ["a/b"] }, { env: ENABLED_ENV, fetchImpl: async () => new Response("err", { status: 500 }) }); expect(notOk.candidates).toEqual([]);