From e9e166f3aa568a47db523d468a30c16a3b5b2fcb Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Sat, 19 Sep 2026 09:48:03 +0800 Subject: [PATCH] emrg: the merge condition names the clause the counter applies --- emrg/server/evolution_prompt.md | 4 +- .../test_evolution_prompt_merge_condition.py | 241 ++++++++++++++++++ 2 files changed, 244 insertions(+), 1 deletion(-) create mode 100644 tests/test_evolution_prompt_merge_condition.py diff --git a/emrg/server/evolution_prompt.md b/emrg/server/evolution_prompt.md index 70201d0c..3101a4c4 100644 --- a/emrg/server/evolution_prompt.md +++ b/emrg/server/evolution_prompt.md @@ -152,6 +152,8 @@ cd {{ source_dir }} && gh pr list -R {{ owner }}/{{ repo }} --limit 20 - **⚠️ A CONFLICTING fork PR also gets zero CI checks** (#716 lesson: `mergeable: CONFLICTING` / `mergeable_state: dirty` → GitHub refuses to run CI for a dirty PR; `gh workflow run` cannot target fork refs, close/reopen does NOT re-fire checks for dirty PRs). Unblock path: check `maintainer_can_modify: true`, fetch `refs/pull/N/head`, create a local branch, `git merge master`, resolve conflicts, `git push :` — the `pull_request` synchronize event then fires CI. Post a comment explaining the maintainer push. Never ask the author to rebase blindly when you can resolve the conflict yourself as Committer. - Local verification (pytest + npm test) is necessary but NOT sufficient — CI is the only place the actionlint gate (#444) and the full doc-count guard (#511) run - Check merge conditions: does the PR's comment history already have 3 consecutive ✅ from different cycles with no ❌ in between? + - ⚡ **Ask the instrument instead of counting the ✅ lines by eye**: `scripts/check-vote-count.py ` is the reading of that condition, and the comment history misleads in both directions. It counts per *cycle* (one cycle voting twice is one vote), a ❌ resets the run, and it carries a clause the line above does not spell out — **a vote submitted before the head push is void** — so a branch refreshed after its approvals still shows several "✅ LGTM" while it has far fewer counting votes. Its companion `scripts/check-merge-freshness.py ` asks the other half: is that green CI still about the tree the merge would produce? + - ⚡ **When a stale head has votes at risk, do not refresh it just to make it fresh**: a push moves the head and voids every vote standing on it. Measure the tree the merge would land instead (`scripts/check-merge-plan-suite.py `) and cast the vote on that — the head does not move, so the standing votes stay valid. A refresh is `git merge master` into the branch, then a push; a rebase cannot be published here (the push is refused, and the force-push it would need is forbidden). - ⚠️ Query comments with the REST API (GraphQL needs `read:org` scope, often missing from the token): `gh api repos/{{ owner }}/{{ repo }}/issues//comments --jq '.[] | "\(.user.login): \(.body)"'` and `gh api repos/{{ owner }}/{{ repo }}/pulls//reviews --jq '.[] | "\(.user.login) [\(.state)]: \(.body)"'` @@ -371,7 +373,7 @@ git push origin feature/ gh pr create -R {{ owner }}/{{ repo }} --title "emrg: " --body "brief description of changes and reasons" ``` -**Merge condition**: the PR's comment history must have at least **3 consecutive ✅ LGTMs from different evolution cycles** with no `❌ needs fix` in between, before a Committer may run `gh pr merge --squash`. +**Merge condition**: the PR's comment history must have at least **3 consecutive ✅ LGTMs from different evolution cycles** with no `❌ needs fix` in between, before a Committer may run `gh pr merge --squash`. `scripts/check-vote-count.py ` is the reading of that condition — it counts per cycle, and a vote submitted before the head push is void — so ask it rather than counting the ✅ lines; `scripts/check-merge-freshness.py ` says whether the green CI is still about the tree that would land. **Not pushing = not done**. diff --git a/tests/test_evolution_prompt_merge_condition.py b/tests/test_evolution_prompt_merge_condition.py new file mode 100644 index 00000000..676b49f5 --- /dev/null +++ b/tests/test_evolution_prompt_merge_condition.py @@ -0,0 +1,241 @@ +"""The shipped template's merge condition must name the rule the counter enforces. + +Why this file exists +-------------------- +The template states the merge condition as *"3 consecutive ✅ LGTMs from different +cycles with no ❌ in between"* and tells the cycle to read it off the PR's comment +history. `scripts/check-vote-count.py` exists because that reading is wrong, and its +own docstring carries the measurement (2026-09-11: #1133/#1134/#1136/#1137 each +showed 4-6 "✅ LGTM" lines and each had **0 valid votes**). Its condition has a clause +the template's sentence did not: **a vote submitted before the head push is void**. +So an instance whose task prompt is built from the shipped template was told a rule +narrower than the one this project's gates measure, and pointed at a route (count the +✅ lines by hand) that answers wrongly whenever a head has moved. + +A sibling defect — the vote *body* — was fixed the same way (#1409): the template's +example now names the cycle the counter reads. This file is the other half of that +contract, the *condition* rather than the body, pinned where a cycle reads it +(§1.1 Step 1's "Check merge conditions" and §5's "Merge condition"). + +Why the pin renders instead of grepping the template +---------------------------------------------------- +The template is rendered with ``undefined=jinja2.Undefined``, so a name that is not +in the builder's context renders as the empty string: text present in the file can be +absent from the prompt an instance receives, and a file-level grep would still pass. +These tests render through the *real* builder (``TaskHandler._build_evolution_prompt``) +and read the rendered prompt. + +The control +----------- +A prose pin can only show that a sentence is present, so the last test drives the +counter itself over the same three approvals twice — once submitted after the head +push, once before it. That is what makes the phrase list below evidence rather than +decoration: the clause the template now states is one the instrument enforces. + +Named limit +----------- +This pins that the condition and its instrument are stated where the prompt is built. +It cannot pin that an agent then runs the tool, and it says nothing about *whether* a +vote should be cast — that is the abstain rule, issue #1408. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +import pytest +import yaml + +from emrg.protocol import InstanceIdentity +from emrg.server import scheduler as mod +from emrg.server.scheduler import TaskHandler + +REPO_ROOT = Path(__file__).resolve().parents[1] +PROMPTS_DIR = REPO_ROOT / "emrg" / "server" +COUNTER = REPO_ROOT / "scripts" / "check-vote-count.py" + +#: The instrument that reads the condition, and its two companions. Each is checked as +#: a verbatim substring of the shipped wording, so a rename of a script or a dropped +#: clause makes these tests say so instead of passing on a paraphrase. +COUNTER_SCRIPT = "scripts/check-vote-count.py" +FRESHNESS_SCRIPT = "scripts/check-merge-freshness.py" +PLAN_SUITE_SCRIPT = "scripts/check-merge-plan-suite.py" + +#: The clause the counter implements and the template's one-line condition omitted. +CLAUSE = "before the head push is void" + +#: The route a voter takes when a stale head has votes standing on it. Stated because +#: the tempting alternative — refresh the branch to make it fresh — is the one action +#: that destroys the votes it was meant to preserve. +STALE_ROUTE = "voids every vote standing on it" + +#: The two places the condition is read: deciding whether to vote (§1.1) and deciding +#: whether the merge may proceed (§5). Each is delimited rather than searched globally, +#: because the question is *where the sentence sits*: a clause in a section nobody +#: reads before voting does not answer the question the reader has. +STEP1_START = "- Check merge conditions" +STEP1_END = "**Issue management**" +SUBMIT_START = "**Merge condition**" +SUBMIT_END = "**Not pushing = not done**" + + +def _load_counter(): + """`scripts/check-vote-count.py` (hyphenated, so not importable by name).""" + spec = importlib.util.spec_from_file_location("check_vote_count_merge_condition", COUNTER) + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def counter(): + return _load_counter() + + +@pytest.fixture(scope="module") +def rendered(tmp_path_factory) -> str: + """The real template, rendered through the real builder with the real context.""" + tmp_path = tmp_path_factory.mktemp("merge-condition") + project_dir = tmp_path / "demoproj" + project_dir.mkdir(exist_ok=True) + (tmp_path / "projects.yml").write_text( + yaml.safe_dump([{"name": "demoproj", "path": str(project_dir)}]), encoding="utf-8" + ) + original = mod.config_dir + mod.config_dir = lambda: tmp_path + try: + handler = TaskHandler( + name="demo-task", + config={"project": "demoproj"}, + interval=300, + identity=InstanceIdentity(), + template_path=PROMPTS_DIR / "evolution_prompt.md", + ) + return handler._build_evolution_prompt() + finally: + mod.config_dir = original + + +def _region(text: str, start: str, end: str, what: str) -> str: + """The shipped text between two anchors, or a failure to measure — never a pass.""" + i = text.find(start) + j = text.find(end) + assert i != -1, ( + f"the {what} anchor {start!r} is gone from the rendered prompt — this test " + "cannot measure where the condition is stated, which is a failure to measure, " + "not a pass" + ) + assert j > i, ( + f"the {what} region's end anchor {end!r} no longer follows {start!r} — the " + "prompt was restructured, so the region this test reads is no longer the one " + "it describes" + ) + return text[i:j] + + +def test_the_condition_names_the_instrument_that_reads_it(rendered: str) -> None: + """Both sites name the counter, so a cycle asks it instead of counting by eye.""" + for start, end, what in ( + (STEP1_START, STEP1_END, "§1.1 review"), + (SUBMIT_START, SUBMIT_END, "§5 submit"), + ): + region = _region(rendered, start, end, what) + assert COUNTER_SCRIPT in region, ( + f"{what}: the merge condition is stated without naming {COUNTER_SCRIPT}, " + "the instrument whose reading of it is the one this repo's gates use — so " + "a reader has only the comment history, which the counter was written " + "because that misleads" + ) + + +def test_the_stale_head_route_is_stated_where_a_voter_decides(rendered: str) -> None: + """§1.1 must carry the clause, the freshness question and the route to take.""" + region = _region(rendered, STEP1_START, STEP1_END, "§1.1 review") + for term in (CLAUSE, FRESHNESS_SCRIPT, PLAN_SUITE_SCRIPT, STALE_ROUTE): + assert term in region, ( + f"§1.1 no longer states {term!r} beside the merge condition. This is the " + "section a cycle reads when it decides whether to vote, and the fact it " + "decides on is whether its vote will count: a vote submitted before the " + "head push is void, so a stale head is measured on the tree it would land " + "rather than refreshed (a push voids the votes it was meant to preserve)" + ) + + +def test_the_submit_section_states_the_clause_too(rendered: str) -> None: + """§5's one-line condition must carry the clause its sentence used to omit.""" + region = _region(rendered, SUBMIT_START, SUBMIT_END, "§5 submit") + for term in (CLAUSE, FRESHNESS_SCRIPT): + assert term in region, ( + f"§5's Merge condition no longer carries {term!r}. The sentence alone reads " + "as 'three ✅ in the comment history', which is the narrower rule the " + "counter's docstring measures four PRs against (4-6 approvals, 0 valid " + "votes each)" + ) + + +HEAD = "b" * 40 +PUSH = "2026-09-19T01:00:00Z" +AFTER = "2026-09-19T02:00:00Z" +BEFORE = "2026-09-19T00:00:00Z" + + +class _FakeGh: + """Canned answers for the three calls `check_pr` makes; nothing leaves the process.""" + + def __init__(self, reviews: list[dict]) -> None: + self.reviews = reviews + + def __call__(self, args: list[str]) -> object: + if args[:2] == ["pr", "view"]: + return { + "number": 1, + "title": "t", + "headRefOid": HEAD, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + } + assert args and args[0] == "api", args + return {"t": PUSH} + + def paginated(self, args: list[str]) -> list: + assert "/reviews" in " ".join(args), args + return self.reviews + + +def _counted(counter, monkeypatch, at: str) -> "object": + reviews = [ + {"at": at, "body": f"\u2705 LGTM — cycle cyc20260919-00000{i}"} for i in range(1, 4) + ] + fake = _FakeGh(reviews) + monkeypatch.setattr(counter, "_gh_json", fake) + monkeypatch.setattr(counter, "_gh_json_paginated", fake.paginated) + return counter.check_pr(1, counter.DEFAULT_MIN_VOTES, mergeability_wait=0.0) + + +def test_the_counter_voids_the_vote_the_clause_names(counter, monkeypatch) -> None: + """The control: the same three approvals count after the push, and not before. + + Without this, the phrases above could be satisfied by sentences describing a rule + nothing enforces. Both directions are shown because only one of them is the + failure: the votes are identical, the head push is the only difference, and the + difference between 3/3 and 0/3 is what the template now tells a reader to ask + about instead of counting. + """ + after = _counted(counter, monkeypatch, AFTER) + assert after.valid_count == 3, ( + f"three approvals submitted after the head push counted {after.valid_count} — " + "the control needs this direction green before the other one means anything" + ) + + before = _counted(counter, monkeypatch, BEFORE) + assert before.valid_count == 0, ( + "the same three approvals submitted *before* the head push counted " + f"{before.valid_count}: the clause the template states is not the one the " + "instrument applies, so the pin above would be pinning prose" + ) + assert all("submitted before the head push" in v.why for v in before.votes), ( + [v.why for v in before.votes] + )