fix: a lease commit spammed the pull request's timeline

reviewbot/credentials.py

@@ -116,6 +116,34 @@
     return when > now
 
 
+def lease_message(verb: str, holder: str) -> str:
+    """The commit message for a lease write. NEVER the raw holder.
+
+    `holder` is `<owner>/<repo>#<pr>#<run>-<attempt>`, and `<owner>/<repo>#<pr>`
+    is exactly GitHub's cross-repository issue reference syntax. Writing it into
+    a commit message in the store posted a "referenced this in
+    code-review-credentials" event onto the pull request's own timeline -- once
+    on borrow and once on return, on every review, for the life of the branch.
+    Measured 2026-09-22 on MarketData-App/api#463: twelve lease commits, twelve
+    timeline events, timestamps matching to the second.
+
+    So the message spells the same three facts without the `#`. The HOLDER is
+    untouched: `release` compares it to decide whether this run still owns the
+    lease, and that identity is the whole reason it carries the run id and
+    attempt. Only the human-facing text changes.
+
+    `lease_spans` reads both this and the old `#`-joined form, because the
+    store holds months of the old one.
+    """
+    repo, *rest = holder.split("#")
+    out = f"lease {verb} by {repo}"
+    if len(rest) >= 1 and rest[0]:
+        out += f" pull {rest[0]}"
+    if len(rest) >= 2 and rest[1]:
+        out += f" run {rest[1]}"
+    return out
+
+
 def acquire(api, holder: str, run_url: str, now: datetime.datetime, ttl_minutes: int = 20) -> bool:
     """Take the lease, or return False. Never raises for an ordinary loss.
 
@@ -133,7 +161,11 @@
         "expires_at": (now + datetime.timedelta(minutes=ttl_minutes)).isoformat(),
     }
     return api.put_file(
-        LEASE_PATH, json.dumps(body, indent=2) + "\n", f"lease taken by {holder}", LEASE_BRANCH, sha
+        LEASE_PATH,
+        json.dumps(body, indent=2) + "\n",
+        lease_message("taken", holder),
+        LEASE_BRANCH,
+        sha,
     )
 
 
@@ -158,7 +190,11 @@
         return
     body = {"holder": None, "run_url": None, "acquired_at": None, "expires_at": None}
     api.put_file(
-        LEASE_PATH, json.dumps(body, indent=2) + "\n", f"lease freed by {holder}", LEASE_BRANCH, sha
+        LEASE_PATH,
+        json.dumps(body, indent=2) + "\n",
+        lease_message("freed", holder),
+        LEASE_BRANCH,
+        sha,
     )
 
 
@@ -175,7 +211,10 @@
 # module reports occupancy, and `lease_report` points the reader at the log
 # string for the question the history cannot answer.
 
-_LEASE_MESSAGE = re.compile(r"^lease (taken|freed) by (\S+)$")
+# BOTH MESSAGE FORMATS. The trailing groups are the form `lease_message`
+# writes now; a token carrying `#` is the form written before 2026-09-22,
+# and the store holds months of it.
+_LEASE_MESSAGE = re.compile(r"^lease (taken|freed) by (\S+)(?: pull (\S+))?(?: run (\S+))?$")
 
 # How far BEFORE the reported window to read the history.
 #
@@ -246,14 +285,28 @@
     is skipped for the same reason `_held` treats an unparseable lease as free
     -- this must not be the thing that breaks.
     """
-    open_spans: dict[str, datetime.datetime] = {}
+    open_spans: dict[tuple, tuple[str, datetime.datetime]] = {}
     out: list[LeaseSpan] = []
     for entry in reversed(commits or []):
         body = (entry.get("commit") or {}).get("message") or ""
         match = _LEASE_MESSAGE.match(body.strip().splitlines()[0] if body.strip() else "")
         if not match:
             continue
-        verb, holder = match.group(1), match.group(2)
+        verb, token = match.group(1), match.group(2)
+        # One key and one holder for one hold, whichever format wrote it, so a
+        # `taken` pairs with its `freed` on identity rather than on spelling
+        # and `LeaseSpan.holder` reads the same either way.
+        if "#" in token:
+            repo, pr, run = _parse_holder(token)
+            holder = token
+        else:
+            repo, run = token, match.group(4) or ""
+            raw_pr = match.group(3)
+            pr = int(raw_pr) if raw_pr and raw_pr.isdigit() else None
+            # Rebuilt, never padded: a hold with no pull request is
+            # `verification`, not `verification#None#`.
+            holder = "#".join([repo, *([raw_pr] if raw_pr else []), *([run] if run else [])])
+        key = (repo, pr, run)
         stamp = ((entry.get("commit") or {}).get("committer") or {}).get("date")
         try:
             when = datetime.datetime.fromisoformat((stamp or "").replace("Z", "+00:00"))
@@ -262,15 +315,13 @@
         if when.tzinfo is None:
             when = when.replace(tzinfo=datetime.UTC)
         if verb == "taken":
-            open_spans[holder] = when
+            open_spans[key] = (holder, when)
             continue
