From 95a197b3418b0a22b746a33abaaad6ab2d74df7b Mon Sep 17 00:00:00 2001 From: operator Date: Sat, 3 Oct 2026 17:21:47 +0900 Subject: [PATCH] fix: inherit records, not every trailer paragraph, on squash-preserve git decides what a trailer block is, and a message's last paragraph qualifies whatever its keys mean. A merge commit whose final paragraph held one `Claude-Session:` line was therefore collected as an inherited record, stamped with `Provenance: inherited `, and the composed squash message was refused by `commitlore validate` with `unknown-key Claude-Session` -- blocking the merge that `squash-preserve` had been run to protect. An inherited block now carries only the keys this protocol defines, and a block left with none contributes nothing. The filter is per trailer rather than per block because a paragraph mixing `Limit:` with a foreign key is a real record: dropping the block would lose it, and inheriting it whole would still fail validate on the foreign key. The collision check had to agree. Stripping those keys makes a faithful copy differ from its origin, and the mixed case stopped failing on `unknown-key` and started failing on `duplicate-id` -- so the inherited-copy comparison compares the record's own content and ignores what the vocabulary does not cover, the same exemption #1148 made for the transport stamp. Both halves are load-bearing: the new tests were run with each change absent in turn and fail without it. Limit: git's grammar makes a message's last paragraph a trailer block whatever its keys mean, so the paragraph cannot be refused at parse time Limit: the inherited-copy comparison now ignores keys outside the vocabulary, so a copy carrying a different value for one than its origin did is no longer reported as a collision Ruled-out: dropping only the blocks whose every key is foreign | a paragraph mixing `Limit:` with `Claude-Session:` is a real record, and inheriting it whole still composes a message validate refuses on the foreign key Ruled-out: comparing an inherited copy against its origin in full | stripping the foreign keys makes every faithful copy read as a divergent re-declaration, the false refusal #1148 removed for the provenance stamp Warn: a commit that already landed with an inherited non-record block re-squashes to a block whose only key is `Provenance:` -- it validates, and it says nothing Blast: module Undo: easy Certainty: firm Record-Id: r-nonrecordblock1153 Provenance: drafted --- src/core/squash.ts | 33 +++++++++++++- src/core/stale.ts | 33 +++++++++++++- test/squash.test.ts | 107 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 2 deletions(-) diff --git a/src/core/squash.ts b/src/core/squash.ts index bd550e3d..c33a24d6 100644 --- a/src/core/squash.ts +++ b/src/core/squash.ts @@ -69,6 +69,7 @@ import { CERTAINTY_VALUES, SINGLE_VALUED, UNDO_VALUES, + isCommitLoreKey, type Trailer, } from './types.js'; @@ -291,6 +292,36 @@ const recordIdOf = (record: CollectedRecord): string | undefined => const contentSet = (trailers: readonly Trailer[]): Set => new Set(trailers.map((trailer) => `${trailer.key}${NUL}${trailer.value}`)); +/** + * One commit's blocks reduced to the records among them: the trailers whose + * keys this protocol defines, and no block left without one. + * + * git's grammar decides what a trailer block is, and a message's last + * paragraph qualifies whatever its keys mean -- so a paragraph holding only + * `Claude-Session:` is a trailer block. It is not a record, and inheriting it + * wrote a key SPEC §3 does not define into the message the squash was about to + * commit, which `commitlore validate` then refused, blocking the merge + * `squash-preserve` had been run to protect (#1153). + * + * `types.ts isCommitLoreKey` already answers this question for the index, + * where the same paragraphs were served to agents as recorded decisions + * (#335). Here it is asked per trailer rather than per block, because what + * this module emits is a commit message that has to validate: a paragraph + * mixing `Limit:` with a foreign key is a real record, and dropping the whole + * block to be rid of the foreign key would lose it. `Signed-off-by:` goes the + * same way as any other key outside the vocabulary -- it is an attestation + * about the commit that carried it, and inheriting it would assert a sign-off + * on a commit nobody signed. + * + * Applied where both channels become candidate records, before they are + * matched (`mergeCommitBlocks`), so a message cannot reintroduce through its + * mirror what the mirror dropped, or the other way round. + */ +const recordsAmong = (blocks: readonly Trailer[][]): Trailer[][] => + blocks + .map((block) => block.filter((trailer) => isCommitLoreKey(trailer.key))) + .filter((block) => block.length > 0); + /** * Matches one commit's message blocks against its mirrored note blocks * (SPEC §2.4), so a source commit that itself carries several record blocks — @@ -433,7 +464,7 @@ export const collectRange = (range: string, opts: SquashOptions = {}): Collected const noteBlocks = cachedNote ?? (mirrored.has(sha) ? readRecordBlocks(sha, opts) : []); if (cachedNote === undefined) opts.cache?.notes.set(sha, noteBlocks); - const blocks = mergeCommitBlocks(messageBlocks, noteBlocks); + const blocks = mergeCommitBlocks(recordsAmong(messageBlocks), recordsAmong(noteBlocks)); for (const trailers of blocks) { if (trailers.length === 0) continue; diff --git a/src/core/stale.ts b/src/core/stale.ts index a6c5bc95..14a258ae 100644 --- a/src/core/stale.ts +++ b/src/core/stale.ts @@ -19,6 +19,7 @@ import { RECORD_ID_RE, SINGLE_VALUED, + isCommitLoreKey, parseProvenance, type Lifecycle, type Record, @@ -471,6 +472,36 @@ const payloadSignatureWithoutProvenance = (record: StaleRecord): string => .sort() .join('\u0001'); +/** + * `payloadSignatureWithoutProvenance` restricted to the keys this protocol + * defines (`types.ts isCommitLoreKey`). + * + * What an inherited copy carries is the record, not whatever else the origin's + * trailer paragraph happened to hold: `squash-preserve` drops the foreign keys + * so the message it composes passes `validate` at all (#1153), and a + * `Signed-off-by:` on the origin is an attestation about that commit which + * must not be copied onto another. Compared in full, every faithful copy of + * such a record would read as divergent and be reported as a `duplicate-id` + * -- the same false refusal #1148 removed for the provenance stamp, one key + * class over. + * + * Only the inherited-copy comparison uses this. Weakening it costs the case + * where a copy carries a *different* value for a foreign key than its origin + * did, which stops being a collision; the record's own content still has to + * match exactly. + */ +const recordPayloadSignature = (record: StaleRecord): string => + record.trailers + .filter( + (trailer) => + trailer.key !== RECORD_ID_KEY && + trailer.key !== PROVENANCE_KEY && + isCommitLoreKey(trailer.key), + ) + .map((trailer) => `${trailer.key}\u0000${trailer.value}`) + .sort() + .join('\u0001'); + /** * Whether `message` is a fold of several records and `note` is one of them * (#1116). @@ -564,7 +595,7 @@ const collisionRivals = (group: StaleRecord[]): StaleRecord[] => group.flatMap(( if (isOwnCommitMirror(record, group)) return []; const origin = inheritedOrigin(record, group); if (origin === undefined) return [record]; - if (payloadSignatureWithoutProvenance(record) === payloadSignatureWithoutProvenance(origin)) return []; + if (recordPayloadSignature(record) === recordPayloadSignature(origin)) return []; return [{ ...record, trailers: [ diff --git a/test/squash.test.ts b/test/squash.test.ts index a99745d3..f11ee653 100644 --- a/test/squash.test.ts +++ b/test/squash.test.ts @@ -710,4 +710,111 @@ describe('squash-preserve', () => { expect(outcome.stderr).toContain('nothing to preserve'); expect(readFileSync(draft, 'utf8')).toBe(GITHUB_DRAFT); }); + + /** + * bug-issue-1153: the paragraph a non-CommitLore trailer forms is a trailer + * block to git and not a record to this protocol, and inheriting it put a key + * SPEC §3 does not define into the message the squash was about to commit — + * which `commitlore validate` then refused, blocking the merge it was run to + * protect. + */ + it('inherits nothing from a paragraph that carries no record key (bug-issue-1153)', () => { + const repo = initRepo('squash-nonrecord-paragraph'); + const base = commitFile(repo, 'seed.txt', 'seed\n', 'seed\n'); + const merge = commitFile( + repo, + 'merge.txt', + 'merge\n', + 'Merge pull request #1051 from feature\n\nClaude-Session: https://claude.ai/code/session_01ABC\n', + ); + const recorded = commitFile( + repo, + 'queue.ts', + 'export const workers = 3;\n', + 'cap the pool\n\nLimit: the vendor caps us at three concurrent workers\nRecord-Id: r-nonrec1153\n', + ); + const draft = draftFile(repo, 'squash the branch\n'); + + const outcome = runSquashPreserve({ range: `${base}..HEAD`, messageFile: draft, cwd: repo }); + expect(outcome.code).toBe(0); + + const composed = readFileSync(draft, 'utf8'); + // The specific diagnosis, not merely "validate passed": the unfixed build + // failed with exactly this violation, and a draft can fail validate for + // reasons that have nothing to do with this issue. + const result = runValidate({ messageFile: draft, cwd: repo }); + expect(result.violations).not.toContainEqual( + expect.objectContaining({ rule: 'unknown-key', key: 'Claude-Session' }), + ); + expect(result.code).toBe(0); + expect(result.violations).toEqual([]); + expect(composed).not.toContain('Claude-Session'); + expect(composed).not.toContain(`inherited ${merge}`); + expect(composed).not.toContain(merge); + + // Control: the commit that did record something is still inherited, so the + // fix is a filter on non-records and not a filter on everything. + const blocks = parseRecordBlocks(composed); + expect(value(blockById(blocks, 'r-nonrec1153'), 'Limit')).toBe( + 'the vendor caps us at three concurrent workers', + ); + expect(value(blockById(blocks, 'r-nonrec1153'), 'Provenance')).toBe(`inherited ${recorded}`); + expect(collectRange(`${base}..HEAD`, { cwd: repo }).map((entry) => entry.sha)).toEqual([ + recorded, + ]); + + // The notes mirror carries what the message does, so the same non-record + // paragraph cannot come back through the other channel on the next squash. + git(repo, ['commit', '--quiet', '-F', draft, '--allow-empty']); + expect(runSquashPreserve({ range: `${base}..HEAD`, target: 'HEAD', cwd: repo }).code).toBe(0); + for (const block of readRecordBlocks(head(repo), { cwd: repo })) { + expect(block.map((trailer) => trailer.key)).not.toContain('Claude-Session'); + } + }); + + /** + * The same defect through the door the first case does not cover: a record + * and a foreign trailer in one paragraph. Dropping the whole block would lose + * a real record, so the filter is per trailer — an inherited block carries + * only keys this protocol defines, which is what makes the composed message + * validate by construction rather than by luck. + */ + it('inherits only protocol keys from a block that mixes them (bug-issue-1153)', () => { + const repo = initRepo('squash-mixed-paragraph'); + const base = commitFile(repo, 'seed.txt', 'seed\n', 'seed\n'); + const mixed = commitFile( + repo, + 'queue.ts', + 'export const retries = 2;\n', + 'retry on 429\n\n' + + 'Warn: do not raise the retry ceiling without re-reading the vendor quota\n' + + 'Record-Id: r-mixed1153\n' + + 'X-Deploy-Window: tuesdays\n' + + 'Signed-off-by: A Committer \n' + + 'Claude-Session: https://claude.ai/code/session_01ABC\n', + ); + const draft = draftFile(repo, 'squash the mixed branch\n'); + + expect(runSquashPreserve({ range: `${base}..HEAD`, messageFile: draft, cwd: repo }).code).toBe( + 0, + ); + + const composed = readFileSync(draft, 'utf8'); + const block = blockById(parseRecordBlocks(composed), 'r-mixed1153'); + expect(value(block, 'Warn')).toBe( + 'do not raise the retry ceiling without re-reading the vendor quota', + ); + expect(value(block, 'Provenance')).toBe(`inherited ${mixed}`); + // An `X-` extension is SPEC §3 vocabulary and stays; a sign-off is an + // attestation about another commit and is not inheritable onto this one. + expect(value(block, 'X-Deploy-Window')).toBe('tuesdays'); + expect(block.map((trailer) => trailer.key)).not.toContain('Claude-Session'); + expect(block.map((trailer) => trailer.key)).not.toContain('Signed-off-by'); + + const result = runValidate({ messageFile: draft, cwd: repo }); + expect(result.violations).not.toContainEqual( + expect.objectContaining({ rule: 'unknown-key', key: 'Claude-Session' }), + ); + expect(result.code).toBe(0); + }); });