Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/loopover-engine/src/discovery-index-contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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.",
]);
});
6 changes: 5 additions & 1 deletion packages/loopover-miner/lib/discovery-index-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
);
Expand Down
29 changes: 29 additions & 0 deletions test/unit/discovery-index-contract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([]);
});
});
28 changes: 27 additions & 1 deletion test/unit/miner-discovery-index-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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: [
Expand All @@ -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([]);
Expand Down