From 017abd92361661d814f81617f7c8c619730b0711 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 19:08:08 -0400 Subject: [PATCH] feat(sdk): add --respect-staged opt-in to gsd-sdk query commit --files (#3522) Closes #3522. When --respect-staged is passed the git add loop is skipped entirely so per-hunk staging from git add -p is preserved. Default behavior (full re-stage) is unchanged. The #3061 pathspec invariant holds under both modes. Nothing-staged within scope returns { committed: false, reason: 'nothing staged' } without error. Co-Authored-By: Claude Sonnet 4.6 --- .changeset/kind-foxes-click.md | 5 ++ docs/CLI-TOOLS.md | 4 +- sdk/src/query/QUERY-HANDLERS.md | 2 +- sdk/src/query/commit.test.ts | 121 ++++++++++++++++++++++++++++++++ sdk/src/query/commit.ts | 71 +++++++++++++++---- 5 files changed, 187 insertions(+), 16 deletions(-) create mode 100644 .changeset/kind-foxes-click.md diff --git a/.changeset/kind-foxes-click.md b/.changeset/kind-foxes-click.md new file mode 100644 index 000000000..ad82444f2 --- /dev/null +++ b/.changeset/kind-foxes-click.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3522 +--- +**`gsd-sdk query commit --files` gains `--respect-staged` opt-in** — pass `--respect-staged` to skip the automatic `git add` step and commit only what is already staged within the requested pathspec. Callers using `git add -p` to stage individual hunks no longer have their partial staging silently overwritten. Without the flag, default behavior is unchanged (#3061 invariant preserved under both modes). diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 505b5807d..1732ee3a8 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -422,11 +422,13 @@ node gsd-tools.cjs audit-open [--json] node gsd-tools.cjs from-gsd2 [--path ] [--force] [--dry-run] # Git commit with config checks -node gsd-tools.cjs commit [--files f1 f2] [--amend] [--no-verify] +node gsd-tools.cjs commit [--files f1 f2] [--amend] [--no-verify] [--respect-staged] ``` > `--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) node gsd-tools.cjs websearch [--limit N] [--freshness day|week|month] ``` diff --git a/sdk/src/query/QUERY-HANDLERS.md b/sdk/src/query/QUERY-HANDLERS.md index b6189e3c6..1b936abb6 100644 --- a/sdk/src/query/QUERY-HANDLERS.md +++ b/sdk/src/query/QUERY-HANDLERS.md @@ -298,7 +298,7 @@ Disposition: **Registered** = handled in `createRegistry()` under the listed SDK | `state` (subcommands) | `state.load`, `state.json`, `state.get`, `state.update`, `state.patch`, … | Registered | Dotted and `state …` space aliases in `index.ts`. | | `resolve-model` | `resolve-model` | Registered | | | `find-phase` | `find-phase` | Registered | Golden: subset parity (see above). | -| `commit`, `check-commit`, `commit-to-subrepo` | `commit`, `check-commit`, `commit-to-subrepo` | Registered | | +| `commit`, `check-commit`, `commit-to-subrepo` | `commit`, `check-commit`, `commit-to-subrepo` | Registered | `commit`: `--files` re-stages full files by default; use `--respect-staged` to skip `git add` and commit only what is already staged within the pathspec (#3522). | | `verify-summary` | `verify-summary`, `verify.summary`, `verify summary` | Registered | | | `template` | `template.fill`, `template.select`, … | Registered | | | `frontmatter` | `frontmatter.get`, `frontmatter.set`, … | Registered | | diff --git a/sdk/src/query/commit.test.ts b/sdk/src/query/commit.test.ts index 96606c85a..f5ba9bab9 100644 --- a/sdk/src/query/commit.test.ts +++ b/sdk/src/query/commit.test.ts @@ -339,3 +339,124 @@ describe('commit input validation and option safety (#3061)', () => { expect(committed).toContain(dashName); }); }); + +// ─── --respect-staged flag (#3522) ──────────────────────────────────────── +// +// When --respect-staged is passed: +// 1. The git add loop is skipped entirely — partial (per-hunk) staging is +// preserved rather than overwritten by a full `git add -- `. +// 2. If nothing is staged within the requested pathspec, the handler +// returns { committed: false, reason: 'nothing staged' } without error. +// 3. The #3061 invariant still holds: files staged *outside* --files +// do not appear in the commit (trailing -- pathspec still applied). +// 4. Without --respect-staged, --files re-stages the full file as before +// (back-compat: partial staging is overwritten). + +describe('commit --respect-staged (#3522)', () => { + // Each test needs an existing HEAD so we can create tracked files and + // simulate `git add -p` style partial staging. + 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('commits only the pre-staged file when --respect-staged is passed (simulated git add -p)', async () => { + const { commit } = await import('./commit.js'); + + // Simulate a `git add -p` scenario using two files: + // - staged-hunk.ts: the hunk the operator chose to stage (staged beforehand) + // - unstaged-hunk.ts: the hunk the operator left out of the index + // Both paths are listed in --files. Without --respect-staged the handler + // would git add both, committing all working-tree changes. With + // --respect-staged it must commit only what is already in the index. + await writeFile(join(tmpDir, 'staged-hunk.ts'), '// selected hunk\nconst a = 1;\n'); + await writeFile(join(tmpDir, 'unstaged-hunk.ts'), '// skipped hunk\nconst b = 2;\n'); + + // Stage only the selected hunk — the skipped hunk stays in the work tree. + execSync('git add staged-hunk.ts', { cwd: tmpDir, stdio: 'pipe' }); + + const result = await commit( + ['feat: partial hunk commit', '--files', 'staged-hunk.ts', 'unstaged-hunk.ts', '--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); + // Only the pre-staged hunk must appear in the commit. + expect(committedFiles).toContain('staged-hunk.ts'); + expect(committedFiles).not.toContain('unstaged-hunk.ts'); + }); + + it('returns { committed: false, reason: "nothing staged" } when --respect-staged has nothing staged in pathspec', async () => { + const { commit } = await import('./commit.js'); + + // Create a file but do NOT stage it. + await writeFile(join(tmpDir, 'unstaged.ts'), '// not staged\n'); + + const result = await commit( + ['feat: should not commit', '--files', 'unstaged.ts', '--respect-staged'], + tmpDir, + ); + expect((result.data as { committed: boolean }).committed).toBe(false); + expect((result.data as { reason: string }).reason).toBe('nothing staged'); + }); + + it('files staged outside --files pathspec are excluded under --respect-staged (#3061 invariant)', async () => { + const { commit } = await import('./commit.js'); + + // Stage an "in-scope" file and an "out-of-scope" file. + await writeFile(join(tmpDir, 'in-scope.ts'), '// in scope\n'); + await writeFile(join(tmpDir, 'out-of-scope.ts'), '// out of scope\n'); + execSync('git add in-scope.ts out-of-scope.ts', { cwd: tmpDir, stdio: 'pipe' }); + + // --files only names in-scope.ts. --respect-staged skips git add. + const result = await commit( + ['feat: scoped commit', '--files', 'in-scope.ts', '--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('in-scope.ts'); + expect(committedFiles).not.toContain('out-of-scope.ts'); + + // out-of-scope.ts must remain staged (not committed, not lost). + 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'); + + // Write a file, land it, then modify it in working tree without staging. + await writeFile(join(tmpDir, 'full-file.ts'), '// original\n'); + execSync('git add full-file.ts', { cwd: tmpDir, stdio: 'pipe' }); + execSync('git commit -m "land file"', { cwd: tmpDir, stdio: 'pipe' }); + + // Unstaged modification in the working tree. + await writeFile(join(tmpDir, 'full-file.ts'), '// modified in working tree\n'); + + // Without --respect-staged, --files full-file.ts should run git add and + // stage the full working-tree content before committing. + const result = await commit( + ['feat: full restage', '--files', 'full-file.ts'], + tmpDir, + ); + expect((result.data as { committed: boolean }).committed).toBe(true); + + // The full working-tree change must be in the commit. + const diff = execSync('git show HEAD -- full-file.ts', { cwd: tmpDir, encoding: 'utf-8' }); + expect(diff).toContain('// modified in working tree'); + }); +}); diff --git a/sdk/src/query/commit.ts b/sdk/src/query/commit.ts index 0e494d37a..8d1f702e7 100644 --- a/sdk/src/query/commit.ts +++ b/sdk/src/query/commit.ts @@ -91,7 +91,19 @@ export function sanitizeCommitMessage(text: string): string { * Checks commit_docs config (unless --force), sanitizes message, * stages specified files (or all .planning/), and commits. * - * @param args - args[0]=message, remaining=file paths or flags (--force, --amend, --no-verify) + * By default, `--files ` runs `git add -- ` for each named file + * before committing. This means any per-hunk staging set up via `git add -p` + * is **overwritten** by a full re-stage of the file's working-tree content. + * + * Pass `--respect-staged` to skip the `git add` step entirely. The handler + * will commit only what is already staged within the requested pathspec. If + * nothing is staged within that scope, the handler returns + * `{ committed: false, reason: 'nothing staged' }` without error. The #3061 + * leak-prevention invariant still holds: the trailing `-- ` pathspec + * on the commit ensures files staged outside `--files ` are excluded. + * + * @param args - args[0]=message, remaining=file paths or flags + * (--force, --amend, --no-verify, --respect-staged) * @param projectDir - Project root directory * @returns QueryResult with commit result */ @@ -102,10 +114,11 @@ export const commit: QueryHandler = async (args, projectDir, workstream) => { const hasForce = allArgs.includes('--force'); const hasAmend = allArgs.includes('--amend'); const hasNoVerify = allArgs.includes('--no-verify'); + const hasRespectStaged = allArgs.includes('--respect-staged'); const filesIndex = allArgs.indexOf('--files'); const endIndex = filesIndex !== -1 ? filesIndex : allArgs.length; // CodeRabbit #6: don't strip arbitrary `--foo` tokens from commit messages - const knownFlags = new Set(['--force', '--amend', '--no-verify']); + const knownFlags = new Set(['--force', '--amend', '--no-verify', '--respect-staged']); const messageArgs = allArgs.slice(0, endIndex).filter(a => !knownFlags.has(a)); const message = messageArgs.join(' ') || undefined; const filePaths = @@ -142,30 +155,60 @@ export const commit: QueryHandler = async (args, projectDir, workstream) => { // 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 } }; + + // When --respect-staged is set, skip re-staging so that per-hunk staging + // from `git add -p` is preserved. Without the flag, run git add for each + // path (default behavior, back-compat). + if (!hasRespectStaged) { + 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 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) : []; + // When --respect-staged is set the caller may list paths that git doesn't + // know yet (e.g. untracked files the operator intentionally skipped via + // `git add -p`). Passing those unknown paths directly as a git pathspec + // 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[] = (() => { + if (hasRespectStaged) { + const allStaged = execGit(projectDir, ['diff', '--cached', '--name-only']); + 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 diffResult = execGit(projectDir, ['diff', '--cached', '--name-only', '--', ...pathsToCommit]); + return diffResult.stdout ? diffResult.stdout.split('\n').filter(Boolean) : []; + })(); if (stagedFiles.length === 0) { return { data: { committed: false, reason: 'nothing staged' } }; } - // 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. + // Build commit command. The trailing pathspec ensures the commit captures + // only files within the requested scope (#3061 invariant). + // + // For the default path: use `pathsToCommit` (the caller's requested scope). + // For --respect-staged: use `stagedFiles` (the exact set already in the + // index within scope). This avoids git rejecting the commit when the caller + // listed paths that are not yet known to git (e.g. skipped hunks from + // `git add -p` that were intentionally left unstaged). + const commitPathspec = hasRespectStaged ? stagedFiles : pathsToCommit; const commitArgs: string[] = hasAmend ? ['commit', '--amend', '--no-edit'] : ['commit', '-m', sanitized ?? '']; if (hasNoVerify) commitArgs.push('--no-verify'); - commitArgs.push('--', ...pathsToCommit); + commitArgs.push('--', ...commitPathspec); const commitResult = execGit(projectDir, commitArgs); if (commitResult.exitCode !== 0) {