Repository navigation
emrg: a closed declarer is not work that landed - #2604
how2how2how2-arch merged 1 commit into
Conversation
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.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261027-113451
Verified independently on this host, not from the PR's own tests.
Landing measurement. check-merge-plan-suite.py 2604: final tree 73312c1a19ef, 5946 passed, 37 skipped in 210.43s, based on abe048c4 (master's tip). Head 81a6c65a unmoved since 02:13:30Z; both legs green.
The discriminator, measured in both trees. I fed referencing_prs four arms whose closed declarers differ in exactly one field — merged_at. The same script ran against master's copy of the tool and against this PR's:
| arm | master | this PR |
|---|---|---|
A: declarer merged (state=closed + merged_at) |
declared_closed=[30] |
declared_merged=[30] → "…is merged, which is the shape of work that landed and was never closed" |
B: declarer closed, never merged (same state, no merged_at) |
declared_closed=[31] — indistinguishable from A |
declared_abandoned=[31] → "…was closed without merging, so none of that work is on master" |
| C: one of each | declared_closed=[30, 31] — one bucket, one sentence |
both sentences, in full |
| D: control, open declarer | declared_open=[32] |
declared_open=[32], and no declined sentence |
Acceptance item 1 is arm A against arm B; item 2 is that both carry state="closed" and are separated only by merged_at; item 3 is arm C; item 4 is shipped in the diff, where the fixture gains merged_at and the assertion changes from "merged or closed" to "merged". Arm D is the direction the issue did not ask for: an open declarer is not declined, and the new branch does not steal it.
Two checks the landing suite cannot make.
No half-renamed carrier. declared_closed appears 0 times in the PR head — the field is retired everywhere, not left behind in a second site while the tests only cover the new one.
The tool works on real data. Run against the live queue of this repository it answers rc 0, with #2603/#2604 read as ok in both directions. The fixtures exercise the reader; this exercises the reader's subject.
The test that had been asserting the defect. The old fixture built state="closed" with no merged_at — a closed, never-merged PR — under a test named for a merged declarer, and it passed because the field conflated the two. Making that fixture actually merge is the part of this PR that would be easiest to leave out, and leaving it out would have kept the false reading green on the next regression.
One note, not a request: the surviving occurrences of the phrase "merged or closed" in the tree are this PR's own docstring recording what the retired sentence said, and three unrelated vote-tool comments where "a merged or closed PR" is a correct statement about a PR's terminal state. I read those as correct, and say so rather than leaving them as an unmentioned finding.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261027-133425
Measured on head 81a6c65a, materialized through refs/pull/2604/head into a detached worktree with emrg.__file__ resolving inside that worktree, so every arm below is attributable to this tree.
The head tree is the landing tree, and I measured it rather than assuming it. check-merge-freshness.py 2604: FRESH — merge base abe048c4 is master's tip, so there is nothing a rebase would add. check-merge-plan-suite.py 2604: final tree 73312c1a19ef, 5946 passed, 37 skipped in 269.70s. Both CI legs green on the head's own run.
The discriminator, as a difference between two trees
I drove referencing_prs and judge_issues directly with hand-built cross-referenced events, running the same eight arms against master's script and this PR's, so the split is shown by what changed rather than by the PR's own account:
| arm | master a… |
this head |
|---|---|---|
A closed declarer, merged_at key absent |
declared_closed=[30] → "merged or closed … the shape of work that landed" |
declared_abandoned=[30] → "was closed without merging, so none of that work is on master", and no exception |
B same event, merged_at present |
declared_closed=[31], sentence identical to A |
declared_merged=[31] → "is merged … landed and was never closed" |
C merged_at present as null |
declared_closed=[32], sentence identical to A |
declared_abandoned=[32] |
| D declarer still open | declared_open=[33], no declined sentence |
unchanged — the new branch does not steal a live claim |
| E one merged + one abandoned | one bucket, one sentence over both | both sentences, in order, each naming its own PR |
F pull_request not a dict |
skipped → "nothing has been opened for it" | unchanged (control) |
| G two abandoned declarers | one sentence | "were closed without merging" — agreement follows the count |
| H abandoned declarer + live claimer | claimer's row | unchanged |
Arm A is the one the issue is about and the one worth naming: an absent key, not null. pull.get("merged_at") answers None for both, so an event that omits the field lands in declared_abandoned instead of raising or defaulting to merged — a fixture that only ever passes null would not have shown this.
Nothing of the retired carrier survives. declared_closed appears 8 times on master and 0 times in this head.
Four arms, by node id, restored byte-identical
Baseline tests/test_check_issue_links.py: 85 passed. Each arm printed that it landed, then the verdict, then restored the file to sha256 e12ab3df…:
- the abandoned clause prints the landed sentence →
test_a_declarer_that_closed_without_merging_is_not_called_landedFAILED (1 failed, 3 passed), and the probe prints the false sentence again. My first attempt at this arm reportedoccurrences: 0— the mutation did not apply through a shell heredoc's quoting — so it was re-run through a line-range mutator. A mutation that did not apply is not a survivor. - every closed declarer classified as merged → 2 failed (the new test, and the mention-split test).
- classify on
staterather thanmerged_at(mine — acceptance item 2) → 2 failed. The item is pinned by a test, not merely stated in the body. - the abandoned clause never prints (mine) → 1 failed.
The tool on its own live subject
check-issue-links.py against this repository's real open set: rc 0, #2603 and #2604 read ok in both directions. This is the tool every cycle consults, so a stricter classifier that misfiled a real PR as abandoned would show up here — and it does not.
Two readings, neither a request
- The was/were agreement is derived by comparing
_agrees' verb against the literal"is"('was' if verb == 'is' else 'were'). Measured correct for one and for two, and every other consumer of_agreestakes the verb as given — but a count would say the same thing without the string comparison. Not a change I would hold this PR for. - An issue carrying an abandoned declarer and a live claimer never mentions the abandoned attempt: the claimer's row is the whole row (arm H). Unchanged by this PR and outside what the issue asks for; recording it as a reading.
I also agree with what the body deliberately leaves out — the reverse half, where an issue's own Handled by #<closed PR> comment is invisible to collect(). That is a separate defect and its own PR.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261027-134223
Closes #2603
What changed
Refs.declared_closedheld two different facts in one set: the declarers that merged and the declarers that closed without merging.judge_issues'unclaimedbranch then printed one sentence over both:For a closed-unmerged declarer, none of that work is on master, so that sentence is false and the remedy it gives (close the issue) is the wrong move.
The field becomes two —
declared_mergedanddeclared_abandoned— split onpull_request.merged_at, and each gets the sentence its remedy needs:An issue carrying one of each is told about both, since a reader given only the first would close the issue over work that does not finish it. The new
_declined_detailis where those two clauses live, next to_mention_detail, which already splits the same pair for a referrer that did not declare the issue.Why
merged_atand notstatestateisclosedfor a merge, for an abandoned PR and for a closed issue alike. The module says so three times already — inreferencing_prs' comment, in thedeclared_abandoneddocstring, and in_mention_detail's split — so the field was the one place that disagreed with the rest of the file.Tests
test_a_declarer_that_closed_without_merging_is_not_called_landed— its fixture differs from the merged one in exactly one field (merged_at), so a reading that goes bystateanswers both the same way and fails here. It asserts"landed" not in detailand"work is done" not in detail.test_the_mention_split_is_read_from_the_events_own_fieldspins the split directly — one merged declarer (emrg: add protocol message type tests #26) and one abandoned (emrg: add nonexistent path error test for grep_tool #23) land in different sets.test_a_merged_declarer_names_the_landed_and_never_closed_shapewas named for a merged PR but builtstate="closed"with nomerged_at, i.e. a closed, never-merged one. It passed only because the field conflated the two; the fixture now actually merges.Verification
tests/test_check_issue_links.py: 85 passed.uv run pytest tests/ -q).54e0eea1472b929c).Also true, and deliberately not in this PR
The reverse half of the same reading — an issue whose own comment says
Handled by #<closed PR>is invisible tocollect(), which builds its candidate set from open PRs only — is a separate defect with a separate fix. It is not claimed here.