fix(changelog): strike the committed conflict markers, and fail on merge residue anywhere (#1491) - #1492
Merged
Merged
Conversation
1622e02 left `<<<<<<< HEAD`, `=======` and `>>>>>>> be03af9` in the [Unreleased] block. Both sides are distinct entries — #1442's stop-prompt bounds and #1371's noise filter — and each appears exactly once in the file, so the conflict was purely additive. Both are kept verbatim; only the three marker lines go. Refs #1491
Nothing read a tracked file for shape, so three marker lines sat in CHANGELOG/v4.md through a fully green suite. The guard is repo-wide, not changelog-scoped: the same accident put markers in a design doc during the #1362/#1378 rebase, and a check scoped to the file hit last catches only the previous incident. The separator is conditional. Seven '<' or '>' plus a space open no legitimate line here, but a line of seven '=' is a Setext underline in Markdown and a section rule in reStructuredText, both of which occur in docs/ — so it counts only in a file that already carries an open, close or diff3 base marker. Marker literals are built from repeated characters so the guard and its test do not trip themselves; an exclusion list was the alternative and every entry on one is a file that can carry markers to main unseen. Mutation-checked: restoring the markers on v4.md turns three tests red and the script exits 1 naming all three lines; making the separator unconditional reddens the Setext control; dropping the close marker reddens four; counting undecodable files as scanned reddens the non-vacuity arm. The live test asserts the scan read >100 files, because an ls-files that returned nothing would otherwise pass having read none. Closes #1491
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request removes committed conflict markers from ChangesConflict marker detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Reviewer's GuideAdds a repo-wide merge-conflict marker guard that runs in CI and pytest, removes committed markers from CHANGELOG/v4.md, and documents the fix and guard behaviour in the changelog. Sequence diagram for repo-wide merge-conflict marker guardsequenceDiagram
actor Developer
participant Pytest
participant CI_workflow as CI_workflow_staging_gate
participant Script as check_conflict_markers_py
participant Git as git
Developer->>Pytest: run tests
Pytest->>Script: main(argv)
CI_workflow->>Script: main(argv)
Script->>Git: tracked_files(repo)
Git-->>Script: list of tracked files
Script->>Script: scan(paths)
Script->>Script: find_markers(text)
Script-->>Pytest: exit 1/2/0 with stderr annotations
Script-->>CI_workflow: exit 1/2/0 with stderr annotations
Flow diagram for conflict marker detection with conditional separatorflowchart TD
A[read_text utf-8] --> B[split into lines]
B --> C{line startswith OPEN_MARKER or CLOSE_MARKER<br/>or line rstrip == BASE_MARKER}
C -->|any match| D[collect unambiguous markers]
C -->|no match| E[return empty list]
D --> F{line rstrip == SEPARATOR}
F -->|collect| G[add separator lines]
F -->|none| H[keep only unambiguous markers]
G --> I[sort markers and return]
H --> I[sort markers and return]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_conflict_markers.py`:
- Around line 54-59: Update the conflict-marker scan comprehension in
scripts/check_conflict_markers.py:54-59 to recognize BASE_MARKER when it appears
at the end of a line or is followed by a space and label, while preserving
existing marker detection. Update the test fixture in
tests/test_conflict_markers_1491.py:60-64 to use a labeled diff3 base marker and
retain the expected four reported lines.
- Around line 88-94: The scan loop in scripts/check_conflict_markers.py (the
path-reading logic around find_markers) must read symlink payloads without
following links, while treating other read failures as scan failures rather than
silently skipping them; update tests/test_conflict_markers_1491.py at lines
91-101 to create a broken symlink with an open-marker payload and assert that
the scan reports it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 19ac0c81-0b58-49df-a52f-37051605e843
📒 Files selected for processing (4)
.github/workflows/staging-gate.ymlCHANGELOG/v4.mdscripts/check_conflict_markers.pytests/test_conflict_markers_1491.py
…lowed Two review findings on the guard itself. The ancestor marker only matched a bare seven pipes, but a real merge writes it labelled — '||||||| merged common ancestors' from a merge, '||||||| <ref>' from a --conflict=diff3 checkout — so the only form the rule caught was the one this repo could not acquire. It now accepts bare or labelled, and moves from the unambiguous set to the conditional one beside the separator: seven pipes is also a Markdown table row of six empty cells. That gives up nothing real, because git writes all of a conflict's markers or none. scan() followed symlinks and swallowed every OSError. Following meant scanning whatever the link pointed at, possibly outside the repo and tracked by nobody, and reporting it against a path whose committed blob is a target string; a dangling link raised and was silently skipped. A symlink is now read with readlink, which is the content git stores, and any other read failure raises UnreadableFile — main turns that into exit 2, because a skip is indistinguishable from a clean file in the summary and would shrink coverage while still reporting success. Mutation-checked: dropping the condition reddens the table and Setext controls; matching only the bare ancestor marker reddens both labelled cases; following symlinks reddens both link tests; restoring the silent OSError skip reddens the unreadable-file arm. Refs #1491
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged c8ebb35 → |
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.
Closes #1491
What was wrong
CHANGELOG/v4.mdonmaincarried three committed conflict markers inside[Unreleased], left by1622e02a:Both sides were distinct entries — #1442's stop-prompt bounds (line 65) and
#1371's noise filter (line 67) — and each appears exactly once in the file, so
the conflict was purely additive. Nothing was lost. Both entries are kept
verbatim; only the three marker lines go.
Why nothing caught it
The suite was fully green with the markers on
main. Nothing in it opens atracked file to read its shape. The two checks that do read the changelog ask
other questions:
release-docs-checkasks whether[Unreleased]was drained atrelease time,
check_changelog_dupes.pyasks whether two entries restate eachother. Merge residue is neither.
The guard
scripts/check_conflict_markers.py, run by pytest and byrelease-docs-check(the latter so the failure lands as a file annotation on the offending line
rather than as a test failure the author has to locate).
docs/design/write-log-as-truth.mdduring the feat(detectors): pin and version the thresholds behind the 2.8% non-spine edges #1362/fix(spine): report both directions of the divergence and correct its denominator (#1356) #1378 rebase. A checkscoped to whichever file was hit last is a check for the previous incident.
<or>followed bya space open no legitimate line in this repo. A line of seven
=is a Setextheading underline in Markdown and a section rule in reStructuredText, and both
occur under
docs/. So a separator counts only in a file that already carriesan open, close or diff3 base marker — the only shape a real conflict leaves.
An unconditional rule would make the check unrunnable, and an unrunnable check
gets switched off rather than fixed.
the script and its test do not trip the check they implement. Every path on an
exclusion list is a path that can carry markers to
mainunseen.Mutation checks
CHANGELOG/v4.mdtest_a_setext_underline_alone_is_not_a_conflictredtest_scan_skips_binary_without_counting_itredThe live test asserts the scan read more than 100 files. A
git ls-filesthatreturned nothing would otherwise pass having read none — which is how this class
of check stops working quietly.
Out of scope
The
CHANGELOG/unreleased/restructure (#1475, PR #1487), which removes theconflict class itself rather than guarding its residue.
Summary by Sourcery
Guard against committed merge-conflict markers across the repository and document the fix in the changelog.
Bug Fixes:
CHANGELOG/v4.mdand record the incident and repair in the changelog.Enhancements:
check_conflict_markers.pyscript that scans tracked text files for merge-conflict residue and fails on findings.Tests:
Summary by CodeRabbit