fix(noema): fail closed on a transport error instead of crashing the required check

CHANGELOG.md

@@ -5,6 +5,60 @@ 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.) 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.)
+  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

docs/product-technical-gap-baseline.md

@@ -2344,6 +2344,84 @@ 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: 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.
+
+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는 아래 순서를 유지한다.

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
@@ -922,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.
 
@@ -935,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()
@@ -994,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}",
@@ -1030,9 +1040,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,9 +1070,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 as exc:
-        if repair_error:
-            raise
+    except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc:
+        if is_retry:
+            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."
@@ -1077,6 +1089,7 @@ def call_llm(
             review_context,
             changed_paths,
             str(exc),
+            is_retry=True,
         )
     return verdict