From f8240a8f8425daa1145aab438108912a2a8b9633 Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 21 Sep 2026 15:39:56 +0000 Subject: [PATCH 1/4] fix(review-gate-sweep): classify a truncated GitHub read as degraded, not as a stranded PR (BLO-35151) `http.client.IncompleteRead` hangs off HTTPException, not OSError, so it escaped every arm of the abort handler. Uncaught it exits 1 == EXIT_ALARM, which is the "a PR is stranded, go review it" signal -- so a truncated open-PR page sent a human looking for review work that did not exist. Measured on run 35605218498 (2026-09-21T13:22Z, master 702cb7d7): IncompleteRead(703602 bytes read, 68373 more expected). This is the hazard the existing OSError arm already documents for TimeoutError, arriving through the one door that arm cannot cover. Catch the HTTPException base so BadStatusLine/LineTooLong take the same arm. No retry: the sweep is hourly and idempotent, and the 13:22Z truncation self-healed at 14:24Z on the same commit. The schedule is the retry. Scope honestly -- the workflow fails the job on any non-zero exit, so this does not turn a red run green. It makes the red correctly diagnosed. Extract the handler into run_cli() so the arms are reachable from tests. None of them were, which is why this hole sat open. All three guard properties mutation-tested (delete arm / narrow base to IncompleteRead / exit EXIT_ALARM) -- each turns the suite red. 88 -> 95 tests. Co-Authored-By: Claude --- .github/scripts/sweep-stalled-ally-reviews.py | 47 +++++++++++- .../test_sweep_stalled_ally_reviews.py | 75 +++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index d449fac67784..64fa6c776e95 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -44,6 +44,7 @@ """ import argparse +import http.client import json import os import re @@ -1486,13 +1487,27 @@ def main(argv=None): sys.exit(EXIT_SWEEP_DEGRADED) -if __name__ == "__main__": +def run_cli(): + """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 +1522,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 (truncated 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 +1559,7 @@ 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) + + +if __name__ == "__main__": + run_cli() diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index d63c6fa96d6d..5a1a7e68c235 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.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,79 @@ 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 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 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) + + if __name__ == "__main__": unittest.main() From af074326a27720692741b5cfaecfe85e66d0e816 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Wed, 23 Sep 2026 03:27:14 +0000 Subject: [PATCH 2/4] fix(review-gate-sweep): close the exit-code class with a terminal arm Ally's Important on #1980: run_cli() enumerated observed exception classes (RateLimitExhausted, HTTPError, URLError, HTTPException, OSError), so any other class escaping main() still exited 1 == EXIT_ALARM. The reachable case is json.JSONDecodeError (a ValueError) from _request()'s json.loads on a 200 carrying a non-JSON proxy/WAF page during the unisolated open-PR pagination, which reported "go review a stranded PR" through a second door. Append a terminal `except Exception` (not BaseException, so main()'s own sys.exit(EXIT_ALARM) still passes through) that prints the traceback and exits EXIT_SWEEP_DEGRADED. Also widen the HTTPException log line to "malformed or truncated HTTP response" since the arm covers BadStatusLine and friends, and add two tests: a table over unrelated exception classes pinning the class-level invariant, and a SystemExit pass-through guard. Controls: `python3 -m unittest discover -s .github/scripts -p 'test_sweep_*.py'` 97/97 OK. Deleting the terminal arm errors the four subTests of test_unenumerated_exception_classes_are_degraded_not_alarm; changing it to `except BaseException` fails test_deliberate_alarm_exit_passes_through_terminal_arm. Restored, green. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Omar Ramadan --- .github/scripts/sweep-stalled-ally-reviews.py | 14 ++++++++++- .../test_sweep_stalled_ally_reviews.py | 25 +++++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index 64fa6c776e95..2aa180be3d30 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -50,6 +50,7 @@ import re import sys import time +import traceback import urllib.error import urllib.request from datetime import datetime, timezone @@ -1546,7 +1547,7 @@ def run_cli(): # 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 (truncated response): %r" % error, file=sys.stderr) + 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 @@ -1559,6 +1560,17 @@ def run_cli(): # 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) if __name__ == "__main__": diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index 5a1a7e68c235..922f682a820c 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -2002,7 +2002,7 @@ def test_alarm_and_degraded_are_distinct_nonzero_codes(self): 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 response", err) + 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. @@ -2012,7 +2012,7 @@ def test_bad_status_line_is_degraded_not_alarm(self): """ code, err = self._exit_code_for(http.client.BadStatusLine("garbage")) self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) - self.assertIn("truncated response", err) + 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. @@ -2040,6 +2040,27 @@ def test_rate_limit_keeps_its_own_message(self): 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", "", 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", 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) + if __name__ == "__main__": unittest.main() From 15aea8fd6c9674120c5d54a5b59db39e76d590c3 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Wed, 23 Sep 2026 09:03:01 +0000 Subject: [PATCH 3/4] fix(review-gate-sweep): close the exit-code class over the arm bodies too Ally's Important at d3e21c1f: the terminal arm in run_cli() only closes the class for exceptions raised by main(); the arm bodies are siblings of it, so an exception raised while REPORTING a failure (live case: error.read() on an HTTPError re-raising IncompleteRead off the socket) escaped and exited 1 == EXIT_ALARM, the exact BLO-35151 false alarm re-entering through the error path. Close it structurally, the way the last round closed the sibling case: the existing try/except chain is now _dispatch(), and run_cli() wraps it in one outer `except Exception` that exits EXIT_SWEEP_DEGRADED. `Exception`, not `BaseException`, so every arm's own sys.exit(SystemExit) still passes through. Test: TestRunCliExitCodePolicy gains a case that raises from an arm BODY (an HTTPError whose fp.read() raises IncompleteRead), which no existing test could reach because they all raise out of main(). Controls (python3 -m unittest discover -s .github/scripts -p 'test_sweep_*.py'): positive 98 tests OK; with the wrapper removed the new test errors with an uncaught IncompleteRead; with the wrapper widened to BaseException test_deliberate_alarm_exit_passes_through_terminal_arm fails 2 != 1. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Omar Ramadan --- .github/scripts/sweep-stalled-ally-reviews.py | 17 ++++++++++++++++- .../scripts/test_sweep_stalled_ally_reviews.py | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index 2aa180be3d30..055a2d8ad5ed 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -1488,7 +1488,7 @@ def main(argv=None): sys.exit(EXIT_SWEEP_DEGRADED) -def run_cli(): +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 @@ -1573,5 +1573,20 @@ def run_cli(): 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() diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index 922f682a820c..45c4b5469752 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -2062,5 +2062,22 @@ def test_deliberate_alarm_exit_passes_through_terminal_arm(self): 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() From 75c5d870679f2ce99771b254e4d5f60cd66e683e Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Thu, 24 Sep 2026 07:12:48 +0000 Subject: [PATCH 4/4] test(sweep): pin the terminal arm to its own log line (BLO-35151) test_unenumerated_exception_classes_are_degraded_not_alarm asserted the substring "crashed", which both _dispatch()'s terminal arm ("crashed before completing") and run_cli()'s catch-all ("crashed while reporting a failure") print. Deleting the terminal arm left the suite green, because run_cli() caught the escaped error with the same exit code. Assert the terminal arm's exact phrase: the suite stays green, and the terminal-arm deletion goes back to FAILED (failures=4). Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/test_sweep_stalled_ally_reviews.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index 45c4b5469752..0ac949b7ad9a 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -2053,7 +2053,7 @@ def test_unenumerated_exception_classes_are_degraded_not_alarm(self): with self.subTest(error=type(error).__name__): code, err = self._exit_code_for(error) self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) - self.assertIn("crashed", err) + self.assertIn("crashed before completing", err) def test_deliberate_alarm_exit_passes_through_terminal_arm(self): """SystemExit is a BaseException; the terminal `except Exception`