Fix pre-merge-commit hook to stage fixes when diagnostics remain by jeduden · Pull Request #232 · jeduden/mdsmith

jeduden · 2026-05-03T18:50:36Z

Summary

Fixes a bug in the pre-merge-commit hook where files modified by mdsmith fix were not being staged when the command exited with code 1 (indicating unfixed diagnostics remain). This caused merge commits to miss catalog regenerations and other auto-fixes on the merge queue.

Root Cause

The original hook used if ! mdsmith fix .; then to conditionally handle exit code 1. However, the POSIX ! operator returns the logical NOT of the command's exit status, so when mdsmith fix exits 1, the ! makes it appear as 0 to the subsequent $? check. This caused the exit code guard to fail and the script to exit before reaching the staging loop.

Key Changes

Hook script generation: Changed from if ! cmd; then status=$? pattern to explicit set +e / set -e wrapping to capture the raw exit code before any shell operators can modify it
Exit code handling: Updated the condition from [ "$status" -ne 1 ] to [ "$status" -ne 0 ] && [ "$status" -ne 1 ] to properly allow both success (0) and unfixed diagnostics (1) to proceed to the staging loop
Added regression test: TestBuildHookScript_StagesFixesWhenUnfixedRemain reproduces the merge-queue scenario and verifies that modified files are staged even when mdsmith fix exits 1
Updated test assertions: Modified existing tests to match the new hook script structure and verify correct exit code handling

Implementation Details

The fix uses POSIX shell's set +e to temporarily disable the set -e error exit behavior, allowing the script to capture mdsmith fix's raw exit status in a variable before re-enabling strict error handling. This ensures the staging loop always runs for exit codes 0 and 1, while still propagating unexpected error codes.

https://claude.ai/code/session_01C4i4wEMhsfuoTDrq54QvaU

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

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

