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); + }); });