Preserve merge conflict contents in imported worktrees - #77
Merged
Conversation
Merge-request isolation currently reports conflicts without preserving the other side in the working file. Define a fallback that uses the trusted Git executable for normal three-way merges while keeping the PATH-hijack boundary. Keep trusted global attribute-driver policy separate from this focused data preservation fix. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Git already shell-quotes merge labels, so adding double quotes around those placeholders would make hostile command substitutions executable. A missing persisted executable would also look like an ordinary conflict and reproduce the data-hiding failure. Use fixed labels, an explicit executable guard, and documented binary-status mapping so the implementation plan preserves the untrusted-tree boundary. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Mapping every status above 128 would hide a signaled merge process as an ordinary conflict and could leave the current side without markers. Limit the binary exception to merge-file's exact status 255 and preserve all signal failures. Make the Windows command path and cross-platform test coverage explicit before implementation starts. Generated with Codex Co-authored-by: Codex <codex@openai.com>
The design now has verified decisions for shell quoting, missing executables, binary conflicts, signal failures, and Git for Windows paths. Record the test-first execution sequence before production code changes so these security and data-preservation constraints stay coupled to behavior tests. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Keep POSIX quoting behavior in one reusable internal package so Git command builders and related tests use the same safe treatment of spaces and quotes. This removes the duplicated private helper without changing credential-helper command output. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Imported merge-request worktrees used a merge driver that reported every merge as a conflict without writing the base or other side to the worktree. This hid useful conflict contents and made non-overlapping edits impossible to merge normally. Use the resolved Git executable for three-way merge-file behavior. Keep fixed marker labels and propagate operation failures so untrusted labels, PATH changes, and missing executables cannot turn errors into incomplete conflicts. Generated with Codex Co-authored-by: Codex <codex@openai.com>
The PATH regression could pass after any pre-driver merge error and no longer exercised the safe diff replacement. The label test also combined branch and subject payloads in a rebase, which did not prove branch-label behavior. Require the expected unmerged state and diff3 contents after preserving the successful diff probe. Exercise hostile branch and subject labels through merge and rebase separately so each Git label path remains covered. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Future merge-driver changes need the same boundary as the current repair. Without a durable contract, a later implementation could again hide text conflict contents or turn an unavailable Git process into a per-file conflict. Record the observable behavior for executable resolution, text and binary conflicts, process failures, and Unix and Git for Windows support. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Git expands merge-driver placeholders across the complete configured command before the shell parses it. A resolved executable path containing `%Y` could therefore splice an untrusted branch label into the command despite POSIX shell quoting and run its command substitutions. Keep literal percent signs intact through Git's template layer before applying shell quoting. Record the two escaping layers and the existing platform Git version floors so future changes preserve the same boundary. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Git before 2.42 turns every positive custom merge-driver result into a conflict. Git merge-file also uses status 255 for both binary content and input or output failures. Together, those rules can hide a real operation failure behind a current-only conflict. Require Git 2.42 outside Windows. Classify binary inputs with the pinned Git executable in an attribute-free, helper-free context, then pass all text merge statuses through. Binary conflicts stay local without masking merge failures. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Merge input classification could inherit a caller's repository bindings or large-file threshold. Plain text could then look binary and leave only the current side in an ordinary conflict even though merge-file could preserve all three inputs. Clear repository bindings and counted configuration before classification. Pin the classifier to merge-file's maximum text size so ambient Git state cannot change the result. Generated with Codex Co-authored-by: Codex <codex@openai.com>
The counted-config regression proves that the nested classifier resets GIT_CONFIG_COUNT. It cannot independently detect removal of the explicit large-file threshold because the same reset removes its test input. Supply a one-byte threshold through an isolated global config instead. Removing only the 1023m pin now reproduces the current-only conflict and protects the reason for matching merge-file's text limit. Generated with Codex Co-authored-by: Codex <codex@openai.com>
roborev: Combined Review (
|
The merge-driver contract says a crashed merge process must fail the whole operation, not record a conflict. Nothing tested that. The existing coverage stops at status 255, which the driver reaches by returning normally, so a change that mapped every status above 128 back to a conflict would keep every test green while restoring the original bug: an unmerged path whose working file holds only the current side. Kill the merge process with a signal and require the operation to abort with a clean tree. Reintroducing the status mapping now fails this test. Also assert that no classifier helper marker appears in the hooks directory. The classifier runs Git there with -C, so a helper invoked from that directory would have left its marker where nothing looked. Document that a caller's Git runner governs process policy rather than which Git installation the merge driver pins. Git runs the driver itself and cannot route it back through the callback, so a runner pointing at another Git does not redirect it. Generated with Claude Code (claude-fable-5) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
roborev: Combined Review (
|
The spec and implementation plan for the safe merge driver were working notes for building the change, not guidance for people using this module. Keeping them added a docs/superpowers tree that nothing else in the repository uses, next to docs that describe the shipped packages. The durable rules those notes established already live where a future change will actually meet them: the merge-driver contract and its Git version floors are recorded in git/AGENTS.md, and the reasoning behind each decision is in the commit history and the tests. Generated with Claude Code (claude-fable-5) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
roborev: Combined Review (
|
wesm
added a commit
to kenn-io/kwt
that referenced
this pull request
Sep 1, 2026
This PR proves that kwt preserves both sides of a merge conflict in an imported pull-request worktree. Before the kit repair, a custom merge driver could report a conflict while leaving only the current side in the file; staging that file could lose the incoming change. ## What this changes - Adds a regression test through `GitBackend.ImportPullRequest` that performs a real conflicting merge and checks `UU` status, diff3 markers, and both sides' content. - Uses released `go.kenn.io/kit v0.23.0`, which contains kit PR #77's safe merge-driver repair. - Documents the higher Git floor for pull-request import in the installation guides, CLI reference, pull-request contract, command help, and changelog. ## Landing order 1. Run the focused and full checks with `go.kenn.io/kit v0.23.0`. 2. Merge this kwt PR. 3. Release kwt from the merged, tagged dependency. Related: #112 Related kit change: kenn-io/kit#77 ## Verification note The focused pull-request suite and full `make test` pass against released kit v0.23.0. `make build`, `make vet`, `make lint`, and `make docs-check` also pass. The full suite previously hit an unrelated 2-second SSH resolver timing bound once; the rerun completed successfully. Co-authored-by: Wes McKinney <wesm@users.noreply.github.com>
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.
Conflicted files in imported merge-request worktrees contain their conflict
markers again.
Before this change, Kit replaced every custom merge driver the fetched tree can
select with
f() { return 1; }; f. That returns a conflict but never writes themerged file, so Git marked the path unmerged while the file on disk still held
only your side — no markers, none of the other side's changes. Staging it
dropped their work without warning. Reported as kenn-io/kwt#112.
The replacement now runs Git's own three-way merge:
Git's built-in binary driver.
quietly leaving a one-sided file.
Two things in
git/managed/untrusted_tree.goare worth a close read.The command uses fixed
current/base/otherlabels instead of the%S,%X, and%Yplaceholders. Git inserts%A,%O, and%Bunquoted, so thecommand has to wrap those in double quotes — but it inserts the label
placeholders already quoted. Putting the labels inside the same double quotes
would let a branch name or commit message containing
$(...)run as a commandduring any merge or rebase in the worktree. Dropping the labels avoids that.
Binary files are detected up front with
git diff --no-index --numstatratherthan from
merge-file's exit code, because 255 means both "binary" and "I/Ofailure". That check runs with repository bindings, injected config,
attributes, and external diff helpers all turned off, so nothing in the fetched
tree can steer it.
One compatibility note: imports now need Git 2.42.0 or newer on macOS and
Linux, up from 2.39.1. Older Git reports every failing driver as a plain
conflict, so it cannot tell a crash from a real conflict. The Git for Windows
requirement is unchanged.
Eleven tests drive the real driver through actual imports, covering hostile
branch names and commit subjects, a signal-killed merge, and binary conflicts.
git/internal/shellquotenow holds the shell-quoting helper thatgit/cmdkept to itself.