Let an unrunnable COMMITLORE_BIN fall through instead of killing the commit - #429
Merged
Conversation
…commit The stub's `COMMITLORE_BIN` branch execs the file itself, so the target has to be executable in its own right. `dist/cli.js` carries a `#!/usr/bin/env node` shebang and mode 600 -- it is not. A failed exec terminates the shell non-zero, and the hook then blocks whatever git operation it sits next to: a commit for `commit-msg`, a push for `pre-push`. That contradicts what the branch already says about itself. The comment above it promises that a value which does not resolve falls through to the remaining resolution steps, and that is true of one failing the `.mjs`/`.js` allowlist. It was never true of one that passed the allowlist and could not be run. An `-x` test makes the promise hold. The recorded-path branch never had this shape: it resolves an interpreter and checks that first, which is why an ordinary `hooks install` was never affected and only the environment variable was. Found while writing the pre-push test for #422, by pointing the variable at `dist/cli.js` -- the obvious value for a test harness, and the documented purpose of the variable. Limit: the allowlist accepts `.mjs`/`.js` paths, which are exactly the files most likely to carry a shebang and no execute bit Ruled-out: running the value through node instead of exec'ing it | this branch has no recorded interpreter to use, and inventing one would give an environment variable more trust than the recorded path gets, not less Ruled-out: reporting the unrunnable value and exiting non-zero | a hook that cannot find its checker has nothing to reject, which is why the unresolved ending already exits 0 Warn: changing the stub makes `hooks status` report existing installs as outdated until they are reinstalled Blast: module Undo: easy Certainty: firm Verified: three behavioural cases drive real `git commit` runs and all three go red without the guard, one of them with `Permission denied` from the hook; a fourth pins that the branch is still reachable and that every exec of the variable is preceded by its check Unverified: whether an executable value is still used, end to end -- the bundle resolves its own package root and reads `spec/schema/record.schema.json` beneath it, so a copy outside a complete installation fails for an unrelated reason and the case is pinned on the stub text instead Provenance: authored Record-Id: r-binx428
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (94)
Ruled out (218)
Warnings (53)
Truncated: 47 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
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.
Closes #428.
The defect
The stub's
COMMITLORE_BINbranchexecs the file itself, so the target has to be executable in its own right.dist/cli.jscarries a#!/usr/bin/env nodeshebang and mode-rw-------— it is not.A failed
execterminates the shell non-zero, and the hook then blocks the git operation it sits next to:commit-msgblocks the commit.pre-push(#416) blocks the push.Why it contradicts the stub's own text
The comment above that branch promises:
True of a value failing the
.mjs/.jsallowlist. Never true of one that passed the allowlist and could not be run — that one does not fall through, it kills the shell.The recorded-path branch never had this shape. It resolves an interpreter and checks it first (
[ -x "$recorded_node" ], thenexec "$recorded_node" "$recorded"), which is why an ordinaryhooks installwas unaffected and only the environment variable was fragile.Fix
An
-xtest, so the promise holds.How it was found
Writing
test/pre-push-hook.test.tsfor #422, by pointing the variable atdist/cli.js— the obvious value for a test harness, and the variable's documented purpose ("so a checkout can point the hook at a specific build (a test harness, a monorepo's local bin)"). Every push exited 1.Tests
Three drive real
git commitinvocations, because the defect is in shell text only git runs and is invisible from the TypeScript that generates it. All three go red without the guard:The second matters as much as the first: falling through must still reach a working gate, or the fix would have quietly disabled validation for anyone with the variable set.
The fourth is a stub-text assertion rather than a process, and the commit says why: an executable copy of the bundle cannot be made in a test, because it resolves its own package root and reads
spec/schema/record.schema.jsonbeneath it, so a copy outside a complete installation fails for an unrelated reason. It is recorded asUnverified:rather than claimed.Note
This changes the stub, so
hooks statusreports existing installs asoutdateduntil reinstalled. That is the designed consequence of a stub change.Verification
test/dogfood.test.tsre-run after committing: 9 passed.devafter M5: the surviving rows, deviations 3 and 4, and an appendix that records what was seen #425 and Propose unattended capture, and what stops it minting instructions #427.