From 85ef9553d210a7c703c1a8b0d57129c42f12a3d2 Mon Sep 17 00:00:00 2001 From: Nicholas Ferrer Date: Tue, 5 May 2026 15:30:05 -0300 Subject: [PATCH] fix(commit): scope every commit call to its staged pathspec The commit handler ran `git add ` followed by `git commit` without a pathspec, so anything pre-staged externally before the handler ran was swept into the commit. #2767 fixed every call site to use --files but left the handler emitting a pathspec-less commit, so the bug survived the well-formed form too. Compute pathsToCommit once and pass `'--', ...pathsToCommit` to every git commit invocation: regular, --amend, and commit-to-subrepo. The staged-files check uses the same pathspec so "nothing staged" reflects what would actually be committed, not unrelated index entries. Two follow-up safeguards on the same surface: * When `--files` is passed but every following token gets filtered out (e.g. `--files --no-verify`), reject with `--files requires at least one path` instead of silently falling back to .planning/. * Both `git add` invocations now use the `--` separator so a path starting with `-` (e.g. a file literally named `-A.md`) is treated as a pathspec rather than a git option. Adds five regression tests in `commit.test.ts`: three covering the pathspec scope (`--files`, `.planning/` fallback, and `--amend` with pre-staged unrelated changes), one covering the empty `--files` rejection, and one covering the `-A.md` round-trip. Closes #3061 --- .changeset/gentle-birds-caper.md | 5 ++ sdk/src/query/commit.test.ts | 139 +++++++++++++++++++++++++++++++ sdk/src/query/commit.ts | 35 ++++++-- 3 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 .changeset/gentle-birds-caper.md diff --git a/.changeset/gentle-birds-caper.md b/.changeset/gentle-birds-caper.md new file mode 100644 index 000000000..d64708aec --- /dev/null +++ b/.changeset/gentle-birds-caper.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3106 +--- +**`gsd-sdk query commit` is now scoped to its own staged paths.** Pre-staged unrelated index entries (for example a prior `git rm`) no longer leak into the commit alongside the files passed via `--files`. The same scope guarantee now applies to the `.planning/` fallback, `--amend`, and `commit-to-subrepo`. diff --git a/sdk/src/query/commit.test.ts b/sdk/src/query/commit.test.ts index 66c3c1859..96606c85a 100644 --- a/sdk/src/query/commit.test.ts +++ b/sdk/src/query/commit.test.ts @@ -200,3 +200,142 @@ describe('checkCommit', () => { expect((result.data as { can_commit: boolean }).can_commit).toBe(true); }); }); + +// ─── pathspec scope regression (#3061) ──────────────────────────────────── +// +// The handler must commit only the paths it staged itself, even when the +// caller's git index already had unrelated entries staged before the call. +// Before the fix, `git commit` ran without a pathspec and swept those +// pre-staged entries into the commit alongside the requested files. + +describe('commit pathspec scope (#3061)', () => { + // Each test needs an existing HEAD so we can pre-stage a deletion against it. + beforeEach(async () => { + await writeFile(join(tmpDir, 'README.md'), 'init\n'); + execSync('git add README.md', { cwd: tmpDir, stdio: 'pipe' }); + execSync('git commit -m "init"', { cwd: tmpDir, stdio: 'pipe' }); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: true }), + ); + }); + + it('--files commits only the named paths when an unrelated change is pre-staged', async () => { + const { commit } = await import('./commit.js'); + await writeFile(join(tmpDir, '.planning', 'STATE.md'), '# State\n'); + + // Operator scenario from the issue: a `git rm` is already in the index + // before the workflow's commit step runs. + execSync('git rm README.md', { cwd: tmpDir, stdio: 'pipe' }); + + const result = await commit(['docs: state only', '--files', '.planning/STATE.md'], tmpDir); + expect((result.data as { committed: boolean }).committed).toBe(true); + + const committed = execSync('git show --name-only --format= HEAD', { cwd: tmpDir, encoding: 'utf-8' }) + .trim() + .split('\n'); + expect(committed).toContain('.planning/STATE.md'); + expect(committed).not.toContain('README.md'); + + // The pre-staged deletion must remain staged-but-uncommitted. + const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }); + expect(status).toMatch(/^D {2}README\.md/m); + }); + + it('.planning/ fallback commits only planning paths when an unrelated change is pre-staged', async () => { + const { commit } = await import('./commit.js'); + await writeFile(join(tmpDir, '.planning', 'STATE.md'), '# State\n'); + + execSync('git rm README.md', { cwd: tmpDir, stdio: 'pipe' }); + + const result = await commit(['docs: planning'], tmpDir); + expect((result.data as { committed: boolean }).committed).toBe(true); + + const committed = execSync('git show --name-only --format= HEAD', { cwd: tmpDir, encoding: 'utf-8' }) + .trim() + .split('\n'); + expect(committed).not.toContain('README.md'); + expect(committed.some(f => f.startsWith('.planning/'))).toBe(true); + + const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }); + expect(status).toMatch(/^D {2}README\.md/m); + }); + + it('--amend with --files keeps the amend within the named pathspec', async () => { + const { commit } = await import('./commit.js'); + + // Land an initial planning commit to amend, and assert the setup landed. + // If it silently failed the amend would target the wrong HEAD and the + // assertions below would still pass for the wrong reason. + await writeFile(join(tmpDir, '.planning', 'STATE.md'), '# State v1\n'); + const setup = await commit(['docs: initial state', '--files', '.planning/STATE.md'], tmpDir); + expect((setup.data as { committed: boolean }).committed).toBe(true); + + // Modify STATE.md, then pre-stage an unrelated change before amending. + await writeFile(join(tmpDir, '.planning', 'STATE.md'), '# State v2\n'); + execSync('git rm README.md', { cwd: tmpDir, stdio: 'pipe' }); + + const result = await commit(['docs: amended', '--amend', '--files', '.planning/STATE.md'], tmpDir); + expect((result.data as { committed: boolean }).committed).toBe(true); + + const committed = execSync('git show --name-only --format= HEAD', { cwd: tmpDir, encoding: 'utf-8' }) + .trim() + .split('\n'); + expect(committed).toContain('.planning/STATE.md'); + expect(committed).not.toContain('README.md'); + + const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }); + expect(status).toMatch(/^D {2}README\.md/m); + }); +}); + +// ─── input validation and option-injection safety (#3061 follow-ups) ────── +// +// Two guards that travel with the pathspec rewrite: +// 1. --files with no usable paths fails fast instead of falling back to +// .planning/, which would silently swap the caller's intended scope. +// 2. Every git add invocation uses the `--` separator so a path that +// starts with `-` is treated as a pathspec rather than an option. + +describe('commit input validation and option safety (#3061)', () => { + beforeEach(async () => { + await writeFile(join(tmpDir, 'README.md'), 'init\n'); + execSync('git add README.md', { cwd: tmpDir, stdio: 'pipe' }); + execSync('git commit -m "init"', { cwd: tmpDir, stdio: 'pipe' }); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: true }), + ); + }); + + it('--files with no usable paths is rejected instead of silently using .planning/', async () => { + const { commit } = await import('./commit.js'); + // Drop a planning change that the .planning/ fallback would otherwise pick up. + await writeFile(join(tmpDir, '.planning', 'STATE.md'), '# State\n'); + + const result = await commit(['msg', '--files', '--no-verify'], tmpDir); + expect((result.data as { committed: boolean }).committed).toBe(false); + expect((result.data as { reason: string }).reason).toContain('--files requires at least one path'); + + // The handler must not have staged anything: if it had silently fallen + // back to .planning/, STATE.md would now show up in the staged list. + const stagedAfter = execSync('git diff --cached --name-only', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + expect(stagedAfter).toBe(''); + }); + + it('stages a file whose name starts with "-" instead of misparsing it as a git option', async () => { + const { commit } = await import('./commit.js'); + // A filename like `-A.md` is the canonical option-injection trap: + // without the `--` separator, `git add -A.md` would be parsed as a flag. + const dashName = '-A.md'; + await writeFile(join(tmpDir, dashName), 'dash content\n'); + + const result = await commit(['feat: add dash file', '--files', dashName], tmpDir); + expect((result.data as { committed: boolean }).committed).toBe(true); + + const committed = execSync('git show --name-only --format= HEAD', { cwd: tmpDir, encoding: 'utf-8' }) + .trim() + .split('\n'); + expect(committed).toContain(dashName); + }); +}); diff --git a/sdk/src/query/commit.ts b/sdk/src/query/commit.ts index 6a714cbb8..0e494d37a 100644 --- a/sdk/src/query/commit.ts +++ b/sdk/src/query/commit.ts @@ -132,27 +132,40 @@ export const commit: QueryHandler = async (args, projectDir, workstream) => { // Sanitize message const sanitized = message ? sanitizeCommitMessage(message) : message; - // Stage files - const filesToStage = filePaths.length > 0 ? filePaths : ['.planning/']; - for (const file of filesToStage) { - const addResult = execGit(projectDir, ['add', file]); + // If --files was passed explicitly, the caller asked for an explicit scope. + // Falling back to .planning/ when every following token got filtered out + // would silently swap the requested scope, so reject the call instead. + if (filesIndex !== -1 && filePaths.length === 0) { + return { data: { committed: false, reason: '--files requires at least one path' } }; + } + + // Compute pathspec once: the handler commits exactly the paths it staged, + // never anything that was pre-staged externally (#3061). + const pathsToCommit = filePaths.length > 0 ? filePaths : ['.planning/']; + for (const file of pathsToCommit) { + // The `--` separator keeps any path that starts with `-` from being + // interpreted as a git option (e.g. a file literally named `-A`). + const addResult = execGit(projectDir, ['add', '--', file]); if (addResult.exitCode !== 0) { return { data: { committed: false, reason: addResult.stderr || `failed to stage ${file}`, exitCode: addResult.exitCode } }; } } - // Check if anything is staged - const diffResult = execGit(projectDir, ['diff', '--cached', '--name-only']); + // Check if anything is staged within the pathspec we're about to commit. + const diffResult = execGit(projectDir, ['diff', '--cached', '--name-only', '--', ...pathsToCommit]); const stagedFiles = diffResult.stdout ? diffResult.stdout.split('\n').filter(Boolean) : []; if (stagedFiles.length === 0) { return { data: { committed: false, reason: 'nothing staged' } }; } - // Build commit command + // Build commit command. The trailing `-- pathsToCommit` ensures the commit + // captures only files within the requested scope, even when the caller's + // index already had unrelated entries staged before this handler ran. const commitArgs: string[] = hasAmend ? ['commit', '--amend', '--no-edit'] : ['commit', '-m', sanitized ?? '']; if (hasNoVerify) commitArgs.push('--no-verify'); + commitArgs.push('--', ...pathsToCommit); const commitResult = execGit(projectDir, commitArgs); if (commitResult.exitCode !== 0) { @@ -276,13 +289,17 @@ export const commitToSubrepo: QueryHandler = async (args, projectDir, workstream } const fileArgs = files.length > 0 ? files : ['.']; - const addResult = spawnSync('git', ['-C', projectDir, 'add', ...fileArgs], { stdio: 'pipe', encoding: 'utf-8' }); + // The `--` separator keeps any path that starts with `-` from being + // interpreted as a git option (e.g. a file literally named `-A`). + const addResult = spawnSync('git', ['-C', projectDir, 'add', '--', ...fileArgs], { stdio: 'pipe', encoding: 'utf-8' }); if (addResult.status !== 0) { return { data: { committed: false, reason: addResult.stderr || 'git add failed' } }; } + // Pathspec on the commit keeps the scope identical to what was just staged, + // so any pre-staged external changes do not leak in (#3061). const commitResult = spawnSync( - 'git', ['-C', projectDir, 'commit', '-m', sanitized], + 'git', ['-C', projectDir, 'commit', '-m', sanitized, '--', ...fileArgs], { stdio: 'pipe', encoding: 'utf-8' }, ); if (commitResult.status !== 0) {