CI: detect evil merges in test files (a conflict resolution that matches neither parent)

[jaylfc]

CARD TITLE (intent, not commit subject): CI: detect evil merges in test files (a conflict resolution that matches neither parent)

Autonomous build of board card tsk-hlnd2t.

The guard previously compared the merge result only against the
git merge-tree conflict-marker baseline. That incorrectly flagged
legitimate conflict resolutions that took one side entirely, because
the conflict-marker blob never matches a clean parent blob.

The fix adds a second check: when head differs from merge-tree,
verify that it also differs from both parents before declaring a
violation. If head matches either parent exactly, the resolution
is a clean wholesale take and the guard stays green.

Updated CLI output to match the documented gate contract:
EVIL-MERGE FAIL: matches neither parent
merge M
parent1 P1
parent2 P2

Added control test for the conflict-resolution-by-taking-one-side
case, which was the untested false positive that motivated the fix.

RED-FIRST proof:

FAILED tests/test_check_evil_merge.py::TestEvilMergeGuard::test_conflict_resolved_by_taking_one_side_wholesale_stays_green - AssertionError

Green after fix:

tests/test_check_evil_merge.py tests/test_check_gate_integrity.py tests/test_check_doc_gate.py -q
120 passed in 4.12s

Files:
changelog.d/tsk-hlnd2t-evil-merge-parent-match.md | 8 ++++
scripts/check_evil_merge.py | 18 ++++----
tests/test_check_evil_merge.py | 52 +++++++++++++++++++++--
3 files changed, 66 insertions(+), 12 deletions(-)

Summary by CodeRabbit

• Bug Fixes
  • Fixed merge validation to allow conflict resolutions that correctly match one parent’s content.
  • Updated failure messages to clearly identify cases that match neither parent, including the merge and parent hashes.
• Tests
  • Added regression coverage for conflict resolutions that take one parent’s content wholesale.
• Documentation
  • Added a changelog entry describing the merge validation fix and updated CLI output.

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: aa44b2af-5608-4fb3-8ae7-0661c4495673

📥 Commits

Reviewing files that changed from the base of the PR and between 0d6e8e4 and ff2d04e.

📒 Files selected for processing (3)

• changelog.d/tsk-hlnd2t-evil-merge-parent-match.md
• scripts/check_evil_merge.py
• tests/test_check_evil_merge.py

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The evil-merge guard now accepts conflict resolutions identical to either parent. It reports failures only when content differs from both parents and includes the merge and parent hashes. A regression test covers wholesale resolution of a conflict using one parent’s content.

Changes

Evil-merge guard correction

Layer / File(s)	Summary
Parent comparison and failure output scripts/check_evil_merge.py	The guard ignores blobs that match either parent. Failure output now states matches neither parent and includes the merge and both parent hashes.
Wholesale parent resolution coverage tests/test_check_evil_merge.py, changelog.d/tsk-hlnd2t-evil-merge-parent-match.md	A regression test verifies that resolving a conflict with one parent’s content produces no violations. The changelog records the updated behavior and output.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ff2d0

No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: updating the CI guard to detect evil merges when a conflict resolution matches neither parent.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 u…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch exec/tsk-hlnd2t

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)

• scripts/check_evil_merge.py
• tests/test_check_evil_merge.py
• changelog.d/tsk-hlnd2t-evil-merge-parent-match.md

---

_{Reviewed by step-3.7-flash:free · Input: 0 · Output: 0 · Cached: 0}

[jaylfc]

Bounced — the relaxation is unconditional, so the guard now passes a clean merge that silently deletes the other parent's test

The false positive you fixed is real: on a conflicted path git merge-tree emits a conflict-marker blob that no clean
resolution can ever equal, so a wholesale take-one-side resolution was always flagged. That part of the diagnosis is right.

But the escape hatch was added to the elif head_blob != expected: arm, which is not the conflict arm. That arm also
covers paths where git merge-tree produced a clean auto-merge result and the merge commit overrode it. Adding
head_blob != p1_blob and head_blob != p2_blob there blinds the guard to the case it exists to catch: a merge that takes
one parent wholesale and silently drops the test the other parent added.

Measured, same probe against both trees

Base test file; side-a edits the top of it; side-b appends a new test_security_check at the bottom. The paths do not
overlap, so git auto-merges with no conflict at all — then the merge commit writes side-a's blob wholesale.

--- PR #2967 tree (ff2d04ef5) ---
merge rc = 0 | CLEAN | Auto-merging tests/test_widget.py
side-b's test_security_check present at merge result: False
VIOLATIONS: NONE (guard is GREEN)

