diff --git a/.claude/rules/grade-layer.md b/.claude/rules/grade-layer.md index b2dbda3b..17dd37bb 100644 --- a/.claude/rules/grade-layer.md +++ b/.claude/rules/grade-layer.md @@ -54,6 +54,8 @@ Sequential, not parallel (mirrors prune DEC-028). `asyncio.gather` deferred to v The cached prompt block is the rubric criterion list (constant per run); the dynamic block is the per-pair `...` envelope. Anthropic prompt-cache TTL defaults to `"1h"` for the grader (vs. drafter's `"5m"`) — 60 sequential calls fit easily with margin for stalls. +**Tolerant JSON extraction (issue #144).** `parse_grade_response` routes the response through `signalforge._common.json_payload.extract_json_payload` (after `_strip_code_fence`) so a judge that narrates a prose preamble before the `{` still parses. The judge model (`claude-sonnet-4-6`) does NOT support an assistant-turn prefill (API 400), so the parser is the only JSON-only guardrail. Same decode rule as the drafter — decode at the first structural char (`{` or `[`) only, return unchanged on failure — see `llm-drafter.md` § "Tolerant JSON extraction"; a no-JSON response still routes to `GradeOutputError(violation_type="json_parse")` and the conservative degrade. + ## Reproducibility hash fields on every GradeEvent (DEC-010, DEC-019) Every `GradeEvent` carries five 16-hex blake2b-8 fingerprints: diff --git a/.claude/rules/llm-drafter.md b/.claude/rules/llm-drafter.md index 88f5b329..fa3759ec 100644 --- a/.claude/rules/llm-drafter.md +++ b/.claude/rules/llm-drafter.md @@ -44,6 +44,17 @@ The `tests/llm/test_prompt_cache_stability.py` snapshot pins the cached-block by `signalforge.llm.client` emits `WARNING: cache marker no-op` only when **both** `cache_creation_input_tokens == 0` and `cache_read_input_tokens == 0` despite the cached block carrying a marker and passing pre-send size check. `cache_creation == 0` alone is the **normal healthy cache-hit case**. Any future "cache health" signal must apply the same dual-zero pattern. +## Tolerant JSON extraction — prose-preamble guardrail (issue #144) + +`claude-sonnet-4-6` reproducibly narrates a reasoning preamble ("I need to analyze the business rules carefully...") **before** the JSON object on the business-rules drafting path, so `parse_draft_response`'s strict `CandidateSchema.model_validate_json` failed at line 1. **Assistant-turn prefill (the usual "JSON only" guardrail) is NOT available on this model** — the API rejects it with HTTP 400 `"This model does not support assistant message prefill. The conversation must end with a user message."` So the **parser is the only place a JSON-only guarantee can live**; the prompt is advisory. + +`parse_draft_response` routes `raw_text` through `signalforge._common.json_payload.extract_json_payload` before `model_validate_json`. The helper strips a leading prose preamble (and trailing content) around a cleanly-decodable JSON value. Two load-bearing rules: + +1. **Decode at the FIRST `{`/`[` only — never scan deeper.** A truncated outer object (whose first brace fails to decode) would otherwise match a complete *inner* fragment, silently turning a "not valid JSON" failure into a wrong-shape parse. On first-candidate failure the helper returns the input unchanged so the strict parser raises the normal `LLMOutputJSONError` with the correct excerpt/position — the truncated/garbage paths are unchanged. +2. **Error envelopes keep the ORIGINAL `raw_text`** (preamble included) so incident reports show exactly what the model emitted; only the `model_validate_json` call sees the extracted payload. The response-audit `response_text_hash` is likewise unchanged (hashes the full API response). + +The grade parser shares the same helper (`grade-layer.md`). Prompt-level "JSON only" hardening (the issue's option 3) was deliberately NOT added: it would rotate the cached system-prompt golden for no load-bearing gain once the parser is the guarantee. + ## Whole-draft fail-loud anchor contract (DEC-003, DEC-022) `signalforge.draft.parser._validate_anchor_contract` collects **every** violation — never short-circuits. Returns a tuple; non-empty raises `LLMOutputAnchorContractError(violations=...)` with the full list. Three independent checks per column: diff --git a/src/signalforge/_common/json_payload.py b/src/signalforge/_common/json_payload.py new file mode 100644 index 00000000..21a727da --- /dev/null +++ b/src/signalforge/_common/json_payload.py @@ -0,0 +1,56 @@ +"""Tolerant JSON-payload extraction shared by the LLM-response parsers. + +Issue #144: ``claude-sonnet-4-6`` reproducibly emitted a reasoning preamble +("I need to analyze the business rules carefully...") *before* the JSON +object on the business-rules drafting path, so the strict +``json.loads`` / ``model_validate_json`` parse failed at line 1. The +obvious guardrail — an assistant-turn prefill forcing JSON-only output — +is **not available**: the API rejects it with +``"This model does not support assistant message prefill. The +conversation must end with a user message."`` (HTTP 400). The parser is +therefore the only place a JSON-only guarantee can live, and prompts are +advisory at best. + +:func:`extract_json_payload` locates the first complete JSON value +embedded in a response and returns just that substring, so the caller's +existing strict parser runs on clean JSON. Both the drafter +(:func:`signalforge.draft.parser.parse_draft_response`) and the grader +(:func:`signalforge.grade.parser.parse_grade_response`) route through it. +""" + +from __future__ import annotations + +import json + +__all__ = ("extract_json_payload",) + + +def extract_json_payload(text: str) -> str: + """Return the first complete JSON value embedded in ``text``. + + Tolerates a leading prose preamble (and any trailing content) around a + single JSON object/array — the issue #144 failure mode, where the model + narrates plain sentences before the ``{``. The text is + whitespace-stripped, then decoding is attempted **at the first** ``{`` + or ``[`` via :meth:`json.JSONDecoder.raw_decode`; on success the + substring spanning that value is returned (trailing content discarded). + + Decoding is attempted at the first structural character ONLY — never at + later braces. Scanning deeper would let a *truncated* outer object + (whose first ``{`` fails to decode) match a complete **inner** fragment, + silently turning a "not valid JSON" failure into a wrong-shape parse. + So if the first candidate fails to decode, the whitespace-stripped input + is returned unchanged and the caller's strict parser raises its normal + JSON error with the correct excerpt/position. This helper never raises + and never mutates the JSON bytes — it only trims a leading preamble and + trailing content around a cleanly-decodable value. + """ + stripped = text.strip() + first = next((i for i, ch in enumerate(stripped) if ch in "{["), -1) + if first == -1: + return stripped + try: + _value, end = json.JSONDecoder().raw_decode(stripped, first) + except json.JSONDecodeError: + return stripped + return stripped[first:end] diff --git a/src/signalforge/draft/parser.py b/src/signalforge/draft/parser.py index 50072827..54e000b5 100644 --- a/src/signalforge/draft/parser.py +++ b/src/signalforge/draft/parser.py @@ -42,6 +42,7 @@ from pydantic import ValidationError +from signalforge._common.json_payload import extract_json_payload from signalforge.draft.errors import ( LLMOutputAnchorContractError, LLMOutputJSONError, @@ -207,8 +208,18 @@ def parse_draft_response( sniff message text to render an incident report. """ # Stage 1 — JSON parse + Pydantic validation. + # + # Extract the embedded JSON object first (issue #144): some models + # (notably claude-sonnet-4-6 on the business-rules path) narrate a + # prose preamble before the `{`, and the model does not support an + # assistant-turn prefill to force JSON-only output. `extract_json_payload` + # strips the preamble; on a response with no JSON it returns the text + # unchanged so the error path below still fires with the right excerpt. + # The error envelopes keep the ORIGINAL `raw_text` so incident reports + # show exactly what the model emitted, preamble included. + payload = extract_json_payload(raw_text) try: - candidate = CandidateSchema.model_validate_json(raw_text) + candidate = CandidateSchema.model_validate_json(payload) except ValidationError as exc: if _is_json_invalid_error(exc): # Recover (line, column) positional context by re-parsing @@ -218,7 +229,7 @@ def parse_draft_response( # stdlib parser), fall through to the validation-error path # so we never lose the failure signal. try: - json.loads(raw_text) + json.loads(payload) except json.JSONDecodeError as decode_exc: raise LLMOutputJSONError( "LLM response was not valid JSON.", diff --git a/src/signalforge/grade/parser.py b/src/signalforge/grade/parser.py index d7aa4281..5c9a6ac0 100644 --- a/src/signalforge/grade/parser.py +++ b/src/signalforge/grade/parser.py @@ -13,9 +13,14 @@ 1. Strip surrounding whitespace and a single optional Markdown code fence (the model occasionally wraps its JSON in `````json ... - `````; we strip the common cases but do **not** attempt to - extract JSON from arbitrary prose). -2. ``json.loads`` the stripped text. A :class:`json.JSONDecodeError` + `````), then extract the embedded JSON value via + :func:`signalforge._common.json_payload.extract_json_payload` — the + judge can narrate a prose preamble before the ``{`` and the model + does not support an assistant-turn prefill to force JSON-only output + (issue #144). Extraction decodes at the first ``{``/``[`` only and + returns the text unchanged when no JSON value is present, so a + genuinely prose-only / truncated response still fails loud at step 2. +2. ``json.loads`` the extracted text. A :class:`json.JSONDecodeError` raises :class:`GradeOutputError` with ``violation_type="json_parse"``; a top-level non-object payload (list / scalar / etc.) likewise raises ``violation_type="json_parse"`` @@ -47,6 +52,7 @@ import json import math +from signalforge._common.json_payload import extract_json_payload from signalforge.grade.errors import GradeOutputError from signalforge.grade.models import GradingResult from signalforge.grade.rubric import Criterion @@ -99,7 +105,12 @@ def parse_grade_response( with a ``violation_type`` from the locked taxonomy on every malformed shape. """ - cleaned = _strip_code_fence(response_text) + # Strip a Markdown code fence, then extract the embedded JSON object — + # the judge (claude-sonnet-4-6) can narrate a prose preamble before the + # `{`, and the model does not support an assistant-turn prefill to force + # JSON-only output (issue #144). `extract_json_payload` returns the text + # unchanged when no JSON value is present so the error path still fires. + cleaned = extract_json_payload(_strip_code_fence(response_text)) try: payload = json.loads(cleaned) except json.JSONDecodeError as exc: diff --git a/tests/_common/test_json_payload.py b/tests/_common/test_json_payload.py new file mode 100644 index 00000000..c29fe446 --- /dev/null +++ b/tests/_common/test_json_payload.py @@ -0,0 +1,80 @@ +"""Tests for :func:`signalforge._common.json_payload.extract_json_payload`. + +The helper is the load-bearing JSON-only guardrail for the LLM-response +parsers (issue #144): claude-sonnet-4-6 can narrate a prose preamble +before the JSON object and does NOT support an assistant-turn prefill to +force JSON-only output, so the parser must tolerate the preamble. +""" + +from __future__ import annotations + +import json + +import pytest + +from signalforge._common.json_payload import extract_json_payload + +pytestmark = pytest.mark.unit + + +def test_plain_json_object_passthrough() -> None: + text = '{"a": 1, "b": [2, 3]}' + assert extract_json_payload(text) == text + + +def test_strips_leading_prose_preamble() -> None: + """The issue #144 failure mode: prose before the object.""" + preamble = "I need to analyze the business rules carefully. The first rule is a tautology. " + payload = '{"columns": [], "tests": []}' + extracted = extract_json_payload(preamble + payload) + assert extracted == payload + assert json.loads(extracted) == {"columns": [], "tests": []} + + +def test_strips_trailing_content_after_object() -> None: + payload = '{"score": 0.8, "passed": true}' + assert extract_json_payload(payload + "\n\nHope that helps!") == payload + + +def test_strips_markdown_code_fence_via_first_brace() -> None: + text = '```json\n{"ok": true}\n```' + assert json.loads(extract_json_payload(text)) == {"ok": True} + + +def test_decodes_at_first_brace_when_it_is_valid_json() -> None: + """When the first structural char begins a valid object, it is returned + even if more prose/JSON follows.""" + text = 'Result: {"verdict": "yes"} and some trailing note {"ignored": 1}' + assert json.loads(extract_json_payload(text)) == {"verdict": "yes"} + + +def test_truncated_outer_object_not_rescued_by_inner_fragment() -> None: + """A truncated outer object must NOT be silently replaced by a complete + inner fragment — decoding is attempted at the first brace only, so this + returns unchanged and the caller's strict parser fails loud.""" + truncated = '{"columns": [{"name": "order_id"}], "tests": [' # no closing braces + assert extract_json_payload(truncated) == truncated + with pytest.raises(json.JSONDecodeError): + json.loads(extract_json_payload(truncated)) + + +def test_extracts_leading_array() -> None: + text = "Sure! [1, 2, 3]" + assert extract_json_payload(text) == "[1, 2, 3]" + + +def test_no_json_returns_stripped_input_unchanged() -> None: + """No decodable JSON → return the stripped text so the caller's strict + parser raises its normal error with the right excerpt.""" + text = " this is not json at all " + assert extract_json_payload(text) == "this is not json at all" + + +def test_empty_input_returns_empty() -> None: + assert extract_json_payload(" ") == "" + + +def test_does_not_mutate_json_bytes() -> None: + """Whitespace inside the object is preserved verbatim (no re-encoding).""" + payload = '{"a": 1,\n "b": 2}' + assert extract_json_payload("preamble " + payload) == payload diff --git a/tests/draft/test_parser.py b/tests/draft/test_parser.py index 4c37896f..ea1119a2 100644 --- a/tests/draft/test_parser.py +++ b/tests/draft/test_parser.py @@ -75,6 +75,28 @@ def test_parse_draft_response_happy_path() -> None: assert {c.name for c in result.columns} == _FCT_ORDERS_COLUMNS +def test_parse_draft_response_tolerates_prose_preamble() -> None: + """Issue #144: claude-sonnet-4-6 narrates before the `{` on the + business-rules path and the model rejects an assistant prefill, so the + parser must strip the preamble and still parse the embedded JSON.""" + raw = ( + "I need to analyze the business rules carefully. The first rule is a " + "tautology, so I'll only propose tests that add signal.\n\n" + ) + _read("llm_response_valid.json") + result = parse_draft_response(raw, _FCT_ORDERS_COLUMNS, llm_result_meta=_meta()) + assert isinstance(result, CandidateSchema) + assert result.name == "fct_orders" + assert {c.name for c in result.columns} == _FCT_ORDERS_COLUMNS + + +def test_parse_draft_response_pure_prose_still_raises_json_error() -> None: + """A response with no JSON object at all still fails loud (the preamble + tolerance must not mask a genuinely empty/garbage response).""" + raw = "I cannot help with that request." + with pytest.raises(LLMOutputJSONError): + parse_draft_response(raw, _FCT_ORDERS_COLUMNS, llm_result_meta=_meta()) + + def test_parse_draft_response_truncated_raises_json_error() -> None: raw = _read("llm_response_truncated.json") with pytest.raises(LLMOutputJSONError) as excinfo: diff --git a/tests/grade/test_parser.py b/tests/grade/test_parser.py index ca9d9407..c06093ee 100644 --- a/tests/grade/test_parser.py +++ b/tests/grade/test_parser.py @@ -63,6 +63,15 @@ def test_parse_grade_response_strips_unfenced_code_fence() -> None: assert result.criterion_id == _CRITERION.id +def test_parse_grade_response_tolerates_prose_preamble() -> None: + """Issue #144: the judge can narrate before the `{` and the model + rejects an assistant prefill, so the parser strips the preamble.""" + raw = "Let me think about this. The description is clear, so:\n\n" + json.dumps(_payload()) + result = parse_grade_response(raw, artifact_id=_ARTIFACT_ID, criterion=_CRITERION) + assert result.criterion_id == _CRITERION.id + assert result.score == 0.8 + + def test_parse_grade_response_strips_surrounding_whitespace() -> None: raw = " \n\t" + json.dumps(_payload()) + "\n " result = parse_grade_response(raw, artifact_id=_ARTIFACT_ID, criterion=_CRITERION)