Skip to content

emrg: a closed declarer is not work that landed - #2604

Merged
how2how2how2-arch merged 1 commit into
argszero:masterfrom
pm25coder:fix/a-closed-declarer-is-not-landed
Oct 27, 2026
Merged

how2how2how2-arch merged 1 commit into
argszero:masterfrom
pm25coder:fix/a-closed-declarer-is-not-landed

Conversation

@pm25coder

Copy link
Copy Markdown
Collaborator

Closes #2603

What changed

Refs.declared_closed held two different facts in one set: the declarers that merged and the declarers that closed without merging. judge_issues' unclaimed branch then printed one sentence over both:

#30 declared Closes #10 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

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_merged and declared_abandoned — split on pull_request.merged_at, and each gets the sentence its remedy needs:

  • merged → the work is on master and the issue is stale: close it with that reading, or state here what it still leaves;
  • closed without merging → none of it is on master and the issue is not stale: continue that branch and reopen it rather than opening a second PR for the same issue.

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_detail is 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_at and not state

state is closed for a merge, for an abandoned PR and for a closed issue alike. The module says so three times already — in referencing_prs' comment, in the declared_abandoned docstring, and in _mention_detail's split — so the field was the one place that disagreed with the rest of the file.

Tests

  • New: 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 by state answers both the same way and fails here. It asserts "landed" not in detail and "work is done" not in detail.
  • Extended: test_the_mention_split_is_read_from_the_events_own_fields pins 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.
  • Fixed: test_a_merged_declarer_names_the_landed_and_never_closed_shape was named for a merged PR but built state="closed" with no merged_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.
  • Full suite: 5468 passed, 514 skipped (uv run pytest tests/ -q).
  • Two mutation arms, each printing that the mutation landed before measuring, both killed on the named test:
    • the abandoned clause prints the landed sentence → 1 failed / 84 passed;
    • every closed declarer classified as merged → 2 failed / 83 passed.
  • Source file restored byte-identical after the arms (sha256 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 to collect(), which builds its candidate set from open PRs only — is a separate defect with a separate fix. It is not claimed here.

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 how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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_landed FAILED (1 failed, 3 passed), and the probe prints the false sentence again. My first attempt at this arm reported occurrences: 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 state rather than merged_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

  1. 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 _agrees takes 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.
  2. 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 how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261027-134223

@how2how2how2-arch
how2how2how2-arch merged commit 7ac31ac into argszero:master Oct 27, 2026
2 checks passed
@argszero argszero mentioned this pull request Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: a declaration that closed without merging reads as work that landed

3 participants