-        start = open_spans.pop(holder, None)
-        if start is None:
+        found = open_spans.pop(key, None)
+        if found is None:
             continue
-        repo, pr, run = _parse_holder(holder)
-        out.append(LeaseSpan(holder, repo, pr, run, start, when))
-    for holder, start in open_spans.items():
-        repo, pr, run = _parse_holder(holder)
+        out.append(LeaseSpan(found[0], repo, pr, run, start=found[1], end=when))
+    for (repo, pr, run), (holder, start) in open_spans.items():
         out.append(LeaseSpan(holder, repo, pr, run, start, None))
     out.sort(key=lambda s: s.start)
     return out

tests/test_credentials.py

@@ -270,3 +270,68 @@ def test_a_run_still_frees_its_own_lease(store):
     credentials.release(api, "MarketData-App/sdk-py#100#551-1")
     written = json.loads(base64.b64decode(transport.calls[-1]["body"]["content"]))
     assert written["holder"] is None
+
+
+# --- the commit message must not reference the pull request ----------------
+#
+# `holder` is `<owner>/<repo>#<pr>#<run>-<attempt>`, and `<owner>/<repo>#<pr>`
+# is GitHub's CROSS-REPOSITORY ISSUE REFERENCE syntax. Putting it in a commit
+# message posted a "referenced this in code-review-credentials" event onto the
+# pull request's timeline -- twice per review, on every review, forever.
+# Measured 2026-09-22 on MarketData-App/api#463: twelve lease commits, twelve
+# timeline events, timestamps matching to the second.
+#
+# The holder itself is unchanged: `release` compares it to decide whether this
+# run still owns the lease, and that identity is why it carries the run id.
+# Only the human-facing message is rewritten.
+
+HOLDER = "MarketData-App/api#463#35743856939-1"
+
+
+def _message_of(transport):
+    return transport.calls[-1]["body"]["message"]
+
+
+def test_acquire_does_not_write_a_cross_repository_reference(store):
+    api, transport = store
+    lease_body(transport, None, None)
+    transport.add("PUT", f"/repos/{STORE}/contents/codex/lease.json", data={"commit": {}})
+    credentials.acquire(api, HOLDER, "https://run/1", NOW)
+    assert "#" not in _message_of(transport)
+    assert "api#463" not in _message_of(transport)
+
+
+def test_release_does_not_write_a_cross_repository_reference(store):
+    api, transport = store
+    lease_body(transport, HOLDER, "2026-09-16T14:20:00+00:00")
+    transport.add("PUT", f"/repos/{STORE}/contents/codex/lease.json", data={"commit": {}})
+    credentials.release(api, HOLDER)
+    assert "#" not in _message_of(transport)
+
+
+def test_the_message_still_names_the_repository_pull_request_and_run(store):
+    api, transport = store
+    lease_body(transport, None, None)
+    transport.add("PUT", f"/repos/{STORE}/contents/codex/lease.json", data={"commit": {}})
+    credentials.acquire(api, HOLDER, "https://run/1", NOW)
+    message = _message_of(transport)
+    assert message == "lease taken by MarketData-App/api pull 463 run 35743856939-1"
+
+
+def test_the_lease_body_still_carries_the_holder_unchanged(store):
+    # The identity `release` compares. Changing it would reopen the race that
+    # `#<run_id>-<run_attempt>` was added to close.
+    api, transport = store
+    lease_body(transport, None, None)
+    transport.add("PUT", f"/repos/{STORE}/contents/codex/lease.json", data={"commit": {}})
+    credentials.acquire(api, HOLDER, "https://run/1", NOW)
+    written = json.loads(base64.b64decode(transport.calls[-1]["body"]["content"]))
+    assert written["holder"] == HOLDER
+
+
+def test_a_holder_with_no_pull_request_still_gets_a_message(store):
+    api, transport = store
+    lease_body(transport, None, None)
+    transport.add("PUT", f"/repos/{STORE}/contents/codex/lease.json", data={"commit": {}})
+    credentials.acquire(api, "verification", "https://run/1", NOW)
+    assert _message_of(transport) == "lease taken by verification"

tests/test_lease_report.py

@@ -385,3 +385,136 @@ def test_a_nonpositive_window_is_refused(monkeypatch):
     for bad in ("0", "-3"):
         with pytest.raises(SystemExit):
             cli.main(["lease-report", "--days", bad])
