[verified] Allow rework from a single valid review request

This commit is contained in:
2026-10-13 11:14:11 +00:00
parent 31ac634585
commit a7a13aa4d5
4 changed files with 149 additions and 10 deletions
+135 -2
View File
@@ -131,7 +131,7 @@ class OrderedDecisionTests(unittest.TestCase):
self.assertEqual(receipt['status'],'no_op',receipt['reason_codes'])
self.assertEqual(receipt['decision']['selected_review_ids']['gitea-review-agent'],303)
def test_missing_stale_and_dismissed_current_roles_block(self):
def test_missing_stale_and_dismissed_approvals_are_not_noop_eligible(self):
import copy
self.assertEqual(self.snapshot()['status'],'no_op')
for rows in ([self.rows[0]], [self.rows[1]], []):
@@ -409,7 +409,7 @@ class OrderedDecisionTests(unittest.TestCase):
if value == 'missing': self.assertNotIn(field, raw_record)
else: self.assertEqual(raw_record[field], value)
def test_counter_mismatch_does_not_unblock_missing_code_review(self):
def test_counter_mismatch_does_not_unblock_lone_approval(self):
self.set_rows(self.rows[:1])
self.rb.pr['comments'] = self.rb.underlying['comments'] = 0
receipt = self.snapshot()
@@ -417,6 +417,139 @@ class OrderedDecisionTests(unittest.TestCase):
self.assertEqual(receipt['counter_diagnostics']['pr']['status'], 'mismatch')
self.assertEqual(receipt['decision']['rework_review_ids'], [])
def spec_body(self, verdict, head=None):
from harness.tests.spec import test_implementation_spec_review_support as spec_tests
result = spec_tests.result(verdict)
result['target'] = {'owner':'acme','repository':'demo','issue':7,'pull':9,
'head':head or self.inp['head']}
result['contract_sha256'] = self.inp['contract_sha256']
if verdict == 'request_changes':
result['criteria'][0].update(result='unsatisfied', reason='Bounded regression finding.')
result['conclusion'] = 'Bounded correction required.'
return spec_tests.mod.render_review(result, {'src/a.py','tests/test_a.py'}, declaration={'mode':'none'})
def test_lone_spec_request_changes_allows_rework(self):
rows = [dict(self.rows[0], state='REQUEST_CHANGES', body=self.spec_body('request_changes'))]
self.set_rows(rows)
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'rework', receipt['reason_codes'])
self.assertEqual(receipt['decision']['selected_review_ids']['gitea-spec-agent'], 301)
self.assertIsNone(receipt['decision']['selected_review_ids']['gitea-review-agent'])
self.assertEqual(receipt['decision']['rework_review_ids'], [301])
self.assertIn('missing_current_review', receipt['reason_codes'])
def test_lone_code_request_changes_allows_rework(self):
import copy
row = copy.deepcopy(self.rows[1])
row.update(state='REQUEST_CHANGES', body=self.code_body('request_changes'))
self.set_rows([row])
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'rework', receipt['reason_codes'])
self.assertIsNone(receipt['decision']['selected_review_ids']['gitea-spec-agent'])
self.assertEqual(receipt['decision']['selected_review_ids']['gitea-review-agent'], 302)
self.assertEqual(receipt['decision']['rework_review_ids'], [302])
self.assertIn('missing_current_review', receipt['reason_codes'])
def test_valid_request_changes_survives_invalid_counterpart_history(self):
import copy
for request_index in (0, 1):
for excluded in ('stale', 'dismissed', 'old_head'):
with self.subTest(request_role=request_index, excluded=excluded):
rows = copy.deepcopy(self.rows)
request = rows[request_index]
request.update(state='REQUEST_CHANGES', body=self.spec_body('request_changes')
if request_index == 0 else self.code_body('request_changes'))
other = rows[1-request_index]
if excluded == 'old_head':
other.update(commit_id='a'*40, body=self.spec_body('approve', head='a'*40)
if request_index == 1 else self.code_body('approve').replace(self.inp['head'], 'a'*40))
else: other[excluded] = True
self.set_rows(rows)
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'rework', receipt['reason_codes'])
self.assertEqual(receipt['decision']['rework_review_ids'], [request['id']])
self.assertIsNone(receipt['decision']['selected_review_ids'][other['user']['login']])
self.assertIn('missing_current_review', receipt['reason_codes'])
def test_excluded_lone_requests_cannot_authorize_rework(self):
import copy
for request_index in (0, 1):
for excluded in ('stale', 'dismissed', 'old_head'):
with self.subTest(request_role=request_index, excluded=excluded):
request = copy.deepcopy(self.rows[request_index])
head = 'a'*40 if excluded == 'old_head' else self.inp['head']
request.update(state='REQUEST_CHANGES', commit_id=head,
body=self.spec_body('request_changes', head=head) if request_index == 0
else self.code_body('request_changes').replace(self.inp['head'], head))
if excluded != 'old_head': request[excluded] = True
self.set_rows([request])
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'blocked')
self.assertEqual(receipt['decision']['rework_review_ids'], [])
def test_superseded_request_with_missing_counterpart_stays_blocked(self):
import copy
request = copy.deepcopy(self.rows[1])
request.update(state='REQUEST_CHANGES', body=self.code_body('request_changes'))
approval = copy.deepcopy(self.rows[1])
approval.update(id=303, submitted_at='2026-01-02T00:00:00Z', updated_at='2026-01-02T00:00:00Z',
html_url=self.rb.pr['html_url'] + '#pullreview-303')
self.set_rows([request, approval])
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'blocked', receipt['reason_codes'])
self.assertNotEqual(receipt['status'], 'rework')
self.assertNotEqual(receipt['status'], 'no_op')
self.assertEqual(receipt['decision']['rework_review_ids'], [])
def test_true_blockers_override_valid_request_changes(self):
import copy
request = copy.deepcopy(self.rows[1])
request.update(state='REQUEST_CHANGES', body=self.code_body('request_changes'))
for state in ('COMMENT', 'PENDING'):
with self.subTest(state=state):
rows = copy.deepcopy(self.rows)
rows[1] = copy.deepcopy(request)
rows[0]['state'] = state
self.set_rows(rows)
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'blocked')
self.assertIn('comment_manual_classification' if state == 'COMMENT'
else 'pending_authoritative_review', receipt['reason_codes'])
rows = copy.deepcopy(self.rows)
rows[1] = request
rows[0]['body'] = 'missing marker'
self.set_rows(rows)
self.assertEqual(self.snapshot()['status'], 'blocked')
rows = copy.deepcopy(self.rows)
rows[1] = request
rows[0]['body'] = rows[0]['body'].replace(self.inp['contract_sha256'], 'f'*64)
self.set_rows(rows)
self.assertEqual(self.snapshot()['status'], 'blocked')
rows = copy.deepcopy(self.rows)
result = __import__('harness.tests.spec.test_implementation_spec_review_support', fromlist=['result'])
spec_result = result.result('blocked_spec')
spec_result['target'] = {'owner':'acme','repository':'demo','issue':7,'pull':9,'head':self.inp['head']}
spec_result['contract_sha256'] = self.inp['contract_sha256']
spec_result['criteria'][0]['result'] = 'indeterminate'
spec_result['scope']['spec_conflicts'] = ['src/a.py']
rows[0].update(state='COMMENT', body=result.mod.render_review(spec_result, {'src/a.py','tests/test_a.py'}, declaration={'mode':'none'}))
rows[1] = request
self.set_rows(rows)
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'blocked')
self.assertIn('blocked_spec', receipt['reason_codes'])
def test_lone_request_changes_keeps_rework_despite_counter_mismatch(self):
import copy
request = copy.deepcopy(self.rows[1])
request.update(state='REQUEST_CHANGES', body=self.code_body('request_changes'))
self.set_rows([request])
self.rb.pr['comments'] += 2
receipt = self.snapshot()
self.assertEqual(receipt['status'], 'rework', receipt['reason_codes'])
self.assertEqual(receipt['decision']['rework_review_ids'], [302])
self.assertEqual(receipt['counter_diagnostics']['pr']['status'], 'mismatch')
def test_counter_mismatch_does_not_hide_per_review_inline_count_mismatch(self):
self.rb.reviews[0]['comments_count'] = 1
self.rb.pr['comments'] = self.rb.underlying['comments'] = 0