* 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 <noreply@anthropic.com> * 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 (`* <subject>`) 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 <noreply@anthropic.com> * docs(#4454): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user