diff --git a/scripts/check-issue-links.py b/scripts/check-issue-links.py index 187afd80..dbbe0e53 100644 --- a/scripts/check-issue-links.py +++ b/scripts/check-issue-links.py @@ -488,6 +488,12 @@ class Refs: happen to it (a rejected or change-requested PR is updated in place, never replaced). + One fact is about the **issue itself** rather than about any referrer, and it is kept + here because it comes off the same event stream and is only ever read beside + `declared_merged`: `reopened_after_merge` says this issue was reopened after the + newest merged declarer landed. That is the fact `_declined_detail` needs to stop + calling a deliberately-reopened issue "never closed". + Each clause is written for the number of referrers it names (`_agrees`): the first revision of the fix above printed *"#1614 referenced it and are merged, and neither declares"* for one referrer and *"#1565 #1569 #1607 ... and neither declares"* for @@ -502,6 +508,7 @@ class Refs: mentioned_open: set[int] = field(default_factory=set) mentioned_landed: set[int] = field(default_factory=set) mentioned_abandoned: set[int] = field(default_factory=set) + reopened_after_merge: bool = False def _gh(args: list[str]) -> str: @@ -734,6 +741,23 @@ def _parse_instant(text: str) -> dt.datetime | None: return None +def _utc_instant(text: object) -> dt.datetime | None: + """`text` as a **comparable** instant, or `None` when it is not one. + + `_parse_instant` answers instants as written, and GitHub writes them with a `Z` + offset, so a value without one parses *naive* — and comparing a naive datetime with + an aware one raises `TypeError`. Assuming UTC for the offset-less case keeps every + pair comparable, and it is the right assumption: every instant compared here comes + from GitHub, whose timestamps are UTC. + """ + if not isinstance(text, str) or not text: + return None + parsed = _parse_instant(text.replace("Z", "+00:00")) + if parsed is None: + return None + return parsed if parsed.tzinfo else parsed.replace(tzinfo=dt.timezone.utc) + + def _retention_key(row: dict) -> str | None: """The key the retention rule ranks a **completed** row by, or `None` for any other row. @@ -1171,9 +1195,21 @@ def referencing_prs(events: list[dict], issue_number: int) -> Refs: referrer that was only closed). `merged_at` is the discriminator rather than `state`, because `state` is `closed` for an abandoned PR and for a closed *issue* alike, while only a merge says the change is on master. + + The `reopened` events are the one exception to "only `cross-referenced` matters": a + reopen **after** a merged declarer is what says that merge did not finish the work, + and without it `_declined_detail` tells the reader to close the issue over it. Every + other event kind is still skipped. """ refs = Refs() + reopened: list[dt.datetime] = [] + merged: list[dt.datetime] = [] for event in events: + if event.get("event") == "reopened": + stamp = _utc_instant(event.get("created_at")) + if stamp is not None: + reopened.append(stamp) + continue if event.get("event") != "cross-referenced": continue source = (event.get("source") or {}).get("issue") or {} @@ -1196,6 +1232,9 @@ def referencing_prs(events: list[dict], issue_number: int) -> Refs: refs.declared_open.add(pr) elif pull.get("merged_at"): refs.declared_merged.add(pr) + stamp = _utc_instant(pull.get("merged_at")) + if stamp is not None: + merged.append(stamp) else: refs.declared_abandoned.add(pr) elif open_pr: @@ -1204,6 +1243,12 @@ def referencing_prs(events: list[dict], issue_number: int) -> Refs: refs.mentioned_landed.add(pr) else: refs.mentioned_abandoned.add(pr) + # Only a **declared** merge closes the issue, so only those instants are compared: a + # merged mention leaves the issue open, and a reopen after it says nothing about + # whether the work finished. Compared against the newest one, so the fact reads "it + # was reopened after every merge that could have closed it". + if merged and reopened: + refs.reopened_after_merge = max(reopened) > max(merged) return refs @@ -1369,16 +1414,34 @@ def _declined_detail(refs: Refs, number: int) -> str: Both can be present at once (a merged attempt and an abandoned one on one issue), and a reader told only the first would close the issue over work that does not finish it. + + A third fact can hide inside the merged one and it reverses its remedy: the issue was + closed **by that merge** and reopened afterwards. "Landed and was never closed" is + then false - the merge that landed the work is exactly what closed it - and the reopen + is the record that the landing did not finish the work. Measured 2026-10-27 on issue + #2358's own timeline, where the merged declarer closed it at `01:51:39Z` and + `reopened` follows at `01:52:36Z`: the sentence sent a reader to close an issue + somebody had deliberately reopened. That clause therefore stops advising the close + and says what the reopen already says. """ clauses: list[str] = [] if refs.declared_merged: verb, _ = _agrees(refs.declared_merged) - clauses.append( - f"{_numbers(refs.declared_merged)} declared `Closes #{number}` and {verb} " - "merged, which is the shape of work that landed and was never closed - " - "either close this issue with the reading that says the work is done, or " - "state here what it still leaves" - ) + if refs.reopened_after_merge: + clauses.append( + f"{_numbers(refs.declared_merged)} declared `Closes #{number}` and {verb} " + "merged, and this issue was reopened after that merge - the merge is what " + "closed it, so the reopen is the reading that says the landing did not " + "finish the work: do not close this issue on that merge, state here what " + "it still leaves" + ) + else: + clauses.append( + f"{_numbers(refs.declared_merged)} declared `Closes #{number}` and {verb} " + "merged, which is the shape of work that landed and was never closed - " + "either close this issue with the reading that says the work is done, or " + "state here what it still leaves" + ) if refs.declared_abandoned: verb, _ = _agrees(refs.declared_abandoned) clauses.append( diff --git a/tests/test_check_issue_links.py b/tests/test_check_issue_links.py index 84a584d7..b17e2869 100644 --- a/tests/test_check_issue_links.py +++ b/tests/test_check_issue_links.py @@ -139,6 +139,19 @@ def _merged(number: int, *, body: str = "", merged_at: str = "2026-09-25T00:00:0 return _refers_to(number, is_pr=True, state="closed", body=body, merged_at=merged_at) +def _reopened(created_at: str = "2026-09-25T01:00:00Z") -> dict: + """A `reopened` event on the subject's own timeline - a state change, not a reference. + + It sits beside the `closed` event it follows. Measured 2026-10-27 on + `issues/2358/timeline`: `closed` `2026-10-18T01:51:39Z`, `reopened` + `2026-10-18T01:52:36Z`, `closed` `2026-10-18T02:04:45Z` - all three after the merged + declarer's cross-reference at `2026-10-17T23:03:00Z`. It is the only thing that + separates "the merge closed it and somebody reopened it on purpose" from "the merge + should have closed it and did not", which are the same row without it. + """ + return {"event": "reopened", "created_at": created_at} + + class FakeGh: """`_gh` replaced by a routing table; every call recorded. @@ -849,6 +862,106 @@ def test_a_declarer_that_closed_without_merging_is_not_called_landed( assert "work is done" not in detail +def test_an_issue_reopened_after_a_merge_is_not_called_never_closed( + mod, monkeypatch, capsys +) -> None: + """The merged declarer *did* close the issue; a reopen came after it. + + Measured shape (issue #2358's own timeline, 2026-10-27): the merged declarer's + reference, then a close, then a `reopened`. The reading used to print "landed and was + never closed" over that and send the reader to close an issue somebody had reopened + on purpose - the reopen being the record that the landing did not finish the work. + """ + fake = FakeGh( + [_issue(10, "reopened after the merge")], + [], + { + 10: [ + _refers_to( + 30, is_pr=True, state="closed", body="Closes #10.", + merged_at="2026-09-25T00:00:00Z", + ), + _reopened("2026-09-25T00:01:00Z"), + ] + }, + ) + _install(mod, monkeypatch, fake) + + rc, out = _run(mod, capsys) + + assert rc == 1, out + detail = _detail(out, "#10 issue UNCLAIMED") + assert "#30 declared `Closes #10`" in detail + assert "reopened after that merge" in detail + assert "never closed" not in detail + assert "work is done" not in detail + + +def test_a_reopen_before_the_merge_leaves_the_landed_reading_alone( + mod, monkeypatch, capsys +) -> None: + """The control, and it differs from the test above by the reopen's position. + + A reopen **before** the merge says nothing about that merge: the merge still closed + the issue, so "landed and was never closed" is the right sentence. Without this + direction a reading that called every merged declarer's issue reopened would pass the + test above, which is what a mutation arm is for, not what this file is for. + """ + fake = FakeGh( + [_issue(10, "reopened long before the merge")], + [], + { + 10: [ + _reopened("2026-09-24T00:00:00Z"), + _refers_to( + 30, is_pr=True, state="closed", body="Closes #10.", + merged_at="2026-09-25T00:00:00Z", + ), + ] + }, + ) + _install(mod, monkeypatch, fake) + + rc, out = _run(mod, capsys) + + assert rc == 1, out + detail = _detail(out, "#10 issue UNCLAIMED") + assert "landed and was never closed" in detail + assert "reopened after that merge" not in detail + + +def test_the_reopen_fact_is_read_against_a_declared_merge_only(mod) -> None: + """`reopened_after_merge` compares a reopen with a **declared** merge, nothing else. + + Two readings in one test: a merged *mention* never closes the issue, so a reopen + after it is no evidence about it; a declared merge before the reopen is evidence. + A reading that compared against every merge it saw would flip the flag off the + mention alone and print the corrective sentence where the close-it reading is right. + """ + mention_only = mod.referencing_prs( + [ + _merged(40, merged_at="2026-09-20T00:00:00Z"), + _reopened("2026-09-21T00:00:00Z"), + ], + 10, + ) + assert mention_only.mentioned_landed == {40} + assert mention_only.reopened_after_merge is False + + declared = mod.referencing_prs( + [ + _refers_to( + 30, is_pr=True, state="closed", body="Closes #10.", + merged_at="2026-09-20T00:00:00Z", + ), + _reopened("2026-09-21T00:00:00Z"), + ], + 10, + ) + assert declared.declared_merged == {30} + assert declared.reopened_after_merge is True + + def test_a_closed_issue_naming_a_pr_is_not_a_live_link(mod, monkeypatch, capsys) -> None: """The open-queue boundary, and it is stated in the row rather than implied.