Fall back to diff3 conflict markers in resolve-conflicts - #212
Conversation
The skill's conflict analysis reads the base section of each hunk to tell a stacked-PR duplicate from a real divergence. Git writes that section only when merge.conflictStyle is diff3 or zdiff3, which the bundled skill cannot assume on other hosts. Step 2b now runs ensure-diff3-markers.sh, which re-creates the markers of each conflicted file in diff3 style when the host leaves the setting unset. It skips a file with no markers, so a rerere replay survives, and it reports only the files that gained a base section, because a gitattributes merge driver re-runs on the checkout and its output wins. Generated-By: PostHog Desktop Task-Id: f162bd9d-1c27-4e1f-9cd9-4875d93975b1
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in marker detection, path handling, merge-driver reporting, and test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a portable fallback for recreating diff3 conflict markers, documents the workflow, adds tests, and wires them into CI.
Changes:
- Adds marker reconstruction with safeguards for resolved and merge-driver-managed files.
- Updates conflict-resolution guidance.
- Adds focused tests and CI coverage.
File summaries
| File | Summary |
|---|---|
ai/skills/resolve-conflicts/SKILL.md |
Documents diff3 marker preparation and edge cases. |
ai/skills/resolve-conflicts/scripts/test-ensure-diff3-markers.sh |
Tests marker rewriting behavior. |
ai/skills/resolve-conflicts/scripts/ensure-diff3-markers.sh |
Recreates diff3 markers for eligible conflicts. |
.github/workflows/test.yml |
Runs the resolve-conflicts test suites in CI. |
Review findings: the documentation examples and markerless-path guidance need clarification (nits). The script needs flexible marker-size detection, safe handling of dash-prefixed filenames, and post-checking after failed merge-driver execution (moderate). The diff3/zdiff3 tests should configure the style before merging and use byte-preserving comparisons (moderate).
Review details
Suppressed comments (8)
ai/skills/resolve-conflicts/SKILL.md:141
- Step 2b can also skip a path with no conflict markers (the rerere/binary/delete-modify cases described above), so it is not true that every skipped file carries standard two-way markers. Distinguish markerless, already-resolved paths from unresolved two-way conflicts here so the agent does not analyze an already-resolved file as a hunk.
For conflicts that remain after mergiraf (or for `other` category files), read the file and analyze each conflict hunk. Step 2b gives most files diff3-style markers. A file it skipped still carries standard two-way markers, so handle both:
ai/skills/resolve-conflicts/scripts/ensure-diff3-markers.sh:34
- These checks assume Git always emits seven-character markers. Git supports
merge.conflictMarkerSizevalues below seven, in which case a valid conflict begins with fewer than seven</|characters and this script skips it without adding a base section. Match the marker grammar (at least three characters) instead of a fixed width.
if ! grep -qI '^<<<<<<<' "$file" 2>/dev/null; then
continue
fi
if grep -qI '^|||||||' "$file" 2>/dev/null; then
continue
fi
ai/skills/resolve-conflicts/scripts/ensure-diff3-markers.sh:29
$fileis passed as a positional argument without--, so a conflicted path beginning with-is parsed as a grep option rather than a filename. That can make grep read the filename stream as stdin or fail, causing this conflict to be skipped; terminate grep options before the path.
if ! grep -qI '^<<<<<<<' "$file" 2>/dev/null; then
ai/skills/resolve-conflicts/scripts/ensure-diff3-markers.sh:32
- This grep has the same option-parsing problem for conflicted paths whose names begin with
-: the path can be treated as an option instead of a file, so an existing base section may not be detected. Add--before the path.
if grep -qI '^|||||||' "$file" 2>/dev/null; then
ai/skills/resolve-conflicts/scripts/ensure-diff3-markers.sh:43
- The final base-section check also treats a filename beginning with
-as grep options, so a successfully rewritten conflict can be omitted from the reported output for such paths. Terminate grep options before$file.
if grep -qI '^|||||||' "$file" 2>/dev/null; then
ai/skills/resolve-conflicts/scripts/ensure-diff3-markers.sh:39
- An unresolved custom merge driver conventionally exits nonzero even when it writes a valid diff3 conflict containing a base section. This
continueskips the post-check in that case, so the script fails to report a file that did gain a base section, contrary to the documented output contract; keep the warning but let the base-section check run after a failed checkout.
if ! git checkout --conflict=diff3 -- "$file" 2>/dev/null; then
echo "Warning: could not rewrite conflict markers in $file" >&2
continue
fi
ai/skills/resolve-conflicts/scripts/test-ensure-diff3-markers.sh:104
setup_repoperforms the merge at line 45 while the style is unset, and this test setsmerge.conflictStyleonly afterward. As a result, the file is still two-way, so thediff3/zdiff3cases verify only the early exit and do not cover leaving an actual diff3 file byte-identical. Configure the style before the merge (or parameterizesetup_repo) and compare the resulting file.
repo=$(setup_repo)
git -C "$repo" config merge.conflictStyle "$style"
local before
before=$(cat "$repo/conflict.txt")
local output
output=$(cd "$repo" && bash "$SCRIPT")
assert_output "$style style prints nothing" "" "$output"
assert_output "$style style leaves the file untouched" "$before" "$(cat "$repo/conflict.txt")"
ai/skills/resolve-conflicts/scripts/test-ensure-diff3-markers.sh:104
- The
catcommand substitutions strip trailing newlines, so this assertion does not actually verify the claimed byte-identical behavior for thediff3/zdiff3cases. Hash the working-tree file (or compare it withcmp) before and after instead.
before=$(cat "$repo/conflict.txt")
local output
output=$(cd "$repo" && bash "$SCRIPT")
assert_output "$style style prints nothing" "" "$output"
assert_output "$style style leaves the file untouched" "$before" "$(cat "$repo/conflict.txt")"
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The script changes to the top of the working tree before it reads the conflicted paths. git diff reports those paths from the top level, so a run from a subdirectory opened none of them and exited without output, which looks the same as the no-op on a host that already uses diff3. Marker detection reads the conflict-marker-size attribute of each file. Git accepts any width down to one character, and a fixed seven-character pattern reported a shorter marker as no marker at all. Reads merge.conflictStyle without --default, so git older than 2.18 does not abort the step. Passes -- to grep, so a path that starts with a dash is not read as an option. The style cases configure git before the merge, so the marker check carries them rather than the config gate alone, and they compare bytes with cmp, which a $(cat) comparison does not. New cases cover a run from a subdirectory, a shortened marker width, a skipped file followed by a conflicted one, the empty base section of an add/add conflict, and the survival of a file that already carries diff3 markers. Step 2b states the hand-edit warning before the command, so an agent reading top-down sees it before it runs the script. Step 2d shows the ours/theirs labels the rewrite produces, and says that a skipped path may carry no markers at all. Generated-By: PostHog Desktop Task-Id: f162bd9d-1c27-4e1f-9cd9-4875d93975b1
The
resolve-conflictsskill decides how to resolve a hunk by reading its base section. Its stacked-PR duplicate detection keys off whether the base is empty, and its decision table has aBasecolumn. Git writes that section only whenmerge.conflictStyleisdiff3orzdiff3. This repo'sgit/gitconfig.symlinksets it, but the skill is bundled for use on other hosts, where it usually isn't set and git writes two-way markers instead.Step 2b now runs
scripts/ensure-diff3-markers.sh, which re-creates the markers of each conflicted file withgit checkout --conflict=diff3when the host leaves the setting unset. It runs before the mergiraf pass, sincemergiraf solvealso resolves more of a file when it can see the base.Two behaviors worth knowing about, both now documented in the skill:
The script skips a conflicted path with no markers. That path holds a binary or delete/modify conflict, or a resolution rerere already replayed, and a rewrite would destroy the resolution. It cannot detect a half-finished hand edit, so the skill tells the agent to ask first if the user was already editing.
A merge driver named by a gitattributes
merge=rule re-runs on the checkout and its output wins, so the rewrite does not always produce a base section. The script reports only the files that ended up with one. On a host with mergiraf wired globally the driver already emits base sections, so the script skips everything, which is the intended no-op.The re-merge also relabels the markers
oursandtheirs, so a rebase conflict loses the sha and subject of the commit being replayed. The AI-analysis step notes that.Test plan
scripts/test-ensure-diff3-markers.shcovers the unset andmergestyles rewriting,diff3andzdiff3leaving the file byte-identical, a rerere-style pre-resolved working tree surviving, an already-diff3 file being skipped, an add/add conflict with an empty base, a merge driver whose output has no base section going unreported, a filename with a space, and running outside a repository. It isolates from the host gitconfig withGIT_CONFIG_GLOBAL=/dev/null, otherwise this machine's owndiff3setting hides every rewrite case.That suite and the two existing
resolve-conflictssuites, which were not wired into CI before, are now in.github/workflows/test.yml. All 15 steps of that job pass locally, andshellcheckis clean on the new scripts.