From 81a6c65af499953a84ce7c7ad2ca29a3821cfeb6 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Tue, 27 Oct 2026 10:13:06 +0800 Subject: [PATCH] emrg: a closed declarer is not work that landed declared_closed held the merged declarers and the closed-unmerged ones in one set, so the unclaimed row printed the landed sentence over an attempt that never reached master - and told the reader to close the issue with a reading that says the work is done. The field is now declared_merged / declared_abandoned, split on merged_at (the discriminator this module already reads one branch down), and the two facts get the two sentences their opposite remedies need. --- scripts/check-issue-links.py | 74 ++++++++++++++++++++++++++------- tests/test_check_issue_links.py | 68 ++++++++++++++++++++++++++---- 2 files changed, 119 insertions(+), 23 deletions(-) diff --git a/scripts/check-issue-links.py b/scripts/check-issue-links.py index bd192141..187afd80 100644 --- a/scripts/check-issue-links.py +++ b/scripts/check-issue-links.py @@ -102,8 +102,10 @@ ------ linked the reading is complete in both directions unclaimed no open PR declares the issue. The row names what it can see instead: - closed PRs that declared it (the "landed and never closed" shape), or - PRs that referenced it without declaring (a mention is not a claim) + PRs that declared it and closed - **merged** (the "landed and never + closed" shape) and **closed without merging** (an attempt that ended) + are separate sentences, because only the first leaves work on master - + or PRs that referenced it without declaring (a mention is not a claim) duplicate more than one open PR declares the same open issue one-way one side names the other and is not named back. Both directions get it, because they have different readers: a PR that declares no issue leaves @@ -468,9 +470,12 @@ class Refs: * `declared_open` - open PRs whose body carries a closing keyword for this issue: the PRs that claim it, and the only set that decides the issue's state; - * `declared_closed` - closed or merged PRs that declared it. A merged declarer whose - issue is still open is the anomaly worth naming (the keyword should have closed - it), and a closed unmerged declarer is simply abandoned work; + * `declared_merged` - merged PRs that declared it. A merged declarer whose issue is + still open is the anomaly worth naming: the keyword should have closed it, so the + work is on master and the issue is stale; + * `declared_abandoned` - closed, never-merged PRs that declared it. Nothing of that + work is on master, so the issue is **not** stale: the attempt ended, and the rule + answers it in its own PR rather than by opening a second one; * `mentioned_open`, `mentioned_landed`, `mentioned_abandoned` - PRs that referenced the issue **without** declaring it, split three ways because the reader's next move differs for each, and folding them together is a defect this tool shipped (issue @@ -492,7 +497,8 @@ class Refs: """ declared_open: set[int] = field(default_factory=set) - declared_closed: set[int] = field(default_factory=set) + declared_merged: set[int] = field(default_factory=set) + declared_abandoned: set[int] = field(default_factory=set) mentioned_open: set[int] = field(default_factory=set) mentioned_landed: set[int] = field(default_factory=set) mentioned_abandoned: set[int] = field(default_factory=set) @@ -1182,7 +1188,16 @@ def referencing_prs(events: list[dict], issue_number: int) -> Refs: # "not open" and never as "open". open_pr = str(source.get("state") or "unknown") == "open" if issue_number in declared_claims(source.get("body")): - (refs.declared_open if open_pr else refs.declared_closed).add(pr) + # A closed declarer is two facts, not one, and the discriminator is already + # in hand: `merged_at` is read one branch down for a non-declaring referrer, + # for the same reason. Folding them into one set printed "landed" over an + # attempt that never reached master. + if open_pr: + refs.declared_open.add(pr) + elif pull.get("merged_at"): + refs.declared_merged.add(pr) + else: + refs.declared_abandoned.add(pr) elif open_pr: refs.mentioned_open.add(pr) elif pull.get("merged_at"): @@ -1339,6 +1354,42 @@ def _mention_detail(refs: Refs, number: int) -> str: return "; ".join(clauses) +def _declined_detail(refs: Refs, number: int) -> str: + """What to do about declarers that closed with no open one left. + + Two facts, one sentence each, because the remedies are opposite: a **merged** + declarer means the work is on master and the issue is stale, while a **closed, + unmerged** declarer means none of it is on master and the issue is not stale at all. + One sentence for both — "merged or closed, which is the shape of work that landed" — + told a reader to close an issue over work that had been abandoned, and this module + states the distinction three times elsewhere: `referencing_prs` uses `merged_at` + rather than `state`, the `declared_abandoned` field says so, and `_mention_detail` + splits the same pair for a referrer that did not declare. A declarer is the same + pair one step stronger, so it gets the same two sentences. + + 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. + """ + 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.declared_abandoned: + verb, _ = _agrees(refs.declared_abandoned) + clauses.append( + f"{_numbers(refs.declared_abandoned)} declared `Closes #{number}` and " + f"{'was' if verb == 'is' else 'were'} closed without merging, so none of " + "that work is on master - continue that branch and reopen it rather than " + "opening a second PR for the same issue" + ) + return "; ".join(clauses) + + def judge_issues( issues: list[dict], refs_by_issue: dict[int, Refs], @@ -1375,13 +1426,8 @@ def judge_issues( ) ) continue - if refs.declared_closed: - detail = ( - f"{_numbers(refs.declared_closed)} declared `Closes #{number}` and " - "are merged or closed, 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 open the PR that finishes it" - ) + if refs.declared_merged or refs.declared_abandoned: + detail = _declined_detail(refs, number) elif refs.mentioned_landed or refs.mentioned_abandoned or refs.mentioned_open: detail = _mention_detail(refs, number) else: diff --git a/tests/test_check_issue_links.py b/tests/test_check_issue_links.py index 500b3843..84a584d7 100644 --- a/tests/test_check_issue_links.py +++ b/tests/test_check_issue_links.py @@ -673,20 +673,33 @@ def test_the_mention_split_is_read_from_the_events_own_fields(mod) -> None: _refers_to(20, is_pr=True, body="open mention"), # open PR _merged(21), # merged _refers_to(22, is_pr=True, state="closed", body="abandoned"), # closed - _refers_to(23, is_pr=True, state="closed", body="Closes #10."), # declared + _refers_to(23, is_pr=True, state="closed", body="Closes #10."), # declared, closed _refers_to(24, is_pr=True, body="Closes #10."), # declared, open _refers_to(25, is_pr=False), # an *issue* + _refers_to( # declared, merged + 26, is_pr=True, state="closed", body="Closes #10.", + merged_at="2026-09-25T00:00:00Z", + ), ] refs = mod.referencing_prs(events, 10) assert refs.declared_open == {24} - assert refs.declared_closed == {23} + # A declarer that closed is two facts: #26 reached master, #23 did not. One set for + # both is what printed "landed" over an abandoned attempt. + assert refs.declared_merged == {26} + assert refs.declared_abandoned == {23} assert refs.mentioned_open == {20} assert refs.mentioned_landed == {21} assert refs.mentioned_abandoned == {22} - # #25 is an issue referencing the subject, so it is in none of the five sets above. - assert 25 not in refs.mentioned_open | refs.mentioned_landed | refs.mentioned_abandoned + # #25 is an issue referencing the subject, so it is in none of the sets above. + assert 25 not in ( + refs.declared_merged + | refs.declared_abandoned + | refs.mentioned_open + | refs.mentioned_landed + | refs.mentioned_abandoned + ) def test_a_mention_is_not_a_claim(mod, monkeypatch, capsys) -> None: @@ -784,7 +797,13 @@ def test_a_merged_declarer_names_the_landed_and_never_closed_shape( [], { 10: [ - _refers_to(30, is_pr=True, state="closed", body="Closes #10."), + # `merged_at` is what makes this merged; `state="closed"` alone is also + # what an abandoned PR reports, and the fixture said "merged" while + # building the other one until the field stopped conflating them. + _refers_to( + 30, is_pr=True, state="closed", body="Closes #10.", + merged_at="2026-09-25T00:00:00Z", + ), _refers_to(40, is_pr=True, state="open", body="just mentions #10"), ] }, @@ -796,8 +815,38 @@ def test_a_merged_declarer_names_the_landed_and_never_closed_shape( assert rc == 1, out detail = _detail(out, "#10 issue UNCLAIMED") assert "#30 declared `Closes #10`" in detail - assert "merged or closed" in detail + assert "merged" in detail assert "landed and was never closed" in detail + assert "closed without merging" not in detail + + +def test_a_declarer_that_closed_without_merging_is_not_called_landed( + mod, monkeypatch, capsys +) -> None: + """The mirror of the sentence above, and the one that used to be wrong. + + `state` is `closed` for a merge and for an abandoned attempt alike, so a reading + that folds the two prints "work that landed" over a PR that never reached master, + and tells the reader to close the issue with a reading that says the work is done. + This fixture and the one above differ in exactly one field (`merged_at`), so a + reading that goes by the state answers both the same way and this is what notices. + """ + fake = FakeGh( + [_issue(10, "an attempt that ended")], + [], + {10: [_refers_to(30, is_pr=True, state="closed", body="Closes #10.")]}, + ) + _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 "closed without merging" in detail + # Nothing of that work is on master, so the landed reading is false here. + assert "landed" not in detail + assert "work is done" not in detail def test_a_closed_issue_naming_a_pr_is_not_a_live_link(mod, monkeypatch, capsys) -> None: @@ -1022,7 +1071,7 @@ def test_the_two_directions_are_read_from_opposite_sources_and_never_transposed( refs = mod.referencing_prs(events, 10) assert refs.declared_open == {20} - assert refs.declared_closed == set() + assert refs.declared_merged == set() and refs.declared_abandoned == set() # #30 is an *open* PR that only mentions the issue: the bucket a citation belongs in. assert refs.mentioned_open == {30} assert refs.mentioned_landed == set() and refs.mentioned_abandoned == set() @@ -1033,11 +1082,12 @@ def test_the_two_directions_are_read_from_opposite_sources_and_never_transposed( empty = mod.referencing_prs([], 10) assert ( empty.declared_open, - empty.declared_closed, + empty.declared_merged, + empty.declared_abandoned, empty.mentioned_open, empty.mentioned_landed, empty.mentioned_abandoned, - ) == (set(), set(), set(), set(), set()) + ) == (set(), set(), set(), set(), set(), set()) assert mod.referencing_issues([], {10}) == set() assert mod.named_by_issue({}) == {} assert mod.declared_claims(None) == set()