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.
argszero
added a commit
that referenced
this pull request
Sep 13, 2026
…#1145) * emrg: read a veto stated below a prose intro, not just the first line `_classify` read the verdict only from the body's first content line. A veto whose mark sits *below* a prose intro therefore classified as `comment`: Checked all three fixes. ❌ Needs fix: the third one still leaks and `check_pr` skips comments outright, so the **run was never reset** - the stale approvals in front of it still read as live and the tool would call a PR mergeable on reviews a veto had already answered. That is the same dangerous direction the decorated-mark fix (#1144) closed, reached by a different route: the mark is not decorated here, it is simply not on line one. Found in cyc20260911-153707 by probing `_classify` itself, after that PR had already been merged on three votes - the shape was not covered by its tests, and `master` misreads it identically, so this is a follow-up rather than a catch. The first line still decides whenever it states anything, so #1144's precedence is untouched. Only when the first line states no verdict at all (a prose sentence - neither a mark nor a claim about LGTM) do the later lines get scanned, and only a **stated** veto counts: the mark must open its line. That keeps this repo's approvals, which routinely *describe* a veto they resolved ("The earlier ❌ was resolved by pushing the fix myself"), classified as approvals - the opposite error would reset the run and discard every approval before it. A later line that states the other verdict stops the scan entirely. Measured: 0 of 381 real bodies on the 60 most recent PRs change class (237 approve / 10 veto / 134 comment, identical to master), so there is no regression on real data; 7 real bodies do carry a veto mark below line one and all 7 are approvals discussing a resolved veto. The hole is latent, not live - closed because "latent" and "safe" are not the same claim. Also pins that an unattributable veto (no cycle id) still resets the run: not being able to *number* a veto does not make it not a veto. 4 new tests, 3 mutations of the new rule all killed. Doc count 1438 -> 1442 by measurement. * emrg: read a quoted veto as a quotation, not as a stated one 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. * emrg: the unbalanced-fence fallback covers the tail, not the whole body `_fence_flags` returned `[False] * len(lines)` when any opener was unmatched, which re-opened every fence in the body - including regions that closed properly and are therefore not ambiguous. A body that quotes a veto inside a *closed* fence and later leaves one stray opener came back as a *stated* veto, and since a veto resets the run (with no cycle-id check), a legitimate three-approval run read 0/3. Measured on the shape the contributor reported (how2how2how2-arch on #1145): Reviewed on Windows. ``` Needs fix: quoted example <- quoted, inside a CLOSED fence ``` Note on formatting. ``` <- stray opener body-wide fallback -> `veto` (master -> `comment`) Restricting the fallback to the region from the unmatched opener onward keeps the closed regions' reading and preserves the property the fallback exists for: a real veto below a genuinely unclosed fence is still a `veto`. Scope, measured rather than assumed: the fallback fires on **0 of 477** real bodies in the corpus, so this is a correctness fix to the tool's own stated guarantee, not a live miscount. Head vs fixed over those 477 bodies: **0** change class. Over 477 bodies the fix moves nothing; the mutant that restores the body-wide fallback is killed by the new test. Co-authored-by: how2how2how2-arch (reported and proposed the tail-only fix) --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
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).