diff --git a/reviewbot/credentials.py b/reviewbot/credentials.py index 4b705cf..f439298 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,28 @@ 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, 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 @@ 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] = (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 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..75c4890 100644 --- a/tests/test_lease_report.py +++ b/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 `/#` 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