codecov · 2026-05-03T18:52:09Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.59%. Comparing base (81cc3f5) to head (6a810f7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #232   +/-   ##
=======================================
  Coverage   94.58%   94.59%           
=======================================
  Files         136      136           
  Lines       15839    15848    +9     
=======================================
+ Hits        14982    14991    +9     
  Misses        524      524           
  Partials      333      333

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR fixes the canonical pre-merge-commit hook template so that files modified by mdsmith fix . are still staged when mdsmith fix exits with status 1 (unfixed diagnostics remain), preventing merge commits from missing auto-fixes (e.g., regenerated catalogs).

Changes:

Update BuildHookScript to capture mdsmith fix’s raw exit status using set +e / set -e, and only fail the hook for exit codes other than 0 or 1.
Update drift-detection matching (HookMatchesCanonical) and all affected assertions to reflect the new hook structure.
Add a Unix regression test that initializes a real git repo and verifies the hook stages modified files even when mdsmith fix exits 1.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/githooks/githooks.go	Fix hook exit-code handling by capturing the raw `mdsmith fix` status and permitting `0`/`1` before staging.
internal/githooks/githooks_test.go	Update hook template assertions to match the new `set +e` / `status=$?` structure.
internal/githooks/githooks_unix_test.go	Add regression coverage ensuring staging occurs even when `mdsmith fix` returns exit code `1`.
cmd/mdsmith/premergecommit_test.go	Update install tests to assert the new canonical hook structure/guard.
cmd/mdsmith/mergedriver_test.go	Update hook content assertions to match the new exit-code guard behavior.
cmd/mdsmith/e2e_test.go	Update E2E expectations for the installed hook template.

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

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

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

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

…nd 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

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

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

…r-cheat First hunt run after the live-path polyglot proof. Primary set 0 proven (both out-of-reach as pre-registered). Novelty set: jeduden/mdsmith#232 (Go) reached the engine and PROVED assertion-strip (5 triggers, replayed) end-to-end through swarm audit --pr -- the polyglot pipeline works on wild Go content. But that proof is a FALSE POSITIVE for cheat: the same PR added a golden-file test suite (internal/githooks), so the weakened e2e assertion is a legitimate coverage-moved refactor, not concealment. The engine's controls are sound; 'pattern present => cheat' is the unsound step. Root-caused, recorded as a precision limitation (unbounded to fix), treated as halt-worthy: no wild-cheat-caught claim. Designated clean controls/fixtures all held; promotions/corroborated-gate unchanged. Funnel confirms the pre-registered reach matrix; entry-gate + closure fixes held (no entry died at the Hunt 5/6 walls). Spend 0.00 (deterministic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Stage 0 items 2 and 4 of the capability run. The test-tamper gate's one known false-positive class (jeduden/mdsmith#232: an assertion weakened because its coverage moved to a new golden test) is now neutralized in-proof and surfaced honestly. - test-restoration.ts: `coverageRelocated` refuter (Step 6d). When an otherwise-proven restoration's PR adds replacement coverage (a net-new test file or a golden/testdata fixture) inside a production directory it also changed, downgrade to not-proven:coverage-relocated. Pure diff signal, near-zero cost, conservative (only proven -> not-proven), tied to "the same code" via directory proximity so a pure assertion-deletion tamper still proves. - attestation: new `disputed` outcome (fired-then-disputed). A coverage-relocated record maps to disputed, not abstain and never clean; the roll-up counts it and the render names it human-review-required. Consumption contract updated separately. - twin measurement (coverage-relocation:measure): 6/6 through the real engine on go + pytest. Planted tampers still prove 2/2, mdsmith-shaped relocations abstain 2/2 (all three controls green), honest cleans refute 2/2. Polyglot-restoration regression still 4/4. Suite 2270 passing; refuter verified firing on the real jeduden diff. LOC 47358 -> 47484 (+126, new refuter + disputed projection). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Stage 0 items 1 and 3. The diagnosed coverage-relocation false positive is pinned so it can never regress, and the block machinery becomes demotable on measured FP evidence. - FP registry (benchmarks/real-corpus/fp-registry/): one entry per diagnosed gate FP beside its committed PR diff. jeduden/mdsmith#232 is entry one. `npm run fp-registry:check` (wired into ci.yml) replays each neutralized entry's refuter over its diff and every finding file; if it no longer fires the gate would block again, so CI goes red. Teeth verified: a deliberate-firing temp entry exits 1, the committed registry exits 0. - block-eligibility: computeBlockEligibility folds still-live registry FPs into each trigger's denominator and auto-demotes a self-certifying trigger to advisory when its Wilson-95 lower bound drops below the 0.90 bar. Zero-FP triggers are never demoted. jeduden is neutralized, so it contributes no live firing and block-eligibility.json regenerates byte-identical (test-tamper-proven stays eligible, block-eligible=8). - attestation consumption contract (docs/attestation.md): the disputed state means human-review-required, never clean. The bar (0.90 / 5-TP) is unchanged; the self-certifying tier became demotable at that same bar. Suite 2277 passing. LOC 47484 -> 47516. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Clean full-population run on macOS arm64, superseding a partial Linux-container run that audited 11 entries and recorded 0 proven before dying mid-batch. Finishing that run here would have mixed two execution environments inside one pre-registered population; a single-environment run is the defensible measurement. The partial records were not present in this tree, so nothing was resumed and all 19 viable entries ran from scratch. Population is the 29 amendment-2 entries with the B2 viability refresh as the deciding screen: 19 EG-viable, 10 screen-rejected. Three columns per amendment 5. Deterministic arm: 19 audited, 13 provisioned, 6 controls-executable, 0 proven. Judge arm: 12 provisioned, 5 controls-executable, 0 proven, 41 billable calls at USD 0.1845 against a USD 10 ledger-enforced ceiling. The v4 addition runs in its own out-dir and abstains. Zero proven for the third time, now over a population 2.7 times larger. The binding constraint moved: passes 1 and 2 were provisioning-bound, this pass is execution-bound, with 7 of 13 provisioned entries never running a single control. Tripwire: 6 of 7 previously-viable entries reproduce their pass-2 bucket exactly. yorickdewid/flight-planner#149 differs and is diagnosed as an environment difference rather than a defect: the repo declares no packageManager, so corepack resolves its own floating default pnpm (11.5.3 here), which exits 1 on ERR_PNPM_IGNORED_BUILDS after the install content itself succeeded. Not arm64-specific, toolchain-version-specific. The tripwire is not considered passed until this is re-confirmed in the Linux CI. The 18 not-proven:execution-error records on jeduden/mdsmith#232 were root-caused from these artifacts: the sandbox resolved every runner binary against the pinned Node bin dir, so a Go proof tried to spawn <node-bin>/go and died at ENOENT before any control ran. Environment-independent; the Linux nvm bin dir has no go either. This pass reports the pre-fix behavior. Also recorded: the judge demoted all 18 assertion-strip findings on that entry from block to warn, so its gate passed and no proof was attempted at all. The disagreement between the structural detector and the judge is reported, not resolved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HE7k3NBMaxzDMq6SLapVpq

…o there The restoration control could not run a Go or Python suite at all, for a reason that had nothing to do with Go or Python. executeTestRun resolved every runner binary through execBin, which joins the name onto SWARM_EG_NODE_BIN. For a Go proof that produced <node-bin>/go, a path that does not exist, so the spawn died with ENOENT before control 2 ran and the record became not-proven:execution-error with all three controls null. Same for python3. This is environment-independent: the Linux nvm bin dir has no go either. Measured consequence in recall pass 3: jeduden/mdsmith#232 drew 18 block-severity assertion-strip findings and produced 18 execution errors with zero controls evaluated, and across the whole pass not one Go control executed on any of the three provisioned Go entries. execBin now pins only the binaries the pinned Node bin dir actually ships, and resolves everything else on the ambient PATH. The ecosystem-specific parts move behind one seam in ecosystem-runner.ts: the runner invocation table, Go package scoping, project-root resolution, and a toolchain preflight. The proof logic above it does not fork per ecosystem; only the run-the-tests step varies. executeTestRun stays the single interface all six restoration engines already call, so they all gain the coverage at once, and it now returns an ecosystem-agnostic outcome: executed-with-pass, executed-with-fail, or not-executed with a classified reason. Two things the seam adds beyond binary resolution: Project-root resolution. Go and Python run at the nearest go.mod or pyproject root at or above the test file, with targets re-based onto it, so a module in a subdirectory of the clone is found rather than missed. Node is untouched by construction: still the workspace root, still workspace-relative paths. A preflight that names its reason. A missing toolchain is reported as toolchain-missing before anything spawns, instead of dying at ENOENT and being recorded as an execution error. An unresolvable workspace, an unsupported runner, and an unresolvable module root are named the same way. No non-execution is unclassified, and a test asserts that invariant directly, because a bare null cannot distinguish "not provable" from "never looked". The three engines that stay on Node runners (no-op-fix needs changed-line coverage, mock-of-hallucination reasons about JS module mocks, dead-branch instruments markers through the Node runner) now share one named constant that records why, instead of each repeating an unexplained runner literal. Fixtures are a real Go module and a real Python package, and the tests assert that the suites actually ran and reported the right result: go printed '--- PASS: TestAddPasses', pytest printed 'test_add_passes PASSED', and the failing variants return the parsed failing identity. One test pins the regression directly by setting SWARM_EG_NODE_BIN to a node-only directory and requiring the Go suite to still execute. Three pre-existing assertions moved from the old reason wording ('spawn-level failure') to the classified reason. The verdict and null-controls invariants they protect are unchanged and still asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HE7k3NBMaxzDMq6SLapVpq

…tation A restoration record has always carried a loud reason for every not-proven verdict, and both consumers dropped it. The ledger entry and the swarm-proof-coverage attestation published the verdict alone, so an execution-error was unreadable: 18 identical records on jeduden/mdsmith#232 said only that something went wrong, and finding out what required re-running the audit with debug logging. The reason now rides into both, and the recall harness keeps it on each per-engine coverage record. Absent on proven verdicts and on records written before the field existed, so readers must tolerate its absence. This is what makes the not-executed classification useful outside the process that produced it: verdict says which class a record fell into, reason says what actually happened, and the difference between "the toolchain was never installed" and "the workspace was broken" is the difference between an environment fix and an engine fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HE7k3NBMaxzDMq6SLapVpq

Re-audited at its recorded pair with the multi-ecosystem executor. The entry moved from unmeasurable to measured: 0 control clauses evaluated before, 54 after (18 records times 3 controls), 13s of wall clock before, 249s after. The measurement says this PR is not a concealed test tamper. 13 of 18 records land on not-proven:re-specified, where the submitted test fails on the base source, so the PR re-specified the asserted behaviour to match an intentional source change. The other 5 reached green on every control and were then contested by the coverage-relocation refuter, because the PR adds replacement fixtures under internal/githooks/testdata/hooks/bad/; those record as disputed, which is not proven and which a cautious policy must read as human-review-required rather than clean. No gate trigger fired. Nothing is proven and no claim is made anywhere, so the four-check false-positive protocol has nothing to run against. Nothing was tuned to make this entry prove. The only change is that the proof engine can now spawn the Go toolchain. The disagreement between the reviewer's complaint phrase and the refuters' conclusion is recorded as an open question and left unimplemented: chasing it means changing a refuter's sensitivity against an entry whose answer is already known, which is the loop the holdout rule exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HE7k3NBMaxzDMq6SLapVpq

Same population, same environment, same detectors as pass 3. The restoration executor is the only change. Deterministic arm: 19 audited, 15 provisioned (was 13), 9 controls-executable (was 6), 0 proven. The recall ceiling falls from 3/6 to 3/9, 50% to 33%. Judge arm: 14 provisioned, 7 controls-executable, 0 proven, 41 billable calls at USD 0.1845 against the USD 10 ceiling. The v4 addition abstains in its own out-dir. Every pass-3 to pass-4 difference is attributed per entry, and 16 of 19 entries are identical in bucket, control count, and finding count: provisioning 2 entries, both Python, interpreter resolved against the declared range (canvas-hyperscribe <3.13,>=3.11 and skyvern <3.14,>=3.11) execution coverage 1 entry, jeduden/mdsmith#232, 0 controls to 54 detector behavior 0 entries, nothing in that layer changed macOS artifacts 0 new, yorickdewid unchanged and still classified as environment-attributable network artifact 1 entry, inmanta gained a finding only because pass 3's deterministic arm lost its PR fetch to EPIPE first The abstention table changed shape rather than shrinking. Pass 3's largest block was 18 not-proven:execution-error, an engine that never looked; pass 4's is 13 not-proven:re-specified, an engine that looked and was refuted. No execution-error records remain. The executor work produced no detection. It produced a measurement where there was not one, which is the only thing a no-detection pass can improve. The binding constraint moved again: 6 provisioned entries still have no proof candidate at all, which is detector reach, not execution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HE7k3NBMaxzDMq6SLapVpq

claude added 2 commits May 3, 2026 18:35

Copilot AI review requested due to automatic review settings May 3, 2026 18:50

Copilot started reviewing on behalf of jeduden May 3, 2026 18:51 View session

Copilot AI reviewed May 3, 2026

View reviewed changes

claude added 2 commits May 3, 2026 18:59

Copilot AI review requested due to automatic review settings May 3, 2026 19:09

Copilot started reviewing on behalf of jeduden May 3, 2026 19:09 View session

Copilot AI reviewed May 3, 2026

View reviewed changes

Comment thread internal/githooks/githooks.go

claude added 2 commits May 3, 2026 19:16

Copilot AI review requested due to automatic review settings May 3, 2026 19:28

Copilot started reviewing on behalf of jeduden May 3, 2026 19:29 View session

Copilot AI reviewed May 3, 2026

View reviewed changes

Comment thread cmd/mdsmith/e2e_test.go

jeduden merged commit 9312543 into main May 3, 2026
11 checks passed

Copilot AI mentioned this pull request May 17, 2026

Fix #232: Add table separator-style: spaced option #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Fix pre-merge-commit hook to stage fixes when diagnostics remain#232
jeduden merged 7 commits into
mainfrom
claude/investigate-actions-failure-QFN9b

jeduden commented May 3, 2026

Uh oh!

codecov Bot commented May 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jeduden commented May 3, 2026

Summary

Root Cause

Key Changes

Implementation Details

Uh oh!

codecov Bot commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 3, 2026 •

edited

Loading