Skip to content

fix(eval): skip non-text blocks when extracting Claude judge responses - #3886

Closed
itomek wants to merge 2 commits into
mainfrom
issue-3884
Closed

itomek wants to merge 2 commits into
mainfrom
issue-3884

Conversation

@itomek

@itomek itomek commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

The eval judge crashes the moment it talks to its own default model. claude-opus-5 (the default DEFAULT_CLAUDE_MODEL) is an extended-thinking model that prepends a ThinkingBlock with no .text attribute — ClaudeClient indexed message.content[0].text blindly, so the very first judged reply threw AttributeError and every eval gate using the default judge was unrunnable.

  • Fix: ClaudeClient._first_text() scans the response for the first block that actually has text instead of assuming block 0. Used at all four analyze_file / analyze_file_with_usage call sites. A response with no text block at all raises a ValueError naming the model and the block types received — no silent ""/None that would turn a broken judge into a silently wrong score.
  • Through the AMD gateway, claude-opus-5 sometimes returns ['thinking','text'] and sometimes ['text']. Both shapes are observed; the trigger is unidentified. content[0].text is therefore unsafe on this path regardless of how often each shape occurs. A reviewer who runs one call and sees ['text'] has observed the benign shape, not disproved the bug.

Closes #3884

Test plan

  • tests/unit/eval/test_claude_judge.py — 34 passed (5 new cases pin _first_text: HTML/binary branches of both analyze_file and analyze_file_with_usage, plus the no-text-block error path)
  • python -m pytest tests/ -x -k "claude_judge or claude" — 131 passed, 21 skipped
  • python util/lint.py --all — all quality checks passed
  • Live before/after through the real gateway (ANTHROPIC_BASE_URL + Ocp-Apim key), same model, same call:
    • Before (unfixed): AttributeError: 'ThinkingBlock' object has no attribute 'text'
    • After (fixed): 'The sky appears blue because of Rayleigh scattering, in which air molecules scatter shorter (blue) wavelengths of sunlight more strongly than longer ones.'
Reviewer note on response-shape variability

Sample from an independent verification run against this branch (same max_tokens, same model, consecutive calls):

0: mt=64  blocks=['thinking','text'] | content[0].text -> AttributeError | _first_text -> 'OK'
1: mt=64  blocks=['thinking','text'] | content[0].text -> AttributeError | _first_text -> 'OK'
2: mt=64  blocks=['thinking','text'] | content[0].text -> AttributeError | _first_text -> 'OK'
3: mt=512 blocks=['thinking','text'] | content[0].text -> AttributeError | _first_text -> 'Sunlight contains all colors, '

A separate call in the same session returned a bare ['text'] (no thinking block), which the old code happened to handle. The sample is too small and uncontrolled (gateway routing, thinking budget, and model pinning were not held constant) to support a rate for either shape — the point is only that both shapes occur, not how often. A judge whose pass/fail depends on response shape is worse than one that fails deterministically: it's what trains reviewers to ignore a red eval gate as noise.

claude-opus-5 (the default judge model, config.py DEFAULT_CLAUDE_MODEL)
prepends a ThinkingBlock with no .text attribute. Indexing
message.content[0].text blindly crashes with AttributeError on the
model's first reply. Add failing coverage for analyze_file and
analyze_file_with_usage's four call sites (#3884), plus a case
asserting a response with no text block raises an actionable error
naming the model and block types instead of silently degrading.
claude-opus-5 (DEFAULT_CLAUDE_MODEL) prepends a ThinkingBlock with no
.text attribute, so message.content[0].text crashed with AttributeError
on the judge's very first reply -- every eval gate that uses the
default judge was unrunnable.

Add ClaudeClient._first_text(), which scans for the first block that
actually has text instead of indexing position 0, and use it at all
four call sites in analyze_file / analyze_file_with_usage. A response
with no text block at all raises a ValueError naming the model and the
block types received, rather than returning None/ and silently
turning a broken judge into a wrong eval score.
@github-actions github-actions Bot added eval Evaluation framework changes tests Test changes performance Performance-critical changes labels Sep 15, 2026
@itomek
itomek marked this pull request as ready for review September 15, 2026 06:16
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

This fixes a real crash: the eval judge blew up on the very first reply from its own default model, because that model answers with a thinking block first and the code always read block zero. The fix scans for the first block that actually carries text, and fails loudly with a useful message if none does — the right call for a judge, since a quietly empty verdict is worse than a red gate.

Two things worth a follow-up, neither blocking:

  • The same crash still exists in the eval PDF document generator. It reads block zero of a response from the same default model, so generating eval documents will hit the identical error this PR just fixed everywhere else. Worth patching in this PR while the context is fresh.
  • The new helper stops at the first text block, while three existing judges in the eval package join every text block. If a reply ever comes back with more than one text segment, this one silently returns a truncated verdict. Matching the existing join behaviour removes that risk.

