Merge pull request #418 from igerber/fix-rerun-on-push

Create new comment on synchronize/reopened (not update canonical)

Author: igerber

.github/workflows/ai_pr_review.yml

@@ -113,7 +113,12 @@ jobs:
           PR_BODY: ${{ steps.pr.outputs.body }}
           BASE_SHA: ${{ steps.pr.outputs.base_sha }}
           HEAD_SHA: ${{ steps.pr.outputs.head_sha }}
-          IS_RERUN: ${{ github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' }}
+          # IS_RERUN must agree with the post-comment step's `isRerun` JS
+          # expression below: any non-`opened` pull_request event is also a
+          # rerun. Otherwise synchronize/reopened pushes get a new comment
+          # but the prompt omits <previous-ai-review-output>, and the model
+          # cannot focus on whether prior findings were addressed.
+          IS_RERUN: ${{ github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' || (github.event_name == 'pull_request' && github.event.action != 'opened') }}
           PREV_REVIEW: ${{ steps.prev_review.outputs.body }}
           PREV_REVIEW_FOUND: ${{ steps.prev_review.outputs.found }}
         run: |
@@ -213,7 +218,7 @@ jobs:
           model: gpt-5.4
           effort: xhigh
 
-      - name: Post PR comment (new on /ai-review, update on opened)
+      - name: Post PR comment (new on every event except initial open)
         uses: actions/github-script@v9
         env:
           CODEX_FINAL_MESSAGE: ${{ steps.run_codex.outputs.final-message }}
@@ -227,10 +232,19 @@ jobs:
             const { owner, repo } = context.repo;
             const issue_number = Number(process.env.PR_NUMBER);
 
-            // If this run was triggered by /ai-review (issue_comment or review_comment), create a NEW comment.
+            // Treat anything other than the PR's initial `opened` event as a
+            // rerun for comment-posting purposes:
+            //  - issue_comment / pull_request_review_comment: explicit /ai-review trigger
+            //  - pull_request.synchronize: push to PR branch
+            //  - pull_request.reopened: PR was reopened
+            // Only `pull_request.opened` updates the canonical comment in-place
+            // (creating it if absent). All other events create a new comment so
+            // that prior review history is preserved.
             const isRerun =
               context.eventName === "issue_comment" ||
-              context.eventName === "pull_request_review_comment";
+              context.eventName === "pull_request_review_comment" ||
+              (context.eventName === "pull_request" &&
+               context.payload.action !== "opened");
 
             // Marker for the "canonical" auto comment
             const marker = "<!-- ai-pr-review:codex:auto -->";

tests/test_openai_review.py

@@ -1755,6 +1755,86 @@ def test_workflow_sanitizes_pr_body_closing_tag(self):
         assert "</previous-ai-review-output>" in text
 
 
+class TestWorkflowCommentPosting:
+    """The workflow has TWO rerun-detection gates that must agree:
+      1. YAML `IS_RERUN` env in the prompt-build step — controls whether
+         the prompt includes the <previous-ai-review-output> block and
+         re-review framing.
+      2. JS `isRerun` in the post-comment step — controls whether the
+         comment is created fresh or updates the canonical comment.
+
+    If they disagree, you get nonsense states like "new comment posted but
+    prompt didn't see prior review" (synchronize bug pre-fix) or "canonical
+    comment updated but prompt was framed as rerun" (the inverse).
+
+    The contract: `pull_request.opened` is non-rerun; everything else
+    (`pull_request.synchronize`, `pull_request.reopened`, `issue_comment`,
+    `pull_request_review_comment`) is a rerun."""
+
+    @pytest.fixture
+    def workflow_text(self):
+        assert _SCRIPT_PATH is not None
+        repo_root = _SCRIPT_PATH.parent.parent.parent
+        wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml"
+        if not wf.exists():
+            pytest.skip("workflow not found")
+        return wf.read_text()
+
+    # --- Post-comment JS gate ---
+
+    def test_js_isrerun_includes_non_opened_pull_request(self, workflow_text):
+        """JS gate: non-opened pull_request events create a new comment."""
+        assert 'context.payload.action !== "opened"' in workflow_text, (
+            "post-comment isRerun must treat pull_request events other than "
+            "'opened' as reruns; otherwise synchronize/reopened overwrite the "
+            "canonical review comment and lose prior content."
+        )
+
+    def test_js_isrerun_still_includes_comment_events(self, workflow_text):
+        assert 'context.eventName === "issue_comment"' in workflow_text
+        assert 'context.eventName === "pull_request_review_comment"' in workflow_text
+
+    # --- YAML IS_RERUN gate ---
+
+    def test_yaml_isrerun_includes_non_opened_pull_request(self, workflow_text):
+        """YAML gate: non-opened pull_request events make the prompt include
+        the previous-review block. Must agree with the JS gate above."""
+        assert (
+            "github.event_name == 'pull_request' && github.event.action != 'opened'"
+            in workflow_text
+        ), (
+            "prompt-build IS_RERUN must include non-opened pull_request events "
+            "alongside comment triggers; otherwise synchronize/reopened pushes "
+            "create a new comment but the prompt omits <previous-ai-review-output>."
+        )
+
+    def test_yaml_isrerun_still_includes_comment_events(self, workflow_text):
+        assert "github.event_name == 'issue_comment'" in workflow_text
+        assert "github.event_name == 'pull_request_review_comment'" in workflow_text
+
+    # --- Parity contract: both gates must enumerate the same trigger set ---
+
+    def test_both_gates_enumerate_same_triggers(self, workflow_text):
+        """Whatever the gates use to express the rerun set, both must mention
+        each of the four rerun-trigger names so they cannot silently disagree.
+
+        This is a string-presence check (not a true semantic equality), but it
+        catches the realistic regression: someone editing one gate and
+        forgetting the other."""
+        rerun_signals = [
+            "issue_comment",
+            "pull_request_review_comment",
+            # The synchronize/reopened branch is expressed via action != opened
+            # in both gates, so we anchor on the action-comparison strings:
+            "github.event.action != 'opened'",  # YAML
+            'context.payload.action !== "opened"',  # JS
+        ]
+        for signal in rerun_signals:
+            assert signal in workflow_text, (
+                f"Expected rerun-set signal {signal!r} not found in workflow YAML"
+            )
+
+
 class TestExtractResponseText:
     def test_prefers_output_text_field(self, review_mod):
         result = {"output_text": "Direct text.", "output": []}