From bcf357a96fde74d39fc4726ffabb5079a744ef28 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 07:01:58 +0000 Subject: [PATCH 1/3] Harden the eval-canary safeguards: per-case rr diff, dispatch failure issues, contract pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the RAG canary infrastructure (2026-08-12) found the mechanism sound but with manual/silent gaps. This change mechanizes and pins them; no retrieval behaviour changes. - compare-retrieval-eval: new per-case rr@10/contentRR@10 diff with --fail-on-regression, mechanizing the canary-pair protocol's 'zero per-case rr regressions' gate that was previously eyeballed from tee'd logs; fails closed on summary-only artifacts and non-identical case sets. - eval-canary.yml: open the failure issue on failed dispatches too (a red dispatch is usually half a canary pair), and feed answer-targeting.log, validate_override and answer_targeting outcomes into the issue body. - build-ranking-snapshot: fail loudly when a hardNegativeTemplates caseId matches no artifact case (a renamed golden case previously dropped its hard negatives silently); CLI guarded so tests can import convertArtifact. - New contract tests: hard-negative attachment guard, and a strict/wide alias-tier pin so a bulk merge of src/lib/eval-document-matching.ts into scripts/lib/clinical-aliases.ts can no longer pass CI. - docs/rag-behaviour: fix stale claims that the canary has workflow_dispatch / a ref input (both deliberately absent); document the per-case compare command. - outstanding-issues: capture the snapshot-freshness dispatch handoff (#304) and deferred canary observations (#305). RAG impact: no retrieval behaviour change — canary tooling, docs, and contract-test hardening only; golden fixture and ranking code untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UoHPMZ1FinSYzH7rJ42xTo --- .github/workflows/eval-canary.yml | 12 ++- docs/outstanding-issues.md | 4 +- docs/rag-behaviour/README.md | 6 +- docs/rag-behaviour/safeguards.md | 10 ++- scripts/build-ranking-snapshot.ts | 20 ++++- scripts/compare-retrieval-eval.ts | 107 +++++++++++++++++++++++-- tests/build-ranking-snapshot.test.ts | 60 ++++++++++++++ tests/clinical-aliases-tiering.test.ts | 60 ++++++++++++++ tests/compare-retrieval-eval.test.ts | 47 ++++++++++- tests/eval-canary-workflow.test.ts | 17 +++- 10 files changed, 326 insertions(+), 17 deletions(-) create mode 100644 tests/build-ranking-snapshot.test.ts create mode 100644 tests/clinical-aliases-tiering.test.ts diff --git a/.github/workflows/eval-canary.yml b/.github/workflows/eval-canary.yml index 740df851bb..e8ce8b1b20 100644 --- a/.github/workflows/eval-canary.yml +++ b/.github/workflows/eval-canary.yml @@ -197,8 +197,10 @@ jobs: retention-days: 30 if-no-files-found: ignore + # Dispatch failures matter as much as scheduled ones — a red dispatch is usually one + # half of a canary pair gating a behaviour change, so it must not fail silently. - name: Open or update canary failure issue - if: failure() && github.event_name == 'schedule' + if: failure() && (github.event_name == 'schedule' || github.event_name == 'repository_dispatch') uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | @@ -211,6 +213,7 @@ jobs: const logPaths = [ ".local/eval-canary/golden-retrieval.log", ".local/eval-canary/answer-quality.log", + ".local/eval-canary/answer-targeting.log", ].map((item) => path.join(process.env.GITHUB_WORKSPACE, item)); const failureText = logPaths .filter((item) => fs.existsSync(item)) @@ -222,12 +225,15 @@ jobs: preflight: "${{ steps.preflight.outcome }}", setup_node: "${{ steps.setup_node.outcome }}", install: "${{ steps.install.outcome }}", + validate_override: "${{ steps.validate_override.outcome }}", project_guard: "${{ steps.project_guard.outcome }}", golden_retrieval: "${{ steps.golden_retrieval.outcome }}", answer_quality: "${{ steps.answer_quality.outcome }}", + answer_targeting: "${{ steps.answer_targeting.outcome }}", }; + const trigger = context.eventName === "schedule" ? "Weekly (scheduled)" : "Dispatched"; const body = [ - `Weekly eval canary failed on ${new Date().toISOString()}.`, + `${trigger} eval canary failed on ${new Date().toISOString()}.`, "", `Run: ${runUrl}`, "", @@ -257,7 +263,7 @@ jobs: await github.rest.issues.create({ owner: context.repo.owner, repo: context.repo.repo, - title: "Eval canary failure: weekly evaluation did not complete", + title: "Eval canary failure: evaluation did not complete", labels: [label], body, }); diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 4cadf4ea42..fabef1d34d 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -166,7 +166,7 @@ removed after current-main verification; it is not missing recommended work. | 111 | `#289` | A2 | High — auth/identity | Next auth module touch | 1–2 hours | Export a named helper (e.g. `authorizationIdentity(headers)`) from the auth module and use it at every property-access call site; consider a lint rule or branded type so `.Authorization` stops type-checking at all. **Stop:** do not change `authorizationHeadersForAccessToken` to emit uppercase — lowercase is the correct Fetch/Headers convention and callers that pass the object wholesale to `fetch` depend on it. | - + ## Open items > **Merged-main canary update (2026-07-23, run `30018289898`):** the new structured report correctly recorded evaluated tree `c24f2e8f2d30d0c59fc1eba025d3dcd63478137e`, run/attempt identity and `cross-region-runner` latency context. Golden retrieval remained 36/36 with document/content recall 1.0 and no failed cases. The 44-case answer gate had grounded-supported and unsupported-correct rates of 1.0, but failed because `neuroleptic-side-effect-escalation` again returned one citation where two are required (citation-failure rate 0.0227). `admission-discharge-comparison` again omitted the specific AKG admission document after `comparison_source_extractive_fallback`; `admission-discharge-coverage-paraphrase` was advisory-only at 24,870 ms. Answer cost was reported as `$0.234736`. Do not retry immediately: retain this as the first structured datapoint, compare it with the scheduled 2026-07-26 report, and keep retrieval/ranking unchanged. @@ -329,6 +329,8 @@ removed after current-main verification; it is not missing recommended work. | #301 | P3 | issue | Two sessions built #262 part 3 in parallel because the GATES.md row understated what had shipped | On 2026-08-09 two branches implemented the same raw-value ratchet independently. PR #1780 landed rawPaddingLiterals/rawRadiusLiterals/rawLineHeightLiterals; a concurrent session built arbitraryPadding/arbitraryGap/arbitraryRadius/arbitraryLeading against the same four files and discovered the collision only when syncing before PR. The duplicate was dropped and only the uncovered gap family was rebuilt on #1780's predicate (rawGapLiterals, 34 sites). Root cause is the same failure this document keeps producing: the §3 row read 'Contract ratchet \| implemented-partial (colour/shadow/tap literals only)' and named none of the metrics #1780 had just shipped, so the row still advertised the work as unstarted. Identical to the 2026-08-09 finding that four of #264's six prohibitions were already gated while their rows read 'planned'. Both rows are corrected now. Next action: when a gate lands, update its §3 row IN THE SAME COMMIT - a row that understates shipped work is not a stale doc, it is a duplicate-work generator. Consider asserting in a test that every metric key in design-system-contract-baseline.json appears somewhere in GATES.md. Stop: do not rely on the ledger alone to prevent this - both sessions had ledger access. | session 2026-08-09 M4; PR #1780 collision | 2026-08-09 | | #302 | P3 | issue | `tests/helpers/style-contracts.ts` contains escaped line-break artifacts in the exemption map | `smart-search-phone-ticker*` entries were merged with literal backtick-`r`n escapes, which makes the style-exemptions object invalid for the required parse and blocks local checks. Cleanly split each ticker exemption to one line and keep the same reason text so the exception intent is preserved. | PR #1815 unblock follow-up (`tests/helpers/style-contracts.ts`) | 2026-08-11 | | #303 | P3 | task | `issues:next-id` is out of sync with declared rows | The outstanding-issues marker is `issues:next-id=302` with no `#302`/`#303` rows in either open or resolved tables, which `check:outstanding-issues` flags as missing-issue failures. Add both rows and bump marker to `304` to keep the ledger monotonic. | `docs/outstanding-issues.md` | 2026-08-11 | +| #304 | P2 | task | Canary snapshot refresh blocked in cloud sessions: dispatch needs operator gh identity | The ranking snapshot (scripts/fixtures/rag-ranking-candidate-snapshot.v1.json, generatedAt 2026-07-20, sourceRunId 29763761133) trips the 30-day freshness gate in tests/ranking-tuning.test.ts on ~2026-08-19, going red across the whole unit suite. Refresh requires an eval-canary repository dispatch (approved by the user 2026-08-12), but eval-canary.yml deliberately accepts only repository_dispatch and the cloud GitHub MCP toolset has no repository-dispatch call, so the dispatch must come from an operator identity: gh api repos/bigsimmo/database/dispatches -f event_type=eval-canary. After the run is green, download the eval-canary-output artifact and run npm run build:ranking-snapshot -- --input --output scripts/fixtures/rag-ranking-candidate-snapshot.v1.json --source-run-id , then confirm tests/ranking-tuning.test.ts passes. Provider-backed (~$1-2), already user-approved for this refresh. | session 2026-08-12 RAG canary review | 2026-08-12 | +| #305 | P3 | rec | Canary has no latency-mode coverage and its cost readout is a known lower bound | Two informational gaps from the 2026-08-12 canary review, deferred by scope decision. (1) eval:retrieval:latency (p90 20s gate) is never wired into eval-canary.yml, so live retrieval latency regressions are invisible to the weekly canary while the answer step relaxes its own gates via EVAL_LATENCY_CONTEXT=cross-region-runner. (2) estimated_cost_usd applies one rate set (gpt-5.6-terra) to all usage including 2x-priced strong-model retries, so any cost trend understates strong-retry runs — the workflow comments say so, but eval:trend consumers may not read them. Also noted: the workflow-wide concurrency group (eval-canary, cancel-in-progress false) can queue a dispatched pair run behind a scheduled run, interleaving pair evidence; and fixture coverage gaps tracked in #018 remain uncatchable by the canary. Next: decide whether a monthly latency-mode dispatch is worth the spend; add a strong-usage split to the estimator if cost trends start driving decisions. | session 2026-08-12 RAG canary review | 2026-08-12 | ## Resolved / archive diff --git a/docs/rag-behaviour/README.md b/docs/rag-behaviour/README.md index 7afb3595b4..11be5f4795 100644 --- a/docs/rag-behaviour/README.md +++ b/docs/rag-behaviour/README.md @@ -18,8 +18,10 @@ Standing rules (mirrored in `AGENTS.md` so every agent session inherits them): `scripts/pr-policy.mjs` — the check fails without it). 2. **Canary for behaviour changes.** Any retrieval/ranking/ordering behaviour change requires a live eval-canary pair — baseline + post — with doc/content recall pinned at 1.0 and zero - per-case regressions, before the change is trusted. `workflow_dispatch` on - `eval-canary.yml` (provider-backed → explicit user approval per run). + per-case regressions, before the change is trusted. Runs fire via an `eval-canary` + repository dispatch (`eval-canary.yml` deliberately has no `workflow_dispatch` and no + `ref` input — it always loads from the default branch; provider-backed → explicit user + approval per run). 3. **Offline green is necessary, never sufficient.** The Phase C regression passed 121/121 offline tests and an adversarial code review, then failed 3/36 live within one run. The live corpus is the only authority on ordering behaviour. diff --git a/docs/rag-behaviour/safeguards.md b/docs/rag-behaviour/safeguards.md index 5a94f999a5..cc2f6127b5 100644 --- a/docs/rag-behaviour/safeguards.md +++ b/docs/rag-behaviour/safeguards.md @@ -64,8 +64,14 @@ keys above relevance, read this folder first. **Also:** do not add `review_due` refuted (`refuted-approaches.md` § Refutation 3; ledger `#032`). The eval-canary pair protocol: 1. Baseline: latest green canary on current main (or one dispatch). -2. Change merges (or runs from a branch via the `ref` dispatch input). -3. Post: one dispatch; gates = recall 1.0/1.0, zero per-case rr regressions. +2. Change merges to main. (Post runs are post-merge only: the workflow accepts only the + `eval-canary` repository dispatch and always loads code from the default branch — no + `workflow_dispatch`, no `ref` input; both are asserted absent by + `tests/eval-canary-workflow.test.ts`.) +3. Post: one dispatch; gates = recall 1.0/1.0, zero per-case rr regressions. Compare the + pair's `--json-out` artifacts with `npm run eval:retrieval:compare -- + --fail-on-regression` — it diffs per-case `rr@10`/`contentRR@10` and exits + non-zero on any per-case regression or a non-identical case set. 4. Regression → immediate single-commit revert + one confirmation dispatch. Provider-backed dispatches always need explicit user approval (~$1–2 each). diff --git a/scripts/build-ranking-snapshot.ts b/scripts/build-ranking-snapshot.ts index 10814f60e7..40f825729d 100644 --- a/scripts/build-ranking-snapshot.ts +++ b/scripts/build-ranking-snapshot.ts @@ -1,6 +1,7 @@ import { createHash } from "node:crypto"; import { readFileSync, writeFileSync } from "node:fs"; import { resolve } from "node:path"; +import { pathToFileURL } from "node:url"; import type { RagQueryClass } from "../src/lib/types"; import { candidateFeatures, @@ -78,7 +79,7 @@ const hardNegativeTemplates: Array<{ features: RankingCandidateFeatures; }>; -function convertArtifact(artifact: RetrievalArtifact, sourceRunId?: string): RankingSnapshot { +export function convertArtifact(artifact: RetrievalArtifact, sourceRunId?: string): RankingSnapshot { // Floor, not an exact pin: the golden fixture only ever grows, and a short artifact means a // truncated or filtered eval run that must not silently become the tuner's ground truth. if (!Array.isArray(artifact.results) || artifact.results.length < 36) { @@ -88,6 +89,18 @@ function convertArtifact(artifact: RetrievalArtifact, sourceRunId?: string): Ran })`, ); } + // Templates attach by caseId filter, so a renamed golden case would silently drop its + // hard negatives and erode the below-threshold protection the tuner's floor depends on. + const artifactCaseIds = new Set(artifact.results.map((testCase) => testCase.id)); + const orphanTemplateCaseIds = [ + ...new Set(hardNegativeTemplates.map((item) => item.caseId).filter((caseId) => !artifactCaseIds.has(caseId))), + ]; + if (orphanTemplateCaseIds.length > 0) { + throw new Error( + `Hard-negative template caseId(s) match no artifact case: ${orphanTemplateCaseIds.join(", ")}. ` + + "A renamed or removed golden case must update hardNegativeTemplates in the same change.", + ); + } const cases = artifact.results.map((testCase) => { const expectedDocuments = testCase.expectedDocumentSubstrings ?? []; const expectedContent = testCase.expectedContentTerms ?? []; @@ -156,4 +169,7 @@ function main() { console.log(JSON.stringify({ output: resolve(output), cases: snapshot.cases.length, version: snapshot.version })); } -main(); +// Guarded so unit tests can import convertArtifact without executing the CLI. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main(); +} diff --git a/scripts/compare-retrieval-eval.ts b/scripts/compare-retrieval-eval.ts index 134b94c373..d7506187eb 100644 --- a/scripts/compare-retrieval-eval.ts +++ b/scripts/compare-retrieval-eval.ts @@ -103,6 +103,68 @@ export type RetrievalEvalComparison = { missingRequired: string[]; }; +export type PerCaseRankRegression = { + caseId: string; + metric: "reciprocalRankAt10" | "contentReciprocalRankAt10"; + baseline: number; + candidate: number; +}; + +export type PerCaseRankComparison = { + regressions: PerCaseRankRegression[]; + // Case IDs present in one artifact but not the other. A vanished case is not a "no + // regression" — the pair is not comparable case-for-case, so callers must fail closed. + missingInCandidate: string[]; + missingInBaseline: string[]; + comparedCaseCount: number; +}; + +type PerCaseResult = Record; + +function readCaseRank(result: PerCaseResult, key: string): number | undefined { + const value = result[key]; + return typeof value === "number" && Number.isFinite(value) ? value : undefined; +} + +// The safeguards protocol's "zero per-case rr regressions" gate (docs/rag-behaviour/ +// safeguards.md, canary-pair step 3), mechanized: any per-case drop in doc-level rr@10 or +// content rr@10 between the baseline and post artifacts is a regression, even when the +// case still passes its top-5 recall gate (rank depth can silently erode otherwise). +export function comparePerCaseRanks( + baselineResults: PerCaseResult[], + candidateResults: PerCaseResult[], +): PerCaseRankComparison { + const metrics = ["reciprocalRankAt10", "contentReciprocalRankAt10"] as const; + const candidateById = new Map(); + for (const result of candidateResults) { + if (typeof result.id === "string") candidateById.set(result.id, result); + } + const baselineIds = new Set(); + const regressions: PerCaseRankRegression[] = []; + const missingInCandidate: string[] = []; + let comparedCaseCount = 0; + for (const baselineResult of baselineResults) { + if (typeof baselineResult.id !== "string") continue; + baselineIds.add(baselineResult.id); + const candidateResult = candidateById.get(baselineResult.id); + if (!candidateResult) { + missingInCandidate.push(baselineResult.id); + continue; + } + comparedCaseCount += 1; + for (const metric of metrics) { + const baselineRank = readCaseRank(baselineResult, metric); + const candidateRank = readCaseRank(candidateResult, metric); + if (baselineRank === undefined || candidateRank === undefined) continue; + if (candidateRank < baselineRank) { + regressions.push({ caseId: baselineResult.id, metric, baseline: baselineRank, candidate: candidateRank }); + } + } + } + const missingInBaseline = [...candidateById.keys()].filter((id) => !baselineIds.has(id)); + return { regressions, missingInCandidate, missingInBaseline, comparedCaseCount }; +} + // Pure so it can be unit-tested without touching the filesystem or process exit code. export function compareRetrievalEval(baseline: EvalSummary, candidate: EvalSummary): RetrievalEvalComparison { const rows: ComparisonRow[] = []; @@ -135,14 +197,18 @@ function readPayload(path: string): EvalPayload { } function main() { - const [, , baselinePath, candidatePath] = process.argv; + const positional = process.argv.slice(2).filter((arg) => arg !== "--fail-on-regression"); + const failOnRegression = process.argv.includes("--fail-on-regression"); + const [baselinePath, candidatePath] = positional; if (!baselinePath || !candidatePath) { - throw new Error("Usage: tsx scripts/compare-retrieval-eval.ts "); + throw new Error( + "Usage: tsx scripts/compare-retrieval-eval.ts [--fail-on-regression]", + ); } - const baseline = readPayload(baselinePath).summary ?? {}; - const candidate = readPayload(candidatePath).summary ?? {}; - const { rows, missingRequired } = compareRetrievalEval(baseline, candidate); + const baselinePayload = readPayload(baselinePath); + const candidatePayload = readPayload(candidatePath); + const { rows, missingRequired } = compareRetrievalEval(baselinePayload.summary ?? {}, candidatePayload.summary ?? {}); console.log("Retrieval eval comparison: candidate (delta from baseline)"); for (const row of rows) { @@ -154,6 +220,37 @@ function main() { console.error("A missing decision metric is not the same as 0 — regenerate the eval JSON with the full summary."); process.exitCode = 1; } + + const baselineResults = baselinePayload.results; + const candidateResults = candidatePayload.results; + if (Array.isArray(baselineResults) && Array.isArray(candidateResults)) { + const perCase = comparePerCaseRanks(baselineResults, candidateResults); + console.log(`\nPer-case rank comparison over ${perCase.comparedCaseCount} shared case(s):`); + if (perCase.regressions.length === 0) { + console.log(" zero per-case rr regressions"); + } + for (const regression of perCase.regressions) { + console.log( + ` REGRESSION ${regression.caseId} ${regression.metric}: ${regression.baseline.toFixed(4)} -> ${regression.candidate.toFixed(4)}`, + ); + } + for (const id of perCase.missingInCandidate) console.log(` MISSING in candidate: ${id}`); + for (const id of perCase.missingInBaseline) console.log(` NEW in candidate (no baseline): ${id}`); + const notComparable = perCase.missingInCandidate.length > 0; + if (failOnRegression && (perCase.regressions.length > 0 || notComparable)) { + console.error( + "\nPer-case gate failed: the canary-pair protocol (docs/rag-behaviour/safeguards.md) requires zero per-case rr regressions over an identical case set.", + ); + process.exitCode = 1; + } + } else if (failOnRegression) { + // Summary-only artifacts cannot prove the per-case gate — fail closed rather than + // reporting a green pair without the evidence. + console.error( + "\n--fail-on-regression requires per-case results in both artifacts (re-run the eval with --json-out).", + ); + process.exitCode = 1; + } } if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { diff --git a/tests/build-ranking-snapshot.test.ts b/tests/build-ranking-snapshot.test.ts new file mode 100644 index 0000000000..1cb66bab02 --- /dev/null +++ b/tests/build-ranking-snapshot.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import { convertArtifact } from "../scripts/build-ranking-snapshot"; + +// The 11 golden case IDs that hardNegativeTemplates in build-ranking-snapshot.ts attach to. +const TEMPLATED_CASE_IDS = [ + "agitation-im-po-options", + "opioid-withdrawal-doses", + "clozapine-anc-threshold", + "lithium-therapy-monitoring", + "alcohol-ciwa-threshold", + "clozapine-cbc-abbreviation-threshold", + "flowchart-next-step", + "patient-safety-plan-include", + "active-community-patient-ed", + "admission-discharge-comparison", + "depression-adults-vs-children", +]; + +function artifactCase(id: string) { + return { + id, + query: `query for ${id}`, + expectedDocumentSubstrings: ["doc"], + expectedContentTerms: [], + topResults: [], + }; +} + +function artifactWithCases(ids: string[]) { + return { results: ids.map(artifactCase) }; +} + +function fillerIds(count: number): string[] { + return Array.from({ length: count }, (_, index) => `filler-case-${index + 1}`); +} + +describe("build-ranking-snapshot hard-negative attachment", () => { + it("attaches every hard-negative template when all templated cases are present", () => { + const ids = [...TEMPLATED_CASE_IDS, ...fillerIds(36 - TEMPLATED_CASE_IDS.length)]; + const snapshot = convertArtifact(artifactWithCases(ids)); + const hardNegativeCount = snapshot.cases + .flatMap((snapshotCase) => snapshotCase.candidates) + .filter((candidate) => candidate.hardNegative).length; + expect(hardNegativeCount).toBe(12); + }); + + it("fails loudly when a templated golden case is renamed or removed", () => { + // Templates match caseId by filter; before this guard a rename silently dropped its hard + // negatives, eroding below-threshold protection with no error until snapshot validation. + const withoutClozapineAnc = TEMPLATED_CASE_IDS.filter((id) => id !== "clozapine-anc-threshold"); + const ids = [...withoutClozapineAnc, ...fillerIds(36 - withoutClozapineAnc.length)]; + expect(() => convertArtifact(artifactWithCases(ids))).toThrow( + /Hard-negative template caseId\(s\) match no artifact case: clozapine-anc-threshold/, + ); + }); + + it("still enforces the 36-case floor before template attachment", () => { + expect(() => convertArtifact(artifactWithCases(TEMPLATED_CASE_IDS))).toThrow(/at least 36 cases/); + }); +}); diff --git a/tests/clinical-aliases-tiering.test.ts b/tests/clinical-aliases-tiering.test.ts new file mode 100644 index 0000000000..a3f95dc2e9 --- /dev/null +++ b/tests/clinical-aliases-tiering.test.ts @@ -0,0 +1,60 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; +import { clinicalContentAliases, clinicalDocumentAliases } from "../scripts/lib/clinical-aliases"; + +// Tiering contract for the alias tables (scripts/lib/clinical-aliases.ts header, mirrored in +// docs/rag-behaviour/safeguards.md): the STRICT tier feeds the zero-tolerance golden gates and +// the tuner's relevance grades, while src/lib/eval-document-matching.ts keeps a deliberately +// WIDER table for captured-case coverage reporting. Until this test existed the split was +// enforced only by comment — a bulk merge of the wide tier into the strict one would have +// passed CI while silently loosening clinical ground truth. +// +// Every key added to the strict tier is a clinical-governance change needing its own evidence +// trail. Updating the pinned key lists below is the conscious act that records that decision — +// never sync them from the wide tier. + +const STRICT_DOCUMENT_ALIAS_KEYS = [ + "AgitationArousalPharmaMgt", + "AdmissionCommunityPts", + "ActiveCommunityPtED", + "ClozapinePresAdminMonitor", + "PtSafetyPlan", +]; + +const STRICT_CONTENT_ALIAS_KEYS = [ + "anc", + "ciwa", + "fbc", + "im", + "mg", + "microgram", + "po", + "prn", + "red", + "route", + "threshold", + "withhold", +]; + +describe("clinical alias tiering (strict vs wide)", () => { + it("pins the strict document-alias key set — additions need their own evidence trail", () => { + expect(Object.keys(clinicalDocumentAliases).sort()).toEqual([...STRICT_DOCUMENT_ALIAS_KEYS].sort()); + }); + + it("pins the strict content-alias key set — additions need their own evidence trail", () => { + expect(Object.keys(clinicalContentAliases).sort()).toEqual([...STRICT_CONTENT_ALIAS_KEYS].sort()); + }); + + it("keeps wide-tier-only coverage keys out of the strict tier", () => { + // "Clozapine GP Shared Care" is the wide tier's marker example (named in the strict file's + // TIERING comment): topically adjacent, correct for coverage reporting, wrong as golden + // ground truth. Its appearance here is the signature of a bulk merge. + expect(Object.keys(clinicalDocumentAliases)).not.toContain("Clozapine GP Shared Care"); + }); + + it("retains the TIERING contract comment in the strict module", () => { + const source = readFileSync(new URL("../scripts/lib/clinical-aliases.ts", import.meta.url), "utf8"); + expect(source).toContain("do not merge with src/lib/eval-document-matching.ts"); + expect(source).toContain("not a bulk merge"); + }); +}); diff --git a/tests/compare-retrieval-eval.test.ts b/tests/compare-retrieval-eval.test.ts index 4daf9d7fa6..1aa74137c4 100644 --- a/tests/compare-retrieval-eval.test.ts +++ b/tests/compare-retrieval-eval.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { compareRetrievalEval } from "../scripts/compare-retrieval-eval"; +import { comparePerCaseRanks, compareRetrievalEval } from "../scripts/compare-retrieval-eval"; // A complete retrieval eval summary as emitted by summarizeGoldenRetrievalResults, trimmed to the // fields the comparison reads. @@ -75,3 +75,48 @@ describe("compareRetrievalEval", () => { expect(row(comparison, "index_units_layer_count").baseline.present).toBe(false); }); }); + +// Per-case results as written by eval-retrieval's --json-out (trimmed to the compared fields). +function perCase(id: string, rr: number, contentRR: number): Record { + return { id, reciprocalRankAt10: rr, contentReciprocalRankAt10: contentRR }; +} + +describe("comparePerCaseRanks", () => { + it("reports zero regressions for an identical pair", () => { + const results = [perCase("a", 1, 1), perCase("b", 0.33, 0.5)]; + const comparison = comparePerCaseRanks(results, results); + expect(comparison.regressions).toEqual([]); + expect(comparison.comparedCaseCount).toBe(2); + expect(comparison.missingInCandidate).toEqual([]); + expect(comparison.missingInBaseline).toEqual([]); + }); + + it("flags any per-case rr drop even when the case would still pass its top-5 gate", () => { + const comparison = comparePerCaseRanks([perCase("patient-property", 1, 1)], [perCase("patient-property", 0.11, 1)]); + expect(comparison.regressions).toEqual([ + { caseId: "patient-property", metric: "reciprocalRankAt10", baseline: 1, candidate: 0.11 }, + ]); + }); + + it("tracks doc-level and content-level rank drops independently", () => { + const comparison = comparePerCaseRanks([perCase("a", 0.5, 1)], [perCase("a", 0.5, 0.33)]); + expect(comparison.regressions).toEqual([ + { caseId: "a", metric: "contentReciprocalRankAt10", baseline: 1, candidate: 0.33 }, + ]); + }); + + it("does not count an improvement as a regression", () => { + const comparison = comparePerCaseRanks([perCase("a", 0.33, 0.5)], [perCase("a", 1, 1)]); + expect(comparison.regressions).toEqual([]); + }); + + it("surfaces non-identical case sets so callers can fail closed", () => { + const comparison = comparePerCaseRanks( + [perCase("kept", 1, 1), perCase("dropped", 1, 1)], + [perCase("kept", 1, 1), perCase("added", 1, 1)], + ); + expect(comparison.missingInCandidate).toEqual(["dropped"]); + expect(comparison.missingInBaseline).toEqual(["added"]); + expect(comparison.comparedCaseCount).toBe(1); + }); +}); diff --git a/tests/eval-canary-workflow.test.ts b/tests/eval-canary-workflow.test.ts index 648a9afa87..9e797d08c1 100644 --- a/tests/eval-canary-workflow.test.ts +++ b/tests/eval-canary-workflow.test.ts @@ -39,8 +39,23 @@ describe("eval canary workflow input", () => { expect(workflow.slice(installStart, projectGuardStart)).not.toContain("OPENAI_API_KEY"); }); + it("opens the failure issue for scheduled AND dispatched runs", () => { + // A red dispatch is usually half of a canary pair gating a behaviour change — it must + // not fail silently while only weekly runs get an issue. + expect(workflow).toContain( + "if: failure() && (github.event_name == 'schedule' || github.event_name == 'repository_dispatch')", + ); + expect(workflow).toContain('context.eventName === "schedule" ? "Weekly (scheduled)" : "Dispatched"'); + }); + + it("feeds every eval log and live step outcome into the failure issue", () => { + expect(workflow).toContain('".local/eval-canary/answer-targeting.log"'); + expect(workflow).toContain('validate_override: "${{ steps.validate_override.outcome }}"'); + expect(workflow).toContain('answer_targeting: "${{ steps.answer_targeting.outcome }}"'); + }); + it("distinguishes provider outages from retrieval regressions in the failure issue", () => { - expect(workflow).toContain('title: "Eval canary failure: weekly evaluation did not complete"'); + expect(workflow).toContain('title: "Eval canary failure: evaluation did not complete"'); expect(workflow).toContain("Resolve provider quota/auth/config failures before rerunning"); expect(workflow).toContain( "Do not bisect or revert code until provider health and the failure class are confirmed", From a462ab52e286d38fac573281d39349dcdade2f98 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 07:03:55 +0000 Subject: [PATCH 2/3] Record eval-canary review in the branch review ledger Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UoHPMZ1FinSYzH7rJ42xTo --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index b087c87a17..88d788916e 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -897,3 +897,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-11 | claude/spacing-icon-design-review-rxwh28 | 5b96281ee7da817d5ce7f1102004ebe6f861b920 | pr-1815 heavy review-and-fix | remote already merged main (shadow-tight Switch kept); cherry-picked privacy -mb-4 reclaim + calculators dock cancel; removed duplicate UniversalSearchAlsoMatches; rail-aware section-sheet focus restore; dispositioned CodeRabbit docs/ledger/gates nits and outdated Sentry skeleton gap | verify:cheap PASS prior tip; verify:pr-local PASS prior tip; vitest privacy+in-page-nav 28 passed on cherry-pick; merge-tree clean vs origin/main | | 2026-08-12 | PR #1815 / claude/spacing-icon-design-review-rxwh28 | 9f266210f02081be54d407c70a85f52fed436128 | babysit | no remaining actionable findings; one pre-existing thread resolved as no-change (Dockerfile.worker follow-up needed) | required checks: Gitleaks PR policy PR required (all pass); targeted vitest passed: tests/document-frame-contract.test.ts + tests/in-page-nav-header.dom.test.tsx | | 2026-08-12 | 1815 | 27ce96e1755055ceee2eeae02d6efdf11259fcde | babysit | fixed | Unit coverage: targeted vitest passed: tests/shared-home-empty-state.dom.test.tsx (17 passed). PR required still blocked on pre-existing check failure at old remote head before sync. | +| 2026-08-12 | claude/rag-canary-test-review-seprbt | bcf357a96fde74d39fc4726ffabb5079a744ef28 | eval-canary review: workflow, compare tooling, snapshot builder, alias tiering, rag-behaviour docs | PR #1843 opened; no retrieval behaviour change; snapshot refresh handed off as /issues #304 | verify:pr-local (green except env-only #296), eval:rag:offline 574/574, focused suites 40/40 | From ea357ca78f42d09bc37350c8840234aad3cc231e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 10:33:46 +0000 Subject: [PATCH 3/3] Fail closed on any non-comparable canary pair; guard the failure-issue analyzer import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for PR #1843 (sentry, codex, coderabbit, copilot all converged on the first item): - compare-retrieval-eval --fail-on-regression now fails on candidate-only cases (missingInBaseline) and on absent/non-finite per-case rank metrics, not just baseline-only cases — a superset candidate could previously pass the identical-case-set gate. CLI-level exit-code tests added. - eval-canary.yml failure-issue step falls back to a fixed diagnosis when the analyzer import fails (e.g. checkout failed), so a red dispatch still opens an issue. - safeguards.md: canary compare command moved to a fenced block so it copy/pastes intact. RAG impact: no retrieval behaviour change — eval tooling and workflow only. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UoHPMZ1FinSYzH7rJ42xTo --- .github/workflows/eval-canary.yml | 13 ++++- docs/rag-behaviour/safeguards.md | 11 ++-- scripts/compare-retrieval-eval.ts | 29 +++++++--- tests/compare-retrieval-eval.test.ts | 81 +++++++++++++++++++++++++++- 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/.github/workflows/eval-canary.yml b/.github/workflows/eval-canary.yml index e8ce8b1b20..50fef0e6c4 100644 --- a/.github/workflows/eval-canary.yml +++ b/.github/workflows/eval-canary.yml @@ -207,7 +207,18 @@ jobs: const fs = require("node:fs"); const path = require("node:path"); const { pathToFileURL } = require("node:url"); - const { analyzeFailureText } = await import(pathToFileURL(`${process.env.GITHUB_WORKSPACE}/scripts/productivity-core.mjs`).href); + // If checkout failed, the analyzer module is absent; the issue must still open, + // so fall back to a fixed diagnosis instead of letting the import throw. + let analyzeFailureText = () => ({ + category: "unclassified", + confidence: "none", + reason: "failure analyzer unavailable (checkout likely failed before scripts/ existed); inspect step outcomes", + }); + try { + ({ analyzeFailureText } = await import(pathToFileURL(`${process.env.GITHUB_WORKSPACE}/scripts/productivity-core.mjs`).href)); + } catch (error) { + core.warning(`analyzeFailureText unavailable, using fallback diagnosis: ${error.message}`); + } const label = "eval-canary"; const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; const logPaths = [ diff --git a/docs/rag-behaviour/safeguards.md b/docs/rag-behaviour/safeguards.md index cc2f6127b5..af209c44d7 100644 --- a/docs/rag-behaviour/safeguards.md +++ b/docs/rag-behaviour/safeguards.md @@ -69,9 +69,14 @@ refuted (`refuted-approaches.md` § Refutation 3; ledger `#032`). The eval-canar `workflow_dispatch`, no `ref` input; both are asserted absent by `tests/eval-canary-workflow.test.ts`.) 3. Post: one dispatch; gates = recall 1.0/1.0, zero per-case rr regressions. Compare the - pair's `--json-out` artifacts with `npm run eval:retrieval:compare -- - --fail-on-regression` — it diffs per-case `rr@10`/`contentRR@10` and exits - non-zero on any per-case regression or a non-identical case set. + pair's `--json-out` artifacts — the command diffs per-case `rr@10`/`contentRR@10` and + exits non-zero on any per-case regression, a non-identical case set, or an unavailable + rank metric: + + ```bash + npm run eval:retrieval:compare -- --fail-on-regression + ``` + 4. Regression → immediate single-commit revert + one confirmation dispatch. Provider-backed dispatches always need explicit user approval (~$1–2 each). diff --git a/scripts/compare-retrieval-eval.ts b/scripts/compare-retrieval-eval.ts index d7506187eb..3d1dced625 100644 --- a/scripts/compare-retrieval-eval.ts +++ b/scripts/compare-retrieval-eval.ts @@ -116,9 +116,15 @@ export type PerCaseRankComparison = { // regression" — the pair is not comparable case-for-case, so callers must fail closed. missingInCandidate: string[]; missingInBaseline: string[]; + // Shared cases where a rank metric is absent or non-finite on either side. Skipping the + // metric would report "zero regressions" without evidence for it, so these also make the + // pair non-comparable. + unavailableMetrics: Array<{ caseId: string; metric: (typeof PER_CASE_RANK_METRICS)[number] }>; comparedCaseCount: number; }; +const PER_CASE_RANK_METRICS = ["reciprocalRankAt10", "contentReciprocalRankAt10"] as const; + type PerCaseResult = Record; function readCaseRank(result: PerCaseResult, key: string): number | undefined { @@ -134,7 +140,6 @@ export function comparePerCaseRanks( baselineResults: PerCaseResult[], candidateResults: PerCaseResult[], ): PerCaseRankComparison { - const metrics = ["reciprocalRankAt10", "contentReciprocalRankAt10"] as const; const candidateById = new Map(); for (const result of candidateResults) { if (typeof result.id === "string") candidateById.set(result.id, result); @@ -142,6 +147,7 @@ export function comparePerCaseRanks( const baselineIds = new Set(); const regressions: PerCaseRankRegression[] = []; const missingInCandidate: string[] = []; + const unavailableMetrics: PerCaseRankComparison["unavailableMetrics"] = []; let comparedCaseCount = 0; for (const baselineResult of baselineResults) { if (typeof baselineResult.id !== "string") continue; @@ -152,17 +158,20 @@ export function comparePerCaseRanks( continue; } comparedCaseCount += 1; - for (const metric of metrics) { + for (const metric of PER_CASE_RANK_METRICS) { const baselineRank = readCaseRank(baselineResult, metric); const candidateRank = readCaseRank(candidateResult, metric); - if (baselineRank === undefined || candidateRank === undefined) continue; + if (baselineRank === undefined || candidateRank === undefined) { + unavailableMetrics.push({ caseId: baselineResult.id, metric }); + continue; + } if (candidateRank < baselineRank) { regressions.push({ caseId: baselineResult.id, metric, baseline: baselineRank, candidate: candidateRank }); } } } const missingInBaseline = [...candidateById.keys()].filter((id) => !baselineIds.has(id)); - return { regressions, missingInCandidate, missingInBaseline, comparedCaseCount }; + return { regressions, missingInCandidate, missingInBaseline, unavailableMetrics, comparedCaseCount }; } // Pure so it can be unit-tested without touching the filesystem or process exit code. @@ -236,10 +245,18 @@ function main() { } for (const id of perCase.missingInCandidate) console.log(` MISSING in candidate: ${id}`); for (const id of perCase.missingInBaseline) console.log(` NEW in candidate (no baseline): ${id}`); - const notComparable = perCase.missingInCandidate.length > 0; + for (const entry of perCase.unavailableMetrics) { + console.log(` METRIC UNAVAILABLE ${entry.caseId} ${entry.metric}: absent or non-finite on one side`); + } + // Any difference in the case sets — either direction — or an unavailable metric means the + // pair cannot prove the identical-case-set gate; fail closed rather than report a pass. + const notComparable = + perCase.missingInCandidate.length > 0 || + perCase.missingInBaseline.length > 0 || + perCase.unavailableMetrics.length > 0; if (failOnRegression && (perCase.regressions.length > 0 || notComparable)) { console.error( - "\nPer-case gate failed: the canary-pair protocol (docs/rag-behaviour/safeguards.md) requires zero per-case rr regressions over an identical case set.", + "\nPer-case gate failed: the canary-pair protocol (docs/rag-behaviour/safeguards.md) requires zero per-case rr regressions over an identical case set with both rank metrics present.", ); process.exitCode = 1; } diff --git a/tests/compare-retrieval-eval.test.ts b/tests/compare-retrieval-eval.test.ts index 1aa74137c4..49e9de5706 100644 --- a/tests/compare-retrieval-eval.test.ts +++ b/tests/compare-retrieval-eval.test.ts @@ -1,4 +1,8 @@ -import { describe, expect, it } from "vitest"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, describe, expect, it } from "vitest"; import { comparePerCaseRanks, compareRetrievalEval } from "../scripts/compare-retrieval-eval"; // A complete retrieval eval summary as emitted by summarizeGoldenRetrievalResults, trimmed to the @@ -119,4 +123,79 @@ describe("comparePerCaseRanks", () => { expect(comparison.missingInBaseline).toEqual(["added"]); expect(comparison.comparedCaseCount).toBe(1); }); + + it("surfaces candidate-only cases even when every shared case is clean", () => { + // Review finding (PR #1843): candidate-only cases must not slip past the identical- + // case-set gate just because nothing regressed among the shared cases. + const comparison = comparePerCaseRanks( + [perCase("shared", 1, 1)], + [perCase("shared", 1, 1), perCase("extra", 1, 1)], + ); + expect(comparison.regressions).toEqual([]); + expect(comparison.missingInBaseline).toEqual(["extra"]); + }); + + it("records an absent or non-finite rank metric as unavailable instead of skipping it", () => { + const baseline = [perCase("a", 1, 1)]; + const candidate = [{ id: "a", reciprocalRankAt10: Number.NaN } as Record]; + const comparison = comparePerCaseRanks(baseline, candidate); + expect(comparison.regressions).toEqual([]); + expect(comparison.unavailableMetrics).toEqual([ + { caseId: "a", metric: "reciprocalRankAt10" }, + { caseId: "a", metric: "contentReciprocalRankAt10" }, + ]); + }); + + it("reports no unavailable metrics for a complete pair", () => { + const results = [perCase("a", 1, 1)]; + expect(comparePerCaseRanks(results, results).unavailableMetrics).toEqual([]); + }); +}); + +// CLI-level exit-code contract for --fail-on-regression: the pure-function tests above prove +// the classification, these prove main() actually turns each non-comparable shape into a +// non-zero exit (review finding on PR #1843 — the candidate-only shape previously exited 0). +describe("compare-retrieval-eval CLI --fail-on-regression", () => { + const dir = mkdtempSync(join(tmpdir(), "compare-eval-")); + afterAll(() => rmSync(dir, { recursive: true, force: true })); + + const summary = { + case_count: 1, + document_recall_at_5: 1, + content_recall_at_5: 1, + top_k_hit_rate: 1, + content_mrr_at_10: 1, + content_mrr_case_count: 1, + failed_cases: [], + }; + + function writeArtifact(name: string, results: Array>): string { + const filePath = join(dir, name); + writeFileSync(filePath, JSON.stringify({ summary, results })); + return filePath; + } + + function runCli(baseline: string, candidate: string) { + return spawnSync( + process.execPath, + ["scripts/run-tsx.mjs", "scripts/compare-retrieval-eval.ts", baseline, candidate, "--fail-on-regression"], + { cwd: join(__dirname, ".."), encoding: "utf8" }, + ); + } + + it("exits 0 for an identical clean pair", () => { + const baseline = writeArtifact("clean-base.json", [perCase("a", 1, 1)]); + const candidate = writeArtifact("clean-cand.json", [perCase("a", 1, 1)]); + const out = runCli(baseline, candidate); + expect(out.stdout).toContain("zero per-case rr regressions"); + expect(out.status).toBe(0); + }); + + it("exits non-zero when the candidate carries a case the baseline lacks", () => { + const baseline = writeArtifact("subset-base.json", [perCase("a", 1, 1)]); + const candidate = writeArtifact("superset-cand.json", [perCase("a", 1, 1), perCase("extra", 1, 1)]); + const out = runCli(baseline, candidate); + expect(out.stdout).toContain("NEW in candidate (no baseline): extra"); + expect(out.status).toBe(1); + }); });