parse recognizes every record block (#89); fix bench-ablation dist/ race, scale mcp.test.ts timeout (#88) - #91
Merged
Merged
Conversation
`parse` and `context` disagreed on how many records a message contains. bug-issue-60 (#86) taught `context`, `validate` and the index to recognize every record block a message carries (SPEC §2.4), but `parse` still called `parseCommitMessage` alone — for a multi-record message it kept answering from the pre-#86 grammar while every other consumer route had moved on. `parse --help` calls itself the command for asking exactly this question. Reproduced before (dist/commitlore.mjs, shipped bundle): $ node dist/commitlore.mjs parse --message-file /tmp/gh.txt Limit: only a test 2 Record-Id: r-ghtest2 $ node dist/commitlore.mjs context --no-index context ... — 2 limits, 0 ruled-out, 0 warnings, 0 other in 2 records ... r-ghtest1 ... only a test 1 r-ghtest2 ... only a test 2 Fix: `core/trailers.ts` gains `labelRecordBlocks`, which wraps `parseRecordBlocks` and marks each block `own` (the message's own last paragraph, SPEC B1) or `earlier` (a block the grammar recovered). `parse` reports every block. The multi-block form is additive, the same way SPEC §2.4 itself only adds to §2.1–2.3: a single-block message's output is byte-for-byte what it always was, in both text and `--json`, verified directly against the previously shipped `dist/commitlore.mjs` across every fixture in `spec/fixtures/` (valid, boundary, invalid) before rebuilding. `--json`'s `trailers` key keeps meaning "the message's own block"; a new `blocks` array appears only when there is more than one. Reproduced after: $ node dist/commitlore.mjs parse --message-file /tmp/gh.txt # record 1/2 — earlier Limit: only a test 1 Record-Id: r-ghtest1 # record 2/2 — own Limit: only a test 2 Record-Id: r-ghtest2 Also checked, as asked: two blocks in one message declaring the same Record-Id. `parse` used to show one and say nothing. Traced whether that path reaches `identityCollision` (the field `context --json` already exposes): it does not, anywhere. `core/stale.ts` `findIdCollisions` only fires when a *notes*-sourced record disagrees with a commit's own content — a group with no `notes` record in it, which is exactly what two same-message commit blocks are, never trips it. Confirmed directly: both `commitlore context --json` and `commitlore validate` accept a message with two blocks declaring one Record-Id and different content today, silently merging them (`context` folds the two `Limit:` values into one record; `validate` exits 0). `parse` now runs its own, local check — independent of `findIdCollisions`, scoped to the one message being parsed — and surfaces the collision: `identityCollision: true` per block in `--json`, a `Record-Id collision` marker in text, and a stderr diagnostic naming the id. Full suite (dev tip, 33dc7aa): 1297 tests, 36 files (1296 passed, 1 skipped) — confirmed by running it myself, not assumed. Limit: `parse` has no git-commit context (no sha, no notes mirror) — its identityCollision check is local to the one message being parsed and cannot detect a Record-Id that collides with something already committed elsewhere in history the way context's fold does. Warn: context and validate still do not catch two commit-sourced blocks in the *current* message sharing one Record-Id unless a notes mirror also disagrees (findIdCollisions requires a notes-sourced record in the group) — parse's check is intentionally independent, not a call into that function, so fixing this in context/validate is separate work. Ruled-out: expanding findIdCollisions/index-db to flag a same-message duplicate id generally | reaches into query.ts's lifecycle fold and every consumer route (context, guard, stale) for a case bug-issue-89 only asked parse to surface; the smaller, requested fix is in parse alone. Verified: byte-identity of single-block parse output (text and --json) against the previously shipped dist/commitlore.mjs across every spec/fixtures/{valid,boundary,invalid} fixture, zero diffs; new parse/labelRecordBlocks tests (test/cli.test.ts, test/trailers.test.ts) green; full suite green; manual reproduction of both VERIFY cases against the rebuilt dist/commitlore.mjs. Blast: module Undo: easy Certainty: firm Provenance: authored Record-Id: r-parsemulti CommitLore-Version: 2.0.0
…ng test builds (#88) `test/bench-ablation.test.ts` — "accepts all six arms" — failed CI on #79 and #87 and failed locally under concurrent load, always passing in isolation (20/20). Diagnosed before changing anything, per bug-issue-88: Evidence for (b), not (a). `bench/runner.ts` hashes the whole `dist/` tree at startup (`digestDistTree`) and re-hashes it before every one of the six arms (`bench/hooks-settings.ts` `writeArmSettings`), refusing an arm when the two disagree — a real, useful check: its own comment records that it is what caught `dist/core/guard.js` changing mid-run under a previous measurement. `dist/` is one directory shared by every vitest worker, though, and four *other* test files (`cli.test.ts`, `mcp.test.ts`, `action-lint.test.ts`, `action-preserve.test.ts`) each rebuild it via their own `tsc -p tsconfig.json` in a `beforeAll`. Reproduced directly: invoking `bench/runner.ts` in a loop while a separate process repeatedly rebuilds `dist/` fails about 1 run in 5 (4/20 and 1/15 across two independent runs) with exactly the reported symptom: Command failed: node --experimental-strip-types bench/runner.ts ... ... "stopped_by":"error", ... "error":"dist/ changed after the benchmark matrix started: expected sha256 b29dbd..., found e8b24d..." `execFileSync` (what `runRunner` uses) is fully synchronous, so it blocks the event loop for its whole duration — vitest's own per-test timeout timer cannot fire until it returns, and by then there is nothing left to time out. Confirmed independently with a throwaway probe test (`await setTimeout(6000)` fails at vitest's 5000ms default; the same 6s of work behind a synchronous call does not, it just reports its real duration). So this was never a timeout problem — raising one would not have changed anything, and would have hidden a check that is doing exactly its job. Fix: removed the shared mutable state instead of loosening the check that correctly depends on it. `DIST_DIR` (`bench/hooks-settings.ts`) now reads `COMMITLORE_BENCH_DIST_DIR` when set, falling back to the existing `<repo>/dist` — unset everywhere except this one test, so a real benchmark run is unaffected. `bench-ablation.test.ts` snapshots `dist/` into a private, unshared temp copy once per file and points every runner invocation at it. Also routed `ablationShim`'s previously-hardcoded `dist/core/inject.js` reference through the same `DIST_DIR`, which was inconsistent with the rest of the module and would have kept reading the live tree even after this fix. `mcp.test.ts` does not share this cause. It never calls `bench/runner.ts` or the digest check — it is one of the four *racers*, not a victim of the same race. But its JSON-RPC assertions `await` a real round trip over a spawned server's stdio, unlike `bench-ablation`'s synchronous `execFileSync` calls, so they genuinely are subject to vitest's fixed 5000ms default under real concurrency (mechanically confirmed with the same probe: an awaited 6s wait fails at the default, a synchronous one does not). Scaled its budget instead of guessing a bigger number: `vi.setConfig({ testTimeout: 5_000 * Math.max(availableParallelism() - 1, 1) })`, the same worker count vitest's own default pool sizing already uses (`getDefaultThreadsCount`) — 55s on this 12-core box, 3x on a 4-core CI runner, unchanged on a single core. Ran the fixed test 20/20 consecutive clean under the same concurrent-tsc- rebuild load that failed 4/20 before the fix (measured directly against `bench/runner.ts` in a loop, not only through vitest). mcp.test.ts: 43/43 passing across 3 self-concurrent full runs plus 6 tsc rebuild loops and 8 CPU-bound processes; could not force an actual failure on this 12-core dev machine even under that load, consistent with the CI report coming from a more constrained/shared runner rather than a logic bug. Full suite (dev tip, 33dc7aa): 1297 tests, 36 files (1296 passed, 1 skipped) — confirmed by running it myself, not assumed. Limit: the fix insulates bench-ablation.test.ts from the race; it does not remove the underlying design (four test files independently, redundantly rebuilding one shared dist/ in their own beforeAll). A fifth file doing the same thing, or a future check elsewhere that also depends on dist/'s mid-run stability, can still race the same way. Warn: COMMITLORE_BENCH_DIST_DIR is read once at module load (`bench/hooks-settings.ts` DIST_DIR is a top-level const) — set it before importing the module, not after. Ruled-out: consolidating the four files' redundant tsc rebuilds into one vitest globalSetup | would remove the race for every file at once and is the more complete root-cause fix, but touches five files' bootstrap plus global config, and mcp.test.ts's beforeAll already tolerates unrelated in-flight tsc errors elsewhere in src/ (OWNED_SOURCES filtering) in a way a single shared build step cannot preserve without its own redesign — bigger blast radius than this issue asked for. Ruled-out: raising bench-ablation's timeout, globally or per-test | proven not to be a timeout problem at all: execFileSync blocks synchronously, so vitest's timeout mechanism cannot even engage against it (verified with a probe test); the actual failure is the runner subprocess legitimately exiting 1. Verified: direct runner-loop reproduction of the pre-fix failure (4/20 and 1/15, exact CI error message); vi.setConfig mechanism confirmed with a synthetic 6s-test probe both failing at the unmodified default and passing once scaled; 20/20 clean runs of the previously-flaky test under load post-fix; mcp.test.ts 43/43 across 3x self-concurrency plus heavy synthetic load; test/bench-provenance.test.ts (the direct unit test of writeArmSettings' digest check) unaffected; full suite green. Blast: local Undo: easy Certainty: firm Provenance: authored Record-Id: r-distrace88 CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 2 commits in Active constraints for the paths this PR touchesLimits (30)
Ruled out (80)
Warnings (45)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
This was referenced Jul 28, 2026
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
Two independent fixes, kept as separate commits.
parsereported only the message's own last paragraph whilecontextreported every record block for the same message (bug-issue-60/Recognize multi-record messages so squash inheritance stops losing identity and provenance (#60) #86 fixed the latter but missedparse).parsenow reports every block, labeledownvsearlier, with a single-block message's output kept byte-identical. Also found and fixed: two blocks in one message declaring the sameRecord-Idwere silently merged byparsewith no signal at all —parsenow detects and surfaces that collision.test/bench-ablation.test.ts's "accepts all six arms" test flaked under concurrent load (CI Clarify query path-history guidance #79, Deterministic measurement suite for issue #59, re-measured against current dev #87). Diagnosed as a real contention bug, not a too-small timeout, and fixed the contention.mcp.test.ts's reported flake has a different, genuinely timeout-shaped mechanism; scaled its budget instead of guessing a bigger constant.#89 —
parserecognizes every record blockReproduced before (previously shipped
dist/commitlore.mjs):parseshowed one record;contextshowed two, for the identical message.parse --helpcalls itself the command for this exact question.Reproduced after:
parseandcontextnow agree on record count.--jsongained ablocksarray ({ own, identityCollision, trailers }per block); the existingtrailerskey is unchanged in meaning (the message's own block).Byte-identity for single-block messages: verified directly against the previously-shipped
dist/commitlore.mjs, before rebuilding, across every fixture underspec/fixtures/{valid,boundary,invalid}— text and--jsonboth diffed byte-for-byte identical, zero differences.Also checked, as asked: two blocks in one message declaring the same
Record-Id. Traced whether this reachesidentityCollision(the fieldcontext --jsonalready exposes for a different case). It does not —core/stale.ts'sfindIdCollisionsonly fires when a notes-sourced record disagrees with a commit's own content; a group with nonotesrecord in it (which is what two same-message commit blocks are) never trips it. Confirmed directly: bothcommitlore context --jsonandcommitlore validateaccept such a message today and silently merge the two blocks (contexteven folds the two differentLimit:values into one record).parsenow runs its own local check — independent offindIdCollisions, scoped to the one message being parsed — and surfaces it (identityCollision: trueper block in--json, a text marker, and a stderr diagnostic). Whethercontext/validateshould also catch the same-message case is left open; deliberately out of scope here (see the commit'sRuled-out:trailer).#88 —
bench-ablation.test.tsflake: diagnosis and fixEvidence for a real contention bug, not a too-small timeout (checked before changing anything):
bench/runner.tshashes the wholedist/tree at startup and re-hashes it before each of the six arms (bench/hooks-settings.tswriteArmSettings), refusing an arm when the two disagree. That check is real and useful — its own comment records that it once caught a genuinedist/core/guard.jsdrift mid-measurement.dist/is one directory shared by every vitest worker, though, and four other test files (cli.test.ts,mcp.test.ts,action-lint.test.ts,action-preserve.test.ts) each rebuild it via their owntsc -p tsconfig.jsonin abeforeAll. Any of those rebuilds overlapping withbench-ablation's runner invocation legitimately (and correctly, per the check's own contract) trips it.Reproduced directly, invoking
bench/runner.tsin a loop while a separate process repeatedly rebuildsdist/: 4/20 and 1/15 failures across two independent runs, each with exactly the reported symptom:runRunner'sexecFileSynccall is fully synchronous, so it blocks the event loop for its whole duration — vitest's own per-test timeout timer cannot even fire until it returns. Confirmed with a throwaway probe: anawait setTimeout(6000)test fails at vitest's 5000ms default; the identical 6 seconds of work done synchronously does not, it just reports its real duration. This was never a timeout problem — raising one would have hidden a check that was doing exactly its job.Fix: removed the shared mutable state instead of loosening the check.
DIST_DIR(bench/hooks-settings.ts) now readsCOMMITLORE_BENCH_DIST_DIRwhen set, defaulting to the existing<repo>/dist(unset everywhere except this one test — a real benchmark run is unaffected).bench-ablation.test.tssnapshotsdist/into a private, unshared temp copy once per file and points every runner invocation at it. Also fixedablationShim's separate, previously-hardcodeddist/core/inject.jsreference to go through the sameDIST_DIR, which was inconsistent and would have kept reading the live, shared tree even after this fix.mcp.test.tsdoes not share this cause. It never callsbench/runner.tsor the digest check — it's one of the four racers against bench-ablation, not a victim of the same race. But its JSON-RPC assertionsawaita real round trip (unlike bench-ablation's synchronousexecFileSync), so they are mechanically subject to vitest's fixed 5000ms default under real concurrency — confirmed with the same probe technique (an awaited 6s wait fails at the default; a synchronous one does not). Scaled its budget instead of guessing a bigger constant:vi.setConfig({ testTimeout: 5_000 * Math.max(availableParallelism() - 1, 1) }), tying it to the same worker count vitest's own default pool sizing already uses.Verification — consecutive clean runs under deliberate load:
bench/runner.tsinvoked directly in a loop against a competingtscrebuild loop: 20/20 clean post-fix, under the identical load that produced 4/20 failures pre-fix.mcp.test.tsalone: 43/43 passing across 3 self-concurrent full runs plus 6 paralleltscrebuild loops and 8 CPU-bound processes — could not force an actual failure on this 12-core dev machine even under that load, consistent with the CI report coming from a more constrained/shared runner rather than a logic bug.tscrebuild loops + 3–4 CPU-bound processes running for the whole suite's ~10–11 minute duration, on two separate worktrees checked out at this branch): 36 files, 1335 tests (1334 passed, 1 skipped) — clean on both. 3/3 consecutive clean full-suite runs under deliberate load overall.One honest caveat found along the way, not fixed here: under a much heavier, sustained synthetic load (multiple
tscrebuild loops running continuously for the suite's entire ~10+ minute duration — well beyond what a singlebeforeAllracing another produces), onemcp.test.tstest that spawns the CLI (commitlore context --json) hit a transientSyntaxError: The requested module '../core/squash.js' does not provide an export named 'collectRange'— the same underlying hazard as #88 (a subprocess readingdist/while another process is mid-write to it), surfacing as a module-resolution error instead of a digest mismatch, becausemcp.test.tsreads the live, shareddist/directly rather than a private snapshot. This needs sustained, unrealistic rebuild pressure to reproduce (not a single siblingbeforeAll), so it is reported rather than fixed here — a natural next step if it recurs would be the more thorough fix already considered and ruled out for this PR (consolidating the four files' redundanttscrebuilds into onevitestglobalSetup, see the #88 commit'sRuled-out:trailer).Both
33dc7aa(dev tip) — 36 files, 1297 tests (1296 passed, 1 skipped).node dist/commitlore.mjs validate --commit HEADindividually.dev.