From f4768a349c66e30c64dd725a1865634f5e47d4af Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 12:28:14 -0400 Subject: [PATCH 1/2] fix: a lease commit spammed the pull request's timeline `holder` is `/##-`, and `/#` is exactly GitHub's cross-repository issue reference syntax. It went into the lease commit message verbatim, so every borrow and every return posted a "referenced this in code-review-credentials" event onto the reviewed pull request's own timeline. Two events per review, 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. That pull request's timeline holds more lease references than commits, comments and labels combined. `lease_message` now spells the same three facts without the `#`: lease taken by MarketData-App/api pull 463 run 35743856939-1 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; rewriting it would reopen the race it was added to close. Only the human-facing message changes. `lease_spans` reads both forms and keys a hold on (repo, pr, run) rather than on the message's spelling, so the months of `#`-joined history already in the store keep parsing and `lease-report` is unaffected. This cannot retire the timeline events already posted. GitHub builds a cross-reference from the commit and offers no way to delete one. --- reviewbot/credentials.py | 71 +++++++++++++++++++++++++------ tests/test_credentials.py | 65 ++++++++++++++++++++++++++++ tests/test_lease_report.py | 86 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 12 deletions(-) diff --git a/reviewbot/credentials.py b/reviewbot/credentials.py index 4b705cf..b9e8364 100644 --- a/reviewbot/credentials.py +++ b/reviewbot/credentials.py @@ -116,6 +116,34 @@ def _held(lease: dict, now: datetime.datetime) -> bool: return when > now +def lease_message(verb: str, holder: str) -> str: + """The commit message for a lease write. NEVER the raw holder. + + `holder` is `/##-`, and `/#` + 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 @@ def acquire(api, holder: str, run_url: str, now: datetime.datetime, ttl_minutes: "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 @@ def release(api, holder: str) -> None: 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 @@ def release(api, holder: str) -> None: # 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,24 @@ def lease_spans(commits: list[dict]) -> list[LeaseSpan]: 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, 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 for one hold, whichever format wrote it, so a `taken` and its + # `freed` pair on identity rather than on spelling. + if "#" in token: + repo, pr, run = _parse_holder(token) + key = (repo, pr, run) + 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 + 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,16 +311,14 @@ def lease_spans(commits: list[dict]) -> list[LeaseSpan]: if when.tzinfo is None: when = when.replace(tzinfo=datetime.UTC) if verb == "taken": - open_spans[holder] = when + open_spans[key] = when continue - start = open_spans.pop(holder, None) + start = open_spans.pop(key, None) if start 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(holder, repo, pr, run, start, None)) + out.append(LeaseSpan(token, repo, pr, run, start, when)) + for (repo, pr, run), start in open_spans.items(): + out.append(LeaseSpan(f"{repo}#{pr}#{run}", repo, pr, run, start, None)) out.sort(key=lambda s: s.start) return out diff --git a/tests/test_credentials.py b/tests/test_credentials.py index 2939502..bfa728d 100644 --- a/tests/test_credentials.py +++ b/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 `/##-`, and `/#` +# 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 + # `#-` 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" diff --git a/tests/test_lease_report.py b/tests/test_lease_report.py index 5c78e6d..7664f8a 100644 --- a/tests/test_lease_report.py +++ b/tests/test_lease_report.py @@ -385,3 +385,89 @@ 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 `/#` 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 From fb444cd2de0727f0dd1b2f8ef9296e14a3b14884 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 12:39:21 -0400 Subject: [PATCH 2/2] fix: LeaseSpan.holder depended on the message format and on being closed a1f9e042 from the bot's review of this pull request. A new-format span took the bare repository token as its `holder` ("MarketData-App/api"), and an open span with no pull request was given a fabricated "verification#None#". So the field's meaning depended both on which message wrote the hold and on whether it had been freed. The holder is now rebuilt once, beside the pairing key, and carried through the open-span map. Old and new formats yield the identical string, an open hold reads the same as a closed one, and a holder with no pull request stays "verification" rather than being padded. One test round-trips every shape through `lease_message` and back. 2f9d3e2b (nit) Not taken, for the third time and the same reason: dated measurements in docstrings are this repository's house style, present in credentials.py, github.py, claude.py and cli.py. --- reviewbot/credentials.py | 26 ++++++++++++--------- tests/test_lease_report.py | 47 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/reviewbot/credentials.py b/reviewbot/credentials.py index b9e8364..f439298 100644 --- a/reviewbot/credentials.py +++ b/reviewbot/credentials.py @@ -285,7 +285,7 @@ def lease_spans(commits: list[dict]) -> list[LeaseSpan]: is skipped for the same reason `_held` treats an unparseable lease as free -- this must not be the thing that breaks. """ - open_spans: dict[tuple, 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 "" @@ -293,16 +293,20 @@ def lease_spans(commits: list[dict]) -> list[LeaseSpan]: if not match: continue verb, token = match.group(1), match.group(2) - # One key for one hold, whichever format wrote it, so a `taken` and its - # `freed` pair on identity rather than on spelling. + # 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) - key = (repo, pr, run) + 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 - key = (repo, pr, run) + # 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")) @@ -311,14 +315,14 @@ def lease_spans(commits: list[dict]) -> list[LeaseSpan]: if when.tzinfo is None: when = when.replace(tzinfo=datetime.UTC) if verb == "taken": - open_spans[key] = when + open_spans[key] = (holder, when) continue - start = open_spans.pop(key, None) - if start is None: + found = open_spans.pop(key, None) + if found is None: continue - out.append(LeaseSpan(token, repo, pr, run, start, when)) - for (repo, pr, run), start in open_spans.items(): - out.append(LeaseSpan(f"{repo}#{pr}#{run}", repo, pr, run, start, None)) + 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 diff --git a/tests/test_lease_report.py b/tests/test_lease_report.py index 7664f8a..75c4890 100644 --- a/tests/test_lease_report.py +++ b/tests/test_lease_report.py @@ -471,3 +471,50 @@ def test_the_message_a_lease_write_produces_is_the_message_the_report_reads(): 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