From e4dd0cbdd5ccbfb2f553dc3f161c810ced370da4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 25 Jul 2026 01:50:36 -0400 Subject: [PATCH] fix(#2523): normalize --files to repo-relative; reject out-of-repo; gate push on git-add exit (#2638) * test(#2523): absolute + mixed + out-of-repo --files paths * fix(#2523): normalize --files to repo-relative; reject out-of-repo; gate push on git-add exit * chore(#2523): backfill changeset pr to 2638 --- .changeset/lively-finches-leap.md | 5 +++ src/commands.cts | 35 ++++++++++++++-- tests/commit-files-pathspec.test.cjs | 62 ++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 .changeset/lively-finches-leap.md diff --git a/.changeset/lively-finches-leap.md b/.changeset/lively-finches-leap.md new file mode 100644 index 000000000..9246fbc95 --- /dev/null +++ b/.changeset/lively-finches-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2638 +--- +**`query commit --files` now accepts absolute paths** — `cmdCommit` used `path.join(cwd, file)`, which concatenates instead of resetting on an absolute path, so absolute `--files` entries (e.g. the absolute `phase_dir` emitted by `init phase-op` since #2428) were joined to `cwd+absPath` (non-existent) and silently dropped as `nothing_to_commit` — and a mixed relative/absolute list committed the relative entries while reporting `committed:true`. Absolute paths are now normalized to repo-relative before staging/branch-detection, so they commit correctly and the phase-branch detection no longer matches digit-hyphen runs in the absolute prefix. (#2523) diff --git a/src/commands.cts b/src/commands.cts index cdf2f4bde..bdb4b8310 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -769,6 +769,28 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u return; } + // Normalize --files to repo-relative paths (#2523). path.join(cwd, absPath) + // concatenates instead of resetting (path.resolve does), so absolute phase_dir + // paths emitted by init phase-op (#2428) were joined to cwd+absPath (non-existent) + // and silently dropped by the existence check — and a mixed relative/absolute list + // then committed the relative entries while reporting committed:true. Relative-izing + // also keeps the phase-branch detection below from matching digit-hyphen runs in the + // absolute prefix (e.g. /proj-7-decoy2/ → wrong phase branch). + const filesRel = files && files.length > 0 + ? files.map(f => toPosixPath(path.relative(cwd, path.resolve(cwd, f)))) + : []; + // Reject --files entries resolving OUTSIDE the project root: fail loudly rather + // than silently skip or reach `git add` (which rejects them and would pollute the + // index via an unconditional stagedPaths.push) (#2523). `toPosixPath` only flips + // backslashes, so a `..` escape is still detectable. + for (const rel of filesRel) { + if (rel.startsWith('..') || path.isAbsolute(rel)) { + const result = { committed: false, hash: null, reason: 'path_outside_repo', path: rel }; + output(result, raw, 'failed'); + return; + } + } + // Ensure branching strategy branch exists before first commit (#1278). // Pre-execution workflows (discuss, plan, research) commit artifacts but the branch // was previously only created during execute-phase — too late. @@ -777,7 +799,7 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u let branchName: string | null = null; if (branchingStrategy === 'phase') { // Determine which phase we're committing for from the file paths - const phaseMatch = (files || []).join(' ').match(/(\d+(?:\.\d+)*)-/); + const phaseMatch = (filesRel || []).join(' ').match(/(\d+(?:\.\d+)*)-/); if (phaseMatch) { const phaseNum = phaseMatch[1]; const phaseInfo = findPhaseInternal(cwd, phaseNum) as Record | null; @@ -809,7 +831,7 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // Stage files const explicitFiles = files && files.length > 0; - const filesToStage = explicitFiles ? files : ['.planning/']; + const filesToStage = explicitFiles ? filesRel : ['.planning/']; const stagedPaths: string[] = []; for (const file of filesToStage) { const fullPath = path.join(cwd, file); @@ -824,8 +846,13 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // removed planning files are not left dangling in the index. execGit(['rm', '--cached', '--ignore-unmatch', file], { cwd }); } else { - execGit(['add', file], { cwd }); - stagedPaths.push(file); + const addResult = execGit(['add', file], { cwd }); + // Only record paths that actually staged — a failed `git add` (permissions, + // out-of-repo edge) must not enter the commit pathspec (#2523). Mirrors + // cmdCommitToSubrepo's exitCode-gated push. + if (addResult.exitCode === 0) { + stagedPaths.push(file); + } } } diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index 488934209..93c532da1 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -173,4 +173,66 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { 'extra.txt should remain staged, not absorbed into a commit', ); }); + + 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) + // → silently skipped as nothing_to_commit (#2523). + fs.writeFileSync(path.join(tmpDir, '.planning', 'A.md'), 'a\n'); + const absPath = path.join(tmpDir, '.planning', 'A.md'); + const res = runGsdTools(['commit', 'docs: abs path', '--files', absPath], tmpDir); + const parsed = JSON.parse(res.output); + assert.strictEqual(parsed.committed, true, `absolute path must commit, not nothing_to_commit: ${res.output}`); + + // The absolute path must land in the commit, normalized to repo-relative. + const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + assert.strictEqual(diff, '.planning/A.md', `absolute --files path must be committed (normalized to relative); got: ${diff}`); + }); + + test('#2523: mixed relative+absolute --files list commits BOTH (no silent partial commit)', () => { + // The sharpest symptom: a mixed list committed the relative entry, dropped the + // absolute one, and reported committed:true (#2523). Both must land. + fs.writeFileSync(path.join(tmpDir, '.planning', 'REL.md'), 'r\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ABS.md'), 'a\n'); + const absPath = path.join(tmpDir, '.planning', 'ABS.md'); + const res = runGsdTools( + ['commit', 'docs: mixed', '--files', '.planning/REL.md', absPath], + tmpDir, + ); + const parsed = JSON.parse(res.output); + assert.strictEqual(parsed.committed, true, `mixed list must commit: ${res.output}`); + + const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' }) + .trim().split('\n').sort(); + assert.deepStrictEqual( + diff, + ['.planning/ABS.md', '.planning/REL.md'], + `mixed relative+absolute list must commit BOTH entries (the bug dropped the absolute one); got: ${diff.join(',')}`, + ); + }); + + test('#2523: out-of-repo --files path is rejected loudly (path_outside_repo), no index pollution', (t) => { + // An absolute path resolving OUTSIDE the project root must fail loudly, not + // silently skip and not pollute the index via a failed git add (#2523). + const outsideDir = path.join(tmpDir, '..', `gsd-2523-outside-${process.pid}-${Date.now()}`); + fs.mkdirSync(outsideDir, { recursive: true }); + t.after(() => cleanup(outsideDir)); + const outsideFile = path.join(outsideDir, 'secret.md'); + fs.writeFileSync(outsideFile, 's\n'); + + const res = runGsdTools( + ['commit', 'docs: outside', '--files', path.resolve(outsideFile)], + tmpDir, + ); + const parsed = JSON.parse(res.output); + assert.strictEqual(parsed.committed, false, 'out-of-repo path must not commit'); + assert.strictEqual(parsed.reason, 'path_outside_repo', `out-of-repo path must fail loudly: ${res.output}`); + + // No new commit created (still at the single initial commit). + const logCount = execSync('git rev-list --count HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + assert.strictEqual(logCount, '1', 'no new commit must be created for an out-of-repo path'); + // Index stays clean (no pollution from a failed git add). + const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + assert.strictEqual(status, '', `index must be clean (no pollution): ${status}`); + }); });