diff --git a/package-lock.json b/package-lock.json index dd3ecccc..8d6c902a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.6", + "version": "0.10.7", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.6", + "version": "0.10.7", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.6", + "version": "0.10.7", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.6", + "version": "0.10.7", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.6", + "version": "0.10.7", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.6", + "version": "0.10.7", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index 3cf1cacb..fb7c85e8 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.6", + "version": "0.10.7", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 264a0a3d..0d113006 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.6", + "version": "0.10.7", "description": "Agent-agnostic, GitHub-style diff viewer and code review tool with a live agent loop", "type": "module", "bin": { diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 724d6880..8e4c2906 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -48,7 +48,7 @@ import { revertHunk, getRefCapabilities, getHeadHash, - isDirty, + getDirtyPaths, getTree, getTreeEntries, getTreeFingerprint, @@ -254,6 +254,25 @@ function serveStatic(res: ServerResponse, filePath: string) { res.end(content); } +/** + * Refuses with a 409 naming the files when any of them has uncommitted local changes, answering + * whether it did. Only the files a comment anchors to matter: anchors are working-tree line + * numbers, so a commented file must still match the PR head, while dirt elsewhere — a scratch + * file, an unrelated edit — shifts none of them. + */ +function refusedDirtyFiles(res: ServerResponse, filePaths: string[]): boolean { + if (filePaths.length === 0) { + return false; + } + const dirty = new Set(getDirtyPaths()); + const blocked = [...new Set(filePaths)].filter(filePath => dirty.has(filePath)); + if (blocked.length === 0) { + return false; + } + sendError(res, 409, `Uncommitted local changes in ${blocked.join(', ')}. Commit or stash them first.`); + return true; +} + function descriptionForRef(ref: string): string { if (WORKING_TREE_REFS.has(ref)) { const labels: Record = { @@ -726,16 +745,15 @@ export function startServer(options: ServerOptions): Promise { sendError(res, 409, 'Local branch is out of sync with the PR. Push or pull your git changes first.'); return; } - if (isDirty()) { - sendError(res, 409, 'You have uncommitted local changes. Commit or stash them first.'); - return; - } withJsonBody(res, req, 'Failed to create review', parseReviewSubmission, (submission) => oneGhMutationAtATime(async () => { // A verdict carries its own meaning; only a plain comment needs something in it. if (submission.event === 'COMMENT' && submission.comments.length === 0 && !submission.body.trim()) { sendError(res, 400, 'A comment review needs a summary or at least one comment'); return; } + if (refusedDirtyFiles(res, submission.comments.map(comment => comment.filePath))) { + return; + } const result = await createGitHubReview( githubRemote.owner, githubRemote.repo, @@ -776,11 +794,6 @@ export function startServer(options: ServerOptions): Promise { sendError(res, 409, 'Local branch is out of sync with the PR. Push or pull your git changes first.'); return; } - if (isDirty()) { - sendError(res, 409, 'You have uncommitted local changes. Commit or stash them first.'); - return; - } - withJsonBody(res, req, 'Failed to pull comments', parsePullCommentsRequest, (body) => oneGhMutationAtATime(async () => { const sid = body.sessionId; const [remoteThreads, remoteState] = await Promise.all([ @@ -789,6 +802,12 @@ export function startServer(options: ServerOptions): Promise { ]); const localThreads = getThreadsForSession(sid); + // Only incoming threads anchor anything; threads already known re-pull freely. + const incoming = remoteThreads.filter(rt => !existingThreadFor(localThreads, rt)); + if (refusedDirtyFiles(res, incoming.map(rt => rt.filePath))) { + return; + } + const settled = remoteState ? threadsResolvedRemotely(localThreads, remoteState) : []; for (const threadId of settled) { updateThreadStatus(threadId, 'resolved'); diff --git a/packages/cli/tests/dirty-guard.test.ts b/packages/cli/tests/dirty-guard.test.ts new file mode 100644 index 00000000..a0915b7d --- /dev/null +++ b/packages/cli/tests/dirty-guard.test.ts @@ -0,0 +1,197 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { existsSync, mkdtempSync, writeFileSync, rmSync, mkdirSync, chmodSync } from 'node:fs'; +import { join, delimiter } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; +let origPath: string | undefined; +let port: number; +let close: () => void; +let markerPath: string; +let sessionId: string; + +// A fake gh serving one pull request whose head is the repo's real HEAD, so the head-sync guard +// passes and only dirtiness decides. Posting a review touches FAKE_GH_MARKER, so a test can tell +// "refused before the forge" from "the forge refused". +function writeFakeGh(dir: string, headSha: string): void { + writeFileSync(join(dir, 'gh'), `#!/bin/sh +case "$1 $2" in + "--version ") echo "gh version 2.0.0"; exit 0 ;; + "auth status") exit 0 ;; + "pr view") + echo '{"number":1,"title":"One PR","url":"https://github.com/o/r/pull/1","headRefOid":"${headSha}","createdAt":"2026-01-01T00:00:00Z","author":{"login":"me"},"body":""}' + exit 0 ;; + "pr diff") cat <<'PATCH' +diff --git a/a.ts b/a.ts +index 0000000..1111111 100644 +--- a/a.ts ++++ b/a.ts +@@ -1,3 +1,4 @@ + line one ++line two added + line three + line four +PATCH + exit 0 ;; + "api user") echo "me"; exit 0 ;; + "api repos/o/r/pulls/1/comments") + if [ "$3" = "--jq" ]; then echo 0; exit 0; fi + if [ -n "$FAKE_GH_EXTRA_COMMENT" ]; then + echo '[{"id":55,"in_reply_to_id":null,"path":"a.ts","side":"RIGHT","line":2,"start_line":null,"body":"remote finding","user":{"login":"alice","type":"User"},"created_at":"2026-01-01T00:00:00Z"},{"id":56,"in_reply_to_id":null,"path":"a.ts","side":"RIGHT","line":3,"start_line":null,"body":"second finding","user":{"login":"alice","type":"User"},"created_at":"2026-01-02T00:00:00Z"}]' + else + echo '[{"id":55,"in_reply_to_id":null,"path":"a.ts","side":"RIGHT","line":2,"start_line":null,"body":"remote finding","user":{"login":"alice","type":"User"},"created_at":"2026-01-01T00:00:00Z"}]' + fi + exit 0 ;; + "api repos/o/r/pulls/1/reviews") + if [ "$3" = "--method" ]; then + cat > /dev/null + touch "$FAKE_GH_MARKER" + echo '{"id":7,"html_url":"https://github.com/o/r/pull/1#pullrequestreview-7"}' + else + echo "[]" + fi + exit 0 ;; + "api repos/o/r/pulls/1/reviews/7/comments") echo "[]"; exit 0 ;; + "api graphql") echo '{}'; exit 0 ;; + *) echo "[]"; exit 0 ;; +esac +`); + chmodSync(join(dir, 'gh'), 0o755); +} + +async function post(path: string, body: unknown): Promise<{ status: number; body: { error?: string; submitted?: number; pulled?: number; skipped?: number } }> { + const res = await fetch(`http://127.0.0.1:${port}${path}`, { + method: 'POST', + headers: { 'x-diffity-agent': '1', 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); + return { status: res.status, body: await res.json() }; +} + +// Line 4 on purpose: the fake forge already holds comments on lines 2 and 3, and an existing +// comment at the same position would make the submission "skipped" rather than "submitted". +function submissionOn(filePath: string) { + return { + event: 'COMMENT', + body: 'Summary', + comments: [{ filePath, side: 'RIGHT', startLine: null, endLine: 4, body: 'P2: check this' }], + }; +} + +beforeAll(async () => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-dirty-')); + repoDir = join(root, 'repo'); + mkdirSync(repoDir); + + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + const git = (args: string[]) => execFileSync('git', args, { cwd: repoDir, stdio: 'pipe', encoding: 'utf-8' }); + git(['config', 'user.email', 't@t']); + git(['config', 'user.name', 'T']); + writeFileSync(join(repoDir, 'a.ts'), 'line one\nline two added\nline three\nline four\n'); + writeFileSync(join(repoDir, 'b.ts'), 'const b = 1;\n'); + git(['add', '.']); + git(['commit', '-m', 'init']); + git(['remote', 'add', 'origin', 'https://github.com/o/r.git']); + const headSha = git(['rev-parse', 'HEAD']).trim(); + + const fakeBin = join(root, 'bin'); + mkdirSync(fakeBin); + writeFakeGh(fakeBin, headSha); + origPath = process.env.PATH; + process.env.PATH = `${fakeBin}${delimiter}${origPath ?? ''}`; + markerPath = join(root, 'review-posted'); + process.env.FAKE_GH_MARKER = markerPath; + + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); + + const { startServer } = await import('../src/server.js'); + const started = await startServer({ port: 0, diffArgs: [], effectiveRef: 'work' }); + port = started.port; + close = started.close; + + const info = await fetch(`http://127.0.0.1:${port}/api/info`, { headers: { 'x-diffity-agent': '1' } }); + sessionId = (await info.json()).sessionId; +}, 20000); + +afterAll(() => { + close?.(); + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + delete process.env.FAKE_GH_MARKER; + delete process.env.FAKE_GH_EXTRA_COMMENT; + if (origPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = origPath; + } + rmSync(root, { recursive: true, force: true }); +}); + +describe('dirt only blocks the files it can mis-anchor', () => { + it('posts a review while an unrelated scratch file is dirty', async () => { + writeFileSync(join(repoDir, 'scratch.mjs'), 'console.log("validated a claim");\n'); + + const { status, body } = await post('/api/github/create-review', submissionOn('a.ts')); + + expect(status).toBe(200); + expect(body.submitted).toBe(1); + expect(existsSync(markerPath)).toBe(true); + rmSync(markerPath); + }, 15000); + + it('refuses when a commented file itself is dirty, before reaching the forge', async () => { + writeFileSync(join(repoDir, 'a.ts'), 'line one CHANGED\nline two added\nline three\nline four\n'); + + const { status, body } = await post('/api/github/create-review', submissionOn('a.ts')); + + expect(status).toBe(409); + expect(body.error).toContain('a.ts'); + expect(existsSync(markerPath)).toBe(false); + + execFileSync('git', ['checkout', '--', 'a.ts'], { cwd: repoDir, stdio: 'pipe' }); + }, 15000); + + it('pulls comments while an unrelated scratch file is dirty', async () => { + writeFileSync(join(repoDir, 'scratch.mjs'), 'console.log("validated a claim");\n'); + + const { status, body } = await post('/api/github/pull-comments', { sessionId }); + + expect(status).toBe(200); + expect(body.pulled).toBe(1); + }, 15000); + + it('re-pulls already-known threads even when their file is dirty', async () => { + writeFileSync(join(repoDir, 'a.ts'), 'line one CHANGED\nline two added\nline three\nline four\n'); + + const { status, body } = await post('/api/github/pull-comments', { sessionId }); + + expect(status).toBe(200); + expect(body.pulled).toBe(0); + expect(body.skipped).toBe(1); + }, 15000); + + it('refuses to pull a new thread onto a dirty file', async () => { + writeFileSync(join(repoDir, 'a.ts'), 'line one CHANGED\nline two added\nline three\nline four\n'); + process.env.FAKE_GH_EXTRA_COMMENT = '1'; + + const { status, body } = await post('/api/github/pull-comments', { sessionId }); + + expect(status).toBe(409); + expect(body.error).toContain('a.ts'); + }, 15000); + + it('pulls that same thread once the file is clean again', async () => { + execFileSync('git', ['checkout', '--', 'a.ts'], { cwd: repoDir, stdio: 'pipe' }); + + const { status, body } = await post('/api/github/pull-comments', { sessionId }); + + expect(status).toBe(200); + expect(body.pulled).toBe(1); + expect(body.skipped).toBe(1); + }, 15000); +}); diff --git a/packages/git/package.json b/packages/git/package.json index 3a0c2724..e39b3813 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.6", + "version": "0.10.7", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/git/src/exec.ts b/packages/git/src/exec.ts index e0a9f290..523a5322 100644 --- a/packages/git/src/exec.ts +++ b/packages/git/src/exec.ts @@ -58,6 +58,18 @@ export function git(args: string[]): string { return execFileLarge('git', args); } +/** + * `git` without the trim, for fixed-width output: `status --porcelain -z` starts an entry with + * a status column that can be a space, which trimming would eat. + */ +export function gitUntrimmed(args: string[]): string { + return execFileSync('git', args, { + encoding: 'utf-8', + stdio: STDIO, + maxBuffer: MAX_BUFFER, + }); +} + export function gitWithStdin(args: string[], input: string): string { return execFileSync('git', args, { encoding: 'utf-8', diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index 880e74e1..96ae3aeb 100644 --- a/packages/git/src/index.ts +++ b/packages/git/src/index.ts @@ -3,7 +3,7 @@ export type { RefCapabilities } from './repo.js'; export { isGitRepo, getRepoRoot, getRepoName, getCurrentBranch, getRepoInfo, getHeadHash, getDiffityDir, getDiffityDirPath, isDataDirUntracked, getRefCapabilities, isValidGitRef } from './repo.js'; export { getDiff, getDiffFiles, getDiffStat, getDiffStatForRef, getRenameStatus, getUntrackedFiles, getUntrackedDiff, getFileContent, getFileLineCount, getMergeBase, normalizeRef, resolveBaseRef, resolveThroughUpstream, resolveDiffArgs, resolveRef, revertFile, revertHunk, WORKING_TREE_REFS } from './diff.js'; export type { RefDiffArgs } from './diff.js'; -export { isDirty } from './status.js'; +export { getDirtyPaths } from './status.js'; export { getRecentCommits } from './commits.js'; export { readRepoConfig, resolveDataDir, REPO_CONFIG_FILE, DEFAULT_SEVERITIES } from './config.js'; export type { RepoConfig, ReviewConfig } from './config.js'; diff --git a/packages/git/src/status.ts b/packages/git/src/status.ts index 50196c65..50b3f28e 100644 --- a/packages/git/src/status.ts +++ b/packages/git/src/status.ts @@ -1,5 +1,24 @@ -import { exec } from './exec.js'; +import { gitUntrimmed } from './exec.js'; -export function isDirty(): boolean { - return exec('git status --porcelain').length > 0; +/** + * Every path `git status` reports as changed — staged, unstaged or untracked. A rename or copy + * contributes both of its endpoints. Untracked directories are expanded to their files, matching + * how the working-tree diff enumerates them. NUL framing so paths with spaces, quotes or newlines + * come through verbatim. + */ +export function getDirtyPaths(): string[] { + const fields = gitUntrimmed(['status', '--porcelain=v1', '-z', '--untracked-files=all']) + .split('\0') + .filter(Boolean); + const paths: string[] = []; + for (let i = 0; i < fields.length; i++) { + const status = fields[i].slice(0, 2); + paths.push(fields[i].slice(3)); + // With -z the pre-rename path is its own NUL-separated field after the entry. + if (status.includes('R') || status.includes('C')) { + i++; + paths.push(fields[i]); + } + } + return paths; } diff --git a/packages/git/tests/dirty-paths.test.ts b/packages/git/tests/dirty-paths.test.ts new file mode 100644 index 00000000..132b5e04 --- /dev/null +++ b/packages/git/tests/dirty-paths.test.ts @@ -0,0 +1,125 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execSync } from 'node:child_process'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let repoDir: string; +let origCwd: string; + +function git(cmd: string) { + execSync(`git ${cmd}`, { cwd: repoDir, stdio: 'pipe' }); +} + +function writeFile(name: string, content: string) { + writeFileSync(join(repoDir, name), content); +} + +beforeAll(() => { + origCwd = process.cwd(); + repoDir = mkdtempSync(join(tmpdir(), 'diffity-test-')); + + git('init -b main'); + git('config user.email "test@test.com"'); + git('config user.name "Test"'); + + writeFile('committed.txt', 'committed\n'); + writeFile('to-rename.txt', 'stable content that survives the rename\n'); + writeFile('to-delete.txt', 'doomed\n'); + writeFile('with space.txt', 'spaced\n'); + git('add .'); + git('commit -m "initial commit"'); + + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + rmSync(repoDir, { recursive: true, force: true }); +}); + +describe('getDirtyPaths', () => { + it('returns nothing in a clean tree', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + expect(getDirtyPaths()).toEqual([]); + }); + + it('reports unstaged, staged and untracked paths', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + writeFile('committed.txt', 'edited\n'); + writeFile('staged.txt', 'staged\n'); + git('add staged.txt'); + writeFile('untracked.txt', 'untracked\n'); + + const paths = getDirtyPaths(); + expect(paths).toContain('committed.txt'); + expect(paths).toContain('staged.txt'); + expect(paths).toContain('untracked.txt'); + + git('reset HEAD staged.txt'); + git('checkout -- committed.txt'); + rmSync(join(repoDir, 'staged.txt')); + rmSync(join(repoDir, 'untracked.txt')); + }); + + it('reports an unstaged edit whose status column starts with a space', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + writeFile('committed.txt', 'edited\n'); + + // ` M committed.txt` — a trimming parse would eat the leading space and shift the path. + expect(getDirtyPaths()).toEqual(['committed.txt']); + + git('checkout -- committed.txt'); + }); + + it('reports a deleted file', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + rmSync(join(repoDir, 'to-delete.txt')); + + expect(getDirtyPaths()).toEqual(['to-delete.txt']); + + git('checkout -- to-delete.txt'); + }); + + it('reports both endpoints of a staged rename', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + git('mv to-rename.txt renamed.txt'); + + const paths = getDirtyPaths(); + expect(paths).toContain('renamed.txt'); + expect(paths).toContain('to-rename.txt'); + + git('mv renamed.txt to-rename.txt'); + }); + + it('reports the files inside an untracked directory, not the collapsed directory', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + mkdirSync(join(repoDir, 'newdir')); + writeFile(join('newdir', 'new.txt'), 'inside\n'); + + // Plain porcelain collapses this to `?? newdir/`, while the working-tree diff enumerates + // the file itself — the guard must speak the same language as the diff. + expect(getDirtyPaths()).toEqual(['newdir/new.txt']); + + rmSync(join(repoDir, 'newdir'), { recursive: true }); + }); + + it('reports a path with a space verbatim', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + writeFile('with space.txt', 'edited\n'); + + expect(getDirtyPaths()).toEqual(['with space.txt']); + + git('checkout -- "with space.txt"'); + }); + + it('reports a path with a newline verbatim', async () => { + const { getDirtyPaths } = await import('../src/status.js'); + writeFile('line\nbreak.txt', 'untracked\n'); + + // Without -z this entry would be quoted as "line\nbreak.txt" and split by a line parser. + expect(getDirtyPaths()).toEqual(['line\nbreak.txt']); + + rmSync(join(repoDir, 'line\nbreak.txt')); + }); +}); diff --git a/packages/github/package.json b/packages/github/package.json index e675d796..6e2a96d9 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.6", + "version": "0.10.7", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 173c7644..b9688ad8 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.6", + "version": "0.10.7", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 42b9b2b4..882e5072 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.6", + "version": "0.10.7", "type": "module", "private": true, "scripts": {