Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review https://arxiv.org/abs/2606.27226. In particular, "Each verdict does come with a natural-language explanation alongside the binary judgment, which supports interpretability, but that's a rationale for the yes/no call, not an escape hatch to abstain." Update the format to support a short single sentence to explain the rationale. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 230ad4d. Eval responses now support a short single-sentence rationale alongside the binary YES/NO verdict, and the parser, JSONL output, step summary, docs, and regression tests were updated accordingly. |
|
@copilot apply string escaping to rationale |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot rational should be passed through the string sanitizer |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: feature · Risk: low · Priority: medium · Score: 43/100 (impact 18, urgency 12, quality 13) Recommended action: Normalizes eval outputs to binary YES/NO to reduce ambiguity in daily eval reporting. Draft, 6 comments already in discussion, no CI yet — defer pending review resolution and undraft.
|
PR Triage
Draft; normalizes eval outputs to binary YES/NO, needs undraft + CI.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
There was a problem hiding this comment.
Pull request overview
Normalizes BinEval results to binary YES/NO while adding rationale capture and rendering.
Changes:
- Converts missing, malformed, and
UNKNOWNanswers toNO. - Adds sanitized rationale parsing and summary display.
- Updates tests and documentation for the binary contract.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/run_evals.cjs |
Implements binary normalization and rationale parsing. |
actions/setup/js/run_evals.test.cjs |
Tests parsing, normalization, and rationales. |
actions/setup/js/render_evals_summary.cjs |
Renders binary tallies and rationales. |
actions/setup/js/render_evals_summary.test.cjs |
Tests updated summary output. |
.github/aw/evals.md |
Documents binary results and rationale records. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| function normalizeRationale(rationale) { | ||
| return sanitizeContent( | ||
| String(rationale) | ||
| .replace(/[\r\n]+/g, " ") | ||
| .replace(/\s+/g, " ") | ||
| .trim() | ||
| ).trim(); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The normalization logic is correct and well-covered by tests.
normalizeEvalAnswercleanly handles empty, UNKNOWN, and mixed-case inputs.- The positional fallback now triggers on
!parsed.answer(empty string) rather than=== "UNKNOWN". This is intentional: an explicit UNKNOWN from the LLM now normalizes to NO vianormalizeEvalAnswerinstead of falling back to positional lookup — consistent with the stated goal. - The
searchContentorder swap (extracted text first) correctly prioritizes structured assistant output for ID-based matching. - Rationale sanitization via
sanitizeContentprevents injection through rationale fields.
LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.8 AIC · ⌖ 9.2 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 93/100 — Excellent
📊 Metrics (17 tests)
✨ Quality HighlightsDesign Invariants Protected
Coverage Excellence
Test Inflation: EXCELLENT
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two non-blocking observations.
📋 Key Themes & Highlights
Key Themes
- Missing pure-function unit tests:
normalizeEvalAnswerandnormalizeRationaleare exported and now form the normalization contract, but they are only exercised throughparseMainintegration tests. Cheap isolated unit tests would protect the contract directly. - Rationale normalization divergence:
render_evals_summary.cjsdoes a bare.trim()on rationale when reading stored JSONL, whereasrun_evals.cjsuses the fullnormalizeRationale(includingsanitizeContent). These two paths should share the same function.
Positive Highlights
- ✅ Clean
normalizeEvalAnswerfunction — single responsibility, easy to reason about - ✅
extractedText + " " + logContentordering fix is a correct and well-reasoned improvement for the JSONL log search - ✅ Judge prompt update is coherent: removing UNKNOWN from the format spec and adding rationale guidance in one atomic change
- ✅ Good test coverage on the integration paths and edge cases (explicit UNKNOWN, missing answer, sanitized rationale)
- ✅ HTML entity escaping added to
escapeMarkdownCellis a welcome correctness fix
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.8 AIC · ⌖ 8.35 AIC · ⊞ 7.1K
Comment /matt to run again
| @@ -227,7 +254,22 @@ describe("run_evals.cjs", () => { | |||
| await parseMain(); | |||
There was a problem hiding this comment.
[/tdd] No isolated unit tests for normalizeEvalAnswer and normalizeRationale — both are now exported pure functions but are only tested through parseMain integration paths.
💡 Suggested tests
describe('normalizeEvalAnswer', () => {
it('normalizes YES (case-insensitive)', () => expect(normalizeEvalAnswer(' yes ')).toBe('YES'));
it('normalizes UNKNOWN to NO', () => expect(normalizeEvalAnswer('UNKNOWN')).toBe('NO'));
it('normalizes empty string to NO', () => expect(normalizeEvalAnswer('')).toBe('NO'));
});
describe('normalizeRationale', () => {
it('collapses newlines to spaces', () => expect(normalizeRationale('a
b')).toBe('a b'));
it('sanitizes script tags', () => expect(normalizeRationale('<script>')).not.toContain('<script>'));
});Pure-function tests make normalization-contract regressions immediately visible and cheap to run.
@copilot please address this.
| .trim() | ||
| .toUpperCase(), | ||
| answer: normalizeEvalAnswer(String(record.answer ?? "")), | ||
| rationale: String(record.rationale ?? "").trim(), |
There was a problem hiding this comment.
[/codebase-design] render_evals_summary.cjs re-implements rationale normalization (String(record.rationale ?? '').trim()) independently, while run_evals.cjs has the canonical normalizeRationale for this purpose. Divergence risks subtle differences (no sanitization here) if rationale handling evolves.
💡 Suggestion
Import and reuse normalizeRationale from run_evals.cjs here, the same way normalizeEvalAnswer is already imported:
const { normalizeEvalAnswer, normalizeRationale } = require('./run_evals.cjs');
// ...
rationale: normalizeRationale(String(record.rationale ?? '')),This keeps the normalization contract in one place.
@copilot please address this.
|
@copilot run pr-finisher skill |
|
@copilot quick triage for this PR:
|
The daily evals report showed degraded signal quality because binary eval questions were surfacing
UNKNOWN, which inflated ambiguity in per-workflow pass rates and aggregate YES-rate reporting. This change makes eval outputs strictly binary so missing or indeterminate evidence is recorded consistently asNO.Eval parsing
YESorNOonly.UNKNOWNoutputs asNO.Judge prompt contract
<question-id>: YES<question-id>: NOUNKNOWNtoNO.Rendered summaries
UNKNOWNfrom step-summary rendering.YES/NOtallies.Docs and regression coverage
UNKNOWNanswers.Example of the new behavior:
Run: https://github.com/github/gh-aw/actions/runs/30715596263