fix(eval): refuse a context window wider than the retrieval depth - #201
Merged
mrsibe merged 1 commit intoSep 30, 2026
Merged
Conversation
The sweep contained cells the harness cannot fill. The harness retrieves `candidateK` passages and the context metrics look at the first `contextK` of them, so `candidateK=5, contextK=8` reports on five passages while claiming eight. The dashboard showed `contextK=5` and `contextK=8` at `candidateK=5` as **identical** — and because `evidencePrecisionAtK` divides by the passages actually retrieved, nothing exposed it. Found in the #192 review. A wrong number that looks like a measurement is worse than a failure, so the invariant is enforced rather than documented: - **The harness refuses it.** `contextK > candidateK` throws before indexing, naming both numbers: `contextK (8) cannot exceed candidateK (5): the harness retrieves candidateK passages, so a wider context window can never be filled.` - **The sweep skips those cells** and says so. The report lists the skipped combinations in a "Skipped cells" section instead of quietly omitting rows, because "we did not measure this" and "this measured the same as its neighbour" are different statements and the old output showed the second while meaning the first. `docs/eval/sweep-v1.6.*` is regenerated: 22 rows instead of 24, and the misleading `candidateK=5 / contextK=8` row is gone rather than silently equal to `5`. The production cells (`candidateK=20, contextK ∈ {3,5,8}`) are unaffected. ## Testing - `npm run typecheck` — clean - `npm test` — 502 pass, including 3 new harness-invariant tests that need no vector store, because the check runs before any database work: the refusal, the message naming both numbers, and `contextK == candidateK` being accepted - `node_modules/electron/dist/electron.exe . --eval-harness --eval-candidate-k=5 --eval-context-k=8` — refused with the message above - `npm run eval:sweep` — 22 rows, 2 skipped and listed Part of #192 (review follow-up).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #199 (
feat/eval-sweep-dashboard). Retarget tomainafter the chain merges.What does this PR do?
Refuses a context window wider than the retrieval depth, instead of reporting it as if it were measured. Found in the #192 review.
The bug
The harness retrieves
candidateKpassages and the context metrics look at the firstcontextKof them.candidateK=5, contextK=8therefore reports on five passages while claiming eight. The dashboard showed:Identical — "8 is as good as 5", apparently. It was not a measurement at all: passages 6–8 did not exist.
evidencePrecisionAtKdivides by the passages actually retrieved, so nothing exposed it either.The fix
A wrong number that looks like a measurement is worse than a failure, so the invariant is enforced rather than documented.
contextK > candidateKthrows before indexing, naming both numbers:contextK (8) cannot exceed candidateK (5): the harness retrieves candidateK passages, so a wider context window can never be filled.docs/eval/sweep-v1.6.*is regenerated: 22 rows instead of 24, and the misleadingcandidateK=5 / contextK=8row is gone rather than silently equal to5.The production cells (
candidateK=20, contextK ∈ {3,5,8}) are unaffected.Testing
npm run typecheck— cleannpm test— 502 pass, including 3 new harness-invariant tests that need no vector store, because the check runs before any database work: the refusal, the message naming both numbers, andcontextK == candidateKbeing acceptedelectron . --eval-harness --eval-candidate-k=5 --eval-context-k=8— refused with the message abovenpm run eval:sweep— 22 rows, 2 skipped and listedRelated
Part of #192 (review follow-up).