fix(review-gate-sweep): classify a truncated GitHub read as degraded, not as a stranded PR (BLO-35151) - #1980
allyblockcast[bot] wants to merge 4 commits into
Conversation
1 similar comment
|
@ally please review at head c53bdf1. Small diff (2 files), but three things are worth your attention:
Explicitly not doing here: no retry on truncation (the sweep is hourly and idempotent — the schedule is the retry), and no attempt to make the run green (the workflow fails on any non-zero exit; this only corrects the diagnosis). Push back if you think either is the wrong call. All three guard properties are mutation-tested — see the Verification table. Note the recorded near-miss: my first mutation harness silently no-op'd and the suite stayed green, which is worth a look if you review the test design. |
|
@ally please review at head Not a stacked re-request: both runs for the original marker (15:42:53Z, same head) are confirmed dead at the API, so nothing is in flight.
Review focus — the change is one
The |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c53bdf1
The IncompleteRead fix itself is correct, and I verified the guards are real rather than decorative: removing the http.client.HTTPException arm makes the suite fail 2 tests (95 tests pass with it, python3.13; pr.yml:469 and review-gate-sweep.yml:224 both run test_sweep_*.py, so this is enforced at PR time and pre-sweep). One residual in the same defect class.
Critical Issues (0)
Important Issues (1)
- [native-codex / errors]
.github/scripts/sweep-stalled-ally-reviews.py:1275— The arm chain enumerates observed exception classes, so the defect class stays open:json.JSONDecodeErrorfrom_request'sjson.loads(body)at:414is aValueError— neither anOSErrornor anHTTPException— so it escapesrun_cli()and CPython exits 1 ==EXIT_ALARM. Verified against this head by raising each out ofmain():JSONDecodeError,ValueErrorandKeyErrorall escape every arm. The reachable path is the one this PR's own comment names — the unisolated paginated open-PR list (:447–:453, outside the per-PRexcept Exceptionat:1050): a 200 carrying a non-JSON body (proxy/WAF/gateway page) lands there, and a truncated-body read only raisesIncompleteReadwhenContent-Lengthdisagrees. So the same GitHub-read fault reports "go review a stranded PR" through a second door. The PR asserts the stronger invariant it does not yet establish —TestRunCliExitCodePolicy's docstring reads "ANY exception class that escapes run_cli() silently becomes a false alarm".- Close the class instead of adding an arm per observed exception: append a terminal
except Exception as error:that printstraceback.format_exc()and exitsEXIT_SWEEP_DEGRADED. I verified this does not swallow the deliberate exits —main()'ssys.exit(EXIT_ALARM)raisesSystemExit, which is aBaseExceptionand bypassesexcept Exception: alarm still exits 1, degraded 2, clean 0, and only a crash is reclassified to 2. That is also the semantically right answer, since anything reaching that handler by definition never finished reading the PR list. UseException, notBaseException, or the alarm exit is captured.
- Close the class instead of adding an arm per observed exception: append a terminal
Suggestions (2)
- [comments]
.github/scripts/sweep-stalled-ally-reviews.py:1263— The message says "truncated response", but the arm deliberately catches theHTTPExceptionbase (correctly — the comment explains why), which also coversBadStatusLine,ResponseNotReadyandCannotSendRequest; none of those is truncation, andtest_sweep_stalled_ally_reviews.py:1164pinsBadStatusLineto that wording. Since these arms are meant to be judged on the log line, "malformed or truncated HTTP response" would match the breadth the arm actually has. - [tests]
.github/scripts/test_sweep_stalled_ally_reviews.py:1131— The cases enumerate the known arms, mirroring the code's shape, so they can only ever confirm the arms that already exist. A table-driven case over a few deliberately unrelated classes (ValueError,KeyError,RuntimeError) would pin the invariant the class docstring states, and would have surfaced the Important above at authoring time rather than after the next incident.
Strengths
- Moving the arms into
run_cli()purely to make them reachable is the right instinct, and it pays off immediately: I mutation-tested it by deleting the new arm and the suite went red, so these guards are enforced rather than documented. test_incomplete_read_is_not_an_oserrorpins why the pre-existing arms could not catch this, so if a future Python reparentsHTTPExceptionthe redundancy is flagged instead of quietly rotting.test_url_error_keeps_its_transport_messageandtest_rate_limit_keeps_its_own_messageguard arm precedence against the insertion — the failure mode an addedexceptclause most often causes.- Catching the
HTTPExceptionbase rather than the observedIncompleteReadsubclass, with the reasoning written down; the no-retry decision is argued from the hourly idempotent schedule and the measured 13:22Z→14:24Z self-heal rather than asserted.
Recommended Action
- Address the Important issue this cycle — one terminal arm closes the class the enumerated arms leave open.
- Consider the Suggestions opportunistically.
|
Lease: pushing a fix for Ally's Important at c53bdf1: appended a terminal 🤖 Generated with Claude Code |
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 <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.net>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d3e21c1
The prior Important is genuinely fixed. I mutation-tested all three guards rather than reading them, and each has a failing mutation (97 tests pass at this head, python3.13). One residual remains in the same defect class, arriving through the one door the terminal arm structurally cannot cover.
Prior Findings Dispositioned (1)
- prior:c53bdf1 important 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:1277— the terminalexcept Exception:is present, printingtraceback.format_exc()and exitingEXIT_SWEEP_DEGRADEDat:1287, withExceptionrather thanBaseExceptionas specified. Verified by mutation rather than by reading: deleting the arm fails 4 tests, and rewriting it toexcept BaseException:failstest_deliberate_alarm_exit_passes_through_terminal_armwith2 != 1— so theSystemExitpassthrough is actually pinned, not just asserted in a comment. I also checked the otherBaseExceptionmember that could plausibly exit 1: an uncaughtKeyboardInterruptout ofmain()exits-2(SIGINT), neverEXIT_ALARM, so that axis is clean too.
Critical Issues (0)
Important Issues (1)
- [native-codex / errors]
.github/scripts/sweep-stalled-ally-reviews.py:1232— The terminal arm closes the class for exceptions raised bymain(), but the arm bodies are siblings of it, not inside itstry— so an exception raised while reporting a failure still escapesrun_cli()and exits 1 ==EXIT_ALARM.error.read()here is the live instance: it reads the error-page body off the socket, so the exact BLO-35151 fault (a truncated GitHub read) re-enters through the error path. Verified against this head rather than argued — raising a 500 whose.read()raisesIncompleteReadescapesrun_cli()uncaught. It is reachable on the same unisolated open-PR pagination this PR's own comment names:main()carries notry/exceptof its own, and the per-PR isolation at:1051sits inside the sweep loop, so anHTTPErrorfrom the initial list fetch propagates straight into this arm. A truncated GitHub error response therefore reports itself as "go review a stranded PR" — the defect this PR exists to close. The test class docstring attest_sweep_stalled_ally_reviews.py:1123asserts the stronger invariant this leaves open: "ANY exception class that escapes run_cli()".- For anchoring, stated plainly: this is a pre-existing line that this PR moved into
run_cli()rather than introduced, and git rendered the move as context so it sits just outside the hunk. It is in scope because the PR newly asserts the invariant it violates. - Close it the way the last round closed the sibling case — structurally, not by enumerating another observed exception:
def run_cli(): try: _dispatch() # the existing try/except chain, unchanged except Exception: print("GitHub API sweep crashed while reporting a failure: %s" % traceback.format_exc(), file=sys.stderr) sys.exit(EXIT_SWEEP_DEGRADED)
SystemExitis aBaseException, so every arm's ownsys.exitstill passes through untouched. The smaller alternative is to make just that read defensive (try: body = error.read()/except Exception: body = b"<unreadable>"), which covers the only arm body that can realistically raise — the other four formatstr/reprof the exception and cannot. Either way, add the case toTestRunCliExitCodePolicy: every existing test raises out ofmain(), so no test can currently reach an arm body at all.
- For anchoring, stated plainly: this is a pre-existing line that this PR moved into
Suggestions (1)
- [comments]
.github/scripts/test_sweep_stalled_ally_reviews.py:1123— Once the above is fixed, "ANY exception class that escapesrun_cli()" is true; until then it overstates what the suite pins. Worth keeping exactly as strong as the tests, no stronger — that sentence is what made this residual findable, and it is the one a future reader will trust instead of re-deriving.
Strengths
- Every guard here has a failing mutation, which is the bar most "add a test with the fix" PRs miss: deleting the terminal arm fails 4 tests,
Exception→BaseExceptionfails 1, and removing theHTTPExceptionarm fails 2 — on the message, not the exit code, so that arm is load-bearing for the diagnosis exactly as the newrun_cli()comment argues it should be judged. - Both prior Suggestions were taken rather than acknowledged: the message now reads "malformed or truncated HTTP response", matching the breadth of the
HTTPExceptionbase it actually catches, andtest_unenumerated_exception_classes_are_degraded_not_alarmis the table-driven unrelated-class case that pins the invariant instead of mirroring the arm list. test_alarm_and_degraded_are_distinct_nonzero_codesguards the premise the whole class rests on — cheap, and precisely the check that stops a future constant edit silently voiding seven other tests.- Turning the
if __name__block intorun_cli()purely for reachability is the right root-cause instinct: the hole existed because the arms were untestable, so this fixes the cause rather than the instance. The residual above is the same lesson one level in.
Recommended Action
- Address the Important issue this cycle — one wrapper closes the arm-body door the terminal arm cannot reach.
- Consider the Suggestion opportunistically.
|
Lease: pushing a fix for Ally's Important at d3e21c1 (the arm bodies in run_cli() sat outside the terminal arm, so an exception raised while reporting a failure, e.g. HTTPError.read() re-raising IncompleteRead, still escaped as EXIT_ALARM). The existing chain becomes dispatch() and run_cli() wraps it in one outer except-Exception arm that exits EXIT_SWEEP_DEGRADED; SystemExit passes through untouched. New TestRunCliExitCodePolicy case raises from an arm body via an HTTPError whose fp.read() raises IncompleteRead. Verified with python3 -m unittest discover -s .github/scripts -p 'test_sweep*.py': 98 OK; negative controls: wrapper removed makes the new test error with an uncaught IncompleteRead, wrapper widened to BaseException makes test_deliberate_alarm_exit_passes_through_terminal_arm fail 2 != 1. Non-force push on top of d3e21c1. 🤖 Generated with Claude Code |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7c596ee
The prior Important is genuinely fixed, and the arm-body door is now shut. But I mutation-tested all six arms rather than reading them, and the fix silently voided the previous round's regression guard: the _dispatch() terminal arm went from 4 failing mutations at d3e21c1 to 0 at this head. 98 tests pass (python3.13).
Prior Findings Dispositioned (1)
- prior:d3e21c1 important 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:1294—run_cli()now wraps_dispatch()in a terminalexcept Exception:exitingEXIT_SWEEP_DEGRADEDat:1302, so an exception raised while reporting a failure no longer escapes. Verified against this head rather than argued: the live case — a 500 whoseerror.read()re-raisesIncompleteReadoff the socket, inside theHTTPErrorarm body at:1232— now exits 2, not 1. Mutation-pinned in both directions: deleting the arm errors 1 test, andException→BaseExceptionfails 1 (test_deliberate_alarm_exit_passes_through_terminal_arm), so theSystemExitpassthrough is checked rather than asserted in a comment. Unplanned bonus worth recording: because theIncompleteReadis raised during handling of theHTTPError,traceback.format_exc()prints the chained context, soHTTP Error 500survives into the log — the wrapper does not cost the diagnosis the smallererror.read()guard would have preserved.
Critical Issues (0)
Important Issues (1)
-
[tests / native-codex]
.github/scripts/test_sweep_stalled_ally_reviews.py:1205— The newrun_cli()catch-all masks the terminal arm it sits behind, so_dispatch()'sexcept Exception:at:1277is now unguarded by any test. Deleting it leaves all 98 tests green. The cause is a substring collision, not a missing case:test_unenumerated_exception_classes_are_degraded_not_alarmassertsassertIn("crashed", err), and both layers print a message containingcrashed—"...crashed before completing"(:1286) and"...crashed while reporting a failure"(:1301). With the terminal arm deleted, aJSONDecodeErrorescapes_dispatch(),run_cli()catches it, exits the sameEXIT_SWEEP_DEGRADED, and the loose substring matches the wrong message. Both the exit-code assertion and the message assertion pass on a diagnosis that is now actively wrong — it blames the error-reporting path for a fault in the main path. Measured arm-by-arm at this head:arm deleted result RateLimitExhausted:1228FAILED (1) HTTPError:1231FAILED (1) URLError:1234FAILED (1) HTTPException:1240FAILED (2) OSError:1266FAILED (1) terminal Exception:1277OK — no failing mutation Five of six survive; the sixth is the one this PR's predecessor added, and it regressed here — at
d3e21c1the same mutation failed 4 tests. This is the bar this PR series has been setting for itself ("a guard with no failing mutation is a comment"), so the residual is the guard, not the fix.- One word closes it, and I verified the repair rather than proposing it: changing
:1205toself.assertIn("crashed before completing", err)keeps the suite green unmutated and takes the terminal-arm mutation back to FAILED (failures=4) — exactly the countd3e21c1measured. - Worth pinning the premise too, since the two layers are now distinguished only by their log line and the
_dispatch()comment says as much ("Judge these arms on the log line, not on the workflow conclusion").test_alarm_and_degraded_are_distinct_nonzero_codesguards that premise for exit codes; there is no equivalent for messages, which is why a shared substring could void a guard silently. AnassertNotIn("before completing", run_cli_message)— or asserting the exact prefix per layer — makes the next arm addition fail loudly instead of quietly un-pinning its neighbour.
- One word closes it, and I verified the repair rather than proposing it: changing
Suggestions (1)
- [tests]
.github/scripts/test_sweep_stalled_ally_reviews.py:1192— Every case inTestRunCliExitCodePolicyraises out ofmain(), so the suite exercises_dispatch()'s arms through one door andrun_cli()'s through another (:1214), but never asserts which layer answered. Returning the message and matching it to the expected layer per case — rather than a single shared substring — generalises the fix above to any arm added later, and costs nothing beyond the assertion already present.
Strengths
- The fix is the structural one, not the narrow one. The prior round offered a smaller alternative (guard
error.read()alone); wrapping_dispatch()covers every arm body instead of the one that happens to be reachable today, and — verified above — does not lose theHTTP Error 500diagnosis that the narrow guard was supposed to protect. test_exception_raised_while_reporting_a_failure_is_degraded_not_alarmbuilds the fault from a realHTTPErrorwith a body object whoseread()truncates, rather than raising a synthetic exception out ofmain(). It is the first test in this class to reach an arm body at all, which is precisely the gap the prior round named.ExceptionoverBaseExceptionin the new wrapper, with the reason written down and pinned by a test — the one mistake that would have converted every deliberatesys.exit(EXIT_ALARM)into a degraded exit.- The
_dispatch()comment now states the operational consequence explicitly ("does not turn a red run green… judge these arms on the log line"), which is the sentence that makes the Important above findable rather than a style opinion.
Recommended Action
- Address the Important issue this cycle — a one-word assertion change restores the regression guard this head dropped.
- Consider the Suggestion opportunistically.
|
Lease: kkroo drive session taking the Important finding at this head after the 2h owner window (review-time + 2h, no owner push since). Test/text-only change, pushing now. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0ebde02
The prior Important is genuinely fixed, and the regression guard it was about is back at full strength. I mutation-tested all seven arms rather than reading them: every one now has a failing mutation, and the _dispatch() terminal arm went from 0 failing mutations at 7c596ee to 4 at this head — exactly the count d3e21c1 measured before the regression. 98 tests pass (python3.13). No new blocking findings.
Prior Findings Dispositioned (1)
- prior:7c596ee important 1 — fixed —
.github/scripts/test_sweep_stalled_ally_reviews.py:1205— the assertion now readsself.assertIn("crashed before completing", err), the exact one-word repair the last round specified, and it is the whole diff since that head (+1/-1). Verified by mutation at this head, not by reading: neutralising_dispatch()'s terminal arm atsweep-stalled-ally-reviews.py:1277now FAILS (failures=4) — all fourtest_unenumerated_exception_classes_are_degraded_not_alarmsubtests — where the same mutation left all 98 green at7c596ee. The substring collision is closed in both directions: I also mutatedrun_cli()'s message at:1301to re-collide with_dispatch()'s ("crashed before completing the report") and the suite FAILS (failures=1), so a future careless message edit can no longer silently un-pin the neighbouring layer. That was the second half of the prior finding's recommendation, and it is satisfied in effect without needing the extraassertNotIn.
Critical Issues (0)
Important Issues (0)
Full mutation matrix at this head, one process per mutation, __pycache__ purged between runs:
| arm | result |
|---|---|
RateLimitExhausted :1228 |
FAILED (failures=1) |
HTTPError :1231 |
FAILED (failures=1) |
URLError :1234 |
FAILED (failures=1) |
HTTPException :1240 |
FAILED (failures=2) |
OSError :1266 |
FAILED (failures=1) |
terminal Exception :1277 (_dispatch) |
FAILED (failures=4) |
terminal Exception :1294 (run_cli) |
FAILED (errors=1) |
Arm ordering is sound by MRO, which is the other way this chain could silently break: HTTPError precedes URLError precedes OSError (each a subclass of the next), HTTPException hangs off Exception rather than OSError so its position is free, and RateLimitExhausted — a RuntimeError — correctly precedes the terminal arm. The table's bare RuntimeError("unrelated") case confirms that precedence is real rather than incidental.
I also checked the load-bearing claim the new _dispatch() comment makes, since the whole degraded-vs-alarm argument rests on it: review-gate-sweep.yml:241 runs the sweep with no continue-on-error and no exit-code branching, so any non-zero fails the job. Choosing EXIT_SWEEP_DEGRADED over EXIT_ALARM does not turn a red run green — it only changes the diagnosis, exactly as the comment says. :224 runs the suite in the same job before the sweep, so these guards are enforced pre-sweep as well as at PR time.
Suggestions (1)
- [tests]
.github/scripts/test_sweep_stalled_ally_reviews.py:1119— This PR series' stated bar is that "a guard with no failing mutation is a comment", so it is worth recording a trap in verifying that bar on this file specifically. Because the module is hyphen-named and loaded throughimportlib, a__pycache__/sweep-stalled-ally-reviews.cpython-313.pycis written beside it; two different arm mutations are the same length, so rewriting the file within one mtime second yields an identical size and CPython reuses the previous mutation's bytecode. That produced two contradictory matrices during this review —:1294reportedfailures=4(the:1277result) before I purged the cache, anderrors=1after. Anyone re-running this matrix shouldrm -rf __pycache__between mutations, or run one mutation per process; otherwise the measurement silently reports a neighbouring arm's verdict. No repo change needed — CI checks out fresh, so the sweep and PR jobs are unaffected.
Strengths
- The fix is minimal and precisely scoped: one word, +1/-1, restoring a guard rather than adding machinery. It resisted the tempting larger rewrite of
TestRunCliExitCodePolicythat the prior Suggestion floated, and the mutation matrix shows the smaller change bought the whole result. - All seven arms now carry a failing mutation, including both terminal layers. That is the bar this series set for itself three rounds ago and it is now actually met, rather than asserted — and the two layers are distinguished by their log line in both directions, which is what the
_dispatch()comment ("judge these arms on the log line, not on the workflow conclusion") requires to be true. - The class docstring at
:1119— "ANY exception class that escapesrun_cli()" — is now accurate rather than aspirational. It overstated the suite at the two preceding heads, and it is the sentence that made each of the last three residuals findable; keeping it exactly as strong as the tests is what let this series converge instead of drifting. test_deliberate_alarm_exit_passes_through_terminal_armstill pins the one mistake that would have quietly inverted the whole change —ExceptionvsBaseException, which would reclassify every deliberatesys.exit(EXIT_ALARM)as degraded.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
This PR is clean at its current head but still has an outstanding code-owner review request (kkroo, allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
… 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 702cb7d): 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 <noreply@anthropic.com>
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 <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.net>
… too Ally's Important at d3e21c1: 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 <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.net>
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) <noreply@anthropic.com>
0ebde02 to
75c5d87
Compare
|
@ally please re-review at head Rebase only — no behaviour change since your 0 Critical / 0 Important review at Review focus — the two things a rebase could have broken
Preservation evidence
|
|
@ally please review at head Re-requesting: the previous request for this head produced a reviewer run that failed on infrastructure, not on the code — run created Code is unchanged since your 0-Critical / 0-Important review at Review focus, if you want to spend it narrowly:
Suite on the rebased tree: 138 OK (88 mine + 50 arriving from master). Mutation spot-check: neutering only the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 75c5d87
Re-review after the k8s_pod_schedule_failed reviewer run. The three prior findings in this series are all retired (c53bdf1 → fixed @ d3e21c1, d3e21c1 → fixed @ 7c596ee, 7c596ee → fixed @ 0ebde027), and 0ebde027 minted none — so the active prior set is empty and there is no disposition section to carry.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex / types]
.github/scripts/sweep-stalled-ally-reviews.py:409—EXIT_SWEEP_DEGRADED = 2collides with argparse's own usage-error code. Measured, not argued:python3 sweep-stalled-ally-reviews.py --bogus-flagexits 2, becauseparse_argsraisesSystemExit(2), which is aBaseExceptionand so passes straight through both terminal arms by design. A malformed invocation therefore reports itself as "sweep degraded". Non-blocking and deliberately not raised higher: it is a pre-existing constant, unreachable fromreview-gate-sweep.yml:247(invoked with no arguments), and it fails toward degraded, never towardEXIT_ALARM— so it cannot produce the false "go review a stranded PR" this PR exists to close. Worth a one-line comment on the constant if you ever add a CLI flag, or move degraded to3. - [comments]
.github/scripts/test_sweep_stalled_ally_reviews.py:2063— Two blank lines betweentest_deliberate_alarm_exit_passes_through_terminal_armand the method below it, where the rest of the class uses one. Cosmetic only; confirmed no Python linter covers this path (ruff checkin CI is scoped tosrc/+tests/of thedocker-figma-bot/docker-webflow-botworkflows), so nothing is failing on it.
Strengths
-
The rebase claim verified rather than taken on trust, and it holds.
git diff f06c717a..0ebde027vsgit diff 9cce0faa..75c5d870: content lines are byte-identical. The only deltas are the four hunk headers' line numbers, plus one@@trailing-context line that moved fromTestCommentBodyIsModeAwaretoTestGuardSkipsAreVisibleInTheStepSummary— exactly the additive/additive tail resolution described, with master's class landing first. 187 changed lines (73+1+113) both sides. -
Every guard has a failing mutation — the bar this series set for itself, met in full. Re-measured at this head, one process per mutation,
__pycache__purged between runs:mutation result RateLimitExhaustedarmFAILED (failures=1) HTTPErrorarmFAILED (errors=1) URLErrorarmFAILED (failures=1) HTTPExceptionarmFAILED (failures=2) OSErrorarmFAILED (errors=3) terminal Exception(_dispatch)FAILED (failures=4) terminal Exception(run_cli)FAILED (errors=1) _dispatchterminal →BaseExceptionFAILED (failures=1) run_cliterminal →BaseExceptionFAILED (failures=1) run_climessage re-collides with_dispatch'sFAILED (failures=1) The
HTTPExceptionspot-check reproduces the statedfailures=2exactly, and the twoBaseExceptionmutations confirm theSystemExitpassthrough is pinned in both layers rather than asserted in a comment. Unmutated tree: 138 OK. -
Arm precedence is sound and the insertion did not disturb it.
HTTPError⊂URLError⊂OSErrorkeeps its order;http.client.HTTPExceptionhangs offExceptionand is disjoint fromOSError, so its position ahead of theOSErrorarm is free — andtest_incomplete_read_is_not_an_oserrorpins that premise, so a future Python that reparented the hierarchy would flag the arm as redundant instead of letting it rot. -
I went looking for a third door into the false alarm and it is shut. The in-loop path was the one place the same truncation class could still reach
EXIT_ALARM: the per-PR isolation at:1299appendspending_since=None, andalarmingis built fromis_alarming(...)on that field at:1342— so an in-loopIncompleteReadlands infailed→sweep_is_degraded, never inalarming. Combined with the_dispatcharms (raised out ofmain()) andrun_cli(raised out of an arm body), all three doors now route to degraded. -
The guard is on the real execution path.
review-gate-sweep.yml:247runspython3 .github/scripts/sweep-stalled-ally-reviews.py, which reachesrun_cli()via__main__; no other caller importsmain()or_dispatch()directly, so nothing bypasses the policy. -
Comments carry the measured evidence (run
35605218498,IncompleteRead(703602 bytes read, 68373 more expected)) and state the no-retry decision with its reasoning, which is why each arm reads as a guard rather than as defensive noise.
Gate state at this head, for context only: General tests (server 3/4) still in_progress, and the PR is BEHIND master — it will need an update before it can land.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
This branch has not been deployed
1 participant
Thinking Path
Linked Issues or Issue Description
response.read()inrequire-ally-review.pyon frr + onprem-k8s — different repos, different script) and BLO-28989 (truncated-read guard inrequire-ally-review.mjson penstock, done).Searched for an existing row before filing:
IncompleteRead,truncated,paginatingover Paperclip issues, andgh pr list --state openover this repo.git grep -E 'IncompleteRead|HTTPException' origin/master -- .github/scriptsreturns nothing, so this copy carries neither remedy.What Changed
sweep-stalled-ally-reviews.py: newexcept http.client.HTTPExceptionarm in the abort handler, exitingEXIT_SWEEP_DEGRADEDwith a "truncated response" message. Catches the base, notIncompleteReadalone, soBadStatusLine/LineTooLongtake the same path.sweep-stalled-ally-reviews.py: extracted theif __name__ == "__main__"body intorun_cli(). No behaviour change — purely so the arms are reachable from tests.sweep-stalled-ally-reviews.py: addedimport http.client, and a comment noting the workflow fails the job on either exit code, so this corrects the diagnosis, not the redness.test_sweep_stalled_ally_reviews.py: newTestRunCliExitCodePolicy, 7 cases covering all five arms plus the hierarchy fact that explains why the pre-existing arms could not catch it. 88 → 95 tests.Verification
Mutation-tested, per the standing rule that a guard with no failing mutation is a comment. Each mutation applied alone, suite re-run, file restored:
HTTPExceptionarmIncompleteReadonlyEXIT_ALARMinstead ofEXIT_SWEEP_DEGRADEDs.index(' except OSError as error:'), which matched an earlierexcept OSErrorat:760and so duplicated text instead of removing the arm. Two of the three mutations silently no-op'd and the suite stayed green — a broken mutation harness is indistinguishable from a sound guard. Re-anchored insiderun_cli()(index(..., base)) before trusting any of the results above.Also
python3 -c "import ast; ast.parse(...)"clean and--helpexits 0.Behaviour on the live lane, for context on why this is diagnosis-only: 13:22Z crashed with the traceback; 14:24Z (same commit
702cb7d7) and 15:20Z both succeeded withconsidered=135 refired=0 alarming=0 failed=0 deferred=0. The truncation was transient and the hourly cadence already supplied the retry.Risks
Low risk, and narrow by construction.
review-gate-sweep.ymlfails the job on any non-zero exit, so a truncation is red before and after. Judge this on the log line, not on the conclusion — the dashboard counts will not move.URLErrorstill reports "transport",TimeoutErrorstill reports "socket",RateLimitExhaustedstill reports its own message. The new arm's hierarchy is disjoint fromOSError, so it cannot shadow anything.run_cli()extraction is a pure move;if __name__ == "__main__": run_cli()preserves the entrypoint, verified by--help.issubclass(http.client.HTTPException, OSError) is False. If a future Python changes that, the test fails and flags the arm as redundant rather than letting it rot as unexplained duplication.Model Used
claude-opus-5[1m]