Skip to content

engine(discovery): contractVersion is never sent, read, or validated on either side of the wire #9615

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

packages/loopover-engine/src/discovery-index-contract.ts is the versioned wire contract between the
miner and the optional hosted discovery-index. It exports DISCOVERY_INDEX_CONTRACT_VERSION = 1 and
declares contractVersion: number on both DiscoveryIndexRequest and DiscoveryIndexResponse.

Nothing anywhere reads it:

  • normalizeDiscoveryIndexRequest returns
    { request: { contractVersion: DISCOVERY_INDEX_CONTRACT_VERSION, query }, warnings } — the raw
    contractVersion on the incoming object is never inspected.
  • normalizeDiscoveryIndexResponse does the same: whatever version the hosted server declared is
    discarded and replaced with the local constant. A v2 server answering a v1 client is relabelled as
    v1 and processed as if nothing were wrong.
  • packages/loopover-miner/lib/discovery-index-client.ts's queryDiscoveryIndex builds
    const { request } = normalizeDiscoveryIndexRequest(query) and then sends
    body: JSON.stringify(request.query) — so contractVersion never reaches the wire at all on the
    query path.
  • The server (packages/discovery-index/src/app.ts, POST /v1/discovery-index/query) calls
    normalizeDiscoveryIndexRequest(body) and uses only request.query; its soft-claim route's
    parseSoftClaimRequest (packages/discovery-index/src/soft-claim.ts:46-56) reads repoFullName,
    issueNumber and action and ignores contractVersion.

The field is therefore inert on all four paths. That is a real gap because the module is explicitly
built as a tolerant parser over "the OPTIONAL, only-partially-trusted hosted index" (module header,
and the #6774 hardening at line 239) — a version skew is precisely the condition its warnings channel
exists to surface, and it currently reports nothing. Note that buildSoftClaimRequest
(packages/loopover-engine/src/discovery-soft-claim.ts:85) does put contractVersion on its
payload, so the request side is also internally inconsistent: one of the two request builders puts the
version on the wire and the other strips it.

This issue makes the field carry its declared meaning without changing any accept/reject behaviour.

Requirements

  • normalizeDiscoveryIndexRequest MUST, when the raw input carries a contractVersion that is a
    number and is not equal to DISCOVERY_INDEX_CONTRACT_VERSION, push a warning of exactly the form
    `DiscoveryIndexRequest declared contractVersion ${received}; this build speaks ${DISCOVERY_INDEX_CONTRACT_VERSION}.`.
    A contractVersion that is absent, or is not a number, MUST NOT produce a warning (tolerant-parser
    convention: an older/looser sender is not an error).
  • normalizeDiscoveryIndexResponse MUST do the same, with DiscoveryIndexResponse in place of
    DiscoveryIndexRequest in the message.
  • Neither function may start throwing, may drop candidates, or may change the emitted
    contractVersion (it stays DISCOVERY_INDEX_CONTRACT_VERSION on both outputs) — the warning is the
    only behaviour change.
  • queryDiscoveryIndex in packages/loopover-miner/lib/discovery-index-client.ts MUST send the whole
    normalized request (JSON.stringify(request)) rather than request.query, so the version actually
    reaches the server. normalizeDiscoveryIndexRequest already reads repos/orgs/searchTerms/
    limit/cursor off the top level, so the server MUST be able to parse the widened body unchanged —
    a test must prove that.
  • No change to packages/discovery-index/src/app.ts or to parseSoftClaimRequest.

⚠️ Required pattern: follow this module's own existing warning style — clampLimit
(packages/loopover-engine/src/discovery-index-contract.ts:145-155) and the #6774 candidate cap
(lines 244-247) both push a single templated string into the warnings array and continue. What does
NOT satisfy this issue: throwing or returning null on a version mismatch; rejecting the response's
candidates; adding a new exported assertContractVersion helper as a separate surface; introducing
version negotiation (min/max supported ranges, downgrade paths); or bumping
DISCOVERY_INDEX_CONTRACT_VERSION.

Deliverables

  • normalizeDiscoveryIndexRequest({ contractVersion: 99, repos: ["a/b"] }).warnings contains the
    exact request-side message above, and .request.contractVersion === 1, asserted by a new named
    test.
  • normalizeDiscoveryIndexResponse({ contractVersion: 99, candidates: [] }).warnings contains the
    exact response-side message, and .response.contractVersion === 1, asserted by a new named test.
  • normalizeDiscoveryIndexRequest({ repos: ["a/b"] }) and
    normalizeDiscoveryIndexRequest({ contractVersion: "1", repos: ["a/b"] }) each produce no
    version warning, asserted by new test cases (both non-warning arms).
  • normalizeDiscoveryIndexResponse({ contractVersion: 1, candidates: [<one valid candidate>] })
    produces no warning and still returns that candidate, asserted by a test.
  • queryDiscoveryIndex sends a JSON body whose parsed form has a top-level contractVersion of
    1 and the query fields, asserted by a new test using the existing fetchImpl injection
    seam in DiscoveryIndexClientOptions.
  • A test feeding that exact body back through normalizeDiscoveryIndexRequest and asserting the
    resulting request.query is deep-equal to the one the client normalized — proving the server's
    parse is unaffected by the widened body.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
adding the two warnings without fixing the client so the version still never reaches the wire — does
not resolve this issue.

Test Coverage Requirements

packages/loopover-engine/src/**/*.ts and packages/loopover-miner/lib/**/*.ts are both inside
coverage.include in vitest.config.ts; packages/loopover-engine/src/ additionally carries its own
engine Codecov flag. The 99%+ branch-counted codecov/patch gate applies to this whole diff. Both
arms of each new conditional need a test: contractVersion absent, present-and-equal,
present-and-different, and present-but-not-a-number — on the request side and on the response side
independently. The two mismatch cases are the required named regression tests.

Expected Outcome

A hosted discovery-index speaking a different contract version is surfaced through the parser's
existing warnings channel instead of being silently relabelled, and the query request actually
carries the version field its own type declares — the same thing buildSoftClaimRequest already does.

Links & Resources

  • packages/loopover-engine/src/discovery-index-contract.ts (lines 14, 34-37, 64-69, 166-181,
    232-259)
  • packages/loopover-engine/src/discovery-soft-claim.ts:85 (the request builder that already sends it)
  • packages/loopover-miner/lib/discovery-index-client.ts (queryDiscoveryIndex, EMPTY_QUERY_RESPONSE)
  • packages/discovery-index/src/app.ts (POST /v1/discovery-index/query)

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions