fix(eval): record the answer text the quality gates judge, and delete a waiver that never matched - #2602
Conversation
… a waiver that never matched The Eval Canary has been red since 2026-08-22 — eleven days across four runs — on one step, "Answer-quality subset (live generation)". Golden retrieval passed in every one of those runs; only answer generation blocked. Two defects, both of which made the red uninvestigable rather than merely present. 1. The harness discards the text it judges. The blocking gate is a text-shape predicate: isLaunderedGuidanceWrapperAnswer inspects the answer's opening sentence. Nothing in the uploaded artifact records that sentence, or the answer at all — I downloaded both the failing and the last-good artifact to confirm it. So the only way to see what the gate rejected was to pay for another live run. Record it now: the opening sentence for every case, and the full answer only for cases a text-shape gate rejected, so reports stay small. 2. The allowance has never matched anything. PR #2301 added two entries keyed `quality-<id>` while every id in ragEvalCases is bare, and the comparison is `allowed.id === result.id`. One entry names a case that does not exist in the fixture at all. So the file read as though two degradations were signed off while the gate ran at zero tolerance throughout. Emptying it is a no-op at runtime — proven, since neither entry could match — and stops the source asserting a waiver that is not real. The unit test made the SAME typo, inventing fixtures with the same prefixed ids, so config and test agreed and neither could fail. That is why review did not catch it. The mechanism test now supplies its own allowance through a new optional parameter, so it can no longer be satisfied by whatever the production list happens to contain, and a new test fails if any allowance id names no case in the fixture. Mutation-tested: it goes red on a bad id and green when restored. This deliberately does NOT make the canary green. Three real cases — clozapine-monitoring, clozapine-monitoring-paraphrase and nocc-requirements — are degrading to source-backed review, and whether their answers are genuinely poor or the predicate is over-firing cannot be judged until the next run records the text. Adding them to the allowance is what the threshold's own comment forbids: "Do not add an entry to clear a red run." Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The push guard caught this: the result object gained answerOpeningSentence and answerText without the type learning about them, so tsc failed while the unit tests passed. Tests exercise the report builder through its own fixtures and never typecheck the production write path — running them was not sufficient evidence for this change, and the guard was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_17f6cd05-3998-41e6-95da-0057addd8a61) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ec636d626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f579b369-4004-4811-8f42-5ada2bcdec53) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_61d3617a-9dba-44df-be2d-5d67ff4a5840) |
Pull request was converted to draft
eval-quality.ts's answerText field was meant to record the answer prose a text-shape quality gate judged and rejected, but by the time it read answer.answer, finalizeRagAnswerQualityCore had already overwritten it with a generic evidence-gap response — and for the source-backed-review branch in rag.ts, a second overwrite replaced it again with an unrelated fallback wrapper. Neither overwrite left the originally rejected text anywhere the harness could read. Add RagAnswer.rejectedCandidateText, populated at the two points in rag-extractive-answer.ts that discard a rejected candidate's answer text, and threaded through rag.ts's source-backed-review branch so it survives that branch's second candidate substitution. eval-quality.ts now prefers this field over answer.answer when recording what a text-shape gate rejected. Purely additive: no change to routing, scoring, or the answer delivered to users. RAG impact: no retrieval behaviour change — captures the pre-fallback rejected candidate's text into a new debug-only field for the eval harness; does not alter which candidate is chosen, its content, or any scoring/ranking/routing decision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ENQDEPFuwDZNoNssvV1PTc
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #15845 (failure). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…ainability budget The previous commit's rejectedCandidateText debug field pushed rag.ts to 4368 lines, 6 over the 4362-line no-growth budget enforced by check-maintainability-budgets.mjs. Tightening the new code alone (shortening its comment) could not close the gap without shrinking below the minimum Prettier-formatted footprint of the capture/restore statements it needs. Mechanically move the self-contained LLM query-classifier fallback cluster (schemas, memo cache, analyzeQueryWithClassifierFallback, and the shared uniqueTextValues helper) into a new src/lib/rag/rag-classifier-fallback.ts, matching the existing pattern of rag.ts delegating to sibling rag-*.ts modules. Also extract the small generic awaitWithCallerSignal helper into src/lib/rag/rag-abort-signal.ts so both rag.ts and the new module can import it without a circular dependency. rag.ts re-exports analyzeQueryWithClassifierFallback and resetClassifierVerdictMemoForTests so existing test imports (rag.analyzeQueryWithClassifierFallback, etc.) are unaffected. No behavior change: this is a pure code move plus import/export bookkeeping. rag.ts drops from 4368 to 4075 lines, well under the 4362 ceiling. RAG impact: no retrieval behaviour change — mechanical extraction of the classifier-fallback cluster into its own module; the code and its call sites are unchanged, only their file location and import paths move. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ENQDEPFuwDZNoNssvV1PTc
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d50a21f0-4ff1-489c-bc6a-e60a2a831a90) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b58e7f89-3841-4754-855f-f3117441c0fc) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_47dbce21-cff9-4a88-9aec-a8ca82964778) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_db6a342f-3f2c-4ad2-b56a-3cb6b9f36f80) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cf27280b-8cf9-4ba8-914e-edbad67813a8) |
Summary
source_backed_review_fallbackallowance that has never matched a case, and add a test that fails if an allowance entry ever names a case that does not exist.This deliberately does not turn the canary green. Three real cases are degrading and nobody can yet see why. Making it green would mean waving them through, which the threshold's own comment forbids.
The problem
Eval Canaryhas been failing since 2026-08-22 — eleven days and four runs. In every one of them theGolden retrieval eval (live corpus)step passed; onlyAnswer-quality subset (live generation)failed. So retrieval, ranking and recall have been healthy throughout, and the red is confined to answer generation.The blocking threshold is
source_backed_review_fallback_unaccounted, and the three cases tripping it areclozapine-monitoring,clozapine-monitoring-paraphraseandnocc-requirements. All three are grounded, supported and cited — they degrade to a source-backed review answer, which is the designed conservative behaviour, not a safety failure.Defect 1 — the harness discards the text it judges
The gate that blocks is a text-shape predicate:
isLaunderedGuidanceWrapperAnswer(added957b76afa, 2026-08-22, six hours before the first red) inspects the answer's opening sentence and rejects a fluent non-sentence.I downloaded the uploaded artifacts from both the freshest failure (
33582708071) and the last success (32111839806, 2026-08-18) and checked every file in each. The per-case record stores the question, route, model, citations, timings, cost and routing reason — and no answer text at all. The Markdown twin andanswer-quality.logare the same report.golden-retrieval.jsonis an unrelated fixture that stores retrieved snippets, never a generated answer.So the only way to see what the gate rejected was to spend another live run. That is why this sat red for eleven days: the block names a case, and then the evidence needed to judge it has already been thrown away.
This PR records it:
answerOpeningSentencefor every case (the exact span the predicate reads), andanswerTextonly for cases a text-shape gate rejected, so reports stay small.Defect 2 — the allowance has never matched anything
PR #2301 (
62cfcab48, 2026-08-22, ~80 minutes after the first red) added two accepted-degradation entries:Every case id in
ragEvalCasesis bare (discharge-documentation), and the comparison isallowed.id === result.id. Verified against the live artifact: no case id in either run carries aquality-prefix, andantipsychotic-metabolic-monitoringdoes not exist as a case at all.Neither entry has matched a single case since the day it landed. The source has read as though two degradations were signed off while the gate ran at zero tolerance throughout, on cases nobody had accepted.
Emptying the list is therefore a no-op at runtime — it cannot change a comparison that never matched — and it stops the file asserting a waiver that does not exist. Re-accepting a degradation now requires naming a real case id, with evidence.
Why review did not catch it
The unit test made the same typo. It invented fixture cases with the same
quality-prefixed ids, so the test proved the mechanism worked in isolation while the production path matched nothing. Config and test agreed with each other, so both passed and neither could fail.Fixed two ways: the mechanism test now supplies its own allowance through a new optional
sourceBackedReviewFallbackAllowanceparameter, so it can never again be satisfied by whatever the production list happens to contain; and a new test asserts that every production allowance id exists inragEvalCases.Verification
covering
eval-quality,rag-guidance-wrapper-quality-gate,eval-retrievaland the RAG source-pin contract testrag-imputation-contract.npm run typecheckexits 0.The new guard was mutation-tested rather than assumed. Adding an allowance entry naming a nonexistent case turns it red:
and it returns green when the entry is removed. A check nobody has watched fail is not yet a check.
The second commit exists because the push guard caught a real typecheck error the unit tests could not: the result object gained two fields without the type learning about them. Recorded honestly rather than squashed away — running the tests was not sufficient evidence for this change, and the guard was.
No live Supabase, OpenAI or Railway command was run. The artifacts inspected are already-published CI outputs.
RAG impact: no retrieval behaviour change — captures the pre-fallback rejected candidate's text into a new debug-only field for the eval harness; does not alter which candidate is chosen, its content, or any scoring/ranking/routing decision.
Risk and rollout
rejectedCandidateTextis a new optional debug-only field onRagAnswer, populated in exactly two places (finalQualityFailure,finalizeRagAnswerQualityCore) right before the existing fallback substitution already replacesanswer.answer; it is read only byscripts/eval-quality.tsand asserted by tests, never by product/UI code. The allowance-list change is a documented no-op at runtime (proved above). TheanalyzeQueryWithClassifierFallback/awaitWithCallerSignalmoves intorag-classifier-fallback.ts/rag-abort-signal.tsare pure relocations — same logic, same exports re-exported fromrag.ts— covered by the unchanged, still-passing test suite.Clinical Governance Preflight
This PR touches
src/lib/rag/rag.ts,src/lib/rag/rag-extractive-answer.ts, andsrc/lib/types.ts(RagAnswer), which trips the clinical-risk file classifier — not because any clinical decision-support behaviour changes, but because the diff is inside the RAG answer pipeline. Every item below is answered against the actual diff, not by assumption.Unchanged; nothing here touches citation, source-linking, or verification logic.
None introduced or touched.
Clinical KB Database(sjrfecxgysukkwxsowpy)Unchanged; no Supabase env value, migration, or configured project touched.
Unchanged;
rejectedCandidateTextis server-side eval-harness-only and never serialized to a client response.Unchanged.
Unchanged. The allowance-list emptying is a proven no-op at runtime (it matched zero cases before and after), so no case that previously blocked now passes, and no case that previously passed now blocks.
Not applicable: no clinical decision-support behaviour changes. The delivered answer users see is byte-identical before and after this PR;
rejectedCandidateTextis a debug-only field read only by the offline eval harness, and the classifier-fallback/abort-signal relocation is a same-behaviour code move covered by the existing, still-passing test suite (85/85).What this does not do, deliberately
It does not clear the red.
clozapine-monitoring,clozapine-monitoring-paraphraseandnocc-requirementsstill block, and they should: whether their answers are genuinely poor or the predicate is over-firing on a legitimate list-shaped clinical answer cannot be judged from metadata alone. The threshold's own comment is explicit — "Do not add an entry to clear a red run. A block here names the offending case and reason: investigate that case."After this merges, one canary run will record those three answers for the first time. That is the evidence needed to decide between fixing the answers, fixing the predicate, or accepting the degradations with a real, correctly-keyed entry.
🤖 Generated with Claude Code
Note
Low Risk
Additive debug-only fields and eval reporting; allowance emptying is documented as a no-op at runtime. Classifier/abort extraction is a pure move with existing test coverage.
Overview
Improves answer-quality eval observability so failed canaries can be debugged without another live run. The harness now records
answerOpeningSentenceon every case (what text-shape gates inspect) andanswerTextonly when a text-shape gate rejected the answer, preferringRagAnswer.rejectedCandidateText—the prose the gate judged before generic fallbacks replacedanswer.answer.The RAG pipeline gains a debug-only
rejectedCandidateTextfield, set when final quality gates or source-backed review fallbacks discard candidate prose, with no change to delivered answers or routing.Eval threshold hygiene: production
ragSourceBackedReviewFallbackAllowanceis cleared because entries usedquality--prefixed ids that never matched bareragEvalCasesids (runtime no-op, but misleading in source). Tests now pass an explicit allowance override to exercise the mechanism, plus guards that production allowance ids exist in fixtures and reasons are emittable.Refactor:
awaitWithCallerSignalandanalyzeQueryWithClassifierFallbackmove torag-abort-signal.ts/rag-classifier-fallback.tswith re-exports fromrag.ts(behavior unchanged).Reviewed by Cursor Bugbot for commit 4f577f7. Configure here.