From b1bdb0bc772716934d336f393d797da187cce5be Mon Sep 17 00:00:00 2001 From: MrSibe Date: Wed, 30 Sep 2026 17:23:22 +0800 Subject: [PATCH] fix(retrieval): trace the dense channel's threshold for hybrid too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). --- src/main/services/retrieval/DenseRetriever.ts | 8 ++--- .../services/retrieval/HybridRetriever.ts | 20 +++++++---- src/main/services/retrieval/trace.ts | 4 +-- src/main/services/retrieval/types.ts | 32 ++++++++++++++++- src/shared/types/chat.ts | 12 +++++-- src/shared/utils/answerSources.ts | 8 +++-- test/answerSources.test.ts | 25 ++++++++++++-- test/retrievalContract.test.ts | 34 ++++++++++++++++--- 8 files changed, 118 insertions(+), 25 deletions(-) diff --git a/src/main/services/retrieval/DenseRetriever.ts b/src/main/services/retrieval/DenseRetriever.ts index 95fece4..dc6e715 100644 --- a/src/main/services/retrieval/DenseRetriever.ts +++ b/src/main/services/retrieval/DenseRetriever.ts @@ -12,7 +12,7 @@ import type { EmbeddingService } from '../EmbeddingService' import type { CandidateHit } from './candidates' import { hydrateEvidence } from './evidence' import { buildRetrievalTrace } from './trace' -import { effectiveCandidateK, DEFAULT_TOP_K } from './types' +import { denseChannelThreshold, effectiveCandidateK, DEFAULT_TOP_K } from './types' import type { RetrievalRequest, RetrievalResult, Retriever } from './types' const STRATEGY = 'dense' @@ -31,7 +31,7 @@ export class DenseRetriever implements Retriever { // 第一阶段按 `candidateK` 取宽；`topK` 的截断由调用方决定，因为 hybrid 需要的是 // 比最终交付更宽的一池子候选。 const candidateK = effectiveCandidateK(request) - const threshold = request.threshold ?? 0.5 + const threshold = denseChannelThreshold('dense', request.threshold) // E5 要求 query 前缀，与索引时的 document 前缀区分 await this.embeddingService.ensureReady() @@ -51,7 +51,7 @@ export class DenseRetriever implements Retriever { async search(request: RetrievalRequest): Promise { const topK = request.topK ?? DEFAULT_TOP_K const candidateK = effectiveCandidateK(request) - const threshold = request.threshold ?? 0.5 + const threshold = denseChannelThreshold('dense', request.threshold) const startedAt = performance.now() // 单策略没有可精排的下游，取宽再截到 `topK` 与直接按 `topK` 查 KNN 等价； @@ -66,7 +66,7 @@ export class DenseRetriever implements Retriever { filter: request.filter, candidateK, topK, - threshold, + denseThreshold: threshold, durationMs: performance.now() - startedAt }) } diff --git a/src/main/services/retrieval/HybridRetriever.ts b/src/main/services/retrieval/HybridRetriever.ts index d0a78b9..7f946bf 100644 --- a/src/main/services/retrieval/HybridRetriever.ts +++ b/src/main/services/retrieval/HybridRetriever.ts @@ -6,6 +6,7 @@ import { DenseRetriever } from './DenseRetriever' import { hydrateEvidence } from './evidence' import { buildRetrievalTrace } from './trace' import { + denseChannelThreshold, effectiveCandidateK, DEFAULT_TOP_K, type RetrievalRequest, @@ -44,9 +45,12 @@ export class HybridRetriever implements Retriever { const startedAt = performance.now() let hits: CandidateHit[] - // BM25 没有「相似度阈值」这个概念，所以 sparse 的 trace 里 threshold 保持缺省， - // 而不是拿 dense 的 0.5 冒充。 - let threshold: number | undefined + // 传给 dense 通道的值和写进 trace 的值是 **同一个** 变量，来自同一个函数。 + // `hybrid` 也跑 dense 所以也有阈值；`sparse` 没有，于是它是 undefined。 + // + // 分开算两次就是 #192 评审发现的 bug：hybrid 的 dense 腿用着 0.5，而 trace 写 + // `undefined`，快照于是声称那次 hybrid 没有阈值。 + const denseThreshold = denseChannelThreshold(strategy, request.threshold) if (strategy === 'sparse') { hits = searchChunksFts(request.notebookId, request.query, { @@ -54,15 +58,17 @@ export class HybridRetriever implements Retriever { documentIds: request.filter?.documentIds }).slice(0, topK) } else if (strategy === 'hybrid') { - const denseHits = await this.dense.candidateHits(request) + const denseHits = await this.dense.candidateHits({ ...request, threshold: denseThreshold }) const sparseHits = searchChunksFts(request.notebookId, request.query, { limit: candidateK, documentIds: request.filter?.documentIds }) hits = rrfFuse([denseHits, sparseHits]).slice(0, topK) } else { - threshold = request.threshold ?? 0.5 - hits = (await this.dense.candidateHits({ ...request, threshold })).slice(0, topK) + hits = (await this.dense.candidateHits({ ...request, threshold: denseThreshold })).slice( + 0, + topK + ) } const evidence = hits.length === 0 ? [] : hydrateEvidence(getDatabase(), hits) @@ -74,7 +80,7 @@ export class HybridRetriever implements Retriever { filter: request.filter, candidateK, topK, - threshold, + denseThreshold, durationMs: performance.now() - startedAt }) } diff --git a/src/main/services/retrieval/trace.ts b/src/main/services/retrieval/trace.ts index ae788cc..70a7c80 100644 --- a/src/main/services/retrieval/trace.ts +++ b/src/main/services/retrieval/trace.ts @@ -5,7 +5,7 @@ export interface RetrievalTraceInput { filter?: RetrievalFilter candidateK: number topK: number - threshold?: number + denseThreshold?: number durationMs: number } @@ -30,7 +30,7 @@ export function buildRetrievalTrace(input: RetrievalTraceInput): RetrievalTrace durationMs: input.durationMs } - if (input.threshold !== undefined) trace.threshold = input.threshold + if (input.denseThreshold !== undefined) trace.denseThreshold = input.denseThreshold return trace } diff --git a/src/main/services/retrieval/types.ts b/src/main/services/retrieval/types.ts index 921a345..fc04fae 100644 --- a/src/main/services/retrieval/types.ts +++ b/src/main/services/retrieval/types.ts @@ -110,6 +110,27 @@ export function effectiveCandidateK(request: { return Math.max(request.candidateK ?? DEFAULT_CANDIDATE_K, topK) } +/** 没有指定时 dense 通道的相似度下限，与引入双 K 之前一致。 */ +export const DEFAULT_DENSE_THRESHOLD = 0.5 + +/** + * 某个策略真正作用在 **dense 通道** 上的相似度下限。 + * + * `hybrid` 也跑 dense，所以它同样有阈值；只有 `sparse` 没有，因为 BM25 没有「相似 + * 度阈值」这个概念。 + * + * 用 **一个** 函数产出这个值，是为了让「传给 dense 通道的值」和「写进 trace 的值」无法 + * 再分开：#192 评审发现的 bug 就是它们各自有一个 `?? 0.5` —— hybrid 的 dense 腿用了 + * 0.5，而 trace 写的 `threshold: undefined`，于是快照无法复现那次检索。 + */ +export function denseChannelThreshold( + strategy: RetrievalStrategy, + requested?: number +): number | undefined { + if (strategy === 'sparse') return undefined + return requested ?? DEFAULT_DENSE_THRESHOLD +} + /** * 一次检索实际生效的参数。 * @@ -127,7 +148,16 @@ export interface RetrievalTrace { candidateK: number /** 最终交付的证据条数。 */ topK: number - threshold?: number + /** + * 真正作用在 **dense 通道** 上的相似度下限。 + * + * 字段名不是 `threshold` 而是 `denseThreshold`，因为 `hybrid` 也在跑 dense：它不 + * 是「没有阈值」，而是 dense 那一路有 0.5。叫 `threshold` 会让快照看上去说 hybrid + * 没有阈值，于是“这次检索是怎么发生的”就复现不出来了（#192 评审）。 + * + * 缺省只表示 **dense 通道没跑**（`sparse`），不是「阈值等于 0」。 + */ + denseThreshold?: number durationMs: number } diff --git a/src/shared/types/chat.ts b/src/shared/types/chat.ts index 55e7170..fdd237b 100644 --- a/src/shared/types/chat.ts +++ b/src/shared/types/chat.ts @@ -162,8 +162,16 @@ export interface RetrievalSnapshot { */ candidateK: number topK: number - /** 缺省表示该策略没有阈值，不是「阈值等于 0」。 */ - threshold?: number + /** + * 作用在 **dense 通道** 上的相似度下限。 + * + * `hybrid` 也有这个值（它的 dense 那一路），所以缺省只表示 dense 通道没跑 + * （`sparse`），不是「阈值等于 0」。 + * + * #192 评审之前这个字段叫 `threshold`：那时 hybrid 的快照写的是 undefined，而它 + * 实际跑了带 0.5 的 dense。旧记录里的值本来就是 dense 阈值，解析时按 dense 阈值读。 + */ + denseThreshold?: number durationMs: number } diff --git a/src/shared/utils/answerSources.ts b/src/shared/utils/answerSources.ts index ffb7271..321b478 100644 --- a/src/shared/utils/answerSources.ts +++ b/src/shared/utils/answerSources.ts @@ -115,8 +115,12 @@ export const parseRetrievalSnapshot = (metadata: unknown): RetrievalSnapshot | n } } - const threshold = toFiniteNumber(candidate.threshold) - if (threshold !== undefined) snapshot.threshold = threshold + // Written as `threshold` before the #192 review established that `hybrid` also runs a + // dense leg. A legacy value is a dense threshold and is read as one; the backfill says + // what that retrieval actually did. + const denseThreshold = + toFiniteNumber(candidate.denseThreshold) ?? toFiniteNumber(candidate.threshold) + if (denseThreshold !== undefined) snapshot.denseThreshold = denseThreshold return snapshot } diff --git a/test/answerSources.test.ts b/test/answerSources.test.ts index 5de2726..3996f39 100644 --- a/test/answerSources.test.ts +++ b/test/answerSources.test.ts @@ -153,7 +153,7 @@ test('a retrieval snapshot round-trips', () => { scope: { documentIds: ['doc_1', 'doc_2'] }, candidateK: 20, topK: 8, - threshold: 0.5, + denseThreshold: 0.5, durationMs: 42.4 } }) @@ -163,7 +163,7 @@ test('a retrieval snapshot round-trips', () => { scope: { documentIds: ['doc_1', 'doc_2'] }, candidateK: 20, topK: 8, - threshold: 0.5, + denseThreshold: 0.5, durationMs: 42.4 }) }) @@ -174,7 +174,26 @@ test('a snapshot without a scope means the whole notebook', () => { }) assert.deepEqual(snapshot?.scope, {}) - assert.equal(snapshot?.threshold, undefined) + assert.equal(snapshot?.denseThreshold, undefined) +}) + +/** + * Snapshots written before the #192 review called the field `threshold`. The value was + * always the dense leg's floor, so it is read as one rather than dropped. + */ +test('a snapshot with the legacy `threshold` field reads it as the dense threshold', () => { + const snapshot = parseRetrievalSnapshot({ + retrievalSnapshot: { + strategy: 'hybrid', + scope: {}, + topK: 3, + threshold: 0.5, + durationMs: 7 + } + }) + + assert.equal(snapshot?.denseThreshold, 0.5) + assert.equal(snapshot?.candidateK, 3) }) /** diff --git a/test/retrievalContract.test.ts b/test/retrievalContract.test.ts index 9d32292..a662346 100644 --- a/test/retrievalContract.test.ts +++ b/test/retrievalContract.test.ts @@ -2,7 +2,9 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { buildRetrievalTrace } from '../src/main/services/retrieval/trace.ts' import { + denseChannelThreshold, DEFAULT_CANDIDATE_K, + DEFAULT_DENSE_THRESHOLD, effectiveCandidateK } from '../src/main/services/retrieval/types.ts' @@ -20,7 +22,7 @@ test('a trace carries the effective search parameters and no empty fields', () = strategy: 'dense', candidateK: 20, topK: 5, - threshold: 0.5, + denseThreshold: 0.5, durationMs: 12.5 }) @@ -29,16 +31,40 @@ test('a trace carries the effective search parameters and no empty fields', () = scope: {}, candidateK: 20, topK: 5, - threshold: 0.5, + denseThreshold: 0.5, durationMs: 12.5 }) - // An unset threshold means "no threshold", not "threshold 0". + // An absent dense threshold means "no dense leg ran", not "threshold 0". assert.equal( - 'threshold' in buildRetrievalTrace({ strategy: 'dense', candidateK: 20, topK: 5, durationMs: 1 }), + 'denseThreshold' in + buildRetrievalTrace({ strategy: 'sparse', candidateK: 20, topK: 5, durationMs: 1 }), false ) }) +/** + * The bug the #192 review found: `hybrid` runs a dense leg, so it *has* a similarity + * floor. The retriever applied 0.5 to that leg while the trace recorded `undefined`, so + * the snapshot could not reproduce the retrieval it was describing. + */ +test('a strategy traces the threshold its dense leg actually applied', () => { + assert.equal(denseChannelThreshold('dense'), DEFAULT_DENSE_THRESHOLD) + assert.equal(denseChannelThreshold('hybrid'), DEFAULT_DENSE_THRESHOLD) + assert.equal(denseChannelThreshold('hybrid', 0.3), 0.3) + assert.equal(denseChannelThreshold('dense', 0), 0) + // Only a strategy with no dense leg has no dense threshold. + assert.equal(denseChannelThreshold('sparse', 0.3), undefined) + + const hybrid = buildRetrievalTrace({ + strategy: 'hybrid', + candidateK: 20, + topK: 3, + denseThreshold: denseChannelThreshold('hybrid'), + durationMs: 4 + }) + assert.equal(hybrid.denseThreshold, DEFAULT_DENSE_THRESHOLD) +}) + test('the trace keeps the first-stage width and the final count apart', () => { const trace = buildRetrievalTrace({ strategy: 'hybrid',