Fix evals judge parser dropping YES/NO answers as UNKNOWN for Claude engine output - #49323
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Ready to approve
The focused parser change correctly addresses the reported JSONL shape and includes appropriate regression tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes eval result parsing for Claude’s nested assistant JSONL events.
Changes:
- Extracts text from Claude
assistant.message.contentarrays. - Adds unit and end-to-end regression coverage for multi-question answers.
File summaries
| File | Description |
|---|---|
actions/setup/js/run_evals.cjs |
Supports Claude-native assistant events. |
actions/setup/js/run_evals.test.cjs |
Verifies extraction and answer parsing. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49323 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel: PR #49323 Analysis Complete. Score: 92/100 (Excellent). 2 new JS tests analyzed: 100% design tests (0% implementation). Regression tests verify Claude native assistant JSONL parsing with multiline answers. Edge-case coverage excellent. Inflation ratio 7:1 justified for surgical bug fix. APPROVED: 0% implementation tests (threshold 30%). Unable to post PR comment due to safeoutputs auth bridge issue. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix correctly extends the v3 branch to also match Claude engine native assistant events. The two assistant branches remain mutually exclusive (one checks obj.message.content, the other obj.content as a plain string), so no double-extraction risk. Test coverage for the regression is solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.7 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor suggestion on control flow; overall the fix is clean and well-tested.
📋 Key Themes & Highlights
Key Theme
- Independent
ifbranches: The twoifblocks inextractAssistantTextFromJsonlLogare notelse if, leaving a theoretical double-count path for anassistantevent that has bothmessage.contentand a flatobj.contentfield. Flagged in the inline comment.
Positive Highlights
- ✅ Root cause is precisely diagnosed and clearly described in the PR body
- ✅ Minimal, surgical fix — only two changed lines in production code
- ✅ Regression test directly reproduces the real-world failure (multi-question answer with embedded newlines), covering all three answer states (YES, NO, UNKNOWN)
- ✅ Unit test for
extractAssistantTextFromJsonlLogisolates the new code path clearly - ✅ Inline comment in the source accurately explains why both event types are handled identically
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.3 AIC · ⊞ 7K
Comment /matt to run again
| // v1 legacy schema: assistant event carries raw text content directly | ||
| if (obj.type === "assistant" && typeof obj.content === "string" && obj.content) { | ||
| texts.push(obj.content); | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The two if branches are independent, not else if — an assistant event with both message.content array and a flat obj.content string would push text twice. In practice this shape doesn't appear, but using else if makes the mutual-exclusivity explicit and defends against unexpected future payloads.
💡 Suggested fix
if ((obj.type === "turn_end" || obj.type === "assistant") && obj.message && Array.isArray(obj.message.content)) {
for (const part of obj.message.content) {
if (part && typeof part.text === "string") {
texts.push(part.text);
}
}
} else if (obj.type === "assistant" && typeof obj.content === "string" && obj.content) {
texts.push(obj.content);
}Using else if documents the intent that these two shapes are mutually exclusive and prevents double-counting if a future schema change produces an object with both fields.
@copilot please address this.
There was a problem hiding this comment.
Applied in 7147347 — changed to else if so the two branches are mutually exclusive.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: approve with one non-blocking concern
Correct, well-tested fix for the Claude-native assistant event shape. One latent maintainability/correctness gap flagged inline.
Details
The widened condition on extractAssistantTextFromJsonlLog correctly recognizes Claude's native message.content array shape for assistant events. However, the new branch and the pre-existing legacy obj.content string branch are not mutually exclusive (both check obj.type === "assistant" independently), which could double-push text if a future/hybrid log shape carries both fields — flagged as a review comment with a suggested else if fix. Not blocking since no current known JSONL shape triggers it and test coverage for the new case is solid.
🔎 Code quality review by PR Code Quality Reviewer · auto · 25.8 AIC · ⊞ 7.8K
Comment /review to run again
| } | ||
| } | ||
| // v1 legacy schema: assistant event carries raw text content | ||
| // v1 legacy schema: assistant event carries raw text content directly |
There was a problem hiding this comment.
The two if blocks are independent (not else if), so an event with type === "assistant" and a nested message.content array could also match the legacy obj.content string check on the same object, pushing duplicate text.
💡 Why this matters
The widened condition on line 331 now treats assistant events the same as turn_end for the nested-array shape, but the pre-existing legacy branch below it (line 339) still independently checks obj.type === "assistant" regardless of whether the first branch already consumed it. Nothing guards against an object that happens to carry both a message.content array and a top-level content string — if any current or future Claude/engine payload variant includes both fields, the same logical answer text gets pushed twice into texts, which is then joined and searched by extractAnswerByID. Duplicate text is much harder to detect than the original UNKNOWN failure mode this PR fixes, since the regex would still match (just redundantly), masking future format-drift bugs instead of surfacing them.
Suggested fix: make the branches mutually exclusive, e.g.
if ((obj.type === "turn_end" || obj.type === "assistant") && obj.message && Array.isArray(obj.message.content)) {
for (const part of obj.message.content) {
if (part && typeof part.text === "string") texts.push(part.text);
}
} else if (obj.type === "assistant" && typeof obj.content === "string" && obj.content) {
texts.push(obj.content);
}No current test in this PR exercises an object satisfying both conditions simultaneously, so this gap is untested.
There was a problem hiding this comment.
Applied in 7147347 — changed to else if so the two branches are mutually exclusive, preventing double-extraction for any future payload carrying both fields.
PR TriageCategory: bug (evals judge parser fix) Rationale: Fixes a real production issue (Daily Evals Feature Report flagging false DEGRADED status due to YES/NO answers being misparsed as UNKNOWN across many unrelated workflows). Root-caused with a traced example run, includes a 49-line regression test, and has 2 Labels applied:
|
|
@copilot run pr-finisher skill |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
Please address any remaining review feedback, confirm the failing check is understood, and rerun the relevant checks once ready.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The review feedback has been addressed (commit Regarding the Local validation summary:
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Thanks for the fix! 🎯 This PR looks excellent — the root cause analysis is thorough, the solution is surgical and well-targeted, and the test coverage is comprehensive. You have added both a regression test (parses multiple ID-based answers from Claude engine's native assistant JSONL event) that reproduces the exact real-world issue, and a focused unit test for the The changes to This is ready for merge!
|
|
🎉 This pull request is included in a new release. Release: |
The Daily Evals Feature Report flagged the evals feature as DEGRADED: a 48.6% overall YES rate driven by 32
UNKNOWNanswers spread across many unrelated workflows (Design Decision Gate, AI Moderator, Daily Assign Issue To User, etc.).Tracing a real run (Design Decision Gate, run
30608805370) showed the judge model actually answeredaction-taken: YESanddecision-justified: YES, but the pipeline recorded both asUNKNOWN.Root cause
extractAssistantTextFromJsonlLog(actions/setup/js/run_evals.cjs) only decoded two JSONL shapes: the v3turn_endevent (message.contentarray) and the legacyassistantevent (flat stringcontent).assistantevent with a nestedmessage.contentarray — a shape the function didn't recognize.\nescape sequence (backslash +n) sits directly before the next question ID. That letternis a word character, so the\bword-boundary regex inextractAnswerByIDfails to match — silently downgrading every question after the first toUNKNOWN.Fix
assistantevents with amessage.contentarray the same asturn_endevents, so text is extracted with real newlines before regex matching.Tests
extractAssistantTextFromJsonlLogcovering the Claude-nativeassistant/message.contentshape.parseMainregression test reproducing the exact real-world log (multi-question answer string with embedded\n), asserting all three answers resolve correctly instead of only the first.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.