Skip to content

Trace the StructuredJson rejection to one root cause (3.7) - #191

Merged
joslat merged 2 commits into
mainfrom
findings/structured-json-root-cause
Aug 13, 2026
Merged

Trace the StructuredJson rejection to one root cause (3.7)#191
joslat merged 2 commits into
mainfrom
findings/structured-json-root-cause

Conversation

@joslat

@joslat joslat commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Both rejection symptoms are one bug.

The validator reconciles AgentEval's own verdict (question.Correct) against our re-parse of the free-text explanation. Under StructuredJson that explanation is no longer yes/no prose, so the re-parse yields the wrong boolean — and the same failed parse triggers the diagnostic judge retry once per question, which drove base judge calls to zero and tripped the call-accounting bound.

Confirmed against the package rather than assumed: QuestionResult exposes Correct, JudgeExplanation, JudgeRawResponse, JudgeReasoning, JudgeStatus, RawScore — and no structured verdict property. There is nothing to read a verdict from except Correct.

That makes the fix principled, not a loosened bound. The reconciliation exists to catch AgentEval's free-text parser mis-scoring — exactly the bug StructuredJson eliminates. Re-parsing prose to second-guess a structured verdict checks the thing the protocol removed. Skipping it there doesn't weaken the guard; the guard's subject no longer exists, and keeping it is what manufactures the false rejection.

Call accounting falls out of the same reasoning: the diagnostic retry repairs unparseable free-text verdicts, so under StructuredJson it should never fire — and once it doesn't, base judge calls equal the question count and the existing bound passes untouched. The bounds should not be widened; loose enough to admit both judge shapes is loose enough to admit real anomalies.

Remaining work now named precisely: plumb the active protocol into LongMemEvalRunValidator and LongMemEvalPostRunDiagnostics, neither of which currently knows which one ran.

Docs only. No source changes, no guard relaxed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE

Both rejection symptoms are the same bug. The validator reconciles
AgentEval's own verdict, question.Correct, against our re-parse of the
free-text explanation. Under StructuredJson that explanation is no
longer yes/no prose, so the re-parse yields the wrong boolean -- and the
same failed parse triggers the post-run diagnostic judge retry once per
question, which is what drove base judge calls to zero and tripped the
call-accounting bound.

Confirmed against the package rather than assumed:
AgentEval.Memory.External.Models.QuestionResult exposes Correct,
JudgeExplanation, JudgeRawResponse, JudgeReasoning, JudgeStatus and
RawScore, and no structured verdict property at all. There is nothing to
read a verdict from except Correct.

That is what makes the fix principled rather than a loosened bound. The
reconciliation exists to catch AgentEval's free-text parser mis-scoring
-- exactly the bug StructuredJson eliminates. Under that protocol,
re-parsing prose to second-guess a structured verdict checks the thing
the protocol removed, so skipping it does not weaken the guard: the
guard's subject no longer exists, and keeping it is what manufactures
the false rejection.

The call accounting falls out of the same reasoning. The diagnostic
retry repairs unparseable free-text verdicts; under StructuredJson it
should never fire, and once it does not, base judge calls equal the
question count and the existing bound passes untouched. The bounds
should not be widened -- loose enough to admit both judge shapes is
loose enough to admit real anomalies.

Remaining work is now named precisely: plumb the active protocol into
LongMemEvalRunValidator and LongMemEvalPostRunDiagnostics, neither of
which currently knows which one ran.

Docs only; no source changes, and no guard relaxed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE
Copilot AI lite review requested due to automatic review settings August 13, 2026 00:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Documents the root cause behind StructuredJson run rejection by tying both the correctness reconciliation failure and the call-accounting anomaly to the same underlying FreeText-shaped re-parse/retry logic, and outlines the principled fix direction (protocol plumbing + conditional behavior) without relaxing bounds.

Changes:

  • Adds a new “Root cause” section explaining why re-parsing JudgeExplanation breaks under StructuredJson and triggers per-question diagnostic retries.
  • Updates the “What is actually needed” section to specify plumbing JudgeVerdictProtocol into the validator/diagnostics and skipping FreeText-only checks under StructuredJson (without widening bounds).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +76 to +78
The same reasoning disposes of the call accounting: the diagnostic retry repairs unparseable
free-text verdicts, so under StructuredJson it should never fire, and once it does not,
`baseJudgeCalls == questionCount` and the bound is satisfied without being touched.
StopOnErrorLeavesTheRestUnattemptedRatherThanFailed passed locally and
failed on CI with NotAttemptedCount 0. The implementation is fine; the
test's premise was wrong.

It failed request index 0 and assumed that request runs first. All
requests are submitted through Task.Run, which guarantees nothing about
submission order, so on a wide CI runner a later index won the
concurrency gate first and every request completed before the failure
fired -- leaving nothing unattempted and the assertion with nothing to
find. A test that passes on a small box and fails on a wide one is
asserting something the implementation never promised.

Now keyed to the first DISPATCH rather than to an index: whichever
request the scheduler runs first is the one that fails, so the stop
genuinely precedes the remaining work regardless of ordering. Run five
times locally to confirm rather than once.

Worth stating plainly: I introduced this in the bulk-ingestion change
and it reached main. CI caught it on an unrelated docs-only PR, which is
the good outcome available once a flaky test exists -- but the test
should not have been written against an ordering guarantee that was
never made.

4,370 unit tests green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants