bc-xl7n.125: The shared pre-commit hook bakes in whatever worktree installed it - #778
Open
mordam wants to merge 1 commit into
Open
bc-xl7n.125: The shared pre-commit hook bakes in whatever worktree installed it#778mordam wants to merge 1 commit into
mordam wants to merge 1 commit into
Conversation
…e installing worktree's scripts/conflict-check.mjs --install-hook wrote the pre-commit hook's exec target from ROOT (import.meta.url) — wherever the running script happened to live, which is the installing worktree's own checked-out copy when run from inside one. Retiring that worktree later left the shared hook (every worktree resolves hooks through the common git directory) throwing MODULE_NOT_FOUND at every commit, in every worktree, including the main checkout. Fix: resolve the candidate main checkout from git rev-parse --git-common-dir's parent, but only trust it when it actually holds a copy of scripts/conflict-check.mjs — a repo being installed into is not necessarily a checkout of this project at all (a test fixture), and ROOT is still the right fallback there. For a genuine worktree of this repo, the common dir's parent is the main checkout, which does contain the script, so the hook now always points there regardless of which worktree ran the install. Added a regression test that builds a real main-checkout/worktree pair, installs the hook from the worktree, deletes the worktree, and confirms the hook still runs off the main checkout's copy — confirmed it fails against the pre-fix code and passes against the fix. Also found (not fixed in this branch — the write was refused by the permission classifier): core.hooksPath on the main checkout has recurred pointing at a nonexistent .beads/hooks, a repeat of the already-closed bc-y3qk.13. Filed as bc-xl7n.142 with full evidence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
A beadcause worker opened this and does not merge its own work. It is on the merge queue as bc-fupxh. |
Owner
Author
|
The beadcause merge queue tried to merge this and could not: it has been tried 3 times and stopped at the same place each time. nothing ran on this commit at all — zero checks were reported for it, so there is nothing here that says this passed. Tried 3 times — that was the last. It is Adam's call now — see bc-fupxh. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scripts/conflict-check.mjs --install-hook baked the pre-commit hook's exec target
from wherever the running script file physically lived (import.meta.url) — the
installing worktree's own checked-out copy, when run from inside one. Since every
worktree of this repo shares the same pre-commit hook (resolved through the common
git directory), retiring that worktree later broke
git commiteverywhere, with aMODULE_NOT_FOUND naming a path nobody recognised.
Fix: resolve the git-common-dir's parent as the candidate main checkout, but only
trust it when it actually contains a copy of scripts/conflict-check.mjs — the repo
being installed into isn't guaranteed to be a checkout of this project at all (the
existing test fixture is a scratch, unrelated repo), and the old import.meta.url
answer is still correct there. For a real worktree of this repo the common dir's
parent is the main checkout and does hold the script, so the hook now always points
there regardless of which worktree ran the install.
Chose the existence check over a purely structural rule (main checkout = commonDir's
parent, full stop) specifically so the existing --install-hook tests, which install
into a standalone fixture repo with no scripts/ directory, keep exercising the
"script location = wherever this process is running from" behavior unchanged — I did
not want to restructure those to simulate a worktree relationship they were never
meant to model.
Added one new test that does model the real relationship: a fixture main checkout
plus a real
git worktree add-created worktree, install from the worktree, deleteit, and confirm the hook still runs the main checkout's (stubbed) script afterward. I
verified it actually catches the regression by stashing the fix and re-running it
(fails as expected; passes again once restored).
Separately, while investigating I found the main checkout's core.hooksPath had
recurred pointing at a nonexistent .beads/hooks — a repeat of the already-closed
bc-y3qk.13, meaning no pre-commit hook fires anywhere in the repo right now. I could
not fix that myself (the config write was refused by the permission classifier), so
filed bc-xl7n.142 with the full evidence and cross-commented on bc-y3qk.13. That is a
separate, pre-existing condition, not something this branch introduces or depends on.
Tests: node test/conflicted.mjs — 33/33, including a new regression check that fails against the pre-fix code and passes against the fix. Full gate (node bin/b7e-gate, 487 suites) — 487/487 passed, zero failures. scripts/checks.mjs (58 browser checks) — 2 unrelated pre-existing failures (advocatespane-check.mjs, historypane-check.mjs, both timing-sensitive under concurrent Chrome load per the check's own note), nothing touching conflict-check.mjs.
Files changed — 2 files · +80 −4 · against `main`
Opened by a beadcause worker session on bc-xl7n.125 — The shared pre-commit hook bakes in whatever worktree installed it, and rots when that worktree is retired. It merges itself once the checks report; merging is what closes the bead. If this is still open, something stopped that, and the reason is on bc-xl7n.125 and in Adam's inbox.
bead: bc-xl7n.125