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) {