From b8c70a33c08ed5324c9d09974e4cbea547a42a39 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 8 Sep 2026 02:01:12 -0400 Subject: [PATCH] fix(#4454): surface skipped explicit --files paths instead of silent success (#4538) * fix(#4454): surface skipped explicit --files paths instead of silent success cmdCommit's staging loop deliberately skips a --files path that no longer exists on disk when the caller passed --files explicitly (#2014 guards against staging an unwanted deletion for a temporarily-absent file), but recorded nothing about the skip. The final result reported unqualified committed: true, so a caller had no way to distinguish "everything in --files landed" from "some paths were silently excluded because they didn't exist" -- a real file deletion meant to be committed (e.g. phase.complete removing .planning/milestone.lock) would silently never land, with git status still showing it unstaged after a "successful" commit. Tracks skipped paths in a skippedFiles array and surfaces them as skipped_files (matching this result family's existing snake_case precedent, timed_out) in the success result AND the nothing_to_commit result (reachable when every named path was missing), included only when non-empty so the common case's payload shape is unchanged. Also extended to the SECOND nothing_to_commit result (reached when git itself reports "nothing to commit" after the nothingToCommit guard was false -- the residual partial-skip + partialCommitRefused window the surrounding comments already document) for the same consistency. The #2014 staging/deletion guard itself is untouched -- purely a visibility fix, exactly as the issue requested. Regression tests cover: existing-plus-missing file (the issue's own repro shape, with the missing path genuinely tracked-then-deleted so the #2014 assertion is meaningful, not vacuous against an never-tracked path); only-existing files (no skipped_files key at all); only-missing files (nothing_to_commit with skipped_files); default mode unaffected; and multiple missing files reported in order. Co-Authored-By: Claude Sonnet 5 * fix: recover ack trailers buried by squash-merge concatenation Discovered while verifying an unrelated PR (#4454): tests/emitted-attribution.test.cjs failed citing pr-branch.md's growth as unacknowledged, even though PR #4537 (#4447) had genuinely acked it via an Emitted-Drift-Ack-Growth commit trailer. Root cause: git's %(trailers:...) placeholder (used by readAckTrailers) only recognizes a trailer block that is the TRUE TERMINAL block of a commit message. GitHub's squash-merge commit body concatenates every constituent commit's subject+body in order, then unconditionally appends its own `---------` separator and Co-authored-by trailers. Confirmed directly against a real squash commit: %(trailers) returns ONLY GitHub's own two Co-authored-by lines -- not even the LAST original commit's own trailer survives, since GitHub's appended suffix breaks the backward scan before it ever reaches past `---------` to any original commit's content, last or not. An ack trailer added on any non-final commit of a PR (the common case -- more commits routinely land after an ack, e.g. a lint fix or a changeset) is therefore silently invisible forever to any future comparison against a base predating that squash. Fix: readAckTrailers now runs a second pass (extractSquashBuriedTrailers) alongside the existing whole-message read. For each commit in range, if its raw body contains GitHub's squash-suffix signal, the pre-suffix text (everything before the LAST such signal -- an earlier bullet's own body may legitimately contain a markdown horizontal rule that looks the same, so anchoring on the first occurrence would truncate too early and miss a later bullet's real ack) is split on squash-bullet (`* `) boundaries, and each chunk is independently trailer-parsed via `git interpret-trailers --parse` -- the same underlying algorithm as %(trailers:...), but runnable against arbitrary text rather than only a real commit object. This finds a trailer buried in ANY bullet, not just the last one. Scoped tightly to avoid reintroducing the false-positive class %(trailers:...) was originally chosen to prevent (a mid-body MENTION of trailer syntax must stay inert): the sub-chunk pass activates only on commits matching the squash-suffix signal, so an ordinary commit whose body happens to contain markdown bullets is completely unaffected, and each chunk still goes through git's own strict per-chunk terminal-block detection. Verified end-to-end against a real squash commit (recovers the buried trailer) and four adversarial fixtures now pinned as regression tests: ordinary bullet prose with no squash suffix (stays empty); a squash-shaped commit where one bullet's body merely mentions trailer syntax mid-paragraph (stays inert, the "row 32" false-positive class, now verified at per-chunk granularity); and an earlier bullet's own markdown horizontal rule not truncating the scan before a later bullet's real ack (the last-match-not-first-match case an isolated review pass caught during this same fix). This overrides one-concern-per-PR per CLAUDE.md's Defects & Warnings policy -- a genuine defect discovered mid-work is fixed inline, not deferred to a separate issue (spawn_task for this was correctly blocked by the no-defer guard). Co-Authored-By: Claude Sonnet 5 * docs(#4454): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: sim Co-authored-by: Claude Sonnet 5 --- .changeset/graceful-dogs-bark.md | 5 + src/commands.cts | 40 +++++++- tests/commit-files-pathspec.test.cjs | 123 ++++++++++++++++++++++++ tests/emitted-ack-trailer.test.cjs | 139 +++++++++++++++++++++++++++ tests/helpers/emitted-runtime.cjs | 136 +++++++++++++++++++++++++- 5 files changed, 438 insertions(+), 5 deletions(-) create mode 100644 .changeset/graceful-dogs-bark.md diff --git a/.changeset/graceful-dogs-bark.md b/.changeset/graceful-dogs-bark.md new file mode 100644 index 000000000..513c98c47 --- /dev/null +++ b/.changeset/graceful-dogs-bark.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4538 +--- +**`commit --files` now reports which explicitly-named paths were skipped** — a path named in `--files` that no longer exists on disk was silently dropped from the commit (guarding against staging an unwanted deletion), but the result reported unqualified success with no way to tell a partial commit from a complete one. The result now includes `skipped_files` naming any dropped path, present only when something was actually skipped. (#4454) diff --git a/src/commands.cts b/src/commands.cts index 875629738..58cb8d837 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1727,6 +1727,10 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // a linked worktree, timeout) was discarded and the operator saw a downstream // pathspec error pointing at an innocent file. const stagingFailures: Array<{ file: string; error: string; timed_out: boolean }> = []; + // #4454: explicit --files paths skipped because they were missing from disk + // (the #2014 guard below). Tracked so the caller can tell a partial commit + // from a complete one instead of an unqualified `committed: true`. + const skippedFiles: string[] = []; // Paths already in the index BEFORE this call. On a staging failure the // rollback below unstages only what THIS call added — unstaging a path the // caller had staged themselves would destroy their work. @@ -1741,6 +1745,9 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // Caller passed an explicit --files list: missing files are skipped. // Staging a deletion here would silently remove tracked planning files // (e.g. STATE.md, ROADMAP.md) when they are temporarily absent (#2014). + // #4454: record what was skipped so the caller can tell a partial + // commit from a complete one, instead of an unqualified success. + skippedFiles.push(file); continue; } // Default mode (staging all of .planning/): stage the deletion so @@ -2041,7 +2048,15 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u ).exitCode === 0 && !assumeUnchangedWouldRecord())); if (nothingToCommit) { - const result = { committed: false, hash: null, reason: 'nothing_to_commit' }; + // #4454: an explicit --files list where every named path was missing + // reaches this branch via `stagedPaths.length === 0` above — surface + // which path(s) were the reason, same as the success result below. + const result = { + committed: false, + hash: null, + reason: 'nothing_to_commit', + ...(skippedFiles.length > 0 ? { skipped_files: skippedFiles } : {}), + }; output(result, raw, 'nothing'); return; } @@ -2110,7 +2125,18 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u return; } if (commitResult.stdout.includes('nothing to commit') || commitResult.stderr.includes('nothing to commit')) { - const result = { committed: false, hash: null, reason: 'nothing_to_commit' }; + // #4454: this is the residual window the surrounding comments already + // document (a partial skip + partialCommitRefused bypassing the diff + // probe + git's own empty-commit refusal) — skippedFiles can be + // non-empty here too, and omitting it would be the same misreport + // this fix exists to close, just on the other branch that reaches + // "nothing to commit". + const result = { + committed: false, + hash: null, + reason: 'nothing_to_commit', + ...(skippedFiles.length > 0 ? { skipped_files: skippedFiles } : {}), + }; output(result, raw, 'nothing'); return; } @@ -2127,7 +2153,15 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // Get short hash const hashResult = execGit(['rev-parse', '--short', 'HEAD'], { cwd }); const hash = hashResult.exitCode === 0 ? hashResult.stdout : null; - const result = { committed: true, hash, reason: 'committed' }; + // #4454: report explicit --files paths that were skipped as missing (the + // #2014 guard above) so a caller can tell a partial commit from a complete + // one, without changing the payload shape when nothing was skipped. + const result = { + committed: true, + hash, + reason: 'committed', + ...(skippedFiles.length > 0 ? { skipped_files: skippedFiles } : {}), + }; output(result, raw, hash || 'committed'); } diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index 2a91f326c..7e6ed04ba 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -183,6 +183,129 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { ); }); + // ── #4454: skipped-missing --files paths are reported, not silently dropped ── + + test('#4454: --files with one existing + one missing file reports skipped_files and does not commit the deletion', () => { + // Mirrors the issue's own repro shape: an existing modified file alongside + // a missing tracked one (e.g. milestone.lock + state.json). state.json + // must be TRACKED then removed from disk — an untracked-and-missing path + // would make the #2014 assertion below vacuous, since `git rm --cached` + // on a never-tracked path is a no-op with or without the skip guard. + fs.writeFileSync(path.join(tmpDir, '.planning', 'milestone.lock'), 'a\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'state.json'), '{}\n'); + gitOrThrow(['add', '.planning/milestone.lock', '.planning/state.json'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed milestone.lock and state.json'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'milestone.lock'), 'b\n'); + fs.unlinkSync(path.join(tmpDir, '.planning', 'state.json')); + + const res = runGsdTools( + ['commit', 'docs: update milestone', '--files', '.planning/milestone.lock', '.planning/state.json'], + tmpDir, + ); + const parsed = JSON.parse(res.output); + + assert.strictEqual(parsed.committed, true, `expected a commit: ${res.output}`); + assert.ok(typeof parsed.hash === 'string' && parsed.hash.length > 0, 'hash must be a non-empty string'); + assert.deepStrictEqual( + parsed.skipped_files, ['.planning/state.json'], + `skipped_files must name exactly the missing path: ${res.output}`, + ); + + // #2014: state.json was TRACKED and is now missing from disk — the skip + // guard must leave its deletion unstaged, not just leave an + // already-untracked path alone (which would prove nothing). + const diffNameStatus = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-status'], { + cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS, + }); + assert.ok( + !diffNameStatus.includes('state.json'), + `no deletion of the still-tracked-on-disk-missing file may be recorded: ${diffNameStatus}`, + ); + const lsTree = gitOrThrow(['ls-tree', '-r', '--name-only', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + assert.ok( + lsTree.includes('.planning/state.json'), + `state.json must still be tracked at HEAD — its deletion must not have been committed: ${lsTree}`, + ); + }); + + test('#4454: --files with only existing files omits skipped_files entirely', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'RESEARCH.md'), '# Research\n'); + + const res = runGsdTools( + ['commit', 'docs: artifacts only', '--files', '.planning/PLAN.md', '.planning/RESEARCH.md'], + tmpDir, + ); + const parsed = JSON.parse(res.output); + + assert.strictEqual(parsed.committed, true, `expected a commit: ${res.output}`); + assert.ok( + !('skipped_files' in parsed), + `no skipped_files key may be present when nothing was skipped: ${res.output}`, + ); + }); + + test('#4454: --files naming only missing file(s) reports nothing_to_commit with skipped_files', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n'); + gitOrThrow(['add', '.planning/STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'add STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md')); + + const res = runGsdTools( + ['commit', 'docs: try', '--files', '.planning/STATE.md'], + tmpDir, + ); + const parsed = JSON.parse(res.output); + + assert.strictEqual(parsed.committed, false); + assert.strictEqual(parsed.reason, 'nothing_to_commit'); + assert.deepStrictEqual( + parsed.skipped_files, ['.planning/STATE.md'], + `an all-missing --files list reaches nothing_to_commit via stagedPaths.length === 0; ` + + `skipped_files must still name the reason: ${res.output}`, + ); + }); + + test('#4454: default mode (no --files) with a missing tracked file is unaffected — no skipped_files anywhere', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n'); + gitOrThrow(['add', '.planning/STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'add STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md')); + fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n'); + + const res = runGsdTools(['commit', 'docs: default mode'], tmpDir); + const parsed = JSON.parse(res.output); + + assert.strictEqual(parsed.committed, true, `expected a commit: ${res.output}`); + assert.ok(!('skipped_files' in parsed), `default mode never sets explicitFiles: ${res.output}`); + + // The deletion IS staged and committed as before — explicitFiles is false here. + const diffNameStatus = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-status'], { + cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS, + }); + assert.ok( + diffNameStatus.includes('D\t.planning/STATE.md'), + `default mode must still stage/commit the deletion: ${diffNameStatus}`, + ); + }); + + test('#4454: --files naming two missing files reports both, in the order they were named', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n'); + + const res = runGsdTools( + ['commit', 'docs: mixed missing', '--files', '.planning/PLAN.md', '.planning/GONE-A.md', '.planning/GONE-B.md'], + tmpDir, + ); + const parsed = JSON.parse(res.output); + + assert.strictEqual(parsed.committed, true, `expected a commit: ${res.output}`); + assert.deepStrictEqual( + parsed.skipped_files, + ['.planning/GONE-A.md', '.planning/GONE-B.md'], + `both missing paths must be reported in the order named: ${res.output}`, + ); + }); + test('#2523: absolute --files path inside the repo is committed, not silently dropped', () => { // init phase-op emits phase_dir as an ABSOLUTE path (#2428); cmdCommit must // accept it. The bug was path.join(cwd, absPath) → cwd+absPath (non-existent) diff --git a/tests/emitted-ack-trailer.test.cjs b/tests/emitted-ack-trailer.test.cjs index 2507c93bb..039bd5f16 100644 --- a/tests/emitted-ack-trailer.test.cjs +++ b/tests/emitted-ack-trailer.test.cjs @@ -616,6 +616,145 @@ describe('real fixture reads via readAckTrailers', () => { ); }, ); + + // #4454 follow-on: a trailer buried mid-message by squash-merge concatenation is + // recovered per-bullet, without reintroducing the false-positive class the + // whole-message %(trailers:...) design deliberately avoids (row 32). These three + // rows mirror a real squash commit's shape directly (GitHub's own suffix, `* ` + // bullet boundaries) rather than a synthetic simplification of it, since the whole + // point is that the ordinary `%(trailers:...)` reader cannot see through this shape. + + test( + 'a growth trailer buried mid-message by squash-merge concatenation is recovered — #4454', + (t) => { + const dir = makeTempRepo('gsd-ack-trailer-4454-buried-'); + withCleanup(t, dir); + commitMessage(dir, 'base\n\nno trailer\n'); + const baseSha = headSha(dir); + // Mirrors GitHub's real squash-merge shape: several `* ` bullets + // concatenated, the ack trailer landing on a NON-LAST one, followed by more + // bullets and then GitHub's own appended suffix — the exact shape that made + // #4447's ack invisible to a later PR's emitted-attribution check. + commitMessage( + dir, + 'fix(#9999): example squash-merged PR (#1000)\n\n' + + '* fix(#9999): first commit\n\n' + + 'Some body text for the first commit.\n\n' + + '* fix(#9999): second commit grows a workflow file\n\n' + + 'Body text.\n\n' + + `${trailerLine(ACK_TRAILER_GROWTH, 'some-workflow.md', 'deliberate growth for #9999')}\n\n` + + '* fix(#9999): third commit, unrelated\n\n' + + 'More body text, landing AFTER the trailer above in the squashed message.\n\n' + + '---------\n\n' + + 'Co-authored-by: real-author \n', + ); + const r = readAckTrailers({ baseRef: baseSha, headRef: 'HEAD', cwd: dir }); + assert.equal(r.errors.length, 0); + assert.equal( + r.growth.get('some-workflow.md')?.reason, + 'deliberate growth for #9999', + 'the buried trailer must be recovered even though a later bullet and ' + + "GitHub's own appended suffix follow it in the squashed message", + ); + }, + ); + + test( + 'an ordinary commit with markdown bullets but no squash suffix finds nothing — #4454 (no false positive)', + (t) => { + const dir = makeTempRepo('gsd-ack-trailer-4454-ordinary-'); + withCleanup(t, dir); + commitMessage(dir, 'base\n\nno trailer\n'); + const baseSha = headSha(dir); + // A normal commit body using markdown bullets for ordinary prose — no GitHub + // squash suffix anywhere, so the sub-chunk recovery pass must not activate at + // all, regardless of what the bullets contain. + commitMessage( + dir, + 'docs: explain the ack mechanism\n\n' + + 'This adds documentation. Notes:\n\n' + + '* first bullet, ordinary prose\n' + + `* second bullet mentioning ${ACK_TRAILER_GROWTH}: foo.md${ACK_TRAILER_DELIM}not a real trailer, just prose in a bullet\n` + + '* third bullet\n\n' + + 'No real trailer in this commit.\n', + ); + const r = readAckTrailers({ baseRef: baseSha, headRef: 'HEAD', cwd: dir }); + assert.equal(r.errors.length, 0); + assert.equal(r.growth.size, 0, 'ordinary bullet prose must never be recognized as a trailer'); + assert.equal(r.hash.size, 0); + }, + ); + + test( + 'a squash-shaped commit where one bullet merely MENTIONS trailer syntax mid-paragraph stays inert — #4454 (row 32, per-chunk)', + (t) => { + const dir = makeTempRepo('gsd-ack-trailer-4454-mention-'); + withCleanup(t, dir); + commitMessage(dir, 'base\n\nno trailer\n'); + const baseSha = headSha(dir); + // Squash-shaped (has the GitHub suffix), but the trailer-looking text inside + // its one bullet is MID-PARAGRAPH — followed by more prose within that SAME + // chunk — so it is not that chunk's own terminal line and must stay inert, + // exactly as the whole-message design already guaranteed for the un-squashed + // case (row 32). + commitMessage( + dir, + 'chore: squash-shaped with mid-chunk mention (#1000)\n\n' + + '* docs: explain the ack mechanism\n\n' + + 'This docs commit teaches contributors the trailer format, e.g.\n' + + `${trailerLine(ACK_TRAILER_GROWTH, 'some-file.md', 'example text in the docs body')}\n` + + 'followed by more prose so it is NOT this chunk\'s terminal line.\n\n' + + 'Co-Authored-By: Someone \n\n' + + '---------\n\n' + + 'Co-authored-by: real-author \n', + ); + const r = readAckTrailers({ baseRef: baseSha, headRef: 'HEAD', cwd: dir }); + assert.equal(r.errors.length, 0); + assert.equal(r.growth.size, 0, 'a mid-paragraph mention of trailer syntax must never be recognized'); + assert.equal(r.hash.size, 0); + }, + ); + + // Isolated review finding (2026-09-08): a bare `.match()` against the squash-suffix + // pattern returns the FIRST occurrence scanning left-to-right, not the last. GitHub's + // OWN appended suffix is always the true tail of the message, so an EARLIER bullet's + // own body legitimately using a markdown horizontal rule (also `---------`-shaped, + // e.g. as a section break in prose) truncated the scan too early and silently + // excluded a LATER bullet's real ack from the split/parse pass — reintroducing the + // exact #4454 bug this whole function exists to fix. Reproduced directly against a + // real fixture before the fix (readAckTrailers returned an EMPTY growth map here); + // must find the ack after it. + test( + 'an earlier bullet\'s own markdown HR does not truncate the scan before a LATER bullet\'s real ack — #4454 (last-match, not first-match)', + (t) => { + const dir = makeTempRepo('gsd-ack-trailer-4454-earlier-hr-'); + withCleanup(t, dir); + commitMessage(dir, 'base\n\nno trailer\n'); + const baseSha = headSha(dir); + commitMessage( + dir, + 'chore: squash-shaped with an EARLIER HR before the real ack bullet (#1000)\n\n' + + '* docs: a bullet with its own legitimate markdown HR\n\n' + + 'Some docs text explaining something.\n\n' + + '---------\n\n' + + 'That has its own HR divider used as a section break in ordinary prose.\n\n' + + '* fix: second bullet carries the real ack\n\n' + + 'Body text.\n\n' + + `${trailerLine(ACK_TRAILER_GROWTH, 'foo.md', 'real ack, must be recovered')}\n\n` + + '* fix: third bullet, unrelated\n\n' + + 'More text.\n\n' + + '---------\n\n' + + 'Co-authored-by: real-author \n', + ); + const r = readAckTrailers({ baseRef: baseSha, headRef: 'HEAD', cwd: dir }); + assert.equal(r.errors.length, 0); + assert.equal( + r.growth.get('foo.md')?.reason, 'real ack, must be recovered', + 'the real ack must be found even though an EARLIER bullet contains its own ' + + 'HR-shaped divider before it', + ); + }, + ); }); // ─── hostile IO: uncomputable range, subprocess failure, timeout — rows 22-24 ─ diff --git a/tests/helpers/emitted-runtime.cjs b/tests/helpers/emitted-runtime.cjs index bb2477e95..81c5758cb 100644 --- a/tests/helpers/emitted-runtime.cjs +++ b/tests/helpers/emitted-runtime.cjs @@ -47,6 +47,7 @@ const { } = require('./install-shared.cjs'); const { ACK_TRAILER_HASH, ACK_TRAILER_GROWTH, parseAckTrailers } = require('./emitted-diff.cjs'); const { runGit, OUTCOME } = require('./process-seam.cjs'); +const { escapeRegex } = require('../../gsd-core/bin/lib/pattern.cjs'); const REPO_ROOT = path.join(__dirname, '..', '..'); const FIXTURE_SUBDIR = 'tests/fixtures/golden-install-parity'; @@ -723,8 +724,10 @@ function currentSizes({ repoRoot = REPO_ROOT } = {}) { * clean exit — same "a git failure is a hard error, never an empty result" law as * `resolveChangedPaths` above. */ -function ackTrailerGit(args, { cwd = REPO_ROOT, timeoutMs = GIT_TIMEOUT_MS } = {}) { - const result = runGit([...safeDirArgs(cwd), ...args], { cwd, timeoutMs }); +function ackTrailerGit(args, { cwd = REPO_ROOT, timeoutMs = GIT_TIMEOUT_MS, input } = {}) { + const spawnOpts = { cwd, timeoutMs }; + if (input !== undefined) spawnOpts.input = input; + const result = runGit([...safeDirArgs(cwd), ...args], spawnOpts); if (result.outcome === OUTCOME.EXITED && result.exitCode === 0) { return result.stdout; } @@ -734,6 +737,111 @@ function ackTrailerGit(args, { cwd = REPO_ROOT, timeoutMs = GIT_TIMEOUT_MS } = { ); } +// #4454 follow-on — SQUASH-MERGE BURIAL. `%(trailers:...)` (used by readAckTrailers +// below) only recognises a trailer block that is the TRUE TERMINAL block of a commit +// message: consecutive `Token: value` lines running to the very end, nothing after. +// GitHub's squash-merge commit body is every constituent commit's subject+body +// concatenated in order, followed by its OWN appended `---------` separator and +// `Co-authored-by:` trailers. Two independent commits landing after the one carrying +// an ack trailer — including GitHub's own appended suffix, which follows EVERY squash +// commit unconditionally — silently bury it: git's parser (correctly, by its own +// contract) stops at the first non-conforming line scanning backward, so it never even +// reaches past `---------` to see the real content underneath, let alone further back +// to an earlier bullet. Confirmed directly against a real squash commit (gsd-core +// 78013b3b74): `%(trailers)` there returns ONLY GitHub's own two `Co-authored-by:` +// lines — not even the last original commit's own trailer survives, only GitHub's +// appended one. +// +// FIX: independently trailer-parse each squash-BULLET sub-chunk of the raw message, +// using `git interpret-trailers --parse` (the same underlying algorithm as +// `%(trailers:...)`, but runnable against arbitrary TEXT via stdin rather than only a +// real commit object) — so each original commit's own terminal trailer block is found +// on its own terms, independent of what got concatenated after it. +// +// SCOPED TIGHTLY to avoid reintroducing the false-positive class `%(trailers:...)` was +// chosen to prevent (row 32 — prose that merely MENTIONS trailer syntax must stay +// inert): this sub-chunk pass activates ONLY when the raw body contains GitHub's own +// distinctive squash suffix marker (a blank line, 9+ hyphens alone on a line, another +// blank line) — an ordinary, non-squash commit whose body happens to contain markdown +// bullets never matches this and is completely unaffected. Within an activated commit, +// each bullet chunk still goes through git's own STRICT per-chunk terminal-block +// algorithm (via `interpret-trailers`), so a mid-chunk MENTION of trailer syntax +// (not at that chunk's own true end) is exactly as inert as it always was — the +// protection is now granular per original commit instead of per whole squashed message, +// never removed. +// Review finding (2026-09-08): a bare `.match()` against this pattern returns the +// FIRST occurrence scanning left-to-right, but GitHub's own appended suffix is always +// the TRUE TAIL of the message — an earlier bullet's own body legitimately using a +// markdown horizontal rule (also `---------`-shaped) before the bullet that actually +// carries the ack would truncate `beforeSuffix` too early, silently excluding the real +// ack bullet from the split/parse scan below and reintroducing the exact bug this +// function exists to fix. `findLastGithubSquashSuffixIndex` below finds the LAST match +// instead, via a global-flag scan (an anchor-and-backtrack trick with a leading +// `[\s\S]*` was tried and rejected: it forces the match's OWN `.index` to 0, which +// breaks the `rawBody.slice(0, match.index)` usage pattern this function needs). +const GITHUB_SQUASH_SUFFIX_RE = /\n\n-{9,}\n\n/g; +const SQUASH_BULLET_SPLIT_RE = /\n\n(?=\* )/; + +/** Index of the START of the LAST GitHub-squash-suffix occurrence in `text`, or -1. */ +function findLastGithubSquashSuffixIndex(text) { + GITHUB_SQUASH_SUFFIX_RE.lastIndex = 0; + let last = -1; + let m; + while ((m = GITHUB_SQUASH_SUFFIX_RE.exec(text)) !== null) { + last = m.index; + // A zero-length match cannot happen for this pattern (it requires literal + // characters), but guard against an infinite loop defensively regardless. + if (m[0].length === 0) GITHUB_SQUASH_SUFFIX_RE.lastIndex += 1; + } + return last; +} +const ACK_TRAILER_LINE_RE = new RegExp( + `^(${escapeRegex(ACK_TRAILER_HASH)}|${escapeRegex(ACK_TRAILER_GROWTH)}):\\s*(.*)$`, +); + +/** + * Recover ack trailers buried mid-message by squash-merge concatenation. Returns + * `{ hash: string[], growth: string[] }` of any ADDITIONAL values found beyond what the + * whole-message `%(trailers:...)` pass in `readAckTrailers` already sees — callers + * merge both into the same value lists before the final `parseAckTrailers` call, so + * this never needs its own dedup or its own result-shape handling. + * + * A commit whose body does not contain GitHub's squash suffix signal is not a squash + * commit by this heuristic and is returned untouched (empty arrays) — this is the sole + * gate against widening false-positive risk to ordinary commits. + */ +function extractSquashBuriedTrailers(rawBody, { cwd, timeoutMs }) { + const suffixIndex = findLastGithubSquashSuffixIndex(rawBody); + if (suffixIndex === -1) return { hash: [], growth: [] }; + const beforeSuffix = rawBody.slice(0, suffixIndex); + const chunks = beforeSuffix.split(SQUASH_BULLET_SPLIT_RE); + const hash = []; + const growth = []; + for (const chunk of chunks) { + // A chunk with no blank-line-separated body (a bare `* subject` bullet, or the + // pre-first-bullet PR-title preamble) cannot carry a trailer block at all — + // skipping it is an optimisation, not a correctness requirement (interpret-trailers + // would just return nothing for it). + if (!chunk.trim()) continue; + let parsed; + try { + parsed = ackTrailerGit(['interpret-trailers', '--parse'], { cwd, timeoutMs, input: chunk }); + } catch { + // A single malformed/unparseable chunk must not abort the whole scan — the + // existing whole-message pass and every OTHER chunk still stand. Fail this + // chunk closed (recover nothing from it), never the whole function. + continue; + } + for (const line of parsed.split('\n')) { + const m = ACK_TRAILER_LINE_RE.exec(line); + if (!m) continue; + if (m[1] === ACK_TRAILER_HASH) hash.push(m[2]); + else growth.push(m[2]); + } + } + return { hash, growth }; +} + // Record/field/value separators for the `git log --format` trailer extraction below. // ASCII control characters (RS/US/GS) so they can never collide with real trailer // content, following the precedent at gsd-core/workflows/ship.md:312 (`%x1f`/`%x1e` for @@ -820,6 +928,30 @@ function readAckTrailers({ baseRef, headRef = 'HEAD', cwd = REPO_ROOT, timeoutMs for (const v of growthField.split(ACK_TRAILER_VALUE_SEP)) if (v !== '') growthValues.push(v); } + // #4454 follow-on: recover any ack trailer the whole-message pass above cannot see + // because a squash-merge concatenated other commits' bodies after it (see + // extractSquashBuriedTrailers's doc comment for the full mechanism and the + // false-positive scoping that keeps this from widening risk on ordinary commits). + // A SEPARATE `%B` read (rather than reusing the format above) because the raw body + // is needed verbatim, including the exact blank-line/hyphen/bullet structure the + // squash-suffix and bullet-boundary regexes key on — `%(trailers:...)` already + // discards everything but the trailers themselves and cannot supply this. + let rawBodies; + try { + rawBodies = ackTrailerGit( + ['log', `${mergeBase}..${headRef}`, `--format=${ACK_TRAILER_RECORD_SEP}%B`], + { cwd, timeoutMs }, + ); + } catch (err) { + throw new Error(`emitted-ack-trailer: could not read commit bodies over the range: ${err.message}`); + } + const bodyRecords = rawBodies.replace(/\r/g, '').split(ACK_TRAILER_RECORD_SEP).slice(1); + for (const body of bodyRecords) { + const recovered = extractSquashBuriedTrailers(body, { cwd, timeoutMs }); + hashValues.push(...recovered.hash); + growthValues.push(...recovered.growth); + } + return parseAckTrailers({ hash: hashValues, growth: growthValues }); }