From 5bfe3ea115987845495b2980f3a51afbab4a0e90 Mon Sep 17 00:00:00 2001 From: alamb-hex Date: Mon, 10 Aug 2026 16:52:22 -0400 Subject: [PATCH 1/4] fix(git-commit): validate scoped `files`, fix deletion/empty-check bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hardens the WIP scoped-staging change to git-commit: - validate body.files strictly (reject absolute paths, .., non-string entries) with 400s instead of silently dropping bad entries - 400 naming any explicit file that doesn't exist and isn't a known git-tracked deletion, instead of a raw `git add` failure - fix existsSync-based fallback filtering incorrectly treating deleted lockfiles as absent (git status now consulted for deletions) - fix the "nothing to commit" check swallowing real `git diff --cached` failures as if they were staged changes (now checks exit code 1 specifically) - revert the no-files fallback to the original `git add -A` so the four existing callers (patches page, security accordion, project detail's git panel, MCP git_commit) keep working exactly as before; none of them send `files` yet, so activating scoped staging for the patch auto-commit flows is a follow-up Adds route.test.ts (16 cases) mocking child_process via execFile's promisify.custom symbol — no real git commands run in tests. --- .../projects/[id]/git-commit/route.test.ts | 325 ++++++++++++++++++ src/app/api/projects/[id]/git-commit/route.ts | 177 +++++++++- 2 files changed, 486 insertions(+), 16 deletions(-) create mode 100644 src/app/api/projects/[id]/git-commit/route.test.ts diff --git a/src/app/api/projects/[id]/git-commit/route.test.ts b/src/app/api/projects/[id]/git-commit/route.test.ts new file mode 100644 index 0000000..65e9e19 --- /dev/null +++ b/src/app/api/projects/[id]/git-commit/route.test.ts @@ -0,0 +1,325 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// `route.ts` does `promisify(execFile)` once at module load time. Node's +// `child_process.execFile` normally advertises a custom promisify +// implementation via the well-known `nodejs.util.promisify.custom` symbol; +// we replicate that hookup on our mock so `promisify(execFile)` resolves +// through `mockExecFileAsync` instead of trying to spawn a real process. +// This guarantees no real git commands ever run against a real project. +const mockExecFileAsync = vi.hoisted(() => vi.fn()); + +vi.mock('child_process', () => { + const execFile: unknown = vi.fn((...args: unknown[]) => { + const cb = args[args.length - 1]; + if (typeof cb === 'function') { + (cb as (err: Error) => void)( + new Error('execFile called directly in test — expected promisify(execFile) path') + ); + } + }); + Object.defineProperty(execFile, Symbol.for('nodejs.util.promisify.custom'), { + value: (...args: unknown[]) => mockExecFileAsync(...args), + }); + return { execFile }; +}); + +vi.mock('@/lib/config', () => ({ getProject: vi.fn() })); +vi.mock('@/lib/logger', () => ({ + logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }, +})); +vi.mock('fs', () => ({ existsSync: vi.fn() })); + +import { getProject } from '@/lib/config'; +import { existsSync } from 'fs'; +import { POST } from './route'; + +const PROJECT = { id: 'proj-a', name: 'proj-a', path: '/repos/proj-a' }; + +function makeRequest(body: unknown) { + return { + json: async () => body, + } as unknown as Parameters[0]; +} + +function makeParams(id = 'proj-a') { + return { params: Promise.resolve({ id }) }; +} + +/** Configure mockExecFileAsync to answer a scripted sequence of git calls. */ +function scriptGit( + handlers: Record Promise<{ stdout: string; stderr: string }>> +) { + mockExecFileAsync.mockImplementation( + async (cmd: string, args: string[]) => { + if (cmd !== 'git') throw new Error(`unexpected command: ${cmd}`); + const [sub] = args; + const handler = handlers[sub]; + if (!handler) { + throw new Error(`no handler configured for git ${sub} (${args.join(' ')})`); + } + return handler(args); + } + ); +} + +function gitError(code: number, message = 'git failed') { + const err = new Error(message) as Error & { code: number }; + err.code = code; + return err; +} + +const OK = { stdout: '', stderr: '' }; + +describe('POST /api/projects/[id]/git-commit', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getProject).mockReturnValue(PROJECT as ReturnType); + }); + + it('404s when the project does not exist', async () => { + vi.mocked(getProject).mockReturnValue(undefined); + const res = await POST(makeRequest({ message: 'x' }), makeParams()); + expect(res.status).toBe(404); + }); + + it('400s when message is missing', async () => { + const res = await POST(makeRequest({}), makeParams()); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/message/i); + }); + + describe('fallback staging (no `files` in body)', () => { + it('runs `git add -A` — preserving prior behavior for callers that do not send `files`', async () => { + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); // staged changes present + }, + commit: async () => ({ stdout: 'commit ok', stderr: '' }), + }); + + const res = await POST(makeRequest({ message: 'chore: bump deps' }), makeParams()); + const data = await res.json(); + + expect(data.success).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith( + 'git', + ['add', '-A'], + expect.objectContaining({ cwd: PROJECT.path }) + ); + }); + }); + + describe('explicit `files` list', () => { + it('stages exactly the named files via `git add --`', async () => { + vi.mocked(existsSync).mockReturnValue(true); + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => ({ stdout: 'commit ok', stderr: '' }), + }); + + const res = await POST( + makeRequest({ message: 'chore: bump lodash', files: ['package.json', 'pnpm-lock.yaml'] }), + makeParams() + ); + const data = await res.json(); + + expect(data.success).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith( + 'git', + ['add', '--', 'package.json', 'pnpm-lock.yaml'], + expect.objectContaining({ cwd: PROJECT.path }) + ); + }); + + it('stages a deleted lockfile that no longer exists on disk (deletion, not silently skipped)', async () => { + // pnpm-lock.yaml was removed (e.g. switching package managers) so it + // is absent from disk, but `git status --porcelain` still reports it. + vi.mocked(existsSync).mockImplementation((p) => !String(p).endsWith('pnpm-lock.yaml')); + scriptGit({ + status: async () => ({ stdout: ' D pnpm-lock.yaml\n', stderr: '' }), + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => ({ stdout: 'commit ok', stderr: '' }), + }); + + const res = await POST( + makeRequest({ message: 'chore: switch package manager', files: ['pnpm-lock.yaml'] }), + makeParams() + ); + const data = await res.json(); + + expect(data.success).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith( + 'git', + ['add', '--', 'pnpm-lock.yaml'], + expect.objectContaining({ cwd: PROJECT.path }) + ); + }); + + it('400s naming a file that does not exist and is not a known deletion', async () => { + vi.mocked(existsSync).mockReturnValue(false); + scriptGit({ + status: async () => ({ stdout: '', stderr: '' }), // git knows nothing about it either + }); + + const res = await POST( + makeRequest({ message: 'x', files: ['does-not-exist.json'] }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toContain('does-not-exist.json'); + expect(mockExecFileAsync).not.toHaveBeenCalledWith( + 'git', + expect.arrayContaining(['add']), + expect.anything() + ); + }); + + it('400s on an absolute path', async () => { + const res = await POST( + makeRequest({ message: 'x', files: ['/etc/passwd'] }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/relative/i); + }); + + it('400s on a path containing ".."', async () => { + const res = await POST( + makeRequest({ message: 'x', files: ['../../etc/passwd'] }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/relative/i); + }); + + it('400s on a path that traverses out of the project via a nested segment', async () => { + const res = await POST( + makeRequest({ message: 'x', files: ['sub/../../outside.txt'] }), + makeParams() + ); + expect(res.status).toBe(400); + expect(res.status).toBe(400); + }); + + it('400s on a non-string entry (e.g. a nested array) instead of silently dropping it', async () => { + const res = await POST( + makeRequest({ message: 'x', files: ['package.json', ['../../etc/passwd']] }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/string/i); + }); + + it('400s on a non-array `files` value', async () => { + const res = await POST( + makeRequest({ message: 'x', files: 'package.json' }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/array/i); + }); + + it('400s on an empty `files` array rather than silently falling back', async () => { + const res = await POST(makeRequest({ message: 'x', files: [] }), makeParams()); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/empty/i); + }); + }); + + describe('nothing to commit', () => { + it('returns success:false without committing when nothing is staged', async () => { + scriptGit({ + add: async () => OK, + diff: async () => OK, // exit 0 = no staged differences + }); + + const res = await POST(makeRequest({ message: 'x' }), makeParams()); + const data = await res.json(); + + expect(data).toEqual({ success: false, error: 'No changes to commit' }); + expect(mockExecFileAsync).not.toHaveBeenCalledWith( + 'git', + expect.arrayContaining(['commit']), + expect.anything() + ); + }); + + it('treats a `git diff --cached` failure (not exit 1) as a real error, not "changes present"', async () => { + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(128, 'fatal: not a git repository'); + }, + }); + + const res = await POST(makeRequest({ message: 'x' }), makeParams()); + expect(res.status).toBe(500); + expect(mockExecFileAsync).not.toHaveBeenCalledWith( + 'git', + expect.arrayContaining(['commit']), + expect.anything() + ); + }); + }); + + describe('successful commit', () => { + it('commits and reports success, logging source/advisories metadata', async () => { + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => ({ stdout: '[main abc1234] chore: fix', stderr: '' }), + }); + + const res = await POST( + makeRequest({ + message: 'chore: fix cve', + source: 'cve-lite', + advisories: ['GHSA-1234'], + }), + makeParams() + ); + const data = await res.json(); + + expect(res.status).toBe(200); + expect(data).toEqual({ success: true, output: '[main abc1234] chore: fix' }); + expect(mockExecFileAsync).toHaveBeenCalledWith( + 'git', + ['commit', '-m', 'chore: fix cve'], + expect.objectContaining({ cwd: PROJECT.path }) + ); + }); + + it('surfaces the commit failure as a 500', async () => { + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => { + throw new Error('commit hook rejected'); + }, + }); + + const res = await POST(makeRequest({ message: 'x' }), makeParams()); + expect(res.status).toBe(500); + const data = await res.json(); + expect(data.error).toMatch(/commit hook rejected/); + }); + }); +}); diff --git a/src/app/api/projects/[id]/git-commit/route.ts b/src/app/api/projects/[id]/git-commit/route.ts index fe14350..ed8359e 100644 --- a/src/app/api/projects/[id]/git-commit/route.ts +++ b/src/app/api/projects/[id]/git-commit/route.ts @@ -3,9 +3,55 @@ import { getProject } from '@/lib/config'; import { execFile } from 'child_process'; import { promisify } from 'util'; import { logger } from '@/lib/logger'; +import { existsSync } from 'fs'; +import { isAbsolute, join, relative } from 'path'; const execFileAsync = promisify(execFile); +/** + * Validate that a caller-supplied relative path is safe to hand to `git add`: + * a non-empty string, not absolute, no `.`/`..` path segments, and (after + * joining onto the project directory) still resolves inside it. This input + * is untrusted — it comes straight from the request body and is passed to a + * shell-adjacent git invocation, so reject anything suspicious outright + * rather than trying to sanitize it. + */ +function isSafeProjectRelativePath(cwd: string, candidate: string): boolean { + if (typeof candidate !== 'string' || candidate.length === 0) return false; + if (candidate.trim() !== candidate) return false; + if (isAbsolute(candidate)) return false; + + const segments = candidate.split(/[\\/]+/); + if (segments.some((segment) => segment === '' || segment === '.' || segment === '..')) { + return false; + } + + const resolvedRelative = relative(cwd, join(cwd, candidate)); + if (resolvedRelative.startsWith('..') || isAbsolute(resolvedRelative)) return false; + + return true; +} + +/** True if `relPath` is something `git add -- relPath` can actually stage: + * present on disk, or a deletion git already knows about (tracked file + * removed from the working tree). Without this check, `git add` fails on a + * caller-supplied path that doesn't exist, and — critically — a naive + * `existsSync` check alone would also skip real deletions, since a deleted + * file by definition doesn't exist on disk anymore. */ +async function isStageable(cwd: string, relPath: string): Promise { + if (existsSync(join(cwd, relPath))) return true; + try { + const { stdout } = await execFileAsync( + 'git', + ['status', '--porcelain', '--', relPath], + { cwd } + ); + return stdout.trim().length > 0; + } catch { + return false; + } +} + export async function POST( request: NextRequest, { params }: { params: Promise<{ id: string }> } @@ -41,25 +87,124 @@ export async function POST( const cwd = project.path; - // Stage all changes (including untracked files) - await execFileAsync('git', ['add', '-A'], { cwd }); + // Stage ONLY the files the caller names, never the whole working tree — + // unless the caller doesn't opt in, in which case we preserve the + // route's original `git add -A` behavior exactly (see below). A bare + // `git add -A` here previously swept unrelated uncommitted work into + // dependency-patch commits (which then auto-deployed for some managed + // projects). `files` is untrusted request input and is passed to `git + // add`, so it is validated strictly: non-array/non-string entries, + // absolute paths, and anything that escapes the project directory are + // all rejected with a 400 rather than silently dropped or coerced. + let filesToStage: string[] | null = null; + + if (body.files !== undefined) { + if (!Array.isArray(body.files)) { + return NextResponse.json( + { error: '`files` must be an array of relative path strings' }, + { status: 400 } + ); + } + + if (body.files.length === 0) { + return NextResponse.json( + { + error: + '`files` must not be empty; omit the field entirely to stage the default set', + }, + { status: 400 } + ); + } - // Check if there are changes to commit - try { - const { stdout: statusOutput } = await execFileAsync( - 'git', - ['status', '--porcelain'], - { cwd } - ); + const invalid: string[] = []; + const unsafe: string[] = []; + const safe: string[] = []; + + for (const entry of body.files as unknown[]) { + if (typeof entry !== 'string' || entry.length === 0) { + invalid.push(JSON.stringify(entry)); + continue; + } + if (!isSafeProjectRelativePath(cwd, entry)) { + unsafe.push(entry); + continue; + } + safe.push(entry); + } + + if (invalid.length > 0) { + return NextResponse.json( + { + error: `\`files\` entries must be non-empty strings; got: ${invalid.join(', ')}`, + }, + { status: 400 } + ); + } + + if (unsafe.length > 0) { + return NextResponse.json( + { + error: `\`files\` entries must be relative paths inside the project (no absolute paths or ".."): ${unsafe.join(', ')}`, + }, + { status: 400 } + ); + } + + const missing: string[] = []; + for (const relPath of safe) { + if (!(await isStageable(cwd, relPath))) { + missing.push(relPath); + } + } + + if (missing.length > 0) { + return NextResponse.json( + { error: `File(s) not found: ${missing.join(', ')}` }, + { status: 400 } + ); + } + + filesToStage = safe; + } + + if (filesToStage) { + await execFileAsync('git', ['add', '--', ...filesToStage], { cwd }); + } else { + // No `files` supplied — this is the same `git add -A` the route has + // always run, kept as the default specifically so existing callers + // (patches page, security accordion, project detail's git panel, the + // MCP server) that don't yet send `files` keep working exactly as + // before. + await execFileAsync('git', ['add', '-A'], { cwd }); + } - if (!statusOutput.trim()) { - return NextResponse.json({ - success: false, - error: 'No changes to commit', - }); + // Check if the scoped stage actually produced staged changes. This + // checks the index (`git diff --cached`), not the whole worktree + // (`git status --porcelain` would also report unstaged/untracked files + // outside what we just staged, which is the wrong signal here). A + // non-zero exit from `git diff --cached --quiet` means there ARE staged + // differences (exit 1) — but it can also mean the command itself failed + // for an unrelated reason, so only exit code 1 is treated as "there are + // changes"; anything else is a real failure and is surfaced, not + // swallowed as if there were changes to commit. + let hasStaged: boolean; + try { + await execFileAsync('git', ['diff', '--cached', '--quiet'], { cwd }); + hasStaged = false; // exit 0 = no staged differences + } catch (err) { + const code = (err as { code?: number }).code; + if (code === 1) { + hasStaged = true; // exit 1 = staged differences present + } else { + throw err; // genuine failure (bad repo, git missing, etc.) } - } catch { - // Continue with commit attempt + } + + if (!hasStaged) { + return NextResponse.json({ + success: false, + error: 'No changes to commit', + }); } // Execute git commit From 5ca0f1ea6a8616b8edce6b7ada8e744f1d314455 Mon Sep 17 00:00:00 2001 From: alamb-hex Date: Mon, 10 Aug 2026 17:07:41 -0400 Subject: [PATCH 2/4] feat(git-commit): activate scoped staging for dependency-commit callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a `scope: 'dependencies'` request option to the git-commit route, resolved server-side into package.json + whichever lockfile actually exists in the project (browsers can't reliably guess this, and the route's strict `files` validation 400s on any path that doesn't exist). Mutually exclusive with `files`; unknown scope or a project with no package.json both 400 instead of falling back to `git add -A`. Wires it into the two callers the original hardening was meant for — the patches-page and cve-lite-remediation commit buttons — so dependency-patch commits no longer sweep unrelated in-progress work into an auto-deploying commit. project-detail's general commit button and the MCP git_commit tool are intentionally left on `git add -A`. --- .../projects/[id]/git-commit/route.test.ts | 70 +++++++++++++++ src/app/api/projects/[id]/git-commit/route.ts | 85 +++++++++++++++++-- src/app/patches/page.tsx | 8 +- .../security/project-security-accordion.tsx | 12 ++- src/lib/patch-storage.ts | 6 +- 5 files changed, 172 insertions(+), 9 deletions(-) diff --git a/src/app/api/projects/[id]/git-commit/route.test.ts b/src/app/api/projects/[id]/git-commit/route.test.ts index 65e9e19..2b777c8 100644 --- a/src/app/api/projects/[id]/git-commit/route.test.ts +++ b/src/app/api/projects/[id]/git-commit/route.test.ts @@ -240,6 +240,76 @@ describe('POST /api/projects/[id]/git-commit', () => { }); }); + describe('scope: "dependencies"', () => { + it('stages package.json + the detected lockfile and nothing else', async () => { + // Only package.json and pnpm-lock.yaml exist on disk; the other known + // lockfiles (package-lock.json, yarn.lock, bun.lockb) do not. + vi.mocked(existsSync).mockImplementation( + (p) => + String(p).endsWith('package.json') || String(p).endsWith('pnpm-lock.yaml') + ); + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => ({ stdout: 'commit ok', stderr: '' }), + }); + + const res = await POST( + makeRequest({ message: 'chore: bump deps', scope: 'dependencies' }), + makeParams() + ); + const data = await res.json(); + + expect(data.success).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith( + 'git', + ['add', '--', 'package.json', 'pnpm-lock.yaml'], + expect.objectContaining({ cwd: PROJECT.path }) + ); + }); + + it('400s when both `files` and `scope` are sent', async () => { + const res = await POST( + makeRequest({ message: 'x', files: ['package.json'], scope: 'dependencies' }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/mutually exclusive/i); + expect(mockExecFileAsync).not.toHaveBeenCalled(); + }); + + it('400s on an unknown scope value', async () => { + const res = await POST( + makeRequest({ message: 'x', scope: 'everything' }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/scope/i); + expect(mockExecFileAsync).not.toHaveBeenCalled(); + }); + + it('400s naming the problem when there is no package.json to resolve `dependencies` scope from', async () => { + vi.mocked(existsSync).mockReturnValue(false); + + const res = await POST( + makeRequest({ message: 'x', scope: 'dependencies' }), + makeParams() + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/package\.json/i); + expect(mockExecFileAsync).not.toHaveBeenCalledWith( + 'git', + expect.arrayContaining(['add']), + expect.anything() + ); + }); + }); + describe('nothing to commit', () => { it('returns success:false without committing when nothing is staged', async () => { scriptGit({ diff --git a/src/app/api/projects/[id]/git-commit/route.ts b/src/app/api/projects/[id]/git-commit/route.ts index ed8359e..25f44c7 100644 --- a/src/app/api/projects/[id]/git-commit/route.ts +++ b/src/app/api/projects/[id]/git-commit/route.ts @@ -5,6 +5,7 @@ import { promisify } from 'util'; import { logger } from '@/lib/logger'; import { existsSync } from 'fs'; import { isAbsolute, join, relative } from 'path'; +import { LOCKFILES } from '@/lib/patch-storage'; const execFileAsync = promisify(execFile); @@ -52,6 +53,37 @@ async function isStageable(cwd: string, relPath: string): Promise { } } +/** Scope values `scope` may take. Resolution is server-side and filesystem-aware — + * see `resolveDependenciesScope` — specifically so callers never have to guess a + * project's package manager / lockfile name from the browser. */ +const KNOWN_SCOPES = ['dependencies'] as const; +type KnownScope = (typeof KNOWN_SCOPES)[number]; + +function isKnownScope(value: string): value is KnownScope { + return (KNOWN_SCOPES as readonly string[]).includes(value); +} + +/** + * Resolve `scope: 'dependencies'` into the concrete file set to stage: + * `package.json` plus whichever lockfile(s) actually exist in the project. + * This is deliberately done server-side (not left to the caller to guess) — + * the browser doesn't know the project's filesystem, and the route's `files` + * validation 400s on any path that doesn't exist, so a caller sending a + * speculative `pnpm-lock.yaml` would break every npm/yarn project. + * + * Returns `null` (not an empty array) when there's nothing stageable at all — + * no `package.json` — so the caller can 400 instead of silently no-op'ing. + */ +function resolveDependenciesScope(cwd: string): string[] | null { + if (!existsSync(join(cwd, 'package.json'))) return null; + + const files = ['package.json']; + for (const lockfile of LOCKFILES) { + if (existsSync(join(cwd, lockfile))) files.push(lockfile); + } + return files; +} + export async function POST( request: NextRequest, { params }: { params: Promise<{ id: string }> } @@ -96,8 +128,51 @@ export async function POST( // add`, so it is validated strictly: non-array/non-string entries, // absolute paths, and anything that escapes the project directory are // all rejected with a 400 rather than silently dropped or coerced. + // + // `scope` is the second, additive way to opt in: a caller that knows + // *what kind* of change it made (e.g. a dependency patch) but not the + // project's exact filesystem layout (which lockfile it uses, if any) + // sends `scope: 'dependencies'` and the server resolves it into concrete + // paths — see `resolveDependenciesScope`. `files` and `scope` are + // mutually exclusive; at most one of them ends up populating + // `filesToStage` below. let filesToStage: string[] | null = null; + if (body.files !== undefined && body.scope !== undefined) { + return NextResponse.json( + { error: '`files` and `scope` are mutually exclusive; send only one' }, + { status: 400 } + ); + } + + if (body.scope !== undefined) { + if (typeof body.scope !== 'string' || !isKnownScope(body.scope)) { + return NextResponse.json( + { + error: `Unknown \`scope\`: ${JSON.stringify(body.scope)}. Known scopes: ${KNOWN_SCOPES.join(', ')}`, + }, + { status: 400 } + ); + } + + // Only one scope exists today, but resolution is dispatched by value + // (rather than assuming `dependencies`) so adding a second scope later + // doesn't require touching this branch. + const resolved = + body.scope === 'dependencies' ? resolveDependenciesScope(cwd) : null; + + if (!resolved) { + return NextResponse.json( + { + error: `scope: 'dependencies' found nothing stageable — no package.json in this project`, + }, + { status: 400 } + ); + } + + filesToStage = resolved; + } + if (body.files !== undefined) { if (!Array.isArray(body.files)) { return NextResponse.json( @@ -170,11 +245,11 @@ export async function POST( if (filesToStage) { await execFileAsync('git', ['add', '--', ...filesToStage], { cwd }); } else { - // No `files` supplied — this is the same `git add -A` the route has - // always run, kept as the default specifically so existing callers - // (patches page, security accordion, project detail's git panel, the - // MCP server) that don't yet send `files` keep working exactly as - // before. + // Neither `files` nor `scope` supplied — this is the same `git add -A` + // the route has always run, kept as the default specifically so + // callers that intentionally commit "whatever is dirty" (project + // detail's general git panel, the MCP server's git_commit tool) keep + // working exactly as before. await execFileAsync('git', ['add', '-A'], { cwd }); } diff --git a/src/app/patches/page.tsx b/src/app/patches/page.tsx index 865470b..b44df42 100644 --- a/src/app/patches/page.tsx +++ b/src/app/patches/page.tsx @@ -467,7 +467,13 @@ export default function PatchesPage() { const res = await fetch(`/api/projects/${projectId}/git-commit`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ message: state.pendingCommit.message }), + // Scope this commit to dependency files (package.json + whatever + // lockfile the project actually has) so an unrelated in-progress + // change in the working tree doesn't get swept into — and + // auto-deployed by — a dependency-patch commit. The server resolves + // the concrete file set; the browser doesn't know the project's + // filesystem layout. + body: JSON.stringify({ message: state.pendingCommit.message, scope: 'dependencies' }), }); const data = await res.json(); diff --git a/src/components/security/project-security-accordion.tsx b/src/components/security/project-security-accordion.tsx index 1e5cf6f..79600d4 100644 --- a/src/components/security/project-security-accordion.tsx +++ b/src/components/security/project-security-accordion.tsx @@ -661,7 +661,17 @@ export function ProjectSecurityAccordion({ try { const res = await fetch(`/api/projects/${project.id}/git-commit`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ message: pendingCommit.message, source: 'cve-lite', advisories: pendingCommit.advisories }), + // Scope this commit to dependency files (package.json + whatever + // lockfile the project actually has) so unrelated in-progress work + // in the tree isn't swept into — and potentially auto-deployed by — + // a cve-lite remediation commit. Resolved server-side; the browser + // doesn't know the project's filesystem layout. + body: JSON.stringify({ + message: pendingCommit.message, + source: 'cve-lite', + advisories: pendingCommit.advisories, + scope: 'dependencies', + }), }); const data = await res.json().catch(() => ({})); if (!res.ok || (data as { success?: boolean }).success === false) throw new Error((data as { error?: string }).error ?? `commit failed (HTTP ${res.status})`); diff --git a/src/lib/patch-storage.ts b/src/lib/patch-storage.ts index dbdce49..0300135 100644 --- a/src/lib/patch-storage.ts +++ b/src/lib/patch-storage.ts @@ -23,8 +23,10 @@ const CACHE_TTL_JITTER_MS = 15 * 60 * 1000; // Bump when the cache schema changes to force automatic invalidation of old entries const CACHE_SCHEMA_VERSION = 3; -// Lockfiles fingerprinted (alongside package.json) to detect out-of-band dep changes -const LOCKFILES = ['package-lock.json', 'pnpm-lock.yaml', 'yarn.lock', 'bun.lockb']; +// Lockfiles fingerprinted (alongside package.json) to detect out-of-band dep changes. +// Exported so other server-side code (e.g. the git-commit route's `scope: 'dependencies'` +// resolution) can stage the same file set without redefining it and drifting out of sync. +export const LOCKFILES = ['package-lock.json', 'pnpm-lock.yaml', 'yarn.lock', 'bun.lockb']; /** * Fingerprint a project's dependency state from package.json + its lockfile(s). From 7f6cd70b3e50a90d11d0c84586cc9df35611d271 Mon Sep 17 00:00:00 2001 From: alamb-hex Date: Mon, 10 Aug 2026 17:39:23 -0400 Subject: [PATCH 3/4] fix(git-commit): close pathspec-magic escape and stop swallowing git errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defense-in-depth holes in the caller-supplied `files` path, all reproduced against real git in throwaway repos. `--` terminates git's *option* parsing but does not disable pathspec magic, so a leading `:` re-anchors the argument outside the project. From a cwd inside a subdirectory: git add -- ':/root.txt' -> exit 0, stages repo-root root.txt git add -- ':(top)apps/api/.env' -> exit 0, stages apps/api/.env Both passed every existing check (not absolute, no `.`/`..` segment, `relative()` containment satisfied), and `git status --porcelain -- ':(top)…'` reports the file, so `isStageable` returned true as well. The blast radius is the enclosing git repository, not `project.path` — harmless while a project is its repo root, a real escape otherwise. Fixed on both sides: the validator now rejects a leading `:`, and every git invocation runs with GIT_LITERAL_PATHSPECS=1 so the magic prefixes cannot fire at all. This route never relies on pathspec magic itself. `.git/config` and `.git/hooks/pre-commit` also passed validation. Git neutralizes them (`git add -- .git/config` exits 0 and stages nothing), so there was no breach, but it left a hole in the check and surfaced as a confusing "No changes to commit". Any `.git` segment is now rejected, case-insensitively. Control characters are rejected too, so a NUL in a path yields a clear 400 rather than an opaque ERR_INVALID_ARG_VALUE 500. Finally, `isStageable`'s bare `catch { return false }` collapsed every possible failure of `git status --porcelain` — git missing, not a repository, unreadable index, invalid argument — into "that file does not exist", so the route answered 400 `File(s) not found: x` for a file that plainly does exist. That is the same swallow-every-error-into-one- meaning bug this branch already fixed at the empty-check. Only a clean exit-0-with-empty-output now means "not stageable"; anything else is rethrown and surfaces as a 500. --- .../projects/[id]/git-commit/route.test.ts | 107 ++++++++++++++++++ src/app/api/projects/[id]/git-commit/route.ts | 88 ++++++++++---- 2 files changed, 172 insertions(+), 23 deletions(-) diff --git a/src/app/api/projects/[id]/git-commit/route.test.ts b/src/app/api/projects/[id]/git-commit/route.test.ts index 2b777c8..eb7c96a 100644 --- a/src/app/api/projects/[id]/git-commit/route.test.ts +++ b/src/app/api/projects/[id]/git-commit/route.test.ts @@ -70,6 +70,19 @@ function gitError(code: number, message = 'git failed') { const OK = { stdout: '', stderr: '' }; +/** Every git call the route makes, as `[cmd, args, options]` triples. */ +function gitCalls(): Array<[string, string[], Record]> { + return mockExecFileAsync.mock.calls as Array<[string, string[], Record]>; +} + +/** The argv of the single `git ` invocation matching `match`, or undefined. */ +function gitArgs(sub: string, match?: (args: string[]) => boolean): string[] | undefined { + return gitCalls() + .filter(([, args]) => args[0] === sub && (!match || match(args))) + .map(([, args]) => args)[0]; +} + + describe('POST /api/projects/[id]/git-commit', () => { beforeEach(() => { vi.clearAllMocks(); @@ -109,6 +122,23 @@ describe('POST /api/projects/[id]/git-commit', () => { expect.objectContaining({ cwd: PROJECT.path }) ); }); + + it('sets GIT_LITERAL_PATHSPECS=1 on every git invocation', async () => { + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => ({ stdout: 'commit ok', stderr: '' }), + }); + + await POST(makeRequest({ message: 'x' }), makeParams()); + + expect(gitCalls().length).toBeGreaterThan(0); + for (const [, , opts] of gitCalls()) { + expect((opts.env as Record).GIT_LITERAL_PATHSPECS).toBe('1'); + } + }); }); describe('explicit `files` list', () => { @@ -238,6 +268,83 @@ describe('POST /api/projects/[id]/git-commit', () => { const data = await res.json(); expect(data.error).toMatch(/empty/i); }); + + // F3: `--` terminates option parsing but does NOT disable pathspec magic. + // Verified against real git from a subdirectory: `git add -- ':/root.txt'` + // exits 0 and stages the repo-root file; `git add -- ':(top)apps/api/.env'` + // likewise. Both pass isAbsolute/`..`/containment checks. + it.each([ + [':/root.txt', 'repo-root magic'], + [':(top)apps/api/.env', '(top) magic'], + [':!package.json', 'exclude magic'], + [':(exclude)package.json', '(exclude) magic'], + ])('400s on git pathspec magic %s (%s) without running git', async (badPath) => { + vi.mocked(existsSync).mockReturnValue(true); + const res = await POST( + makeRequest({ message: 'x', files: [badPath] }), + makeParams() + ); + expect(res.status).toBe(400); + expect(mockExecFileAsync).not.toHaveBeenCalled(); + }); + + // F8: `.git/config`, `.git/hooks/pre-commit` previously passed validation. + it.each(['.git/config', '.git/hooks/pre-commit', 'sub/.git/config', '.GIT/config'])( + '400s on a `.git` path segment (%s) without running git', + async (badPath) => { + vi.mocked(existsSync).mockReturnValue(true); + const res = await POST( + makeRequest({ message: 'x', files: [badPath] }), + makeParams() + ); + expect(res.status).toBe(400); + expect(mockExecFileAsync).not.toHaveBeenCalled(); + } + ); + + it('400s on a path containing a NUL byte without running git', async () => { + vi.mocked(existsSync).mockReturnValue(true); + const res = await POST( + makeRequest({ message: 'x', files: ['package.json\u0000evil'] }), + makeParams() + ); + expect(res.status).toBe(400); + expect(mockExecFileAsync).not.toHaveBeenCalled(); + }); + + it('no git command runs at all when a traversal path is rejected', async () => { + for (const badPath of ['/etc/passwd', '../../etc/passwd', 'sub/../../outside.txt']) { + vi.clearAllMocks(); + vi.mocked(getProject).mockReturnValue(PROJECT as ReturnType); + const res = await POST(makeRequest({ message: 'x', files: [badPath] }), makeParams()); + expect(res.status).toBe(400); + expect(mockExecFileAsync).not.toHaveBeenCalled(); + } + }); + + // F9: a failing `git status --porcelain -- ` used to be swallowed + // into `false`, so the route answered 400 "File(s) not found: x" for a file + // that exists — the same collapse-every-error-into-one-meaning bug this + // branch fixed at the empty-check. + it('surfaces a `git status` failure as a 500, not a 400 "file not found"', async () => { + vi.mocked(existsSync).mockReturnValue(false); + scriptGit({ + status: async () => { + throw gitError(128, 'fatal: not a git repository'); + }, + }); + + const res = await POST( + makeRequest({ message: 'x', files: ['package.json'] }), + makeParams() + ); + + expect(res.status).toBe(500); + const data = await res.json(); + expect(data.error).toMatch(/not a git repository/); + expect(data.error).not.toMatch(/not found/i); + expect(gitArgs('add')).toBeUndefined(); + }); }); describe('scope: "dependencies"', () => { diff --git a/src/app/api/projects/[id]/git-commit/route.ts b/src/app/api/projects/[id]/git-commit/route.ts index 25f44c7..845ae7c 100644 --- a/src/app/api/projects/[id]/git-commit/route.ts +++ b/src/app/api/projects/[id]/git-commit/route.ts @@ -9,23 +9,60 @@ import { LOCKFILES } from '@/lib/patch-storage'; const execFileAsync = promisify(execFile); +/** + * Options for every git invocation this route makes. + * + * `GIT_LITERAL_PATHSPECS=1` is the belt to the validator's braces: `--` + * terminates *option* parsing but does NOT disable git's pathspec magic, so a + * pathspec like `:/root.txt` or `:(top)apps/api/.env` is still reinterpreted as + * repo-root-relative and escapes the project directory (verified against real + * git). With this env var set, git treats every pathspec as a literal path + * relative to the cwd, so the magic prefixes cannot fire at all. Consequence: + * this route must never rely on pathspec magic itself — it doesn't. + */ +function gitOptions(cwd: string, extra: Record = {}) { + return { + cwd, + env: { ...process.env, GIT_LITERAL_PATHSPECS: '1' }, + ...extra, + }; +} + /** * Validate that a caller-supplied relative path is safe to hand to `git add`: - * a non-empty string, not absolute, no `.`/`..` path segments, and (after - * joining onto the project directory) still resolves inside it. This input - * is untrusted — it comes straight from the request body and is passed to a - * shell-adjacent git invocation, so reject anything suspicious outright - * rather than trying to sanitize it. + * a non-empty string, not absolute, no pathspec-magic prefix, no `.`/`..` or + * `.git` path segments, no control characters, and (after joining onto the + * project directory) still resolves inside it. This input is untrusted — it + * comes straight from the request body and is passed to a shell-adjacent git + * invocation, so reject anything suspicious outright rather than trying to + * sanitize it. */ function isSafeProjectRelativePath(cwd: string, candidate: string): boolean { if (typeof candidate !== 'string' || candidate.length === 0) return false; if (candidate.trim() !== candidate) return false; if (isAbsolute(candidate)) return false; + // A leading `:` makes git reinterpret the whole argument as pathspec magic + // (`:/x` = repo root, `:(top)x`, `:(exclude)x`, `:!x`, ...). None of those are + // legitimate file paths, and every one of them escapes `cwd`. + if (candidate.startsWith(':')) return false; + + // NULs and other control bytes cannot appear in an execFile argument (Node + // throws ERR_INVALID_ARG_VALUE); reject them here so the caller gets a clear + // 400 rather than an opaque 500 from deep inside the git call. + // biome-ignore lint/suspicious/noControlCharactersInRegex: rejecting control characters is the point + if (/[\u0000-\u001f\u007f]/.test(candidate)) return false; + const segments = candidate.split(/[\\/]+/); if (segments.some((segment) => segment === '' || segment === '.' || segment === '..')) { return false; } + // Nothing under `.git` is ever a legitimate commit target. Git already + // neutralizes `git add -- .git/config` (exit 0, stages nothing), but that + // produces a confusing "No changes to commit" instead of a clear rejection, + // and leaving the hole open weakens a defense-in-depth check. Case-insensitive + // because `.GIT` resolves to the same directory on case-insensitive volumes. + if (segments.some((segment) => segment.toLowerCase() === '.git')) return false; const resolvedRelative = relative(cwd, join(cwd, candidate)); if (resolvedRelative.startsWith('..') || isAbsolute(resolvedRelative)) return false; @@ -38,19 +75,24 @@ function isSafeProjectRelativePath(cwd: string, candidate: string): boolean { * removed from the working tree). Without this check, `git add` fails on a * caller-supplied path that doesn't exist, and — critically — a naive * `existsSync` check alone would also skip real deletions, since a deleted - * file by definition doesn't exist on disk anymore. */ + * file by definition doesn't exist on disk anymore. + * + * Deliberately does NOT catch: a clean non-match is `git status` exiting 0 with + * empty output, and that is the only thing that may be reported as "not + * stageable". Any *failure* of the status call (git missing, not a repository, + * unreadable index, an invalid argument) is a different fact entirely and is + * rethrown so it surfaces as a 500 — swallowing it would answer 400 "File(s) + * not found: x" for a file that plainly does exist, which is the same + * collapse-every-error-into-one-meaning bug this route fixed at the + * empty-check. */ async function isStageable(cwd: string, relPath: string): Promise { if (existsSync(join(cwd, relPath))) return true; - try { - const { stdout } = await execFileAsync( - 'git', - ['status', '--porcelain', '--', relPath], - { cwd } - ); - return stdout.trim().length > 0; - } catch { - return false; - } + const { stdout } = await execFileAsync( + 'git', + ['status', '--porcelain', '--', relPath], + gitOptions(cwd) + ); + return stdout.trim().length > 0; } /** Scope values `scope` may take. Resolution is server-side and filesystem-aware — @@ -126,8 +168,8 @@ export async function POST( // dependency-patch commits (which then auto-deployed for some managed // projects). `files` is untrusted request input and is passed to `git // add`, so it is validated strictly: non-array/non-string entries, - // absolute paths, and anything that escapes the project directory are - // all rejected with a 400 rather than silently dropped or coerced. + // absolute paths, pathspec magic, .git segments, and anything that + // escapes the project directory are all rejected with a 400 rather than silently dropped or coerced. // // `scope` is the second, additive way to opt in: a caller that knows // *what kind* of change it made (e.g. a dependency patch) but not the @@ -219,7 +261,7 @@ export async function POST( if (unsafe.length > 0) { return NextResponse.json( { - error: `\`files\` entries must be relative paths inside the project (no absolute paths or ".."): ${unsafe.join(', ')}`, + error: `\`files\` entries must be plain relative paths inside the project (no absolute paths, "..", ".git", or ":" pathspec magic): ${unsafe.join(', ')}`, }, { status: 400 } ); @@ -243,14 +285,14 @@ export async function POST( } if (filesToStage) { - await execFileAsync('git', ['add', '--', ...filesToStage], { cwd }); + await execFileAsync('git', ['add', '--', ...filesToStage], gitOptions(cwd)); } else { // Neither `files` nor `scope` supplied — this is the same `git add -A` // the route has always run, kept as the default specifically so // callers that intentionally commit "whatever is dirty" (project // detail's general git panel, the MCP server's git_commit tool) keep // working exactly as before. - await execFileAsync('git', ['add', '-A'], { cwd }); + await execFileAsync('git', ['add', '-A'], gitOptions(cwd)); } // Check if the scoped stage actually produced staged changes. This @@ -264,7 +306,7 @@ export async function POST( // swallowed as if there were changes to commit. let hasStaged: boolean; try { - await execFileAsync('git', ['diff', '--cached', '--quiet'], { cwd }); + await execFileAsync('git', ['diff', '--cached', '--quiet'], gitOptions(cwd)); hasStaged = false; // exit 0 = no staged differences } catch (err) { const code = (err as { code?: number }).code; @@ -286,7 +328,7 @@ export async function POST( const { stdout, stderr } = await execFileAsync( 'git', ['commit', '-m', message], - { cwd, timeout: 30000 } + gitOptions(cwd, { timeout: 30000 }) ); // Log success From 3c4736abadbe2559a2bce9cc8dff8ca5ac598ab4 Mon Sep 17 00:00:00 2001 From: alamb-hex Date: Mon, 10 Aug 2026 17:40:00 -0400 Subject: [PATCH 4/4] fix(git-commit): commit the scoped pathspec, and resolve it from git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The branch scoped `git add` but then ran `git commit -m ` with no pathspec, which commits the entire index. The bug it exists to fix was therefore still live, and harder to spot, because the UI reported the commit as scoped. Reproduced against real git: git add src/feature.ts # developer's own WIP git add -- package.json pnpm-lock.yaml # what this route does git commit -m "chore(deps): bump" -> commit contains src/feature.ts, package.json, pnpm-lock.yaml `git diff --cached --quiet` had the same hole: with no pathspec it reports "changes present" whenever *anything* is staged. A no-op dependency patch plus unrelated staged work skipped the "No changes to commit" branch and produced a commit whose entire content was that unrelated work, under a `chore(deps):` message. Both now carry the same pathspec the staging used. The bare forms are kept for the unscoped `git add -A` fallback, so project-detail's general git panel and the MCP `git_commit` tool keep committing the whole tree exactly as before. The dependency file set is now resolved from `git status --porcelain -z` instead of `existsSync` over a fixed root-only list, which fixes three failures at once: - A lockfile that is present on disk but gitignored (one stray `npm install` in a pnpm repo) was included, so `git add` aborted with "paths are ignored by one of your .gitignore files" *after* it had already staged package.json — HTTP 500 with the index left dirty by the route, and every retry failing identically. Git does not report ignored files, so they no longer enter the set. - A *deleted* lockfile was excluded, because an existsSync filter cannot see a deletion. Switching package managers produced a commit that omitted the removal and left the repo tracking a lockfile that no longer exists. Git reports deletions, so they are now included. - Nested workspace manifests were never staged. `npm install --workspaces` / `pnpm add -r` rewrite every workspace package.json; committing the lockfile without them yields a commit on which `install --frozen-lockfile` fails. They are now picked up at any depth. Selection rules: basename in package.json + the shared LOCKFILES list, at any depth; `-z` so paths are never quoted regardless of core.quotePath; `node_modules` excluded at any depth; collapsed untracked directory entries skipped rather than staging a whole tree; rename/copy records consume their source field, which goes into the commit pathspec (so the deletion half is recorded) but never into `git add`, which rejects it once the rename is in the index. Porcelain paths are repo-root-relative, so they are translated through `git rev-parse --show-prefix` and the listing is bounded with `-- .`; a project nested in a larger repo no longer sees its siblings' manifests. A failing `git status` propagates rather than resolving to an empty set, an unresolved merge conflict in a dependency file is a 409 instead of a commit full of conflict markers, and an empty resolved set short-circuits to "No changes to commit" rather than falling through to an empty pathspec. Work the developer had already staged is excluded from the scoped commit but stays staged; it is now reported back in a `warnings` field and logged, rather than being silently absorbed into the commit or silently reset. Tests assert on the actual argv handed to git — the previous suite fully mocked child_process without checking arguments, which is why it could not see either missing pathspec. --- .../projects/[id]/git-commit/route.test.ts | 286 +++++++++++++++- src/app/api/projects/[id]/git-commit/route.ts | 320 ++++++++++++++++-- 2 files changed, 559 insertions(+), 47 deletions(-) diff --git a/src/app/api/projects/[id]/git-commit/route.test.ts b/src/app/api/projects/[id]/git-commit/route.test.ts index eb7c96a..fd1cd48 100644 --- a/src/app/api/projects/[id]/git-commit/route.test.ts +++ b/src/app/api/projects/[id]/git-commit/route.test.ts @@ -53,6 +53,12 @@ function scriptGit( async (cmd: string, args: string[]) => { if (cmd !== 'git') throw new Error(`unexpected command: ${cmd}`); const [sub] = args; + // The scoped-commit index-residue probe (`git diff --cached --name-only + // -z --relative`). Answers "nothing else was already staged" unless a + // test overrides it with an explicit `diffNameOnly` handler. + if (sub === 'diff' && args.includes('--name-only')) { + return (handlers.diffNameOnly ?? (async () => OK))(args); + } const handler = handlers[sub]; if (!handler) { throw new Error(`no handler configured for git ${sub} (${args.join(' ')})`); @@ -70,6 +76,12 @@ function gitError(code: number, message = 'git failed') { const OK = { stdout: '', stderr: '' }; +/** Build `git status --porcelain -z` output: NUL-terminated records, no quoting. + * Rename/copy records are `XY \0\0`, so pass those as two entries. */ +function porcelainZ(...records: string[]) { + return { stdout: records.map((r) => `${r}\0`).join(''), stderr: '' }; +} + /** Every git call the route makes, as `[cmd, args, options]` triples. */ function gitCalls(): Array<[string, string[], Record]> { return mockExecFileAsync.mock.calls as Array<[string, string[], Record]>; @@ -82,6 +94,42 @@ function gitArgs(sub: string, match?: (args: string[]) => boolean): string[] | u .map(([, args]) => args)[0]; } +/** + * Script the git calls a `scope: 'dependencies'` request makes. + * `status` is the porcelain listing the resolver reads; `stagedIndex` is what + * `git diff --cached --name-only -z --relative` reports (the pre-existing index). + */ +function scriptScopeGit(options: { + status: string[]; + prefix?: string; + stagedIndex?: string[]; + hasStagedInScope?: boolean; + commitStdout?: string; +}) { + const { + status, + prefix = '', + stagedIndex = [], + hasStagedInScope = true, + commitStdout = '[main abc1234] scoped commit', + } = options; + + mockExecFileAsync.mockImplementation(async (cmd: string, args: string[]) => { + if (cmd !== 'git') throw new Error(`unexpected command: ${cmd}`); + if (args[0] === 'rev-parse') return { stdout: prefix ? `${prefix}\n` : '\n', stderr: '' }; + if (args[0] === 'status') return porcelainZ(...status); + if (args[0] === 'add') return OK; + if (args[0] === 'diff') { + if (args.includes('--name-only')) { + return { stdout: stagedIndex.map((p) => `${p}\0`).join(''), stderr: '' }; + } + if (hasStagedInScope) throw gitError(1); + return OK; + } + if (args[0] === 'commit') return { stdout: commitStdout, stderr: '' }; + throw new Error(`no handler configured for git ${args.join(' ')}`); + }); +} describe('POST /api/projects/[id]/git-commit', () => { beforeEach(() => { @@ -123,6 +171,25 @@ describe('POST /api/projects/[id]/git-commit', () => { ); }); + it('leaves `git diff --cached` and `git commit` unscoped on the -A path (no pathspec)', async () => { + scriptGit({ + add: async () => OK, + diff: async () => { + throw gitError(1); + }, + commit: async () => ({ stdout: 'commit ok', stderr: '' }), + }); + + await POST(makeRequest({ message: 'chore: bump deps' }), makeParams()); + + // The general-purpose callers (project-detail's git panel, the MCP + // git_commit tool) must keep committing the whole index. + expect(gitArgs('diff')).toEqual(['diff', '--cached', '--quiet']); + expect(gitArgs('commit')).toEqual(['commit', '-m', 'chore: bump deps']); + // ...and the index-residue probe is scoped-only, so it never runs here. + expect(gitArgs('diff', (a) => a.includes('--name-only'))).toBeUndefined(); + }); + it('sets GIT_LITERAL_PATHSPECS=1 on every git invocation', async () => { scriptGit({ add: async () => OK, @@ -164,6 +231,15 @@ describe('POST /api/projects/[id]/git-commit', () => { ['add', '--', 'package.json', 'pnpm-lock.yaml'], expect.objectContaining({ cwd: PROJECT.path }) ); + // F1/F2: staging scope is worthless unless the empty-check and the + // commit carry the same pathspec — a bare `git commit -m` commits the + // whole index, including a developer's unrelated pre-staged work. + expect(gitArgs('diff', (a) => a.includes('--quiet'))).toEqual([ + 'diff', '--cached', '--quiet', '--', 'package.json', 'pnpm-lock.yaml', + ]); + expect(gitArgs('commit')).toEqual([ + 'commit', '-m', 'chore: bump lodash', '--', 'package.json', 'pnpm-lock.yaml', + ]); }); it('stages a deleted lockfile that no longer exists on disk (deletion, not silently skipped)', async () => { @@ -348,19 +424,17 @@ describe('POST /api/projects/[id]/git-commit', () => { }); describe('scope: "dependencies"', () => { + beforeEach(() => { + // The route's cheap pre-flight ("is this even a JS project?") is the only + // remaining filesystem check; the file *set* comes from git. + vi.mocked(existsSync).mockReturnValue(true); + }); + it('stages package.json + the detected lockfile and nothing else', async () => { - // Only package.json and pnpm-lock.yaml exist on disk; the other known - // lockfiles (package-lock.json, yarn.lock, bun.lockb) do not. - vi.mocked(existsSync).mockImplementation( - (p) => - String(p).endsWith('package.json') || String(p).endsWith('pnpm-lock.yaml') - ); - scriptGit({ - add: async () => OK, - diff: async () => { - throw gitError(1); - }, - commit: async () => ({ stdout: 'commit ok', stderr: '' }), + // git reports package.json and pnpm-lock.yaml as changed, plus unrelated + // work that must not be swept in. + scriptScopeGit({ + status: [' M package.json', ' M pnpm-lock.yaml', ' M src/feature.ts', '?? notes.md'], }); const res = await POST( @@ -377,6 +451,194 @@ describe('POST /api/projects/[id]/git-commit', () => { ); }); + it('carries the scoped pathspec into `git diff --cached` and `git commit`', async () => { + // F1/F2. Without the pathspec on the commit, `git commit -m` commits the + // whole index and the developer's own staged WIP rides along under a + // `chore(deps)` message; without it on the empty-check, an unrelated + // staged file makes a no-op dependency patch look committable. + scriptScopeGit({ status: [' M package.json', ' M pnpm-lock.yaml'] }); + + await POST( + makeRequest({ message: 'chore(deps): bump', scope: 'dependencies' }), + makeParams() + ); + + expect(gitArgs('diff', (a) => a.includes('--quiet'))).toEqual([ + 'diff', '--cached', '--quiet', '--', 'package.json', 'pnpm-lock.yaml', + ]); + expect(gitArgs('commit')).toEqual([ + 'commit', '-m', 'chore(deps): bump', '--', 'package.json', 'pnpm-lock.yaml', + ]); + }); + + it('F4: never stages a gitignored lockfile, because git does not report one', async () => { + // A stray `npm install` in a pnpm repo that gitignores package-lock.json. + // The old existsSync-based resolver included it; `git add` then aborted + // ("paths are ignored by one of your .gitignore files") *after* staging + // package.json, leaving the index dirty and every retry failing. + scriptScopeGit({ status: [' M package.json', ' M pnpm-lock.yaml'] }); + + const res = await POST( + makeRequest({ message: 'chore: bump', scope: 'dependencies' }), + makeParams() + ); + + expect((await res.json()).success).toBe(true); + expect(gitArgs('add')).toEqual(['add', '--', 'package.json', 'pnpm-lock.yaml']); + expect(gitArgs('add')).not.toContain('package-lock.json'); + }); + + it('F5: includes a deleted lockfile so the removal is committed', async () => { + scriptScopeGit({ status: [' M package.json', ' D yarn.lock', '?? pnpm-lock.yaml'] }); + + const res = await POST( + makeRequest({ message: 'chore: switch package manager', scope: 'dependencies' }), + makeParams() + ); + + expect((await res.json()).success).toBe(true); + expect(gitArgs('add')).toEqual([ + 'add', '--', 'package.json', 'yarn.lock', 'pnpm-lock.yaml', + ]); + expect(gitArgs('commit')).toEqual([ + 'commit', '-m', 'chore: switch package manager', '--', + 'package.json', 'yarn.lock', 'pnpm-lock.yaml', + ]); + }); + + it('F6: includes nested workspace manifests at any depth, excluding node_modules', async () => { + scriptScopeGit({ + status: [ + ' M package.json', + ' M packages/a/package.json', + ' M apps/web/nested/deep/package.json', + ' M pnpm-lock.yaml', + ' M node_modules/left-pad/package.json', + ' M packages/a/node_modules/dep/package.json', + ' M packages/a/src/index.ts', + ], + }); + + await POST(makeRequest({ message: 'chore: bump', scope: 'dependencies' }), makeParams()); + + expect(gitArgs('add')).toEqual([ + 'add', '--', + 'package.json', + 'packages/a/package.json', + 'apps/web/nested/deep/package.json', + 'pnpm-lock.yaml', + ]); + }); + + it('skips collapsed untracked directory entries rather than staging a whole tree', async () => { + scriptScopeGit({ status: [' M package.json', '?? vendor/', '?? node_modules/'] }); + + await POST(makeRequest({ message: 'chore: bump', scope: 'dependencies' }), makeParams()); + + expect(gitArgs('add')).toEqual(['add', '--', 'package.json']); + }); + + it('puts a staged rename source in the commit pathspec but not in `git add`', async () => { + // `git add -- ` fails with "did not match any files" once the + // rename is in the index, but the old path must still be in the commit + // pathspec or the deletion half of the rename is never recorded. + scriptScopeGit({ status: ['R pnpm-lock.yaml', 'yarn.lock', ' M package.json'] }); + + await POST(makeRequest({ message: 'chore: swap lockfile', scope: 'dependencies' }), makeParams()); + + expect(gitArgs('add')).toEqual(['add', '--', 'pnpm-lock.yaml', 'package.json']); + expect(gitArgs('commit')).toEqual([ + 'commit', '-m', 'chore: swap lockfile', '--', + 'pnpm-lock.yaml', 'yarn.lock', 'package.json', + ]); + }); + + it('translates repo-root-relative porcelain paths for a project nested in a larger repo', async () => { + scriptScopeGit({ + prefix: 'apps/api/', + status: [ + ' M apps/api/package.json', + ' M apps/api/pnpm-lock.yaml', + ' M apps/web/package.json', + ], + }); + + await POST(makeRequest({ message: 'chore: bump', scope: 'dependencies' }), makeParams()); + + // cwd-relative (git pathspecs resolve against cwd), and a sibling + // project's manifest is not ours to commit. + expect(gitArgs('add')).toEqual(['add', '--', 'package.json', 'pnpm-lock.yaml']); + }); + + it('reports "No changes to commit" — without staging or committing — when git shows no dependency changes', async () => { + scriptScopeGit({ status: [' M src/feature.ts'] }); + + const res = await POST( + makeRequest({ message: 'chore: bump', scope: 'dependencies' }), + makeParams() + ); + + expect(await res.json()).toEqual({ success: false, error: 'No changes to commit' }); + expect(gitArgs('add')).toBeUndefined(); + expect(gitArgs('commit')).toBeUndefined(); + // Crucially it must NOT fall through to an empty pathspec, which would + // degrade back to whole-index behavior. + expect(gitArgs('diff')).toBeUndefined(); + }); + + it('surfaces a `git status` failure instead of resolving an empty set', async () => { + mockExecFileAsync.mockImplementation(async (cmd: string, args: string[]) => { + if (args[0] === 'rev-parse') return { stdout: '\n', stderr: '' }; + if (args[0] === 'status') throw gitError(128, 'fatal: not a git repository'); + throw new Error(`unexpected git ${args.join(' ')}`); + }); + + const res = await POST( + makeRequest({ message: 'chore: bump', scope: 'dependencies' }), + makeParams() + ); + + expect(res.status).toBe(500); + expect(gitArgs('commit')).toBeUndefined(); + }); + + it('409s on an unresolved merge conflict in a dependency file', async () => { + scriptScopeGit({ status: ['UU package.json'] }); + + const res = await POST( + makeRequest({ message: 'chore: bump', scope: 'dependencies' }), + makeParams() + ); + + expect(res.status).toBe(409); + expect((await res.json()).error).toMatch(/merge conflict/i); + expect(gitArgs('add')).toBeUndefined(); + expect(gitArgs('commit')).toBeUndefined(); + }); + + it('reports pre-existing staged work it excluded, and neither commits nor resets it', async () => { + scriptScopeGit({ + status: [' M package.json', 'M src/feature.ts'], + stagedIndex: ['package.json', 'src/feature.ts'], + }); + + const res = await POST( + makeRequest({ message: 'chore(deps): bump', scope: 'dependencies' }), + makeParams() + ); + const data = await res.json(); + + expect(data.success).toBe(true); + expect(data.warnings).toEqual([expect.stringContaining('src/feature.ts')]); + expect(gitArgs('commit')).toEqual([ + 'commit', '-m', 'chore(deps): bump', '--', 'package.json', + ]); + // No `git reset` / `git restore` — destroying staged work would be far + // worse than the over-broad commit this route is fixing. + expect(gitArgs('reset')).toBeUndefined(); + expect(gitArgs('restore')).toBeUndefined(); + }); + it('400s when both `files` and `scope` are sent', async () => { const res = await POST( makeRequest({ message: 'x', files: ['package.json'], scope: 'dependencies' }), diff --git a/src/app/api/projects/[id]/git-commit/route.ts b/src/app/api/projects/[id]/git-commit/route.ts index 845ae7c..c407632 100644 --- a/src/app/api/projects/[id]/git-commit/route.ts +++ b/src/app/api/projects/[id]/git-commit/route.ts @@ -95,7 +95,7 @@ async function isStageable(cwd: string, relPath: string): Promise { return stdout.trim().length > 0; } -/** Scope values `scope` may take. Resolution is server-side and filesystem-aware — +/** Scope values `scope` may take. Resolution is server-side and git-aware — * see `resolveDependenciesScope` — specifically so callers never have to guess a * project's package manager / lockfile name from the browser. */ const KNOWN_SCOPES = ['dependencies'] as const; @@ -105,25 +105,195 @@ function isKnownScope(value: string): value is KnownScope { return (KNOWN_SCOPES as readonly string[]).includes(value); } +/** Basenames that count as "a dependency file", at any depth. `package.json` + * plus the lockfile list already used to fingerprint dependency state for + * patch-cache invalidation — reused rather than duplicated so the two cannot + * drift apart. */ +const DEPENDENCY_FILENAMES: ReadonlySet = new Set([ + 'package.json', + ...LOCKFILES, +]); + +/** Paths resolved for a scope, split by what each is safe to be used for. */ +interface ScopedPaths { + /** Paths to hand to `git add --`. */ + stage: string[]; + /** Pathspec for `git diff --cached` and `git commit` — a superset of `stage`, + * additionally carrying the *source* path of a rename that git has already + * staged. That source path must appear in the commit pathspec (otherwise the + * deletion half of the rename is not recorded) but must NOT be passed to + * `git add`, which fails with `fatal: pathspec '' did not match any + * files` once the rename is in the index. Verified against real git. */ + commit: string[]; + /** Dependency files with an unresolved merge conflict. Staging these would + * commit conflict markers, and git refuses a partial commit during a merge + * anyway, so the route rejects instead. */ + conflicted: string[]; +} + +/** True for any path with a `node_modules` segment. Installed packages ship + * their own `package.json` and lockfiles; none of them belong in a commit. */ +function isInsideNodeModules(repoPath: string): boolean { + return repoPath.split('/').includes('node_modules'); +} + +/** `git status --porcelain` reports paths relative to the *repository root*, + * not to the cwd, while every pathspec we later pass back to git (under + * `GIT_LITERAL_PATHSPECS`) is interpreted relative to the cwd. Convert, and drop + * anything that isn't under the project directory. `prefix` is + * `git rev-parse --show-prefix`: empty at the repo root (the case for every + * currently configured project), `sub/dir/` for a project nested inside a + * larger repo. */ +function toProjectRelative(repoPath: string, prefix: string): string | null { + if (!prefix) return repoPath || null; + if (!repoPath.startsWith(prefix)) return null; + const rel = repoPath.slice(prefix.length); + return rel.length > 0 ? rel : null; +} + /** - * Resolve `scope: 'dependencies'` into the concrete file set to stage: - * `package.json` plus whichever lockfile(s) actually exist in the project. - * This is deliberately done server-side (not left to the caller to guess) — - * the browser doesn't know the project's filesystem, and the route's `files` - * validation 400s on any path that doesn't exist, so a caller sending a - * speculative `pnpm-lock.yaml` would break every npm/yarn project. + * Resolve `scope: 'dependencies'` into the concrete file set to stage, from + * **git's view of what changed** rather than from `existsSync` over a fixed + * root-only list. That single change fixes three failures at once: + * + * - a lockfile that exists on disk but is gitignored (one stray `npm install` + * in a pnpm repo) is no longer included, so `git add` no longer aborts with + * "paths are ignored by one of your .gitignore files" *after* having already + * staged `package.json` — which left the index dirty and made every retry + * fail identically. git does not report ignored files, so they never enter + * the set; + * - a *deleted* lockfile is included, because git reports deletions — an + * `existsSync` filter by definition cannot see them, so switching package + * managers produced a commit that omitted the removal and left the repo + * tracking a lockfile that no longer exists; + * - nested workspace `package.json` files are included at any depth, because + * git reports them wherever they are. `npm install --workspaces` / + * `pnpm add -r` rewrite every workspace manifest; committing the lockfile + * without them yields a commit on which `install --frozen-lockfile` fails. + * + * Accepted porcelain entries (`XY `, NUL-separated via `-z` so paths are + * never quoted or escaped regardless of `core.quotePath`): + * - any ordinary change — `M`, `A`, `D`, `T`, `R`, `C` in either column — whose + * basename is in `DEPENDENCY_FILENAMES`; + * - untracked *files* (`??`) with a dependency basename, e.g. a brand-new + * lockfile. Untracked *directory* entries (git collapses them to a single + * `?? some/dir/` record) are skipped: staging a whole directory would sweep + * in everything inside it, which is precisely what this route exists to stop. + * The trade-off is that a dependency file inside a wholly-untracked new + * directory is not picked up; default (`-unormal`) untracked handling is kept + * rather than `-uall` so git never has to enumerate every untracked file in + * the tree. + * Rejected: anything under `node_modules/` at any depth; anything outside the + * project directory; ignored entries; and unmerged entries (`U` in either + * column, plus `AA`/`DD`), which are reported back as `conflicted`. * - * Returns `null` (not an empty array) when there's nothing stageable at all — - * no `package.json` — so the caller can 400 instead of silently no-op'ing. + * No depth bound is applied: the basename allowlist plus the `node_modules` + * exclusion already bound the set to real manifests, and a depth cap would + * silently drop legitimate deeply-nested workspace packages. + * + * Any failure of the git calls propagates — a resolver that cannot see the + * repository must not answer "nothing changed". */ -function resolveDependenciesScope(cwd: string): string[] | null { - if (!existsSync(join(cwd, 'package.json'))) return null; +async function resolveDependenciesScope(cwd: string): Promise { + const { stdout: prefixOut } = await execFileAsync( + 'git', + ['rev-parse', '--show-prefix'], + gitOptions(cwd) + ); + const prefix = prefixOut.trim(); + + // `-- .` limits the report to the project directory: without it, a project + // nested inside a larger repository would pull in sibling projects' manifests. + const { stdout } = await execFileAsync( + 'git', + ['status', '--porcelain', '-z', '--', '.'], + gitOptions(cwd, { maxBuffer: 32 * 1024 * 1024 }) + ); + + const stage: string[] = []; + const commit: string[] = []; + const conflicted: string[] = []; + const seenStage = new Set(); + const seenCommit = new Set(); + + const record = (repoPath: string, alsoStage: boolean) => { + if (isInsideNodeModules(repoPath)) return; + const rel = toProjectRelative(repoPath, prefix); + if (rel === null) return; + if (!seenCommit.has(rel)) { + seenCommit.add(rel); + commit.push(rel); + } + if (alsoStage && !seenStage.has(rel)) { + seenStage.add(rel); + stage.push(rel); + } + }; + + const fields = stdout.split('\0'); + for (let i = 0; i < fields.length; i++) { + const entry = fields[i]; + // `XY ` — shortest possible record is 4 chars. The split leaves a + // trailing empty field after the final NUL. + if (!entry || entry.length < 4) continue; + + const x = entry[0]; + const y = entry[1]; + const path = entry.slice(3); + + // Rename/copy records carry the source path in the *next* NUL-separated + // field; consume it here so parsing stays in sync whether or not the entry + // ends up being selected. + const isRenameOrCopy = x === 'R' || x === 'C' || y === 'R' || y === 'C'; + const source = isRenameOrCopy ? fields[++i] : undefined; + + if (x === '!' && y === '!') continue; // ignored (only emitted with --ignored) + if (path.endsWith('/')) continue; // collapsed untracked directory + if (isInsideNodeModules(path)) continue; + + const basename = path.slice(path.lastIndexOf('/') + 1); + if (!DEPENDENCY_FILENAMES.has(basename)) continue; + + const isUnmerged = + x === 'U' || y === 'U' || (x === 'A' && y === 'A') || (x === 'D' && y === 'D'); + if (isUnmerged) { + const rel = toProjectRelative(path, prefix); + if (rel !== null) conflicted.push(rel); + continue; + } - const files = ['package.json']; - for (const lockfile of LOCKFILES) { - if (existsSync(join(cwd, lockfile))) files.push(lockfile); + record(path, true); + // Rename source: commit pathspec only, never `git add`. + if (source) record(source, false); } - return files; + + return { stage, commit, conflicted }; +} + +/** + * Paths already in the index that this scoped commit is about to leave behind. + * + * Passing a pathspec to `git commit` correctly excludes a developer's + * pre-staged unrelated work from *this* commit — but it stays staged, and their + * next commit picks it up. Silently absorbing it is the bug this route exists to + * fix; silently resetting it would destroy staged work, which is worse. So it is + * neither committed nor touched — just reported. + * + * `--relative` makes the output cwd-relative so it compares directly against the + * scoped pathspec (which is also cwd-relative). For a project nested inside a + * larger repo that means staged files elsewhere in that repo are not reported; + * they are also outside the project's purview. + */ +async function findStagedOutsideScope(cwd: string, scopePaths: string[]): Promise { + const { stdout } = await execFileAsync( + 'git', + ['diff', '--cached', '--name-only', '-z', '--relative'], + gitOptions(cwd, { maxBuffer: 32 * 1024 * 1024 }) + ); + const inScope = new Set(scopePaths); + return stdout + .split('\0') + .filter((p) => p.length > 0 && !inScope.has(p)); } export async function POST( @@ -168,17 +338,24 @@ export async function POST( // dependency-patch commits (which then auto-deployed for some managed // projects). `files` is untrusted request input and is passed to `git // add`, so it is validated strictly: non-array/non-string entries, - // absolute paths, pathspec magic, .git segments, and anything that - // escapes the project directory are all rejected with a 400 rather than silently dropped or coerced. + // absolute paths, pathspec magic, `.git` segments, and anything that + // escapes the project directory are all rejected with a 400 rather than + // silently dropped or coerced. // // `scope` is the second, additive way to opt in: a caller that knows // *what kind* of change it made (e.g. a dependency patch) but not the - // project's exact filesystem layout (which lockfile it uses, if any) - // sends `scope: 'dependencies'` and the server resolves it into concrete - // paths — see `resolveDependenciesScope`. `files` and `scope` are - // mutually exclusive; at most one of them ends up populating - // `filesToStage` below. - let filesToStage: string[] | null = null; + // project's exact layout (which lockfile it uses, which workspaces it has) + // sends `scope: 'dependencies'` and the server resolves it from git — see + // `resolveDependenciesScope`. `files` and `scope` are mutually exclusive; + // at most one of them ends up populating the two path lists below. + // + // Two lists, not one: `stagePaths` is what `git add` receives, `scopePaths` + // is the pathspec for the empty-check and the commit. They differ only for + // an already-staged rename (see `ScopedPaths.commit`). Both are null on the + // unscoped fallback path, which keeps the bare `git add -A` / + // `git diff --cached` / `git commit -m` forms. + let stagePaths: string[] | null = null; + let scopePaths: string[] | null = null; if (body.files !== undefined && body.scope !== undefined) { return NextResponse.json( @@ -197,22 +374,42 @@ export async function POST( ); } + // Cheap pre-flight: `scope: 'dependencies'` against a project with no + // package.json at all is a caller mistake, not "nothing to commit", and + // is worth a distinct 400 before any git process is spawned. + if (!existsSync(join(cwd, 'package.json'))) { + return NextResponse.json( + { + error: `scope: 'dependencies' found nothing stageable — no package.json in this project`, + }, + { status: 400 } + ); + } + // Only one scope exists today, but resolution is dispatched by value // (rather than assuming `dependencies`) so adding a second scope later // doesn't require touching this branch. const resolved = - body.scope === 'dependencies' ? resolveDependenciesScope(cwd) : null; + body.scope === 'dependencies' ? await resolveDependenciesScope(cwd) : null; if (!resolved) { + return NextResponse.json( + { error: `Unknown \`scope\`: ${JSON.stringify(body.scope)}` }, + { status: 400 } + ); + } + + if (resolved.conflicted.length > 0) { return NextResponse.json( { - error: `scope: 'dependencies' found nothing stageable — no package.json in this project`, + error: `Unresolved merge conflict in: ${resolved.conflicted.join(', ')}. Resolve it before committing dependency changes.`, }, - { status: 400 } + { status: 409 } ); } - filesToStage = resolved; + stagePaths = resolved.stage; + scopePaths = resolved.commit; } if (body.files !== undefined) { @@ -281,11 +478,23 @@ export async function POST( ); } - filesToStage = safe; + stagePaths = safe; + scopePaths = safe; } - if (filesToStage) { - await execFileAsync('git', ['add', '--', ...filesToStage], gitOptions(cwd)); + // An empty scoped set must short-circuit here. Falling through would run + // `git add --` and then `git diff --cached --quiet --` / `git commit -m … --` + // with no pathspec at all, which is exactly the whole-index behavior this + // route is scoping away from. + if (scopePaths && scopePaths.length === 0) { + return NextResponse.json({ + success: false, + error: 'No changes to commit', + }); + } + + if (stagePaths) { + await execFileAsync('git', ['add', '--', ...stagePaths], gitOptions(cwd)); } else { // Neither `files` nor `scope` supplied — this is the same `git add -A` // the route has always run, kept as the default specifically so @@ -295,8 +504,33 @@ export async function POST( await execFileAsync('git', ['add', '-A'], gitOptions(cwd)); } - // Check if the scoped stage actually produced staged changes. This - // checks the index (`git diff --cached`), not the whole worktree + // Report — never absorb, never reset — anything the developer had staged + // that this commit's pathspec excludes. See `findStagedOutsideScope`. + const stagedOutsideScope = scopePaths + ? await findStagedOutsideScope(cwd, scopePaths) + : []; + const warnings = + stagedOutsideScope.length > 0 + ? [ + `Left staged, not included in this commit: ${stagedOutsideScope.join(', ')}. These were already in the index; they remain staged and will be picked up by your next commit.`, + ] + : undefined; + if (warnings) { + logger.warn( + 'git', + 'commit_scope_index_residue', + `Scoped commit excluded ${stagedOutsideScope.length} already-staged path(s)`, + { projectId: id, meta: { paths: stagedOutsideScope, ...(source ? { source } : {}) } } + ); + } + + // Check if the scoped stage actually produced staged changes — scoped to + // the same pathspec the commit will use. Without the pathspec this reads + // the whole index, so a no-op dependency patch plus unrelated staged work + // reported "changes present" and produced a commit whose entire content + // was that unrelated work, under a `chore(deps): …` message. + // + // This checks the index (`git diff --cached`), not the whole worktree // (`git status --porcelain` would also report unstaged/untracked files // outside what we just staged, which is the wrong signal here). A // non-zero exit from `git diff --cached --quiet` means there ARE staged @@ -304,9 +538,13 @@ export async function POST( // for an unrelated reason, so only exit code 1 is treated as "there are // changes"; anything else is a real failure and is surfaced, not // swallowed as if there were changes to commit. + const diffArgs = scopePaths + ? ['diff', '--cached', '--quiet', '--', ...scopePaths] + : ['diff', '--cached', '--quiet']; + let hasStaged: boolean; try { - await execFileAsync('git', ['diff', '--cached', '--quiet'], gitOptions(cwd)); + await execFileAsync('git', diffArgs, gitOptions(cwd)); hasStaged = false; // exit 0 = no staged differences } catch (err) { const code = (err as { code?: number }).code; @@ -321,13 +559,24 @@ export async function POST( return NextResponse.json({ success: false, error: 'No changes to commit', + ...(warnings ? { warnings } : {}), }); } - // Execute git commit + // Execute git commit. When a scoped file set is in play the pathspec is + // mandatory: `git commit -m ` with no pathspec commits the ENTIRE + // index, so scoping `git add` alone accomplished nothing except making the + // resulting over-broad commit harder to spot, since the UI reported it as + // scoped. The pathspec form commits worktree content for those paths, which + // is what we want here, and it does record deletions and staged renames + // (both verified against real git). + const commitArgs = scopePaths + ? ['commit', '-m', message, '--', ...scopePaths] + : ['commit', '-m', message]; + const { stdout, stderr } = await execFileAsync( 'git', - ['commit', '-m', message], + commitArgs, gitOptions(cwd, { timeout: 30000 }) ); @@ -344,6 +593,7 @@ export async function POST( return NextResponse.json({ success: true, output: stdout || stderr || 'Commit successful', + ...(warnings ? { warnings } : {}), }); } catch (error) { console.error('Git commit failed:', error);