Fix pre-merge-commit hook to stage fixes when diagnostics remain (#232)

* test: reproduce hook bug where fixes aren't staged when fix exits 1

The merge queue's pre-merge-commit hook silently drops files modified
by 'mdsmith fix .' whenever fix returns exit code 1 (unfixed
diagnostics remain). This was observed on bisect branch
merge-queue/batch-bisect-224-1777817057 (SHA b1ade01) where
mdsmith fix regenerated PLAN.md to reflect plan/120's new status
but the change never reached the merge commit.

Root cause: the canonical hook script uses

    if ! '$exe' fix .; then
      status=$?
      if [ "$status" -ne 1 ]; then
        exit "$status"
      fi
    fi

POSIX '! cmd' returns the logical NOT of cmd's exit status, so when
mdsmith fix exits 1, $? immediately after is 0. The script then
captures status=0, the [ 0 -ne 1 ] test is true, and the hook exits
0 BEFORE running the 'git add' staging loop.

The new test stands up a tiny git repo with a fake mdsmith that
modifies a tracked file and exits 1, runs the canonical hook, and
asserts the modified file ends up in the index. The test fails
today, demonstrating the exit-code clobbering bug.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

* fix: capture mdsmith fix exit status before negation in hook

POSIX 'if ! cmd; then status=$?; ...' clobbers cmd's exit code: $?
after '! cmd' is the logical NOT of cmd's exit status, so when
mdsmith fix exits 1 (unfixed diagnostics remain), status captures 0.
The follow-up '[ "$status" -ne 1 ]' is then true and the hook exits
0 before the 'git add' staging loop ever runs.

The visible symptom on the merge queue: PLAN.md (and other catalog
files) get regenerated by mdsmith fix in the working tree, but the
merge commit ships the stale pre-fix content because nothing
staged the fixes. CI on the resulting commit then fails on MDS019,
even though the same fix run locally succeeds.

The new template wraps the fix invocation in 'set +e' / 'set -e' and
captures $? directly, then propagates any exit status that is
neither 0 (clean) nor 1 (unfixed remain) so genuine config errors
or panics still abort the merge:

    set +e
    '$exe' fix .
    status=$?
    set -e
    if [ "$status" -ne 0 ] && [ "$status" -ne 1 ]; then
      exit "$status"
    fi

HookMatchesCanonical and the cmd/mdsmith install tests are updated
to match the new fragments. The previously-failing reproduction
test in internal/githooks/githooks_unix_test.go now passes.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

* Replace script-content assertions with golden file + behavior tests

Content checks on BuildHookScript's output (Contains calls looking for
specific shell fragments) were brittle: any whitespace or wording change
broke them. Replace with:

- A golden-file test in internal/githooks that byte-compares
  BuildHookScript("/usr/local/bin/mdsmith") against
  testdata/pre-merge-commit.golden.sh; run with UPDATE_GOLDEN=1 to
  regenerate the golden file when the template changes.

- Equality checks in the install/ensure tests that compare the written
  hook content against githooks.BuildHookScript (the canonical output),
  so they verify behavior (correct file written) without embedding
  shell fragments inline.

- The e2e install test drops content checks entirely; exact rendering
  is covered by the golden-file test; the e2e test confirms the file
  exists and is executable.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

* Add e2e behavioral tests covering all pre-merge-commit hook branches

The hook has four observable branches:
1. fix exits 0, nothing modified → staging loop runs, nothing staged
2. fix exits 0, files modified → staging loop stages them
3. fix exits 1 (unfixed diagnostics remain) → staging loop still runs
   and stages the fixable changes (regression for bisect-branch bug)
4. fix exits anything else → hook propagates the code, aborting merge

Each branch now has a dedicated e2e test in
TestE2E_PreMergeCommitHook_*. Tests 1–3 install the hook via
`mdsmith pre-merge-commit install` (real binary) and exercise it
in a real git repo with real markdown files. Test 4 installs a fake
mdsmith that exits 2 to verify the propagation path.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

