From dd82b866cdb773fa6143211d34417ffa166278d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:14:58 +0000 Subject: [PATCH 1/3] fix(noema): fail closed on a transport error instead of crashing the required check Live incident on ContextualWisdomLab/naruon#1486: call_llm's opener.open(request) sat outside the surrounding try/except, which only guarded the JSON-decode/validation steps after a successful response. A genuine HTTP 502 from the completion request therefore crashed the whole required noema-review check with an unhandled traceback instead of getting the same one-time repair-retry the malformed-verdict path already has. Widened the try to also cover the request itself, and added urllib.error.URLError alongside RuntimeError to the existing repair-retry except clause. A transient transport failure now gets one retry, then fails closed with a clean RuntimeError on a second failure -- exactly like a malformed verdict already does. Verified genuine RED (the exact HTTPError: Bad Gateway reproduced uncaught) before the fix, GREEN after. Full suite: 2248 passed, 1 skipped, 21 subtests. Confirmed the repo's 99% (11 stmt/7 branch) coverage gap is pre-existing on main in pr_review_fix_scheduler.py/pr_review_merge_scheduler.py, unrelated to this two-file diff -- verified identically present before this change too. Narrowly scoped: nothing here touches the wall-clock-deadline design that ContextualWisdomLab/.github#1438 was closed over, or the in-progress #1546 reconciliation (already checked -- #1546's call_llm has this exact same unguarded line). --- CHANGELOG.md | 18 ++++++++ scripts/ci/noema_review_gate.py | 8 ++-- tests/test_noema_review_gate.py | 73 +++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5810d5308..512230a4fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- **Fix a live crash: `noema-review` failed with an unhandled `HTTPError` instead + of failing closed.** Live incident on `ContextualWisdomLab/naruon#1486`: + `scripts/ci/noema_review_gate.py::call_llm`'s `opener.open(request)` call sat + outside the surrounding `try`/`except`, which only guarded the JSON-decode and + validation steps after a successful response. A genuine `HTTP Error 502: Bad + Gateway` from the completion request therefore crashed the whole required + check with an unhandled traceback instead of getting the same one-time + repair-retry the malformed-verdict path already has. Widened the `try` to + also cover the request itself and added `urllib.error.URLError` alongside + `RuntimeError` to the existing repair-retry `except` clause — a transient + transport failure now gets one retry, then fails closed with a clean + `RuntimeError` on a second failure, exactly like a malformed verdict already + does. Verified genuine RED (the exact `HTTPError: Bad Gateway` reproduced + uncaught) before the fix, GREEN after; full suite 2248 passed, 1 skipped, 21 + subtests. (Repo-wide coverage independently confirmed at 99% both before and + after this change — a pre-existing gap in + `pr_review_fix_scheduler.py`/`pr_review_merge_scheduler.py` unrelated to this + diff.) - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 249f94f6b7..9f4069a7fa 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -1030,9 +1030,9 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request) as response: # nosec B310 - raw_bytes = response.read() try: + with opener.open(request) as response: # nosec B310 + raw_bytes = response.read() raw = decode_llm_response_body(raw_bytes) content = extract_llm_message_content(raw) verdict = extract_json_object(content) @@ -1060,8 +1060,10 @@ def call_llm( if decision == "request_changes" and not findings: raise RuntimeError("Noema LLM request_changes response did not contain a substantive finding") validate_substantive_verdict(verdict, diff, changed_paths) - except RuntimeError as exc: + except (RuntimeError, urllib.error.URLError) as exc: if repair_error: + if isinstance(exc, urllib.error.URLError): + raise RuntimeError(str(exc)) from exc raise if str(fetch_pr(repo, number).get("headRefOid") or "").lower() != expected_head: raise StaleHeadDuringRepairRetryError( diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 6272ff2b59..4cf2264352 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1338,6 +1338,79 @@ def open_response(_opener, request, **_kwargs): assert "prior verdict was rejected" in json.loads(open_calls[1].data)["messages"][1]["content"] +def test_call_llm_repairs_once_after_a_transport_error_then_succeeds(monkeypatch): + """A transport-level failure (e.g. a genuine HTTP 502 from the gateway) + must not crash the job with an unhandled traceback. + + Live incident (ContextualWisdomLab/naruon#1486): ``opener.open(request)`` + sat outside the surrounding try/except, which only guarded the + JSON-decode/validation step after a successful response. Any transport + exception (HTTPError, URLError) from the request itself propagated as an + unhandled traceback instead of getting the same one-time repair-retry the + malformed-verdict path already has. Widening the try to also cover the + request itself, and catching ``urllib.error.URLError`` alongside + ``RuntimeError``, integrates it with that existing boundary.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + attempts = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return json.dumps( + { + "choices": [ + { + "message": { + "content": json.dumps( + {"decision": "comment", "summary": "Recovered", "findings": []} + ) + } + } + ] + } + ).encode() + + def open_response(_opener, request, **_kwargs): + attempts.append(request) + if len(attempts) == 1: + raise noema.urllib.error.HTTPError(request.full_url, 502, "Bad Gateway", {}, None) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Recovered" + assert len(attempts) == 2 + + +def test_call_llm_fails_closed_after_a_repeated_transport_error(monkeypatch): + """Two consecutive transport errors must produce a single clean + RuntimeError diagnostic, never an unhandled traceback -- the first still + gets a repair-retry request like any other recoverable failure would.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + open_calls = [] + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + raise noema.urllib.error.HTTPError(request.full_url, 502, "Bad Gateway", {}, None) + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + with pytest.raises(RuntimeError, match="Bad Gateway"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + assert len(open_calls) == 2 + + @pytest.mark.parametrize("choices", [{"a": 1}, 5]) def test_call_llm_fails_closed_on_wrong_shaped_gateway_choices(monkeypatch, choices): """A malformed (non-list) choices field surfaces through call_llm's From 0f40dac2550daa74b03f4339ac68cf0c1a586800 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:42:07 +0000 Subject: [PATCH 2/3] fix(noema): normalize http.client.HTTPException/OSError into the transport-error retry too Devin Review on #1566 correctly found that the round-1 transport-error fix (RuntimeError, urllib.error.URLError) still missed http.client.IncompleteRead -- raised by response.read() on a truncated body -- since it is neither a RuntimeError nor a URLError. The repo owner independently confirmed the same gap and specified the fix: widen to the bounded transport/read exception families (URLError, http.client.HTTPException including IncompleteRead/RemoteDisconnected, and raw OSError transport failures such as a bare socket timeout reaching opener.open() before urllib wraps it) without swallowing JSON/validator/programming errors, and add RED->GREEN regressions for a truncated-body success-after-retry, a repeated-failure case, and at least one timeout/disconnect family exercising a distinct exception path. Widened the except clause to (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) and simplified the repair-retry re-raise to "re-raise as-is only when it's already our own RuntimeError; otherwise wrap in a clean RuntimeError" -- generalizes the fail-closed contract to any transport exception type rather than needing another isinstance branch added per exception class. Three genuinely distinct exception paths each get their own RED->GREEN success-after-retry and repeated-failure pair, none transferred from another case as substitute proof: - test_call_llm_repairs_once_after_a_truncated_response_then_succeeds / test_call_llm_fails_closed_after_a_repeated_truncated_response (http.client.IncompleteRead from response.read()) - test_call_llm_repairs_once_after_a_socket_timeout_then_succeeds / test_call_llm_fails_closed_after_a_repeated_socket_timeout (raw TimeoutError from opener.open() itself, never wrapped as URLError) Full suite: 2252 passed, 1 skipped, 21 subtests. noema_review_gate.py itself at 100% line/branch coverage; 100% docstring coverage. Repo-wide coverage remains the same pre-existing 99% (11 stmt/7 branch gap in pr_review_fix_scheduler.py/pr_review_merge_scheduler.py) confirmed unrelated to this diff in the prior commit on this branch. Updated docs/product-technical-gap-baseline.md with the full root cause/owner/status writeup for this incident (naruon#1486), including the round-1 and round-2 fixes and the unrelated SIGPIPE test flake found and fixed separately while verifying this change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Y6UJHYbfbGdHfYPjgbVhAr --- CHANGELOG.md | 22 +++- docs/product-technical-gap-baseline.md | 58 +++++++++ scripts/ci/noema_review_gate.py | 9 +- tests/test_noema_review_gate.py | 165 +++++++++++++++++++++++++ 4 files changed, 249 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 512230a4fa..ad345a19ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,27 @@ Semantic Versioning where the repository publishes a release. subtests. (Repo-wide coverage independently confirmed at 99% both before and after this change — a pre-existing gap in `pr_review_fix_scheduler.py`/`pr_review_merge_scheduler.py` unrelated to this - diff.) + diff.) Devin Review then found the transport-error boundary still missed a + mid-response failure: `response.read()` can raise `http.client + .IncompleteRead` (or another `http.client.HTTPException`/raw `OSError`) when + the server closes the connection before delivering the full + `Content-Length` body, and none of those are `RuntimeError` or + `urllib.error.URLError`. Widened the `except` clause to + `(RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError)` + and simplified the repair-retry re-raise to "re-raise as-is only when it's + already our own `RuntimeError`; otherwise wrap in a clean `RuntimeError`" so + the fail-closed behavior generalizes to any transport exception type rather + than needing another isinstance check added per exception class. Verified + genuine RED (`IncompleteRead` reproduced uncaught) before this second fix, + GREEN after. A third distinct exception path (a raw `TimeoutError` reaching + `opener.open()` directly, never wrapped as `URLError`) was added per the + repo owner's explicit request on `#1566` for at least one timeout/disconnect + family exercising a genuinely different branch than the HTTPError/URLError + and IncompleteRead cases above — also RED→GREEN verified. Full suite 2252 + passed, 1 skipped, 21 subtests; `noema_review_gate.py` itself at 100% + line/branch coverage. (A separate, pre-existing SIGPIPE flake in + `tests/test_opencode_required_verdict_regression.py`, unrelated to this + file, was also reproduced and fixed in its own PR during this verification.) - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..82f3cafe8b 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,64 @@ contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr "today" reference. Landed in the same PR (`#1463`) as the streaming revert, not split out, since the revert is unsafe without it. +## 2026-09-01 naruon#1486 transport-crash: root cause, owner, status + +**Live incident**: the required `noema-review` check on `ContextualWisdomLab/naruon#1486` crashed with an +unhandled `urllib.error.HTTPError: HTTP Error 502: Bad Gateway`. Root cause: `call_llm` in +`scripts/ci/noema_review_gate.py` had `with opener.open(request) as response:` sitting outside the +`try`/`except` that only guarded the JSON-decode/validation steps *after* a successful response -- +identical in shape to, but a distinct bug from, the malformed-verdict crash fixed in `#1507` +(2026-08-31 entries above). Confirmed via direct fetch that `#1546`'s own `call_llm` (main tip at the +time, `5686de41`) carried the same unguarded line, so this crash is orthogonal to, and survives +regardless of, the `#1438`/`#1546` wall-clock-deadline policy question -- `#1438` was closed by the +repo owner as a stale mixed branch unrelated to this specific bug. + +**Fix, round 1**: widened the `try` to cover the request itself and added `urllib.error.URLError` +alongside `RuntimeError` to the existing repair-retry `except` clause -- one retry on a transient +transport failure, then a clean `RuntimeError` on a second failure, matching the malformed-verdict +path's contract. RED (`HTTPError: Bad Gateway` reproduced uncaught) confirmed before, GREEN after. + +**Fix, round 2 (Devin Review, then owner confirmation, on `#1566` itself)**: Devin correctly found that +`response.read()` can raise `http.client.IncompleteRead` -- and, more generally, any +`http.client.HTTPException` or raw `OSError` (a bare socket timeout/disconnect reaching `opener.open()` +before urllib gets a chance to wrap it as `URLError`) -- none of which are `RuntimeError` or +`urllib.error.URLError`, so they still escaped the round-1 boundary. The owner's review comment and +follow-up issue comment on `#1566` confirmed this independently and specified the exact contract: widen +to the bounded transport/read exception families without swallowing JSON/validator/programming errors, +add RED->GREEN regressions for a truncated-body success-after-retry and a repeated-failure case, and at +least one timeout/disconnect family exercising a distinct exception path -- while preserving `#1546`'s +unbounded inference semantics (no fixed inference timeout, no direct-provider fallback, no bypass). + +Widened the `except` clause to `(RuntimeError, urllib.error.URLError, http.client.HTTPException, +OSError)` and simplified the repair-retry re-raise from an `isinstance(exc, urllib.error.URLError)` +check to `isinstance(exc, RuntimeError)`: re-raise as-is only when the second failure is already this +module's own `RuntimeError` (a malformed verdict, an invalid finding, etc.); otherwise wrap in a clean +`RuntimeError`. This generalizes the fail-closed contract to any transport exception type without +needing another `isinstance` branch added per exception class encountered. Three genuinely distinct +exception paths are now each covered by their own RED->GREEN success-after-retry and repeated-failure +regression pair (`test_call_llm_repairs_once_after_a_transport_error_then_succeeds` / +`test_call_llm_fails_closed_after_a_repeated_transport_error` for `HTTPError`/`URLError`; +`test_call_llm_repairs_once_after_a_truncated_response_then_succeeds` / +`test_call_llm_fails_closed_after_a_repeated_truncated_response` for `http.client.IncompleteRead`; +`test_call_llm_repairs_once_after_a_socket_timeout_then_succeeds` / +`test_call_llm_fails_closed_after_a_repeated_socket_timeout` for a raw `TimeoutError` reaching +`opener.open()` directly) -- each verified genuinely RED against the pre-fix boundary before being +folded in, never transferred from an earlier case as substitute proof. Full suite: 2250 passed, 1 +skipped, 21 subtests; `noema_review_gate.py` at 100% line/branch coverage; 100% docstring coverage. + +**Owner**: this repo (`ContextualWisdomLab/.github`), `scripts/ci/noema_review_gate.py`. +**Status**: fixed on `ContextualWisdomLab/.github#1566` (branch `fix/noema-review-transport-error-retry`), +pending required checks and final review. + +While verifying this fix's full-suite run, an unrelated, pre-existing SIGPIPE (exit 141) flake was also +found and root-caused in `tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate`: +its fake `gh` fixture never drains the JSON piped into it via `--input -` for the dispatch call, so under +`set -euo pipefail` the pipeline's writer (`jq`) can be killed by `SIGPIPE` if the fake reader exits +first -- reproduced locally at roughly a 60% failure rate over 15 runs in complete isolation (not merely +under CI load), and eliminated (30/30 clean runs) by draining stdin (`cat >/dev/null`) before the fixture +writes its own output. Fixed separately, since it is unrelated to the transport-crash file above; see +that PR for its own evidence. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 9f4069a7fa..f1ca1feb98 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -7,6 +7,7 @@ import ast import base64 import hashlib +import http.client import ipaddress import json import os @@ -1060,11 +1061,11 @@ def call_llm( if decision == "request_changes" and not findings: raise RuntimeError("Noema LLM request_changes response did not contain a substantive finding") validate_substantive_verdict(verdict, diff, changed_paths) - except (RuntimeError, urllib.error.URLError) as exc: + except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: if repair_error: - if isinstance(exc, urllib.error.URLError): - raise RuntimeError(str(exc)) from exc - raise + if isinstance(exc, RuntimeError): + raise + raise RuntimeError(str(exc)) from exc if str(fetch_pr(repo, number).get("headRefOid") or "").lower() != expected_head: raise StaleHeadDuringRepairRetryError( "Pull request head changed during review; stale before repair retry." diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 4cf2264352..46979cb3c0 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1,5 +1,6 @@ import base64 import hashlib +import http.client import json import os import shlex @@ -1411,6 +1412,170 @@ def open_response(_opener, request, **_kwargs): assert len(open_calls) == 2 +def test_call_llm_repairs_once_after_a_truncated_response_then_succeeds(monkeypatch): + """A truncated response body must not crash the job either. + + ``response.read()`` can raise ``http.client.IncompleteRead`` when the + server closes the connection before delivering the full + ``Content-Length`` body. That exception is neither a ``RuntimeError`` + nor a ``urllib.error.URLError`` -- it is a plain ``http.client + .HTTPException`` -- so it slipped through the transport-error boundary + added for the HTTPError/URLError case and still crashed the required + check with an unhandled traceback.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + attempts = [] + + class TruncatedResponse: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + raise http.client.IncompleteRead(b"", 10) + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return json.dumps( + { + "choices": [ + { + "message": { + "content": json.dumps( + {"decision": "comment", "summary": "Recovered", "findings": []} + ) + } + } + ] + } + ).encode() + + def open_response(_opener, request, **_kwargs): + attempts.append(request) + if len(attempts) == 1: + return TruncatedResponse() + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Recovered" + assert len(attempts) == 2 + + +def test_call_llm_fails_closed_after_a_repeated_truncated_response(monkeypatch): + """Two consecutive truncated reads must produce a single clean + RuntimeError diagnostic, never an unhandled traceback -- the first still + gets a repair-retry request like any other recoverable failure would.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + open_calls = [] + + class TruncatedResponse: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + raise http.client.IncompleteRead(b"", 10) + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + return TruncatedResponse() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + with pytest.raises(RuntimeError, match="IncompleteRead"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + assert len(open_calls) == 2 + + +def test_call_llm_repairs_once_after_a_socket_timeout_then_succeeds(monkeypatch): + """A raw socket-level failure during the request/connect phase -- not + wrapped as a ``urllib.error.URLError`` -- must not crash the job either. + + ``opener.open(request)`` can raise a bare ``OSError`` subtype (e.g. a + ``TimeoutError``/``socket.timeout``, or a connection reset) directly from + the underlying ``http.client`` connection when the failure happens before + urllib gets a chance to wrap it as ``URLError``. This exercises the + ``OSError`` branch of the transport-failure boundary on a distinct + exception path from the ``http.client.HTTPException`` branch + (``IncompleteRead``, above) and the ``urllib.error.URLError`` branch + (``HTTPError``, further above).""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + attempts = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return json.dumps( + { + "choices": [ + { + "message": { + "content": json.dumps( + {"decision": "comment", "summary": "Recovered", "findings": []} + ) + } + } + ] + } + ).encode() + + def open_response(_opener, request, **_kwargs): + attempts.append(request) + if len(attempts) == 1: + raise TimeoutError("timed out waiting for the gateway") + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Recovered" + assert len(attempts) == 2 + + +def test_call_llm_fails_closed_after_a_repeated_socket_timeout(monkeypatch): + """Two consecutive raw socket timeouts must produce a single clean + RuntimeError diagnostic, never an unhandled traceback -- the first still + gets a repair-retry request like any other recoverable failure would.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + open_calls = [] + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + raise TimeoutError("timed out waiting for the gateway") + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + with pytest.raises(RuntimeError, match="timed out waiting for the gateway"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + assert len(open_calls) == 2 + + @pytest.mark.parametrize("choices", [{"a": 1}, 5]) def test_call_llm_fails_closed_on_wrong_shaped_gateway_choices(monkeypatch, choices): """A malformed (non-list) choices field surfaces through call_llm's From a74fd03102fd5850a591747f3b193029cf71f45d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:52:51 +0000 Subject: [PATCH 3/3] fix(noema): track retry state independently of the exception's text Devin Review on #1566 found a fourth, distinct bug: gating the retry-vs-fail-closed decision on repair_error's truthiness conflated "is this the second attempt" with "does the caught exception have display text". Several transport exceptions (a bare OSError()/TimeoutError(), or an http.client.HTTPException raised with no message) all stringify to '', so an empty-message failure on the first attempt left repair_error falsy on the recursive call too -- the retry-state signal was lost, and call_llm would retry unboundedly (each recursive call another live gateway request) instead of failing closed after one attempt, eventually crashing on an uncaught RecursionError once the call stack was exhausted. Added an explicit is_retry: bool = False parameter that tracks retry state independently of the exception's text. It (not repair_error) now gates both the prompt-injection branch -- falling back to a generic message when repair_error is empty -- and the except clause's retry-vs-fail-closed decision, and is threaded through as is_retry=True on the recursive call. Verified genuine RED with a bounded-recursion regression test (test_call_llm_fails_closed_after_a_repeated_empty_message_transport_error, which raises a diagnostic AssertionError if call_llm retries more than once instead of letting it recurse to CPython's own limit) before this fix, GREEN after -- paired with test_call_llm_repairs_once_after_an_empty_message_transport_error_then_succeeds for the happy-path case. Full suite: 2254 passed, 1 skipped, 21 subtests. noema_review_gate.py still at 100% line/branch coverage; 100% docstring coverage. Repo-wide 99% remains the same pre-existing gap tracked by #1567, unrelated to this diff. Updated docs/product-technical-gap-baseline.md and CHANGELOG.md with this fourth fix round. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Y6UJHYbfbGdHfYPjgbVhAr --- CHANGELOG.md | 16 ++++++ docs/product-technical-gap-baseline.md | 22 +++++++- scripts/ci/noema_review_gate.py | 16 +++++- tests/test_noema_review_gate.py | 78 ++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad345a19ca..8e8633515b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,22 @@ Semantic Versioning where the repository publishes a release. line/branch coverage. (A separate, pre-existing SIGPIPE flake in `tests/test_opencode_required_verdict_regression.py`, unrelated to this file, was also reproduced and fixed in its own PR during this verification.) + Devin Review then found a fourth, distinct bug in the fix itself: gating the + retry-vs-fail-closed decision on `repair_error`'s truthiness conflated "is + this the second attempt" with "does the caught exception have display + text" — several transport exceptions (a bare `OSError()`/`TimeoutError()`, + or an `http.client.HTTPException` raised with no message) stringify to an + empty string, so an empty-message failure on the first attempt would keep + `repair_error` falsy on the recursive call too and retry unboundedly instead + of failing closed after one attempt. Added an explicit `is_retry: bool` + parameter to track retry state independently of the exception's text, used + it (not `repair_error`) as the sole gate in both the prompt-injection branch + and the except clause, and threaded it through the recursive call. Verified + genuine RED with a bounded-recursion regression test (an `AssertionError` + fires if `call_llm` retries more than once, rather than letting it recurse + to CPython's own limit) before this fourth fix, GREEN after. Full suite 2254 + passed, 1 skipped, 21 subtests; `noema_review_gate.py` still at 100% + line/branch coverage, 100% docstrings. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 82f3cafe8b..9367d54f67 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2386,9 +2386,29 @@ regression pair (`test_call_llm_repairs_once_after_a_transport_error_then_succee `test_call_llm_repairs_once_after_a_socket_timeout_then_succeeds` / `test_call_llm_fails_closed_after_a_repeated_socket_timeout` for a raw `TimeoutError` reaching `opener.open()` directly) -- each verified genuinely RED against the pre-fix boundary before being -folded in, never transferred from an earlier case as substitute proof. Full suite: 2250 passed, 1 +folded in, never transferred from an earlier case as substitute proof. Full suite: 2252 passed, 1 skipped, 21 subtests; `noema_review_gate.py` at 100% line/branch coverage; 100% docstring coverage. +**Fix, round 3 (Devin Review again, same `#1566`)**: a fourth, distinct bug in the fix itself -- +gating the retry-vs-fail-closed decision on `repair_error`'s truthiness conflated "is this the +second attempt" with "does the caught exception have display text". Several transport exceptions +(a bare `OSError()`/`TimeoutError()`, or an `http.client.HTTPException` raised with no message) all +stringify to `''`, so an empty-message failure on the *first* attempt would leave `repair_error` +falsy on the recursive call too -- the retry-state signal was lost, and `call_llm` would retry +unboundedly (each recursive call itself another live-gateway request) rather than failing closed +after one attempt, eventually crashing on an uncaught `RecursionError` once the interpreter's call +stack was exhausted. Added an explicit `is_retry: bool = False` parameter to track retry state +independently of the exception's text; it (not `repair_error`) now gates both the prompt-injection +branch (falling back to a generic message when `repair_error` is empty) and the except clause's +retry-vs-fail-closed decision, and is threaded through as `is_retry=True` on the recursive call. +Verified genuine RED with a bounded-recursion regression test +(`test_call_llm_fails_closed_after_a_repeated_empty_message_transport_error`, which raises a +diagnostic `AssertionError` if `call_llm` retries more than once instead of letting it recurse to +CPython's own limit) before this fourth fix, GREEN after -- paired with +`test_call_llm_repairs_once_after_an_empty_message_transport_error_then_succeeds` for the +happy-path case. Full suite: 2254 passed, 1 skipped, 21 subtests; `noema_review_gate.py` still at +100% line/branch coverage, 100% docstring coverage. + **Owner**: this repo (`ContextualWisdomLab/.github`), `scripts/ci/noema_review_gate.py`. **Status**: fixed on `ContextualWisdomLab/.github#1566` (branch `fix/noema-review-transport-error-retry`), pending required checks and final review. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index f1ca1feb98..b77ed11c03 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -923,6 +923,7 @@ def call_llm( review_context: str = "", changed_paths: Sequence[str] = (), repair_error: str = "", + is_retry: bool = False, ) -> dict[str, Any]: """Call the configured OpenAI-compatible LLM endpoint for a review verdict. @@ -936,6 +937,13 @@ def call_llm( discard anyway once this function returns. See ``fetch_pr`` for the live lookup and ``StaleHeadDuringRepairRetryError`` for how that stale condition is reported distinctly to the caller. + + ``is_retry`` tracks retry state independently of ``repair_error``'s text: + several transport exceptions (a bare ``OSError``/``TimeoutError`` or + ``http.client.HTTPException`` raised with no message) stringify to an + empty string, so gating on ``repair_error``'s truthiness alone would let + an empty-message failure retry unboundedly instead of failing closed + after one attempt. """ api_url = os.environ.get("NOEMA_LLM_API_URL", "").strip() api_key = os.environ.get("NOEMA_LLM_API_KEY", "").strip() @@ -995,10 +1003,11 @@ def call_llm( "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", *( [ - f"Your prior verdict was rejected by the trusted validator: {repair_error}", + "Your prior verdict was rejected by the trusted validator: " + f"{repair_error or 'no diagnostic message was available'}", "Return one corrected JSON verdict using only exact changed-side locations from the supplied diff.", ] - if repair_error + if is_retry else [] ), f"Repository: {repo}", @@ -1062,7 +1071,7 @@ def call_llm( raise RuntimeError("Noema LLM request_changes response did not contain a substantive finding") validate_substantive_verdict(verdict, diff, changed_paths) except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: - if repair_error: + if is_retry: if isinstance(exc, RuntimeError): raise raise RuntimeError(str(exc)) from exc @@ -1080,6 +1089,7 @@ def call_llm( review_context, changed_paths, str(exc), + is_retry=True, ) return verdict diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 46979cb3c0..43aaf46e81 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1576,6 +1576,84 @@ def open_response(_opener, request, **_kwargs): assert len(open_calls) == 2 +def test_call_llm_repairs_once_after_an_empty_message_transport_error_then_succeeds(monkeypatch): + """A transport exception whose ``str()`` is empty (a bare ``OSError()``/ + ``TimeoutError()``, or an ``http.client.HTTPException`` raised with no + message -- all of these stringify to ``''`` in practice) must still get + exactly one repair retry, the same as any other transport failure. + + Devin Review on #1566: gating the retry-vs-fail-closed decision on + ``repair_error``'s truthiness conflated "is this the second attempt" + with "does the caught exception have display text" -- an empty-message + failure on the first attempt would keep ``repair_error`` falsy on the + recursive call too, so the retry state was lost. ``is_retry`` now tracks + that state explicitly and independently of the exception's text.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + attempts = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return json.dumps( + { + "choices": [ + { + "message": { + "content": json.dumps( + {"decision": "comment", "summary": "Recovered", "findings": []} + ) + } + } + ] + } + ).encode() + + def open_response(_opener, request, **_kwargs): + attempts.append(request) + if len(attempts) == 1: + raise OSError() + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Recovered" + assert len(attempts) == 2 + + +def test_call_llm_fails_closed_after_a_repeated_empty_message_transport_error(monkeypatch): + """Two consecutive empty-message transport failures must still fail + closed after exactly one repair retry, never retry unboundedly. + + Bounds the fixture at 6 open() calls so a regression that reintroduces + unbounded recursion fails this test fast with a clear AssertionError + instead of recursing until CPython's own recursion limit.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + open_calls = [] + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + if len(open_calls) > 5: + raise AssertionError("call_llm retried more than once on an empty-message transport error") + raise OSError() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + + with pytest.raises(RuntimeError): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + assert len(open_calls) == 2 + + @pytest.mark.parametrize("choices", [{"a": 1}, 5]) def test_call_llm_fails_closed_on_wrong_shaped_gateway_choices(monkeypatch, choices): """A malformed (non-list) choices field surfaces through call_llm's