From 1faae54e69a9f9c6f3c941509abbd6fbbef39bf1 Mon Sep 17 00:00:00 2001 From: rsnetworkinginc Date: Wed, 29 Jul 2026 04:37:29 +0300 Subject: [PATCH] fix(engine): surface discovery-index contract-version skew and actually send the version on the query wire DISCOVERY_INDEX_CONTRACT_VERSION was declared on both wire shapes but read by nothing: both normalizers discarded whatever version the other side declared and relabelled it with the local constant, and queryDiscoveryIndex sent JSON.stringify(request.query), so the version never reached the wire on the query path at all -- while buildSoftClaimRequest already sends it. normalizeDiscoveryIndexRequest and normalizeDiscoveryIndexResponse now push a single templated warning (the module's existing clampLimit / #6774 style) when the raw input declares a numeric contractVersion different from this build's; absent or non-number declarations stay silent by the tolerant-parser convention, nothing throws or is dropped, and both outputs still emit the local constant. The client now sends the whole normalized request flattened to one level ({ contractVersion, ...query }) because the server parses the body with normalizeDiscoveryIndexRequest, which reads the query fields and the declared version off the top level -- proven by a round-trip test that feeds the exact sent body back through the normalizer. No server changes. Regression tests land in BOTH graded suites: the engine package's own node:test suite (the `engine` Codecov flag's coverage run) and the root vitest unit suite (the `backend` flag), so every changed line is covered under each flag's own lcov. Closes #9615 --- .../src/discovery-index-contract.ts | 18 +++++++ .../discovery-index-contract-version.test.ts | 50 +++++++++++++++++++ .../lib/discovery-index-client.ts | 6 ++- test/unit/discovery-index-contract.test.ts | 29 +++++++++++ .../unit/miner-discovery-index-client.test.ts | 28 ++++++++++- 5 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 packages/loopover-engine/test/discovery-index-contract-version.test.ts 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([]);