+
+
+# --- both message formats --------------------------------------------------
+#
+# The message stopped carrying `<owner>/<repo>#<pr>` on 2026-09-22, because
+# GitHub read it as a cross-reference and posted it to the pull request's
+# timeline. The store keeps months of the old form, so both must parse.
+
+
+def new_taken(repo, pr, run, when):
+    return commit(f"lease taken by {repo} pull {pr} run {run}", when)
+
+
+def new_freed(repo, pr, run, when):
+    return commit(f"lease freed by {repo} pull {pr} run {run}", when)
+
+
+def test_the_new_message_format_parses():
+    spans = credentials.lease_spans(
+        [
+            new_freed("MarketData-App/api", 463, "35743856939-1", "2026-09-22T14:59:47Z"),
+            new_taken("MarketData-App/api", 463, "35743856939-1", "2026-09-22T14:59:06Z"),
+        ]
+    )
+    assert len(spans) == 1
+    assert spans[0].repo == "MarketData-App/api"
+    assert spans[0].pr == 463
+    assert spans[0].run == "35743856939-1"
+    assert spans[0].seconds == 41
+
+
+def test_the_old_message_format_still_parses():
+    spans = credentials.lease_spans(
+        [
+            freed("MarketData-App/api#463#35743856939-1", "2026-09-22T14:59:47Z"),
+            taken("MarketData-App/api#463#35743856939-1", "2026-09-22T14:59:06Z"),
+        ]
+    )
+    assert len(spans) == 1
+    assert spans[0].repo == "MarketData-App/api"
+    assert spans[0].pr == 463
+    assert spans[0].run == "35743856939-1"
+
+
+def test_a_history_holding_both_formats_pairs_each_within_its_own():
+    # What the store looks like across the change: older holds in the old
+    # form, newer ones in the new. A span never mixes the two -- both writes
+    # come from one run, on one version of the bot.
+    spans = credentials.lease_spans(
+        [
+            new_freed("o/r", 2, "b-1", "2026-09-22T11:00:00Z"),
+            new_taken("o/r", 2, "b-1", "2026-09-22T10:50:00Z"),
+            freed("o/r#1#a-1", "2026-09-22T10:20:00Z"),
+            taken("o/r#1#a-1", "2026-09-22T10:00:00Z"),
+        ]
+    )
+    assert [s.run for s in spans] == ["a-1", "b-1"]
+    assert [s.seconds for s in spans] == [1200, 600]
+
+
+def test_a_message_with_no_pull_request_parses():
+    spans = credentials.lease_spans(
+        [
+            commit("lease freed by verification", "2026-09-22T10:01:00Z"),
+            commit("lease taken by verification", "2026-09-22T10:00:00Z"),
+        ]
+    )
+    assert spans[0].repo == "verification"
+    assert spans[0].pr is None
+
+
+def test_the_message_a_lease_write_produces_is_the_message_the_report_reads():
+    # The writer and the reader are one round trip, so a change to either
+    # without the other fails here rather than in the next week's report.
+    holder = "MarketData-App/api#463#35743856939-1"
+    spans = credentials.lease_spans(
+        [
+            commit(credentials.lease_message("freed", holder), "2026-09-22T10:10:00Z"),
+            commit(credentials.lease_message("taken", holder), "2026-09-22T10:00:00Z"),
+        ]
+    )
+    assert len(spans) == 1
+    assert spans[0].repo == "MarketData-App/api"
+    assert spans[0].pr == 463
+    assert spans[0].run == "35743856939-1"
+    assert spans[0].seconds == 600
+
+
+def test_the_holder_is_the_same_string_whichever_format_wrote_it():
+    # Found by the bot's review of PR #20 (a1f9e042). `holder` took the bare
+    # repository token on a new-format span and a fabricated
+    # `verification#None#` on an open one, so its meaning depended on the
+    # message's spelling and on whether the hold had been freed.
+    old = credentials.lease_spans(
+        [
+            freed("MarketData-App/api#463#35743856939-1", "2026-09-22T10:10:00Z"),
+            taken("MarketData-App/api#463#35743856939-1", "2026-09-22T10:00:00Z"),
+        ]
+    )
+    new = credentials.lease_spans(
+        [
+            new_freed("MarketData-App/api", 463, "35743856939-1", "2026-09-22T10:10:00Z"),
+            new_taken("MarketData-App/api", 463, "35743856939-1", "2026-09-22T10:00:00Z"),
+        ]
+    )
+    assert old[0].holder == new[0].holder == "MarketData-App/api#463#35743856939-1"
+
+
+def test_an_open_span_carries_the_same_holder_a_closed_one_would():
+    open_only = credentials.lease_spans(
+        [new_taken("MarketData-App/api", 463, "35743856939-1", "2026-09-22T10:00:00Z")]
+    )
+    assert open_only[0].holder == "MarketData-App/api#463#35743856939-1"
+
+
+def test_a_holder_with_no_pull_request_is_not_padded_with_none():
+    spans = credentials.lease_spans([commit("lease taken by verification", "2026-09-22T10:00:00Z")])
+    assert spans[0].holder == "verification"
+
+
+def test_the_holder_round_trips_through_the_message_the_writer_produces():
+    for holder in (
+        "MarketData-App/api#463#35743856939-1",
+        "MarketDataApp/sdk-py#144#35744629707-2",
+        "verification",
+    ):
+        spans = credentials.lease_spans(
+            [
+                commit(credentials.lease_message("freed", holder), "2026-09-22T10:10:00Z"),
+                commit(credentials.lease_message("taken", holder), "2026-09-22T10:00:00Z"),
+            ]
+        )
+        assert spans[0].holder == holder