diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 6f21b1a..c6ca5ff 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -388,7 +388,10 @@ jobs: def select_range(reviews, head, full_base, profile, bot, ancestor): for review in reversed(reviews): - if (review.get('user') or {}).get('login') != bot or review.get('state') != 'APPROVED': + # A push can dismiss an approval without undoing completed analysis. + # Coverage comes from the validated publisher marker, not merge approval. + if ((review.get('user') or {}).get('login') != bot + or review.get('state') not in ('APPROVED', 'DISMISSED')): continue for raw in re.findall(r'', review.get('body') or ''): try: diff --git a/README.md b/README.md index a02e354..04985d6 100644 --- a/README.md +++ b/README.md @@ -150,7 +150,12 @@ The model, reasoning effort, and CLI/action pins remain unchanged by this separa #### Incremental review and execution controls The publisher records an `ai-review-coverage` marker only after a validated review is -posted. An APPROVE verdict without that marker does not count as coverage. A follow-up +posted. An APPROVE verdict without that marker does not count as coverage. Both +`APPROVED` and `DISMISSED` reviews from the trusted bot can supply coverage: dismissing +an approval (including automatic stale-review dismissal on push) preserves the record +of completed analysis. Coverage does not require a currently valid merge approval. +All marker, reviewed-SHA, profile, merge-base, and ancestry checks still apply. Use +`incremental: false` to request a full review regardless of prior coverage. A follow-up PR review starts at the last compatible reviewed SHA when it is an ancestor of the current head and the full PR merge base is unchanged. First reviews, rebases, force pushes, base changes, and incompatible settings fall back to the full PR diff. Same-head diff --git a/tests/test_review_range.py b/tests/test_review_range.py index b8b1a16..367f39e 100644 --- a/tests/test_review_range.py +++ b/tests/test_review_range.py @@ -90,14 +90,25 @@ def test_first_review_full_then_fixup_incremental(self): self.assertEqual((self.root / '.github/codex/output/diff-files.txt').read_text(), 'fixup.txt\n') self.assertEqual(json.loads((self.root / '.github/codex/output/range.json').read_text()), info) + def test_dismissed_completed_review_remains_coverage(self): + info = self.select() + # GitHub dismisses a stale approval on push, preserving its completion marker. + self.reviews = [self.coverage(info, head=self.base), + dict(self.coverage(info), state='DISMISSED')] + info = self.select() + self.assertEqual(info['range_kind'], 'incremental') + self.assertEqual(info['compare_base'], self.previous) + self.assertEqual((self.root / '.github/codex/output/diff-files.txt').read_text(), 'fixup.txt\n') + def test_force_push_and_changed_base_fall_back_to_full(self): info = self.select() self.git('checkout', '-b', 'rewritten', self.base) rewritten = self.commit('rewritten.txt') - self.reviews = [self.coverage(info, head=rewritten)] - self.assertEqual(self.select()['range_kind'], 'full') - self.reviews = [self.coverage(info, base=rewritten)] - self.assertEqual(self.select()['range_kind'], 'full') + for state in ('APPROVED', 'DISMISSED'): + for changes in (dict(head=rewritten), dict(base=rewritten), dict(head=self.head)): + with self.subTest(state=state, changes=changes): + self.reviews = [dict(self.coverage(info, **changes), state=state)] + self.assertEqual(self.select()['range_kind'], 'full') def test_settings_prompt_and_intent_changes_require_full_review(self): info = self.select() @@ -111,15 +122,21 @@ def test_settings_prompt_and_intent_changes_require_full_review(self): (self.ctx / 'pr.json').write_text(json.dumps(dict(title='Fix', body='Changed intent', headRefOid=self.head))) self.assertEqual(self.select()['range_kind'], 'full') - def test_approval_without_trusted_matching_coverage_is_not_progress(self): + def test_review_without_trusted_matching_coverage_is_not_progress(self): info = self.select() - cases = [dict(user={'login': self.bot}, state='APPROVED', commit_id=self.previous, body='Looks good'), - dict(self.coverage(info), user={'login': 'someone-else'}), - dict(self.coverage(info), commit_id=self.head), - dict(self.coverage(info), state='PENDING')] - for review in cases: - with self.subTest(review=review): - self.reviews = [review] + for state in ('APPROVED', 'DISMISSED'): + cases = [dict(user={'login': self.bot}, state=state, commit_id=self.previous, body='Looks good'), + dict(self.coverage(info), state=state, user={'login': 'someone-else'}), + dict(self.coverage(info), state=state, commit_id=self.head), + dict(self.coverage(info, profile='different-settings'), state=state), + dict(self.coverage(info, version=2), state=state)] + for review in cases: + with self.subTest(state=state, review=review): + self.reviews = [review] + self.assertEqual(self.select()['range_kind'], 'full') + for state in ('PENDING', 'COMMENTED', 'CHANGES_REQUESTED'): + with self.subTest(state=state): + self.reviews = [dict(self.coverage(info), state=state)] self.assertEqual(self.select()['range_kind'], 'full') def test_explicit_full_and_commit_mode_ignore_coverage(self):