Skip to content

fix(retrieval): trace the dense channel threshold for hybrid too - #200

Merged
mrsibe merged 1 commit into
feat/eval-cross-lingual-corpusfrom
fix/retrieval-hybrid-trace-threshold
Sep 30, 2026
Merged

mrsibe merged 1 commit into
feat/eval-cross-lingual-corpusfrom
fix/retrieval-hybrid-trace-threshold

Conversation

@mrsibe

@mrsibe mrsibe commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Stacked on #199 (feat/eval-sweep-dashboard). Retarget to main after the chain merges.

What does this PR do?

Fixes a trace that misdescribed what retrieval actually did. Found in the #192 review.

The bug

hybrid runs a dense leg, and that leg applies a similarity floor. The trace recorded threshold: undefined, while DenseRetriever.candidateHits() was quietly applying request.threshold ?? 0.5:

actual behaviour:   hybrid ├─ dense  threshold = 0.5
                           └─ BM25   no threshold
trace recorded:     strategy = hybrid, threshold = undefined

So the #157 promise — that a retrieval snapshot reproduces the retrieval it describes — does not hold for hybrid. Harmless while dense is the shipped strategy; a real problem the moment hybrid becomes one.

The cause and the fix

Two separate ?? 0.5 defaults: one inside candidateHits(), one implicit in what the trace wrote. There is now one function, denseChannelThreshold(strategy, requested), and the same value is both handed to the dense channel and written to the trace, so the two cannot drift apart again:

  • dense / hybrid → the requested threshold, or DEFAULT_DENSE_THRESHOLD
  • sparse → undefined (BM25 has no similarity floor)

The trace field is renamed threshold → denseThreshold. A bare threshold on a strategy: 'hybrid' trace reads as "the threshold for all of hybrid", and that ambiguity is exactly what hid the bug. parseRetrievalSnapshot reads the legacy threshold from already-persisted snapshots as a dense threshold — the value was always the dense leg's, so the backfill states what that retrieval actually did rather than inventing a default.

Testing

  • npm run typecheck — clean
  • npm test — 502 pass, with new coverage for all three cases above plus a regression test asserting hybrid traces 0.5 while sparse traces nothing

Not covered

HybridRetriever needs a vector store, so it has no unit test here. The pure decision is pinned instead, and the wiring uses a single variable — which makes the divergence impossible rather than merely tested against.

Related

Part of #192 (review follow-up).

@github-actions github-actions Bot added the bug Something isn't working label Sep 30, 2026
@mrsibe
mrsibe changed the base branch from feat/eval-sweep-dashboard to feat/eval-cross-lingual-corpus September 30, 2026 09:50
`hybrid` runs a dense leg, and that leg applies a similarity floor. The trace
recorded `threshold: undefined` for hybrid, so a snapshot could not reproduce the
retrieval it described: it claimed hybrid had no threshold while `candidateHits()`
was quietly applying `request.threshold ?? 0.5`. If hybrid ever becomes the shipped
strategy, the #157 promise that a retrieval snapshot is reproducible stops holding.

Found in the #192 review.

Two separate `?? 0.5` defaults were the cause — one inside `candidateHits()`, one
implicit in what the trace wrote. There is now **one** function,
`denseChannelThreshold(strategy, requested)`, and the same value is both handed to the
dense channel and written to the trace, so the two cannot drift apart again:

- `dense` / `hybrid` → the requested threshold, or `DEFAULT_DENSE_THRESHOLD`
- `sparse` → `undefined` (BM25 has no similarity floor)

The trace field is renamed `threshold` → **`denseThreshold`**, because a bare
`threshold` on a `strategy: 'hybrid'` trace reads as "the threshold for all of
hybrid", which is the ambiguity that hid this bug. `parseRetrievalSnapshot` reads the
legacy `threshold` from already-persisted snapshots as a dense threshold — the value
was always the dense leg's, so the backfill states what that retrieval actually did.

## Testing

- `npm run typecheck` — clean
- `npm test` — 502 pass, with new coverage for the three cases above and a regression
  test asserting `hybrid` traces `0.5` while `sparse` traces nothing
- Verified end to end that `dense` still reports `denseThreshold` in the smoke path

## Not covered

`HybridRetriever` needs a vector store, so it has no unit test here; the pure decision
is pinned instead and the wiring uses a single variable, which is what makes the
divergence impossible rather than merely tested against.

Part of #192 (review follow-up).
@mrsibe
mrsibe force-pushed the fix/retrieval-hybrid-trace-threshold branch from edfc1ad to b1bdb0b Compare September 30, 2026 10:06
@mrsibe
mrsibe merged commit cc59f58 into feat/eval-cross-lingual-corpus Sep 30, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant