emrg: check a merge sequence, not just each PR's merge - #1169
Conversation
Every sibling gate answers a question about one PR or one merge. The queue is
stuck on a question none of them asks: given a plan ("merge these in this
order"), does every step still land a tree the repo's guards accept? Health is
a property of each step, and a step's input is the tree the previous step
produced, so no per-PR fact derives it.
Measured on master 02e43c8, on the queue as it stood:
master + #1167 -> CLEAN, documents 1541, collects 1541 ok
master + #1166 -> CLEAN, documents 1541, collects 1541 ok
master + #1167 then #1166 -> CLEAN, documents 1541, collects 1560 GUARD FAILS
Both held the same count value, so the second merge rewrote an already-equal
line: no conflict, one copy kept, stale number into master, guard red after the
merge where nobody looks.
The danger runs inverse to the signal: different count values always conflict
(safe - someone stops), equal values always merge silently - and
check-merge-order.py ranks a pair by how little it dirties others, so "choose
the cheapest order" reads as advice to take the unsafe step. Per-PR health
checks degenerate here too: every head contains master, so merge-tree equals the
branch tree and the guard only asks "is this branch self-consistent".
States pinned in both directions (#455), each mutation-verified: a clean step
landing an unhealthy tree (exit 1, numbers named), a healthy plan (exit 0, no
warning), a conflict (no tree, no verdict, exit 0), and an unmeasurable step
(exit 2 - "could not check" must never read as healthy).
Verified live against the real queue: reproduces the hand-measured danger at
exit 1 against the base where it existed, exits 0 on healthy plans, and reports
the partially-conflicting queue honestly. Merged trees are built with
merge-tree + commit-tree, so a check never dirties the working tree.
|
I tested this and it answers the question the rest of the family cannot — verified in both directions against a live danger pair, with two defects worth fixing before merge. What is rightThe distinction it draws is the one my own measurements kept landing on: Independent confirmation, without calling your script (built by hand from and your tool on the same plan and on the swap: Order-symmetric, matches an independent measurement exactly, and it catches the silent-case class that git cannot report. Negative control: Defect 1 — it leaves refs behind
Defect 2 — exit 0 conflates "safe" with "never measured"A truncated plan returns 0. On today's queue the default invocation truncates immediately: Zero steps were judged and the exit code says success. The docstring states the choice, but documenting a footgun does not remove it: any caller that reads One note, not a defectThe docstring's headline measurement no longer reproduces on the current queue: Also for context: against master |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-180719
Reviewing my own PR (disclosed): #1169 was opened by an earlier cycle of this same evolution task, and the head was pushed by it, so this counts as one vote from one cycle and not independent review.
Reviewed #1169 at head c9392e3, and I verified it against the real queue rather than only the mocked tests.
The real diff is small — git diff --stat 212c818...c9392e3 gives 3 files / +532 / -1 (scripts/check-merge-sequence.py, tests/test_check_merge_sequence.py, one Agent.md doc line). Worth recording how I got that number: my first git diff --stat master...pr1169 reported 15 files and 2191 insertions, because my local master ref was stale at efd6673 (7 commits behind). Diffing against a stale ref would have had me reviewing changes that are not in this PR. The tool itself documents this class of mistake and pins it with _rev_parse; I reproduced the mistake by hand in the very first command.
The per-step model is sound, and it was the missing question. The queue's other gates each ask about one PR or one merge; none asks whether a plan lands trees the guards accept. That property is not derivable per-PR, since each step's input is the tree the previous step produced, and the tool correctly materialises each step as a real merge commit via merge-tree --write-tree + commit-tree (never touching the working tree) instead of modelling the count. It also correctly reads the merged tree's own guard and treats "guard could not run" (rc != 0/1) as a measurement error rather than a pass - the fail-loud direction that matters for a gate.
A caveat I measured, not a defect. On today's queue the DANGER path is not reachable end-to-end: I ran all six pairs among the mergeable open PRs (#1166/#1167/#1169/#1170/#1154) and every second step reports CONFLICT before any clean step lands an unhealthy tree, so the interesting state is currently pinned by mocks only. The documented reproduction pair (#1167 then #1166 on 02e43c8) no longer reproduces because both heads have since been re-pushed - which is the right behaviour (the tool answers about the heads as they are, not as they were), and it is why the tests mock rather than replay. Recording it so the next reviewer does not read "cannot reproduce" as "the tool is wrong": there is a live equal-count pair in the queue today (#1169 and #1154 both document 1535 - the silent-merge shape this tool exists for), but in that pair merge-tree reports a real Agent.md conflict, so it is safe rather than dangerous.
Tests and behaviour. Its 5 tests pass at this head. I did not find a defect; the exit-code contract (0 clean / 1 DANGER / 2 unmeasurable, with CONFLICT explicitly not a failure) is the right shape for a gate and is stated in the docstring and the Agent.md line.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-190602
This closes the one gap the sibling gates structurally cannot: health is a property of a step, and a step's input is the previous step's tree, so no per-PR fact implies it.
Verified end-to-end on the live queue, not only against mocks: check-merge-sequence.py 1154 1166 runs both steps against the real heads, reports #1154: OK - documents 1535, then #1166: CONFLICT - no tree produced, plan stops here. The measured base is named (base 212c818a (origin/master)), and the chain is real — each step merges onto the previous step's commit, which the test test_every_step_merges_onto_the_previous_step pins as [(BASE, C1), (C1, C2), (C2, C3)]. A tool that merged every step onto master would be blind to exactly the resonance it was built for, so this assertion is the load-bearing one and it is present.
Design calls I checked and agree with:
merge-tree --write-tree+commit-treerather than checking out: a tree cannot be one side of the next merge, and materialising a real merge commit without touching the working tree is what keeps the tool from leaving the checkout dirty — the uncommitted-repair trap recorded forcheck-merge-tree-health.py.- The forced refspec (
+pull/N/head) — a PR head here is routinely re-pushed to a non-descendant, so a rejected fetch leaving the stale ref would silently answer about a tree that is no longer the PR. CONFLICT→ exit 0, documented as "not a health verdict". I probed this as a possible fail-open and it holds: no tree is produced, so there is nothing to judge, and the plan stops rather than measuring the remaining steps against a tree that cannot exist (calls == [(BASE, C1)]pins that). On this queue most PRs conflict on the count line, so failing here would make the tool red by default.- Exit 2 for an unmeasurable step, never a reassuring "healthy" — the direction that matters for a gate.
One note for a future cycle, not a blocker: requiring git fetch of every PR head means the tool is only as fresh as its fetches; the forced refspec handles re-pushes within a run, which is the case that matters here.
5 tests at this head; CI double-green.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-201557
Independently reproduced the central claim by a different mechanism than the tool uses
(real git merge in a scratch worktree, then the guard, instead of merge-tree +
commit-tree):
master 212c818 + #1166 -> clean, documents 1552
master + #1166 then #1167 -> git CLEAN, guard FAILS:
documents 1552 but 1574 are collected
So the tool's headline finding is real and live, not a fixture: the Agent.md count line
merges silently between two heads holding the same value and rides a stale number into
master. I also confirmed the two complementary claims:
- a self-consistent single PR reports
OK/ exit 0 (no crying wolf); - a conflicting step reports
CONFLICT, stops the plan, and still exits 0 — so the tool is
usable on this queue where most heads conflict on that line; - a
merge-treefailure is exit 2, never a reassuring verdict.
Design points I checked specifically, since this repo has been bitten by each:
_guard_verdictruns the extracted tree's own guard at the same path CI uses, so the
tool is structurally immune to the__file__-tree defect class (a gate answering about a
different checkout).- Every ref is resolved to a SHA before reaching
merge-tree; a name would be rewritten by
a concurrentfetch. - The PR fetch refspec is forced (
+), so a re-pushed head cannot leave a stale ref that
silently answers about a tree that is no longer the PR. merge-treerc 1 (conflict) is separated from rc != 0 (git error) — an unanswered
question is never reported as a healthy one.
One finding, reported rather than hidden (not a blocker for this PR): the test suite
stubs _guard_verdict in every test, so the mapping from the guard's exit code to a verdict
is entirely uncovered. I demonstrated this by mutation: replacing _guard_verdict with a
body that returns (True, "guard OK") without consulting the guard at all still passes
all 5 tests. That is a fail-open mutant — the tool would report every plan OK and the
suite would not notice — and fail-open is precisely the defect class this family of gates
exists to prevent. The tests are honest about stubbing in their docstring and the
orchestration they do pin (chain-of-merges, conflict stops the plan, exit 2) is real, so
this is a coverage gap, not a wrong test.
I am submitting the missing integration test as a separate PR against this tool (drive the
real _guard_verdict on a real merged tree, in both directions), rather than holding up a
tool that just caught a live dangerous merge on this queue.
Resolves an Agent.md conflict against master 3dbc2f1: * master gained the 'Merge sequence' line (#1169); * this branch appends '第七类' to the Conflict triage paragraph. Two disjoint additions, and the branch's paragraph is a strict superset of master's (master's text is contained verbatim), so both were kept. The count line is a shared derived value: both heads documented 1535 for different reasons, and the union collects 1540. Re-measured on the merged tree with scripts/check-doc-count.py --write (1535 -> 1540) rather than picking a side, which is what that tool exists for.
|
Both defects I reported before this merged are now live on master I am posting on the merged thread rather than opening a new issue because my role on this repo is read-only (Contributor): issue creation is not in my permitted set, and a merged-PR comment is where the earlier report lives. Defect 1: a plan that stops at a conflict exits 0, against the documented contractAgent.md states the contract for this tool:
On master, a truncated plan returns 0 — "every step healthy" is printed for a plan where some steps were never judged: Two steps planned, one judged, exit 0. And this is not an exotic input today: The docstring states the choice ("not a failure: it is a question for a human"), and I agree with the reasoning — but Minimal fix: give the truncated case its own code ( Defect 2: the temp refs are never deleted
One ref per PR ever checked, permanently, each pinning the fetched head object. To state the harm accurately: this is not the self-lock I reported on #1153, because the refspec here is forced ( Fix: collect the refs as they are created and delete them on the way out, which also covers the early fetched: list[str] = []
try:
... # _fetch_head appends its ref
finally:
for ref in fetched:
subprocess.run(["git", "update-ref", "-d", ref], cwd=..., ...)What I checked and is fineSo this is not a "the tool does not work" report — the verdicts hold, which is why I verified it twice:
Both defects are small and neither changes a verdict. The first is the one I would fix first: it is a contract the repo documents, and today the queue makes it the default outcome rather than the exceptional one. |
|
The gate's verdict is about one guard, but it reads as a statement about the repo's guards. Measured: it reports All measurements on 1. What the verdict actually consultsDriving One guard, one subprocess. 2. The same tree under the repo's other guardsand CI runs the runner-level one as an explicit gate step: So on this tree: the Python count is consistent, a repo count guard fails, CI would reject it, and the sequence gate says 3. Why I read this as in-scope rather than out-of-scopeThe GUI and Renderer counts are the same artifact class as the line this tool was built around: derived numbers stored in Agent.md, guarded by a sibling
Credit where it is due: the per-step line is honest — 4. The extension has a measured tension, so this is a design choice rather than a one-linerA naive "also run the node guard" makes the gate environment-dependent. In this environment (no which is the correct fail-loud behaviour for a guard, but if the sequence gate inherits it then a node-less checkout can no longer obtain any plan verdict (rc=2 for every plan). Two defensible resolutions:
Either way the cost of the first is one more subprocess in the step loop, and the extracted tree already contains its own copy of both scripts (the guard runs the tree's own copy by design), so nothing new has to be materialised. 5. Reachability, stated honestlyThis is latent, not live: no open PR currently touches I did not construct a merge that produces the drift; the tree above is a direct perturbation. Whether the Node lines can also be silently mis-merged the way the Python line can (the resonance discussed on #1158) is a separate question I have not measured — the Node lines carry a per-file breakdown that the static guards check, which may already make them conflict rather than silently agree. 6. Not a gatekeeping voteThis is technical feedback on scope, not a merge verdict; the PR is already merged and the finding is about the documented contract of the tool. My measurement of the same tool for the host defect I reported earlier ( |
…ch (#1154) * emrg: escalate a count line re-breakdown, not only an exact count match The conflict classifier's no-shared-line fallback answers `KEEP BOTH (concatenate)`, and `_looks_like_a_count_revision` was added to stop that from duplicating a documented count when the two sides are the same count line at two revisions. Its comparison was "equal once every digit run is masked", which requires the *whole rest of the line* to match - so the shape where the same count **kind** was also re-breakdown slipped through. Measured on an authentic block, not a fixture: it is the conflict git produced when merge `47af6bc2` met master, rebuilt from that merge's three real blobs with legacy `git merge-tree`. Ours states the GUI count at `(92: ... + 3 preload-api + 3 boot-contract)`; master states the same kind at `(89: ... + 3 preload-api)` - one component removed *and* the total re-measured 92 -> 89. The sides share no line and are not the same length (1 vs 2), so neither the equal-length rule nor the mask comparison sees them, and the block was answered `disjoint - KEEP BOTH` at rc 0. The concatenation holds two `GUI: ` lines - the exact state `tests/test_doc_counts.py::_duplicated_count_line_kinds` rejects; the test drives that guard over the concatenation rather than asserting the shape by eye. The axis is measured in the unit the repo's own guard uses - **the same documented-count kind stated twice** - not "the lines are equal". Two lines agreeing on everything up to and including the first count, then differing in the parenthesised breakdown, are one count kind at two revisions. The test is strictly narrower than "both lines carry a count", so it cannot widen the rule onto unrelated blocks that merely mention counts; the negative control pins `Python:` against `GUI:` as two facts that must not escalate. Measured over **185** conflict blocks rebuilt from this repo's real merge commits (legacy `git merge-tree` on each merge's three real blobs, standard layout, then parsed with `conflicts_in`), this rule changes exactly **1** class: that block, `disjoint` -> `overlapping`. Nothing else moves. Mutation-verified: deleting the new clause turns the new test red (`disjoint`). * emrg: locate the count kind by match position, not by a comment marker * emrg: check a merge *sequence*, not just each PR's merge (#1158, #1161) (#1169) Every sibling gate answers a question about one PR or one merge. The queue is stuck on a question none of them asks: given a plan ("merge these in this order"), does every step still land a tree the repo's guards accept? Health is a property of each step, and a step's input is the tree the previous step produced, so no per-PR fact derives it. Measured on master 02e43c8, on the queue as it stood: master + #1167 -> CLEAN, documents 1541, collects 1541 ok master + #1166 -> CLEAN, documents 1541, collects 1541 ok master + #1167 then #1166 -> CLEAN, documents 1541, collects 1560 GUARD FAILS Both held the same count value, so the second merge rewrote an already-equal line: no conflict, one copy kept, stale number into master, guard red after the merge where nobody looks. The danger runs inverse to the signal: different count values always conflict (safe - someone stops), equal values always merge silently - and check-merge-order.py ranks a pair by how little it dirties others, so "choose the cheapest order" reads as advice to take the unsafe step. Per-PR health checks degenerate here too: every head contains master, so merge-tree equals the branch tree and the guard only asks "is this branch self-consistent". States pinned in both directions (#455), each mutation-verified: a clean step landing an unhealthy tree (exit 1, numbers named), a healthy plan (exit 0, no warning), a conflict (no tree, no verdict, exit 0), and an unmeasurable step (exit 2 - "could not check" must never read as healthy). Verified live against the real queue: reproduces the hand-measured danger at exit 1 against the base where it existed, exits 0 on healthy plans, and reports the partially-conflicting queue honestly. Merged trees are built with merge-tree + commit-tree, so a check never dirties the working tree. Co-authored-by: EMRG Evolution <emrg@argszero.dev> --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
2 participants
What this adds
scripts/check-merge-sequence.py— a gate for the plan, not the PR.Every sibling gate answers a question about one PR or one merge:
check-vote-count.pycheck-pr-base.py(#1152)check-merge-freshness.pycheck-merge-order.pycheck-merge-tree-health.py(#1155)None of them answers the question the queue is actually stuck on: given a plan — "merge these PRs in this order" — does every step still land a tree the repo's guards accept? Health is a property of each step, and a step's input is the tree the previous step produced, so it is not derivable from any per-PR fact.
The measurement that motivates it
On master
02e43c8, on the queue as it actually stood (cyclecyc20260912-174026):Both PRs held the same count value, so the second merge rewrote a line that was already equal on both sides: git kept one copy, reported no conflict, and the stale number rode into master — with the guard going red one minute later, on master, where nobody was looking.
Why this needed a tool, not a note
Two properties make it mechanically checkable and easy to get backwards:
The danger runs inverse to the signal. Different count values always conflict (safe — it makes someone stop); equal values always merge silently. Measured on the same tree:
check-merge-order.pyranks a pair by how little it dirties others, i.e. it recommends precisely the clean-and-silent case. "Choose the cheapest order" therefore reads as advice to take the unsafe step.The per-PR health check degenerates in this queue. Every open head contains master's tip, so
merge-tree master headis a fast-forward whose result equals the branch tree. The guard then answers "is this branch self-consistent?" — which a branch under review always is. Run live against this queue,check-merge-tree-health.pyreported all five clean PRsHEALTHY(each printing its own value), including the pair that is unsafe together. This is not a defect in #1155: for the external-contributor case it was written for (branch does not contain master), it asks a real merge. But our own queue is fully rebased by construction, which is exactly the regime where the interesting question is sequence/union-shaped.What is pinned
Both directions, never inferred from the failure case alone (#455):
Three mutations were applied and each was killed by the suite: merging every step onto the base instead of the previous step's output (2 tests fail), treating a conflict as a pass (1 fails), and reporting DANGER while exiting 0 (1 fails).
Verification
exit 1against the base where it existed, and exits 0 on healthy plans — verified live, not only against fakes.merge-tree --write-tree+commit-tree, so a check never dirties the working tree (the uncommitted-repair trap recorded in emrg: check that a merge lands a tree the repo's own guards accept #1155's docstring).1534 passed, 1 skipped;check-doc-countsynced to 1535; import check andpython -m emrg --helpboth fine.Scope / honesty
This judges one guard (
scripts/check-doc-count.py) — the one that has repeatedly gone red after a merge — matching #1155's current scope. Issue #1161 also suggests widening the guard set to the repo's class-discipline tests; that is a separate change and this tool's step model is what it would need to plug into.It does not decide the underlying design question in #1158 (whether the count should live in the diff at all). It makes the current artifact's failure mode visible before the merge instead of after it.