Real-world evidence

Strong, and it supports the verdict. evidence-bundle.md shows the change driven over real HTTP through the real anthropic SDK (1.5.0) against a local stub returning a thinking-block-first response, so the code saw genuine ThinkingBlock/TextBlock objects rather than test doubles — the gap the unit tests can't cover. All four changed call sites returned the planted fact, and the pre-fix commit crashed on the identical response:

analyze_file(html)            -> 'VERDICT: pass — planted fact violet-otter-92'
analyze_file(pdf)             -> 'VERDICT: pass — planted fact violet-otter-92'
analyze_file_with_usage(txt)  -> 'VERDICT: pass — planted fact violet-otter-92' | usage: {...}
analyze_file_with_usage(pdf)  -> 'VERDICT: pass — planted fact violet-otter-92' | usage: {...}
File "/tmp/prefix-wt/src/gaia/eval/claude.py", line 276, in analyze_file
    return message.content[0].text
AttributeError: 'ThinkingBlock' object has no attribute 'text'

The no-text-block path was exercised too, and names the real SDK class:

ValueError -> Claude model 'claude-opus-5' returned no text content block (block types: ['ThinkingBlock']); cannot extract a judge response.

CLI (gaia eval --help, gaia eval sessions --help) exercised at exit 0. Marked N/A with reasons: MCP and Agent UI (no surface imports this module), and a live call to api.anthropic.com (credential policy). A full gaia eval agent scorecard vs. baseline is marked pending strix-halo lane — no real inference on this runner. I could not run the unit suite here (pytest isn't installed on this lane), so the test-plan numbers rest on the author's run.

🔍 Technical details

🟡 Important

1. The same content[0].text bug survives in the PDF document generator (src/gaia/eval/pdf_document_generator.py:218 and :264)

get_completion_with_usage returns the raw block list (claude.py:203), and PDFDocumentGenerator defaults to DEFAULT_CLAUDE_MODEL = claude-opus-5 (pdf_document_generator.py:32, eval/config.py:16) — the exact model whose thinking block motivated this PR. Both sites therefore raise AttributeError: 'ThinkingBlock' object has no attribute 'text' on the first generated document, same as the four sites you fixed.

            generated_content = (
                "".join(
                    getattr(block, "text", "") for block in response["content"]
                )
                if isinstance(response["content"], list)
                else response["content"]

Apply the same shape at :264 (extension_content).

2. _first_text returns one block where the package's other judges concatenate (src/gaia/eval/claude.py:106-109)

draft_quality.py:283, briefing_quality.py:321, and action_item_quality.py:722 all do "".join(getattr(block, "text", "") for block in content if hasattr(block, "text")). A multi-text-block reply (interleaved thinking, or any future tool-use path) would make _first_text return a truncated verdict silently — the failure mode the PR's own ValueError was added to avoid. Low probability, but the cost of matching the existing pattern is one line:

        texts = [
            getattr(block, "text", None)
            for block in content
            if getattr(block, "text", None) is not None
        ]
        if texts:
            return "".join(texts)

If you deliberately want first-block-only semantics, a one-line WHY comment saying so would stop the next reader from "fixing" it back.

Strengths

  • The error path is exactly right for a judge: a ValueError naming the model and the observed block types, rather than an empty string that would turn a broken judge into a quietly wrong score. Matches CLAUDE.md's no-silent-fallbacks rule and its "actionable errors name what failed / what to do / where to look" bar.
  • The test fixture's use of SimpleNamespace over Mock() is the correct choice, and the comment explains why (a Mock auto-vivifies .text and would pass against the unfixed code). That's the difference between a test that pins the bug and one that only looks like it does.
  • Five cases covering both branches of both changed methods plus the no-text error path, and the evidence bundle closes the remaining gap by driving real SDK block types over HTTP — the unit tests alone couldn't prove the ThinkingBlock shape.

@itomek

itomek commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #3885, which fixes the same bug and more. Mine only covered the four sites in src/gaia/eval/claude.py; #3885 also repairs pdf_document_generator.py:218 and :266, which read response["content"][0].text on the raw block list and have exactly the same failure — I verified both lines. It also factors the fix as a reusable module-level helper rather than a private method, and its error names the remedy as well as the model and block types.

It caught a real error in #3884's acceptance criteria too: gaia eval agent --category tool_selection scores via the claude -p subprocess (src/gaia/eval/runner.py), not ClaudeClient, so this fix does not touch that path and does not resolve #3341. Criterion 4 as I wrote it was wrong.

@itomek itomek closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

eval Evaluation framework changes performance Performance-critical changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(eval): the judge crashes on its own default model — content[0] is a thinking block

1 participant