Record an entry point only when it exists, so the prescribed fix repairs - #312
Merged
Conversation
Closes #296. doctor reported a commit-msg hook failure and prescribed commitlore hooks install. Running it changed nothing, and --force behaved identically. The report attributed that to a short-circuit on "already installed (unchanged)", which is not what happens: recordBinPath runs on every install. The cause is one line further in. It recorded resolve(process.argv[1]) without checking the result exists. When the CLI is invoked by bare name and argv[1] is the string as typed, resolve produces <cwd>/commitlore -- exactly the value in the report, a path that has never existed in that repository. Re-running records the same wrong value, which is why the prescribed remedy looked inert. The hook then reads a present-but-dead commitlore.bin, stops there rather than falling through, and reports a failure whose named fix cannot clear it. Resolution is now three ordered steps. A path with a separator is resolved and must exist as a file. A bare name is looked up on PATH the way the shell that ran it did. If neither yields an existing file, nothing is recorded -- which is strictly better than recording a fiction, because the stub still has three other ways to resolve while a stale value stops it at the first. The stub is untouched. It carries #71's install-root containment, and the report's third suggestion -- falling through to commitlore.node -- would change that logic and would only help future installs, since the stub is a file already written in every affected repository. Record-Id: r-hooks296 Ruled-out: Making hooks install re-validate only when the hook file is unchanged | recordBinPath already runs every time, so the short-circuit was never the cause and gating on it would have left the wrong value being rewritten Ruled-out: Falling through to commitlore.node in the stub | it changes the file that carries install-root containment, and a stub already written in an affected repository is not re-read, so it would repair nobody who has the problem today Ruled-out: Having doctor --fix repoint the config itself | once the prescribed command works, the prescription is true; a second repair path would leave two places able to write the same key Ruled-out: Recording resolve(argv[1]) with a warning when it does not exist | the hook cannot act on a warning it never sees, and a recorded dead path is what stops resolution at the first step Certainty: firm Blast: module Undo: easy Warn: nothing is recorded when the entry cannot be resolved -- the stub then uses COMMITLORE_BIN, PATH or a node_modules walk, which is the intended fallback rather than a failure Verified: four unit tests driving the resolution directly -- an existing absolute path recorded, a bare name with an empty PATH recorded as nothing, a bare name found on PATH recorded at its PATH location, a directory and a nonexistent relative path both refused; 137 tests across the hooks, hook-target, init and doctor suites; a repository seeded with the report's stale value had it replaced by an existing path after one install; full suite 70 files, 1802 passed, 1 skipped; tsc exit 0; dist rebuilt and committed Unverified: the reported argv[1] value could not be reproduced here, because it needs a compiled binary that ADR-0026 removed from the product -- the fix addresses the cause class rather than being replayed against a SEA
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (56)
Ruled out (116)
Warnings (42)
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 #296.
The cause was one line past where the report looked
The report attributed the inert remedy to a short-circuit on
already installed (unchanged). That is not what happens —recordBinPathruns on every install.The cause is that it recorded
resolve(process.argv[1])without checking the result exists. When the CLI is invoked by bare name andargv[1]is the string as typed,resolveproduces<cwd>/commitlore— exactly the reported value, "a path that has never existed in this repository". Re-running records the same wrong value, so following the printed instruction changes nothing. The hook then reads a present-but-deadcommitlore.bin, stops there, and reports a failure whose named fix cannot clear it.The fix
Resolution is three ordered steps:
PATH, the way the shell that ran it didRecording nothing is strictly better than recording a fiction: the stub still has three other ways to resolve, while a stale value stops it at the first.
The stub is untouched
It carries #71's install-root containment. The report's third suggestion — falling through to
commitlore.node— would change that logic and would help nobody who has the problem today, because the stub is a file already written in every affected repository.Verification
PATH→ nothing · bare name onPATH→ itsPATHlocation · directory and nonexistent relative path both refusedtsc --noEmit0 ·dist/rebuilt and committedNot reproduced: the reported
argv[1]value itself needs a compiled binary, which ADR-0026 removed from the product. The fix addresses the cause class — an unvalidated entry — rather than being replayed against a SEA. Two of my own fixtures initially failed because this machine hascommitloreonPATH, which is itself evidence thePATHstep resolves to the value the reporter set by hand as their workaround.