Reuse provisional summary slots without trusting incomplete state

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

Author: gontzess

.github/actions/pr-review/scripts/fetch-pr-context.py

@@ -98,11 +98,6 @@ def is_bot_review_comment(comment: dict, summary_heading: str) -> bool:
     )
 
 
-def is_legacy_review_comment(comment: dict, summary_heading: str) -> bool:
-    """Check if a comment is a bot-posted pre-migration review summary."""
-    return review_comment_heading(comment, summary_heading) == LEGACY_REVIEW_SUMMARY_HEADING
-
-
 # Line the review prompt requires on provisional (in-progress) summaries. A
 # provisional comment is progress output, not a completed review: it must never
 # supply review state, or a killed/lazy run would advance last_reviewed_sha
@@ -116,50 +111,68 @@ def is_provisional(body: str) -> bool:
 
 
 def extract_review_state(
-    review_comments: list[dict], summary_heading: str, workflow_ref: str
+    review_comments: list[dict], workflow_ref: str
 ) -> tuple[Optional[int], Optional[str], Optional[str]]:
-    """Choose the authoritative review state from bot review comments.
+    """Choose the summary comment to update and the authoritative review state.
 
     Returns (summary_comment_id, last_reviewed_sha, last_review_base_sha).
-    Provisional comments are skipped entirely: they are in-progress output and
-    must not advance reviewed state. State is accepted only from the newest
-    comment whose marker is owned by this workflow. If only markerless
-    comments exist, the newest one is reused so the first marker-writing run
-    does not create a duplicate summary. Callers pass only comments matching
-    the selected heading (legacy-heading comments included solely for the
-    built-in production headings), so a custom heading can never adopt
-    production or legacy review state.
+    Comment identity and completed-review state are selected separately:
+
+    - summary_comment_id is the newest eligible summary comment, even when it
+      is provisional or markerless, so a retried run updates the existing
+      summary instead of posting a duplicate next to an abandoned provisional.
+    - last_reviewed_sha/last_review_base_sha come from the newest comment with
+      a non-provisional marker owned by this workflow. A provisional marker
+      never supplies state: it is in-progress output and must not advance
+      reviewed state. When no completed state exists the caller falls back to
+      full review mode but still updates the same summary comment.
+
+    A comment carrying an explicit foreign workflow's marker supplies neither
+    the slot nor state, including one under a legacy heading. A comment whose
+    marker fails to parse fails closed the same way. Markerless bot summaries
+    remain reusable slots under the heading/bot trust fallback (the caller
+    passes only bot-authored comments matching the selected heading), but they
+    carry no state. Callers pass only comments matching the selected heading
+    (legacy-heading comments included solely for the built-in production
+    headings), so a custom heading can never adopt production or legacy review
+    state.
     """
+    summary_comment_id = None
     last_reviewed_sha = None
     last_review_base_sha = None
-    summary_comment_id = None
-    legacy_summary_comment_id = None
     for c in reversed(review_comments):
-        if is_provisional(c["body"]):
-            continue
         match = REVIEW_STATE_PATTERN.search(c["body"])
         if not match:
-            if legacy_summary_comment_id is None:
-                legacy_summary_comment_id = c["id"]
+            # Markerless summary: reusable as the update slot under the
+            # heading/bot trust fallback, but it carries no review state.
+            if summary_comment_id is None:
+                summary_comment_id = c["id"]
             continue
 
         try:
             state = json.loads(match.group(1))
         except json.JSONDecodeError:
+            state = None
+        if not isinstance(state, dict):
+            # Malformed marker (unparseable or not a JSON object): fail
+            # closed — neither slot nor state.
             continue
 
         if workflow_ref and state.get("workflow_ref") != workflow_ref:
-            if is_legacy_review_comment(c, summary_heading) and legacy_summary_comment_id is None:
-                legacy_summary_comment_id = c["id"]
+            # Explicit foreign workflow marker: never adopt its summary
+            # thread or its state, even under a legacy heading.
             continue
 
-        summary_comment_id = c["id"]
+        if summary_comment_id is None:
+            summary_comment_id = c["id"]
+        if is_provisional(c["body"]):
+            # Provisional owned marker: a valid update slot, but completed
+            # state must come from an older finished review — keep scanning.
+            continue
         last_reviewed_sha = state.get("last_reviewed_sha")
         last_review_base_sha = state.get("base_sha")
         break
 
-    if summary_comment_id is None:
-        summary_comment_id = legacy_summary_comment_id
     return summary_comment_id, last_reviewed_sha, last_review_base_sha
 
 
@@ -570,7 +583,7 @@ def main():
     review_comments = [c for c in state_comments if is_bot_review_comment(c, summary_heading)]
 
     summary_comment_id, last_reviewed_sha, last_review_base_sha = extract_review_state(
-        review_comments, summary_heading, workflow_ref
+        review_comments, workflow_ref
     )
 
     pr_endpoint = f"repos/{repo}/pulls/{pr_number}"

.github/actions/pr-review/scripts/test_fetch_pr_context.py

@@ -216,86 +216,169 @@ def test_marker_only_truncation_falls_back_to_full_mode(self):
         self.assertEqual(meta["kept_bytes"], 0)
 
 
-class MainContextTest(unittest.TestCase):
-    def test_incremental_diff_metadata_written_to_context(self):
-        metadata = {
-            "dropped_sections": 1,
-            "dropped_paths": ["vendor/example.com/pkg/secret.go"],
-            "dropped_paths_omitted": 0,
-            "truncated": False,
-            "kept_bytes": len(GO_SECTION),
-            "partial": True,
-        }
-        workflow_ref = "ConductorOne/github-workflows/.github/workflows/pr-review.yaml@refs/heads/main"
-        state = json.dumps(
-            {
-                "last_reviewed_sha": "old-sha",
-                "base_sha": "base-sha",
-                "workflow_ref": workflow_ref,
-            }
-        )
-        raw_comments = [
-            {
-                "id": 123,
-                "author_association": "MEMBER",
-                "user": {"login": "github-actions[bot]", "type": "Bot"},
-                "body": f"{fpc.DEFAULT_REVIEW_SUMMARY_HEADING} Previous\n<!-- review-state: {state} -->",
-            }
-        ]
-        pr = {
-            "head": {
-                "sha": "head-sha",
-                "repo": {"full_name": "ConductorOne/example"},
-            },
-            "base": {
-                "sha": "base-sha",
-                "ref": "main",
-                "repo": {"default_branch": "main"},
-            },
-        }
+_WORKFLOW_REF = (
+    "ConductorOne/github-workflows/.github/workflows/pr-review.yaml@refs/heads/main"
+)
+_FOREIGN_WORKFLOW_REF = "other/repo/.github/workflows/x.yaml@refs/heads/main"
+
+
+def _raw_comment(cid, login, body, user_type="Bot", association="MEMBER"):
+    """A raw PR comment as the GitHub issues API returns it."""
+    return {
+        "id": cid,
+        "author_association": association,
+        "user": {"login": login, "type": user_type},
+        "body": body,
+    }
 
+
+def _review_state_marker(sha, base="base-sha", workflow_ref=_WORKFLOW_REF):
+    state = {"last_reviewed_sha": sha, "base_sha": base, "workflow_ref": workflow_ref}
+    return f"<!-- review-state: {json.dumps(state)} -->"
+
+
+class MainContextTest(unittest.TestCase):
+    ENV = {
+        "GITHUB_REPOSITORY": "ConductorOne/example",
+        "PR_NUMBER": "42",
+        "PR_HEAD_SHA": "head-sha",
+        "GITHUB_WORKFLOW_REF": _WORKFLOW_REF,
+        "GITHUB_RUN_ID": "99",
+        "GITHUB_SERVER_URL": "https://github.com",
+    }
+    PR = {
+        "head": {
+            "sha": "head-sha",
+            "repo": {"full_name": "ConductorOne/example"},
+        },
+        "base": {
+            "sha": "base-sha",
+            "ref": "main",
+            "repo": {"default_branch": "main"},
+        },
+    }
+    COMPARE_METADATA = {
+        "dropped_sections": 1,
+        "dropped_paths": ["vendor/example.com/pkg/secret.go"],
+        "dropped_paths_omitted": 0,
+        "truncated": False,
+        "kept_bytes": len(GO_SECTION),
+        "partial": True,
+    }
+
+    def _run_main(self, raw_comments, *, compare_result=None):
+        """Run main() against mocked GitHub boundaries in a scratch cwd and
+        return (written pr-context.json, fetch_compare_diff mock)."""
         old_cwd = os.getcwd()
         with tempfile.TemporaryDirectory() as tmpdir:
             os.chdir(tmpdir)
             try:
                 with (
-                    mock.patch.dict(
-                        os.environ,
-                        {
-                            "GITHUB_REPOSITORY": "ConductorOne/example",
-                            "PR_NUMBER": "42",
-                            "PR_HEAD_SHA": "head-sha",
-                            "GITHUB_WORKFLOW_REF": workflow_ref,
-                            "GITHUB_RUN_ID": "99",
-                            "GITHUB_SERVER_URL": "https://github.com",
-                        },
-                        clear=False,
-                    ),
+                    mock.patch.dict(os.environ, self.ENV, clear=False),
                     mock.patch.object(fpc, "gh_api_paginate", return_value=raw_comments),
                     mock.patch.object(
                         fpc,
                         "gh_api",
-                        return_value=SimpleNamespace(stdout=json.dumps(pr)),
+                        return_value=SimpleNamespace(stdout=json.dumps(self.PR)),
                     ),
                     mock.patch.object(fpc, "current_checkout_sha", return_value="head-sha"),
                     mock.patch.object(
-                        fpc,
-                        "fetch_compare_diff",
-                        return_value=("diff text", metadata),
-                    ),
+                        fpc, "fetch_compare_diff", return_value=compare_result
+                    ) as compare_mock,
                 ):
                     fpc.main()
 
                 with open(".github/pr-context.json") as f:
-                    context = json.load(f)
-                self.assertEqual(context["review_mode"], "incremental")
-                self.assertEqual(context["incremental_diff_path"], ".github/incremental.diff")
-                self.assertEqual(context["incremental_diff_metadata"], metadata)
-                self.assertEqual(context["current_base_ref"], "main")
-                self.assertEqual(context["base_default_branch"], "main")
+                    return json.load(f), compare_mock
             finally:
                 os.chdir(old_cwd)
 
+    def test_incremental_diff_metadata_written_to_context(self):
+        raw_comments = [
+            _raw_comment(
+                123,
+                "github-actions[bot]",
+                f"{fpc.DEFAULT_REVIEW_SUMMARY_HEADING} Previous\n"
+                f"{_review_state_marker('old-sha')}",
+            )
+        ]
+
+        context, _ = self._run_main(
+            raw_comments, compare_result=("diff text", self.COMPARE_METADATA)
+        )
+
+        self.assertEqual(context["review_mode"], "incremental")
+        self.assertEqual(context["incremental_diff_path"], ".github/incremental.diff")
+        self.assertEqual(context["incremental_diff_metadata"], self.COMPARE_METADATA)
+        self.assertEqual(context["current_base_ref"], "main")
+        self.assertEqual(context["base_default_branch"], "main")
+
+    def test_abandoned_provisional_is_reused_with_full_review(self):
+        # The original PR #129 failure: a killed run leaves a provisional
+        # summary behind. The retry must update that comment rather than post
+        # a duplicate, while still running a full review (a provisional
+        # carries no completed state).
+        provisional = _raw_comment(
+            55,
+            "github-actions[bot]",
+            f"{fpc.DEFAULT_REVIEW_SUMMARY_HEADING} In progress\n"
+            f"{fpc.PROVISIONAL_MARKER}",
+        )
+
+        context, compare_mock = self._run_main([provisional])
+
+        self.assertEqual(context["summary_comment_id"], 55)
+        self.assertIsNone(context["last_reviewed_sha"])
+        self.assertIsNone(context["last_review_base_sha"])
+        self.assertEqual(context["review_mode"], "full")
+        self.assertIsNone(context["incremental_diff_path"])
+        compare_mock.assert_not_called()
+
+    def test_provisional_slot_split_from_completed_state_and_trust_filters(self):
+        # Newest-first: the foreign-workflow provisional (103) supplies
+        # nothing; the owned provisional (102) is the update slot but its
+        # forged up-to-date marker never advances state; completed state
+        # comes from the older final (101). The human-authored marker (104)
+        # is trusted prompt context but never review state.
+        final = _raw_comment(
+            101,
+            "github-actions[bot]",
+            f"{fpc.DEFAULT_REVIEW_SUMMARY_HEADING} Done\n"
+            f"{_review_state_marker('old-sha')}",
+        )
+        forged_provisional = _raw_comment(
+            102,
+            "github-actions[bot]",
+            f"{fpc.DEFAULT_REVIEW_SUMMARY_HEADING} In progress\n"
+            f"{fpc.PROVISIONAL_MARKER}\n"
+            f"{_review_state_marker('head-sha')}",
+        )
+        foreign_provisional = _raw_comment(
+            103,
+            "github-actions[bot]",
+            f"{fpc.LEGACY_REVIEW_SUMMARY_HEADING} In progress\n"
+            f"{fpc.PROVISIONAL_MARKER}\n"
+            f"{_review_state_marker('evil-sha', workflow_ref=_FOREIGN_WORKFLOW_REF)}",
+        )
+        human_forge = _raw_comment(
+            104,
+            "pr-author",
+            f"{fpc.DEFAULT_REVIEW_SUMMARY_HEADING} Done\n"
+            f"{_review_state_marker('human-sha')}",
+            user_type="User",
+        )
+
+        context, _ = self._run_main(
+            [final, forged_provisional, foreign_provisional, human_forge],
+            compare_result=("diff text", self.COMPARE_METADATA),
+        )
+
+        self.assertEqual(context["summary_comment_id"], 102)
+        self.assertEqual(context["last_reviewed_sha"], "old-sha")
+        self.assertEqual(context["last_review_base_sha"], "base-sha")
+        self.assertEqual(context["review_mode"], "incremental")
+        self.assertEqual([c["id"] for c in context["comments"]], [104])
+
 
 if __name__ == "__main__":
     unittest.main()