Bound the Review Wait by Its Timeout, End It by SIGTERM With One Line, and Read the Origin Owner Through origin_slug - #4168

Summary

• Unfiled defect this run found. pr_review.py wait could run past --timeout and then end with nothing printed.
  • backoff() checked the bound only before each sleep, and the schedule's last delay is 120 seconds. So a wait could overrun its bound by up to that delay plus the reads after it.
  • An outer bound set at or below --timeout then killed it with SIGTERM. The script had no SIGTERM handler, so nothing printed.
  • The fix cuts each clock-bounded sleep to the time left, so no poll sleeps past --timeout. The reads that follow the bound still run, and then the digest and status line print.
  • The entry point now follows CODESTYLE.md "Interrupts in Python":
    • A SIGTERM handler raises KeyboardInterrupt carrying the signal number.
    • The entry point prints pr_review.py: interrupted by <SIGNAL>., flushes, restores the default handler, and re-raises the signal. On Windows it exits 130 instead.
    • SIGINT takes the same path.
  • The combined SIGINT/SIGTERM handler is not adopted, because that is the open decision Decide Whether the Python Interrupt Rule Routes SIGINT Through the SIGTERM Handler and Passes Over a Later Signal #4146.
• Move pr_review.origin_owner Onto gh_graphql.origin_slug #3890. origin_owner() now gets the owner from gh_graphql.origin_slug() instead of keeping its own copy of the origin-URL regex.
• Line ceiling. pr_review.py stays under its 2200-line ceiling. Three call sites are reshaped onto fewer lines to make room, and their behavior does not change.
• pr_merge.py and pr_cleanup.py. Both call backoff() with a finite schedule and no clock, which this change does not affect. Their suites pass unchanged.

Tests

• test_the_backoff_sleeps_the_schedule_until_the_timeout now expects the seventh sleep to be cut to the 110 seconds left.

• test_wait_sleeps_no_further_than_its_timeout_and_then_prints_the_digest runs wait --timeout 100 on a fake clock. It asserts that the sleeps stop at 100 seconds rather than 110, and that the digest and status=PENDING print.

• TestWaitInterrupt runs the script with its interpreter directly, from a copy of the scripts tree whose origin names a constructed owner. A stand-in gh on PATH holds the first read open. The case then sends SIGTERM and asserts three things:

  • The process died by SIGTERM.
  • It printed exactly the one interrupt line.
  • Nothing in its process group survived.

  It uses the process-group helpers in tests/test_pre_commit_hook.py, and it is skipped on Windows.

• Each new or changed case fails against develop's pr_review.py and passes here. The full suite passes locally: 4162 tests, 11 skipped.

Open findings, local pass edit budget spent

• introduced, design: scripts/pr_review.py gets to 2198 lines, under its 2200-line ceiling, by reshaping three multi-line calls onto fewer lines rather than by splitting the file along a boundary. The three are origin_owner, _gh_run, and the poll in run_wait. One line of headroom is left.
  • Outcome: declined, with this evidence.
    • The reshaped calls behave the same, and the ceiling is not raised.
    • Splitting the file along a boundary means adding a module. Every new module needs an entry in scripts/README.md, and a peer pull request holds that file this round.
    • So the next change that needs room in pr_review.py does the split, as tests/test_pr_review_size.py asks. The burndown run has been told about it.

Declined findings

• Local pass, scripts/pr_review.py __main__: a suggestion to share the re-raise ending with scripts/backlog_metrics.py reraise. Declined for three reasons:

  • It needs edits to backlog_metrics.py, which is outside this change.
  • It needs a shared home in gh_graphql.py, a library documented as the GraphQL transport.
  • backlog_metrics.py uses the combined-handler variant that Decide Whether the Python Interrupt Rule Routes SIGINT Through the SIGTERM Handler and Passes Over a Later Signal #4146 has yet to decide.

  The interrupt line's wording is matched to backlog_metrics.py's, period included.

Closes on promotion: #3890

🤖 Generated with Claude Code