Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -617,15 +617,24 @@ def report_verdict(pr: dict) -> int:
f'is hosted there and the fix lands there. Merging this pull request anyway is the '
f'maintainer\'s decision to take and not this script\'s, and not the agent\'s.')
return 43
state, _ = head_coverage(pr)
state, line = head_coverage(pr)
if state == PARTIAL:
print('status=COVERAGE_IS_PARTIAL the review covering the head read fewer files than the '
'pull request changed, so part of the diff has no review at all. Every partial on '
'record stayed partial at the identical ratio across every later round, so a '
're-request is not the remedy it reads as, and the reviewer names no file list in '
'these rounds, so which file went unread cannot be read from the API. Splitting is '
'the remedy where it applies, and it does not apply to a promotion. Otherwise this '
'is the maintainer\'s call, taken knowing one file of the diff has no review')
# The unread count comes from the line that decided PARTIAL, never from past rounds.
# None is unreachable there, and narrowing keeps a later change from crashing the gate.
counts = read_coverage(line)
if counts is None:
gap = 'that the counts on that line could not be re-read, so the total is unknown'
else:
unread = counts[1] - counts[0]
gap = (f'{unread} of the {counts[1]} changed files '
f'{"has" if unread == 1 else "have"} no review')
print(f'status=COVERAGE_IS_PARTIAL the review covering the head read fewer files than the '
f'pull request changed, so part of the diff has no review at all. Every partial on '
f'record stayed partial at the identical ratio across every later round, so a '
f're-request is not the remedy it reads as, and the reviewer names no file list in '
f'these rounds, so which file went unread cannot be read from the API. Splitting is '
f'the remedy where it applies, and it does not apply to a promotion. Otherwise this '
f'is the maintainer\'s call, taken knowing {gap}')
return 42
return 0

Expand Down
27 changes: 27 additions & 0 deletions scripts/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,33 @@ def test_status_exits_forty_two_on_a_round_that_read_part_of_the_diff(self) -> N
self.assertEqual(42, pr_review.main(['status', '7', '--repo', 'o/r']))
self.assertIn('status=COVERAGE_IS_PARTIAL', self.out.getvalue())

def test_the_partial_message_counts_the_unread_files_rather_than_assuming_one(self) -> None:
"""Every partial on record skipped exactly one file, which is a measurement of seven
rounds rather than a property the state carries, so the line reads the run it prints."""
for reviewed, changed, phrase in ((2, 3, '1 of the 3 changed files has'),
(5, 9, '4 of the 9 changed files have')):
with self.subTest(reviewed=reviewed, changed=changed):
out = self.enterContext(contextlib.redirect_stdout(io.StringIO()))
self.answer(payload([review(
body=OVERVIEW + f'\nCopilot reviewed {reviewed} out of {changed} changed '
f'files in this pull request and generated no comments.')]))
self.assertEqual(42, pr_review.main(['status', '7', '--repo', 'o/r']))
self.assertIn(phrase, out.getvalue())

def test_the_partial_message_still_blocks_where_the_counts_cannot_be_re_read(self) -> None:
"""Unreachable by construction, since PARTIAL is set only where that line parsed, so the
counts are withheld from `report_verdict` directly. A crash on the blocking path would
read to a caller as this script being broken rather than as a round that read part."""
pr = payload([self.partial()])
unparseable = 'a coverage line carrying no counts at all'
with mock.patch.object(pr_review, 'head_coverage',
return_value=(pr_review.PARTIAL, unparseable)):
self.assertEqual(42, pr_review.report_verdict(pr))
out = self.out.getvalue()
self.assertIn('status=COVERAGE_IS_PARTIAL', out)
self.assertIn('could not be re-read', out)
self.assertNotIn('changed files', out)

def test_status_exits_forty_three_on_a_wording_it_does_not_read(self) -> None:
self.answer(payload([review(body=OVERVIEW +
'\nCopilot reviewed some of the changed files.')]))
Expand Down