Skip to content

Commit d6f4cae

Browse files
jedudenclaude
andauthored
Plan 220: Harden the git-index writers against a transient index.lock (#440)
* Start plan 220: Make the pre-merge-commit hook the single git-index writer https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * plan(220): reconcile with confirmed decision to harden, not single-write The maintainer resolved the open decision (2026-05-31) against the single-writer redesign. Keep MDS048 staging .gitattributes and only harden both git-index writers against a transient index.lock. - status 🔲 → 🔳 - title → "Harden the git-index writers against a transient index.lock" - rewrite Goal/Cause/Design/Tasks/Acceptance Criteria for two hardened writers; record the decision and drop the "Confirm before implementing" ask - refresh PLAN.md catalog via `mdsmith fix PLAN.md` https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * feat(githooks): retry StageGitattributes on a transient index.lock MDS048 stages .gitattributes via `git add` during `mdsmith fix`. When .git/index.lock briefly exists (a concurrent git invocation), that add failed hard and bounced the merge queue. Wrap the add in a bounded retry with backoff: a lock that clears within the window now stages successfully, and a persistent lock fails with a clear "index locked" message instead of a bare exit status. The retry only waits for the holder to release the lock — it never deletes a lock it did not create. A package-level git-add seam plus an overridable backoff schedule let tests drive the transient-clears and persistent-fails cases with a fake git, deterministically and instantly. Task 1 of plan 220. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * feat(githooks): retry the hook's git add on a transient index.lock The pre-merge-commit hook's staging loop ran a bare `git add -- "$f"`. When .git/index.lock briefly existed, that add failed (exit 128) and, under set -e, aborted the hook — bouncing the merge queue. Wrap each add in mdsmith_git_add, a POSIX retry-with-backoff that: - retries while git reports `index.lock ... File exists`, so a lock that clears within the window stages successfully; - backs off with `sleep 0.1 2>/dev/null || sleep 1` (fast on coreutils that honor fractional sleep, portable elsewhere); - on a persistent lock prints `index locked` and exits non-zero so the merge aborts loudly instead of committing a partial tree; - propagates a non-lock add failure immediately; - never deletes .git/index.lock — it only waits for the holder. The staging phase stays under `set +e` so the helper can inspect each add's exit status, and the pipeline status is re-raised afterward (the `while` body runs in a subshell, so the helper's exit would otherwise end only that subshell). HookMatchesCanonical now requires the mdsmith_git_add call, so a hook that drifted back to a bare `git add` loop is flagged (new bad fixture legacy-plain-git-add.sh). Golden file regenerated. Task 2 of plan 220. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * test(e2e): no-commit merge + hook + commit captures both writers' output Plan 220's integration acceptance criterion. Drive the merge-queue invocation model end-to-end with the real binary: `git merge --no-ff --no-commit`, run the installed pre-merge-commit hook, then `git commit`. Assert the merge commit captures both the regenerated .gitattributes (staged by MDS048's kept in-process `git add`) and the regenerated PLAN.md catalog (staged by the hook's hardened staging loop), and that the worktree is clean. The branches are arranged with no PLAN.md/.gitattributes conflict so the per-file merge driver does not pre-regenerate .gitattributes in the worktree; the committed-stale managed block then survives to hook time, giving MDS048 a genuine correction to stage — the realistic shape of the queue scenario, and proof that keeping MDS048 as a stager (rather than dropping it) lands the regenerated .gitattributes in the commit. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * docs(pre-merge-commit): document the index.lock retry behavior Note that the hook retries a transient .git/index.lock with bounded backoff, never deletes a lock it did not create, and exits non-zero with an `index locked` message when the lock persists. Task 3 of plan 220. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * refactor(githooks): extract shell helper + test setup to satisfy funlen golangci-lint funlen flagged BuildHookScript (62 lines) and the new e2e test (44 statements). Extract the hook's mdsmith_git_add shell function into a package-level stagingHelperShellFunc constant, and lift the e2e repo/branch setup into setupNoConflictMergeRepo. The generated hook script is byte-identical (golden unchanged); behavior is unchanged. Complete plan 220: all acceptance criteria verified (go test ./... and go tool golangci-lint run both clean), status 🔳 → ✅, catalog refreshed via `mdsmith fix PLAN.md`. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * test(githooks): cover empty-output stage error; drop unreachable lock branch codecov/patch (target auto, threshold 0%) flagged two new lines in StageGitattributes. The non-lock error path's empty-message branch was never exercised by the existing tests (their fake git always returns non-empty output), so add TestStageGitattributes_NonLockErrorEmptyOutput driving a fake git that fails with no output. The index-locked path's empty-message branch is unreachable: isIndexLockError matches only output containing index.lock and File exists, so the trimmed message is always non-empty there. Remove the dead branch per the repo's policy of not adding defensive branches that cannot be driven red/green. go test ./..., go tool golangci-lint run (0 issues), and mdsmith check . (0 failures) all pass. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * fix(merge-driver): stop MDS048's git add from running inside git merge Root cause of the merge-queue .git/index.lock bounce. git invokes the mdsmith merge driver from inside `git merge`, which holds .git/index.lock for the whole merge. The driver ran fixer.Fix with rule.All(), which includes MDS048 (git-hook-sync), whose Fix does an in-process `git add -- .gitattributes`. So every *.md conflict the driver resolved spawned a git add racing the parent `git merge` for the index lock; with four generated files auto-merging (copilot-instructions.md, AGENTS.md, CLAUDE.md, PLAN.md) that is four races per merge. This is the second index writer that runs DURING the merge, which the hook-side hardening never addressed. Confirmed from the failing GHA job log (PR #432 batch): a clean `git merge` of four driver-managed files, then a git add failing with 'index.lock: File exists', the lock persisting ~3s through `git merge --abort` — far longer than the hook's ~310ms retry budget could clear. Fix: the merge driver runs mergeDriverRules() — rule.All() minus the git-hook-sync rule — so it performs zero git-index mutation; a merge driver must be a pure content transform. The pre-merge-commit hook still runs MDS048 afterward, when git no longer holds the lock. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * test+fix(githooks): conflicting-merge e2e + harden hook git diff status Verifies the merge-driver root-cause fix and addresses the Copilot review on PR #440. - Add TestE2E_PreMergeCommit_ConflictingMergeResolvesWithHookSync: a real conflicting merge of PLAN.md's catalog with git-hook-sync enabled (the merge driver runs, then the hook). Proves the conflict resolves, commits cleanly, keeps .gitattributes, and leaves no stale .git/index.lock now that MDS048 no longer stages inside the driver. Setup factored into setupConflictingMergeRepo. - Hook staging loop: capture git diff's own exit status before the loop so a hard git diff failure is not masked by the pipeline (the pipeline status was the while, which exits 0 on empty input). Update HookMatchesCanonical and regenerate the golden hook. - StageGitattributes: correct the comment so it does not over-state isIndexLockError (it checks for the lock message; a non-empty msg is a consequence). go test ./..., go tool golangci-lint run (0 issues), and mdsmith check . (0 failures) all pass. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * docs(plan 220): center the merge-driver root cause The GHA job log confirmed the real second git-index writer is MDS048's in-process git add, run by the merge driver from inside git merge (which holds .git/index.lock) — not the hook's staging loop, which runs after the merge. Rewrite the summary, Goal, Cause, Design, Tasks, and Acceptance Criteria so the plan matches the implementation: the root-cause fix drops MDS048 from the merge driver's rule set, and the hook-side retry is defense-in-depth. Addresses the Copilot review comments on plan/220. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * test: strengthen lock-retry assertions and conflict-resolution check Apply /code-review (xhigh) findings on PR #440 — test/check hardening, no production behavior change: - PersistentLock test asserts the full retry budget (calls == len(stageRetryBackoff)+1), not merely > 1. - HookMatchesCanonical now requires stage_status=$?, so a drifted hook that keeps mdsmith_git_add but drops the exit-status re-raise (silently swallowing a persistent lock) is flagged as drift. - The conflicting-merge e2e asserts resolution directly via git ls-files -u (no unmerged paths), instead of a CONFLICT-string check the --no-commit exit code skipped in the passing case. - The transient-lock hook test asserts git add was retried exactly 3 times (2 failures + 1 success), so the retry path is verified rather than just eventual success. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * test+fix: pass-2 review — harden canonical drift detection and assertions Second /code-review (xhigh) pass on PR #440, plus the Copilot review: - HookMatchesCanonical now also requires the mdsmith_git_add() helper definition and the diff_status/stage_status exit guards, not just the captures — so a drifted hook that keeps the captures but drops the exit (silently swallowing a staging failure) or the helper definition (runtime error) is flagged as drift. (Copilot review + pass-2 finding.) - withStubGitAdd derives the stub backoff length from production (make of len(origBackoff)), so the persistent-lock assertion validates the real retry budget instead of a magic 5. - The transient-lock hook test derives its expected git-add count from failCount instead of the literal 3. - NoCommitMergeCapturesBoth now runs mdsmith check . (symmetry with the conflicting-merge e2e) to catch a structurally broken PLAN.md a clean worktree would otherwise hide. - Separate the NoCommitMergeCapturesBoth doc comment from setupNoConflictMergeRepo so go/doc attributes each correctly. Both passes found no production correctness bugs. Not applied (noted for follow-up): generalize the merge-driver MDS048 exclusion to a rule capability interface (touches internal/rule + githooksync, beyond this PR); unify the Go/shell retry budgets behind one constant. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ * refactor(merge-driver): exclude index-mutating rules via a capability interface Replace the hardcoded "MDS048" exclusion in mergeDriverRules with a rule.GitIndexMutator capability interface. MDS048 (git-hook-sync) implements it; the merge driver filters by the interface, so any future rule whose Fix mutates the git index is excluded from the merge-driver pipeline automatically — preventing a recurrence of the index.lock race. The test now asserts no GitIndexMutator survives in the merge-driver rule set, not just that one rule is dropped. Addresses the altitude finding from the /code-review passes. https://claude.ai/code/session_01WfEXMpKbVN9JBzn87H6MXQ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d3c968f commit d6f4cae