* Require set +e in HookMatchesCanonical drift check

Without this, a hook that invokes `mdsmith fix .` under active set -e
(without the set +e guard) would pass the drift check even though it
would abort immediately when fix exits 1 — silently skipping the
staging loop that re-stages the fixed files.

Add "set +e" to the required fragments so any installed hook missing
the errexit-disable guard is flagged as drifted and prompts reinstall.
Add TestHookMatchesCanonical_RejectsMissingSetPlusE to pin the contract.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

* Replace fragment-based HookMatchesCanonical tests with golden files and behavioral test

Each bad-hook scenario now lives as a complete shell script under
testdata/hooks/bad/. A single table-driven test reads every file in
that directory and asserts HookMatchesCanonical returns false, replacing
nine inline string-concatenation tests.

A new behavioral test (TestHookScript_MissingSetPlusE_FailsToStageOnExitOne)
runs the missing-set-plus-e golden file in a real git repo and asserts the
file is NOT staged when fake mdsmith exits 1 — documenting the original
merge-queue bug and proving the drift detection is meaningful.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

* Assert unfixable diagnostics remain after ExitOneStagesFixed hook run

The test claimed mdsmith fix exits 1 due to an unfixable diagnostic but
never verified that the diagnostic actually remains after the hook. Add a
mdsmith check assertion after the hook to confirm a non-zero exit, ensuring
the test is not vacuous if the fixture's chosen rule ever becomes fixable.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

---------

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

Author: jeduden

cmd/mdsmith/e2e_test.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/jeduden/mdsmith/internal/githooks"
 	"github.com/jeduden/mdsmith/internal/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -1260,12 +1261,11 @@ func TestE2E_MergeDriver_Install(t *testing.T) {
 	if runtime.GOOS != "windows" {
 		assert.NotZero(t, info.Mode()&0o111, "hook must be executable")
 	}
+	// The exact hook content is verified by the golden-file test in
+	// internal/githooks; here we confirm that install wrote a non-empty file.
 	hookData, err := os.ReadFile(hookPath)
 	require.NoError(t, err)
