Add pre-merge-commit hook to resolve file ordering race (#183)

* Install pre-merge-commit hook to regenerate sections post-merge

The merge driver's per-file `mdsmith fix` could read stale sibling
files because git merges paths one at a time: when the driver runs
on PLAN.md, plan/*.md may still hold "ours" content, so the
regenerated catalog reflects pre-merge sources. CI run 24971661273
hit this — PLAN.md row 94 stayed 🔲 while plan/94 status was ✅.

`mdsmith merge-driver install` now writes a pre-merge-commit hook
that re-runs `mdsmith fix` on the registered files once every
path has reached its final merged state. The merge-queue workflow
builds mdsmith from source so the install logic always matches
the directive schema of the branch being queued.

* Add e2e test reproducing the file-ordering race

Sets up two branches that each bump a different plan file's
status to ✅ and regenerate PLAN.md, then merges them after
running `mdsmith merge-driver install`. Asserts the merged
PLAN.md catalog matches the post-merge plan files and that
`mdsmith check .` reports no drift.

Verified by temporarily disabling the pre-merge-commit hook
install: the test then fails with the exact symptom from CI
run 24971661273 — a stale row in PLAN.md and MDS019
"generated section is out of date".

* Address review: revert workflow, fix hook robustness, add 100% coverage

- Revert merge-queue.yml to downloading the pinned v0.5.0 release binary
  instead of building from source (per jeduden's review request; the
  build-from-source approach will land in a follow-up PR alongside a new
  release that includes this hook fix)
- Handle core.hooksPath: resolveHooksDir() uses git rev-parse --git-path
  hooks so install respects repos that redirect hooks to a custom path
- Handle non-ENOENT read errors on the existing hook file: treat them as
  a safety failure rather than silently overwriting an unreadable hook
- Split per-file fix commands into separate lines so set -e reliably
  aborts the hook on a mdsmith fix or git add failure (the && form can
  suppress errexit in some POSIX shells)
- Add tests covering all new code paths: resolveHooksDir (fallback, default
  git repo, relative and absolute core.hooksPath), unreadable-hook error,
  MkdirAll failure, WriteFile failure, and e2e install with unmanaged hook

https://claude.ai/code/session_012XG9t645SL8z2RW9ik79gw

* Fix test robustness per Copilot review

- Add runtime.GOOS=="windows" skip to the three POSIX permission tests
  to match the pattern used elsewhere in the repo
- TestResolveHooksDir_DefaultGitRepo: derive expected path via
  git rev-parse --git-path hooks instead of hard-coding .git/hooks,
  so the test passes when a developer has core.hooksPath set globally
- TestE2E_MergeDriver_Install: use gitHooksDir() helper instead of
  hard-coded .git/hooks path for the hook assertion
- TestE2E_MergeDriver_Install_UnmanagedHook: use gitHooksDir() for
  hook setup so the pre-existing hook is placed where install looks

https://claude.ai/code/session_012XG9t645SL8z2RW9ik79gw

* Address Copilot review: fix --, Windows exec-bit guards, comment placement

- Add -- separator to mdsmith fix in generated hook so file paths
  starting with - are not misinterpreted as flags
- Guard info.Mode()&0o111 exec-bit assertions with runtime.GOOS != "windows"
  in both the unit test and e2e test (POSIX permission bits don't apply on
  Windows)
- Move the writeFixture doc comment back to sit directly above writeFixture;
  gitHooksDir now has only its own comment above it

https://claude.ai/code/session_012XG9t645SL8z2RW9ik79gw

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: jeduden

cmd/mdsmith/e2e_test.go

@@ -8,6 +8,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"regexp"
+	"runtime"
 	"strconv"
 	"strings"
 	"testing"
@@ -192,6 +193,19 @@ func isolateDir(t *testing.T, dir string) {
 	}
 }
 
+// gitHooksDir returns the effective hooks directory for the git repo at dir,
+// derived via git itself so it respects core.hooksPath.
+func gitHooksDir(t *testing.T, dir string) string {
+	t.Helper()
+	out, err := exec.Command("git", "-C", dir, "rev-parse", "--git-path", "hooks").Output()
+	require.NoError(t, err, "git rev-parse --git-path hooks")
+	p := strings.TrimSpace(string(out))
+	if !filepath.IsAbs(p) {
+		p = filepath.Join(dir, p)
+	}
+	return filepath.Clean(p)
+}
+
 // writeFixture creates a file with the given content in the given directory.
 func writeFixture(t *testing.T, dir, name, content string) string {
 	t.Helper()
@@ -1236,6 +1250,21 @@ func TestE2E_MergeDriver_Install(t *testing.T) {
 	content := string(attrs)
 	assert.Contains(t, content, "PLAN.md merge=mdsmith", "expected PLAN.md entry in .gitattributes")
 	assert.Contains(t, content, "README.md merge=mdsmith", "expected README.md entry in .gitattributes")
+
+	// Verify pre-merge-commit hook was installed and is executable.
+	// Use gitHooksDir to respect core.hooksPath if set globally.
+	hookPath := filepath.Join(gitHooksDir(t, dir), "pre-merge-commit")
+	info, err := os.Stat(hookPath)
+	require.NoError(t, err, "expected pre-merge-commit hook at %s", hookPath)
+	if runtime.GOOS != "windows" {
+		assert.NotZero(t, info.Mode()&0o111, "hook must be executable")
+	}
+	hookData, err := os.ReadFile(hookPath)
+	require.NoError(t, err)
+	assert.Contains(t, string(hookData), "fix",
+		"hook must invoke mdsmith fix; got:\n%s", hookData)
+	assert.Contains(t, string(hookData), "PLAN.md")
+	assert.Contains(t, string(hookData), "README.md")
 }
 
 func TestE2E_MergeDriver_Install_Idempotent(t *testing.T) {
@@ -1255,6 +1284,30 @@ func TestE2E_MergeDriver_Install_Idempotent(t *testing.T) {
 	assert.Equal(t, 1, count, "expected 1 PLAN.md entry, got %d; content:\n%s", count, attrs)
 }
 
+func TestE2E_MergeDriver_Install_UnmanagedHook(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, exec.Command("git", "init", dir).Run(), "git init")
+
+	// Place a user-authored hook (no mdsmith marker) before running install.
+	// Use gitHooksDir so setup targets the same path that install will check.
+	hooksDir := gitHooksDir(t, dir)
+	require.NoError(t, os.MkdirAll(hooksDir, 0o755))
+	hookPath := filepath.Join(hooksDir, "pre-merge-commit")
+	userHook := "#!/bin/sh\necho user hook\n"
+	require.NoError(t, os.WriteFile(hookPath, []byte(userHook), 0o755))
+
+	_, stderr, exitCode := runBinaryInDir(t, dir, "", "merge-driver", "install")
+	assert.Equal(t, 2, exitCode,
+		"expected exit 2 when unmanaged pre-merge-commit hook exists; stderr: %s", stderr)
+	assert.Contains(t, stderr, "pre-merge-commit",
+		"error must reference the hook path; stderr: %s", stderr)
+
+	// Verify the user hook was not clobbered.
+	data, err := os.ReadFile(hookPath)
+	require.NoError(t, err)
+	assert.Equal(t, userHook, string(data), "user hook content must be preserved")
+}
+
 func TestE2E_MergeDriver_Install_CustomFiles(t *testing.T) {
 	dir := t.TempDir()
 
@@ -1401,6 +1454,146 @@ func TestE2E_MergeDriver_SectionMarkersInsideConflict_Preserved(t *testing.T) {
 	assert.Contains(t, content, ">>>>>>>", "expected >>>>>>> marker preserved")
 }
 
+// TestE2E_MergeDriver_FileOrderingRace_Resolved reproduces the
+// CI failure from run 24971661273.
+//
+// Two branches each bump the status of a different plan file from
+// 🔲 to ✅ AND each regenerate PLAN.md against their own working
+// tree. When the branches are merged, both sides have modified
+// PLAN.md vs base, so git invokes the merge driver. The driver's
+// own `mdsmith fix` reads sibling plan/*.md files from the
+// working tree at that moment — but git has not yet processed
+// every plan path, so the regenerated catalog is stale relative
+// to the final merged state.
+//
+// With the pre-merge-commit hook installed by `merge-driver
+// install`, mdsmith fix runs again after every per-file merge has
+// settled, so PLAN.md ends up consistent with plan/*.md.
+const planTmpl = `---
+id: %d
+title: Plan %d
+status: "%s"
+---
+# Plan %d
+
+Body.
+`
+
+const planMdTmpl = "# Plans\n\n" +
+	"<?catalog\n" +
+	"glob:\n  - \"plan/*.md\"\n" +
+	"sort: id\n" +
+	"header: |\n\n  | ID | Status | Title |\n  |----|--------|-------|\n" +
+	"row: \"| {id} | {status} | [{title}]({filename}) |\"\n" +
+	"footer: |\n\n" +
+	"?>\n" +
+	"<?/catalog?>\n"
+
+// completePlanOnBranch creates branch from start, sets plan/<id>.md
+// status to ✅, regenerates PLAN.md, and commits.
+func completePlanOnBranch(t *testing.T, dir, branch, start string, planID int) {
+	t.Helper()
+	gitInDir(t, dir, "checkout", "-b", branch, start)
+	writeFixture(t, dir, fmt.Sprintf("plan/%02d.md", planID),
+		fmt.Sprintf(planTmpl, planID, planID, "✅", planID))
+	_, stderr, code := runBinaryInDir(t, dir, "", "fix", "PLAN.md")
+	require.Equal(t, 0, code, "%s fix failed: %s", branch, stderr)
+	gitCommit(t, dir, fmt.Sprintf("complete plan %d", planID))
+}
+
+func TestE2E_MergeDriver_FileOrderingRace_Resolved(t *testing.T) {
+	dir := t.TempDir()
+	gitInit(t, dir)
+
+	// Catalog over plan/*.md, sorted by id; rule names match the
+	// directives the merge driver knows how to regenerate.
+	writeFixture(t, dir, ".mdsmith.yml",
+		"rules:\n  catalog: true\n  include: true\n")
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "plan"), 0o755))
+	writeFixture(t, dir, "plan/01.md", fmt.Sprintf(planTmpl, 1, 1, "🔲", 1))
+	writeFixture(t, dir, "plan/02.md", fmt.Sprintf(planTmpl, 2, 2, "🔲", 2))
+	writeFixture(t, dir, "PLAN.md", planMdTmpl)
+
+	// Populate the catalog body once so the base commit is clean.
+	_, stderr, code := runBinaryInDir(t, dir, "", "fix", "PLAN.md")
+	require.Equal(t, 0, code, "seed fix failed: %s", stderr)
+	gitCommit(t, dir, "seed")
+	seedSHA := strings.TrimSpace(gitInDir(t, dir, "rev-parse", "HEAD"))
+
+	completePlanOnBranch(t, dir, "ours", seedSHA, 1)
+	completePlanOnBranch(t, dir, "theirs", seedSHA, 2)
+
+	// Install the merge driver — registers git config + the
+	// pre-merge-commit hook that closes the race.
+	gitInDir(t, dir, "checkout", "ours")
+	_, stderr, code = runBinaryInDir(t, dir, "", "merge-driver", "install")
+	require.Equal(t, 0, code, "install failed: %s", stderr)
+
+	// Merge theirs into ours. Both sides modified PLAN.md, so the
+	// per-file driver runs; the hook then re-fixes once every plan
+	// file is in its final merged state.
+	out, err := exec.Command("git", "-C", dir,
+		"-c", "commit.gpgsign=false",
+		"merge", "--no-ff", "-m", "merge theirs", "theirs").CombinedOutput()
+	require.NoError(t, err, "git merge failed: %s", out)
+
+	// PLAN.md catalog must reflect the post-merge plan files.
+	plan, err := os.ReadFile(filepath.Join(dir, "PLAN.md"))
+	require.NoError(t, err)
+	planStr := string(plan)
+	assert.Regexp(t, `\| 1 +\| ✅ +\|`, planStr,
+		"row 1 must show ✅ in merged PLAN.md, got:\n%s", planStr)
+	assert.Regexp(t, `\| 2 +\| ✅ +\|`, planStr,
+		"row 2 must show ✅ in merged PLAN.md, got:\n%s", planStr)
+
+	// Source plan files must agree with the catalog rows.
+	for _, path := range []string{"plan/01.md", "plan/02.md"} {
+		data, err := os.ReadFile(filepath.Join(dir, path))
+		require.NoError(t, err)
+		assert.Contains(t, string(data), `status: "✅"`,
+			"%s status must be ✅ after merge", path)
+	}
+
+	// Whole-tree consistency: a fresh check must report no issues
+	// — exactly what failed in CI run 24971661273.
+	_, stderr, code = runBinaryInDir(t, dir, "", "check", ".")
+	assert.Equal(t, 0, code,
+		"check after merge must pass; stderr:\n%s", stderr)
+}
+
+// gitInit initializes a git repo with isolated user/sign config so
+// commits succeed on machines that have global signing turned on.
+func gitInit(t *testing.T, dir string) {
+	t.Helper()
+	cmds := [][]string{
+		{"init", "-q", "-b", "main", dir},
+		{"-C", dir, "config", "user.name", "test"},
+		{"-C", dir, "config", "user.email", "test@example.com"},
+		{"-C", dir, "config", "commit.gpgsign", "false"},
+		{"-C", dir, "config", "tag.gpgsign", "false"},
+	}
+	for _, c := range cmds {
+		out, err := exec.Command("git", c...).CombinedOutput()
+		require.NoError(t, err, "git %v: %s", c, out)
+	}
+}
+
+// gitInDir runs git in dir and returns stdout. Test fails on non-zero.
+func gitInDir(t *testing.T, dir string, args ...string) string {
+	t.Helper()
+	full := append([]string{"-C", dir}, args...)
+	out, err := exec.Command("git", full...).CombinedOutput()
+	require.NoError(t, err, "git %v: %s", args, out)
+	return string(out)
+}
+
+// gitCommit stages everything and commits with the given message.
+func gitCommit(t *testing.T, dir, msg string) {
+	t.Helper()
+	gitInDir(t, dir, "add", "-A")
+	gitInDir(t, dir, "commit", "-q", "-m", msg)
+}
+
 // ── max-input-size ──────────────────────────────────────────────
 
 func TestCheck_MaxInputSize_ExceedingLimit(t *testing.T) {

cmd/mdsmith/mergedriver.go

@@ -391,12 +391,109 @@ func runMergeDriverInstall(args []string) int {
 		return 2
 	}
 
+	if err := ensurePreMergeCommitHook(repoRoot, files); err != nil {
+		fmt.Fprintf(os.Stderr,
+			"mdsmith: installing pre-merge-commit hook: %v\n", err)
+		return 2
+	}
+
+	hookPath := filepath.Join(resolveHooksDir(repoRoot), "pre-merge-commit")
 	fmt.Fprintf(os.Stderr, "mdsmith: merge driver 'mdsmith' installed\n")
 	fmt.Fprintf(os.Stderr, "  git config: merge.mdsmith.driver\n")
 	fmt.Fprintf(os.Stderr, "  .gitattributes: %s\n", attrPath)
+	fmt.Fprintf(os.Stderr, "  pre-merge-commit hook: %s\n", hookPath)
 	return 0
 }
 
+// preMergeCommitHookMarker identifies the hook as managed by
+// mdsmith so re-running install can safely replace it without
+// stomping on a user-authored hook of the same name.
+const preMergeCommitHookMarker = "# mdsmith merge-driver pre-merge-commit hook"
+
+// resolveHooksDir returns the directory where git hooks should be
+// installed. It respects core.hooksPath if configured so that
+// installations work correctly in repos that redirect hooks to a
+// custom path (e.g. via git config or a repo management tool).
+// Falls back to .git/hooks when git cannot be queried.
+func resolveHooksDir(repoRoot string) string {
+	cmd := exec.Command("git", "-C", repoRoot, "rev-parse", "--git-path", "hooks")
+	if out, err := cmd.Output(); err == nil {
+		p := strings.TrimSpace(string(out))
+		if !filepath.IsAbs(p) {
+			p = filepath.Join(repoRoot, p)
+		}
+		return filepath.Clean(p)
+	}
+	return filepath.Join(repoRoot, ".git", "hooks")
+}
+
+// ensurePreMergeCommitHook writes the pre-merge-commit hook so
+// that after git resolves all per-file merges (including any
+// driver-resolved sections) and before the merge commit is
+// created, mdsmith fix runs once on the registered files.
+//
+// The per-file merge driver cannot do this on its own: when it
+// runs on PLAN.md, sibling plan/*.md source files may still hold
+// "ours" content because git has not merged them yet, so the
+// regenerated catalog reflects a stale view of its sources. The
+// pre-merge-commit hook re-fixes the same files once every path
+// has reached its final merged state.
+func ensurePreMergeCommitHook(repoRoot string, files []string) error {
+	exe, err := resolveInstalledBinary()
+	if err != nil {
+		return fmt.Errorf("cannot locate mdsmith binary: %w", err)
+	}
+
+	hooksDir := resolveHooksDir(repoRoot)
+	hookPath := filepath.Join(hooksDir, "pre-merge-commit")
+
+	// Refuse to clobber a hook the user wrote themselves; replace
+	// only hooks that carry our marker. A non-ENOENT read error is
+	// treated as a safety failure to avoid silently overwriting an
+	// unreadable hook.
+	existing, readErr := os.ReadFile(hookPath)
+	switch {
+	case readErr == nil:
+		if !strings.Contains(string(existing), preMergeCommitHookMarker) {
+			return fmt.Errorf(
+				"%s already exists and is not managed by mdsmith; "+
+					"remove or merge it manually",
+				hookPath)
+		}
+	case os.IsNotExist(readErr):
+		// Hook doesn't exist; safe to create.
+	default:
+		return fmt.Errorf("reading existing hook %s: %w", hookPath, readErr)
+	}
+
+	// Build per-file fix commands as separate lines so that "set -e"
+	// aborts the hook if mdsmith fix or git add fails. Files that no
+	// longer exist (e.g. renamed in this branch) are skipped.
+	var fixCmds strings.Builder
+	for _, f := range files {
+		fmt.Fprintf(&fixCmds,
+			"if [ -e %s ]; then\n  %s fix -- %s\n  git add -- %s\nfi\n",
+			shellQuote(f), shellQuote(exe), shellQuote(f), shellQuote(f))
+	}
+
+	content := "#!/bin/sh\n" +
+		preMergeCommitHookMarker + "\n" +
+		"# Re-runs mdsmith fix once git has resolved every per-file\n" +
+		"# merge, so generated sections reflect the final merged\n" +
+		"# state of every source file. Re-install with:\n" +
+		"#   mdsmith merge-driver install\n" +
+		"set -e\n" +
+		fixCmds.String()
+
+	if err := os.MkdirAll(hooksDir, 0o755); err != nil {
+		return fmt.Errorf("creating %s: %w", hooksDir, err)
+	}
+	if err := os.WriteFile(hookPath, []byte(content), 0o755); err != nil {
+		return fmt.Errorf("writing %s: %w", hookPath, err)
+	}
+	return nil
+}
+
 // registerMergeDriver writes the merge.mdsmith.* keys to local
 // git config. It uses the absolute path of the current executable
 // so the driver works regardless of whether the install directory