From 585e417f9ed4aec62cac0ce68f9379fb9ad6f67f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 19:44:04 -0400 Subject: [PATCH] fix: harden respect-staged pathspec handling --- docs/CLI-TOOLS.md | 1 - sdk/src/query/commit.test.ts | 23 ++++++++++++++++++++++ sdk/src/query/commit.ts | 38 ++++++++++++++++++++++++++++-------- 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 1732ee3a8..5aed2b046 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -426,7 +426,6 @@ node gsd-tools.cjs commit [--files f1 f2] [--amend] [--no-verify] [--r ``` > `--no-verify`: Skips pre-commit hooks. Used by parallel executor agents during wave-based execution to avoid build lock contention (e.g., cargo lock fights in Rust projects). The orchestrator runs hooks once after each wave completes. Do not use `--no-verify` during sequential execution — let hooks run normally. - > `--files ` **staging behaviour**: by default, `--files` runs `git add -- ` for each named file before committing. This overwrites any per-hunk staging set up via `git add -p`. Pass `--respect-staged` to skip the `git add` step and commit only what is already in the index within the requested pathspec. If nothing is staged within that scope, the command returns `{ committed: false, reason: 'nothing staged' }` without error. The trailing `-- ` pathspec on the commit is applied under both modes, so files staged outside the `--files` scope are never included (#3061 invariant). # Web search (requires Brave API key) diff --git a/sdk/src/query/commit.test.ts b/sdk/src/query/commit.test.ts index f5ba9bab9..2ee12a22e 100644 --- a/sdk/src/query/commit.test.ts +++ b/sdk/src/query/commit.test.ts @@ -436,6 +436,29 @@ describe('commit --respect-staged (#3522)', () => { expect(staged).toContain('out-of-scope.ts'); }); + it('treats directory pathspecs without trailing slash as directories under --respect-staged', async () => { + const { commit } = await import('./commit.js'); + + await writeFile(join(tmpDir, 'out-of-scope.ts'), '// out of scope\n'); + execSync('git add .planning/config.json out-of-scope.ts', { cwd: tmpDir, stdio: 'pipe' }); + + const result = await commit( + ['feat: dir scope', '--files', '.planning', '--respect-staged'], + tmpDir, + ); + expect((result.data as { committed: boolean }).committed).toBe(true); + + const committedFiles = execSync('git show --name-only --format= HEAD', { cwd: tmpDir, encoding: 'utf-8' }) + .trim() + .split('\n') + .filter(Boolean); + expect(committedFiles).toContain('.planning/config.json'); + expect(committedFiles).not.toContain('out-of-scope.ts'); + + const staged = execSync('git diff --cached --name-only', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + expect(staged).toContain('out-of-scope.ts'); + }); + it('without --respect-staged, --files re-stages the full file even when partially pre-staged (back-compat)', async () => { const { commit } = await import('./commit.js'); diff --git a/sdk/src/query/commit.ts b/sdk/src/query/commit.ts index 8d1f702e7..75647a285 100644 --- a/sdk/src/query/commit.ts +++ b/sdk/src/query/commit.ts @@ -177,20 +177,42 @@ export const commit: QueryHandler = async (args, projectDir, workstream) => { // causes `git diff --cached` to exit non-zero with "pathspec did not match". // To avoid that, get all staged files and filter by the requested paths in // TypeScript instead. - const stagedFiles: string[] = (() => { + const stagedFilesResult: { files: string[] } | { error: { reason: string; exitCode: number } } = (() => { if (hasRespectStaged) { const allStaged = execGit(projectDir, ['diff', '--cached', '--name-only']); + if (allStaged.exitCode !== 0) { + return { + error: { + reason: allStaged.stderr || allStaged.stdout || 'failed to inspect staged files', + exitCode: allStaged.exitCode, + }, + }; + } const allStagedFiles = allStaged.stdout ? allStaged.stdout.split('\n').filter(Boolean) : []; - // Build a Set of the requested paths for O(1) lookup. - const pathSet = new Set(pathsToCommit); - // For directory entries (e.g. '.planning/'), match by prefix. - return allStagedFiles.filter(f => - pathSet.has(f) || pathsToCommit.some(p => p.endsWith('/') && f.startsWith(p)), - ); + const normalizePathspec = (p: string) => p.replace(/\\/g, '/').replace(/\/+$/, ''); + const normalizedSpecs = pathsToCommit.map(normalizePathspec); + return { + files: allStagedFiles.filter(file => { + const normalizedFile = normalizePathspec(file); + return normalizedSpecs.some(spec => normalizedFile === spec || normalizedFile.startsWith(`${spec}/`)); + }), + }; } const diffResult = execGit(projectDir, ['diff', '--cached', '--name-only', '--', ...pathsToCommit]); - return diffResult.stdout ? diffResult.stdout.split('\n').filter(Boolean) : []; + if (diffResult.exitCode !== 0) { + return { + error: { + reason: diffResult.stderr || diffResult.stdout || 'failed to inspect staged files', + exitCode: diffResult.exitCode, + }, + }; + } + return { files: diffResult.stdout ? diffResult.stdout.split('\n').filter(Boolean) : [] }; })(); + if ('error' in stagedFilesResult) { + return { data: { committed: false, ...stagedFilesResult.error } }; + } + const stagedFiles = stagedFilesResult.files; if (stagedFiles.length === 0) { return { data: { committed: false, reason: 'nothing staged' } }; }