Skip to content

Commit 8ef48e7

Browse files
wolfgang-auraclaude
andcommitted
fix: refuse a comment on somebody else's pull request, and flag a long reply
The comment that drew the complaint on python/mypy#21967 went out twelve seconds before our own pull request closed, so a gate keyed on the close could not have caught it. `handoff` now refuses an issue comment aimed at a pull request another author opened, in any run state; `--closing-reply` reaches only the superseding one, after provenance names it. A reply over 120 words gets a length warning in the block. The procedure says which threads a filed run writes to, and in what order a close is recorded. Follows #87. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent f7109a4 commit 8ef48e7

6 files changed

Lines changed: 186 additions & 16 deletions

File tree

README.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -568,9 +568,14 @@ pull request that superseded it with `--superseded-by`, the case is over and
568568
`handoff` refuses anything aimed at the issue, our pull request or the
569569
superseding one; `handoff-check` re-reads provenance at publish time, so a case
570570
that closes after the preview stops there too. One courtesy reply may still go
571-
out with `--closing-reply`, to one thread, once. On 2026-09-10 python/mypy#21961
572-
was closed in favour of #21967 and a review-style comment went to #21967 in the
573-
same minute; its author asked for the activity to stop. See
571+
out with `--closing-reply`, to one thread, once. Independently of the run's
572+
state, a comment aimed at a pull request somebody else opened is refused: the
573+
harness writes to its own issue and its own pull request, nowhere else. A reply
574+
over 120 words gets a length warning in the block. On 2026-09-10
575+
python/mypy#21961 was closed in favour of #21967; a 158-word review-style
576+
comment went to #21967 twelve seconds before the close and a 184-word one to
577+
#21961, each linking the other, and the author of #21967 asked for the activity
578+
to stop. See
574579
[the run record](docs/runs/0010-mypy-21961-superseded.md) and
575580
[issue #87](https://github.com/wolfgang-aura/Mailman/issues/87).
576581

docs/runs/0010-mypy-21961-superseded.md

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,16 +20,24 @@ on 2026-09-10; #21961 was closed at 20:31 UTC in favour of it.
2020

2121
## What went wrong, in one minute
2222

23-
At 20:31 UTC, the same minute #21961 closed, a review-style comment drafted
24-
here was posted on #21967: probe cases, a test-coverage suggestion, a wording
25-
nit and a cross-link back to #21961. The next morning its author replied
26-
that the cross-linking and AI-generated follow-ups made the thread harder to
27-
follow, and that mypy discourages LLM contributions from new contributors.
23+
Three writes, twelve seconds apart, all drafted here:
2824

29-
Every part of that is fair. The comment was correct and unwanted, which on a
30-
thread that is now somebody else's is the same as wrong. Nothing in
31-
`handoff`, `handoff-check` or `provenance --superseded-by` knew that the
32-
case had closed, so nothing refused it.
25+
| UTC | Thread | Words | Content |
26+
| --- | --- | --- | --- |
27+
| 20:31:12 | #21967, not ours | 158 | review-style comment: probe cases, a test suggestion, a wording nit, a link to #21961 |
28+
| 20:31:18 | #21961 | 184 | reply to the reviewer, linking #21967 |
29+
| 20:31:24 | #21961 | | closed by us |
30+
31+
The next morning the author of #21967 replied that the repeated
32+
cross-linking and AI-generated follow-ups made the thread harder to follow
33+
than it needed to be, and that mypy discourages LLM contributions from new
34+
contributors.
35+
36+
Every part of that is fair. The comment on #21967 was correct and unwanted,
37+
which on a thread that was never ours is the same as wrong. And it went out
38+
before the close, so a gate keyed on the close alone would not have caught
39+
it. The write-closed gate below covers the day after; the foreign-thread
40+
refusal covers the twelve seconds before.
3341

3442
## What changed
3543

@@ -43,6 +51,8 @@ and the superseding pull request are write-closed for the run:
4351
| `handoff-check` | re-reads provenance and refuses with `run-closed` when the case closed after the preview |
4452
| `handoff --closing-reply` | allows one courtesy reply to one thread; a second thread refuses with `closing-reply-spent` |
4553
| `provenance --superseded-by` | prints the rule on stderr at the moment the case is closed |
54+
| `handoff`, any state | refuses a comment on a pull request somebody else opened; `--closing-reply` reaches only the superseding one, after provenance names it |
55+
| `handoff`, any state | warns when a reply passes 120 words: that is a review, not a reply |
4656

4757
The operator posted a three-sentence step-back reply on #21967 on 2026-09-11
4858
and deleted the fork branch. That reply is the run's one closing reply; the
@@ -55,7 +65,10 @@ marker is on disk, and the gate now refuses a second one.
5565
other people. A correct comment there is still noise.
5666
- **Do not review the competing pull request.** The comparison belongs in
5767
our own thread, before the close, or nowhere. Reviewing the winner reads
58-
as contesting the close.
68+
as contesting the close. This is now refused whatever the run's state.
69+
- **One thread, one reply, one event.** Two comments in the same minute that
70+
link each other are the "repeated cross-linking" the author named. Answer
71+
in the thread that asked; let the other thread find it.
5972
- **The stopping reply is short.** Acknowledge, own the call, step back.
6073
Three sentences; no explanation of the process, no restated disclosure.
6174
- **Record it as superseded, not lost.** `--superseded-by` keeps the ledger

mailman/handoff.py

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,12 @@
5151

5252
_KINDS = frozenset({"pull-request", "issue-comment"})
5353

54+
#: Where a follow-up comment stops being a reply and becomes a review. The two
55+
#: comments that drew the complaint on python/mypy were 158 and 184 words; the
56+
#: one that answered a maintainer's two questions was 321 and was welcome, so
57+
#: this warns and does not refuse.
58+
COMMENT_WORD_LIMIT = 120
59+
5460
# Claims only the person posting can make true. The agent cannot read a diff
5561
# on the human's behalf, cannot take responsibility, and cannot vouch for the
5662
# change; a body that says otherwise in the first person is false the moment
@@ -278,6 +284,76 @@ def closed_threads(run_directory: Path, repository: str) -> dict[str, Any]:
278284
return {"closed": True, "why": why, "threads": threads}
279285

280286

287+
def foreign_pull_request(repository: str, number: int) -> str | None:
288+
"""The author of pull request `number` in `repository`, or None.
289+
290+
None means "not a pull request", or "GitHub could not be asked". The
291+
caller treats the second the same as the first, because the thread it is
292+
about to refuse is one it can also name from provenance; this lookup only
293+
catches the case provenance has not seen yet.
294+
"""
295+
if shutil.which("gh") is None:
296+
return None
297+
try:
298+
completed = subprocess.run(
299+
[
300+
"gh",
301+
"api",
302+
f"repos/{repository_slug(repository)}/pulls/{number}",
303+
"--jq",
304+
".user.login",
305+
],
306+
capture_output=True,
307+
text=True,
308+
encoding="utf-8",
309+
errors="replace",
310+
timeout=clamp_timeout_seconds(15),
311+
check=False,
312+
shell=False,
313+
)
314+
except (OSError, subprocess.TimeoutExpired):
315+
return None
316+
if completed.returncode != 0:
317+
return None
318+
return completed.stdout.strip() or None
319+
320+
321+
def foreign_thread_refusal(
322+
run_directory: Path,
323+
*,
324+
repository: str,
325+
kind: str,
326+
issue_number: int | None,
327+
pull_request_lookup: Callable[[str, int], str | None] | None = None,
328+
) -> str | None:
329+
"""Why a comment must not go to this thread: it is somebody else's PR.
330+
331+
The harness reviews its own pull request and answers on its own issue.
332+
It has no business on a pull request someone else opened, whatever the
333+
state of our run: the comment that drew the complaint on python/mypy#21967
334+
went out twelve seconds before our own pull request was closed, when
335+
nothing about the case was closed yet.
336+
See https://github.com/wolfgang-aura/Mailman/issues/87.
337+
"""
338+
if kind != "issue-comment" or issue_number is None:
339+
return None
340+
record = load_provenance(run_directory) or {}
341+
if issue_number == record.get("pull_request"):
342+
return None
343+
if issue_number == upstream_issue_number(run_directory, repository):
344+
return None
345+
lookup = pull_request_lookup or foreign_pull_request
346+
author = lookup(repository, issue_number)
347+
if author is None:
348+
return None
349+
return (
350+
f"#{issue_number} is a pull request by {author}, not ours. This run "
351+
"comments on its own issue and its own pull request; a review of "
352+
"somebody else's pull request is not something it sends, and "
353+
"--closing-reply does not cover it."
354+
)
355+
356+
281357
def closure_refusal(
282358
closure: dict[str, Any], *, kind: str, issue_number: int | None
283359
) -> str | None:
@@ -359,6 +435,20 @@ def _preamble(record: dict[str, Any], claims: list[dict[str, Any]]) -> list[str]
359435
"",
360436
]
361437
)
438+
words = record.get("word_count") or 0
439+
if record.get("kind") == "issue-comment" and words > COMMENT_WORD_LIMIT:
440+
lines.extend(
441+
[
442+
"-" * 72,
443+
f"LENGTH -- {words} words, and a reply is under {COMMENT_WORD_LIMIT}",
444+
"-" * 72,
445+
"",
446+
" A follow-up this long reads as a review. Answer what the thread",
447+
" asked and cut the rest; on python/mypy#21967 two comments of",
448+
" 158 and 184 words, each linking the other, drew a request to stop.",
449+
"",
450+
]
451+
)
362452
warning = record.get("maintainer_edit_warning")
363453
if warning:
364454
lines.extend(
@@ -453,11 +543,23 @@ def build_handoff(
453543
data_root: Path | None = None,
454544
closing_reply: bool = False,
455545
owner_type_lookup: Callable[[str], str | None] = github_owner_type,
546+
pull_request_lookup: Callable[[str, int], str | None] | None = None,
456547
) -> tuple[dict[str, Any], str]:
457548
"""Record the body's digest and render the block that hands it over."""
458549
closure = closed_threads(run_directory, repository)
459550
refusal = closure_refusal(closure, kind=kind, issue_number=issue_number)
460-
if refusal and not closing_reply:
551+
if refusal is None:
552+
# Not one of the case's own threads, so the override has no say.
553+
foreign = foreign_thread_refusal(
554+
run_directory,
555+
repository=repository,
556+
kind=kind,
557+
issue_number=issue_number,
558+
pull_request_lookup=pull_request_lookup,
559+
)
560+
if foreign:
561+
raise ValueError(foreign)
562+
elif not closing_reply:
461563
raise ValueError(refusal)
462564
if closing_reply:
463565
if refusal is None:
@@ -516,6 +618,7 @@ def build_handoff(
516618
"digest": body_digest(body),
517619
"first_person_claims": first_person_claims(body),
518620
"preservation_claims": preservation_claims(body),
621+
"word_count": len(body.split()),
519622
"head_owner": owner,
520623
"head_owner_type": owner_type,
521624
"maintainer_edit_warning": maintainer_edit_warning(owner, owner_type),

mailman/procedure.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,12 @@ status check you did not act on is pure cost.
198198
exact local branch and final body. Run `handoff-check`. Keep all filings
199199
and upstream writes pending. For a self-sourced defect, prepare any required
200200
issue text alongside the PR and ask for approval of the ordered filings.
201+
After filing, the run writes to two threads only: its own issue and its
202+
own pull request. Never a pull request someone else opened. One reply per
203+
thread per event, under 120 words unless a maintainer asked questions. If
204+
our pull request is closed or overtaken, record `provenance --pr N
205+
--superseded-by M` first; after that, `handoff` refuses all three threads
206+
and allows one `--closing-reply`. Nothing else goes out.
201207
16. Refresh the aging evidence for every ready candidate together with
202208
`mailman hunt refresh HUNT_ID --owner TOKEN` immediately before finishing.
203209
Duplicate searches and claim reads expire in an hour, and refreshing them

tests/test_handoff.py

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,9 @@ def setUp(self):
349349
authors = patch("mailman.handoff.check_authorship", return_value={"ok": True, "head": "fixture"})
350350
authors.start()
351351
self.addCleanup(authors.stop)
352+
foreign = patch("mailman.handoff.foreign_pull_request", return_value=None)
353+
foreign.start()
354+
self.addCleanup(foreign.stop)
352355

353356
"""The push-time half of the duplicate check.
354357
@@ -495,6 +498,13 @@ def setUp(self):
495498
)
496499
authors.start()
497500
self.addCleanup(authors.stop)
501+
# #21967 is somebody else's pull request; everything else is an issue.
502+
foreign = patch(
503+
"mailman.handoff.foreign_pull_request",
504+
side_effect=lambda _repo, number: "someone" if number == 21967 else None,
505+
)
506+
foreign.start()
507+
self.addCleanup(foreign.stop)
498508

499509
def _comment(self, root: Path, directory: Path, issue: int, **extra):
500510
body_path = root / "reply.md"
@@ -551,11 +561,41 @@ def test_an_open_run_is_untouched(self) -> None:
551561
with TemporaryDirectory() as name:
552562
root = Path(name)
553563
_, directory = _run_directory(root)
554-
record, _ = self._comment(root, directory, 21967)
564+
record, _ = self._comment(root, directory, 21960)
555565
self.assertFalse(record["closure"]["closed"])
566+
with self.assertRaises(ValueError):
567+
self._comment(root, directory, 21960, closing_reply=True)
568+
569+
def test_somebody_elses_pull_request_is_refused_while_ours_is_open(self) -> None:
570+
# The comment that drew the complaint went out twelve seconds before
571+
# our pull request closed. The close is not what makes it wrong.
572+
with TemporaryDirectory() as name:
573+
root = Path(name)
574+
_, directory = _run_directory(root)
575+
with self.assertRaises(ValueError) as caught:
576+
self._comment(root, directory, 21967)
577+
self.assertIn("pull request by someone, not ours", str(caught.exception))
578+
# And the override does not reach it until provenance names it.
556579
with self.assertRaises(ValueError):
557580
self._comment(root, directory, 21967, closing_reply=True)
558581

582+
def test_a_long_reply_is_flagged_as_a_review(self) -> None:
583+
with TemporaryDirectory() as name:
584+
root = Path(name)
585+
_, directory = _run_directory(root)
586+
body_path = root / "reply.md"
587+
body_path.write_text(("word " * 158).strip(), encoding="utf-8")
588+
record, block = build_handoff(
589+
run_id=directory.name,
590+
run_directory=directory,
591+
body_path=body_path,
592+
kind="issue-comment",
593+
repository="python/mypy",
594+
issue_number=21960,
595+
)
596+
self.assertEqual(record["word_count"], 158)
597+
self.assertIn("LENGTH -- 158 words", block)
598+
559599
def test_one_closing_reply_is_allowed_and_a_second_thread_is_not(self) -> None:
560600
with TemporaryDirectory() as name:
561601
root = Path(name)
@@ -575,7 +615,7 @@ def test_the_case_closing_after_the_handoff_stops_the_publish(self) -> None:
575615
with TemporaryDirectory() as name:
576616
root = Path(name)
577617
_, directory = _run_directory(root)
578-
record, _ = self._comment(root, directory, 21967)
618+
record, _ = self._comment(root, directory, 21960)
579619
self.assertTrue(check_handoff(directory)["ok"])
580620
_close_the_case(directory)
581621
result = check_handoff(directory)

tests/test_handoff_gates.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,9 @@ def setUp(self):
131131
authors = patch("mailman.handoff.check_authorship", return_value={"ok": True, "head": "fixture"})
132132
authors.start()
133133
self.addCleanup(authors.stop)
134+
foreign = patch("mailman.handoff.foreign_pull_request", return_value=None)
135+
foreign.start()
136+
self.addCleanup(foreign.stop)
134137

135138
def _run(self, root: Path):
136139
run = RunRecord(

0 commit comments

Comments
 (0)