Skip to content

fix(engine): surface discovery-index contract-version skew and actually send the version on the query wire - #9631

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-9615
Jul 29, 2026
Merged

fix(engine): surface discovery-index contract-version skew and actually send the version on the query wire#9631
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-9615

Conversation

@rsnetworkinginc

Copy link
Copy Markdown
Contributor

Closes #9615

Summary

fix(engine): surface discovery-index contract-version skew and actually send the version on the query wire

DISCOVERY_INDEX_CONTRACT_VERSION = 1 was declared on both DiscoveryIndexRequest and DiscoveryIndexResponse but read by nothing: both normalizers discarded whatever version the other side declared and relabelled it with the local constant (a v2 server answering a v1 client was silently processed as v1), and queryDiscoveryIndex sent JSON.stringify(request.query) — so the version never reached the wire on the query path at all, even though buildSoftClaimRequest already puts it on its payload. The module is explicitly a tolerant parser over the "OPTIONAL, only-partially-trusted hosted index"; version skew is exactly what its warnings channel exists to surface, and it reported nothing.

Root cause

The version field was written into both normalizer outputs but never inspected on input, and the query client serialized only the inner query object.

Fix

  • normalizeDiscoveryIndexRequest: when the raw input carries a numeric contractVersion different from DISCOVERY_INDEX_CONTRACT_VERSION, it pushes exactly `DiscoveryIndexRequest declared contractVersion ${received}; this build speaks ${DISCOVERY_INDEX_CONTRACT_VERSION}.` — the module's existing clampLimit / discovery-index-contract.ts caps every request-side list but leaves the response candidate list unbounded #6774 push-one-templated-string-and-continue style. Absent or non-number declarations stay silent (tolerant-parser convention).
  • normalizeDiscoveryIndexResponse: same, with DiscoveryIndexResponse in the message. Neither function throws, drops candidates, or changes the emitted contractVersion — the warning is the only behaviour change.
  • queryDiscoveryIndex now sends the whole normalized request — the contract version WITH the query — flattened to one level: JSON.stringify({ contractVersion: request.contractVersion, ...request.query }). Flattened because the server (packages/discovery-index/src/app.ts) parses the body with normalizeDiscoveryIndexRequest, which reads the query fields (and now the declared version) 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. A round-trip test proves the server's parse is unaffected by the widened body.
  • No change to packages/discovery-index/src/app.ts or parseSoftClaimRequest; no new exported helper; no version negotiation; version constant unbumped.

Diff: packages/loopover-engine/src/discovery-index-contract.ts (+18), packages/loopover-miner/lib/discovery-index-client.ts (+6/−2), plus tests (new engine-suite file +50; vitest +57/−1).

Tests (RED → GREEN)

Against the unfixed source:

Test Files  2 failed (2)
     Tests  4 failed | 47 passed (51)

(the two mismatch-warning tests and the two client wire-shape tests fail pre-fix). After the fix, across the contract, client, server (discovery-index/app), soft-claim, discover-CLI, and repo-segment suites:

Test Files  6 passed (6)
     Tests  154 passed (154)     # after npm run build:miner for the CLI spawn tests
  • normalizeDiscoveryIndexRequest({ contractVersion: 99, repos: ["a/b"] }).warnings contains the exact request-side message; .request.contractVersion === 1.
  • normalizeDiscoveryIndexResponse({ contractVersion: 99, candidates: [] }).warnings contains the exact response-side message; .response.contractVersion === 1.
  • Absent and contractVersion: "1" (non-number) each produce no version warning, on both sides; contractVersion: 1 with a valid candidate produces no warning and keeps the candidate.
  • Client (via the existing fetchImpl seam): the sent JSON body parses to { contractVersion: 1, repos: ["a/b"], orgs: [], searchTerms: ["help wanted"], limit: 50, cursor: null } — top-level version and query fields — and feeding that exact body back through normalizeDiscoveryIndexRequest yields a request.query deep-equal to the one the client normalized, with zero warnings (server parse unaffected).
  • The server suite (test/unit/discovery-index/app.test.ts) passes unchanged.
  • New engine-suite packages/loopover-engine/test/discovery-index-contract-version.test.ts mirrors mismatch/absent/non-number/matching (and the non-mapping response arm) inside the engine package's own node:test suite: 804 tests, 804 pass.

Coverage

This diff spans two graded surfaces and is covered under each flag's own coverage run:

  • engine flag (CI's exact recipe: node --experimental-strip-types scripts/engine-coverage.ts, c8 over the engine package's own node:test suite), lcov cross-referenced against this PR's diff hunks:
