Fix npm publish surface (#93), Shape verdict divergence (#90), same-message Record-Id collisions (#92) - #97
Merged
Merged
Conversation
ADR-0011 decided distribution is git clone, not a registry, but package.json did not enforce that: no `private: true` meant `npm publish` would still succeed, and `bin` pointed `commitlore` at `dist/cli.js` -- the unbundled tsc output, which crashes ERR_MODULE_NOT_FOUND: commander on a fresh clone with no node_modules, reproduced before changing anything. `bin` served only an install path ADR-0011 already replaced, so it is removed rather than repointed; `dist/cli.js` itself stays, since CI and scripts/commitlore-run.sh both still run it directly when node_modules already sits next to it, unrelated to what `bin` does. The five `dependencies` moved to `devDependencies`: rebuilt and ran the bundle with node_modules deleted (--version, validate) to confirm esbuild inlines all five, so they were describing a runtime that does not exist. `files` is untouched -- the issue's own "After #39" note says the bin/files/dependencies cleanup is one job once the static binary lands, not two. Verified the rebuild is a no-op (git diff --exit-code -- dist/), so this touches no shipped behavior, only what a registry sees. Closes #93 Ruled-out: repointing bin at dist/commitlore.mjs instead of removing it | that entry exists only to serve a package-manager install ADR-0011 already replaced with a git clone; repointing it keeps maintaining a path to an install mode that will never happen Ruled-out: also removing files now | the issue's own "After #39" note groups bin/files/dependencies as one cleanup once the static binary lands; files alone is inert without a registry rather than actively broken, so doing it now means doing it twice Limit: package.json remains a development artifact (build, typecheck, dependency floor) -- it is not read as a distribution manifest by anything in this repository Blast: local Undo: easy Certainty: firm Verified: dist/ rebuild is byte-identical to HEAD (git diff --exit-code -- dist/), confirming package.json has no effect on the shipped bundle Verified: fresh copy with node_modules deleted runs `commitlore --version` and reports private:true, bin:undefined, dependencies:undefined Verified: scripts/check-engines.mjs, spec/verify.sh, scripts/check-readme-numbers.mjs all pass unchanged Evidence: package.json Evidence: docs/adr/ADR-0011-plugin-first-distribution.md Record-Id: r-fix93pkg Provenance: authored CommitLore-Version: 2.0.0
…ssage-file agree Reproduced against gitseed before changing anything: the same merge commit message read `shape ok` through `--commit` and `shape failed` (unknown-key on the PR-title paragraph) through `--message-file` fed the extracted message text. SPEC S6.1 defines Shape as needing "the message alone" and running "anywhere, including stdin" -- the two paths disagreeing on the same input is exactly what that clause exists to rule out. What I found: the paths had diverged, not the check class. Bug-issue-76's merge-title exclusion (nonTrailerParagraph) gated on source.merge, populated by readCommitSource from `git log --format=%P` parent-counting -- repository information a --message-file/stdin caller never has. --commit and --range populated it; --message-file and stdin silently left it undefined, so the identical excuse applied to one path and not the other for byte-identical text. Shape itself does not need to know a commit has multiple parents: the signal `nonTrailerParagraph` actually wants -- "this paragraph is platform-generated, not an attempted record" -- is recoverable from the message's own first line, since that is exactly what git's merge templates and GitHub's PR-merge button write there on their own. Reconciled by replacing the parent-count signal with a message-only one: looksLikeMergeTitle matches the first line against those templates (`Merge pull request #N from …`, `Merge branch '…'`, `Merge remote-tracking branch '…'`, `Merge tag '…'`), so every input mode computes the same excuse the same way. readCommitSource no longer fetches %P at all -- MessageSource does not carry a merge flag anymore. Closes #90 Ruled-out: keeping the parent-count signal and also computing it for --message-file via .git/MERGE_HEAD | that only exists while a merge is in progress (the commit-msg hook's own scenario), not when re-validating an already-made merge commit's extracted message -- which is the issue's own reproduction and stays unreconciled under that approach Warn: a message whose first line happens to start with one of these templates on a non-merge, non-CommitLore commit would also be excused if its trailer-shaped final paragraph were entirely unknown-key -- the same shape of risk bug-issue-76's original parent-count gate carried for genuine merge commits, now traded for one that is at least computable the same way everywhere Blast: module Undo: easy Certainty: firm Verified: 57/57 in test/validate.test.ts, including the new bug-issue-90 test asserting --commit and --message-file report the identical checks[0] and stderr for the same merge commit's message Verified: gitseed reproduction now reports `shape ok · references ok` through both --commit and --message-file Verified: 315/315 across test/stale.test.ts, test/query.test.ts, test/trailers.test.ts (unaffected) Evidence: test/validate.test.ts Evidence: spec/SPEC.md Record-Id: r-fix90shape Provenance: authored CommitLore-Version: 2.0.0
…validate too Continuing bug-issue-89's finding, which fixed `parse` and flagged the rest as open: core/stale.ts's findIdCollisions only fired when a notes-sourced record disagreed with a commit's own content -- a group with no notes record in it, which is what two same-message commit blocks are, never reached it. `context --json` showed one clean record and `validate` returned 0 for a message parse already refuses, so three commands disagreed about the same message. findIdCollisions now also flags a Record-Id claimed by two commit-sourced records that share a sha -- declared by the same message, not a later commit re-declaring the id over time (a legitimate SPEC S5 lifecycle update, which stays unflagged: it never shares a sha). A note cleanly mirroring its own commit always shares that commit's sha too, so the new check only counts commit-sourced entries -- bug-issue-74's divergent-note case stays gated on payload drift exactly as before. validate's checkReferences built its collision-check array by pairing repositoryRecords -- which already carries the single last-paragraph record collectRecords derives for the commit being checked -- with a per-block candidate, so checking the message's own last block paired that block with a second copy of itself and an earlier block was never placed beside a different one at all. Rebuilt to pass the message's own blocks once each (ownRecords), plus any notes record already found for that sha so bug-issue-74 stays covered, alongside prior. Also fixed, without which the fix above could not be observed through `commitlore context <path>` -- the shape a user actually runs: core/query.ts's collectRows deduplicated rows fetched across aliases by sha+source+seq alone. seq restarts at 0 within every record block (SPEC S2.4), so a commit with two blocks has a seq:1 row in each -- collectRows was silently dropping the second block's rows as "already seen," which is what let a scoped-path query show one clean record instead of a blocked collision. Fixed by keying on block too, matching the trailers table's own unique index (commit_sha, source, block, seq). Closes #92 Ruled-out: flagging every duplicate sha in a group regardless of payload, matching parse's labelRecordBlocks exactly | validate's own collision-check array pairs repositoryRecords (already carrying the message's last block once) with a per-block candidate; an unconditional duplicate-sha rule would flag the message's own single last block against its own re-derived copy, a false positive fixed instead by building ownRecords once per message rather than loosening the predicate past correctness Ruled-out: reverting core/query.ts's collectRows dedup key back to sha+source+seq once findIdCollisions was fixed | that key was already wrong on its own terms (it does not match the trailers table's unique index), and leaving it in place would keep silently dropping a commit's second record block from any scoped-path context query, collision or not Limit: cross-references between two blocks declared by the same commit (a Follows:/Supersedes: naming a sibling block's id) are still reported as dangling rather than resolved against the sibling -- unchanged from before this fix, and called out in validate.ts's own comment as future work Blast: module Undo: easy Certainty: firm Verified: 379/379 across test/validate.test.ts, test/stale.test.ts, test/query.test.ts, test/trailers.test.ts, including new tests for the same-sha collision (core/stale.ts unit, validate --commit, and commitlore context --json at a scoped path) Verified: commitlore context --json <path> on a synthetic two-block same-Record-Id commit now reports identityCollision:true and trust:blocked, matching the existing divergent-note case; before this commit it reported one clean, unflagged record Evidence: test/stale.test.ts Evidence: test/validate.test.ts Evidence: test/query.test.ts Record-Id: r-fix92dupid Follows: r-fix90shape Provenance: authored CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 3 commits in Active constraints for the paths this PR touchesLimits (32)
Ruled out (86)
Warnings (47)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
MongLong0214
added a commit
that referenced
this pull request
Jul 28, 2026
Resolve conflict in dist/commitlore.mjs (generated) by rebuilding with npm run build && npm run bundle. Incoming changes from dev (PR #97): - npm surface removal - Shape verdict fix - Record-Id collision fix Merge complete.
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.
Summary
Three issues, addressed in three commits on top of the current
dev(which already includesfeat-issue-39's single-executable-binary work):package.jsonno longer describes a package this project can publish.contextandvalidatenow refuse two blocks in one message sharing aRecord-Id, the same wayparsealready does.#93 — npm surface
npm publishwould have succeeded (no"private": true), andbinpointedcommitloreatdist/cli.js, the unbundledtscoutput that crashesERR_MODULE_NOT_FOUND: commanderon a fresh clone with nonode_modules."private": true.bin(not repointed — it served an install path ADR-0011 already replaced with a git clone).dist/cli.jsitself stays; CI andscripts/commitlore-run.shstill run it directly whennode_modulesis already present.dependenciesintodevDependencies, verified by rebuilding and running the bundle withnode_modulesdeleted.filesis untouched — deliberately out of scope even thoughfeat-issue-39has since landed; folding it in would be scope creep past what The package is still publishable to npm, and bin points at the entry that crashes without node_modules #93's audit found.Fresh-clone verification (after this PR,
node_modulesdeleted):#90 — Shape verdict depended on repository presence
Reproduced against gitseed before changing anything:
What I found: the paths had diverged, not the check class.
#76's merge-title exclusion (nonTrailerParagraphinvalidate.ts) gated onsource.merge, computed fromgit log --format=%Pparent-counting — repository information a--message-file/stdin caller never has.--commit/--rangepopulated it;--message-file/stdin silently left itundefined. Shape itself does not need to know a commit has multiple parents — the actual signal wanted ("this paragraph is platform-generated, not an attempted record") is recoverable from the message's own first line, since that's exactly whatgit merge's templates and GitHub's PR-merge button write there.Fix:
looksLikeMergeTitlematches the first line against those templates (Merge pull request #N from …,Merge branch '…',Merge remote-tracking branch '…',Merge tag '…') — pure text, available identically in every input mode.readCommitSourceno longer fetches%Pat all.After:
Both paths now agree, exit 0.
#92 — two blocks, one
Record-Id, silently mergedcore/stale.ts'sfindIdCollisionsonly fired when a notes-sourced record disagreed with a commit's own content — a group with nonotesrecord in it, which is what two same-message commit blocks are, never reached it.parsealready detects this locally (#89);contextandvalidatedisagreed with it about the same message.Before (synthetic repro — a message with two blocks sharing
Record-Id: r-dupdup):Fix:
findIdCollisionsnow also flags aRecord-Idclaimed by two commit-sourced records sharing asha(declared by the same message) — a legitimate later-commit re-declaration (SPEC §5 lifecycle update) never shares asha, so that case stays unflagged, and a clean note mirroring its own commit (which does share asha) stays gated on payload drift exactly as before.validate'scheckReferenceswas pairingrepositoryRecords(which already carries the message's own last block once) with a per-blockcandidate, so checking the message's last block paired it with a second copy of itself instead of ever placing two different blocks side by side. Rebuilt to pass the message's own blocks once each (plus any notes record for that commit, so#74's divergent-note case stays covered).commitlore context <path>(the shape a user actually runs):core/query.ts'scollectRowsdeduplicated rows across aliases bysha+source+seqalone.seqrestarts at 0 within every block (SPEC §2.4), so a commit with two blocks has aseq: 1row in each — the second block's rows were silently read as "already seen" and dropped. Fixed by keying onblocktoo, matching thetrailerstable's own unique index.After:
Test plan
tsc --noEmitcleannpm run builddeterministic (git diff --exit-code -- dist/after rebuild)spec/verify.sh,scripts/check-engines.mjs,scripts/check-readme-numbers.mjsall passnode_modules) runs--version,validate,doctornode dist/commitlore.mjs validate --range "origin/dev..HEAD"→shape ok · references ok, every commit carries CommitLore trailersdevtip (which already mergedfeat-issue-39); resolved the expectedpackage.json/CHANGELOG.mdconflict by keeping both sides (keptfeat-issue-39'sprivate: true,build:binaryscript,postjectdevDependency, and this PR'sbinremoval +dependencies→devDependenciesmove)Not merged to
dev— opening for review.