13 files changed

Lines changed: 959 additions & 136 deletions

File tree

PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,5 +149,5 @@ footer: |
149149
| 218 | 🔲 | opus | [WASM size reduction — CUE-free engine path and tinygo support](plan/218_wasm-size-reduction.md) |
150150
| 219 | 🔲 | opus | [Multiplexed AST walk to close the parity gap to mado](plan/219_multiplexed-ast-walk.md) |
151151
| 219 | 🔲 | opus | [Route cmd/mdsmith and the LSP through pkg/mdsmith.Session](plan/219_session-cli-lsp-migration.md) |
152-
| 220 | 🔲 | opus | [Make the pre-merge-commit hook the single git-index writer](plan/220_git-index-lock-retry.md) |
152+
| 220 | | opus | [Harden the git-index writers against a transient index.lock](plan/220_git-index-lock-retry.md) |
153153
<?/catalog?>

cmd/mdsmith/e2e_test.go

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1699,6 +1699,238 @@ func TestE2E_MergeDriver_FileOrderingRace_Resolved(t *testing.T) {
16991699
"check after merge must pass; stderr:\n%s", stderr)
17001700
}
17011701

1702+
// setupConflictingMergeRepo builds a repo whose `ours` and `theirs`
1703+
// branches both regenerate PLAN.md's catalog (ours adds plan 02,
1704+
// theirs adds plan 03), so merging conflicts inside the generated
1705+
// section and the mdsmith merge driver runs. git-hook-sync is
1706+
// enabled and the merge driver + hook are installed; ours is left
1707+
// checked out, ready to merge theirs.
1708+
func setupConflictingMergeRepo(t *testing.T) string {
1709+
t.Helper()
1710+
dir := t.TempDir()
1711+
gitInit(t, dir)
1712+
1713+
writeFixture(t, dir, ".mdsmith.yml",
1714+
"rules:\n catalog: true\n include: true\n git-hook-sync: true\n")
1715+
require.NoError(t, os.MkdirAll(filepath.Join(dir, "plan"), 0o755))
1716+
writeFixture(t, dir, "plan/01.md", fmt.Sprintf(planTmpl, 1, 1, "🔲", 1))
1717+
writeFixture(t, dir, "PLAN.md", planMdTmpl)
1718+
1719+
_, stderr, code := runBinaryInDir(t, dir, "", "merge-driver", "install")
1720+
require.Equal(t, 0, code, "install failed: %s", stderr)
1721+
_, stderr, code = runBinaryInDir(t, dir, "", "fix", "PLAN.md")
1722+
require.Equal(t, 0, code, "seed fix failed: %s", stderr)
1723+
gitCommit(t, dir, "seed")
1724+
seedSHA := strings.TrimSpace(gitInDir(t, dir, "rev-parse", "HEAD"))
1725+
1726+
// ours adds plan 02, theirs adds plan 03: both rewrite PLAN.md's
1727+
// catalog body, so merging conflicts inside the generated section.
1728+
completePlanOnBranch(t, dir, "ours", seedSHA, 2)
1729+
completePlanOnBranch(t, dir, "theirs", seedSHA, 3)
1730+
gitInDir(t, dir, "checkout", "ours")
1731+
return dir
1732+
}
1733+
1734+
// TestE2E_PreMergeCommit_ConflictingMergeResolvesWithHookSync runs the
1735+
// merge-queue model (`git merge --no-ff --no-commit`, then the
1736+
// pre-merge-commit hook, then `git commit`) on a *conflicting* merge of
1737+
// a driver-managed generated file, with git-hook-sync (MDS048) enabled
1738+
// — the shape of the failing queue run.
1739+
//
1740+
// ours adds plan 02 and theirs adds plan 03, so both rewrite PLAN.md's
1741+
// generated catalog and `git merge` invokes the mdsmith merge driver on
1742+
// PLAN.md while git holds .git/index.lock. The driver must resolve the
1743+
// conflict (regenerate the catalog from every plan file) WITHOUT running
1744+
// MDS048's in-process `git add` — that add would race the merge for the
1745+
// lock. MDS048 still runs in the hook afterward, when git no longer
1746+
// holds the lock.
1747+
//
1748+
// This guards the concern that dropping MDS048 from the merge driver
1749+
// could leave the conflict unresolved: it asserts the catalog resolves
1750+
// to list every plan, the merge commits cleanly, .gitattributes
1751+
// survives, the worktree is clean, and no stale .git/index.lock remains.
1752+
func TestE2E_PreMergeCommit_ConflictingMergeResolvesWithHookSync(t *testing.T) {
1753+
if _, err := exec.LookPath("git"); err != nil {
1754+
t.Skip("git not available")
1755+
}
1756+
dir := setupConflictingMergeRepo(t)
1757+
1758+
mergeOut, mergeErr := exec.Command("git", "-C", dir,
1759+
"-c", "commit.gpgsign=false",
1760+
"merge", "--no-ff", "--no-commit", "theirs").CombinedOutput()
1761+
// `git merge --no-commit` exits non-zero by design, and some git
1762+
// versions print "CONFLICT" even when a merge driver resolves the
1763+
// file — so assert resolution directly: no unmerged index paths.
1764+
require.Empty(t, strings.TrimSpace(gitInDir(t, dir, "ls-files", "-u")),
1765+
"merge driver must leave no unmerged paths (conflict resolved); "+
1766+
"merge exit=%v output:\n%s", mergeErr, mergeOut)
1767+
1768+
hook := exec.Command(filepath.Join(gitHooksDir(t, dir), "pre-merge-commit"))
1769+
hook.Dir = dir
1770+
hookOut, hookErr := hook.CombinedOutput()
1771+
require.NoErrorf(t, hookErr, "pre-merge-commit hook failed: %s", hookOut)
1772+
1773+
commitOut, err := exec.Command("git", "-C", dir,
1774+
"-c", "commit.gpgsign=false",
1775+
"commit", "--no-edit").CombinedOutput()
1776+
require.NoErrorf(t, err, "git commit (merge) failed: %s", commitOut)
1777+
1778+
// Conflict resolved: the committed catalog lists all three plans.
1779+
committedPlan := gitInDir(t, dir, "show", "HEAD:PLAN.md")
1780+
for _, id := range []string{"1", "2", "3"} {
1781+
assert.Regexp(t, `\| `+id+` +\|`, committedPlan,
1782+
"merged PLAN.md catalog must list plan %s; got:\n%s", id, committedPlan)
1783+
}
1784+
assert.NotContains(t, committedPlan, "<<<<<<<",
1785+
"no conflict markers may survive into the merge commit")
1786+
1787+
// .gitattributes survived, no stale lock, clean worktree.
1788+
committedAttrs := gitInDir(t, dir, "show", "HEAD:.gitattributes")
1789+
assert.Contains(t, committedAttrs, "merge=mdsmith",
1790+
"merge commit must keep the managed .gitattributes")
1791+
assert.NoFileExists(t, filepath.Join(dir, ".git", "index.lock"),
1792+
"no stale .git/index.lock may remain after the merge")
1793+
status := strings.TrimSpace(gitInDir(t, dir, "status", "--porcelain"))
1794+
assert.Empty(t, status,
1795+
"worktree must be clean after the merge commit; got:\n%s", status)
1796+
1797+
_, stderr, code := runBinaryInDir(t, dir, "", "check", ".")
1798+
assert.Equal(t, 0, code, "check after merge must pass; stderr:\n%s", stderr)
1799+
}
1800+
1801+
// TestE2E_PreMergeCommit_NoCommitMergeCapturesBoth exercises the
1802+
// invocation model the merge queue uses: `git merge --no-ff
1803+
// --no-commit`, then the pre-merge-commit hook, then a separate `git
1804+
// commit`. It is the plan-220 integration acceptance criterion.
1805+
//
1806+
// The repo carries a deliberately stale .gitattributes managed block
1807+
// (missing the canonical *.markdown include) and a PLAN.md whose
1808+
// catalog has not yet been regenerated for a newly merged plan file.
1809+
// The hook's single `mdsmith fix .` then has real work on two fronts:
1810+
// MDS048 rewrites .gitattributes and stages it (its in-process staging
1811+
// is kept, not dropped), and the catalog rule regenerates PLAN.md
1812+
// (staged by the hook's own hardened staging loop). The resulting
1813+
// merge commit must capture both, and the worktree must be clean.
1814+
//
1815+
// The branches are arranged so the merge has no PLAN.md/.gitattributes
1816+
// conflict: `theirs` adds plan/02.md without regenerating PLAN.md, and
1817+
// `ours` touches only an unrelated file. That keeps the per-file merge
1818+
// driver from pre-regenerating .gitattributes in the worktree, so the
1819+
// stale managed block survives to hook time and MDS048 has a genuine
1820+
// correction to stage — the realistic shape of the queue bug.
1821+
1822+
// setupNoConflictMergeRepo builds the repo and two branches for
1823+
// TestE2E_PreMergeCommit_NoCommitMergeCapturesBoth: the merge driver
1824+
// and hook are installed, a deliberately stale .gitattributes managed
1825+
// block (missing *.markdown) is committed directly via git so no later
1826+
// `mdsmith fix` re-canonicalises it, `theirs` adds plan/02.md without
1827+
// regenerating PLAN.md, and `ours` makes an unrelated change. ours is
1828+
// left checked out, ready to merge theirs.
1829+
func setupNoConflictMergeRepo(t *testing.T) string {
1830+
t.Helper()
1831+
dir := t.TempDir()
1832+
gitInit(t, dir)
1833+
1834+
writeFixture(t, dir, ".mdsmith.yml",
1835+
"rules:\n catalog: true\n include: true\n git-hook-sync: true\n")
1836+
require.NoError(t, os.MkdirAll(filepath.Join(dir, "plan"), 0o755))
1837+
writeFixture(t, dir, "plan/01.md", fmt.Sprintf(planTmpl, 1, 1, "🔲", 1))
1838+
writeFixture(t, dir, "PLAN.md", planMdTmpl)
1839+
1840+
_, stderr, code := runBinaryInDir(t, dir, "", "merge-driver", "install")
1841+
require.Equal(t, 0, code, "install failed: %s", stderr)
1842+
_, stderr, code = runBinaryInDir(t, dir, "", "fix", "PLAN.md")
1843+
require.Equal(t, 0, code, "seed fix failed: %s", stderr)
1844+
1845+
staleAttrs := "# BEGIN mdsmith merge-driver\n" +
1846+
"*.md merge=mdsmith\n" +
1847+
"# END mdsmith merge-driver\n"
1848+
writeFixture(t, dir, ".gitattributes", staleAttrs)
1849+
gitCommit(t, dir, "seed (stale .gitattributes)")
1850+
seedSHA := strings.TrimSpace(gitInDir(t, dir, "rev-parse", "HEAD"))
1851+
1852+
gitInDir(t, dir, "checkout", "-q", "-b", "theirs", seedSHA)
1853+
writeFixture(t, dir, "plan/02.md", fmt.Sprintf(planTmpl, 2, 2, "🔲", 2))
1854+
gitInDir(t, dir, "add", "plan/02.md")
1855+
gitInDir(t, dir, "-c", "commit.gpgsign=false", "commit", "-q", "-m",
1856+
"add plan 2 (catalog intentionally not regenerated)")
1857+
1858+
gitInDir(t, dir, "checkout", "-q", "-b", "ours", seedSHA)
1859+
writeFixture(t, dir, "NOTES.txt", "note\n")
1860+
gitInDir(t, dir, "add", "NOTES.txt")
1861+
gitInDir(t, dir, "-c", "commit.gpgsign=false", "commit", "-q", "-m", "ours note")
1862+
return dir
1863+
}
1864+
1865+
func TestE2E_PreMergeCommit_NoCommitMergeCapturesBoth(t *testing.T) {
1866+
if _, err := exec.LookPath("git"); err != nil {
1867+
t.Skip("git not available")
1868+
}
1869+
dir := setupNoConflictMergeRepo(t)
1870+
1871+
// Step 1: `git merge --no-ff --no-commit`, leaving the commit
1872+
// uncreated, exactly as the merge-queue action does.
1873+
out, err := exec.Command("git", "-C", dir,
1874+
"-c", "commit.gpgsign=false",
1875+
"merge", "--no-ff", "--no-commit", "theirs").CombinedOutput()
1876+
// A clean --no-commit merge exits non-zero ("stopped before
1877+
// committing as requested"); only a real conflict is a failure.
1878+
if err != nil {
1879+
require.NotContains(t, string(out), "CONFLICT",
1880+
"merge must apply cleanly with no conflict: %s", out)
1881+
}
1882+
1883+
// Precondition: the stale block survived to hook time (the driver
1884+
// did not pre-regenerate it), so MDS048 has a real correction.
1885+
preHookAttrs, readErr := os.ReadFile(filepath.Join(dir, ".gitattributes"))
1886+
require.NoError(t, readErr)
1887+
require.NotContains(t, string(preHookAttrs), "*.markdown merge=mdsmith",
1888+
"the stale .gitattributes must survive to hook time for this test "+
1889+
"to exercise MDS048's regeneration; got:\n%s", preHookAttrs)
1890+
1891+
// Step 2: run the installed hook, exactly as the action does.
1892+
hookPath := filepath.Join(gitHooksDir(t, dir), "pre-merge-commit")
1893+
hookCmd := exec.Command(hookPath)
1894+
hookCmd.Dir = dir
1895+
hookOut, hookErr := hookCmd.CombinedOutput()
1896+
require.NoErrorf(t, hookErr, "pre-merge-commit hook failed: %s", hookOut)
1897+
1898+
// Step 3: create the merge commit.
1899+
out, err = exec.Command("git", "-C", dir,
1900+
"-c", "commit.gpgsign=false",
1901+
"commit", "--no-edit").CombinedOutput()
1902+
require.NoErrorf(t, err, "git commit (merge) failed: %s", out)
1903+
1904+
// The merge commit's tree must contain both the regenerated
1905+
// .gitattributes (now including the previously missing *.markdown
1906+
// line, staged by MDS048) and the regenerated PLAN.md catalog (now
1907+
// listing plan 2, staged by the hook's staging loop).
1908+
committedAttrs := gitInDir(t, dir, "show", "HEAD:.gitattributes")
1909+
assert.Contains(t, committedAttrs, "*.markdown merge=mdsmith",
1910+
"merge commit must capture the regenerated .gitattributes "+
1911+
"(the *.markdown include MDS048 added); got:\n%s",
1912+
committedAttrs)
1913+
1914+
committedPlan := gitInDir(t, dir, "show", "HEAD:PLAN.md")
1915+
assert.Regexp(t, `\| 1 +\|`, committedPlan,
1916+
"merge commit's PLAN.md must list plan 1; got:\n%s", committedPlan)
1917+
assert.Regexp(t, `\| 2 +\|`, committedPlan,
1918+
"merge commit's PLAN.md must list the merged plan 2 (catalog "+
1919+
"regenerated by the hook); got:\n%s", committedPlan)
1920+
1921+
// The worktree must be clean: every regenerated file the hook
1922+
// touched is committed, nothing left modified or untracked.
1923+
status := gitInDir(t, dir, "status", "--porcelain")
1924+
assert.Empty(t, strings.TrimSpace(status),
1925+
"worktree must be clean after the hook+commit flow; git status:\n%s", status)
1926+
1927+
// A fresh check must pass — catches a structurally broken PLAN.md
1928+
// (malformed table, leftover markers) that the catalog-row regexps
1929+
// above would miss on a clean-but-wrong tree.
1930+
_, checkStderr, checkCode := runBinaryInDir(t, dir, "", "check", ".")
1931+
assert.Equal(t, 0, checkCode, "check after merge must pass; stderr:\n%s", checkStderr)
1932+
}
1933+
17021934
// gitInit initializes a git repo with isolated user/sign config so
17031935
// commits succeed on machines that have global signing turned on.
17041936
func gitInit(t *testing.T, dir string) {

cmd/mdsmith/mergedriver.go

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,34 @@ func readAndRestore(pathname string, backup []byte, backupErr error, mode os.Fil
328328
return fixed, 0
329329
}
330330

331+
// mergeDriverRules is the fix rule set the merge driver runs: every
332+
// registered rule except those that mutate the git index (rules
333+
// implementing rule.GitIndexMutator — today only MDS048,
334+
// git-hook-sync).
335+
//
336+
// git invokes the merge driver from inside `git merge`, which holds
337+
// `.git/index.lock` for the whole merge. A rule whose Fix runs an
338+
// in-process `git add` (e.g. githooks.StageGitattributes) would be a
339+
// second index writer racing the parent `git merge` for that lock,
340+
// which can leave a stale `.git/index.lock` that fails the staging
341+
// step and bounces the merge queue. The merge driver only needs the
342+
// content-regenerating rules to resolve a conflict, so index-mutating
343+
// rules are dropped; the pre-merge-commit hook still runs them
344+
// afterward, when git no longer holds the lock. Filtering by the
345+
// capability interface (not a rule ID) excludes any future
346+
// index-mutating rule automatically.
347+
func mergeDriverRules() []rule.Rule {
348+
all := rule.All()
349+
out := make([]rule.Rule, 0, len(all))
350+
for _, r := range all {
351+
if m, ok := r.(rule.GitIndexMutator); ok && m.MutatesGitIndex() {
352+
continue
353+
}
354+
out = append(out, r)
355+
}
356+
return out
357+
}
358+
331359
// fixFileInPlace runs the mdsmith fix pipeline on a single file.
332360
func fixFileInPlace(path string, maxBytes int64) error {
333361
cfg, _, err := loadConfig("")
@@ -337,7 +365,7 @@ func fixFileInPlace(path string, maxBytes int64) error {
337365

338366
fixer := &fixpkg.Fixer{
339367
Config: cfg,
340-
Rules: rule.All(),
368+
Rules: mergeDriverRules(),
341369
StripFrontMatter: frontMatterEnabled(cfg),
342370
Logger: &vlog.Logger{},
343371
MaxInputBytes: maxBytes,

cmd/mdsmith/mergedriver_test.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,32 @@ import (
1010
"testing"
1111

1212
"github.com/jeduden/mdsmith/internal/githooks"
13+
"github.com/jeduden/mdsmith/internal/rule"
1314
"github.com/stretchr/testify/assert"
1415
"github.com/stretchr/testify/require"
1516
)
1617

18+
func TestMergeDriverRules_ExcludesGitIndexMutators(t *testing.T) {
19+
// Precondition: MDS048 (git-hook-sync) is a registered git-index
20+
// mutator, so the exclusion is meaningful rather than a no-op.
21+
mds048 := rule.ByID("MDS048")
22+
require.NotNil(t, mds048, "precondition: MDS048 must be registered")
23+
m, ok := mds048.(rule.GitIndexMutator)
24+
require.True(t, ok && m.MutatesGitIndex(),
25+
"precondition: MDS048 must declare rule.GitIndexMutator")
26+
27+
rules := mergeDriverRules()
28+
for _, r := range rules {
29+
gm, ok := r.(rule.GitIndexMutator)
30+
assert.False(t, ok && gm.MutatesGitIndex(),
31+
"the merge driver runs inside `git merge` (which holds "+
32+
".git/index.lock); it must exclude every git-index-mutating "+
33+
"rule, but %s remained", r.ID())
34+
}
35+
assert.NotEmpty(t, rules,
36+
"mergeDriverRules must still run the content-regenerating rules")
37+
}
38+
1739
func TestStripSectionConflicts_Diff3CatalogConflict(t *testing.T) {
1840
// diff3-style conflict markers include a ||||||| base section
1941
// between <<<<<<< and =======. The merge driver must strip all

docs/reference/cli/pre-merge-commit.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@ Manage a Git `pre-merge-commit` hook that runs
1111
commit is created. Modified `.md` / `.markdown` files are
1212
re-staged automatically.
1313

14+
A concurrent git process can briefly hold
15+
`.git/index.lock`. When `git add` fails for that reason,
16+
the hook retries with a bounded backoff. A transient
17+
lock no longer aborts the merge. The hook never deletes
18+
a lock it did not create. If the lock outlasts the
19+
retries, the hook prints `index locked` and exits
20+
non-zero, so the merge stops instead of committing a
21+
partially staged tree.
22+
1423
```text
1524
mdsmith pre-merge-commit <subcommand> [args]
1625
```

0 commit comments

Comments
 (0)