OK: packages/loopover-engine/src/discovery-index-contract.ts
  changed lines: 18 | uncovered changed lines: none | partial branches on changed lines: none
  • backend flag (root vitest v8; both packages/loopover-engine/src/** and packages/loopover-miner/lib/** are inside coverage.include):
OK: packages/loopover-engine/src/discovery-index-contract.ts
  changed lines: 18 | uncovered changed lines: none | partial branches on changed lines: none
  whole-file uncovered lines: none | whole-file partial-branch lines: none
OK: packages/loopover-miner/lib/discovery-index-client.ts
  changed lines: 5 | uncovered changed lines: none | partial branches on changed lines: none
  whole-file uncovered lines: none | whole-file partial-branch lines: none

Both arms of each new conditional are tested independently on the request and response sides: contractVersion absent, present-and-equal, present-and-different, and present-but-not-a-number (plus the non-mapping/optional-chain arm on the response side).

Validation commands (repo root)

git diff --check                                  # clean
npm run build --workspace @loopover/engine        # clean tsc build
npm run test --workspace @loopover/engine         # 804 tests, 804 pass, 0 fail
node --experimental-strip-types scripts/engine-coverage.ts   # engine-flag lcov: every patch line covered, zero partial branches
npx vitest run test/unit/discovery-index-contract.test.ts test/unit/miner-discovery-index-client.test.ts test/unit/discovery-index/app.test.ts test/unit/discovery-soft-claim.test.ts test/unit/miner-discover-cli.test.ts test/unit/repo-segment.test.ts   # 154/154 (build:miner first for the CLI spawn tests)
npm run typecheck                                 # clean (root + packages)

…ly 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 /
JSONbored#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 JSONbored#9615
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 02:00:56 UTC

5 files · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This PR fixes a real dead-parameter bug: contractVersion was declared but never read by either normalizer (silent relabeling on skew) and never sent by the miner query client (only request.query was serialized). The fix adds a warning-only skew check on both normalizers and flattens contractVersion into the top-level POST body to match how the server presumably parses it. The change is narrowly scoped, well-tested (including a round-trip test verifying the client's flattened body re-parses cleanly with normalizeDiscoveryIndexRequest), and matches the existing tolerant-parser/warnings-channel convention used elsewhere in this module (clampLimit, #6774 candidate cap).

Nits — 4 non-blocking
  • The server side (packages/discovery-index/src/app.ts) isn't part of this diff, so the claim that flattening contractVersion to the top level matches its parse is asserted but not verifiable from what's shown here.
  • packages/loopover-engine/test/discovery-index-contract-version.test.ts imports from ../dist/index.js rather than ../src/index.ts like the other engine tests reviewed — worth confirming this is the established convention for that test directory and not an accidental drift that could run against a stale build.
  • If packages/discovery-index/src/app.ts is available, verify it does read contractVersion off the top-level body (not a nested query key) to fully close the loop this PR claims to fix.
  • Consider whether a mismatched contractVersion warning on the response side should also be surfaced/logged by the miner client (currently normalizeDiscoveryIndexResponse's warnings are discarded in discovery-index-client.ts — only .response is used), since otherwise the new response-side warning has no consumer yet.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9615
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 27 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor rsnetworkinginc; Gittensor profile; 52 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds numeric-mismatch-only warnings with the exact required message templates to both normalizers, flattens contractVersion into the query client's POST body via spread so the server's unchanged normalizer still parses it, and includes tests covering all six deliverables (warning cases, silent cases for absent/non-number/matching versions, response candidate preservation, and the client r

Review context
  • Author: rsnetworkinginc
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: not available
  • Official Gittensor activity: 52 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Await review-lane availability.
  • Then work through the remaining 1 step in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.10%. Comparing base (8e3e4f5) to head (1faae54).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9631      +/-   ##
==========================================
+ Coverage   90.08%   90.10%   +0.01%     
==========================================
  Files         889      889              
  Lines      112024   112042      +18     
  Branches    26587    26591       +4     
==========================================
+ Hits       100921   100950      +29     
+ Misses       9773     9762      -11     
  Partials     1330     1330              
Flag Coverage Δ
backend 95.56% <100.00%> (+<0.01%) ⬆️
engine 66.96% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...es/loopover-engine/src/discovery-index-contract.ts 97.15% <100.00%> (+4.37%) ⬆️
...kages/loopover-miner/lib/discovery-index-client.ts 100.00% <ø> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 80827df into JSONbored:main Jul 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant