emrg: refuse the conflict layout this tool cannot read - #1146
Closed
argszero wants to merge 4 commits into
Closed
Conversation
added 4 commits
September 11, 2026 11:54
…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.
…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.
`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.
Owner
Author
Dependency (important)This PR stacks on #1143 and touches the tool that PR introduces. It is retargeted to Merge #1143 first, then this one. The diff here is a strict addition ( Until #1143 is merged, CI reports no checks: |
Owner
Author
|
Closing as superseded — the fix now lives on #1143's head ( Why folded in rather than kept separate: #1143 was DIRTY on Also concrete: this PR cannot be CI-validated on its own. Its diff against The change itself is unchanged in substance, and 4/4 mutations were killed on it in isolation before it was folded in:
Verification on the folded head |
argszero
pushed a commit
that referenced
this pull request
Sep 11, 2026
The later-line scan added here reads any line-opening mark as a *stated* verdict.
`_decorated_lines` strips backticks as decoration, so a mark inside a fenced code
block is indistinguishable from prose - and a review that *documents* a veto (a
reproduction snippet, a table of example verdicts) was classified as *stating*
one. Driven through this tool's own `check_pr`:
three approvals, then a review quoting a veto in a fence
master -> run 3 (the body is a comment, skipped)
head -> run 0 (a veto, so the run and every approval before it go)
That is the failure this PR exists to prevent, reached from the other side:
quoting the shape was the one way to void the run the tool protects. Found
independently by two outside contributors (how2how2how2-arch, pm25coder) and
reproduced here before accepting it - it is why this head was not merged at 2/3.
Fenced regions are now dropped from `_decorated_lines`. A mark counts only when
the *reviewer* states it; in a fence the mark opens its line but the body is
quoting. Nesting follows CommonMark: a fence closes only on the same character
with at least the opener's length, because this repo quotes ``` examples inside
```` blocks - a boolean toggle broke on exactly those bodies (measured on a real
review body on this PR). An unclosed fence is treated as prose, so a stray marker
can never hide a real veto and leave stale approvals live.
Measured over 309 real bodies (PRs #1110-#1146): the fence fix alone changes the
class of **1** body - the live regression above, back to the `comment` that
master gives it - and 0 others. Four mutants are killed: fence-blind (no
skipping), length-blind (`==` for the closing fence), unbalanced-hidden, and the
toggle version. Three new tests fail on the unfixed source and pass on this one.
Not adopted: a contribution on the PR also proposed scanning past a later ✅ and
answering `approve` for a stated ✅ below a prose intro. On the same 309 bodies
that widening flips 6 bodies - including 4 approvals into `comment`/`approve`
churn - for no demonstrated defect. Fence awareness is the half that is needed
either way, so only that half ships; the reasoning is recorded in the docstring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
classify-conflict.py(PR #1143) misreads amerge.conflictStyle = diff3conflict block.CONFLICT_BLOCKstill matches it — it swallows the||||||| <base>section into ours, so the two sides it compares are(ours + base)versustheirs.Measured on this host with a real block:
disjoint→ "KEEP BOTH (concatenate)", exit 0duplicate→ "take OURS"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-layoutand exits 1, because 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:
check-doc-count.pynames the same layout and refuses it. The asymmetry justifies the same choice here — that refusal is read-only and merely blocks a resolvable PR, whereas wrong advice here makes the person resolving the conflict delete their own work.Verification
KEEP BOTHabsent), the marker helper in both states, the realistic count-line shape, and a positive control that the default layout still classifies — without it a blanket refusal would pass.disjointfor emrg: measure the checkout you are standing in, not the one the script lives in #1140's branch,duplicatefor emrg: extend the locale-decode guard into the emrg/ package and fix the path readers it found #1136's).check-doc-count.pyre-measured (1460 → 1465) after the new tests.Stacked on #1143's branch (it modifies the tool that PR introduces).