--- origin/dev tree ---
merge rc = 0 | CLEAN | Auto-merging tests/test_widget.py
side-b's test_security_check present at merge result: False
VIOLATIONS: 1 -> tests/test_widget.py

merge rc = 0 is the important line: there is no conflict here, so the false-positive rationale does not apply, and a
newly added security test disappears from the merge result with the gate green. dev catches it; this branch does not.

The fix

check_evil_merge already knows which paths conflicted — _parse_merge_tree_stdout returns conflict_blobs, and the
per-path conflict set is exactly the right scope. Gate the new escape hatch on it:

elif head_blob != expected:
    if path in conflict_blobs:
        # conflicted path: merge-tree's blob carries markers, so a wholesale
        # take-one-side resolution is legitimate.
        p1_blob, p2_blob = p1_blobs.get(path), p2_blobs.get(path)
        if head_blob != p1_blob and head_blob != p2_blob:
            violations.append(...)
    else:
        # clean auto-merge that the merge overrode -> still a violation.
        violations.append(...)

While you are in there: if conflict_blobs: merge_tree_blobs = conflict_blobs (line ~211) means that as soon as any
path conflicts, every clean path loses its merge-tree baseline and falls through to the looser expected is None
branch. Keying off path in conflict_blobs rather than the global fixes that too — worth doing in the same pass, but
say so explicitly if you do.

Tests

Your new CONTROL A (conflict variant) is a good test and should stay. Add the arm above it as a RED case: clean
auto-merge, merge overrides it by taking one parent wholesale, other parent's added test vanishes -> the guard must
report a violation. The current suite cannot fail on this — every existing red case invents content matching neither
parent, and both CONTROL A variants take a parent wholesale in situations where that is legitimate, so nothing
exercises "took a parent wholesale and that was wrong".

The CLI output change is fine, but once the scoping above lands, matches neither parent is no longer accurate for the
clean-merge violation — it will need to distinguish the two failure reasons.

Note for the re-push: Gate integrity is red because this touches scripts/check_*.py. That needs the
gate-integrity-allow label, which is lead-only — I will add it when the branch is ready, do not spend attempts on it.

[jaylfc]

Lead review — blocked, and the gate-integrity-allow label is deliberately withheld until this is fixed. Carried as fix-forward tsk-mpqtau.

The false positive you're fixing is real: a conflict resolved by taking one parent wholesale can never match the conflict-marker blob, so the old guard flagged it. Keep that intent. The problem is that the exemption isn't conditioned on the path having actually conflicted.

The new escape hatch sits in the elif head_blob != expected: branch — which is the branch for paths git merge-tree merged cleanly, where expected is the clean merged blob. So: both sides touch different, non-overlapping parts of the same test file → git merges them cleanly to side-a + side-b → the committer commits parent1's content wholesale instead → side-b's test change is silently discarded → head_blob == p1_blob → exempt → green. That's an evil merge, and it's the same shape that reverted #2957 through a hand-merge in #2972 earlier today.

Measured rather than argued. Repo built as: base test_a=OLD_A, test_b=OLD_B; side-a changes only test_a; side-b changes only test_b; main merges side-a, then merges side-b but commits parent1's content. git merge-tree reports no conflict for that pair, so conflict_blobs is empty. Running check_evil_merge(Path(repo)):

dev  (origin/dev):        1 violation  -> RED, caught
head (ff2d04ef, this PR): 0 violations -> GREEN, missed

Why the PR's own tests can't catch it: test_merge_taking_one_side_wholesale_stays_green was already green before this change, so it never exercises the new branch. The new test_conflict_resolved_by_taking_one_side_wholesale_stays_green builds contradictory edits to the same line, which genuinely conflict — so it only ever exercises the conflict path. Neither test constructs a clean auto-merge that is then resolved by discarding one side, so the suite cannot fail on the defect.

The fix is small: check_evil_merge already computes conflict_blobs from _parse_merge_tree_stdout. Carry that set into the loop and allow the head_blob == p1_blob or head_blob == p2_blob escape only when path in conflict_blobs. Where merge-tree merged cleanly, head_blob must still equal expected. Leave the expected is None branch alone — it has no merge-tree result to compare against.

The card asks for a red-first test on exactly the clean-merge-then-take-one-side case, with the existing conflict-variant control kept green, so the commit body proves both directions. One more thing: EVIL-MERGE FAIL: <path> matches neither parent is only accurate for the conflicted case now — have the message say which rule fired.

I'll add gate-integrity-allow once the guard is shown to still catch the case above.