overseer: advisor parser handles bare-object JSON in prose (#2116)

[jwbron]

Summary

• Under max_turns=1, Claude often emits a bare JSON object surrounded by prose ("Here's the verdict:\n{...}\n") instead of pure JSON or a fenced block. _default_runner stripped fences and ran json.loads on the whole string, so prose-wrapped responses fell through to AdvisorParseError and the consult was lost.
• On JSONDecodeError, fall back to json.JSONDecoder().raw_decode() from the first {. raw_decode is string-aware, so braces inside JSON string values don't fool the scan. Pure-JSON and fenced responses keep hitting the existing fast path; only previously-failing inputs change behavior.
• Adds positive coverage for bare-object, fenced, prose-with-bare-object, and prose-around-fenced responses, plus a no-brace negative case so error semantics for genuinely unparseable text stay intact.

Closes #2116. Carry-forward from the #2096 re-review.

Test plan

• `shared/tests/test_overseer_advisor.py` — 23 tests pass (18 prior + 5 new)
• CI lint + full test suite

[egg-reviewer]

Review

The fix is correct, minimal, and well-targeted at the reported failure mode (prose-wrapped JSON under max_turns=1). The fast paths (pure JSON, fenced) are untouched; only previously-failing inputs change behavior. raw_decode is the right tool — confirmed string-aware so a } inside a JSON string value won't terminate the scan early.

No blocking issues found.

Verified behavior

• raw_decode correctly stops at the closing } of the JSON object even when the value strings contain } characters.
• The start = -1 short-circuit avoids a noisy raw_decode failure on responses with no brace at all (e.g. "absolutely no json here"), keeping the existing error message intact.
• from None vs from exc distinction is intentional: the no-brace path has no useful inner exception to chain; the raw_decode path does.
• The new fall-back also fixes an unrelated case the bug report didn't call out: trailing prose after valid JSON ('{"decision":"watch","reasoning":"r"} thanks!'). json.loads rejects this with Extra data, but raw_decode accepts it. Worth being aware of even though it's a strict improvement.

Non-blocking suggestions

1. Single-attempt fall-back is fragile against prose-with-leading-brace. cleaned.find("{") only ever locates the first {. If the model emits a { in prose before the real JSON — e.g. "see {field_name} below: {valid_verdict}" — raw_decode fails on the unquoted-key snippet and the verdict is lost (AdvisorParseError → CLI exit 3 → overseer back-off). I confirmed this: raw_decode on '{x: 1} but real: {"decision": "watch"}' raises Expecting property name enclosed in double quotes. A trivial loop would be more robust:

pos = 0
payload = None
while True:
    start = cleaned.find("{", pos)
    if start == -1:
        break
    try:
        payload, _ = json.JSONDecoder().raw_decode(cleaned[start:])
        break
    except json.JSONDecodeError:
        pos = start + 1
if payload is None:
    raise AdvisorParseError(...)

Probably rare enough in practice (the system prompt says "Do NOT include any other text"), but cheap insurance for what the PR is trying to defend against.

2. The "string-aware" claim in the comment is untested. The new comment explicitly justifies raw_decode over a manual brace-counting scan by noting it handles } inside JSON string values. None of the five new tests exercise that path. A regression-locking case like:

runner = self._runner_returning(
    'Verdict:\n{"decision": "watch", "reasoning": "value with } embedded brace"}'
)

…would prevent a future "let me just use a regex" simplification from quietly regressing the contract.

3. The trailing-prose case is also untested. Now that '{"decision":"watch","reasoning":"r"} thanks!' parses successfully via the fall-back (it didn't before), there's no test pinning that behavior either.

4. Pre-existing — not for this PR, just flagging. The AdvisorParseError message includes {raw!r}, and the CLI in sandbox/egg_lib/orch_cli.py:1823 prints it to stderr. If the model echoes a credential into its prose response, that token appears in stderr and any captured logs unscrubbed. scrub_secrets is only applied to verdict.issue_body, not to the parse-error path. Out of scope for this PR but worth a future cleanup.

Test coverage

The five new tests cover the four common shapes (bare, fenced, prose+bare, prose+fenced) plus a negative case. The negative test correctly verifies the start == -1 branch rather than only the outer json.loads failure. Solid.

— Authored by egg

[egg-reviewer]

Re-review

The new commit (7599b27) cleanly addresses suggestions 1, 2, and 3 from my prior review. Suggestion 4 (scrub_secrets in the parse-error path) was explicitly out of scope and is unchanged — fine.

No blocking issues found.

Verified behavior of the new loop

decoder = json.JSONDecoder()
payload = None
last_exc: json.JSONDecodeError | None = None
pos = 0
while True:
    start = cleaned.find("{", pos)
    if start == -1:
        break
    try:
        payload, _ = decoder.raw_decode(cleaned[start:])
        break
    except json.JSONDecodeError as exc:
        last_exc = exc
        pos = start + 1
if payload is None:
    raise AdvisorParseError(...) from last_exc

• Termination: pos = start + 1 after every failure guarantees forward progress; the loop ends when find returns -1. No risk of an infinite loop.
• No-brace semantics preserved: When no { is ever found, last_exc stays None. raise X from None_value is semantically equivalent to raise X from None (sets __cause__ = None and __suppress_context__ = True), so the original "no useful inner exception to chain" behavior is intact. The test_no_brace_raises_parse_error test still passes.
• payload is None is a sound success flag: raw_decode on a string starting with { either returns a dict (truthy or empty {}) or raises. Empty {} is not None, so it correctly falls through to model_validate (which then raises a validation error with the right message). No conflation between "decoded {}" and "never decoded."
• Decoder reuse: json.JSONDecoder() is constructed once outside the loop. Minor improvement vs. the old code that constructed it inline. No state-leak concerns — JSONDecoder is stateless per call.
• Worst-case complexity: O(n²) in pathological inputs (n = len(cleaned)), but LLM output is bounded by max_turns=1 and the system prompt. Not a concern.

New tests

All three lock the contracts the prior review flagged as untested:

• test_embedded_brace_in_string_value — pins the string-aware raw_decode contract (a value like "value with } embedded brace" does not terminate the scan early). This is the primary justification for choosing raw_decode over a manual brace scanner; locking it prevents a future "let me simplify with a regex" regression.
• test_trailing_prose_after_payload — pins the trailing-data tolerance ('{"decision":"watch","reasoning":"r"} thanks!'). json.loads rejects this with Extra data; the fall-back accepts it via raw_decode.
• test_stray_leading_brace_skipped — exercises the new loop with 'see {field_name} below: {"decision": "watch", "reasoning": "r"}'. Without the loop, the first {field_name} snippet locks raw_decode onto an unparseable substring; with the loop, the scanner advances past it and finds the real verdict.

I ran pytest shared/tests/test_overseer_advisor.py against the PR HEAD: 26 passed.

Non-blocking observations

1. The chained exception in the stray-brace path is now the last failure, not the first. When the loop tries multiple { positions and all fail, last_exc records the failure from the latest attempted position. For diagnostic purposes the first failure (closest to where the model thought the JSON started) is often more informative — it tells you what went wrong with the model's intended payload, not with random text after it. This is a minor diagnostic-quality nit; no functional impact, and only matters in the rare case where every candidate { fails. Not worth changing.

2. Pre-existing — still flagged from prior review. The AdvisorParseError message includes {raw!r} and scrub_secrets is only applied to verdict.issue_body. If the model echoes a credential in its prose response, it appears in stderr unscrubbed. Out of scope here, just keeping it visible for a future cleanup.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

4 previous review(s) hidden.