fix(review-gate-sweep): classify a truncated GitHub read as degraded, not as a stranded PR (BLO-35151)

.github/scripts/sweep-stalled-ally-reviews.py

@@ -44,11 +44,13 @@
 """
 
 import argparse
+import http.client
 import json
 import os
 import re
 import sys
 import time
+import traceback
 import urllib.error
 import urllib.request
 from datetime import datetime, timezone
@@ -1486,13 +1488,27 @@ def main(argv=None):
         sys.exit(EXIT_SWEEP_DEGRADED)
 
 
-if __name__ == "__main__":
+def _dispatch():
+    """Run `main()` under the abort-vs-alarm exit-code policy.
+
+    A function rather than a bare `if __name__ == "__main__"` body so the arms
+    below are reachable from the test suite. They were not, and that is why
+    the `http.client` hole sat open: every arm here is a guard whose whole
+    purpose is to fire on a path nothing else exercises, and an unreachable
+    guard is a comment. Tests assert each arm's exit code by calling this.
+    """
     # Every arm here exits EXIT_SWEEP_DEGRADED, never EXIT_ALARM: reaching
     # this handler means the sweep aborted outright (typically on the initial
     # open-PR list, before any PR was evaluated), so nothing is known about
     # whether a PR is stranded. Reporting that as the stranded-PR alarm would
     # send a human looking for a PR to review when the actual fault is that
     # the reconciler could not talk to GitHub.
+    #
+    # review-gate-sweep.yml fails the job on ANY non-zero exit, so choosing
+    # EXIT_SWEEP_DEGRADED over EXIT_ALARM does not turn a red run green. What
+    # it buys is that the red is correctly DIAGNOSED -- "could not talk to
+    # GitHub" rather than "go review a stranded PR". Judge these arms on the
+    # log line, not on the workflow conclusion.
     try:
         main()
     except RateLimitExhausted as error:
@@ -1507,6 +1523,32 @@ def main(argv=None):
         # shadow the status-code message above.
         print("GitHub API request failed (transport): %s" % error.reason, file=sys.stderr)
         sys.exit(EXIT_SWEEP_DEGRADED)
+    except http.client.HTTPException as error:
+        # A truncated or malformed response body raises from `http.client`,
+        # whose exception tree hangs off Exception and NOT off OSError:
+        #
+        #     IncompleteRead -> HTTPException -> Exception
+        #     URLError       -> OSError
+        #
+        # So none of the arms above and none below catch it, and uncaught it
+        # exits 1 == EXIT_ALARM -- a truncated GitHub page reporting itself as
+        # "a PR is stranded, go review it". Measured live on run 35605218498
+        # (2026-09-21T13:22Z): `IncompleteRead(703602 bytes read, 68373 more
+        # expected)` while paginating the open-PR list, which a human then had
+        # to read a traceback to tell apart from a real alarm. Exactly the
+        # hazard the OSError arm below already documents, arriving through the
+        # one door that arm cannot cover (BLO-35151).
+        #
+        # Catch the HTTPException BASE, not IncompleteRead: BadStatusLine and
+        # LineTooLong are siblings with identical consequences, and naming
+        # only the subclass we happened to observe would leave the same hole.
+        #
+        # No retry here on purpose. The sweep runs hourly and is idempotent,
+        # so the schedule already supplies the retry; the 13:22Z truncation
+        # self-healed at 14:24Z on the same commit. Retrying in-process would
+        # add a failure mode to buy back an hour that costs nothing.
+        print("GitHub API request failed (malformed or truncated HTTP response): %r" % error, file=sys.stderr)
+        sys.exit(EXIT_SWEEP_DEGRADED)
     except OSError as error:
         # A REQUEST_TIMEOUT_SECONDS expiry during the response *read* raises a
         # bare TimeoutError (== socket.timeout), which is an OSError but NOT a
@@ -1518,3 +1560,33 @@ def main(argv=None):
         # itself an OSError, so the arms above still take precedence.
         print("GitHub API request failed (socket): %r" % error, file=sys.stderr)
         sys.exit(EXIT_SWEEP_DEGRADED)
+    except Exception:
+        # Terminal arm: any exception class not enumerated above (e.g. a
+        # json.JSONDecodeError == ValueError from _request()'s json.loads on a
+        # 200 carrying a non-JSON proxy/WAF page during the unisolated open-PR
+        # pagination) would otherwise escape and exit 1 == EXIT_ALARM. Anything
+        # reaching here by definition never finished reading the PR list, so it
+        # is degraded, not an alarm. `Exception`, NOT `BaseException`: main()'s
+        # deliberate sys.exit(EXIT_ALARM) raises SystemExit, which must pass
+        # through untouched (BLO-35151).
+        print("GitHub API sweep crashed before completing: %s" % traceback.format_exc(), file=sys.stderr)
+        sys.exit(EXIT_SWEEP_DEGRADED)
+
+
+def run_cli():
+    """Run `_dispatch()` under the abort-vs-alarm exit-code policy; see its docstring."""
+    try:
+        _dispatch()
+    except Exception:
+        # The arm BODIES in _dispatch() are siblings of its terminal arm, not
+        # inside its try: an exception raised while REPORTING a failure (live
+        # case: `error.read()` on an HTTPError re-raising IncompleteRead off
+        # the socket) would escape and exit 1 == EXIT_ALARM. `Exception`, not
+        # `BaseException`, so each arm's own sys.exit(SystemExit) passes
+        # through untouched (BLO-35151).
+        print("GitHub API sweep crashed while reporting a failure: %s" % traceback.format_exc(), file=sys.stderr)
+        sys.exit(EXIT_SWEEP_DEGRADED)
+
+
+if __name__ == "__main__":
+    run_cli()

.github/scripts/test_sweep_stalled_ally_reviews.py

@@ -7,6 +7,7 @@
 """
 
 import contextlib
