From d65c8b71d94d33f1f978ce89b929b5e841fcb5e5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 20:50:19 +0200 Subject: [PATCH 01/14] Design safe merge-driver fallback 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 --- .../2026-08-29-safe-merge-driver-design.md | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md new file mode 100644 index 0000000..828b822 --- /dev/null +++ b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md @@ -0,0 +1,83 @@ +# Safe merge-driver fallback design + +## Problem + +Merge-request imports treat the fetched tree as untrusted. Kit therefore +replaces every configured custom merge driver that the tree can select. The +current replacement exits with status 1 without writing a result to Git's +`%A` file. + +Git correctly records an unmerged index entry, but it trusts the custom driver +to populate `%A`. The working file consequently contains only the current +side, with no conflict markers or other-side content. A later `git add` can +hide the omitted changes. + +## Goals + +- Use Git's normal three-way text merge when a custom driver is disabled. +- Preserve diff3 conflict markers and all three inputs when overlap remains. +- Merge non-overlapping edits without reporting a conflict. +- Keep an imported tree from selecting a replacement executable through + `PATH`. +- Preserve the existing filter, diff, hook, fsmonitor, submodule, and + worktree-configuration isolation behavior. + +## Non-goals + +- Distinguishing drivers selected by trusted global attributes from drivers + selected by repository-controlled attributes. +- Changing the public managed-worktree API. +- Replacing Git's built-in merge algorithm or interpreting file contents in + Go. + +## Design + +Before preparing persistent untrusted-tree isolation, Kit resolves the same +`git` executable that its subprocess runner uses. It converts the result to an +absolute, cleaned path and fails the import if no executable can be resolved. + +Kit builds the replacement custom-driver command from that trusted absolute +path. The command invokes: + +```text + merge-file --diff3 --marker-size=%L \ + -L "%X" -L "%S" -L "%Y" "%A" "%O" "%B" +``` + +The executable path is shell-quoted as data. On Windows, path separators are +normalized for the POSIX shell supplied by Git for Windows. The placeholders +remain available for Git to substitute when it runs the driver. + +`git merge-file` overwrites `%A`, exits with status 0 for a clean merge, and +returns a nonzero status when conflicts remain. Git therefore keeps its normal +index state while the working file contains either a clean merged result or a +complete diff3 conflict. + +The resolved command becomes part of the existing untrusted-tree isolation +state. Attribute-driver discovery remains responsible only for finding driver +names; it uses the prepared command when constructing worktree-scoped +`merge..driver` entries. No new public API or persistent file is added. + +## Error handling + +Failure to resolve an absolute Git executable stops the import before the +untrusted worktree is materialized. A later missing or unusable executable +causes Git to fail the merge command rather than run a same-named program found +through the worktree's `PATH`. + +Existing import cleanup and rollback behavior remains unchanged. + +## Tests + +Behavioral tests create temporary repositories through the existing managed +worktree fixtures and exercise the persisted replacement after import: + +1. Two non-overlapping edits merge cleanly and produce the combined file. +2. Overlapping edits leave an unmerged path whose working file contains diff3 + markers and the base, current, and other content. +3. A fake `git` executable placed before the trusted executable in `PATH` is + not invoked during the merge. + +Focused unit coverage verifies shell quoting for executable paths, including +spaces, quotes, and Windows separators where platform-specific handling is +needed. From c5c5cc63e9e2540ac0460261d2688b9f6edebf33 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 22:23:49 +0200 Subject: [PATCH 02/14] Correct safe merge-driver design 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 --- .../2026-08-29-safe-merge-driver-design.md | 88 ++++++++++++++----- 1 file changed, 64 insertions(+), 24 deletions(-) diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md index 828b822..da59c8a 100644 --- a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md +++ b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md @@ -19,6 +19,7 @@ hide the omitted changes. - Merge non-overlapping edits without reporting a conflict. - Keep an imported tree from selecting a replacement executable through `PATH`. +- Preserve Git's ordinary per-file conflict behavior for binary content. - Preserve the existing filter, diff, hook, fsmonitor, submodule, and worktree-configuration isolation behavior. @@ -36,34 +37,69 @@ Before preparing persistent untrusted-tree isolation, Kit resolves the same `git` executable that its subprocess runner uses. It converts the result to an absolute, cleaned path and fails the import if no executable can be resolved. -Kit builds the replacement custom-driver command from that trusted absolute -path. The command invokes: - -```text - merge-file --diff3 --marker-size=%L \ - -L "%X" -L "%S" -L "%Y" "%A" "%O" "%B" +Kit moves the existing POSIX shell single-quote helper into a shared internal +Git package. Both the credential helper and managed-worktree isolation use it, +so executable paths remain data without adding a public API or duplicating +shell quoting. + +Kit builds a shell function around the trusted absolute path. In expanded form, +the function has these semantics: + +```sh +f() { + [ -x '' ] || return 129 + '' merge-file --diff3 --marker-size="$1" \ + -L current -L base -L other "$2" "$3" "$4" + status=$? + if [ "$status" -gt 128 ]; then + return 1 + fi + return "$status" +} +f %L "%A" "%O" "%B" ``` -The executable path is shell-quoted as data. On Windows, path separators are -normalized for the POSIX shell supplied by Git for Windows. The placeholders -remain available for Git to substitute when it runs the driver. +The displayed single quotes around `` represent the shared +helper's output, including its handling of an embedded single quote. + +Git inserts `%A`, `%O`, and `%B` without shell quoting, so the driver command +must keep the double quotes around those placeholders. Git 2.44 and newer +already insert `%S`, `%X`, and `%Y` as shell-single-quoted strings. Placing those +label placeholders inside another pair of double quotes would make command +substitutions in a branch name or commit subject executable. The replacement +therefore uses fixed labels and does not interpolate `%S`, `%X`, or `%Y` at all. +Fixed labels also keep the existing Git 2.39.1 minimum; no version-dependent +fallback or higher version floor is needed. `git merge-file` overwrites `%A`, exits with status 0 for a clean merge, and -returns a nonzero status when conflicts remain. Git therefore keeps its normal -index state while the working file contains either a clean merged result or a +returns the conflict count, capped at 127, when text conflicts remain. The +wrapper passes those statuses through. Git therefore keeps its normal index +state while a text working file contains either a clean merged result or a complete diff3 conflict. The resolved command becomes part of the existing untrusted-tree isolation -state. Attribute-driver discovery remains responsible only for finding driver -names; it uses the prepared command when constructing worktree-scoped -`merge..driver` entries. No new public API or persistent file is added. +state. Attribute-driver discovery uses the prepared command when constructing +worktree-scoped `merge..driver` entries. The command-scope configuration +check continues to classify driver-shaped keys without constructing a merge +command, so that earlier call site does not need a resolved executable path. +No new public API or persistent file is added. ## Error handling Failure to resolve an absolute Git executable stops the import before the -untrusted worktree is materialized. A later missing or unusable executable -causes Git to fail the merge command rather than run a same-named program found -through the worktree's `PATH`. +untrusted worktree is materialized. Before every later merge-driver invocation, +the shell function checks that the persisted path is still executable. A moved +or removed executable returns status 129 before `merge-file` starts. Git treats +that status as a driver failure and aborts the operation instead of recording +an ours-only conflict. + +`git merge-file` rejects binary content with an error status above 128. After +the executable guard succeeds, the wrapper maps such an error to status 1. +This preserves the previous and built-in binary-driver behavior: Git keeps the +current bytes, marks that path conflicted, and continues processing other +paths. Binary files do not receive text conflict markers. The guard runs first, +so an unavailable persisted executable is not converted into an ordinary +conflict. Existing import cleanup and rollback behavior remains unchanged. @@ -74,10 +110,14 @@ worktree fixtures and exercise the persisted replacement after import: 1. Two non-overlapping edits merge cleanly and produce the combined file. 2. Overlapping edits leave an unmerged path whose working file contains diff3 - markers and the base, current, and other content. -3. A fake `git` executable placed before the trusted executable in `PATH` is - not invoked during the merge. - -Focused unit coverage verifies shell quoting for executable paths, including -spaces, quotes, and Windows separators where platform-specific handling is -needed. + markers with the fixed labels and the base, current, and other content. +3. Adversarial branch and commit labels remain inert during a conflicted merge. +4. A missing persisted executable aborts the merge and leaves a clean tree. +5. Binary content produces an ordinary per-file conflict instead of aborting + the whole merge. +6. The existing PATH-hijack fixture is extended so a fake `git` executable + placed before the trusted executable is not invoked during a merge. + +Focused unit coverage moves with the shared single-quote helper and covers +executable paths containing spaces and single quotes. Existing assertions for +the persisted merge-driver value compare against the computed command. From 20e7fc0ca9a5b2aed42fd93103e5354f3bcbda38 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:04:34 +0200 Subject: [PATCH 03/14] Narrow merge-driver status handling 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 --- .../2026-08-29-safe-merge-driver-design.md | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md index da59c8a..8a32fa1 100644 --- a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md +++ b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md @@ -36,6 +36,10 @@ hide the omitted changes. Before preparing persistent untrusted-tree isolation, Kit resolves the same `git` executable that its subprocess runner uses. It converts the result to an absolute, cleaned path and fails the import if no executable can be resolved. +On Windows, Kit emits the drive-qualified path with forward slashes before +shell quoting it. Git for Windows runs custom drivers through its POSIX shell, +and the forward-slash form works for both the shell's executable check and the +Windows executable loader. Kit moves the existing POSIX shell single-quote helper into a shared internal Git package. Both the credential helper and managed-worktree isolation use it, @@ -51,7 +55,7 @@ f() { '' merge-file --diff3 --marker-size="$1" \ -L current -L base -L other "$2" "$3" "$4" status=$? - if [ "$status" -gt 128 ]; then + if [ "$status" -eq 255 ]; then return 1 fi return "$status" @@ -93,13 +97,23 @@ or removed executable returns status 129 before `merge-file` starts. Git treats that status as a driver failure and aborts the operation instead of recording an ours-only conflict. -`git merge-file` rejects binary content with an error status above 128. After -the executable guard succeeds, the wrapper maps such an error to status 1. +`git merge-file` rejects binary content with status 255. After the executable +guard succeeds, the wrapper maps that exact status to status 1. This preserves the previous and built-in binary-driver behavior: Git keeps the current bytes, marks that path conflicted, and continues processing other paths. Binary files do not receive text conflict markers. The guard runs first, so an unavailable persisted executable is not converted into an ordinary -conflict. +conflict. `merge-file` still writes its binary-file diagnostic to standard +error, including Git's temporary filename. + +All other statuses pass through unchanged. In particular, a signal death such +as status 139 remains a driver failure instead of becoming an ours-only +conflict. Statuses 126 and 127 also remain unchanged because `merge-file` can +legitimately return those conflict counts. The executable guard covers a stable +missing or non-executable path, but not a same-user replacement race between the +check and invocation. Git also treats `merge-file`'s own status 128 as an +ordinary conflict; its status contract provides no distinct value that the +wrapper can safely reinterpret. Existing import cleanup and rollback behavior remains unchanged. @@ -121,3 +135,5 @@ worktree fixtures and exercise the persisted replacement after import: Focused unit coverage moves with the shared single-quote helper and covers executable paths containing spaces and single quotes. Existing assertions for the persisted merge-driver value compare against the computed command. +Behavioral tests 1 through 5 run on Unix and Windows, including the emitted +Windows path form. Only the POSIX PATH-hijack fixture in test 6 skips Windows. From 5e345b3fc6429ef1887b4ca04a40ac8682bfd910 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:08:04 +0200 Subject: [PATCH 04/14] Plan safe merge-driver implementation 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 --- .../plans/2026-08-29-safe-merge-driver.md | 338 ++++++++++++++++++ 1 file changed, 338 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-29-safe-merge-driver.md diff --git a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md new file mode 100644 index 0000000..44d6403 --- /dev/null +++ b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md @@ -0,0 +1,338 @@ +# Safe Merge Driver Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use `superpowers:executing-plans` to implement this plan task-by-task. + +**Goal:** Replace untrusted-tree custom merge drivers with a durable driver that performs ordinary three-way text merges, writes diff3 conflict markers, keeps binary conflicts local to the file, and fails the whole Git operation when the pinned Git executable cannot run. + +**Architecture:** Resolve the same `git` executable used by `git/cmd.Runner` before materializing the untrusted tree, persist an absolute shell-quoted path in worktree config, and invoke `git merge-file` from a small shell wrapper. Keep attribute-driver key classification independent from command construction so command-scope checks do not require path resolution. Move the existing POSIX single-quote helper into `git/internal/shellquote` so both owning packages share the escaping rule. + +**Tech Stack:** Go 1.26, Git 2.39.1+, `testify`, standard `os/exec`, repository lifecycle fixtures. + +--- + +## Task 1: Share the existing shell-quoting helper + +**Files:** +- Create: `git/internal/shellquote/shellquote.go` +- Create: `git/internal/shellquote/shellquote_test.go` +- Modify: `git/cmd/gitcmd.go:18-28,187-198` +- Modify: `git/cmd/gitcmd_test.go:569` + +### Step 1: Write the failing helper test + +Create a table test for the exact POSIX single-quote contract: + +```go +package shellquote + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSingle(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name, input, want string + }{ + {name: "empty", want: "''"}, + {name: "spaces", input: "/opt/Git Tools/git", want: "'/opt/Git Tools/git'"}, + {name: "single quote", input: "/opt/Git's/git", want: "'/opt/Git'\\''s/git'"}, + } { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, test.want, Single(test.input)) + }) + } +} +``` + +Run: `go test ./git/internal/shellquote` + +Expected: FAIL because `Single` does not exist. + +### Step 2: Implement and adopt the helper + +```go +// Package shellquote quotes arguments embedded in Git shell command strings. +package shellquote + +import "strings" + +// Single returns value enclosed in POSIX shell single quotes. +func Single(value string) string { + return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" +} +``` + +Import it in `git/cmd/gitcmd.go`, replace `shellSingleQuote(path)` with `shellquote.Single(path)`, and delete the local helper. Make the same replacement in `gitcmd_test.go`. Retain the existing `strings` import because the package has other users. + +### Step 3: Verify and commit the refactor + +Run: + +```sh +gofmt -w git/internal/shellquote/shellquote.go git/internal/shellquote/shellquote_test.go git/cmd/gitcmd.go git/cmd/gitcmd_test.go +go test ./git/internal/shellquote ./git/cmd +``` + +Commit: + +```sh +git add git/internal/shellquote git/cmd/gitcmd.go git/cmd/gitcmd_test.go +git commit -m "refactor(git): share shell quoting helper" +``` + +## Task 2: Add end-to-end merge-driver regression tests + +**Files:** +- Create: `git/managed/untrusted_tree_merge_test.go` +- Modify: `git/managed/lifecycle_mr_test.go:440-725,619-675,803-872` + +### Step 1: Add a reusable imported-worktree fixture + +Build real repositories with `initOriginAndClone`, commit `.gitattributes` containing `payload merge=owned`, create `current` and `other` commits from a shared base, fetch `other` into the clone, and call `CreateWorktreeFromMergeRequest` for `current`. Return the imported worktree path and the other ref. Keep all paths under `t.TempDir`; do not read or alter user Git configuration. + +The helper should accept base/current/other bytes and optional branch/subject strings so every test exercises the persisted worktree driver through ordinary `git merge` or `git rebase`, rather than invoking the wrapper directly: + +```go +type mergeDriverFixture struct { + worktree string + otherRef string +} + +func newMergeDriverFixture( + t *testing.T, base, current, other []byte, + currentSubject, otherBranch string, +) mergeDriverFixture +``` + +Configure the trusted clone with `merge.owned.driver=false` before import. The returned worktree must therefore succeed only if Kit replaces that configured driver. + +### Step 2: Write the five cross-platform behavior tests + +Add these tests without any Windows skip: + +1. `TestUntrustedTreeMergeDriverMergesNonOverlappingText` — merge edits to different lines; assert exit 0, both edits in `payload`, and no unmerged entries. +2. `TestUntrustedTreeMergeDriverWritesDiff3ConflictMarkers` — overlap one line; assert ordinary conflict, `UU payload`, and exact marker labels `<<<<<<< current`, `||||||| base`, `=======`, `>>>>>>> other` with all three bodies. +3. `TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels` — use a branch name and rebase commit subject containing `$()` and backticks that would create relative marker files; assert neither marker exists and conflict markers still use only the fixed labels. +4. `TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears` — copy the resolved executable to a temporary executable path, replace only the fixture worktree's `merge.owned.driver` value with the command constructed for that copy, remove it before merge, and assert Git aborts with a clean worktree rather than recording `UU` with current-only contents. +5. `TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal` — mark both `binary.dat` and `payload` with the same driver; conflict both in one merge; assert both are `UU`, binary bytes remain current without text markers, and `payload` contains diff3 markers. Accept and document the `Cannot merge binary files` stderr line naming Git's temporary file. + +For the hostile-label test, use relative marker names valid under both Git for Windows' shell and POSIX shells; do not embed a platform-specific absolute path in a ref name. For the missing-executable test, use `git config --worktree merge.owned.driver ` after import. This tests the persisted-command failure contract without adding a production seam; ordinary production preparation still calls `exec.LookPath` exactly once. + +### Step 3: Extend the existing POSIX PATH-hijack test + +Keep the current `runtime.GOOS == "windows"` skip only on `TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers`. Add `merge=owned` to its attributes, configure `merge.owned.driver=false`, create a conflicting branch, run the later merge with the imported tree first in `PATH`, and preserve `assert.NoFileExists(marker)`. This verifies the wrapper invokes the absolute Git path instead of a tree-controlled `git` or `sh` helper. + +### Step 4: Run the new tests and confirm RED + +Run: + +```sh +go test ./git/managed -run 'Test(UntrustedTreeMergeDriver|CreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers)' -count=1 +``` + +Expected: FAIL. Non-overlapping text remains current-only, overlapping text lacks conflict markers, and the computed-command test seams do not yet exist. + +Do not commit these failing tests separately. + +## Task 3: Build and persist the safe merge driver + +**Files:** +- Modify: `git/managed/untrusted_tree.go:1-40,144-168,217-227,261-300,526-580` +- Modify: `git/managed/lifecycle_mr_test.go:522,672,870,1057` +- Test: `git/managed/untrusted_tree_merge_test.go` + +### Step 1: Resolve and normalize Git before materialization + +Add `os/exec` and `git/internal/shellquote` imports. Resolve with the same process `PATH` semantics as `gitcmd.Runner`: + +```go +func resolveMergeDriverGitPath() (string, error) { + path, err := exec.LookPath("git") + if err != nil { + return "", fmt.Errorf("resolve Git executable for safe merge driver: %w", err) + } + path, err = filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("make Git executable path absolute: %w", err) + } + path = filepath.Clean(path) + if runtime.GOOS == "windows" { + path = filepath.ToSlash(path) + } + return path, nil +} +``` + +Call this from `prepareUntrustedTreeIsolation` after command-scope validation and before creating or materializing the worktree. Store the generated command on `untrustedTreeIsolation`: + +```go +type untrustedTreeIsolation struct { + runner gitcmd.Runner + config []gitcmd.Config + mergeDriverCommand string +} +``` + +### Step 2: Construct the exact wrapper + +Replace the constant with a builder. The generated command must be semantically equivalent to: + +```sh +f() { + [ -x '' ] || return 129 + '' merge-file --diff3 --marker-size="$1" \ + -L current -L base -L other "$2" "$3" "$4" + status=$? + if [ "$status" -eq 255 ]; then + return 1 + fi + return "$status" +} +f %L "%A" "%O" "%B" +``` + +Build it as one line for Git config: + +```go +func safeMergeDriverCommand(gitPath string) string { + git := shellquote.Single(gitPath) + return `f() { [ -x ` + git + ` ] || return 129; ` + git + + ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + + `status=$?; if [ "$status" -eq 255 ]; then return 1; fi; ` + + `return "$status"; }; f %L "%A" "%O" "%B"` +} +``` + +Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. + +Map only status 255 to conflict. Do not use `-gt 128`, and do not remap 126 or 127: signal deaths must remain Git operation errors, while `merge-file` can legitimately report text conflict counts through 127. The executable guard returns 129 before invocation. An internal `merge-file` `die()` can still exit 128 and be treated by Git as a conflict; Git's custom-driver protocol provides no distinguishable alternative. + +### Step 3: Separate classification from construction + +Extract attribute-driver discovery from config creation: + +```go +type attributeDrivers struct { + filters map[string]struct{} + diffs map[string]struct{} + merges map[string]struct{} +} + +func configuredAttributeDrivers(keys []string) attributeDrivers + +func (drivers attributeDrivers) configured() bool { + return len(drivers.filters)+len(drivers.diffs)+len(drivers.merges) != 0 +} + +func neutralizeAttributeDrivers( + drivers attributeDrivers, mergeDriverCommand string, +) []gitcmd.Config +``` + +Use `configuredAttributeDrivers([]string{key}).configured()` in `isolationSensitiveConfigKey`, where no path is available. In `completeUntrustedTreeIsolation`, discover once and pass `isolation.mergeDriverCommand` to config construction. + +### Step 4: Update computed-command assertions + +Add one test helper used by all three existing config assertions: + +```go +func expectedSafeMergeDriverCommand(t *testing.T) string { + t.Helper() + path, err := resolveMergeDriverGitPath() + require.NoError(t, err) + return safeMergeDriverCommand(path) +} +``` + +Replace constant equality checks at the existing isolation, case-distinct-driver, and selected-config tests. Update the direct `neutralizeAttributeDrivers` classification test for the split API. + +### Step 5: Run focused tests and commit + +Run: + +```sh +gofmt -w git/managed/untrusted_tree.go git/managed/untrusted_tree_merge_test.go git/managed/lifecycle_mr_test.go +go test ./git/managed -run 'Test(UntrustedTreeMergeDriver|CreateWorktreeFromMergeRequest(IsolatesUntrustedTreeGitPrograms|NeutralizesCaseDistinctAttributeDrivers|DoesNotPATHSearchDriverHelpers|InspectsSelectedConfigFiles))' -count=1 +go test ./git/managed -count=1 +``` + +Expected: PASS. Inspect the focused test output to ensure binary rejection is represented as an ordinary conflict while missing executables and signal deaths are operation errors. + +Commit: + +```sh +git add git/managed/untrusted_tree.go git/managed/untrusted_tree_merge_test.go git/managed/lifecycle_mr_test.go +git commit -m "fix(git): preserve merge conflict contents" +``` + +## Task 4: Document the durable boundary and verify the repository + +**Files:** +- Modify: `git/AGENTS.md` + +### Step 1: Record the maintained invariant + +In the untrusted merge-request guidance, state that replacement merge drivers must: + +- invoke the resolved Git executable without a worktree `PATH` lookup; +- write clean text merges and diff3 markers for text conflicts; +- keep binary rejection as an ordinary per-file conflict; +- surface missing executables and process crashes as whole-operation errors; +- behave explicitly on Unix and Git for Windows. + +Do not document the wrapper string itself; document the behavior future implementations must preserve. + +### Step 2: Run fresh full verification + +Run: + +```sh +gofmt -w git/cmd/gitcmd.go git/cmd/gitcmd_test.go git/internal/shellquote/*.go git/managed/untrusted_tree.go git/managed/*_test.go +go test ./git/... -count=1 +go vet ./... +go test ./... -count=1 +make lint +git diff --check +git status --short +``` + +If `make lint` creates the ignored `custom-gcl` binary, leave it untracked/ignored and do not add it. Review `git diff origin/main...HEAD` for unrelated changes and private data before any push. + +### Step 3: Commit documentation if changed + +```sh +git add git/AGENTS.md +git commit -m "docs(git): define safe merge driver behavior" +``` + +Skip this commit if the existing text already expresses the complete invariant and no documentation edit is necessary. + +## Task 5: Push and open the Kit pull request + +### Step 1: Perform pre-push checks + +Follow `kenn:commit`, `kenn:scrub-private-data`, `superpowers:verification-before-completion`, and `kenn:commit-push-pr`. Confirm every accepted repository change is committed and the branch contains no credentials, private paths, private downstream names, or generated scratch files. + +### Step 2: Push the branch + +```sh +git push -u origin fix/merge-driver-conflict-markers +``` + +### Step 3: Open the PR + +Draft the body with `kenn:pr-desc`, describing the current result and reviewer-relevant tradeoffs. Do not include a routine `Validation` section and do not comment on the issue or poll CI. + +```sh +gh pr create \ + --base main \ + --head fix/merge-driver-conflict-markers \ + --title "fix(git): preserve merge conflict contents" \ + --body-file +``` + +The body should explain that imported worktrees now retain normal clean merges and diff3 text conflicts, binary conflicts remain per-file, and a moved/crashed pinned Git executable aborts instead of silently leaving current-only content. Link the originating public issue only if it is appropriate for the Kit repository's public context. From 8fc6395990f706ea97abd44a1552a38e719b22c5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:11:26 +0200 Subject: [PATCH 05/14] refactor(git): share shell quoting helper 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 --- git/cmd/gitcmd.go | 7 ++----- git/cmd/gitcmd_test.go | 3 ++- git/internal/shellquote/shellquote.go | 9 +++++++++ git/internal/shellquote/shellquote_test.go | 23 ++++++++++++++++++++++ 4 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 git/internal/shellquote/shellquote.go create mode 100644 git/internal/shellquote/shellquote_test.go diff --git a/git/cmd/gitcmd.go b/git/cmd/gitcmd.go index 432d050..7064b63 100644 --- a/git/cmd/gitcmd.go +++ b/git/cmd/gitcmd.go @@ -27,6 +27,7 @@ import ( "time" gitenv "go.kenn.io/kit/git/env" + "go.kenn.io/kit/git/internal/shellquote" ) // Config is one temporary git config entry injected through GIT_CONFIG_* env. @@ -187,15 +188,11 @@ func credentialHelper(path string) string { return `!f() { ` + `while IFS= read -r line && [ -n "$line" ]; do :; done; ` + `if [ "$1" = get ]; then ` + - `while IFS= read -r line; do printf '%s\n' "$line"; done < ` + shellSingleQuote(path) + `; ` + + `while IFS= read -r line; do printf '%s\n' "$line"; done < ` + shellquote.Single(path) + `; ` + `fi; ` + `}; f` } -func shellSingleQuote(value string) string { - return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" -} - var ( emptyGlobalConfigOnce sync.Once emptyGlobalConfigPath string diff --git a/git/cmd/gitcmd_test.go b/git/cmd/gitcmd_test.go index 98bac7e..a5fda2f 100644 --- a/git/cmd/gitcmd_test.go +++ b/git/cmd/gitcmd_test.go @@ -16,6 +16,7 @@ import ( Require "github.com/stretchr/testify/require" gitenv "go.kenn.io/kit/git/env" + "go.kenn.io/kit/git/internal/shellquote" ) func TestRunnerCommandUsesDefensiveEnvironment(t *testing.T) { @@ -566,7 +567,7 @@ func captureGitEnv(t *testing.T, runner Runner) string { binDir := t.TempDir() envPath := filepath.Join(t.TempDir(), "env") gitPath := filepath.Join(binDir, "git") - script := "#!/bin/sh\nenv > " + shellSingleQuote(envPath) + "\n" + script := "#!/bin/sh\nenv > " + shellquote.Single(envPath) + "\n" if os.PathSeparator == '\\' { gitPath += ".bat" script = "@echo off\r\nset > " + shellDoubleQuote(envPath) + "\r\n" diff --git a/git/internal/shellquote/shellquote.go b/git/internal/shellquote/shellquote.go new file mode 100644 index 0000000..c453bb6 --- /dev/null +++ b/git/internal/shellquote/shellquote.go @@ -0,0 +1,9 @@ +// Package shellquote quotes arguments embedded in Git shell command strings. +package shellquote + +import "strings" + +// Single returns value enclosed in POSIX shell single quotes. +func Single(value string) string { + return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" +} diff --git a/git/internal/shellquote/shellquote_test.go b/git/internal/shellquote/shellquote_test.go new file mode 100644 index 0000000..2e5a987 --- /dev/null +++ b/git/internal/shellquote/shellquote_test.go @@ -0,0 +1,23 @@ +package shellquote + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSingle(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name, input, want string + }{ + {name: "empty", want: "''"}, + {name: "spaces", input: "/opt/Git Tools/git", want: "'/opt/Git Tools/git'"}, + {name: "single quote", input: "/opt/Git's/git", want: "'/opt/Git'\\''s/git'"}, + } { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, test.want, Single(test.input)) + }) + } +} From 7d1f988551878477e42fffb59f84fdc405c6f21c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:20:10 +0200 Subject: [PATCH 06/14] fix(git): preserve merge conflict contents 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 --- git/managed/lifecycle_mr_test.go | 55 +++-- git/managed/untrusted_tree.go | 87 ++++++-- git/managed/untrusted_tree_merge_test.go | 259 +++++++++++++++++++++++ 3 files changed, 367 insertions(+), 34 deletions(-) create mode 100644 git/managed/untrusted_tree_merge_test.go diff --git a/git/managed/lifecycle_mr_test.go b/git/managed/lifecycle_mr_test.go index c10f2a0..6c282ad 100644 --- a/git/managed/lifecycle_mr_test.go +++ b/git/managed/lifecycle_mr_test.go @@ -69,6 +69,13 @@ func worktreeOnlyConfig(t *testing.T, dir, key string) string { return strings.TrimSpace(string(out)) } +func expectedSafeMergeDriverCommand(t *testing.T) string { + t.Helper() + path, err := resolveMergeDriverGitPath() + Require.NoError(t, err) + return safeMergeDriverCommand(path) +} + // TestCreateWorktreeFromMergeRequestSameRepo covers the same-repo scenario: // the head branch is fetched from origin, the new local branch starts at // it, and upstream tracking points at origin's head branch. @@ -519,7 +526,7 @@ func TestCreateWorktreeFromMergeRequestIsolatesUntrustedTreeGitPrograms(t *testi worktreeConfig(t, dest, "diff.owned.command")) assert.Equal(safeTextconvCommand, worktreeConfig(t, dest, "diff.owned.textconv")) - assert.Equal(safeMergeDriverCommand, + assert.Equal(expectedSafeMergeDriverCommand(t), worktreeConfig(t, dest, "merge.owned.driver")) if err := os.Remove(fsmonitorMarker); err != nil { @@ -669,7 +676,7 @@ func TestCreateWorktreeFromMergeRequestNeutralizesCaseDistinctAttributeDrivers( worktreeOnlyConfig(t, dest, "filter."+driver+".required")) assert.Equal(safeExternalDiffCommand, worktreeOnlyConfig(t, dest, "diff."+driver+".command")) - assert.Equal(safeMergeDriverCommand, + assert.Equal(expectedSafeMergeDriverCommand(t), worktreeOnlyConfig(t, dest, "merge."+driver+".driver")) } } @@ -684,23 +691,38 @@ func TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers( assert := assert.New(t) origin, clone := initOriginAndClone(t) marker := filepath.Join(t.TempDir(), "attacker-sh-ran") - lifecycleGit(t, origin, "checkout", "-q", "-b", "path-driver") require.NoError(os.WriteFile( filepath.Join(origin, ".gitattributes"), - []byte("payload diff=owned\n"), 0o644, + []byte("payload diff=owned merge=owned\n"), 0o644, )) require.NoError(os.WriteFile( - filepath.Join(origin, "payload"), []byte("external\n"), 0o644, + filepath.Join(origin, "payload"), []byte("base\n"), 0o644, )) + for _, helper := range []string{"git", "sh"} { + require.NoError(os.WriteFile( + filepath.Join(origin, helper), + []byte("#!/bin/sh\n: > \""+marker+"\"\nexit 0\n"), 0o755, + )) + } + lifecycleGit(t, origin, "add", ".gitattributes", "payload", "git", "sh") + lifecycleGit(t, origin, "commit", "-qm", "PATH driver base") + lifecycleGit(t, origin, "checkout", "-q", "-b", "path-driver") require.NoError(os.WriteFile( - filepath.Join(origin, "sh"), - []byte("#!/bin/sh\n: > \""+marker+"\"\nexit 0\n"), 0o755, + filepath.Join(origin, "payload"), []byte("current\n"), 0o644, )) - lifecycleGit(t, origin, "add", ".gitattributes", "payload", "sh") - lifecycleGit(t, origin, "commit", "-qm", "PATH driver fixture") + lifecycleGit(t, origin, "commit", "-qam", "PATH driver current") headSHA := lifecycleGit(t, origin, "rev-parse", "HEAD") lifecycleGit(t, origin, "checkout", "-q", "main") + lifecycleGit(t, origin, "checkout", "-q", "-b", "path-driver-other") + require.NoError(os.WriteFile( + filepath.Join(origin, "payload"), []byte("other\n"), 0o644, + )) + lifecycleGit(t, origin, "commit", "-qam", "PATH driver other") + lifecycleGit(t, origin, "checkout", "-q", "main") + lifecycleGit(t, clone, "fetch", "-q", "origin", + "refs/heads/path-driver-other:refs/remotes/origin/path-driver-other") lifecycleGit(t, clone, "config", "diff.owned.command", "false") + lifecycleGit(t, clone, "config", "merge.owned.driver", "false") dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( @@ -712,15 +734,14 @@ func TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers( ExpectedHeadSHA: headSHA, }) require.NoError(err) - require.NoError(os.WriteFile( - filepath.Join(dest, "payload"), []byte("changed\n"), 0o644, - )) - cmd := lifecycleGitCommand(t, dest, "diff", "--", "payload") + cmd := lifecycleGitCommand( + t, dest, "merge", "refs/remotes/origin/path-driver-other", + ) cmd.Env = append(cmd.Env, "PATH="+dest+":"+os.Getenv("PATH")) out, err := cmd.CombinedOutput() - require.NoError(err, string(out)) + require.Error(err, string(out)) assert.NoFileExists(marker) } @@ -867,7 +888,7 @@ func TestCreateWorktreeFromMergeRequestInspectsSelectedConfigFiles(t *testing.T) worktreeOnlyConfig(t, dest, "filter.selected.required")) assert.Equal(safeExternalDiffCommand, worktreeOnlyConfig(t, dest, "diff.selected.command")) - assert.Equal(safeMergeDriverCommand, + assert.Equal(expectedSafeMergeDriverCommand(t), worktreeOnlyConfig(t, dest, "merge.selected.driver")) } @@ -1054,11 +1075,11 @@ func TestSectionLevelConfigKeysDoNotNameSubsections(t *testing.T) { _, ok := configuredSubmoduleName("submodule.path") assert.False(t, ok) - assert.Empty(t, neutralizeAttributeDrivers([]string{ + assert.False(t, configuredAttributeDrivers([]string{ "filter.clean", "diff.command", "merge.driver", - })) + }).configured()) } func TestSafeTextconvPreservesUnterminatedLines(t *testing.T) { diff --git a/git/managed/untrusted_tree.go b/git/managed/untrusted_tree.go index 0376255..b66f8c9 100644 --- a/git/managed/untrusted_tree.go +++ b/git/managed/untrusted_tree.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "os" + "os/exec" "path/filepath" "regexp" "runtime" @@ -13,6 +14,7 @@ import ( "strings" gitcmd "go.kenn.io/kit/git/cmd" + "go.kenn.io/kit/git/internal/shellquote" ) // untrustedTreeIsolation neutralizes Git programs that a fetched tree can @@ -20,8 +22,9 @@ import ( // configuration remain trusted; only the fetched commit is treated as // untrusted. type untrustedTreeIsolation struct { - runner gitcmd.Runner - config []gitcmd.Config + runner gitcmd.Runner + config []gitcmd.Config + mergeDriverCommand string } // Git invokes these through its compiled-in shell because they contain shell @@ -31,9 +34,32 @@ type untrustedTreeIsolation struct { const ( safeExternalDiffCommand = `f() { emit() { prefix=$1; file=$2; line=; while IFS= read -r line; do printf '%s%s\n' "$prefix" "$line"; line=; done < "$file"; case "$line" in "") ;; *) printf '%s%s\n' "$prefix" "$line"; printf '%s\n' '\ No newline at end of file' ;; esac; }; emit - "$2"; emit + "$5"; }; f` safeTextconvCommand = `f() { line=; while IFS= read -r line; do printf '%s\n' "$line"; line=; done < "$1"; case "$line" in "") ;; *) printf '%s' "$line" ;; esac; }; f` - safeMergeDriverCommand = `f() { return 1; }; f` ) +func resolveMergeDriverGitPath() (string, error) { + path, err := exec.LookPath("git") + if err != nil { + return "", fmt.Errorf("resolve Git executable for safe merge driver: %w", err) + } + path, err = filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("make Git executable path absolute: %w", err) + } + path = filepath.Clean(path) + if runtime.GOOS == "windows" { + path = filepath.ToSlash(path) + } + return path, nil +} + +func safeMergeDriverCommand(gitPath string) string { + git := shellquote.Single(gitPath) + return `f() { [ -x ` + git + ` ] || return 129; ` + git + + ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + + `status=$?; if [ "$status" -eq 255 ]; then return 1; fi; ` + + `return "$status"; }; f %L "%A" "%O" "%B"` +} + var untrustedTreeGitVersionPattern = regexp.MustCompile( `(?i)git version (\d+)\.(\d+)(?:\.(\d+))?(?:\.windows\.(\d+))?(?:\s|$)`, ) @@ -149,6 +175,10 @@ func prepareUntrustedTreeIsolation( ); err != nil { return untrustedTreeIsolation{}, err } + gitPath, err := resolveMergeDriverGitPath() + if err != nil { + return untrustedTreeIsolation{}, err + } hooksPath, err := managedEmptyHooksPath(ctx, root) if err != nil { return untrustedTreeIsolation{}, err @@ -164,7 +194,11 @@ func prepareUntrustedTreeIsolation( for _, entry := range config { runner = runner.WithConfig(entry.Key, entry.Value) } - return untrustedTreeIsolation{runner: runner, config: config}, nil + return untrustedTreeIsolation{ + runner: runner, + config: config, + mergeDriverCommand: safeMergeDriverCommand(gitPath), + }, nil } func rejectCommandScopeIsolationOverrides( @@ -223,7 +257,7 @@ func isolationSensitiveConfigKey(key string) bool { strings.HasSuffix(lower, ".fetchrecursesubmodules") { return true } - return len(neutralizeAttributeDrivers([]string{key})) != 0 + return configuredAttributeDrivers([]string{key}).configured() } func managedEmptyHooksPath(ctx context.Context, root string) (string, error) { @@ -282,7 +316,9 @@ func completeUntrustedTreeIsolation( strings.Join(hooks, ", "), ) } - drivers := neutralizeAttributeDrivers(keys) + drivers := neutralizeAttributeDrivers( + configuredAttributeDrivers(keys), isolation.mergeDriverCommand, + ) submodules, err := submoduleFetchRecurseConfig( ctx, worktreePath, isolation.runner, ) @@ -523,10 +559,18 @@ func withoutGitRepositoryBindings(env []string) []string { return clean } -func neutralizeAttributeDrivers(keys []string) []gitcmd.Config { - filters := make(map[string]struct{}) - diffs := make(map[string]struct{}) - merges := make(map[string]struct{}) +type attributeDrivers struct { + filters map[string]struct{} + diffs map[string]struct{} + merges map[string]struct{} +} + +func configuredAttributeDrivers(keys []string) attributeDrivers { + drivers := attributeDrivers{ + filters: make(map[string]struct{}), + diffs: make(map[string]struct{}), + merges: make(map[string]struct{}), + } for _, key := range keys { lower := strings.ToLower(key) switch { @@ -534,25 +578,34 @@ func neutralizeAttributeDrivers(keys []string) []gitcmd.Config { if driver, ok := configuredDriverName( key, []string{".clean", ".smudge", ".process", ".required"}, ); ok { - filters[driver] = struct{}{} + drivers.filters[driver] = struct{}{} } case strings.HasPrefix(lower, "diff."): if driver, ok := configuredDriverName( key, []string{".command", ".textconv"}, ); ok { - diffs[driver] = struct{}{} + drivers.diffs[driver] = struct{}{} } case strings.HasPrefix(lower, "merge."): if driver, ok := configuredDriverName( key, []string{".driver"}, ); ok { - merges[driver] = struct{}{} + drivers.merges[driver] = struct{}{} } } } + return drivers +} + +func (drivers attributeDrivers) configured() bool { + return len(drivers.filters)+len(drivers.diffs)+len(drivers.merges) != 0 +} +func neutralizeAttributeDrivers( + drivers attributeDrivers, mergeDriverCommand string, +) []gitcmd.Config { var config []gitcmd.Config - for _, driver := range sortedDriverNames(filters) { + for _, driver := range sortedDriverNames(drivers.filters) { prefix := "filter." + driver config = append(config, gitcmd.Config{Key: prefix + ".clean", Value: ""}, @@ -561,16 +614,16 @@ func neutralizeAttributeDrivers(keys []string) []gitcmd.Config { gitcmd.Config{Key: prefix + ".required", Value: "false"}, ) } - for _, driver := range sortedDriverNames(diffs) { + for _, driver := range sortedDriverNames(drivers.diffs) { prefix := "diff." + driver config = append(config, gitcmd.Config{Key: prefix + ".command", Value: safeExternalDiffCommand}, gitcmd.Config{Key: prefix + ".textconv", Value: safeTextconvCommand}, ) } - for _, driver := range sortedDriverNames(merges) { + for _, driver := range sortedDriverNames(drivers.merges) { config = append(config, gitcmd.Config{ - Key: "merge." + driver + ".driver", Value: safeMergeDriverCommand, + Key: "merge." + driver + ".driver", Value: mergeDriverCommand, }) } return config diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go new file mode 100644 index 0000000..830b3b4 --- /dev/null +++ b/git/managed/untrusted_tree_merge_test.go @@ -0,0 +1,259 @@ +package managedworktree + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type mergeDriverFixture struct { + worktree string + otherRef string +} + +func newMergeDriverFixture( + t *testing.T, base, current, other []byte, + currentSubject, otherBranch string, +) mergeDriverFixture { + t.Helper() + require := require.New(t) + + if currentSubject == "" { + currentSubject = "current change" + } + if otherBranch == "" { + otherBranch = "merge-driver-other" + } + + origin, clone := initOriginAndClone(t) + require.NoError(os.WriteFile( + filepath.Join(origin, ".gitattributes"), + []byte("payload merge=owned\n"), 0o644, + )) + require.NoError(os.WriteFile( + filepath.Join(origin, "payload"), base, 0o644, + )) + require.NoError(os.WriteFile( + filepath.Join(origin, "binary.dat"), []byte("\x00base\n"), 0o644, + )) + lifecycleGit(t, origin, "add", ".gitattributes", "payload", "binary.dat") + lifecycleGit(t, origin, "commit", "-qm", "merge driver base") + + const currentBranch = "merge-driver-current" + lifecycleGit(t, origin, "checkout", "-q", "-b", currentBranch) + require.NoError(os.WriteFile( + filepath.Join(origin, "payload"), current, 0o644, + )) + lifecycleGit(t, origin, "commit", "-qam", currentSubject) + currentSHA := lifecycleGit(t, origin, "rev-parse", "HEAD") + + lifecycleGit(t, origin, "checkout", "-q", "main") + lifecycleGit(t, origin, "checkout", "-q", "-b", otherBranch) + require.NoError(os.WriteFile( + filepath.Join(origin, "payload"), other, 0o644, + )) + lifecycleGit(t, origin, "commit", "-qam", "other change") + otherSHA := lifecycleGit(t, origin, "rev-parse", "HEAD") + lifecycleGit(t, origin, "checkout", "-q", "main") + + otherRef := "refs/remotes/origin/" + otherBranch + lifecycleGit(t, clone, "fetch", "-q", "origin", + "refs/heads/"+otherBranch+":"+otherRef) + require.Equal(otherSHA, lifecycleGit(t, clone, "rev-parse", otherRef)) + lifecycleGit(t, clone, "config", "merge.owned.driver", "false") + + worktree := filepath.Join(t.TempDir(), "worktree") + _, err := CreateWorktreeFromMergeRequest( + t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), + ProjectRoot: clone, + Branch: "imported-current", + Path: worktree, + Number: 112, + HeadBranch: currentBranch, + HeadRepoCloneURL: origin, + ProjectRepoIdentity: identityOfCloneURL(origin), + ExpectedHeadSHA: currentSHA, + }, + ) + require.NoError(err) + + return mergeDriverFixture{worktree: worktree, otherRef: otherRef} +} + +func TestUntrustedTreeMergeDriverMergesNonOverlappingText(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("alpha\nmiddle\nomega\n"), + []byte("alpha current\nmiddle\nomega\n"), + []byte("alpha\nmiddle\nomega other\n"), + "", "", + ) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.NoError(err, string(out)) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Contains(string(contents), "alpha current") + assert.Contains(string(contents), "omega other") + assert.Empty(lifecycleGit(t, fixture.worktree, "ls-files", "--unmerged")) +} + +func TestUntrustedTreeMergeDriverWritesDiff3ConflictMarkers(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("before\nbase\nafter\n"), + []byte("before\ncurrent\nafter\n"), + []byte("before\nother\nafter\n"), + "", "", + ) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal( + "before\n<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\nafter\n", + string(contents), + ) +} + +func TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + markers := []string{ + "branch-dollar-marker", + "branch-backtick-marker", + "subject-dollar-marker", + "subject-backtick-marker", + } + fixture := newMergeDriverFixture(t, + []byte("base\n"), + []byte("current\n"), + []byte("other\n"), + "current $(touch${IFS}subject-dollar-marker) "+ + "`touch${IFS}subject-backtick-marker`", + "other-$(touch${IFS}branch-dollar-marker)-"+ + "`touch${IFS}branch-backtick-marker`", + ) + + cmd := lifecycleGitCommand(t, fixture.worktree, "rebase", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + for _, marker := range markers { + assert.NoFileExists(filepath.Join(fixture.worktree, marker)) + } + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + for _, want := range []string{ + "<<<<<<< current\n", + "||||||| base\n", + "=======\n", + ">>>>>>> other\n", + "base\n", + "current\n", + "other\n", + } { + assert.Contains(string(contents), want) + } +} + +func TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + + gitPath, err := exec.LookPath("git") + require.NoError(err) + gitContents, err := os.ReadFile(gitPath) + require.NoError(err) + copyName := "git" + if runtime.GOOS == "windows" { + copyName += filepath.Ext(gitPath) + } + gitCopy := filepath.Join(t.TempDir(), copyName) + require.NoError(os.WriteFile(gitCopy, gitContents, 0o755)) + require.NoError(os.Chmod(gitCopy, 0o755)) + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "merge.owned.driver", safeMergeDriverCommand(gitCopy)) + require.NoError(os.Remove(gitCopy)) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Empty(lifecycleGit(t, fixture.worktree, "status", "--short")) + assert.Empty(lifecycleGit(t, fixture.worktree, "ls-files", "--unmerged")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal("current\n", string(contents)) +} + +func TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + + otherWorktree := filepath.Join(t.TempDir(), "other") + lifecycleGit(t, fixture.worktree, "worktree", "add", "--detach", + otherWorktree, fixture.otherRef) + for _, worktree := range []string{fixture.worktree, otherWorktree} { + require.NoError(os.WriteFile( + filepath.Join(worktree, ".gitattributes"), + []byte("payload merge=owned\nbinary.dat merge=owned\n"), 0o644, + )) + } + currentBinary := []byte("\x00current\n") + otherBinary := []byte("\x00other\n") + require.NoError(os.WriteFile( + filepath.Join(fixture.worktree, "binary.dat"), currentBinary, 0o644, + )) + lifecycleGit(t, fixture.worktree, "add", ".gitattributes", "binary.dat") + lifecycleGit(t, fixture.worktree, "commit", "-qm", "current binary") + require.NoError(os.WriteFile( + filepath.Join(otherWorktree, "binary.dat"), otherBinary, 0o644, + )) + lifecycleGit(t, otherWorktree, "add", ".gitattributes", "binary.dat") + lifecycleGit(t, otherWorktree, "commit", "-qm", "other binary") + otherCommit := lifecycleGit(t, otherWorktree, "rev-parse", "HEAD") + lifecycleGit(t, fixture.worktree, "worktree", "remove", "--force", otherWorktree) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", otherCommit) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + status := strings.Split( + lifecycleGit(t, fixture.worktree, "status", "--short"), "\n", + ) + assert.ElementsMatch([]string{"UU binary.dat", "UU payload"}, status) + binary, err := os.ReadFile(filepath.Join(fixture.worktree, "binary.dat")) + require.NoError(err) + assert.Equal(currentBinary, binary) + assert.NotContains(string(binary), "<<<<<<<") + payload, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Contains(string(payload), "<<<<<<< current") + assert.Contains(string(payload), "||||||| base") + assert.Contains(string(payload), ">>>>>>> other") + // Git merge-file names its temporary %A file in this expected diagnostic. + assert.Contains(string(out), "Cannot merge binary files: ") +} From 2914858a1b5c4561a5e662cee0ef8c68b81f1a3c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:30:53 +0200 Subject: [PATCH 07/14] test(git): prove safe merge driver paths 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 --- git/managed/lifecycle_mr_test.go | 28 ++++++- git/managed/untrusted_tree.go | 8 +- git/managed/untrusted_tree_merge_test.go | 102 +++++++++++++++-------- 3 files changed, 96 insertions(+), 42 deletions(-) diff --git a/git/managed/lifecycle_mr_test.go b/git/managed/lifecycle_mr_test.go index 6c282ad..a880cab 100644 --- a/git/managed/lifecycle_mr_test.go +++ b/git/managed/lifecycle_mr_test.go @@ -735,13 +735,35 @@ func TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers( }) require.NoError(err) - cmd := lifecycleGitCommand( + require.NoError(os.WriteFile( + filepath.Join(dest, "payload"), []byte("changed\n"), 0o644, + )) + diffCmd := lifecycleGitCommand(t, dest, "diff", "--", "payload") + diffCmd.Env = append(diffCmd.Env, "PATH="+dest+":"+os.Getenv("PATH")) + diff, err := diffCmd.CombinedOutput() + require.NoError(err, string(diff)) + assert.Contains(string(diff), "-current") + assert.Contains(string(diff), "+changed") + assert.NoFileExists(marker) + require.NoError(os.WriteFile( + filepath.Join(dest, "payload"), []byte("current\n"), 0o644, + )) + + mergeCmd := lifecycleGitCommand( t, dest, "merge", "refs/remotes/origin/path-driver-other", ) - cmd.Env = append(cmd.Env, "PATH="+dest+":"+os.Getenv("PATH")) - out, err := cmd.CombinedOutput() + mergeCmd.Env = append(mergeCmd.Env, "PATH="+dest+":"+os.Getenv("PATH")) + out, err := mergeCmd.CombinedOutput() require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, dest, "status", "--short", "--", "payload")) + payload, readErr := os.ReadFile(filepath.Join(dest, "payload")) + require.NoError(readErr) + assert.Equal( + "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", + string(payload), + ) assert.NoFileExists(marker) } diff --git a/git/managed/untrusted_tree.go b/git/managed/untrusted_tree.go index b66f8c9..2c27562 100644 --- a/git/managed/untrusted_tree.go +++ b/git/managed/untrusted_tree.go @@ -28,9 +28,11 @@ type untrustedTreeIsolation struct { } // Git invokes these through its compiled-in shell because they contain shell -// syntax. They use only shell builtins, so an untrusted worktree cannot steer -// them through PATH. The diff replacement emits a simple old/new rendering; -// the merge replacement declines the custom driver so Git reports a conflict. +// syntax. The diff and textconv replacements use only shell builtins. The +// merge replacement invokes the resolved Git executable by absolute path, so +// an untrusted worktree cannot steer a helper through PATH. The diff replacement +// emits a simple old/new rendering; the merge replacement performs a fixed-label +// diff3 merge. const ( safeExternalDiffCommand = `f() { emit() { prefix=$1; file=$2; line=; while IFS= read -r line; do printf '%s%s\n' "$prefix" "$line"; line=; done < "$file"; case "$line" in "") ;; *) printf '%s%s\n' "$prefix" "$line"; printf '%s\n' '\ No newline at end of file' ;; esac; }; emit - "$2"; emit + "$5"; }; f` safeTextconvCommand = `f() { line=; while IFS= read -r line; do printf '%s\n' "$line"; line=; done < "$1"; case "$line" in "") ;; *) printf '%s' "$line" ;; esac; }; f` diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go index 830b3b4..0b29cc3 100644 --- a/git/managed/untrusted_tree_merge_test.go +++ b/git/managed/untrusted_tree_merge_test.go @@ -133,44 +133,74 @@ func TestUntrustedTreeMergeDriverWritesDiff3ConflictMarkers(t *testing.T) { } func TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels(t *testing.T) { - require := require.New(t) - assert := assert.New(t) - markers := []string{ - "branch-dollar-marker", - "branch-backtick-marker", - "subject-dollar-marker", - "subject-backtick-marker", - } - fixture := newMergeDriverFixture(t, - []byte("base\n"), - []byte("current\n"), - []byte("other\n"), - "current $(touch${IFS}subject-dollar-marker) "+ - "`touch${IFS}subject-backtick-marker`", - "other-$(touch${IFS}branch-dollar-marker)-"+ - "`touch${IFS}branch-backtick-marker`", - ) + t.Run("merge branch label", func(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + markers := []string{"branch-dollar-marker", "branch-backtick-marker"} + fixture := newMergeDriverFixture(t, + []byte("base\n"), + []byte("current\n"), + []byte("other\n"), + "", + "other-$(touch${IFS}branch-dollar-marker)-"+ + "`touch${IFS}branch-backtick-marker`", + ) - cmd := lifecycleGitCommand(t, fixture.worktree, "rebase", fixture.otherRef) - out, err := cmd.CombinedOutput() + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() - require.Error(err, string(out)) - for _, marker := range markers { - assert.NoFileExists(filepath.Join(fixture.worktree, marker)) - } - contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) - require.NoError(err) - for _, want := range []string{ - "<<<<<<< current\n", - "||||||| base\n", - "=======\n", - ">>>>>>> other\n", - "base\n", - "current\n", - "other\n", - } { - assert.Contains(string(contents), want) - } + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + for _, marker := range markers { + assert.NoFileExists(filepath.Join(fixture.worktree, marker)) + } + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal( + "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", + string(contents), + ) + }) + + t.Run("rebase commit subject label", func(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + markers := []string{"subject-dollar-marker", "subject-backtick-marker"} + fixture := newMergeDriverFixture(t, + []byte("base\n"), + []byte("current\n"), + []byte("other\n"), + "current $(touch${IFS}subject-dollar-marker) "+ + "`touch${IFS}subject-backtick-marker`", + "", + ) + + cmd := lifecycleGitCommand(t, fixture.worktree, "rebase", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + for _, marker := range markers { + assert.NoFileExists(filepath.Join(fixture.worktree, marker)) + } + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + for _, want := range []string{ + "<<<<<<< current\n", + "||||||| base\n", + "=======\n", + ">>>>>>> other\n", + "base\n", + "current\n", + "other\n", + } { + assert.Contains(string(contents), want) + } + assert.NotContains(string(contents), "subject-dollar-marker") + assert.NotContains(string(contents), "subject-backtick-marker") + }) } func TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears(t *testing.T) { From 78402284e0db8c6b73199d2f0271c660af18cdef Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:36:51 +0200 Subject: [PATCH 08/14] docs(git): define safe merge driver behavior 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 --- git/AGENTS.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/git/AGENTS.md b/git/AGENTS.md index 53c238c..32461d4 100644 --- a/git/AGENTS.md +++ b/git/AGENTS.md @@ -32,6 +32,12 @@ specific application or forge workflow. settings in the imported worktree. This is a narrow Git execution boundary, not an OS sandbox for hostile configuration, remotes, lifecycle scripts, same-user replacement races, or resource exhaustion. +- Replacement merge drivers for untrusted merge-request imports must run the + resolved Git executable without looking it up through the worktree `PATH`. + They must write clean text merges and diff3 markers for text conflicts, while + treating binary rejection as an ordinary per-file conflict. A missing Git + executable or a merge-process crash must fail the whole operation. Keep the + Unix and Git for Windows behaviors explicit. - Reject isolation-sensitive command-scope configuration during import because worktree configuration cannot outrank it. Explicit command-scope overrides on later Git commands are caller policy, not a sandbox boundary Kit can From 5a7d905554870075dabb200e1cb017a88ca0d359 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Aug 2026 23:52:33 +0200 Subject: [PATCH 09/14] fix(git): escape merge driver template paths 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 --- .../plans/2026-08-29-safe-merge-driver.md | 13 ++++-- .../2026-08-29-safe-merge-driver-design.md | 30 ++++++++---- git/managed/untrusted_tree.go | 4 +- git/managed/untrusted_tree_merge_test.go | 46 +++++++++++++++++++ 4 files changed, 78 insertions(+), 15 deletions(-) diff --git a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md index 44d6403..b483ba9 100644 --- a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md +++ b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md @@ -4,9 +4,9 @@ **Goal:** Replace untrusted-tree custom merge drivers with a durable driver that performs ordinary three-way text merges, writes diff3 conflict markers, keeps binary conflicts local to the file, and fails the whole Git operation when the pinned Git executable cannot run. -**Architecture:** Resolve the same `git` executable used by `git/cmd.Runner` before materializing the untrusted tree, persist an absolute shell-quoted path in worktree config, and invoke `git merge-file` from a small shell wrapper. Keep attribute-driver key classification independent from command construction so command-scope checks do not require path resolution. Move the existing POSIX single-quote helper into `git/internal/shellquote` so both owning packages share the escaping rule. +**Architecture:** Resolve the same `git` executable used by `git/cmd.Runner` before materializing the untrusted tree, escape Git-template percent signs, persist an absolute shell-quoted path in worktree config, and invoke `git merge-file` from a small shell wrapper. Keep attribute-driver key classification independent from command construction so command-scope checks do not require path resolution. Move the existing POSIX single-quote helper into `git/internal/shellquote` so both owning packages share the shell-escaping rule; keep percent escaping local to merge-driver construction. -**Tech Stack:** Go 1.26, Git 2.39.1+, `testify`, standard `os/exec`, repository lifecycle fixtures. +**Tech Stack:** Go 1.27, Git 2.39.1+ on non-Windows platforms, Git for Windows 2.53.0.windows.3+, `testify`, standard `os/exec`, repository lifecycle fixtures. Fixed merge labels add no new Git version floor. --- @@ -119,6 +119,7 @@ Add these tests without any Windows skip: 3. `TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels` — use a branch name and rebase commit subject containing `$()` and backticks that would create relative marker files; assert neither marker exists and conflict markers still use only the fixed labels. 4. `TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears` — copy the resolved executable to a temporary executable path, replace only the fixture worktree's `merge.owned.driver` value with the command constructed for that copy, remove it before merge, and assert Git aborts with a clean worktree rather than recording `UU` with current-only contents. 5. `TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal` — mark both `binary.dat` and `payload` with the same driver; conflict both in one merge; assert both are `UU`, binary bytes remain current without text markers, and `payload` contains diff3 markers. Accept and document the `Cannot merge binary files` stderr line naming Git's temporary file. +6. `TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath` — resolve a copied Git executable from a path containing literal `%Y`, merge a branch label containing `$()` and backticks, and assert no payload executes while Git records the ordinary fixed-label diff3 conflict. For the hostile-label test, use relative marker names valid under both Git for Windows' shell and POSIX shells; do not embed a platform-specific absolute path in a ref name. For the missing-executable test, use `git config --worktree merge.owned.driver ` after import. This tests the persisted-command failure contract without adding a production seam; ordinary production preparation still calls `exec.LookPath` exactly once. @@ -199,7 +200,7 @@ Build it as one line for Git config: ```go func safeMergeDriverCommand(gitPath string) string { - git := shellquote.Single(gitPath) + git := shellquote.Single(strings.ReplaceAll(gitPath, "%", "%%")) return `f() { [ -x ` + git + ` ] || return 129; ` + git + ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + `status=$?; if [ "$status" -eq 255 ]; then return 1; fi; ` + @@ -207,7 +208,11 @@ func safeMergeDriverCommand(gitPath string) string { } ``` -Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. +Git expands merge-driver placeholders before invoking the shell. Doubling +literal percent signs is therefore Git-template escaping and stays local to +this builder; `shellquote.Single` continues to own only POSIX shell quoting. + +Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. The fixed labels add no version requirement beyond the existing non-Windows Git 2.39.1 and Git for Windows 2.53.0.windows.3 import floors. Map only status 255 to conflict. Do not use `-gt 128`, and do not remap 126 or 127: signal deaths must remain Git operation errors, while `merge-file` can legitimately report text conflict counts through 127. The executable guard returns 129 before invocation. An internal `merge-file` `die()` can still exit 128 and be treated by Git as a conflict; Git's custom-driver protocol provides no distinguishable alternative. diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md index 8a32fa1..2feda3b 100644 --- a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md +++ b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md @@ -39,12 +39,19 @@ absolute, cleaned path and fails the import if no executable can be resolved. On Windows, Kit emits the drive-qualified path with forward slashes before shell quoting it. Git for Windows runs custom drivers through its POSIX shell, and the forward-slash form works for both the shell's executable check and the -Windows executable loader. +Windows executable loader. Existing untrusted-tree imports require Git 2.39.1 +or newer on non-Windows platforms and Git for Windows 2.53.0.windows.3 or +newer. Kit moves the existing POSIX shell single-quote helper into a shared internal Git package. Both the credential helper and managed-worktree isolation use it, -so executable paths remain data without adding a public API or duplicating -shell quoting. +so executable paths remain shell data without adding a public API or +duplicating shell quoting. Merge-driver configuration has an earlier template +layer: Git expands percent placeholders before it invokes the shell. The merge +driver builder therefore doubles every literal `%` in the executable path +before passing that path to the POSIX shell-quote helper. This percent escaping +stays local to merge-driver construction because it is a Git-template rule, +not generic shell quoting. Kit builds a shell function around the trusted absolute path. In expanded form, the function has these semantics: @@ -72,8 +79,9 @@ already insert `%S`, `%X`, and `%Y` as shell-single-quoted strings. Placing thos label placeholders inside another pair of double quotes would make command substitutions in a branch name or commit subject executable. The replacement therefore uses fixed labels and does not interpolate `%S`, `%X`, or `%Y` at all. -Fixed labels also keep the existing Git 2.39.1 minimum; no version-dependent -fallback or higher version floor is needed. +Fixed labels add no new version floor: the existing non-Windows Git 2.39.1 and +Git for Windows 2.53.0.windows.3 minimums remain in force, with no +version-dependent fallback. `git merge-file` overwrites `%A`, exits with status 0 for a clean merge, and returns the conflict count, capped at 127, when text conflicts remain. The @@ -126,14 +134,16 @@ worktree fixtures and exercise the persisted replacement after import: 2. Overlapping edits leave an unmerged path whose working file contains diff3 markers with the fixed labels and the base, current, and other content. 3. Adversarial branch and commit labels remain inert during a conflicted merge. -4. A missing persisted executable aborts the merge and leaves a clean tree. -5. Binary content produces an ordinary per-file conflict instead of aborting +4. A resolved Git path containing literal `%Y` cannot expand an untrusted + branch label before shell quoting or execute its payload. +5. A missing persisted executable aborts the merge and leaves a clean tree. +6. Binary content produces an ordinary per-file conflict instead of aborting the whole merge. -6. The existing PATH-hijack fixture is extended so a fake `git` executable +7. The existing PATH-hijack fixture is extended so a fake `git` executable placed before the trusted executable is not invoked during a merge. Focused unit coverage moves with the shared single-quote helper and covers executable paths containing spaces and single quotes. Existing assertions for the persisted merge-driver value compare against the computed command. -Behavioral tests 1 through 5 run on Unix and Windows, including the emitted -Windows path form. Only the POSIX PATH-hijack fixture in test 6 skips Windows. +Behavioral tests 1 through 6 run on Unix and Windows, including the emitted +Windows path form. Only the POSIX PATH-hijack fixture in test 7 skips Windows. diff --git a/git/managed/untrusted_tree.go b/git/managed/untrusted_tree.go index 2c27562..97f716e 100644 --- a/git/managed/untrusted_tree.go +++ b/git/managed/untrusted_tree.go @@ -55,7 +55,9 @@ func resolveMergeDriverGitPath() (string, error) { } func safeMergeDriverCommand(gitPath string) string { - git := shellquote.Single(gitPath) + // Git expands merge-driver placeholders before the shell parses the + // command. Double literal percent signs before applying shell quoting. + git := shellquote.Single(strings.ReplaceAll(gitPath, "%", "%%")) return `f() { [ -x ` + git + ` ] || return 129; ` + git + ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + `status=$?; if [ "$status" -eq 255 ]; then return 1; fi; ` + diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go index 0b29cc3..0040690 100644 --- a/git/managed/untrusted_tree_merge_test.go +++ b/git/managed/untrusted_tree_merge_test.go @@ -203,6 +203,52 @@ func TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels(t *testing.T) { }) } +func TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + gitPath, err := exec.LookPath("git") + require.NoError(err) + gitContents, err := os.ReadFile(gitPath) + require.NoError(err) + gitDir := filepath.Join(t.TempDir(), "git-%Y-bin") + require.NoError(os.MkdirAll(gitDir, 0o755)) + copyName := "git" + if runtime.GOOS == "windows" { + copyName += filepath.Ext(gitPath) + } + gitCopy := filepath.Join(gitDir, copyName) + require.NoError(os.WriteFile(gitCopy, gitContents, 0o755)) + require.NoError(os.Chmod(gitCopy, 0o755)) + t.Setenv("PATH", gitDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + markers := []string{"percent-dollar-marker", "percent-backtick-marker"} + fixture := newMergeDriverFixture(t, + []byte("base\n"), + []byte("current\n"), + []byte("other\n"), + "", + "other-$(touch${IFS}percent-dollar-marker)-"+ + "`touch${IFS}percent-backtick-marker`", + ) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + for _, marker := range markers { + assert.NoFileExists(filepath.Join(fixture.worktree, marker)) + } + payload, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal( + "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", + string(payload), + ) +} + func TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears(t *testing.T) { require := require.New(t) assert := assert.New(t) From 0eb84a532dc9ae3a36dfeb490fd1c32624492a83 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 30 Aug 2026 00:17:00 +0200 Subject: [PATCH 10/14] fix(git): distinguish merge driver failures 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 --- .../plans/2026-08-29-safe-merge-driver.md | 64 +++++++------- .../2026-08-29-safe-merge-driver-design.md | 86 +++++++++++-------- git/AGENTS.md | 11 ++- git/managed/lifecycle_mr_test.go | 12 +-- git/managed/untrusted_tree.go | 37 ++++++-- git/managed/untrusted_tree_merge_test.go | 52 ++++++++++- git/managed/untrusted_tree_test.go | 5 +- 7 files changed, 180 insertions(+), 87 deletions(-) diff --git a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md index b483ba9..0fb478c 100644 --- a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md +++ b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md @@ -4,9 +4,9 @@ **Goal:** Replace untrusted-tree custom merge drivers with a durable driver that performs ordinary three-way text merges, writes diff3 conflict markers, keeps binary conflicts local to the file, and fails the whole Git operation when the pinned Git executable cannot run. -**Architecture:** Resolve the same `git` executable used by `git/cmd.Runner` before materializing the untrusted tree, escape Git-template percent signs, persist an absolute shell-quoted path in worktree config, and invoke `git merge-file` from a small shell wrapper. Keep attribute-driver key classification independent from command construction so command-scope checks do not require path resolution. Move the existing POSIX single-quote helper into `git/internal/shellquote` so both owning packages share the shell-escaping rule; keep percent escaping local to merge-driver construction. +**Architecture:** Resolve the same `git` executable used by `git/cmd.Runner` before materializing the untrusted tree, escape Git-template percent signs, persist an absolute shell-quoted path in worktree config, classify binary inputs without repository attributes or external helpers, and invoke `git merge-file` only for text. Keep attribute-driver key classification independent from command construction so command-scope checks do not require path resolution. Move the existing POSIX single-quote helper into `git/internal/shellquote` so both owning packages share the shell-escaping rule; keep percent escaping local to merge-driver construction. -**Tech Stack:** Go 1.27, Git 2.39.1+ on non-Windows platforms, Git for Windows 2.53.0.windows.3+, `testify`, standard `os/exec`, repository lifecycle fixtures. Fixed merge labels add no new Git version floor. +**Tech Stack:** Go 1.27, Git 2.42.0+ on non-Windows platforms, Git for Windows 2.53.0.windows.3+, `testify`, standard `os/exec`, repository lifecycle fixtures. Git 2.42 is required so custom merge-driver operation-error statuses are not collapsed into conflicts; fixed merge labels add no further floor. --- @@ -118,8 +118,9 @@ Add these tests without any Windows skip: 2. `TestUntrustedTreeMergeDriverWritesDiff3ConflictMarkers` — overlap one line; assert ordinary conflict, `UU payload`, and exact marker labels `<<<<<<< current`, `||||||| base`, `=======`, `>>>>>>> other` with all three bodies. 3. `TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels` — use a branch name and rebase commit subject containing `$()` and backticks that would create relative marker files; assert neither marker exists and conflict markers still use only the fixed labels. 4. `TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears` — copy the resolved executable to a temporary executable path, replace only the fixture worktree's `merge.owned.driver` value with the command constructed for that copy, remove it before merge, and assert Git aborts with a clean worktree rather than recording `UU` with current-only contents. -5. `TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal` — mark both `binary.dat` and `payload` with the same driver; conflict both in one merge; assert both are `UU`, binary bytes remain current without text markers, and `payload` contains diff3 markers. Accept and document the `Cannot merge binary files` stderr line naming Git's temporary file. +5. `TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal` — mark both `binary.dat` and `payload` with the same driver; use worktree diff attributes that falsely force binary to text and text to binary; conflict both in one merge; assert both are `UU`, binary bytes remain current without text markers or a `merge-file` binary diagnostic, and `payload` contains diff3 markers. 6. `TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath` — resolve a copied Git executable from a path containing literal `%Y`, merge a branch label containing `$()` and backticks, and assert no payload executes while Git records the ordinary fixed-label diff3 conflict. +7. `TestUntrustedTreeMergeDriverTreatsMergeFile255AsOperationError` — on POSIX, use a behavioral executable wrapper that delegates classification but returns 255 from `merge-file`; assert the merge aborts without recording an unmerged current-only file. For the hostile-label test, use relative marker names valid under both Git for Windows' shell and POSIX shells; do not embed a platform-specific absolute path in a ref name. For the missing-executable test, use `git config --worktree merge.owned.driver ` after import. This tests the persisted-command failure contract without adding a production seam; ordinary production preparation still calls `exec.LookPath` exactly once. @@ -180,41 +181,40 @@ type untrustedTreeIsolation struct { ### Step 2: Construct the exact wrapper -Replace the constant with a builder. The generated command must be semantically equivalent to: +Replace the constant with a builder that accepts the pinned executable and the +managed empty directory. Before changing the subprocess cwd, convert `%A`, +`%O`, and `%B` to absolute paths. Compare current/base and base/other with: ```sh -f() { - [ -x '' ] || return 129 - '' merge-file --diff3 --marker-size="$1" \ - -L current -L base -L other "$2" "$3" "$4" - status=$? - if [ "$status" -eq 255 ]; then - return 1 - fi - return "$status" -} -f %L "%A" "%O" "%B" +GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES='' \ + '' -c core.attributesFile= -C '' \ + diff --no-index --numstat --no-ext-diff --no-textconv -- "$left" "$right" ``` -Build it as one line for Git config: +Treat a `-\t-` numstat result as binary and return status 1 before +`merge-file`. Treat a classifier command error, empty output for a differing +pair, or malformed output as operation error 129. For text, invoke: -```go -func safeMergeDriverCommand(gitPath string) string { - git := shellquote.Single(strings.ReplaceAll(gitPath, "%", "%%")) - return `f() { [ -x ` + git + ` ] || return 129; ` + git + - ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + - `status=$?; if [ "$status" -eq 255 ]; then return 1; fi; ` + - `return "$status"; }; f %L "%A" "%O" "%B"` -} +```sh +'' merge-file --diff3 --marker-size="$1" \ + -L current -L base -L other "$2" "$3" "$4" ``` +Pass every `merge-file` status through unchanged. + Git expands merge-driver placeholders before invoking the shell. Doubling literal percent signs is therefore Git-template escaping and stays local to this builder; `shellquote.Single` continues to own only POSIX shell quoting. -Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. The fixed labels add no version requirement beyond the existing non-Windows Git 2.39.1 and Git for Windows 2.53.0.windows.3 import floors. +Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. The fixed labels add no version requirement beyond the non-Windows Git 2.42.0 and Git for Windows 2.53.0.windows.3 import floors. -Map only status 255 to conflict. Do not use `-gt 128`, and do not remap 126 or 127: signal deaths must remain Git operation errors, while `merge-file` can legitimately report text conflict counts through 127. The executable guard returns 129 before invocation. An internal `merge-file` `die()` can still exit 128 and be treated by Git as a conflict; Git's custom-driver protocol provides no distinguishable alternative. +The classifier must use the pinned executable, disable system/global/worktree +attributes, prevent repository discovery, and pass both `--no-ext-diff` and +`--no-textconv`, so it cannot invoke a tree-selected helper. Do not map status +255 from `merge-file`: it represents both binary rejection and input/output +failures. Preclassification owns the binary case; all text-merge statuses pass +unchanged. Git 2.42 is the minimum non-Windows version because earlier Git +releases turn every positive custom-driver status into an ordinary conflict. ### Step 3: Separate classification from construction @@ -245,11 +245,13 @@ Use `configuredAttributeDrivers([]string{key}).configured()` in `isolationSensit Add one test helper used by all three existing config assertions: ```go -func expectedSafeMergeDriverCommand(t *testing.T) string { +func expectedSafeMergeDriverCommand(t *testing.T, worktree string) string { t.Helper() path, err := resolveMergeDriverGitPath() require.NoError(t, err) - return safeMergeDriverCommand(path) + hooksPath := worktreeConfig(t, worktree, "core.hooksPath") + require.NotEmpty(t, hooksPath) + return safeMergeDriverCommand(path, hooksPath) } ``` @@ -284,9 +286,13 @@ git commit -m "fix(git): preserve merge conflict contents" In the untrusted merge-request guidance, state that replacement merge drivers must: - invoke the resolved Git executable without a worktree `PATH` lookup; +- require Git 2.42.0+ on non-Windows and Git for Windows + 2.53.0.windows.3+; +- classify binary inputs without repository attributes or external helpers; - write clean text merges and diff3 markers for text conflicts; - keep binary rejection as an ordinary per-file conflict; -- surface missing executables and process crashes as whole-operation errors; +- surface classifier failures, text-merge I/O failures, missing executables, + and process crashes as whole-operation errors; - behave explicitly on Unix and Git for Windows. Do not document the wrapper string itself; document the behavior future implementations must preserve. diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md index 2feda3b..9eebcf6 100644 --- a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md +++ b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md @@ -39,19 +39,21 @@ absolute, cleaned path and fails the import if no executable can be resolved. On Windows, Kit emits the drive-qualified path with forward slashes before shell quoting it. Git for Windows runs custom drivers through its POSIX shell, and the forward-slash form works for both the shell's executable check and the -Windows executable loader. Existing untrusted-tree imports require Git 2.39.1 +Windows executable loader. Existing untrusted-tree imports require Git 2.42.0 or newer on non-Windows platforms and Git for Windows 2.53.0.windows.3 or -newer. +newer. Before Git 2.42, every positive custom merge-driver status is treated as +an ordinary conflict, so a driver cannot distinguish a per-file conflict from +an operation error. Fixed labels themselves add no new version floor. Kit moves the existing POSIX shell single-quote helper into a shared internal Git package. Both the credential helper and managed-worktree isolation use it, so executable paths remain shell data without adding a public API or duplicating shell quoting. Merge-driver configuration has an earlier template layer: Git expands percent placeholders before it invokes the shell. The merge -driver builder therefore doubles every literal `%` in the executable path -before passing that path to the POSIX shell-quote helper. This percent escaping -stays local to merge-driver construction because it is a Git-template rule, -not generic shell quoting. +driver builder therefore doubles every literal `%` in embedded executable and +managed-directory paths before passing those paths to the POSIX shell-quote +helper. This percent escaping stays local to merge-driver construction because +it is a Git-template rule, not generic shell quoting. Kit builds a shell function around the trusted absolute path. In expanded form, the function has these semantics: @@ -59,13 +61,15 @@ the function has these semantics: ```sh f() { [ -x '' ] || return 129 + # Convert the three merge inputs to absolute paths before changing Git's cwd. + # Classify current/base and base/other with the pinned Git executable: + # GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES='' \ + # '' -c core.attributesFile= -C '' \ + # diff --no-index --numstat --no-ext-diff --no-textconv -- "$left" "$right" + # Return 1 before merge-file for binary content and 129 on classifier errors. '' merge-file --diff3 --marker-size="$1" \ -L current -L base -L other "$2" "$3" "$4" - status=$? - if [ "$status" -eq 255 ]; then - return 1 - fi - return "$status" + return $? } f %L "%A" "%O" "%B" ``` @@ -79,9 +83,20 @@ already insert `%S`, `%X`, and `%Y` as shell-single-quoted strings. Placing thos label placeholders inside another pair of double quotes would make command substitutions in a branch name or commit subject executable. The replacement therefore uses fixed labels and does not interpolate `%S`, `%X`, or `%Y` at all. -Fixed labels add no new version floor: the existing non-Windows Git 2.39.1 and -Git for Windows 2.53.0.windows.3 minimums remain in force, with no -version-dependent fallback. +Fixed labels add no version requirement beyond the non-Windows Git 2.42.0 and +Git for Windows 2.53.0.windows.3 minimums, with no version-dependent fallback. + +Before invoking `merge-file`, the wrapper asks the pinned Git executable to +classify the inputs with `diff --no-index --numstat`. It converts the temporary +input names to absolute paths, runs Git from the managed empty hooks directory, +and sets the discovery ceiling to that directory's parent. System and global +attribute files are disabled, and repository discovery is prevented, so +worktree attributes cannot falsely force content to text or binary. The +`--no-ext-diff` and `--no-textconv` flags prevent configured diff and textconv +helpers from running. A binary numstat result returns status 1 before +`merge-file`; a classifier command error or malformed result returns status +129. The wrapper compares current to base and base to other so an unchanged +pair cannot hide binary content in the remaining side. `git merge-file` overwrites `%A`, exits with status 0 for a clean merge, and returns the conflict count, capped at 127, when text conflicts remain. The @@ -105,23 +120,19 @@ or removed executable returns status 129 before `merge-file` starts. Git treats that status as a driver failure and aborts the operation instead of recording an ours-only conflict. -`git merge-file` rejects binary content with status 255. After the executable -guard succeeds, the wrapper maps that exact status to status 1. -This preserves the previous and built-in binary-driver behavior: Git keeps the -current bytes, marks that path conflicted, and continues processing other -paths. Binary files do not receive text conflict markers. The guard runs first, -so an unavailable persisted executable is not converted into an ordinary -conflict. `merge-file` still writes its binary-file diagnostic to standard -error, including Git's temporary filename. - -All other statuses pass through unchanged. In particular, a signal death such -as status 139 remains a driver failure instead of becoming an ours-only -conflict. Statuses 126 and 127 also remain unchanged because `merge-file` can -legitimately return those conflict counts. The executable guard covers a stable -missing or non-executable path, but not a same-user replacement race between the -check and invocation. Git also treats `merge-file`'s own status 128 as an -ordinary conflict; its status contract provides no distinct value that the -wrapper can safely reinterpret. +Binary classification returns status 1 without invoking `merge-file`. Git +keeps the current bytes, marks that path conflicted, and continues processing +other paths. Binary files receive neither text markers nor `merge-file`'s +temporary-file diagnostic. + +Every `merge-file` status passes through unchanged. In particular, status 255 +is not assumed to mean binary: `merge-file` also uses it for input, output, +read, write, and close failures. With the Git 2.42 floor, that status aborts the +operation instead of becoming an ours-only conflict. A signal death such as +status 139 likewise remains an operation error, while statuses 1 through 127 +remain valid text conflict counts. The executable guard covers a stable +missing or non-executable path, but not a same-user replacement race between +the check and invocation. Existing import cleanup and rollback behavior remains unchanged. @@ -137,13 +148,18 @@ worktree fixtures and exercise the persisted replacement after import: 4. A resolved Git path containing literal `%Y` cannot expand an untrusted branch label before shell quoting or execute its payload. 5. A missing persisted executable aborts the merge and leaves a clean tree. -6. Binary content produces an ordinary per-file conflict instead of aborting - the whole merge. -7. The existing PATH-hijack fixture is extended so a fake `git` executable +6. Binary content produces an ordinary per-file conflict alongside a text + conflict, even when worktree attributes try to force the opposite content + classifications. +7. A simulated non-binary `merge-file` status 255 aborts the operation without + leaving a current-only conflict. +8. Git 2.41 is rejected and Git 2.42 is accepted on non-Windows platforms. +9. The existing PATH-hijack fixture is extended so a fake `git` executable placed before the trusted executable is not invoked during a merge. Focused unit coverage moves with the shared single-quote helper and covers executable paths containing spaces and single quotes. Existing assertions for the persisted merge-driver value compare against the computed command. Behavioral tests 1 through 6 run on Unix and Windows, including the emitted -Windows path form. Only the POSIX PATH-hijack fixture in test 7 skips Windows. +Windows path form. The merge-file error simulator and PATH-hijack fixture are +the only POSIX-only tests. diff --git a/git/AGENTS.md b/git/AGENTS.md index 32461d4..61896ff 100644 --- a/git/AGENTS.md +++ b/git/AGENTS.md @@ -34,10 +34,13 @@ specific application or forge workflow. same-user replacement races, or resource exhaustion. - Replacement merge drivers for untrusted merge-request imports must run the resolved Git executable without looking it up through the worktree `PATH`. - They must write clean text merges and diff3 markers for text conflicts, while - treating binary rejection as an ordinary per-file conflict. A missing Git - executable or a merge-process crash must fail the whole operation. Keep the - Unix and Git for Windows behaviors explicit. + They must classify binary inputs without repository attributes or external + diff/textconv helpers, write clean text merges and diff3 markers for text + conflicts, and treat classified binary content as an ordinary per-file + conflict. Classifier failures, text-merge I/O failures, a missing Git + executable, or a merge-process crash must fail the whole operation. This + contract requires Git 2.42.0+ on non-Windows and Git for Windows + 2.53.0.windows.3+. Keep both platform behaviors explicit. - Reject isolation-sensitive command-scope configuration during import because worktree configuration cannot outrank it. Explicit command-scope overrides on later Git commands are caller policy, not a sandbox boundary Kit can diff --git a/git/managed/lifecycle_mr_test.go b/git/managed/lifecycle_mr_test.go index a880cab..eab9e5c 100644 --- a/git/managed/lifecycle_mr_test.go +++ b/git/managed/lifecycle_mr_test.go @@ -69,11 +69,13 @@ func worktreeOnlyConfig(t *testing.T, dir, key string) string { return strings.TrimSpace(string(out)) } -func expectedSafeMergeDriverCommand(t *testing.T) string { +func expectedSafeMergeDriverCommand(t *testing.T, worktree string) string { t.Helper() path, err := resolveMergeDriverGitPath() Require.NoError(t, err) - return safeMergeDriverCommand(path) + hooksPath := worktreeConfig(t, worktree, "core.hooksPath") + Require.NotEmpty(t, hooksPath) + return safeMergeDriverCommand(path, hooksPath) } // TestCreateWorktreeFromMergeRequestSameRepo covers the same-repo scenario: @@ -526,7 +528,7 @@ func TestCreateWorktreeFromMergeRequestIsolatesUntrustedTreeGitPrograms(t *testi worktreeConfig(t, dest, "diff.owned.command")) assert.Equal(safeTextconvCommand, worktreeConfig(t, dest, "diff.owned.textconv")) - assert.Equal(expectedSafeMergeDriverCommand(t), + assert.Equal(expectedSafeMergeDriverCommand(t, dest), worktreeConfig(t, dest, "merge.owned.driver")) if err := os.Remove(fsmonitorMarker); err != nil { @@ -676,7 +678,7 @@ func TestCreateWorktreeFromMergeRequestNeutralizesCaseDistinctAttributeDrivers( worktreeOnlyConfig(t, dest, "filter."+driver+".required")) assert.Equal(safeExternalDiffCommand, worktreeOnlyConfig(t, dest, "diff."+driver+".command")) - assert.Equal(expectedSafeMergeDriverCommand(t), + assert.Equal(expectedSafeMergeDriverCommand(t, dest), worktreeOnlyConfig(t, dest, "merge."+driver+".driver")) } } @@ -910,7 +912,7 @@ func TestCreateWorktreeFromMergeRequestInspectsSelectedConfigFiles(t *testing.T) worktreeOnlyConfig(t, dest, "filter.selected.required")) assert.Equal(safeExternalDiffCommand, worktreeOnlyConfig(t, dest, "diff.selected.command")) - assert.Equal(expectedSafeMergeDriverCommand(t), + assert.Equal(expectedSafeMergeDriverCommand(t, dest), worktreeOnlyConfig(t, dest, "merge.selected.driver")) } diff --git a/git/managed/untrusted_tree.go b/git/managed/untrusted_tree.go index 97f716e..87dc8b0 100644 --- a/git/managed/untrusted_tree.go +++ b/git/managed/untrusted_tree.go @@ -54,14 +54,35 @@ func resolveMergeDriverGitPath() (string, error) { return path, nil } -func safeMergeDriverCommand(gitPath string) string { +func safeMergeDriverCommand(gitPath, classifierDir string) string { // Git expands merge-driver placeholders before the shell parses the // command. Double literal percent signs before applying shell quoting. git := shellquote.Single(strings.ReplaceAll(gitPath, "%", "%%")) - return `f() { [ -x ` + git + ` ] || return 129; ` + git + - ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + - `status=$?; if [ "$status" -eq 255 ]; then return 1; fi; ` + - `return "$status"; }; f %L "%A" "%O" "%B"` + classifier := shellquote.Single(strings.ReplaceAll(classifierDir, "%", "%%")) + ceiling := filepath.Dir(classifierDir) + if runtime.GOOS == "windows" { + classifier = shellquote.Single(strings.ReplaceAll( + filepath.ToSlash(classifierDir), "%", "%%", + )) + ceiling = filepath.ToSlash(ceiling) + } + ceiling = shellquote.Single(strings.ReplaceAll(ceiling, "%", "%%")) + return `f() { [ -x ` + git + ` ] || return 129; ` + + `case "$2" in /*|[A-Za-z]:/*) current=$2 ;; *) current=$PWD/$2 ;; esac; ` + + `case "$3" in /*|[A-Za-z]:/*) base=$3 ;; *) base=$PWD/$3 ;; esac; ` + + `case "$4" in /*|[A-Za-z]:/*) other=$4 ;; *) other=$PWD/$4 ;; esac; ` + + `classify() { output=$(GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES=` + ceiling + ` ` + git + + ` -c core.attributesFile= -C ` + classifier + + ` diff --no-index --numstat --no-ext-diff --no-textconv -- "$1" "$2"); ` + + `status=$?; [ "$status" -le 1 ] || return 129; ` + + `case "$output" in -*) return 1 ;; "") [ "$status" -eq 0 ] || return 129 ;; ` + + `[0-9]*) ;; *) return 129 ;; esac; return 0; }; ` + + `classify "$current" "$base"; status=$?; ` + + `[ "$status" -eq 1 ] && return 1; [ "$status" -eq 0 ] || return "$status"; ` + + `classify "$base" "$other"; status=$?; ` + + `[ "$status" -eq 1 ] && return 1; [ "$status" -eq 0 ] || return "$status"; ` + + git + ` merge-file --diff3 --marker-size="$1" -L current -L base -L other "$2" "$3" "$4"; ` + + `return $?; }; f %L "%A" "%O" "%B"` } var untrustedTreeGitVersionPattern = regexp.MustCompile( @@ -85,7 +106,7 @@ func validateUntrustedTreeCheckoutGitVersion( if supportsUntrustedTreeCheckoutGitVersion(string(out), runtime.GOOS) { return nil } - requirement := "Git 2.39.1 or newer" + requirement := "Git 2.42.0 or newer" if runtime.GOOS == "windows" || isGitForWindowsVersion(string(out)) { requirement = "Git for Windows 2.53.0.windows.3 or newer" } @@ -105,7 +126,7 @@ func supportsUntrustedTreeCheckoutGitVersion(output, goos string) bool { major, _ := strconv.Atoi(match[1]) minor, _ := strconv.Atoi(match[2]) patch, _ := strconv.Atoi(match[3]) - if major < 2 || major == 2 && (minor < 39 || minor == 39 && patch < 1) { + if major < 2 || major == 2 && minor < 42 { return false } if goos != "windows" && match[4] == "" { @@ -201,7 +222,7 @@ func prepareUntrustedTreeIsolation( return untrustedTreeIsolation{ runner: runner, config: config, - mergeDriverCommand: safeMergeDriverCommand(gitPath), + mergeDriverCommand: safeMergeDriverCommand(gitPath, hooksPath), }, nil } diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go index 0040690..1ea7435 100644 --- a/git/managed/untrusted_tree_merge_test.go +++ b/git/managed/untrusted_tree_merge_test.go @@ -10,6 +10,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "go.kenn.io/kit/git/internal/shellquote" ) type mergeDriverFixture struct { @@ -268,7 +270,8 @@ func TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears(t *testing.T) { require.NoError(os.WriteFile(gitCopy, gitContents, 0o755)) require.NoError(os.Chmod(gitCopy, 0o755)) lifecycleGit(t, fixture.worktree, "config", "--worktree", - "merge.owned.driver", safeMergeDriverCommand(gitCopy)) + "merge.owned.driver", safeMergeDriverCommand(gitCopy, + worktreeConfig(t, fixture.worktree, "core.hooksPath"))) require.NoError(os.Remove(gitCopy)) cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) @@ -295,9 +298,13 @@ func TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal(t *testing.T) { for _, worktree := range []string{fixture.worktree, otherWorktree} { require.NoError(os.WriteFile( filepath.Join(worktree, ".gitattributes"), - []byte("payload merge=owned\nbinary.dat merge=owned\n"), 0o644, + []byte("payload merge=owned -diff\nbinary.dat merge=owned diff=owned\n"), 0o644, )) } + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "diff.owned.command", ": > classifier-diff-marker") + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "diff.owned.textconv", ": > classifier-textconv-marker") currentBinary := []byte("\x00current\n") otherBinary := []byte("\x00other\n") require.NoError(os.WriteFile( @@ -330,6 +337,43 @@ func TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal(t *testing.T) { assert.Contains(string(payload), "<<<<<<< current") assert.Contains(string(payload), "||||||| base") assert.Contains(string(payload), ">>>>>>> other") - // Git merge-file names its temporary %A file in this expected diagnostic. - assert.Contains(string(out), "Cannot merge binary files: ") + assert.NotContains(string(out), "Cannot merge binary files: ") + assert.NoFileExists(filepath.Join(fixture.worktree, "classifier-diff-marker")) + assert.NoFileExists(filepath.Join(fixture.worktree, "classifier-textconv-marker")) +} + +func TestUntrustedTreeMergeDriverTreatsMergeFile255AsOperationError(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the merge-file error simulator is a POSIX shell script") + } + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + + gitPath, err := exec.LookPath("git") + require.NoError(err) + gitPath, err = filepath.Abs(gitPath) + require.NoError(err) + simulator := filepath.Join(t.TempDir(), "git") + script := "#!/bin/sh\n" + + "if [ \"$1\" = merge-file ]; then echo simulated-output-failure >&2; exit 255; fi\n" + + "exec " + shellquote.Single(gitPath) + " \"$@\"\n" + require.NoError(os.WriteFile(simulator, []byte(script), 0o755)) + require.NoError(os.Chmod(simulator, 0o755)) + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "merge.owned.driver", safeMergeDriverCommand(simulator, + worktreeConfig(t, fixture.worktree, "core.hooksPath"))) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Contains(string(out), "simulated-output-failure") + assert.Empty(lifecycleGit(t, fixture.worktree, "status", "--short")) + assert.Empty(lifecycleGit(t, fixture.worktree, "ls-files", "--unmerged")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal("current\n", string(contents)) } diff --git a/git/managed/untrusted_tree_test.go b/git/managed/untrusted_tree_test.go index 296ca13..fe83aac 100644 --- a/git/managed/untrusted_tree_test.go +++ b/git/managed/untrusted_tree_test.go @@ -18,8 +18,9 @@ func TestSupportsUntrustedTreeCheckoutGitVersion(t *testing.T) { goos string want bool }{ - {output: "git version 2.39.0", goos: "linux"}, - {output: "git version 2.39.1", goos: "linux", want: true}, + {output: "git version 2.41.0", goos: "linux"}, + {output: "git version 2.41.9", goos: "linux"}, + {output: "git version 2.42.0", goos: "linux", want: true}, {output: "git version 2.45.2 (Apple Git-145)", goos: "darwin", want: true}, {output: "git version 2.52.2.windows.4", goos: "linux"}, {output: "git version 2.52.2.windows.4", goos: "windows"}, From 3cbbc447d92791cf32ea3153ddd8a7054e036513 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 30 Aug 2026 01:21:21 +0200 Subject: [PATCH 11/14] fix(git): isolate merge input classification 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 --- .../plans/2026-08-29-safe-merge-driver.md | 39 ++++-- .../2026-08-29-safe-merge-driver-design.md | 41 ++++--- git/AGENTS.md | 16 +-- git/managed/lifecycle_mr.go | 3 + git/managed/untrusted_tree.go | 7 +- git/managed/untrusted_tree_merge_test.go | 111 +++++++++++++----- 6 files changed, 149 insertions(+), 68 deletions(-) diff --git a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md index 0fb478c..84a446a 100644 --- a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md +++ b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md @@ -110,17 +110,20 @@ func newMergeDriverFixture( Configure the trusted clone with `merge.owned.driver=false` before import. The returned worktree must therefore succeed only if Kit replaces that configured driver. -### Step 2: Write the five cross-platform behavior tests +### Step 2: Write the merge-driver behavior tests -Add these tests without any Windows skip: +Keep these tests cross-platform except for the explicitly POSIX merge-file +error simulator: 1. `TestUntrustedTreeMergeDriverMergesNonOverlappingText` — merge edits to different lines; assert exit 0, both edits in `payload`, and no unmerged entries. 2. `TestUntrustedTreeMergeDriverWritesDiff3ConflictMarkers` — overlap one line; assert ordinary conflict, `UU payload`, and exact marker labels `<<<<<<< current`, `||||||| base`, `=======`, `>>>>>>> other` with all three bodies. 3. `TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels` — use a branch name and rebase commit subject containing `$()` and backticks that would create relative marker files; assert neither marker exists and conflict markers still use only the fixed labels. -4. `TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears` — copy the resolved executable to a temporary executable path, replace only the fixture worktree's `merge.owned.driver` value with the command constructed for that copy, remove it before merge, and assert Git aborts with a clean worktree rather than recording `UU` with current-only contents. +4. `TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears` — create and remove a portable placeholder executable, replace only the fixture worktree's `merge.owned.driver` value with the command constructed for that path, and assert Git aborts with a clean worktree rather than recording `UU` with current-only contents. 5. `TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal` — mark both `binary.dat` and `payload` with the same driver; use worktree diff attributes that falsely force binary to text and text to binary; conflict both in one merge; assert both are `UU`, binary bytes remain current without text markers or a `merge-file` binary diagnostic, and `payload` contains diff3 markers. -6. `TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath` — resolve a copied Git executable from a path containing literal `%Y`, merge a branch label containing `$()` and backticks, and assert no payload executes while Git records the ordinary fixed-label diff3 conflict. +6. `TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath` — persist a missing executable path containing literal `%Y`, merge a branch label containing `$()` and backticks, and assert no payload executes while the merge aborts with a clean index and current text intact. Do not copy a platform Git entrypoint. 7. `TestUntrustedTreeMergeDriverTreatsMergeFile255AsOperationError` — on POSIX, use a behavioral executable wrapper that delegates classification but returns 255 from `merge-file`; assert the merge aborts without recording an unmerged current-only file. +8. `TestUntrustedTreeMergeDriverIgnoresAmbientBigFileThreshold` — launch outer Git with a one-byte counted `core.bigFileThreshold`; assert plain text still receives the exact fixed-label diff3 result. +9. `TestUntrustedTreeMergeDriverClearsInheritedRepositoryBindings` — launch outer Git with explicit absolute `--git-dir` and `--work-tree`, match `.merge_file_*` in worktree attributes, and configure marker-producing diff/textconv helpers; assert text receives diff3 and neither helper runs. For the hostile-label test, use relative marker names valid under both Git for Windows' shell and POSIX shells; do not embed a platform-specific absolute path in a ref name. For the missing-executable test, use `git config --worktree merge.owned.driver ` after import. This tests the persisted-command failure contract without adding a production seam; ordinary production preparation still calls `exec.LookPath` exactly once. @@ -186,8 +189,10 @@ managed empty directory. Before changing the subprocess cwd, convert `%A`, `%O`, and `%B` to absolute paths. Compare current/base and base/other with: ```sh -GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES='' \ - '' -c core.attributesFile= -C '' \ +GIT_CONFIG_COUNT=0 GIT_ATTR_NOSYSTEM=1 \ + GIT_CEILING_DIRECTORIES='' '' \ + -c core.attributesFile= -c core.bigFileThreshold=1023m \ + -C '' \ diff --no-index --numstat --no-ext-diff --no-textconv -- "$left" "$right" ``` @@ -208,13 +213,18 @@ this builder; `shellquote.Single` continues to own only POSIX shell quoting. Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. The fixed labels add no version requirement beyond the non-Windows Git 2.42.0 and Git for Windows 2.53.0.windows.3 import floors. -The classifier must use the pinned executable, disable system/global/worktree -attributes, prevent repository discovery, and pass both `--no-ext-diff` and -`--no-textconv`, so it cannot invoke a tree-selected helper. Do not map status -255 from `merge-file`: it represents both binary rejection and input/output -failures. Preclassification owns the binary case; all text-merge statuses pass -unchanged. Git 2.42 is the minimum non-Windows version because earlier Git -releases turn every positive custom-driver status into an ordinary conflict. +The classifier must use the pinned executable. Before it runs, convert input +paths to absolute paths and unset `GIT_DIR`, `GIT_WORK_TREE`, `GIT_INDEX_FILE`, +`GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, `GIT_COMMON_DIR`, +`GIT_NAMESPACE`, and `GIT_PREFIX`. Set `GIT_CONFIG_COUNT=0`, disable +system/global/worktree attributes, prevent repository discovery, pin +`core.bigFileThreshold=1023m` to merge-file's Git v2.53 `MAX_XDIFF_SIZE`, and +pass both `--no-ext-diff` and `--no-textconv`, so ambient state cannot change +classification or invoke a tree-selected helper. Do not map status 255 from +`merge-file`: it represents both binary rejection and input/output failures. +Preclassification owns the binary case; all text-merge statuses pass unchanged. +Git 2.42 is the minimum non-Windows version because earlier Git releases turn +every positive custom-driver status into an ordinary conflict. ### Step 3: Separate classification from construction @@ -289,6 +299,9 @@ In the untrusted merge-request guidance, state that replacement merge drivers mu - require Git 2.42.0+ on non-Windows and Git for Windows 2.53.0.windows.3+; - classify binary inputs without repository attributes or external helpers; +- clear inherited repository bindings and counted configuration before + classification; +- pin the classifier's large-file boundary to `1023m`; - write clean text merges and diff3 markers for text conflicts; - keep binary rejection as an ordinary per-file conflict; - surface classifier failures, text-merge I/O failures, missing executables, diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md index 9eebcf6..8f3c5e7 100644 --- a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md +++ b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md @@ -62,9 +62,13 @@ the function has these semantics: f() { [ -x '' ] || return 129 # Convert the three merge inputs to absolute paths before changing Git's cwd. + unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ + GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR GIT_NAMESPACE GIT_PREFIX # Classify current/base and base/other with the pinned Git executable: - # GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES='' \ - # '' -c core.attributesFile= -C '' \ + # GIT_CONFIG_COUNT=0 GIT_ATTR_NOSYSTEM=1 \ + # GIT_CEILING_DIRECTORIES='' '' \ + # -c core.attributesFile= -c core.bigFileThreshold=1023m \ + # -C '' \ # diff --no-index --numstat --no-ext-diff --no-textconv -- "$left" "$right" # Return 1 before merge-file for binary content and 129 on classifier errors. '' merge-file --diff3 --marker-size="$1" \ @@ -88,15 +92,20 @@ Git for Windows 2.53.0.windows.3 minimums, with no version-dependent fallback. Before invoking `merge-file`, the wrapper asks the pinned Git executable to classify the inputs with `diff --no-index --numstat`. It converts the temporary -input names to absolute paths, runs Git from the managed empty hooks directory, +input names to absolute paths, then clears inherited repository bindings before +running the nested Git command. `GIT_CONFIG_COUNT=0` discards counted ambient +configuration. The nested command runs from the managed empty hooks directory and sets the discovery ceiling to that directory's parent. System and global attribute files are disabled, and repository discovery is prevented, so -worktree attributes cannot falsely force content to text or binary. The -`--no-ext-diff` and `--no-textconv` flags prevent configured diff and textconv -helpers from running. A binary numstat result returns status 1 before -`merge-file`; a classifier command error or malformed result returns status -129. The wrapper compares current to base and base to other so an unchanged -pair cannot hide binary content in the remaining side. +worktree attributes cannot falsely force content to text or binary. The pinned +`core.bigFileThreshold=1023m` matches `merge-file`'s `MAX_XDIFF_SIZE` boundary, +which Git v2.53 defines as `1024*1024*1023`; a lower ambient threshold therefore +cannot make ordinary text look binary. The `--no-ext-diff` and `--no-textconv` +flags prevent configured diff and textconv helpers from running. A binary +numstat result returns status 1 before `merge-file`; a classifier command error +or malformed result returns status 129. The wrapper compares current to base +and base to other so an unchanged pair cannot hide binary content in the +remaining side. `git merge-file` overwrites `%A`, exits with status 0 for a clean merge, and returns the conflict count, capped at 127, when text conflicts remain. The @@ -145,8 +154,8 @@ worktree fixtures and exercise the persisted replacement after import: 2. Overlapping edits leave an unmerged path whose working file contains diff3 markers with the fixed labels and the base, current, and other content. 3. Adversarial branch and commit labels remain inert during a conflicted merge. -4. A resolved Git path containing literal `%Y` cannot expand an untrusted - branch label before shell quoting or execute its payload. +4. A missing Git path containing literal `%Y` cannot expand an untrusted branch + label before shell quoting or execute its payload; the merge aborts cleanly. 5. A missing persisted executable aborts the merge and leaves a clean tree. 6. Binary content produces an ordinary per-file conflict alongside a text conflict, even when worktree attributes try to force the opposite content @@ -156,10 +165,14 @@ worktree fixtures and exercise the persisted replacement after import: 8. Git 2.41 is rejected and Git 2.42 is accepted on non-Windows platforms. 9. The existing PATH-hijack fixture is extended so a fake `git` executable placed before the trusted executable is not invoked during a merge. +10. A low ambient `core.bigFileThreshold` cannot turn plain text into a + current-only binary conflict. +11. Explicit absolute `--git-dir` and `--work-tree` bindings cannot expose + `.merge_file_*` paths to worktree attributes or configured helpers. Focused unit coverage moves with the shared single-quote helper and covers executable paths containing spaces and single quotes. Existing assertions for the persisted merge-driver value compare against the computed command. -Behavioral tests 1 through 6 run on Unix and Windows, including the emitted -Windows path form. The merge-file error simulator and PATH-hijack fixture are -the only POSIX-only tests. +Behavioral tests 1 through 6 and 10 through 11 run on Unix and Windows, +including the emitted Windows path form. The merge-file error simulator and +PATH-hijack fixture are the only POSIX-only tests. diff --git a/git/AGENTS.md b/git/AGENTS.md index 61896ff..e8f4e6d 100644 --- a/git/AGENTS.md +++ b/git/AGENTS.md @@ -34,13 +34,15 @@ specific application or forge workflow. same-user replacement races, or resource exhaustion. - Replacement merge drivers for untrusted merge-request imports must run the resolved Git executable without looking it up through the worktree `PATH`. - They must classify binary inputs without repository attributes or external - diff/textconv helpers, write clean text merges and diff3 markers for text - conflicts, and treat classified binary content as an ordinary per-file - conflict. Classifier failures, text-merge I/O failures, a missing Git - executable, or a merge-process crash must fail the whole operation. This - contract requires Git 2.42.0+ on non-Windows and Git for Windows - 2.53.0.windows.3+. Keep both platform behaviors explicit. + They must clear inherited repository bindings and counted configuration, + classify binary inputs without repository attributes or external + diff/textconv helpers, and pin `core.bigFileThreshold=1023m` to match + `merge-file`'s maximum text size. They must write clean text merges and diff3 + markers for text conflicts, and treat classified binary content as an + ordinary per-file conflict. Classifier failures, text-merge I/O failures, a + missing Git executable, or a merge-process crash must fail the whole + operation. This contract requires Git 2.42.0+ on non-Windows and Git for + Windows 2.53.0.windows.3+. Keep both platform behaviors explicit. - Reject isolation-sensitive command-scope configuration during import because worktree configuration cannot outrank it. Explicit command-scope overrides on later Git commands are caller policy, not a sandbox boundary Kit can diff --git a/git/managed/lifecycle_mr.go b/git/managed/lifecycle_mr.go index dc25a41..71ac74d 100644 --- a/git/managed/lifecycle_mr.go +++ b/git/managed/lifecycle_mr.go @@ -110,6 +110,9 @@ type mergeRequestRemoteTarget struct { // submodule recursion. The existing repository, its configuration, provider // metadata, remotes, and explicitly configured setup hook remain trusted. // +// Untrusted-tree isolation requires Git 2.42.0 or newer on non-Windows +// platforms and Git for Windows 2.53.0.windows.3 or newer. +// // The function also configures upstream tracking when possible, non-fatally // skipping it when the fork cannot be fetched. Failures after the worktree // exists roll it back. diff --git a/git/managed/untrusted_tree.go b/git/managed/untrusted_tree.go index 87dc8b0..902a66b 100644 --- a/git/managed/untrusted_tree.go +++ b/git/managed/untrusted_tree.go @@ -71,8 +71,11 @@ func safeMergeDriverCommand(gitPath, classifierDir string) string { `case "$2" in /*|[A-Za-z]:/*) current=$2 ;; *) current=$PWD/$2 ;; esac; ` + `case "$3" in /*|[A-Za-z]:/*) base=$3 ;; *) base=$PWD/$3 ;; esac; ` + `case "$4" in /*|[A-Za-z]:/*) other=$4 ;; *) other=$PWD/$4 ;; esac; ` + - `classify() { output=$(GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES=` + ceiling + ` ` + git + - ` -c core.attributesFile= -C ` + classifier + + `unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY ` + + `GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR GIT_NAMESPACE GIT_PREFIX || return 129; ` + + `classify() { output=$(GIT_CONFIG_COUNT=0 GIT_ATTR_NOSYSTEM=1 GIT_CEILING_DIRECTORIES=` + + ceiling + ` ` + git + + ` -c core.attributesFile= -c core.bigFileThreshold=1023m -C ` + classifier + ` diff --no-index --numstat --no-ext-diff --no-textconv -- "$1" "$2"); ` + `status=$?; [ "$status" -le 1 ] || return 129; ` + `case "$output" in -*) return 1 ;; "") [ "$status" -eq 0 ] || return 129 ;; ` + diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go index 1ea7435..59e08df 100644 --- a/git/managed/untrusted_tree_merge_test.go +++ b/git/managed/untrusted_tree_merge_test.go @@ -209,21 +209,6 @@ func TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath(t *testing.T) { require := require.New(t) assert := assert.New(t) - gitPath, err := exec.LookPath("git") - require.NoError(err) - gitContents, err := os.ReadFile(gitPath) - require.NoError(err) - gitDir := filepath.Join(t.TempDir(), "git-%Y-bin") - require.NoError(os.MkdirAll(gitDir, 0o755)) - copyName := "git" - if runtime.GOOS == "windows" { - copyName += filepath.Ext(gitPath) - } - gitCopy := filepath.Join(gitDir, copyName) - require.NoError(os.WriteFile(gitCopy, gitContents, 0o755)) - require.NoError(os.Chmod(gitCopy, 0o755)) - t.Setenv("PATH", gitDir+string(os.PathListSeparator)+os.Getenv("PATH")) - markers := []string{"percent-dollar-marker", "percent-backtick-marker"} fixture := newMergeDriverFixture(t, []byte("base\n"), @@ -233,22 +218,23 @@ func TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath(t *testing.T) { "other-$(touch${IFS}percent-dollar-marker)-"+ "`touch${IFS}percent-backtick-marker`", ) + missingGit := filepath.Join(t.TempDir(), "missing-%Y-git") + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "merge.owned.driver", safeMergeDriverCommand(missingGit, + worktreeConfig(t, fixture.worktree, "core.hooksPath"))) cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) out, err := cmd.CombinedOutput() require.Error(err, string(out)) - assert.Equal("UU payload", - lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + assert.Empty(lifecycleGit(t, fixture.worktree, "status", "--short")) + assert.Empty(lifecycleGit(t, fixture.worktree, "ls-files", "--unmerged")) for _, marker := range markers { assert.NoFileExists(filepath.Join(fixture.worktree, marker)) } payload, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) require.NoError(err) - assert.Equal( - "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", - string(payload), - ) + assert.Equal("current\n", string(payload)) } func TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears(t *testing.T) { @@ -258,17 +244,8 @@ func TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears(t *testing.T) { []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", ) - gitPath, err := exec.LookPath("git") - require.NoError(err) - gitContents, err := os.ReadFile(gitPath) - require.NoError(err) - copyName := "git" - if runtime.GOOS == "windows" { - copyName += filepath.Ext(gitPath) - } - gitCopy := filepath.Join(t.TempDir(), copyName) - require.NoError(os.WriteFile(gitCopy, gitContents, 0o755)) - require.NoError(os.Chmod(gitCopy, 0o755)) + gitCopy := filepath.Join(t.TempDir(), "git") + require.NoError(os.WriteFile(gitCopy, nil, 0o755)) lifecycleGit(t, fixture.worktree, "config", "--worktree", "merge.owned.driver", safeMergeDriverCommand(gitCopy, worktreeConfig(t, fixture.worktree, "core.hooksPath"))) @@ -342,6 +319,76 @@ func TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal(t *testing.T) { assert.NoFileExists(filepath.Join(fixture.worktree, "classifier-textconv-marker")) } +func TestUntrustedTreeMergeDriverIgnoresAmbientBigFileThreshold(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + cmd.Env = append(cmd.Env, + "GIT_CONFIG_COUNT=1", + "GIT_CONFIG_KEY_0=core.bigFileThreshold", + "GIT_CONFIG_VALUE_0=1", + ) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal( + "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", + string(contents), + ) +} + +func TestUntrustedTreeMergeDriverClearsInheritedRepositoryBindings(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + + require.NoError(os.WriteFile( + filepath.Join(fixture.worktree, ".gitattributes"), + []byte("payload merge=owned\n.merge_file_* diff=owned\n"), 0o644, + )) + lifecycleGit(t, fixture.worktree, "add", ".gitattributes") + lifecycleGit(t, fixture.worktree, "commit", "-qm", "bind merge temp attributes") + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "diff.owned.binary", "true") + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "diff.owned.command", ": > classifier-binding-diff-marker") + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "diff.owned.textconv", ": > classifier-binding-textconv-marker") + gitDir := lifecycleGit(t, fixture.worktree, "rev-parse", "--absolute-git-dir") + outside := t.TempDir() + + cmd := lifecycleGitCommand(t, outside, + "--git-dir="+gitDir, + "--work-tree="+fixture.worktree, + "merge", fixture.otherRef, + ) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal( + "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", + string(contents), + ) + for _, dir := range []string{fixture.worktree, outside} { + assert.NoFileExists(filepath.Join(dir, "classifier-binding-diff-marker")) + assert.NoFileExists(filepath.Join(dir, "classifier-binding-textconv-marker")) + } +} + func TestUntrustedTreeMergeDriverTreatsMergeFile255AsOperationError(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("the merge-file error simulator is a POSIX shell script") From ae50c2865514ce21fbccb690451366ac898cfc2d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 30 Aug 2026 02:45:56 +0200 Subject: [PATCH 12/14] test(git): pin merge classifier text limit 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 --- git/managed/untrusted_tree_merge_test.go | 29 ++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go index 59e08df..5696a19 100644 --- a/git/managed/untrusted_tree_merge_test.go +++ b/git/managed/untrusted_tree_merge_test.go @@ -345,6 +345,35 @@ func TestUntrustedTreeMergeDriverIgnoresAmbientBigFileThreshold(t *testing.T) { ) } +func TestUntrustedTreeMergeDriverIgnoresGlobalBigFileThreshold(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + globalConfig := filepath.Join(t.TempDir(), "global.gitconfig") + require.NoError(os.WriteFile(globalConfig, []byte( + "[core]\n\tbigFileThreshold = 1\n", + ), 0o600)) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + cmd.Env = append(isolatedLifecycleBaseEnv(t), + "GIT_CONFIG_GLOBAL="+globalConfig, + "GIT_CONFIG_NOSYSTEM=1", + ) + out, err := cmd.CombinedOutput() + + require.Error(err, string(out)) + assert.Equal("UU payload", + lifecycleGit(t, fixture.worktree, "status", "--short", "--", "payload")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal( + "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", + string(contents), + ) +} + func TestUntrustedTreeMergeDriverClearsInheritedRepositoryBindings(t *testing.T) { require := require.New(t) assert := assert.New(t) From 449196e507c4c91ce516117f23ea4b3eeeaee02d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 30 Aug 2026 23:35:44 +0200 Subject: [PATCH 13/14] test(git): cover merge process crashes 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 --- git/managed/lifecycle.go | 7 ++++ git/managed/untrusted_tree_merge_test.go | 44 +++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/git/managed/lifecycle.go b/git/managed/lifecycle.go index 0cea5f1..a959908 100644 --- a/git/managed/lifecycle.go +++ b/git/managed/lifecycle.go @@ -56,6 +56,13 @@ type HookError struct { } // GitRunner runs one Git command under an application's process policy. +// +// It governs how Kit's own Git commands are executed, not which Git +// installation Kit targets. Merge-request import pins the git found on the +// process PATH into the replacement merge driver, because Git runs that driver +// itself and cannot route it back through this callback. A runner that +// executes some other Git therefore does not redirect the merge driver, and +// import fails outright when no git is on PATH. type GitRunner func( ctx context.Context, runner gitcmd.Runner, dir string, args ...string, ) ([]byte, error) diff --git a/git/managed/untrusted_tree_merge_test.go b/git/managed/untrusted_tree_merge_test.go index 5696a19..ce66e84 100644 --- a/git/managed/untrusted_tree_merge_test.go +++ b/git/managed/untrusted_tree_merge_test.go @@ -395,6 +395,8 @@ func TestUntrustedTreeMergeDriverClearsInheritedRepositoryBindings(t *testing.T) "diff.owned.textconv", ": > classifier-binding-textconv-marker") gitDir := lifecycleGit(t, fixture.worktree, "rev-parse", "--absolute-git-dir") outside := t.TempDir() + classifier := worktreeConfig(t, fixture.worktree, "core.hooksPath") + require.NotEmpty(classifier) cmd := lifecycleGitCommand(t, outside, "--git-dir="+gitDir, @@ -412,7 +414,10 @@ func TestUntrustedTreeMergeDriverClearsInheritedRepositoryBindings(t *testing.T) "<<<<<<< current\ncurrent\n||||||| base\nbase\n=======\nother\n>>>>>>> other\n", string(contents), ) - for _, dir := range []string{fixture.worktree, outside} { + // The classifier runs Git with -C in the hooks directory, so a helper + // invoked there would leave its marker in that directory rather than in + // the worktree or the caller's directory. + for _, dir := range []string{fixture.worktree, outside, classifier} { assert.NoFileExists(filepath.Join(dir, "classifier-binding-diff-marker")) assert.NoFileExists(filepath.Join(dir, "classifier-binding-textconv-marker")) } @@ -453,3 +458,40 @@ func TestUntrustedTreeMergeDriverTreatsMergeFile255AsOperationError(t *testing.T require.NoError(err) assert.Equal("current\n", string(contents)) } + +func TestUntrustedTreeMergeDriverTreatsMergeFileCrashAsOperationError(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the merge-file crash simulator is a POSIX shell script") + } + require := require.New(t) + assert := assert.New(t) + fixture := newMergeDriverFixture(t, + []byte("base\n"), []byte("current\n"), []byte("other\n"), "", "", + ) + + gitPath, err := exec.LookPath("git") + require.NoError(err) + gitPath, err = filepath.Abs(gitPath) + require.NoError(err) + simulator := filepath.Join(t.TempDir(), "git") + script := "#!/bin/sh\n" + + "if [ \"$1\" = merge-file ]; then kill -KILL $$; fi\n" + + "exec " + shellquote.Single(gitPath) + " \"$@\"\n" + require.NoError(os.WriteFile(simulator, []byte(script), 0o755)) + require.NoError(os.Chmod(simulator, 0o755)) + lifecycleGit(t, fixture.worktree, "config", "--worktree", + "merge.owned.driver", safeMergeDriverCommand(simulator, + worktreeConfig(t, fixture.worktree, "core.hooksPath"))) + + cmd := lifecycleGitCommand(t, fixture.worktree, "merge", fixture.otherRef) + out, err := cmd.CombinedOutput() + + // A signal-killed merge process must abort the whole operation rather + // than record a conflict whose working file holds only the current side. + require.Error(err, string(out)) + assert.Empty(lifecycleGit(t, fixture.worktree, "status", "--short")) + assert.Empty(lifecycleGit(t, fixture.worktree, "ls-files", "--unmerged")) + contents, err := os.ReadFile(filepath.Join(fixture.worktree, "payload")) + require.NoError(err) + assert.Equal("current\n", string(contents)) +} From baf69924c6cb25d6235e59fe98e6c46780cd387d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 31 Aug 2026 15:52:35 -0500 Subject: [PATCH 14/14] docs(git): drop merge-driver design working notes 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 --- .../plans/2026-08-29-safe-merge-driver.md | 362 ------------------ .../2026-08-29-safe-merge-driver-design.md | 178 --------- 2 files changed, 540 deletions(-) delete mode 100644 docs/superpowers/plans/2026-08-29-safe-merge-driver.md delete mode 100644 docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md diff --git a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md b/docs/superpowers/plans/2026-08-29-safe-merge-driver.md deleted file mode 100644 index 84a446a..0000000 --- a/docs/superpowers/plans/2026-08-29-safe-merge-driver.md +++ /dev/null @@ -1,362 +0,0 @@ -# Safe Merge Driver Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use `superpowers:executing-plans` to implement this plan task-by-task. - -**Goal:** Replace untrusted-tree custom merge drivers with a durable driver that performs ordinary three-way text merges, writes diff3 conflict markers, keeps binary conflicts local to the file, and fails the whole Git operation when the pinned Git executable cannot run. - -**Architecture:** Resolve the same `git` executable used by `git/cmd.Runner` before materializing the untrusted tree, escape Git-template percent signs, persist an absolute shell-quoted path in worktree config, classify binary inputs without repository attributes or external helpers, and invoke `git merge-file` only for text. Keep attribute-driver key classification independent from command construction so command-scope checks do not require path resolution. Move the existing POSIX single-quote helper into `git/internal/shellquote` so both owning packages share the shell-escaping rule; keep percent escaping local to merge-driver construction. - -**Tech Stack:** Go 1.27, Git 2.42.0+ on non-Windows platforms, Git for Windows 2.53.0.windows.3+, `testify`, standard `os/exec`, repository lifecycle fixtures. Git 2.42 is required so custom merge-driver operation-error statuses are not collapsed into conflicts; fixed merge labels add no further floor. - ---- - -## Task 1: Share the existing shell-quoting helper - -**Files:** -- Create: `git/internal/shellquote/shellquote.go` -- Create: `git/internal/shellquote/shellquote_test.go` -- Modify: `git/cmd/gitcmd.go:18-28,187-198` -- Modify: `git/cmd/gitcmd_test.go:569` - -### Step 1: Write the failing helper test - -Create a table test for the exact POSIX single-quote contract: - -```go -package shellquote - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestSingle(t *testing.T) { - t.Parallel() - for _, test := range []struct { - name, input, want string - }{ - {name: "empty", want: "''"}, - {name: "spaces", input: "/opt/Git Tools/git", want: "'/opt/Git Tools/git'"}, - {name: "single quote", input: "/opt/Git's/git", want: "'/opt/Git'\\''s/git'"}, - } { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - assert.Equal(t, test.want, Single(test.input)) - }) - } -} -``` - -Run: `go test ./git/internal/shellquote` - -Expected: FAIL because `Single` does not exist. - -### Step 2: Implement and adopt the helper - -```go -// Package shellquote quotes arguments embedded in Git shell command strings. -package shellquote - -import "strings" - -// Single returns value enclosed in POSIX shell single quotes. -func Single(value string) string { - return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" -} -``` - -Import it in `git/cmd/gitcmd.go`, replace `shellSingleQuote(path)` with `shellquote.Single(path)`, and delete the local helper. Make the same replacement in `gitcmd_test.go`. Retain the existing `strings` import because the package has other users. - -### Step 3: Verify and commit the refactor - -Run: - -```sh -gofmt -w git/internal/shellquote/shellquote.go git/internal/shellquote/shellquote_test.go git/cmd/gitcmd.go git/cmd/gitcmd_test.go -go test ./git/internal/shellquote ./git/cmd -``` - -Commit: - -```sh -git add git/internal/shellquote git/cmd/gitcmd.go git/cmd/gitcmd_test.go -git commit -m "refactor(git): share shell quoting helper" -``` - -## Task 2: Add end-to-end merge-driver regression tests - -**Files:** -- Create: `git/managed/untrusted_tree_merge_test.go` -- Modify: `git/managed/lifecycle_mr_test.go:440-725,619-675,803-872` - -### Step 1: Add a reusable imported-worktree fixture - -Build real repositories with `initOriginAndClone`, commit `.gitattributes` containing `payload merge=owned`, create `current` and `other` commits from a shared base, fetch `other` into the clone, and call `CreateWorktreeFromMergeRequest` for `current`. Return the imported worktree path and the other ref. Keep all paths under `t.TempDir`; do not read or alter user Git configuration. - -The helper should accept base/current/other bytes and optional branch/subject strings so every test exercises the persisted worktree driver through ordinary `git merge` or `git rebase`, rather than invoking the wrapper directly: - -```go -type mergeDriverFixture struct { - worktree string - otherRef string -} - -func newMergeDriverFixture( - t *testing.T, base, current, other []byte, - currentSubject, otherBranch string, -) mergeDriverFixture -``` - -Configure the trusted clone with `merge.owned.driver=false` before import. The returned worktree must therefore succeed only if Kit replaces that configured driver. - -### Step 2: Write the merge-driver behavior tests - -Keep these tests cross-platform except for the explicitly POSIX merge-file -error simulator: - -1. `TestUntrustedTreeMergeDriverMergesNonOverlappingText` — merge edits to different lines; assert exit 0, both edits in `payload`, and no unmerged entries. -2. `TestUntrustedTreeMergeDriverWritesDiff3ConflictMarkers` — overlap one line; assert ordinary conflict, `UU payload`, and exact marker labels `<<<<<<< current`, `||||||| base`, `=======`, `>>>>>>> other` with all three bodies. -3. `TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabels` — use a branch name and rebase commit subject containing `$()` and backticks that would create relative marker files; assert neither marker exists and conflict markers still use only the fixed labels. -4. `TestUntrustedTreeMergeDriverFailsWhenResolvedGitDisappears` — create and remove a portable placeholder executable, replace only the fixture worktree's `merge.owned.driver` value with the command constructed for that path, and assert Git aborts with a clean worktree rather than recording `UU` with current-only contents. -5. `TestUntrustedTreeMergeDriverKeepsBinaryConflictLocal` — mark both `binary.dat` and `payload` with the same driver; use worktree diff attributes that falsely force binary to text and text to binary; conflict both in one merge; assert both are `UU`, binary bytes remain current without text markers or a `merge-file` binary diagnostic, and `payload` contains diff3 markers. -6. `TestUntrustedTreeMergeDriverEscapesPlaceholdersInGitPath` — persist a missing executable path containing literal `%Y`, merge a branch label containing `$()` and backticks, and assert no payload executes while the merge aborts with a clean index and current text intact. Do not copy a platform Git entrypoint. -7. `TestUntrustedTreeMergeDriverTreatsMergeFile255AsOperationError` — on POSIX, use a behavioral executable wrapper that delegates classification but returns 255 from `merge-file`; assert the merge aborts without recording an unmerged current-only file. -8. `TestUntrustedTreeMergeDriverIgnoresAmbientBigFileThreshold` — launch outer Git with a one-byte counted `core.bigFileThreshold`; assert plain text still receives the exact fixed-label diff3 result. -9. `TestUntrustedTreeMergeDriverClearsInheritedRepositoryBindings` — launch outer Git with explicit absolute `--git-dir` and `--work-tree`, match `.merge_file_*` in worktree attributes, and configure marker-producing diff/textconv helpers; assert text receives diff3 and neither helper runs. - -For the hostile-label test, use relative marker names valid under both Git for Windows' shell and POSIX shells; do not embed a platform-specific absolute path in a ref name. For the missing-executable test, use `git config --worktree merge.owned.driver ` after import. This tests the persisted-command failure contract without adding a production seam; ordinary production preparation still calls `exec.LookPath` exactly once. - -### Step 3: Extend the existing POSIX PATH-hijack test - -Keep the current `runtime.GOOS == "windows"` skip only on `TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers`. Add `merge=owned` to its attributes, configure `merge.owned.driver=false`, create a conflicting branch, run the later merge with the imported tree first in `PATH`, and preserve `assert.NoFileExists(marker)`. This verifies the wrapper invokes the absolute Git path instead of a tree-controlled `git` or `sh` helper. - -### Step 4: Run the new tests and confirm RED - -Run: - -```sh -go test ./git/managed -run 'Test(UntrustedTreeMergeDriver|CreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers)' -count=1 -``` - -Expected: FAIL. Non-overlapping text remains current-only, overlapping text lacks conflict markers, and the computed-command test seams do not yet exist. - -Do not commit these failing tests separately. - -## Task 3: Build and persist the safe merge driver - -**Files:** -- Modify: `git/managed/untrusted_tree.go:1-40,144-168,217-227,261-300,526-580` -- Modify: `git/managed/lifecycle_mr_test.go:522,672,870,1057` -- Test: `git/managed/untrusted_tree_merge_test.go` - -### Step 1: Resolve and normalize Git before materialization - -Add `os/exec` and `git/internal/shellquote` imports. Resolve with the same process `PATH` semantics as `gitcmd.Runner`: - -```go -func resolveMergeDriverGitPath() (string, error) { - path, err := exec.LookPath("git") - if err != nil { - return "", fmt.Errorf("resolve Git executable for safe merge driver: %w", err) - } - path, err = filepath.Abs(path) - if err != nil { - return "", fmt.Errorf("make Git executable path absolute: %w", err) - } - path = filepath.Clean(path) - if runtime.GOOS == "windows" { - path = filepath.ToSlash(path) - } - return path, nil -} -``` - -Call this from `prepareUntrustedTreeIsolation` after command-scope validation and before creating or materializing the worktree. Store the generated command on `untrustedTreeIsolation`: - -```go -type untrustedTreeIsolation struct { - runner gitcmd.Runner - config []gitcmd.Config - mergeDriverCommand string -} -``` - -### Step 2: Construct the exact wrapper - -Replace the constant with a builder that accepts the pinned executable and the -managed empty directory. Before changing the subprocess cwd, convert `%A`, -`%O`, and `%B` to absolute paths. Compare current/base and base/other with: - -```sh -GIT_CONFIG_COUNT=0 GIT_ATTR_NOSYSTEM=1 \ - GIT_CEILING_DIRECTORIES='' '' \ - -c core.attributesFile= -c core.bigFileThreshold=1023m \ - -C '' \ - diff --no-index --numstat --no-ext-diff --no-textconv -- "$left" "$right" -``` - -Treat a `-\t-` numstat result as binary and return status 1 before -`merge-file`. Treat a classifier command error, empty output for a differing -pair, or malformed output as operation error 129. For text, invoke: - -```sh -'' merge-file --diff3 --marker-size="$1" \ - -L current -L base -L other "$2" "$3" "$4" -``` - -Pass every `merge-file` status through unchanged. - -Git expands merge-driver placeholders before invoking the shell. Doubling -literal percent signs is therefore Git-template escaping and stays local to -this builder; `shellquote.Single` continues to own only POSIX shell quoting. - -Do not use `%S`, `%X`, or `%Y`: Git already shell-quotes those placeholders, and outer double quotes would turn hostile label content into executable shell syntax. Preserve the double quotes around `%A`, `%O`, and `%B`, which Git substitutes without shell quoting. The fixed labels add no version requirement beyond the non-Windows Git 2.42.0 and Git for Windows 2.53.0.windows.3 import floors. - -The classifier must use the pinned executable. Before it runs, convert input -paths to absolute paths and unset `GIT_DIR`, `GIT_WORK_TREE`, `GIT_INDEX_FILE`, -`GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, `GIT_COMMON_DIR`, -`GIT_NAMESPACE`, and `GIT_PREFIX`. Set `GIT_CONFIG_COUNT=0`, disable -system/global/worktree attributes, prevent repository discovery, pin -`core.bigFileThreshold=1023m` to merge-file's Git v2.53 `MAX_XDIFF_SIZE`, and -pass both `--no-ext-diff` and `--no-textconv`, so ambient state cannot change -classification or invoke a tree-selected helper. Do not map status 255 from -`merge-file`: it represents both binary rejection and input/output failures. -Preclassification owns the binary case; all text-merge statuses pass unchanged. -Git 2.42 is the minimum non-Windows version because earlier Git releases turn -every positive custom-driver status into an ordinary conflict. - -### Step 3: Separate classification from construction - -Extract attribute-driver discovery from config creation: - -```go -type attributeDrivers struct { - filters map[string]struct{} - diffs map[string]struct{} - merges map[string]struct{} -} - -func configuredAttributeDrivers(keys []string) attributeDrivers - -func (drivers attributeDrivers) configured() bool { - return len(drivers.filters)+len(drivers.diffs)+len(drivers.merges) != 0 -} - -func neutralizeAttributeDrivers( - drivers attributeDrivers, mergeDriverCommand string, -) []gitcmd.Config -``` - -Use `configuredAttributeDrivers([]string{key}).configured()` in `isolationSensitiveConfigKey`, where no path is available. In `completeUntrustedTreeIsolation`, discover once and pass `isolation.mergeDriverCommand` to config construction. - -### Step 4: Update computed-command assertions - -Add one test helper used by all three existing config assertions: - -```go -func expectedSafeMergeDriverCommand(t *testing.T, worktree string) string { - t.Helper() - path, err := resolveMergeDriverGitPath() - require.NoError(t, err) - hooksPath := worktreeConfig(t, worktree, "core.hooksPath") - require.NotEmpty(t, hooksPath) - return safeMergeDriverCommand(path, hooksPath) -} -``` - -Replace constant equality checks at the existing isolation, case-distinct-driver, and selected-config tests. Update the direct `neutralizeAttributeDrivers` classification test for the split API. - -### Step 5: Run focused tests and commit - -Run: - -```sh -gofmt -w git/managed/untrusted_tree.go git/managed/untrusted_tree_merge_test.go git/managed/lifecycle_mr_test.go -go test ./git/managed -run 'Test(UntrustedTreeMergeDriver|CreateWorktreeFromMergeRequest(IsolatesUntrustedTreeGitPrograms|NeutralizesCaseDistinctAttributeDrivers|DoesNotPATHSearchDriverHelpers|InspectsSelectedConfigFiles))' -count=1 -go test ./git/managed -count=1 -``` - -Expected: PASS. Inspect the focused test output to ensure binary rejection is represented as an ordinary conflict while missing executables and signal deaths are operation errors. - -Commit: - -```sh -git add git/managed/untrusted_tree.go git/managed/untrusted_tree_merge_test.go git/managed/lifecycle_mr_test.go -git commit -m "fix(git): preserve merge conflict contents" -``` - -## Task 4: Document the durable boundary and verify the repository - -**Files:** -- Modify: `git/AGENTS.md` - -### Step 1: Record the maintained invariant - -In the untrusted merge-request guidance, state that replacement merge drivers must: - -- invoke the resolved Git executable without a worktree `PATH` lookup; -- require Git 2.42.0+ on non-Windows and Git for Windows - 2.53.0.windows.3+; -- classify binary inputs without repository attributes or external helpers; -- clear inherited repository bindings and counted configuration before - classification; -- pin the classifier's large-file boundary to `1023m`; -- write clean text merges and diff3 markers for text conflicts; -- keep binary rejection as an ordinary per-file conflict; -- surface classifier failures, text-merge I/O failures, missing executables, - and process crashes as whole-operation errors; -- behave explicitly on Unix and Git for Windows. - -Do not document the wrapper string itself; document the behavior future implementations must preserve. - -### Step 2: Run fresh full verification - -Run: - -```sh -gofmt -w git/cmd/gitcmd.go git/cmd/gitcmd_test.go git/internal/shellquote/*.go git/managed/untrusted_tree.go git/managed/*_test.go -go test ./git/... -count=1 -go vet ./... -go test ./... -count=1 -make lint -git diff --check -git status --short -``` - -If `make lint` creates the ignored `custom-gcl` binary, leave it untracked/ignored and do not add it. Review `git diff origin/main...HEAD` for unrelated changes and private data before any push. - -### Step 3: Commit documentation if changed - -```sh -git add git/AGENTS.md -git commit -m "docs(git): define safe merge driver behavior" -``` - -Skip this commit if the existing text already expresses the complete invariant and no documentation edit is necessary. - -## Task 5: Push and open the Kit pull request - -### Step 1: Perform pre-push checks - -Follow `kenn:commit`, `kenn:scrub-private-data`, `superpowers:verification-before-completion`, and `kenn:commit-push-pr`. Confirm every accepted repository change is committed and the branch contains no credentials, private paths, private downstream names, or generated scratch files. - -### Step 2: Push the branch - -```sh -git push -u origin fix/merge-driver-conflict-markers -``` - -### Step 3: Open the PR - -Draft the body with `kenn:pr-desc`, describing the current result and reviewer-relevant tradeoffs. Do not include a routine `Validation` section and do not comment on the issue or poll CI. - -```sh -gh pr create \ - --base main \ - --head fix/merge-driver-conflict-markers \ - --title "fix(git): preserve merge conflict contents" \ - --body-file -``` - -The body should explain that imported worktrees now retain normal clean merges and diff3 text conflicts, binary conflicts remain per-file, and a moved/crashed pinned Git executable aborts instead of silently leaving current-only content. Link the originating public issue only if it is appropriate for the Kit repository's public context. diff --git a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md b/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md deleted file mode 100644 index 8f3c5e7..0000000 --- a/docs/superpowers/specs/2026-08-29-safe-merge-driver-design.md +++ /dev/null @@ -1,178 +0,0 @@ -# Safe merge-driver fallback design - -## Problem - -Merge-request imports treat the fetched tree as untrusted. Kit therefore -replaces every configured custom merge driver that the tree can select. The -current replacement exits with status 1 without writing a result to Git's -`%A` file. - -Git correctly records an unmerged index entry, but it trusts the custom driver -to populate `%A`. The working file consequently contains only the current -side, with no conflict markers or other-side content. A later `git add` can -hide the omitted changes. - -## Goals - -- Use Git's normal three-way text merge when a custom driver is disabled. -- Preserve diff3 conflict markers and all three inputs when overlap remains. -- Merge non-overlapping edits without reporting a conflict. -- Keep an imported tree from selecting a replacement executable through - `PATH`. -- Preserve Git's ordinary per-file conflict behavior for binary content. -- Preserve the existing filter, diff, hook, fsmonitor, submodule, and - worktree-configuration isolation behavior. - -## Non-goals - -- Distinguishing drivers selected by trusted global attributes from drivers - selected by repository-controlled attributes. -- Changing the public managed-worktree API. -- Replacing Git's built-in merge algorithm or interpreting file contents in - Go. - -## Design - -Before preparing persistent untrusted-tree isolation, Kit resolves the same -`git` executable that its subprocess runner uses. It converts the result to an -absolute, cleaned path and fails the import if no executable can be resolved. -On Windows, Kit emits the drive-qualified path with forward slashes before -shell quoting it. Git for Windows runs custom drivers through its POSIX shell, -and the forward-slash form works for both the shell's executable check and the -Windows executable loader. Existing untrusted-tree imports require Git 2.42.0 -or newer on non-Windows platforms and Git for Windows 2.53.0.windows.3 or -newer. Before Git 2.42, every positive custom merge-driver status is treated as -an ordinary conflict, so a driver cannot distinguish a per-file conflict from -an operation error. Fixed labels themselves add no new version floor. - -Kit moves the existing POSIX shell single-quote helper into a shared internal -Git package. Both the credential helper and managed-worktree isolation use it, -so executable paths remain shell data without adding a public API or -duplicating shell quoting. Merge-driver configuration has an earlier template -layer: Git expands percent placeholders before it invokes the shell. The merge -driver builder therefore doubles every literal `%` in embedded executable and -managed-directory paths before passing those paths to the POSIX shell-quote -helper. This percent escaping stays local to merge-driver construction because -it is a Git-template rule, not generic shell quoting. - -Kit builds a shell function around the trusted absolute path. In expanded form, -the function has these semantics: - -```sh -f() { - [ -x '' ] || return 129 - # Convert the three merge inputs to absolute paths before changing Git's cwd. - unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ - GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR GIT_NAMESPACE GIT_PREFIX - # Classify current/base and base/other with the pinned Git executable: - # GIT_CONFIG_COUNT=0 GIT_ATTR_NOSYSTEM=1 \ - # GIT_CEILING_DIRECTORIES='' '' \ - # -c core.attributesFile= -c core.bigFileThreshold=1023m \ - # -C '' \ - # diff --no-index --numstat --no-ext-diff --no-textconv -- "$left" "$right" - # Return 1 before merge-file for binary content and 129 on classifier errors. - '' merge-file --diff3 --marker-size="$1" \ - -L current -L base -L other "$2" "$3" "$4" - return $? -} -f %L "%A" "%O" "%B" -``` - -The displayed single quotes around `` represent the shared -helper's output, including its handling of an embedded single quote. - -Git inserts `%A`, `%O`, and `%B` without shell quoting, so the driver command -must keep the double quotes around those placeholders. Git 2.44 and newer -already insert `%S`, `%X`, and `%Y` as shell-single-quoted strings. Placing those -label placeholders inside another pair of double quotes would make command -substitutions in a branch name or commit subject executable. The replacement -therefore uses fixed labels and does not interpolate `%S`, `%X`, or `%Y` at all. -Fixed labels add no version requirement beyond the non-Windows Git 2.42.0 and -Git for Windows 2.53.0.windows.3 minimums, with no version-dependent fallback. - -Before invoking `merge-file`, the wrapper asks the pinned Git executable to -classify the inputs with `diff --no-index --numstat`. It converts the temporary -input names to absolute paths, then clears inherited repository bindings before -running the nested Git command. `GIT_CONFIG_COUNT=0` discards counted ambient -configuration. The nested command runs from the managed empty hooks directory -and sets the discovery ceiling to that directory's parent. System and global -attribute files are disabled, and repository discovery is prevented, so -worktree attributes cannot falsely force content to text or binary. The pinned -`core.bigFileThreshold=1023m` matches `merge-file`'s `MAX_XDIFF_SIZE` boundary, -which Git v2.53 defines as `1024*1024*1023`; a lower ambient threshold therefore -cannot make ordinary text look binary. The `--no-ext-diff` and `--no-textconv` -flags prevent configured diff and textconv helpers from running. A binary -numstat result returns status 1 before `merge-file`; a classifier command error -or malformed result returns status 129. The wrapper compares current to base -and base to other so an unchanged pair cannot hide binary content in the -remaining side. - -`git merge-file` overwrites `%A`, exits with status 0 for a clean merge, and -returns the conflict count, capped at 127, when text conflicts remain. The -wrapper passes those statuses through. Git therefore keeps its normal index -state while a text working file contains either a clean merged result or a -complete diff3 conflict. - -The resolved command becomes part of the existing untrusted-tree isolation -state. Attribute-driver discovery uses the prepared command when constructing -worktree-scoped `merge..driver` entries. The command-scope configuration -check continues to classify driver-shaped keys without constructing a merge -command, so that earlier call site does not need a resolved executable path. -No new public API or persistent file is added. - -## Error handling - -Failure to resolve an absolute Git executable stops the import before the -untrusted worktree is materialized. Before every later merge-driver invocation, -the shell function checks that the persisted path is still executable. A moved -or removed executable returns status 129 before `merge-file` starts. Git treats -that status as a driver failure and aborts the operation instead of recording -an ours-only conflict. - -Binary classification returns status 1 without invoking `merge-file`. Git -keeps the current bytes, marks that path conflicted, and continues processing -other paths. Binary files receive neither text markers nor `merge-file`'s -temporary-file diagnostic. - -Every `merge-file` status passes through unchanged. In particular, status 255 -is not assumed to mean binary: `merge-file` also uses it for input, output, -read, write, and close failures. With the Git 2.42 floor, that status aborts the -operation instead of becoming an ours-only conflict. A signal death such as -status 139 likewise remains an operation error, while statuses 1 through 127 -remain valid text conflict counts. The executable guard covers a stable -missing or non-executable path, but not a same-user replacement race between -the check and invocation. - -Existing import cleanup and rollback behavior remains unchanged. - -## Tests - -Behavioral tests create temporary repositories through the existing managed -worktree fixtures and exercise the persisted replacement after import: - -1. Two non-overlapping edits merge cleanly and produce the combined file. -2. Overlapping edits leave an unmerged path whose working file contains diff3 - markers with the fixed labels and the base, current, and other content. -3. Adversarial branch and commit labels remain inert during a conflicted merge. -4. A missing Git path containing literal `%Y` cannot expand an untrusted branch - label before shell quoting or execute its payload; the merge aborts cleanly. -5. A missing persisted executable aborts the merge and leaves a clean tree. -6. Binary content produces an ordinary per-file conflict alongside a text - conflict, even when worktree attributes try to force the opposite content - classifications. -7. A simulated non-binary `merge-file` status 255 aborts the operation without - leaving a current-only conflict. -8. Git 2.41 is rejected and Git 2.42 is accepted on non-Windows platforms. -9. The existing PATH-hijack fixture is extended so a fake `git` executable - placed before the trusted executable is not invoked during a merge. -10. A low ambient `core.bigFileThreshold` cannot turn plain text into a - current-only binary conflict. -11. Explicit absolute `--git-dir` and `--work-tree` bindings cannot expose - `.merge_file_*` paths to worktree attributes or configured helpers. - -Focused unit coverage moves with the shared single-quote helper and covers -executable paths containing spaces and single quotes. Existing assertions for -the persisted merge-driver value compare against the computed command. -Behavioral tests 1 through 6 and 10 through 11 run on Unix and Windows, -including the emitted Windows path form. The merge-file error simulator and -PATH-hijack fixture are the only POSIX-only tests.