-	assert.Contains(t, string(hookData), "fix .; then",
-		"hook must invoke mdsmith fix . via the exit-1-tolerant guard; got:\n%s", hookData)
-	assert.Contains(t, string(hookData), "git diff --name-only -- '*.md' '*.markdown'",
-		"hook must stage modified markdown files via the glob-based diff")
+	assert.NotEmpty(t, hookData, "installed hook must not be empty")
 }
 
 func TestE2E_MergeDriver_Install_Idempotent(t *testing.T) {
@@ -1771,3 +1771,158 @@ func TestQuery_MaxInputSize_InvalidValue(t *testing.T) {
 	assert.Equal(t, 2, exitCode, "expected exit code 2 for invalid size")
 	assert.Contains(t, stderr, "invalid max-input-size")
 }
+
+// ── pre-merge-commit hook behavior ──────────────────────────────────────────
+//
+// Each test runs the installed hook script directly (as git would after
+// `git merge --no-commit`) and verifies one branch of the hook logic.
+
+// runHookScript executes .git/hooks/pre-merge-commit in dir and returns
+// its exit code.
+func runHookScript(t *testing.T, dir string) int {
+	t.Helper()
+	hookPath := filepath.Join(gitHooksDir(t, dir), "pre-merge-commit")
+	cmd := exec.Command(hookPath)
+	cmd.Dir = dir
+	if err := cmd.Run(); err != nil {
+		if exitErr, ok := err.(*exec.ExitError); ok {
+			return exitErr.ExitCode()
+		}
+		t.Fatalf("unexpected error running hook: %v", err)
+	}
+	return 0
+}
+
+// gitDiffNames returns the set of files shown by `git diff --name-only`
+// (working tree vs index) in dir — the same query the hook uses to decide
+// what to stage.
+func gitDiffNames(t *testing.T, dir string) string {
+	t.Helper()
+	out, err := exec.Command("git", "-C", dir, "diff", "--name-only").Output()
+	require.NoError(t, err)
+	return strings.TrimSpace(string(out))
+}
+
+// TestE2E_PreMergeCommitHook_ExitZeroNothingStaged covers the branch
+// where mdsmith fix exits 0 and the working tree is already clean: the
+// staging loop runs but `git diff` finds nothing, so nothing is staged
+// and the hook exits 0.
+func TestE2E_PreMergeCommitHook_ExitZeroNothingStaged(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("pre-merge-commit is a POSIX shell script")
+	}
+	dir := t.TempDir()
+	gitInit(t, dir)
+	writeFixture(t, dir, ".mdsmith.yml", "rules: {}\n")
+	writeFixture(t, dir, "README.md", "# Title\n\nHello\n")
+	gitCommit(t, dir, "init")
+
+	_, _, code := runBinaryInDir(t, dir, "", "pre-merge-commit", "install")
+	require.Equal(t, 0, code, "pre-merge-commit install must succeed")
+
+	assert.Equal(t, 0, runHookScript(t, dir),
+		"hook must exit 0 when there is nothing to fix")
+	assert.Empty(t, gitDiffNames(t, dir),
+		"nothing should be staged when the working tree is already clean")
+}
+
+// TestE2E_PreMergeCommitHook_ExitZeroStagesFixed covers the branch
+// where mdsmith fix modifies files and exits 0 (all issues resolved):
+// the staging loop must detect the changed files and stage them.
+func TestE2E_PreMergeCommitHook_ExitZeroStagesFixed(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("pre-merge-commit is a POSIX shell script")
+	}
+	dir := t.TempDir()
+	gitInit(t, dir)
+	writeFixture(t, dir, ".mdsmith.yml", "rules: {}\n")
+	writeFixture(t, dir, "README.md", "# Title\n\nHello\n")
+	gitCommit(t, dir, "init")
+
+	// Simulate post-merge index state: the merged file has trailing spaces.
+	writeFixture(t, dir, "README.md", "# Title\n\nHello   \n")
+	gitInDir(t, dir, "add", "README.md")
+
+	_, _, code := runBinaryInDir(t, dir, "", "pre-merge-commit", "install")
+	require.Equal(t, 0, code)
+
+	assert.Equal(t, 0, runHookScript(t, dir),
+		"hook must exit 0 when all issues are fixed")
+	assert.Empty(t, gitDiffNames(t, dir),
+		"hook must stage the fix so working tree and index match")
+
+	data, err := os.ReadFile(filepath.Join(dir, "README.md"))
+	require.NoError(t, err)
+	assert.Equal(t, "# Title\n\nHello\n", string(data),
+		"mdsmith fix must have removed the trailing spaces")
+}
+
+// TestE2E_PreMergeCommitHook_ExitOneStagesFixed is the regression test
+// for the merge-queue bisect failure: when mdsmith exits 1 (unfixable
+// diagnostics remain), the staging loop must still run and stage the
+// files that fix did manage to clean up.
+func TestE2E_PreMergeCommitHook_ExitOneStagesFixed(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("pre-merge-commit is a POSIX shell script")
+	}
+	dir := t.TempDir()
+	gitInit(t, dir)
+	writeFixture(t, dir, ".mdsmith.yml", "rules: {}\n")
+	writeFixture(t, dir, "README.md", "# Title\n\nHello\n")
+	gitCommit(t, dir, "init")
+
+	// File with a fixable issue (trailing space) AND an unfixable one
+	// (heading contains `!`, caught by a default rule), so fix exits 1.
+	writeFixture(t, dir, "README.md", "# Title!\n\nHello   \n")
+	gitInDir(t, dir, "add", "README.md")
+
+	_, _, code := runBinaryInDir(t, dir, "", "pre-merge-commit", "install")
+	require.Equal(t, 0, code)
+
+	assert.Equal(t, 0, runHookScript(t, dir),
+		"hook must exit 0 even when mdsmith fix exits 1 (unfixed diagnostics remain)")
+	assert.Empty(t, gitDiffNames(t, dir),
+		"hook must stage the fixable changes even when mdsmith fix exits 1")
+
+	data, err := os.ReadFile(filepath.Join(dir, "README.md"))
+	require.NoError(t, err)
+	assert.NotContains(t, string(data), "Hello   ",
+		"trailing spaces must have been removed and staged")
+
+	// Confirm that unfixable diagnostics genuinely remain after the hook ran,
+	// which is what drives `mdsmith fix` to exit 1. If check exits 0 here the
+	// fixture no longer exercises the exit-1 branch and the test is vacuous.
+	_, _, checkCode := runBinaryInDir(t, dir, "", "check", ".")
+	assert.NotEqual(t, 0, checkCode,
+		"mdsmith check must still report diagnostics after the hook: "+
+			"the unfixable issue (heading punctuation) must remain so the "+
+			"test actually exercises the fix-exits-1 branch")
+}
+
+// TestE2E_PreMergeCommitHook_PropagatesHardError covers the branch
+// where mdsmith exits with a code other than 0 or 1 (fatal error). The
+// hook must propagate that code so the merge commit aborts.
+func TestE2E_PreMergeCommitHook_PropagatesHardError(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("pre-merge-commit is a POSIX shell script")
+	}
+	dir := t.TempDir()
+	gitInit(t, dir)
+
+	// Fake mdsmith that exits 2, simulating a fatal error (config parse
+	// failure, crash, etc.).
+	fakeMdsmith := filepath.Join(dir, "fake-mdsmith")
+	require.NoError(t, os.WriteFile(fakeMdsmith,
+		[]byte("#!/bin/sh\nexit 2\n"), 0o755))
+
+	hooksDir := gitHooksDir(t, dir)
+	require.NoError(t, os.MkdirAll(hooksDir, 0o755))
+	require.NoError(t, os.WriteFile(
+		filepath.Join(hooksDir, "pre-merge-commit"),
+		[]byte(githooks.BuildHookScript(fakeMdsmith)),
+		0o755,
+	))
+
+	assert.Equal(t, 2, runHookScript(t, dir),
+		"hook must propagate fatal exit codes (anything other than 0 or 1) to abort the merge")
+}