+import http.client
 import importlib.util
 import io
 import os
@@ -1966,5 +1967,117 @@ def test_a_clean_run_writes_no_guard_section(self):
         self.assertNotIn("withheld by the pre-write guard", summary)
 
 
+class TestRunCliExitCodePolicy(unittest.TestCase):
+    """Every abort arm in run_cli() must exit DEGRADED, never ALARM.
+
+    EXIT_ALARM is the "a PR is stranded, go review it" signal. CPython exits
+    1 on an uncaught exception and EXIT_ALARM is 1, so ANY exception class
+    that escapes run_cli() silently becomes a false alarm -- it sends a human
+    to look for review work that does not exist. That is not hypothetical:
+    `http.client.IncompleteRead` escaped every arm and did exactly this on
+    run 35605218498 (BLO-35151).
+
+    These tests exist because the arms used to live in a bare
+    `if __name__ == "__main__"` block, where no test could reach them.
+    """
+
+    def _exit_code_for(self, error):
+        """Raise `error` out of main() and return run_cli()'s exit code."""
+        original_main = sweep.main
+        sweep.main = lambda: (_ for _ in ()).throw(error)
+        stderr = io.StringIO()
+        try:
+            with contextlib.redirect_stderr(stderr):
+                with self.assertRaises(SystemExit) as caught:
+                    sweep.run_cli()
+        finally:
+            sweep.main = original_main
+        return caught.exception.code, stderr.getvalue()
+
+    def test_alarm_and_degraded_are_distinct_nonzero_codes(self):
+        """The guard below is meaningless if these two ever collide."""
+        self.assertEqual(sweep.EXIT_ALARM, 1)
+        self.assertNotEqual(sweep.EXIT_SWEEP_DEGRADED, sweep.EXIT_ALARM)
+
+    def test_incomplete_read_is_degraded_not_alarm(self):
+        code, err = self._exit_code_for(http.client.IncompleteRead(b"partial", 68373))
+        self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+        self.assertIn("truncated HTTP response", err)
+
+    def test_bad_status_line_is_degraded_not_alarm(self):
+        """The arm catches the HTTPException BASE, not just IncompleteRead.
+
+        Naming only the subclass we happened to observe would leave the same
+        hole open for its siblings.
+        """
+        code, err = self._exit_code_for(http.client.BadStatusLine("garbage"))
+        self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+        self.assertIn("truncated HTTP response", err)
+
+    def test_incomplete_read_is_not_an_oserror(self):
+        """Pins WHY the pre-existing arms could not catch it.
+
+        If a future Python made HTTPException an OSError subclass this test
+        fails, flagging that the dedicated arm is now redundant rather than
+        letting it rot as unexplained duplication.
+        """
+        self.assertFalse(issubclass(http.client.HTTPException, OSError))
+        self.assertTrue(issubclass(urllib.error.URLError, OSError))
+
+    def test_timeout_error_is_degraded_not_alarm(self):
+        code, err = self._exit_code_for(TimeoutError("read timed out"))
+        self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+        self.assertIn("socket", err)
+
+    def test_url_error_keeps_its_transport_message(self):
+        """Arm precedence is unchanged by the insertion above it."""
+        code, err = self._exit_code_for(urllib.error.URLError("dns failure"))
+        self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+        self.assertIn("transport", err)
+
+    def test_rate_limit_keeps_its_own_message(self):
+        code, err = self._exit_code_for(sweep.RateLimitExhausted("budget spent"))
+        self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+        self.assertIn("rate limit exhausted", err)
+
+    def test_unenumerated_exception_classes_are_degraded_not_alarm(self):
+        """Pins the class-level invariant the docstring states, not just the
+        enumerated arms: ANY escaping exception is degraded, never alarm."""
+        import json
+        for error in (
+            json.JSONDecodeError("Expecting value", "<html>", 0),
+            ValueError("unrelated"),
+            KeyError("missing"),
+            RuntimeError("unrelated"),
+        ):
+            with self.subTest(error=type(error).__name__):
+                code, err = self._exit_code_for(error)
+                self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+                self.assertIn("crashed before completing", err)
+
+    def test_deliberate_alarm_exit_passes_through_terminal_arm(self):
+        """SystemExit is a BaseException; the terminal `except Exception`
+        must not reclassify main()'s own sys.exit(EXIT_ALARM) to degraded."""
+        code, _ = self._exit_code_for(SystemExit(sweep.EXIT_ALARM))
+        self.assertEqual(code, sweep.EXIT_ALARM)
+
+
+    def test_exception_raised_while_reporting_a_failure_is_degraded_not_alarm(self):
+        """Arm bodies are siblings of the terminal arm, not inside its try.
+        Live case: HTTPError.read() re-raising IncompleteRead off the socket
+        while the HTTPError arm formats its message (BLO-35151)."""
+        class _TruncatedBody:
+            def read(self):
+                raise http.client.IncompleteRead(b"partial", 100)
+
+            def close(self):
+                pass
+
+        error = urllib.error.HTTPError("https://api.github.com/x", 500, "boom", {}, _TruncatedBody())
+        code, err = self._exit_code_for(error)
+        self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED)
+        self.assertIn("while reporting a failure", err)
+
+
 if __name__ == "__main__":
     unittest.main()