Key the prgate comment marker on the reason alone

publishthing/apps/prgate/github.py

@@ -73,7 +73,6 @@ def gate_pull_request(
         pull_request = event.json_data["pull_request"]
         number = str(event.json_data["number"])
         sender = event.json_data["sender"]["login"]
-        sha = pull_request["head"]["sha"]
 
         gh_repo = thing.github_repo(event.repo_name)
 
@@ -132,23 +131,22 @@ def holds_claim(issue_number: int) -> bool:
         message = messages.close_message(
             result,
             label,
-            sha,
             review_label=review_label,
             deny_label=deny_label,
             policy_url=entry.get("policy_url"),
         )
 
         # github redelivers webhooks, and a reopen re-runs the gate; only
-        # skip the comment when we've already said this exact thing about
-        # this exact commit.  the close below still runs either way, and
+        # skip the comment when we've already said this exact thing on
+        # this pull request.  the close below still runs either way, and
         # closing an already-closed pull request is a no-op.
-        if _already_commented(gh_repo, number, result.reason, sha):
+        if _already_commented(gh_repo, number, result.reason):
             thing.debug(
                 "prgate",
-                "already commented on %s #%s for sha %s, not repeating",
+                "already commented on %s #%s for %s, not repeating",
                 event.repo_name,
                 number,
-                sha,
+                result.reason,
             )
         else:
             # comment before closing: if the close fails we'd rather have
@@ -162,9 +160,9 @@ def holds_claim(issue_number: int) -> bool:
 
 
 def _already_commented(
-    gh_repo: github.GithubRepo, number: str, reason: str, sha: str
+    gh_repo: github.GithubRepo, number: str, reason: str
 ) -> bool:
-    marker = messages.marker(reason, sha)
+    marker = messages.marker_match(reason)
     for comment in gh_repo.get_issue_comments(number):
         if marker in (comment.get("body") or ""):
             return True

publishthing/apps/prgate/messages.py

@@ -17,7 +17,11 @@
 from . import util
 
 # hidden marker so the gate can recognize its own previous comment on a
-# pull request and not repeat itself if github redelivers a webhook.
+# pull request and not repeat itself if github redelivers a webhook or
+# the pull request is closed and reopened.  keyed on the reason alone:
+# the gate's decision is a function of the pull request's description and
+# the issue it names, never of the code, so a new head commit can't
+# change the outcome without also changing the reason.
 MARKER_PREFIX = "<!-- prgate"
 
 # hidden marker recording that this pull request is the one that claimed
@@ -27,8 +31,20 @@
 CLAIM_MARKER_PREFIX = "<!-- prgate-claim"
 
 
-def marker(reason: str, sha: str) -> str:
-    return "%s:%s:%s -->" % (MARKER_PREFIX, reason, sha)
+def marker(reason: str) -> str:
+    return "%s:%s -->" % (MARKER_PREFIX, reason)
+
+
+def marker_match(reason: str) -> str:
+    """The substring that recognizes any past comment for ``reason``.
+
+    Deliberately unterminated: the marker used to carry the head sha as
+    well, and matching on the open prefix means the comments already
+    posted with the old format are still recognized.
+
+    """
+
+    return "%s:%s" % (MARKER_PREFIX, reason)
 
 
 def claim_marker(issue: int) -> str:
@@ -132,7 +148,6 @@ def accepted_message(issue: int, label: str, review_label: str) -> str:
 def close_message(
     result: util.GateResult,
     label: str,
-    sha: str,
     review_label: str = util.DEFAULT_REVIEW_LABEL,
     deny_label: str = util.DEFAULT_DENY_LABEL,
     policy_url: Optional[str] = None,
@@ -162,7 +177,7 @@ def close_message(
         steps = _STEPS_NO_ISSUE
 
     paragraphs = [
-        marker(result.reason, sha),
+        marker(result.reason),
         _INTRO,
         _POLICY % subs,
         (steps % subs).strip(),

tests/test_prgate_hook.py

@@ -304,13 +304,27 @@ def test_redelivery_does_not_repeat_the_comment():
     assert gh_repo.closed == ["7", "7"]
 
 
-def test_new_commit_gets_a_fresh_comment():
+def test_new_commit_does_not_get_a_fresh_comment():
+    """A reopen after a push is the same rejection, not a new one.
+
+    The marker used to carry the head sha, so pushing a commit and
+    reopening produced a second identical comment.
+
+    """
+
+    gh_repo = FakeRepo(comments=[messages.marker(util.CLOSE_NO_ISSUE)])
+    run(gh_repo, make_event(body="no reference"))
+
+    assert len(gh_repo.comments) == 1
+
+
+def test_comments_from_the_old_sha_bearing_marker_are_recognized():
     gh_repo = FakeRepo(
-        comments=[messages.marker(util.CLOSE_NO_ISSUE, "different-sha")]
+        comments=["<!-- prgate:%s:some-sha -->" % util.CLOSE_NO_ISSUE]
     )
     run(gh_repo, make_event(body="no reference"))
 
-    assert len(gh_repo.comments) == 2
+    assert len(gh_repo.comments) == 1
 
 
 def test_close_can_be_disabled():
@@ -335,19 +349,21 @@ def test_maintainer_exemption_is_per_repo():
     assert gh_repo.closed == ["7"]
 
 
-def test_marker_distinguishes_reason_and_sha():
-    assert messages.marker("no_issue", "aaa") != messages.marker(
-        "no_issue", "bbb"
-    )
-    assert messages.marker("no_issue", "aaa") != messages.marker(
-        "issue_closed", "aaa"
-    )
+def test_marker_distinguishes_reason():
+    assert messages.marker("no_issue") != messages.marker("issue_closed")
 
 
 def test_already_commented_matches_only_its_own_marker():
     gh_repo = FakeRepo(comments=["a normal human comment mentioning #5"])
     assert not prgate_github._already_commented(
-        gh_repo, "7", util.CLOSE_NO_ISSUE, SHA
+        gh_repo, "7", util.CLOSE_NO_ISSUE
+    )
+
+
+def test_already_commented_does_not_match_a_different_reason():
+    gh_repo = FakeRepo(comments=[messages.marker(util.CLOSE_ISSUE_CLOSED)])
+    assert not prgate_github._already_commented(
+        gh_repo, "7", util.CLOSE_NO_ISSUE
     )