cmd/mdsmith/mergedriver_test.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/jeduden/mdsmith/internal/githooks"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -478,14 +479,8 @@ func TestEnsurePreMergeCommitHook_CreatesExecutableHook(t *testing.T) {
 
 	data, err := os.ReadFile(hookPath)
 	require.NoError(t, err)
-	content := string(data)
-	assert.Contains(t, content, preMergeCommitHookMarker)
-	assert.Contains(t, content, "if ! '/usr/local/bin/mdsmith' fix .; then",
-		"hook must invoke the resolved mdsmith binary inside the exit-1-tolerant guard")
-	assert.Contains(t, content, `if [ "$status" -ne 1 ]; then`,
-		"hook must propagate exit codes other than 1 (unfixed diagnostics)")
-	assert.Contains(t, content, "git diff --name-only -- '*.md' '*.markdown'",
-		"hook must stage modified markdown files via the glob-based diff")
+	assert.Equal(t, githooks.BuildHookScript("/usr/local/bin/mdsmith"), string(data),
+		"installed hook must match the canonical template")
 }
 
 func TestEnsurePreMergeCommitHook_OverwritesManagedHook(t *testing.T) {
@@ -507,8 +502,8 @@ func TestEnsurePreMergeCommitHook_OverwritesManagedHook(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotContains(t, string(data), "stale content",
 		"managed hook must be replaced, not preserved")
-	assert.Contains(t, string(data), "fix .; then",
-		"replaced hook must use the glob-based template")
+	assert.Equal(t, githooks.BuildHookScript("/usr/local/bin/mdsmith"), string(data),
+		"replaced hook must match the canonical template")
 }
 
 func TestEnsurePreMergeCommitHook_SetsExecutableBitOnExistingHook(t *testing.T) {

cmd/mdsmith/premergecommit_test.go

@@ -253,8 +253,8 @@ func TestPreMergeCommitInstall_NoArgsInstallsCanonicalHook(t *testing.T) {
 	assert.Contains(t, got, "pre-merge-commit hook installed")
 	hookData, err := os.ReadFile(filepath.Join(resolveHooksDir(dir), "pre-merge-commit"))
 	require.NoError(t, err)
-	assert.Contains(t, string(hookData), "fix .; then",
-		"installed hook must use the glob-based template")
+	assert.Equal(t, githooks.BuildHookScript("/usr/local/bin/mdsmith"), string(hookData),
+		"installed hook must match the canonical template")
 }
 
 func TestPreMergeCommitStatus_UnmanagedHook(t *testing.T) {
@@ -358,10 +358,8 @@ func TestPreMergeCommitInstall_CreatesHook(t *testing.T) {
 
 	data, err := os.ReadFile(hookPath)
 	require.NoError(t, err)
-	content := string(data)
-	assert.Contains(t, content, preMergeCommitHookMarker)
-	assert.Contains(t, content, "if ! '/usr/local/bin/mdsmith' fix .; then")
-	assert.Contains(t, content, "git diff --name-only -- '*.md' '*.markdown'")
+	assert.Equal(t, githooks.BuildHookScript("/usr/local/bin/mdsmith"), string(data),
+		"installed hook must match the canonical template")
 }
 
 func TestPreMergeCommitUninstall_RemovesHook(t *testing.T) {

internal/githooks/githooks.go

@@ -937,11 +937,18 @@ func BuildHookScript(exe string) string {
 		"# marked with merge=mdsmith in .gitattributes.\n" +
 		"set -e\n" +
 		"cd \"$(git rev-parse --show-toplevel)\"\n" +
-		"if ! " + shellQuote(exe) + " fix .; then\n" +
-		"  status=$?\n" +
-		"  if [ \"$status\" -ne 1 ]; then\n" +
-		"    exit \"$status\"\n" +
-		"  fi\n" +
+		"# `set +e` around the fix invocation so we can capture its\n" +
+		"# raw exit code. `if ! cmd; then status=$?; ...` looks\n" +
+		"# tempting, but POSIX `! cmd` returns the logical NOT of\n" +
+		"# cmd's exit status, so `$?` immediately after is 0 when\n" +
+		"# cmd exited 1 — and the `[ \"$status\" -ne 1 ]` guard\n" +
+		"# would then exit before the staging loop ever runs.\n" +
+		"set +e\n" +
+		shellQuote(exe) + " fix .\n" +
+		"status=$?\n" +
+		"set -e\n" +
+		"if [ \"$status\" -ne 0 ] && [ \"$status\" -ne 1 ]; then\n" +
+		"  exit \"$status\"\n" +
 		"fi\n" +
 		"git diff --name-only -- '*.md' '*.markdown' | " +
 		"while IFS= read -r f; do\n" +
@@ -966,8 +973,10 @@ func BuildHookScript(exe string) string {
 func HookMatchesCanonical(hook string) bool {
 	required := []string{
 		`cd "$(git rev-parse --show-toplevel)"`,
-		"fix .; then",
-		`if [ "$status" -ne 1 ]; then`,
+		"set +e",
+		" fix .",
+		"status=$?",
+		`if [ "$status" -ne 0 ] && [ "$status" -ne 1 ]; then`,
 		"git diff --name-only -- '*.md' '*.markdown' |",
 		`while IFS= read -r f; do`,
 	}