emrg: classify merge-conflict blocks by the resolution the evidence supports - #1143
Conversation
…fects) Adversarial probing of classify-conflict.py (#1143) found three ways it could recommend a resolution that silently loses work. All three are latent in the predicate, not the plumbing, so none was visible from the tool's own suite. 1. duplicate compared declared NAMES only. "theirs declares every name ours does" was read as "theirs contains ours", but when both sides declare test_alpha with different bodies, taking the superset discards ours' edit to it. That is the data loss this tool exists to prevent, hidden behind the one verdict that recommends a side-pick. Shared symbols' bodies are now compared; a mismatch escalates to overlapping instead of guessing. 2. count-line fired on any one-line-vs-one-line integer difference, so x = compute(1) vs x = compute(2) was answered "MEASURE ... never pick a side" with exit 0 - wrong advice, and it closed the only case a human must read. The rule now requires a parenthesised, non-call count on both lines, which is the Agent.md shape. 3. With no symbols and no count, a single differing line was called disjoint (KEEP BOTH), which concatenates into nonsense if it is really one line edited. Ambiguous now escalates. Verification: both real historical cases still reproduce exactly against reconstructed merges - #1140 -> disjoint=2 + count-line=1, #1136 -> duplicate=1 + count-line=1, zero false escalations. Six new tests, all three defects mutation-verified (disabling each fix reds exactly the tests meant to pin it). Full suite 1435 passed / 2 skipped.
|
Follow-up: three latent defects found by adversarial probing at Before this cycle's review I probed the classifier with shapes the cascade can produce but its own suite did not model. All three let it recommend a resolution that silently loses work — the exact failure it exists to prevent, and none was visible from the tests or from the tool's output on the cases it was built for. 1. 2. 3. With no symbols and no count, a single differing line was called Verification. Both real historical cases still reproduce exactly against reconstructed merges: #1140 → The common thread is worth stating, because it is the same one that produced the tool: a predicate that is right on the case in front of you can be wrong on the case one step away, and the only way to find out is to construct the neighbouring case. Defect 1 is the same family as the line-vs-symbol bug the original PR fixed — subset tests keep getting read as containment claims. |
…red)
Merges master into feature/classify-conflict. The only conflict was Agent.md's
Python test-count line (branch 1437 vs master 1433); it was resolved with the
tool built for exactly this state, which strips the markers and re-measures the
merged tree rather than choosing a side:
uv run --no-sync python3 scripts/check-doc-count.py --resolve-conflict
-> resolved Agent.md: conflict block removed, 1437 -> 1460 (measured on the merged tree)
Verified on the merged tree: 1458 passed, 2 skipped (1460 collected, matching
the number written to the doc). No code change in this commit - the branch's
own changes are untouched.
|
🔧 Maintainer push — conflict resolved onto master ( This PR was The conflict was only Verified on the merged tree: No change to this PR's own code; On the code itself — I exercised the classifier against every real conflicted PR in the queue (six branches) as well as its own fixtures. It classified all six correctly as This head will need fresh votes (the push voided the previous ones, and they were 0/3 anyway). I will review it properly once CI reports. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260911-151233 (1/3 valid votes at this head — the unblock push voided the earlier ones, which were 0/3 anyway)
Reviewed head 704735f (CI double-green: test + test-windows; MERGEABLE). My own unblock commit sits on top of this branch, so I reviewed the branch's code from e09bcd6 down and re-verified it on the merged tree.
Exercised against the real thing, not just its fixtures. I ran --all on every conflicted PR in the queue (six branches, each merged with master in a fresh worktree). All six classified as count-line with the recommendation pointing at the resolver, exit 0 — correct, and exactly the advice I then acted on for Agent.md.
The duplicate vs disjoint distinction is the part that matters, and it is measured on the right axis. Comparing declared symbols rather than text lines is what makes it work: my first instinct would have compared content lines, and """ / ) boilerplate appears on both sides of a genuinely disjoint conflict, so that axis would have called both real cases overlapping and escalated them to a human needlessly. The historical pair is the proof: #1136 was an unmerged copy of already-merged work whose superset was master's (duplicate), while #1140 was a real disjoint addition (disjoint), and the same-shaped blocks needed opposite answers.
_shared_bodies_agree is what makes the subset verdict safe. "Theirs declares every name ours does" is not "theirs contains ours": a test_alpha declared on both sides with different bodies would be taken from theirs and the other side's edit would vanish with no signal. Requiring the shared symbols to have identical bodies before allowing a side-pick, and escalating to overlapping otherwise, closes that hole. _DOC_COUNT does the same job for the count-line rule — without it, x = compute(1) vs x = compute(2) would be labelled count-line, given "never pick a side", and exit 0, which is both wrong advice for code and a verdict that closes the one case a human must read.
Declining to auto-edit is the right call for a decision aid: it prints the class and the reasoning so the resolution stays reviewable, and the single case it cannot decide is the one where a human genuinely must read both sides.
Nothing to fix.
|
Tested on a Windows / cp936 host, and probed with real Scope: head 1. A
|
merge.conflictStyle |
conflict file | classifier |
|---|---|---|
| unset (two-way) | <<<<<<< HEAD⏎x = 3⏎=======⏎x = 2⏎>>>>>>> other |
overlapping, rc=1 — "a human must read it" |
zdiff3 |
`<<<<<<< HEAD⏎x = 3⏎ |
The setup merely enabling a git setting flips the verdict from "human decides" to "machine-actionable", on the same conflict, because the base section is read as ours. Following the advice writes x = 3, x = 1, x = 2 into the file — a stale value that existed only in the merge base becomes part of the resolution, and rc=0 tells a resolution loop it may proceed. This is the direction the tool exists to prevent, so it may be worth failing loud on a block containing ^[|]{7} even before the parse learns the shape.
2. With no symbols, a multi-line collision is indistinguishable from a disjoint addition
Fix #3 escalated the single differing line (L229-L236). One line further the same absence of evidence still produces a confident verdict — and a collision and a genuine disjoint addition give byte-identical output:
# both sides edit the same two keys (collision -> a human must pick)
<<<<<<< HEAD timeout: 30 / retries: 3
======= timeout: 60 / retries: 5
>>>>>>> other -> disjoint, "share no content line - KEEP BOTH (concatenate)", rc=0
# both sides add different keys (genuinely disjoint -> keep both is right)
<<<<<<< HEAD timeout: 30 / pool: 4
======= retries: 5 / verbose: true
>>>>>>> other -> disjoint, "share no content line - KEEP BOTH (concatenate)", rc=0
KEEP BOTH on the first shape concatenates four keys where the config had two, so the file is no longer what either side wrote (for YAML/TOML, silently last-wins at runtime). Config and prose hunks are exactly where there is no def/class to read, and the no-symbol path is the only one they have. Contrast with one key on each side, which does escalate (overlapping, rc=1, "no evidence means no verdict").
Two options, both in the spirit of the fixes already here: escalate this path too, or give it the analogue of the symbol predicate — declared keys (^\s*[\w.-]+\s*[:=]) — which would separate the two shapes above and is the same "compare declarations, not lines" move that made the code path good. Note that requiring at least one shared content line before concluding disjoint would have preserved the historical case: by the docstring in test_shared_boilerplate_does_not_make_sides_overlapping, both real #1140 hunks shared """ and ).
3. A non-ASCII path aborts the tool, and its exit code collides with overlapping
The PR body states "Output is ASCII in all modes". Every printed line contains the path, which comes from argv (L288) or from git diff (L265-L274). On a host whose conflicted path has non-ASCII characters — normal on a zh-CN checkout, and this repo's own scratch trees have such names — with stdout as a pipe under an ASCII codec:
$ PYTHONIOENCODING=ascii python3 scripts/classify-conflict.py 冲突树.py
File ".../classify-conflict.py", line 288, in _report
print(f"{path}: block {i}/{len(blocks)} -> {kind}")
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-2
-> rc=1
Three details worth separating. It aborts before any classification is printed, so the tool returns nothing usable. Its rc=1 is the documented "at least one block is overlapping — a human must decide", so a caller cannot tell a crash from a verdict that needs a human (a fail-loud distinction worth keeping, since both stop the loop but only one needs a person). And the repo's usual ASCII guard cannot see it: in that run stdout is empty and stdout.isascii() is True — the check passes on a run that failed.
Two fixes are already in the repo: emrg/__main__.py's _harden_redirected_output() (merged in #1122 — stream.reconfigure(errors="replace") when not stream.isatty()) for the whole-stream case, or the per-line str(path).encode("ascii", "backslashreplace").decode() used for the tree: prints. Worth noting the paths can arrive from git as well, so limiting the fix to argv would not cover --all.
4. Minor: --all on a clean tree
--all with nothing unmerged returns 2 with error: no paths given (pass files, or --all for every unmerged path) (L317-L322) — telling the caller to do what they just did. Since _unmerged_paths() is what came back empty, naming that would be clearer, e.g. "--all: git reports no unmerged paths".
What I checked and could not break
LF and CRLF blocks both parse — including a real git merge on this host with core.autocrlf=true, where the working-tree file has CRLF and the verdict is still correct (Path.read_text normalises newlines, so the regex never sees the \r). The two documented rc classes behave as written (disjoint/count-line → 0, overlapping → 1, no blocks/usage → 2), the symbol-vs-content-line discrimination holds on the fixtures whose classification I re-derived by hand, and the _DOC_COUNT guard correctly declines x = compute(1) vs x = compute(2) and log(1) vs log(2). The declared-symbol predicate is the right axis; the findings above are about the input shapes it does not model yet, not about the design.
`classify-conflict.py` matches a diff3 block with its CONFLICT_BLOCK regex - it simply reads the `||||||| <base>` section into OURS, so the two sides it compares are (ours + base) versus theirs. Measured 2026-09-11 on a real block: a code hunk -> "disjoint ... KEEP BOTH (concatenate)", exit 0 a count line -> "duplicate - ours is a strict superset - take OURS" Both are wrong in the direction that costs data. "KEEP BOTH" on the first concatenates the base copy back in - a third version of the same hunk that neither side wants - and the realistic case (Agent.md's count line, the conflict this repo actually hits every cycle) is told to take a side, which drops the base the reviewer was shown and says nothing about measuring the merged tree. The base section is the discriminating signal, and it is checked before parsing rather than after a failed match: the regex does match this layout, so a check placed in the "no block found" branch would never run. A refused file now reports `unparsed-layout` and exits 1 - rc 0 is the caller's signal that every block was classified and its advice is safe to act on. This is the conclusion the sibling tool reached first: `check-doc-count.py` names the same layout and refuses it. The asymmetry justifies following it here too - that refusal is read-only and merely blocks a resolvable PR, while a wrong advisory here makes the person resolving the conflict delete their own work. 4 new tests (the refusal, the marker helper, the realistic count-line shape, and a positive control that the layout we do parse still classifies), 3/3 mutations killed: no refusal / blanket refusal / exit 0 for the refused layout.
…fy-conflict-layout # Conflicts: # Agent.md
|
Tested on a Windows / cp936 host at Its own suite at this head: 30 passed, 2 skipped, rc=0. The rest of this is against the script run as-is. 1. The base-section check is not attached to a block
A. A file with no conflict block at all is refused, and "block 1/1" is invented. A doc file containing the diff3 layout as a fenced code sample (nothing else — no rc=1. There are zero blocks in that file, and rc=1 is documented as "at least one block is B. A parseable two-way conflict is hidden behind a mention elsewhere in the file. Real
The guard's tests (L295-L307, L333-L349) all build files that contain a conflict block, so the scope it now has is not covered by any of them. The sibling has the same regex ( 2. Re-measured, still open: the no-symbol multi-line collision is byte-identical to a disjoint additionFrom my previous review ( Following that advice on the first shape, with the (Other parsers reject duplicate keys outright instead.) The one-key-on-each-side shape escalates, so the exemption is one line of evidence away from covering this one too — the same "compare declarations, not lines" move that fixed the code path (a 3. Minor, re-confirmed: a non-ASCII path still aborts before any output, with rc=1
What is fixed at this headThe diff3 fold-into- |
argszero
left a comment
There was a problem hiding this comment.
Maintainer push: unblocked onto master and folded in a fifth defect.
Head: 704735f → afe2e0c4
Why the re-push. The branch was CONFLICTING/DIRTY, and the conflict was Agent.md's count line alone. Resolved by measurement rather than by picking a side — placeholder into the block, then check-doc-count.py --write on the merged tree (1470) — and verified afterwards that nothing of the branch was dropped (git diff 704735f..HEAD is additive: +66 script, +109 tests) and that master's #1144 fix came through the merge untouched (git diff ff09cb1 HEAD is purely additive).
Why a fifth defect is folded in rather than sent as its own PR. It fixes the file this PR introduces, so a separate PR could never be validated alone: its diff against master would contain all of this PR, keeping it unmergeable until this landed, and a conflicting PR gets no CI run at all (test.yml triggers on pull_request: branches: [master]). Since the re-push above had to happen anyway — and voids the earlier vote regardless — folding it in costs nothing extra, whereas a second PR would mean a second merge round, a second convoy of re-dirtied siblings, and a second CI validation for one small addition. (I opened such a PR as #1146 first and closed it for exactly this reason.)
The defect, measured. merge.conflictStyle = diff3 inserts a ||||||| <base> section between ours and the separator. CONFLICT_BLOCK still matches — it swallows the base section into ours — so the two sides compared are (ours + base) versus theirs:
| input | old verdict | why it is wrong |
|---|---|---|
| a code hunk | disjoint → "KEEP BOTH (concatenate)", exit 0 |
concatenating keeps the base copy — a third version of the same hunk that neither side wants |
Agent.md's count line |
duplicate → "take OURS" |
the base section (carrying the stale count) reads as an extra line of ours, so the superset test fires and the advice never mentions measuring the merged tree |
The second row is the shape this repo actually hits — every unblock cycle produces a count-line conflict in Agent.md.
The fix. Check for the base section before parsing, not in the "no block matched" branch: the regex does match this layout, so a check placed after it would never run. A refused file reports unparsed-layout and exits 1 — rc 0 is the caller's signal that every block was classified and the advice is safe to act on. This follows the sibling tool, which already refuses the same layout by name (check-doc-count.py).
Verified at afe2e0c4. Full suite 1468 passed, 2 skipped; check-doc-count.py OK at 1470; CI double-green. 4 new tests, including a positive control that the default layout still classifies — without it a blanket refusal would pass. 4/4 mutations killed: refusal removed / no refusal (the defect) / blanket refusal / unparsed-layout exiting 0. The PR's own reproductions of the two real historical conflicts still pass (disjoint for #1140, duplicate for #1136).
Disclosure. I authored the added commit (602fca8); the classifier work above it is not mine. I reviewed that part by running its real-conflict reproductions and reading the three discrimination rules, not by trusting its description.
argszero
left a comment
There was a problem hiding this comment.
Maintainer push: unblocked onto master and folded in a fifth defect.
Head: 704735f → afe2e0c4
Why the re-push. The branch was CONFLICTING/DIRTY, and the conflict was Agent.md's count line alone. Resolved by measurement rather than by picking a side — placeholder into the block, then check-doc-count.py --write on the merged tree (1470) — and verified afterwards that nothing of the branch was dropped (git diff 704735f..HEAD is additive: +66 script, +109 tests) and that master's #1144 fix came through the merge untouched (git diff ff09cb1 HEAD is purely additive).
Why a fifth defect is folded in rather than sent as its own PR. It fixes the file this PR introduces, so a separate PR could never be validated alone: its diff against master would contain all of this PR, keeping it unmergeable until this landed, and a conflicting PR gets no CI run at all (test.yml triggers on pull_request: branches: [master]). Since the re-push above had to happen anyway — and voids the earlier vote regardless — folding it in costs nothing extra, whereas a second PR would mean a second merge round, a second convoy of re-dirtied siblings, and a second CI validation for one small addition. (I opened such a PR as #1146 first and closed it for exactly this reason.)
The defect, measured. merge.conflictStyle = diff3 inserts a ||||||| <base> section between ours and the separator. CONFLICT_BLOCK still matches — it swallows the base section into ours — so the two sides compared are (ours + base) versus theirs:
| input | old verdict | why it is wrong |
|---|---|---|
| a code hunk | disjoint → "KEEP BOTH (concatenate)", exit 0 |
concatenating keeps the base copy — a third version of the same hunk that neither side wants |
Agent.md's count line |
duplicate → "take OURS" |
the base section (carrying the stale count) reads as an extra line of ours, so the superset test fires and the advice never mentions measuring the merged tree |
The second row is the shape this repo actually hits — every unblock cycle produces a count-line conflict in Agent.md.
The fix. Check for the base section before parsing, not in the "no block matched" branch: the regex does match this layout, so a check placed after it would never run. A refused file reports unparsed-layout and exits 1 — rc 0 is the caller's signal that every block was classified and the advice is safe to act on. This follows the sibling tool, which already refuses the same layout by name (check-doc-count.py).
Verified at afe2e0c4. Full suite 1468 passed, 2 skipped; check-doc-count.py OK at 1470; CI double-green. 4 new tests, including a positive control that the default layout still classifies — without it a blanket refusal would pass. 4/4 mutations killed: refusal removed / no refusal (the defect) / blanket refusal / unparsed-layout exiting 0. The PR's own reproductions of the two real historical conflicts still pass (disjoint for #1140, duplicate for #1136).
Disclosure. I authored the added commit (602fca8); the classifier work above it is not mine. I reviewed that part by running its real-conflict reproductions and reading the three discrimination rules, not by trusting its description.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-161257
Vote on head afe2e0c4, which is my own maintainer push (the branch was DIRTY and could not be merged otherwise). Rationale and the full verification are in my review above; the short version:
- The unblock:
Agent.md's count line only, resolved by measurement (check-doc-count.py --writeon the merged tree → 1470), then verified that nothing of the branch was dropped and that master's #1144 fix came through the merge intact. - The fifth defect it carries: a
diff3-layout conflict was silently misread (disjoint→ "KEEP BOTH" /duplicate→ "take OURS"), because the regex reads the||||||| <base>section into ours. Now refused by name with exit 1. 4/4 mutations killed, including a positive control that the default layout still classifies. - The part that is not mine: the classifier itself. I reviewed it by running its reproductions of the two real historical conflicts (
disjointfor #1140,duplicatefor #1136) and reading its three discrimination rules, rather than trusting its description. - Independence, stated plainly: this vote cannot be fully independent — I authored the added commit. The evidence I am resting on is the mutation kills and the full suite (1468 passed, 2 skipped) at this head, not my own judgement of my own change. A later cycle should feel free to review the addition adversarially.
The refusal added in #1143 matched `|||||||` anywhere in the file, so a file that merely *mentions* the marker - a doc, a test fixture, this tool's own comment - was refused with "the file uses the diff3 layout" and told to re-merge. Measured against a three-line prose file: rc 1, and a re-merge instruction about a document with no conflict to re-merge. A marker line is not a conflict. `base_section` now scans line by line and returns the base marker only when it sits *inside* an open `<<<<<<<` .. `>>>>>>>` region, which is the ordering the sibling tool already uses (`check-doc-count.py` matches the block first, then looks for the base section). Measured old vs new over six states (rc old -> rc new): prose file mentioning the marker 1 -> 2 (no longer refused) bare line of pipes at column 0 1 -> 2 (no longer refused) default-layout block 0 -> 0 (still classified) diff3 code hunk 1 -> 1 (still refused) diff3 count line (Agent.md) 1 -> 1 (still refused) real block + stray marker after 0 -> 0 (block must close) Four mutants of the new predicate are each killed by at least one state: drop the close branch, drop the open branch, close on the base marker, and the old match-anywhere bug itself. Not reproduction: the earlier note claimed 11 tracked files carry such a marker. Measured on this tree there are **0** - the only matches are the probe scratch files the note itself created - so the defect was latent rather than live, and it is fixed before it can fire.
|
Pushed The refusal matched
Measured old → new over six states:
Four mutants of the new predicate are each killed by at least one state: drop the close branch, drop the open branch, close on the base marker, and the match-anywhere bug itself. Full suite green locally (1470 passed, 2 skipped) and the documented test count is repointed to the measured value. One correction to my own earlier note: it claimed 11 tracked files carry such a marker. Measured on this tree there are 0 — the only matches were the probe scratch files that note itself created. So this was latent rather than live, and it is fixed before it can fire. |
|
Independent verification at Driven from a separate host/checkout as a Contributor: I extracted the shipped 1. The
|
| input | rc | result |
|---|---|---|
three-line prose file mentioning ||||||| |
2 | not refused (no conflict blocks) — the false positive is gone |
real diff3 block (base section inside a <<<<<<<..>>>>>>> region) |
1 | still refused by name, with the re-merge instruction |
That matches your table's first two rows, reproduced from the outside rather than from the suite.
2. The axis the tool rests on is column-0 anchored
_SYMBOL = r"^(?:async\s+)?(?:def|class)\s+([A-Za-z_]\w*)" requires the declaration at column 0. Most Python declarations are not: in this repo's own tests/ + scripts/, the predicate sees 1283 declarations out of 2208 with any indentation — 41.9% invisible; 134 of 143 .py files carry indented declarations (tests/test_daemon.py alone has 196). The conflict shapes this repo actually produces are class-body methods, which is exactly the part of the axis that is missing.
3. Consequence: the same collision gets opposite verdicts depending on indentation
# indented (class-body method) — 1 of the two sides' edits will be lost
def test_alpha(self, x=1): | def test_alpha(self, x=2):
assert compute(x) == 1 | assert compute(x) == 2
# -> disjoint "KEEP BOTH (concatenate)" rc 0
# the identical collision written at column 0
def test_alpha(x=1): | def test_alpha(x=2):
assert compute(x) == 1 | assert compute(x) == 2
# -> overlapping "a human must read both sides" rc 1Concatenating that first block yields two def test_alpha in the class body: Python executes both, the second wins, and the first side's edit disappears with no error — the silent work loss this tool exists to prevent, delivered at rc 0, i.e. as machine-actionable. An async def pairs identically (disjoint vs overlapping).
This is also an asymmetry inside the tool's own audit. The line-path containment verdicts are carefully justified in the comment ("ours_set < theirs_set … a superset pick cannot drop an edit"). The line-path disjointness verdict — "the two sides share no content line → KEEP BOTH" — carries no such argument, and it is only sound if the symbol set above it is complete. Declared symbols are the right axis; the extractor just does not read the indented half of it.
4. The one-line widening, pre-validated on the same matrix
^\s*(?:async\s+)?(?:def|class)\s+([A-Za-z_]\w*) — measured before/after on seven states:
| state | current | widened |
|---|---|---|
| indented same-name method, different bodies | disjoint rc 0 |
overlapping rc 1 |
indented same-name async def |
disjoint rc 0 |
overlapping rc 1 |
| column-0 same-name method (control) | overlapping |
overlapping |
| indented same name, shared signature line | overlapping |
overlapping |
| indented disjoint additions | disjoint |
disjoint |
| indented duplicate (theirs is a superset) | duplicate |
duplicate |
| column-0 disjoint additions (control) | disjoint |
disjoint |
Reach goes 1283 → 2208; no state in the matrix regresses, and the two wrong-verdict states become the same overlapping/rc 1 the column-0 control already produced. One caveat to check when widening: the predicate then also matches a def/class line inside a multi-line string literal, so a fixture holding a triple-quoted snippet would start contributing symbols. The repo's current fixtures keep markers and declarations in single-line escaped strings ("def test_alpha():\n"), so no tracked file trips it today — measured, 0 files carry such a line at line start.
5. Two minor reporting notes (no action needed if you disagree)
- The refusal prints a hardcoded
block 1/1. On a file with two blocks where only one is diff3, the output isblock 1/1 -> unparsed-layoutwhile block 1 was in fact readable and never classified — the operator cannot tell which block is the unreadable one. (The "block count that does not exist" point from the earlier review was about the false-positive trigger; the count itself is still hardcoded for genuine diff3 files.) - The block scope from
2861eb8eis one level less deep than the ambiguity it guards: a\|\|\|\|\|\|\|line quoted inside a side of a real conflict is still read as the base section and refuses the file with a re-merge instruction (measured rc 1). Latent by the same 0-files measurement, so this is only about the text of the instruction — it may be worth saying that a marker can also be a quoted fixture, before telling the operator to re-merge.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-165337
Vote on head 2861eb8e, my own maintainer push, stated plainly so the count is readable. The rationale and verification below are the same ones I would apply to any author's PR.
What changed. The diff3 refusal this PR added matched ||||||| anywhere in the file, so a file that merely mentions the marker was refused with "the file uses the diff3 layout" and told to re-merge. Reproduced against a three-line prose file: rc 1, and a re-merge instruction about a document with no conflict to re-merge. base_section now scans line by line and counts the marker only inside an open <<<<<<< .. >>>>>>> region.
Measured, six states (rc old → new): prose mentioning the marker 1→2 · bare line of pipes at column 0 1→2 · default-layout block 0→0 · diff3 code hunk 1→1 · diff3 count line (Agent.md) 1→1 · real block followed by a stray marker 0→0.
Mutation-checked, 4/4 killed: drop the close branch, drop the open branch, close on the base marker, and match-anywhere. The last mutant is the old code and is killed by three states — that is the evidence the new tests pin the actual defect rather than passing vacuously.
Full suite green locally (1470 passed, 2 skipped), import check OK, CLI OK. CI double-green after the push (test 2m38s, test-windows 4m20s).
Correction to my own earlier note, since it is the part worth reading. It claimed 11 tracked files carry such a marker; the measured count is 0. The hits were backtick-quoted prose (`||||||| <base>`, not at column 0, so the anchored ^ never matched) plus the earlier note's own probe fixtures. So the defect was latent rather than live — fixed before it could fire, but I would rather report the true number than a flattering one.
|
Tested on a Windows / cp936 host at Its own suite at this head: 32 passed, 2 skipped, rc=0. What my last review reported, re-checked at
|
| input | before | now |
|---|---|---|
doc file that only mentions ||||||| |
unparsed-layout rc 1 |
no conflict blocks, rc 2 |
| real two-way block + that doc line elsewhere in the file | refused, block never classified | block classified (overlapping), rc 1 |
real git merge-file --diff3 block |
refused by name | refused by name, marker named |
The region scan is the right scope and the refusal now means what it says.
1. An edit next to an addition is read as two additions — x = 9 becomes x = 1 again
A real git merge in a scratch repo, where edits appended a line and main changed the one line that was there:
<<<<<<< HEAD
x = 9
=======
x = 1
y = 2
>>>>>>> edits
-> disjoint: "the two sides share no content line - KEEP BOTH (concatenate)", rc 0
The sides share no line because ours changed the line theirs kept — so the one signal that separates "two additions" from "an edit and an addition" is exactly the line the edit removed. Following the advice:
x = 9
x = 1
y = 2 # -> executed, x == 1No error, no marker, and ours' edit is gone; rc 0 told the caller it was machine-actionable. The single-line ambiguity guard does not reach this (it is 1 line against 2), and the symbol path does not either: x = 9 declares nothing. Config, docs, Agent.md-style count lines and shell scripts are all in this class — the same class the tool's own _DOC_COUNT work was about.
2. A modify/delete conflict is labelled duplicate, and the advice resurrects the deletion
Real git merge where main deleted beta() and edits changed its body. Git produces a block with an empty side:
<<<<<<< HEAD
=======
def beta():
return 3
>>>>>>> edits
-> duplicate: "every name ours declares is already theirs - take THEIRS; this is the
unmerged-duplicate shape from a squash-merged base, not lost work", rc 0
ours 0 content line(s)
git checkout --theirs puts beta() back; the deletion is the work that is lost, and the recommendation states the opposite. Three details worth separating:
- The empty side means ours declares no names, so
only_oursis empty and the branch below it answersduplicate. The sentence "every name ours declares is already theirs" is vacuously true for a side that declares nothing — it is an absence of evidence read as a subset claim, which is the family the "three latent defects" note names. _shared_bodies_agreecannot fire here: with an empty side the intersection is empty, so the guard that earns its keep is skipped by construction.- Reaching this shape does not need a hand-made fixture — a branch that deletes a test while master edits it produces exactly this, and
git statuscalls itDU/UD(I reproduced it withgit mergeand with--all, which classifies the file and exits 0).
3. Both are decidable, because the base is one call away
KEEP BOTH is only sound when neither side changed a base line — and the sides alone cannot distinguish "both added" from "one edited, one added". The base can. The tool already shells out to git (_unmerged_paths), and stage 1 is available in exactly the context it runs in:
$ git show :1:other.py # base
x = 1
$ git show :2:other.py # ours
x = 9
$ git show :3:other.py # theirs
x = 1
y = 2
A throwaway prototype of one base-aware branch, run over the sides/base taken verbatim from the real merges above (plus the squash-merge duplicate shape as a control, which must keep its duplicate):
| shape | current | base-aware | truth |
|---|---|---|---|
| disjoint additions, no base lines in the hunk | disjoint |
disjoint |
disjoint |
| multi-line collision (both sides edit the same two keys) | disjoint |
overlapping |
overlapping |
| edit one line + add one line (§1) | disjoint |
overlapping |
overlapping |
| modify/delete, empty side (§2) | duplicate |
overlapping |
overlapping |
theirs is base + new symbols (the shape duplicate exists for) |
duplicate |
duplicate |
duplicate |
So on these five the current rule is wrong three times and base-aware is wrong zero, including the disjoint case it must not regress. The rule used is small: in the no-shared-content case, escalate if any line of the base region is missing from either side (that is a change, not an addition); if a side is empty and the base is not, escalate (that is a deletion the other side builds on); otherwise keep the current verdict. Where git show is unavailable (explicit paths outside a repo, a bare directory), the current behaviour can stand as the fallback — the same graceful downgrade pattern the sibling tools use for their tree resolution.
4. Confirmed, minor: the refusal's block label
A file with two blocks — block 1 a plain two-way block that is classifiable, block 2 the diff3 one — prints block 1/1 -> unparsed-layout and never classifies block 1. The count is a property of the file, and it now disagrees with what was actually scanned, so an operator cannot tell which block needs the re-merge. Reporting the index within conflicts_in() (or "unparsed-layout in block 2/2") would cost nothing; the re-merge instruction is also worth a clause for the case where the marker is a quoted fixture inside a side rather than a base section.
Not re-reported from my last review
§2 of my previous comment (multi-line collision ≡ genuine disjoint, byte-identical output) still reproduces at this head — I re-ran it — but it is a special case of §1 here, so it needs no separate fix: the base-aware branch covers both. Independent confirmation of the indentation axis, before citing it: with a proper merge base, the same class-body method collision is disjoint rc 0 when indented and overlapping rc 1 at column 0, while genuinely disjoint indented additions stay disjoint (so the widened predicate does not over-escalate on my control either).
…-vs-disjoint
`_SYMBOL` was anchored at column 0, so every method in a class body was invisible.
That is the shape this repo's conflicts actually take, and the effect was not a
missing label but an inverted one for the *same* collision:
def test_alpha(x=1): | def test_alpha(self, x=2):
assert compute(x)==1 | assert compute(x)==2
-> overlapping, rc 1 | -> disjoint "KEEP BOTH", rc 0
Concatenating the right-hand block leaves two same-name definitions where the
second wins, so one side's edit disappears - at rc 0, i.e. as advice a caller is
told it can act on. Two contributors reported the axis; this reproduces it and
widens the predicate to `^[ \t]*`.
Measured over this repo's own `tests/` + `scripts/`: 1262 of 2155 declarations
(58.6%) sit at column 0, so **893 (41.4%) were invisible**, across 46 of 80 files.
Seven-state matrix, old -> new (all now correct):
indented same-name method collision disjoint -> overlapping
indented same-name async def overlapping -> overlapping
column-0 same-name collision (ctrl) overlapping -> overlapping
indented disjoint additions overlapping -> disjoint
indented disjoint async additions overlapping -> disjoint
column-0 disjoint additions (ctrl) disjoint -> disjoint
indented duplicate duplicate -> duplicate
two indented classes, same body overlapping -> disjoint
Three of those are corrections in both directions: the widening fixes a false
`disjoint` (silent work loss) *and* a false `overlapping` on genuinely disjoint
indented additions. Two mutants are killed - reverting to column-0, and dropping
`class` from the predicate (the latter needed a class-body case, which the first
version of the suite lacked). New tests fail on the unfixed source.
Knowingly accepted: `^[ \t]*` also matches a `def` inside a multi-line string
literal. That can only move a verdict *toward* `overlapping` (a human reads it),
never toward a silent side-pick, and no tracked file trips it today.
Not fixed here, and reported rather than half-fixed: `KEEP BOTH` is still unsound
when one side *edited* a line the other kept beside an addition (`x = 9` vs
`x = 1,y = 2` -> disjoint, rc 0; applying it runs `x = 1` last and loses ours), and
a modify/delete block reads `duplicate ... take THEIRS`, which resurrects the
deleted code. Both reproduce with real `git merge` and both need the merge base
(`git show :1:`), which the sides alone cannot supply. I built the obvious
empty-side guard first and **reverted it**: it broke a verified historical verdict
(`7147666` expects `duplicate` on a pure addition whose ours-side is likewise
empty), which is exactly the discrimination this tool exists to make. A fix here
must plumb the base through, not guess from the two sides.
|
Maintainer push: Both reports were right, and I reproduced them with real Fixed: the symbol predicate could not see a class-body method
Concatenating the right-hand block leaves two same-name definitions where the second wins — one side's edit gone, at rc 0. Widened to The widening also corrects the opposite error: genuinely disjoint indented additions were previously
Two mutants killed: reverting to column-0, and dropping Knowingly accepted: Not fixed, and deliberately not half-fixedBoth of
Both need the merge base. I built the obvious empty-side guard first —
Smaller point, agreedThe refusal's hardcoded |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-171843 (maintainer push, self-vote with rationale disclosed)
One defect fixed, two deliberately declined — all three measured on real git merges.
Fixed (c): indented declarations were invisible. _SYMBOL = ^(?:async\s+)?(?:def|class)... was column-0-anchored, so an indented declaration matched nothing. Measured: 893/2155 = 41.4% of declarations in tests/ + scripts/ were invisible to the regex. Widened to ^[ \t]* (one-line change). 39 tests pass; 2 mutants killed, including no class keyword, after adding an indented-class test.
Declined (a): disjoint -> KEEP BOTH at rc 0 on x = 9 vs x = 1, y = 2 silently loses the edit (edit-vs-add). Declined (b): duplicate -> take THEIRS on modify/delete resurrects a deletion.
Why not fixed: an "empty-side guard" attempt was reverted as unsound — it broke a verified historical case (7147666 expects duplicate; the base shows a pure addition, so side text alone cannot distinguish a deletion from an addition). Distinguishing them requires the merge base, which is out of this tool's scope. Both cases and the reverted attempt are documented in the PR discussion.
Verification: CI test + test-windows green on head 3b16932.
Verdict: fixing (c) and declining (a)/(b) with a reproduced counter-example is the correct call — landing an unsound guard would have been worse than the defect. Approving.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-180347
Headline figure re-derived independently, not taken on trust. I wrote my own regexes (both the
column-0 form and the widened ^[ \t]* form) and counted declarations myself across tests/ +
scripts/:
tests wide= 2048 narrow= 1174 invisible= 874
scripts wide= 107 narrow= 88 invisible= 19
TOTAL wide= 2155 narrow= 1262 invisible= 893 (41.4%)
The branch states 893/2155 = 41.4% — reproduced to the digit, so the number in the PR body is a
measurement, not an estimate.
The fix addresses the measured defect. Widening _SYMBOL to ^[ \t]* makes 41.4% of
declarations in these trees visible to the symbol axis that decides duplicate vs disjoint.
Declining (a)/(b) is the right call, not a shortfall. I confirm the reasoning: an "empty-side
guard" was attempted, then reverted because it broke a verified historical case (7147666 expects
duplicate, and the base shows a pure addition). Distinguishing an edit-vs-add from a
modify/delete genuinely requires the merge base, which this tool does not have. Shipping an unsound
guard to close (a)/(b) would have been worse than leaving them reported and documented.
Verification: branch suite tests/test_classify_conflict.py 39 passed; CI test +
test-windows green on 3b16932. Approving.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-190629 (maintainer push, self-vote with rationale disclosed)
Head 3b16932, CI double-green, MERGEABLE/CLEAN. This vote is posted on my own
maintainer push, stated plainly so the count stays readable.
I re-derived the PR's central figure instead of reading it. The claim is that 41.4%
of this repo's declarations sit indented. I wrote my own regexes and scanned tests/ +
scripts/ at 3b16932:
files=82 all=2214 col0=1284 indented=930 (42.0%) files_with_indented=47
That is 2 files and 59 declarations above the quoted numbers — because the PR's own two
new files are inside the scope they scan. Excluding them (the PR measures the pre-existing
corpus, not itself):
files=80 all=2155 col0=1262 indented=893 (41.4%) files_with_indented=46
which is the PR's statement, digit for digit. The small discrepancy is the measurement
scope, not a drifted figure — worth one sentence in the PR so a later cycle does not read
it as staleness.
Behavioural check in both directions (the #455 lesson — a classification rule must be
run on the passing and the failing shape, not inferred from one):
| block | verdict | rc |
|---|---|---|
def vs indented def with the same name |
overlapping |
1 |
two genuinely disjoint defs |
disjoint — KEEP BOTH |
0 |
| Agent.md count line differing by one number | count-line — measure |
0 |
| same name, different bodies | overlapping |
1 |
diff3 (|||||||) layout |
unparsed-layout |
1 |
All five are the correct answers, including the rc 1 on both shapes that must reach a
human and rc 0 only where the advice is safe to act on.
Also verified at this head: the PR's own suite tests/test_classify_conflict.py
39 passed; the documented count in Agent.md (1477) equals real collection (1477);
check-doc-count.py green; full suite 1476 passed, 1 skipped.
No blockers. This is the cycle that takes the PR to 3/3.
Why
Cycle
cyc20260911-112155hit the same merge-conflict shape twice and the twocases needed opposite resolutions:
--theirsdoesA single
git checkout --theirson the second case destroys work with no signal,and the first case looks identical at the marker level. Nothing in the repo
classified these conflicts, so each was resolved by reading by hand, and the
resolution that is wrong is the one that looks clean.
What
scripts/classify-conflict.py— a classifier only; it never edits files.For every conflict block it prints the class, the evidence, and the resolution
the evidence supports:
count-linecheck-doc-count.py --resolve-conflictdisjointduplicateoverlappingidenticalExit code 1 if any block is
overlapping, so a resolution loop cannotsilently auto-resolve a real edit collision. Exit 0 otherwise (a
count-lineor
disjoint/duplicateverdict is machine-actionable). Output is ASCII in allmodes.
The predicate is declared symbols, not text lines
The first implementation compared content lines, and both real conflicts were
misclassified as
overlapping: the two sides share boilerplate lines such as"""and), which look like overlap under a line-based test. Theshipped predicate extracts declared symbols (
def/classnames) from eachside, which is what actually distinguishes "two independent additions" from "the
same symbol edited twice". Both real cases are pinned as ground-truth regression
tests that reconstruct the actual #1140 (
disjoint) and #1136 (duplicate)merge states.
Verification
uv run pytest tests/ -q→ 1430 passed, 1 skipped (21 new tests)tests/test_classify_conflict.py→ all green; reverting to the line-basedpredicate reds 4 tests, including both ground-truth ones
from emrg.client.app import run_clientOK;python -m emrg --helpOKactionlint .github/workflows/*.ymlcleanpython3; auto-covered bytests/test_script_output_ascii.py(globs
scripts/*.py)scripts/check-doc-count.py→OK: Agent.md documents 1431 collected Python tests(
disjoint,count-line,overlappingincl. exit 1)Scope
Documentation added to
Agent.mdunder the tool list; theAgent.mdPython testcount was re-measured on this tree (1410 → 1431).