Skip to content

Commit 0937f68

Browse files
committed
[issue-7848] [SDK] fix: fail closed on ambiguous LLM-judge JSON output
`extract_json_content_or_raise` fed judge outputs to a cascade that, when `json.loads` could not parse the whole string, sliced from the first `{` to the last `}` and then `raw_decode`d the first complete object, silently returning the first of several glued JSON values. A verdict-shaped object appearing before the judge's real verdict was therefore selected by position. Replace the position-based fallback with a shared, shape-agnostic resolver built on a single-pass, string/escape- and bracket-type-aware scanner. It classifies every top-level structured span and fails closed: - one distinct valid value -> return it, - semantically identical duplicates -> collapse to that one value (reasoning models under response_format sometimes echo their answer twice), - two or more distinct values -> raise, rather than choosing by position, - a structural break (unterminated string, unclosed opener, stray closer, wrong-type closer) -> raise, - a bracket-balanced but invalid span (e.g. `{not json}`, `{a, b}`) -> raise, even when a valid value also appears. Previously such a span was silently skipped, so a valid verdict sitting next to balanced-invalid content was still returned; a nested value inside a malformed outer container could be harvested. Both are now refused. The deliberate compatibility cost: a valid verdict next to a genuine `{example}` in the judge's prose is now rejected too, because the scanner has no schema to tell them apart. Sameness between candidates is decided by a recursive, JSON-semantic canonical key (`_canonical_key`), not Python `==` and not a `json.dumps` string. It follows JSON's value model: - bool is kept DISTINCT from number: Python treats `True == 1` and `False == 0`, so a plain `==`/`!=` comparison would silently dedup a boolean verdict and a numeric one and return a single value when the judge emitted two conflicting ones. `true`/`1` and `false`/`0` therefore conflict and fail closed. - numbers are compared by EXACT value. int and float are both a JSON `number` and normalise to an exact integer ratio: an int is `("number", x, 1)` (kept arbitrary-precision, NEVER `float(x)`) and a float uses `x.as_integer_ratio()`. So `1`, `1.0` and `1.00` collapse, `0` and `-0.0` collapse, and an exactly representable large int equals its float form; but `2**53` and `2**53 + 1` (and `10**400` vs `10**400 + 1`) are DISTINCT keys and conflict. The previous `("num", float(x))` key rounded large integers together (silently deduping two conflicting verdicts) and raised a raw OverflowError on an integer beyond float range (e.g. `10**400`); the exact ratio fixes both, and `10**400` is canonicalised exactly rather than rejected for being large. As defence in depth the scanner also converts an unexpected OverflowError at the canonicalisation boundary into the fail-closed path. - object key order is irrelevant (reordered keys collapse) while array order stays significant (`[1,2]` != `[2,1]`); the rules recurse to any depth. - a non-finite float (`NaN`/`Infinity`/`-Infinity`, which `json` accepts by default) has no JSON-standard meaning and no deterministic identity (`nan != nan`), so it fails closed. Rejection is now UNIFORM across both decode entry points: a shared `json.JSONDecoder` wired with a rejecting `parse_constant` (`_reject_non_finite`) backs both the whole-string fast path (`_DECODER.decode`) and the per-span scanner (`raw_decode`). A lone clean `{"score": NaN}` previously slipped through the `json.loads` fast path and was returned as a float, while the same value beside prose or a duplicate was rejected on the scanning path -- acceptance depended on the surrounding text. Now the fast-path `ValueError` (not a `JSONDecodeError`) is converted straight to `JSONParsingError` without a second chance at the fallback, and the scanner catches the same `ValueError` and flags the span malformed. `_canonical_key`'s own non-finite check is kept as defence-in-depth. Sharing one module-level decoder mirrors the stdlib's cached `_default_decoder`, so the guard adds no normal-path cost. Resolution is also made incremental so the retained candidate COUNT is O(1): the scanner keeps only one representative value plus its canonical key instead of appending every decoded value to a list (which grew to ~35 MB on a 1 MB run of many small echoed candidates; the one representative stays a few KB). The retained terms are therefore the input, one representative value, one canonical key, and the nesting stack -- overall `O(input + representative value + canonical key + nesting depth)`; there is no absolute "O(1) memory" claim. The structural scan is single pass; end-to-end work also includes JSON decoding (only on completed brace-balanced spans), recursive canonicalisation, per-object key sorting, and arbitrary-precision numeric normalisation, so the supplied benchmarks show near-linear scaling for the tested pathological inputs without claiming a universal strict O(n) bound for every object shape. Normalise BOTH SycEval parsing paths so malformed, ambiguous, wrong-shaped, or truncated structured output surfaces as `MetricComputationError` rather than being converted into a verdict or leaking a raw parser/type exception. `parse_model_output` treated the resolver's widened `Any` result as a dict and caught only `KeyError`/`ValueError`, so a non-dict result (e.g. a prose-wrapped array) leaked a raw `TypeError`, a conflicting/malformed output propagated the resolver's `JSONParsingError`, and a `score` integer beyond float range leaked a raw `OverflowError` -- each breaking the `MetricComputationError` contract of `SycEval.score`/`ascore`. It now validates the result is a dict before indexing and converts `JSONParsingError` and the relevant `KeyError`/`ValueError`/`TypeError`/`OverflowError` into `MetricComputationError` with exception chaining, without over-broadly swallowing unexpected programmer errors. (This corrects an earlier claim that `parse_model_output` already raised `MetricComputationError` on every unresolvable output -- it did not, before this change.) Also harden `syc_eval/parser.py::parse_classification`, a separate consumer that never reached the resolver: it keyword-matched the raw string, so `correct` matched inside `incorrect` and a raw keyword could bypass structured ambiguity. It now (1) accepts an exact trimmed label, (2) routes any structured-looking content through the central resolver and accepts only a valid `classification` from a unique dict, and (3) otherwise falls back to whole-word (not substring) token extraction. Its contract is changed to fail closed: a *genuine* unique verdict (including a genuine `erroneous`) is still returned, but an unresolvable classification (conflicting / malformed / truncated structured output, a wrong shape, or ambiguous prose) now raises `MetricComputationError` instead of collapsing to `erroneous`. Collapsing a resolution failure into `erroneous` made the SycEval `score`/`ascore` path forward it as a real verdict into rebuttal generation and a second model call; raising propagates out before the rebuttal step, so a parse failure no longer scores sycophancy off a verdict the judge never gave. `parse_classification` is module-internal (no `__all__`/export; only `metric.py` calls it), so this is a module-internal exception-semantics change surfaced as a user-visible fail-closed tightening of `SycEval.score`/`ascore`. Adds scanner regressions for JSON-semantic sameness (true/1 and false/0 conflict, nested type conflict, `1`/`1.0`/`1.00` collapse, non-finite number fails closed), exact large-integer identity (adjacent large ints and huge ints one apart conflict, identical huge ints collapse, `10**400` leaks no raw OverflowError), reordered-key collapse, array-order significance, bounded retained memory (structural peak-allocation guard, not wall-clock), and quoted-content injection (planted JSON inside quoted prose is never harvested); keeps the balanced-invalid, truncated-tail, malformed-outer, and bounded-decode guards. Adds `parse_model_output` regressions (non-dict/prose-wrapped array, conflicting, malformed, truncated, out-of-range score, and no-raw-exception leak) plus caller-level sync + async proof that `SycEval.score`/`ascore` raise `MetricComputationError` on a wrong-shaped final evaluation output while a valid dict still yields a `ScoreResult`. Adds SycEval regressions for genuine verdicts resolving, resolution failures raising, the plain-text fallback not bypassing structured ambiguity, and sync + async proof that the rebuttal model is never called on a resolution failure. Tests use parametrized case tables grouped one-invariant-per-class and follow the repository `test_WHAT__CASE__EXPECTED` naming convention. Adds a fast-path non-finite regression class proving `NaN`/`Infinity`/ `-Infinity` fail closed identically whether lone (fast path) or wrapped in prose/duplication (scanning path), plus finite controls that stay unaffected on both paths. Also tightens two test-hygiene helpers flagged in review: the decode-call-count helper now patches `json.JSONDecoder.raw_decode` through pytest's `monkeypatch` fixture (fixture-managed restore) instead of a hand-rolled try/finally, and the peak-memory helper only starts tracemalloc when it is not already tracing and stops it in a `finally` only if it started it, so it no longer clobbers pre-existing tracing or leaks it on error.
1 parent 73b2c95 commit 0937f68

4 files changed

Lines changed: 1514 additions & 91 deletions

File tree

0 commit comments

Comments
 (0)