From 88588b31b86d3ce7bf606b60674d7f6ce4022a15 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 12:01:08 +0000 Subject: [PATCH] fix(noema): reject a completion the provider declares truncated noema_review_gate.py never read finish_reason -- the field where an OpenAI-compatible provider declares it stopped at the output-token budget rather than because the model finished. Verified on main@f25063882: `grep -c finish_reason` was 0. That is only sometimes caught downstream. Driving main's own parser: '{"decision":"approve",...,"findings":[{"severity":"high"' -> NoemaModelOutputError (unbalanced, fails closed) '{"decision":"approve","summary":"reviewed","findings":[]}' -> parsed as a valid verdict (truncation lands on a closed object) The second case is the dangerous one, and it is not a corner: findings is emitted last, so the likeliest parseable truncation is an approval with an empty or short findings list -- a review cut off mid-thought, accepted as a genuine APPROVE on a required gate. The local repair is deliberately lossless (trailing commas only) so it does not manufacture this; the provider does, and the gate simply never checked where the provider says so. reject_truncated_completion() reads choices[0].finish_reason on the decoded body and raises NoemaModelOutputError only for the unambiguous "length". Missing, empty, or any other value passes untouched, so a provider reporting a vocabulary this gate does not model cannot be failed spuriously; malformed envelopes keep being classified by extract_llm_message_content, which reports their real cause. Deliberately narrow: no retry, no new exception type, no change to who owns repair. The gateway keeps that, per the caller attempts=1 contract. This is the portable part of #1606, which cannot merge as written because it also adds caller-side retry against that contract. Same failure family as #1921 -- a reviewer that could not see everything returning APPROVE with nothing in the output saying so. Tests assert the payload really would have parsed before the guard, that every other finish_reason is allowed, that shape errors stay deferred to the content parser, and that the guard runs ahead of extraction. Full suite 2905 passed, 1 skipped, 21 subtests; noema_review_gate.py 100% coverage over 864 statements / 388 branches; interrogate 100%. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- scripts/ci/noema_review_gate.py | 46 ++++++++++++++++++++ tests/test_noema_review_gate.py | 76 +++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5ab7e830f3..ff3dac2981 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -1206,6 +1206,51 @@ def _extract_json_object_once(text: str) -> dict[str, Any]: ) from exc +def reject_truncated_completion(raw: str) -> None: + """Fail closed when the provider declares it stopped at its output budget. + + An OpenAI-compatible provider reports ``finish_reason="length"`` when it + terminated generation at the completion-token limit rather than because the + model finished. Such a response is only *sometimes* detectable downstream: + if the cut lands mid-token the JSON is unbalanced and + ``extract_json_object`` fails closed, but if it happens to land after a + syntactically complete object the verdict parses cleanly. Because + ``findings`` is emitted last, the most likely parseable truncation is an + approval carrying an empty or short findings list -- a review that was cut + off mid-thought, indistinguishable from a genuine APPROVE. + + Only the unambiguous ``"length"`` value is rejected. A missing, empty, or + otherwise-valued ``finish_reason`` is left alone, so a provider that + reports a vocabulary this gate does not model cannot be failed spuriously. + + Args: + raw: The decoded HTTP response body of a chat-completion request. + + Raises: + NoemaModelOutputError: If the first choice declares + ``finish_reason="length"``. The message embeds no part of the + untrusted body, so it stays safe in the public + ``pull_request_target`` job log. + """ + try: + data = json.loads(raw) + except json.JSONDecodeError: + return + if not isinstance(data, dict): + return + choices = data.get("choices") + if not isinstance(choices, list) or not choices: + return + first_choice = choices[0] + if not isinstance(first_choice, dict): + return + if first_choice.get("finish_reason") == "length": + raise NoemaModelOutputError( + "Noema LLM completion stopped at the provider output-token budget " + "(finish_reason=length); the verdict is truncated and cannot be trusted" + ) + + def extract_llm_message_content(raw: str) -> str: """Parse and validate the OpenAI-compatible chat-completion HTTP envelope. @@ -1593,6 +1638,7 @@ def call_llm( active_phase = "decoding" raw = decode_llm_response_body(raw_bytes) served_model = _extract_served_model(raw) + reject_truncated_completion(raw) content = extract_llm_message_content(raw) verdict = extract_json_object(content) active_phase = "validating" diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5fa23dec53..5ebea5e573 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -2643,3 +2643,79 @@ def test_parse_args_and_main(monkeypatch): noema.main( ["--repo", "owner/repo", "--pr-number", "9", "--expected-head", "A" * 40] ) + + +def test_reject_truncated_completion_rejects_provider_declared_length_stop(): + """A finish_reason=length completion is refused even when its JSON parses. + + Verified against main before the guard existed: a budget-terminated + response whose cut lands after a syntactically complete object parses as a + clean APPROVE. Because ``findings`` is emitted last, the likeliest + parseable truncation is an approval with an empty findings list, which is + indistinguishable from a genuine one. + """ + parseable_but_truncated = json.dumps( + { + "choices": [ + { + "finish_reason": "length", + "message": { + "content": '{"decision":"approve","summary":"reviewed","findings":[]}' + }, + } + ] + } + ) + # The payload really would have parsed: that is what makes the guard necessary. + assert noema.extract_json_object( + noema.extract_llm_message_content(parseable_but_truncated) + ) == {"decision": "approve", "summary": "reviewed", "findings": []} + + with pytest.raises(noema.NoemaModelOutputError, match="finish_reason=length"): + noema.reject_truncated_completion(parseable_but_truncated) + + +@pytest.mark.parametrize( + "finish_reason", + ["stop", "", None, "tool_calls", "end_turn", "content_filter"], +) +def test_reject_truncated_completion_allows_every_other_finish_reason(finish_reason): + """Only the unambiguous length signal is rejected. + + A provider reporting a vocabulary this gate does not model must not be + failed spuriously, so anything other than ``"length"`` passes through. + """ + envelope = json.dumps( + {"choices": [{"finish_reason": finish_reason, "message": {"content": "{}"}}]} + ) + assert noema.reject_truncated_completion(envelope) is None + + +@pytest.mark.parametrize( + "raw", + [ + "not json at all", + json.dumps([]), + json.dumps({}), + json.dumps({"choices": "not-a-list"}), + json.dumps({"choices": []}), + json.dumps({"choices": ["not-an-object"]}), + json.dumps({"choices": [{}]}), + ], +) +def test_reject_truncated_completion_defers_shape_errors_to_the_content_parser(raw): + """This guard answers one question only and never raises a shape error. + + Malformed envelopes are already rejected with precise messages by + ``extract_llm_message_content``; duplicating that here would report the + wrong cause for a body this function cannot classify. + """ + assert noema.reject_truncated_completion(raw) is None + + +def test_call_llm_consults_the_truncation_guard_before_parsing_content(): + """The guard must run on the decoded body, ahead of content extraction.""" + source = Path("scripts/ci/noema_review_gate.py").read_text(encoding="utf-8") + guard = source.index(" reject_truncated_completion(raw)\n") + extract = source.index(" content = extract_llm_message_content(raw)\n") + assert guard < extract