[pipeline-dcdad92d][slice-11/12] Full eval suite (four datasets... - #206
Conversation
🦋 Changeset detectedLatest commit: cef4470 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. [Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/Khan/actions/new/egg/pipeline-dcdad92d/slice-11?filename=.changeset/bright-beers-leave.md&value=---%0A%22review%22%3A%20patch%0A---%0A%0A%5Bpipeline-dcdad92d%5D%5Bslice-11%2F12%5D%20Full%20eval%20suite%20(four%20datasets...%0A) |
0c30b5c to
345d59c
Compare
ee3bc58 to
d7427e8
Compare
345d59c to
44ff953
Compare
d7427e8 to
41c87b4
Compare
…ing (slice-1) R1 (task-1-2): standardize Step 6 review submission on one robust submit-pull-request-review call with a real, non-empty body; remove the empty-body branch and any fallback/retry path. E2 (task-1-3): stage /tmp/gh-aw/review/pr-context.json in Step 1 and wire all six sub-agent read blocks to consume it. Extends #194 diff staging; foundation for slice-7 lenses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
task-1-1: workflows/review/lib/finding-schema.ts - FINDING_SCHEMA_VERSION exported constant; each finding carries schema_version and the validator rejects an unrecognized version (breaking-shape guard). - Finding shape (snake_case wire keys, matching #194 per-run JSON artifacts): id, lens, anchor, severity, confidence, evidence_trace, optional suggested_patch, optional pre_merge_obligation, producing_hunt, model_authored_prose. - Anchor is a discriminated union with the required PR-level type (pr | file | line, line supports side + multi-line start_line..line range). - KNOWN_LENSES (11 specialist + always-on/triage) and SEVERITIES exported for reuse by the slice-3 router and slice-2 verdict; lens/severity validated against them. - validateFinding collects ALL violations (diagnosable per-lens drop rate), plus isValidFinding narrowing wrapper and throwing assertFinding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
44ff953 to
baef924
Compare
41c87b4 to
21b1988
Compare
…nts) - Restore the empty-review-body convention: with inline comments present the verdict is submitted with an empty body; a non-empty body exists only for comment-less reviews and skipped-dimension notes. Keeps the single-call submission rule. - Deliver workflows/review/lib/ to consumer repos: gh-aw's source: import only copies the .md, so the job checks out Khan/actions at the pinned review-v* release (single version surface for prompt + code) and installs its runtime deps. - Drop planning identifiers (E2, R8, slice numbers, operator-directive note) from shipped file comments.
baef924 to
8ccb4db
Compare
21b1988 to
0a45c34
Compare
jeresig
left a comment
There was a problem hiding this comment.
Very interesting... I've been reading through the evals and it's unclear to me what we're actually live-evaling. I don't see us checking out any repos, nor running the review.md, as far as I can tell - so what is the judge actually judging? The deterministic tests make sense to me, though!
| @@ -1,7 +1,7 @@ | |||
| # Causal experiment — webapp PR #40536 (OpenAccess authorization) | |||
There was a problem hiding this comment.
I mentioned this in the other PR, but I don't think we need/want this file?
| workflow_dispatch: {} | ||
|
|
||
| jobs: | ||
| full-suite: |
There was a problem hiding this comment.
IMO, we should split this into two workflows so that we always run the deterministic portion on every PR - but also I don't even know that we need this, these tests should be running as part of our normal test suite runner - right?
| @@ -0,0 +1,150 @@ | |||
| # Review full eval suite, scheduled (R5, task-11-6) | |||
| # | |||
| # STAGED FILE, NOT YET ACTIVE. It lives under `.github-staging/` because no | |||
| name: Review Full Eval Suite | ||
|
|
||
| on: | ||
| schedule: |
There was a problem hiding this comment.
I'm not sure about this running weekly... I guess it could be ok, but we should find a way to report the results somewhere so that we can see them.
| - uses: ./actions/shared-node-cache | ||
| - name: Run judgeCorpus with the live pinned model | ||
| env: | ||
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} |
There was a problem hiding this comment.
I've generated and added an API key to this repo's secrets.
| echo "ANTHROPIC_API_KEY secret not configured; skipping the live-judge run." >&2 | ||
| exit 0 | ||
| fi | ||
| cat > ./eval-full-live-judge.mts <<'SCRIPT' |
There was a problem hiding this comment.
I'm not sure why we're generating this file dynamically, we should just make it a real script instead (that way normal linting/testing/typechecking can happen).
… rendering (slice-2) task-2-1/2-2 verdict.ts: pure computeVerdict(labels + dimension availability + policy conflicts) -> APPROVE | REQUEST_CHANGES | HOLD_FOR_HUMAN. Consumes #194's mechanical label rule (isBlockingLabel); does not re-implement it. R2 gate: a missing core dimension (correctness/skill-severity) or a policy-named conflict holds for a human; a lost pattern-triage is note-and-continue. Documented default blocking threshold = 1 (tunable, not a HITL gate). task-2-3 render-comment.ts: code-owned Conventional-Comment label taxonomy + deterministic label-wrapping/templating from the finding schema; models own all prose. Review-body rendering mirrors review.md Step 6 + skipped-dimension notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pshot tests (task-2-4) Truth-table tests for computeVerdict (verdict.test.ts): APPROVE / REQUEST_CHANGES / HOLD_FOR_HUMAN precedence, the R2 core-dimension gate (correctness + skill-severity absence holds; hold dominates a blocking label with all reasons retained), pattern-triage note-and-continue, policy-named conflicts (verbatim detail passthrough), blocking-threshold clamping (>=1) + never-RC-on-zero, and purity (deterministic, non-mutating). Rendering snapshot tests (render-comment.test.ts): isBlockingLabel over the full label taxonomy + unknown-label safe default; labelForFinding severity x lens matrix (conventions => best-practice, specialists => plain); renderComment inline snapshots (label wrap + verbatim prose + optional suggestion block) with verbatim prose/patch passthrough asserted; renderReviewBody inline snapshots for every verdict x inline branch + skipped-dimension notes + non-empty safe-output guard. Fixtures validated through assertFinding so tests can't pass on schema-invalid findings. 50 tests green; full pnpm test suite 253/253; tsc --noEmit clean (node24). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(task-10-3)
Add a task-10-3 verification block to smoke.test.ts that pins the two
properties the wave-2 recall/precision rebalance (slice-10, review.md
edits 8-13) must not regress, framed in terms of the rebalance's own
mechanism rather than the generic slice-9 gate:
- No recall regression: every must-catch finding is blocking AND
>= medium confidence, so edit-13's posting bar and edit-10's
downgrade/refuter pass (which act only on non-blocking low-confidence
advisories) provably cannot drop it. Still asserts each is posted by
the deterministic path today.
- No new false-block: clean cases carry no blocking finding for the
rebalance to surface; verdict stays APPROVE with nothing blocking.
- The downgrade/collapse path is non-vacuously exercised: a
low-confidence advisory exists and is never a must-catch.
The rebalance is prompt-only and does not touch the deterministic review
path this runner replays, so the smoke set stays green by construction;
this block proves why.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r panel, R6 experiment E8 (coverage first), E11 (confirm before you claim), E12 (cite exact lines or quote) added to the shared R9 producer protocol (Step 3). E9 (blocking requires a concrete failing scenario) added to Step 4 blocking-label criteria. E10 (drop only the refuted; downgrade the uncertain) rewrites the claim-validator keep/drop rule. Blocking-claim refuter panel (task-10-2): new Phase 3b + `claim-refuter` agent (batched/parallel, xhigh per R12), wired to the slice-1 finding confidence field and the slice-2 computed verdict — a refuted blocking claim is downgraded, not deleted, so recomputing computeVerdict over post-refuter labels is the wiring. E13 posting bar in Step 5: ranked posting, inline >= medium confidence, low-confidence in one collapsed <details> section, suggested diffs where clear, no padding. R6 causal experiment (task-10-4): workflows/review/eval/experiments/webapp-40536.md — edits 8+10 via the no-post harness (no consumer write); OpenAccess authorization surfacing recorded against the incident-auth-bypass smoke stand-in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8ccb4db to
38f3e75
Compare
b4852d4 to
f2efbd9
Compare
…ator-confirmed claims An audit of all 319 blocking claims the bot posted on Khan/frontend since the validator shipped (90 PRs, 2026-06-30 to 2026-07-07) found 12 false blocks, of which only 2 were in the refuter panel's addressable class; the panel is unearned machinery (PR #205 discussion). It is removed rather than shipped, recoverable from this history if the eval suite's false-block metric regresses. In its place: - Claim validation is three-state (confirmed / plausible / refuted) with symmetric evidence duties: confirm by citing the failing line, refute by citing the guard. Only a confirmed claim may carry a blocking label into the computed verdict; plausible downgrades to non-blocking. - Author-disputed claims get a usage-depth bar: no re-block unless the re-check traces to actual usage (the frontend #13214 failure shape); otherwise they post as questions. - The no-post runner replays the gate's apply rules deterministically (stage 3b) and the smoke corpus gains six audit-seeded cases: five production false blocks that must approve, one confirmed block that must stay blocking.
…e, gates, version stamp) task-11-1: four datasets loading via the shared loader — golden (human-comment + revert/follow-up ground truth), synthetic-mutations mapped to lenses, clean, and a non-smoke incident repro; holdout/fresh/adversarial tags for the gates. task-11-2: metrics.ts — five metrics (must-catch recall, golden precision, clean false-block, noise, calibration/ECE) computed over a corpus run, pure. task-11-3: judge.ts — Opus-4.8 LLM-judge with a pure request/aggregate core and an injected model seam; deterministic human-audit sample; thumbs (slice-8) calibration pass. task-11-4: gates.ts — golden holdout split + fresh mutations (overfitting report) and the adversarial set as a hard gate for automatic mode. task-11-5: version-stamp.ts — SHA-256 of prompt+config+schema rendered into the existing #194 pr-reviewer HTML marker; the single drift-guard surface (parse + hasDrifted for a consumer sync check). run-types.ts holds the shared EvalRun shape the three eval modules consume. No consumer-repo writes; no benchmark-gated construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…den aggregate join Addresses reviewer_code_holistic v1 NACK (dangling cross-slice build dependency): judge.ts imported `DownvoteReason` from ../lib/thumbs-sweep, but slice-8 (the thumbs sweep) is not in slice-11's ancestry, so that module does not exist on this branch. The judge consumes thumbs labels as data and never needs the sweep module at build time, so the import was gratuitous coupling. Declared a local `DownvoteReason` union (structurally identical to slice-8's) so judge.ts is self-contained and typechecks against the branch as it stands. Also cleared the two non-blocking advisories while re-proposing: - aggregate() now rejects duplicate finding ids across requests (the join is by bare finding id, only case-unique in the schema — a collision would mis-join), and throws on a score whose finding id matches no request. The docstring now matches the code (previously claimed an unknown-id throw that did not exist). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sk-11-6) Adds workflows/review/eval/suite.test.ts, the task-11-6 deliverable: - Suite self-tests over the coder's slice-11 modules (loader four-dataset load, five metrics, adversarial/overfitting gates, LLM-judge with a STUB model, and the version-stamp drift surface), exercised against the real corpus plus focused synthetic fixtures built through the public parseCase/runCase path. - Whole-suite green-on-baseline check: every corpus case meets its expected block; must-catch recall = 100%, clean false-block = 0, adversarial hard gate passes (automatic mode allowed). - CI-wiring guard: the smoke subset is a strict subset (the fast per-PR pnpm-test gate), and the ONLY model seam is judgeCorpus (injected model, async, off the deterministic path) -- so the live-judge full suite is scheduled/opt-in, never per-PR. Metrics+gates run over the full corpus with zero model calls. Deterministic: no model, no network, no clock, no randomness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1-6, resolves gap-1)
f2efbd9 to
983461b
Compare
… is a committed script on a weekly workflow, version-stamp machinery dropped
983461b to
af7ad3a
Compare
…/slice-11 # Conflicts: # workflows/review/eval/corpus/smoke/adversarial-injection-approve.json # workflows/review/eval/smoke.test.ts # workflows/review/lib/lenses.test.ts # workflows/review/review.md
Build the eval suite that measures what this run shipped: four datasets, five metrics, an Opus-4.8 LLM-judge with human audit, overfitting guards, an adversarial hard gate, and the reviewer version stamp that doubles as the single drift-guard surface.
Base PR: #196
What's in this PR
Commits (4):
This slice
Full eval suite (four datasets, five metrics, judge, version stamp)
Files affected:
workflows/review/eval/corpus/workflows/review/eval/metrics.tsworkflows/review/eval/judge.tsworkflows/review/eval/gates.tsworkflows/review/lib/version-stamp.tsworkflows/review/eval/suite.test.tsTasks (6) + acceptance criteria
Stack
pipeline-dcdad92degg/pipeline-dcdad92d/slice-10