From 5096c36756504eaf0775d5151f0cbc24901f68d1 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5)" Date: Tue, 1 Sep 2026 15:36:26 +0200 Subject: [PATCH 1/3] fix: only dirt in a commented file blocks talking to the forge The submit and pull guards refused on any dirt at all, so an untracked scratch file blocked posting a review it could not affect. Dirt only matters where it can mis-anchor: create-review now refuses only when a commented file is dirty, pull-comments only when an incoming thread lands on a dirty file, both naming the files. isDirty() gives way to dirtyPaths(), parsed from NUL-framed porcelain so spaces, renames and the leading-space status column survive. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- package-lock.json | 12 +- packages/api/package.json | 2 +- packages/cli/package.json | 2 +- packages/cli/src/server.ts | 34 +++-- packages/cli/tests/dirty-guard.test.ts | 194 +++++++++++++++++++++++++ packages/git/package.json | 2 +- packages/git/src/exec.ts | 12 ++ packages/git/src/index.ts | 2 +- packages/git/src/status.ts | 22 ++- packages/git/tests/dirty-paths.test.ts | 93 ++++++++++++ packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 13 files changed, 355 insertions(+), 26 deletions(-) create mode 100644 packages/cli/tests/dirty-guard.test.ts create mode 100644 packages/git/tests/dirty-paths.test.ts 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..87184cf9 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -48,7 +48,7 @@ import { revertHunk, getRefCapabilities, getHeadHash, - isDirty, + dirtyPaths, getTree, getTreeEntries, getTreeFingerprint, @@ -726,16 +726,21 @@ 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; } + // Comments anchor to working-tree lines, so a commented file must still match the PR + // head. Dirt elsewhere — a scratch file, an unrelated edit — shifts none of them. + const dirty = new Set(dirtyPaths()); + const dirtyCommented = [...new Set(submission.comments.map(comment => comment.filePath))] + .filter(filePath => dirty.has(filePath)); + if (dirtyCommented.length > 0) { + sendError(res, 409, `Uncommitted local changes in ${dirtyCommented.join(', ')}. Commit or stash them first.`); + return; + } const result = await createGitHubReview( githubRemote.owner, githubRemote.repo, @@ -776,11 +781,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 +789,20 @@ export function startServer(options: ServerOptions): Promise { ]); const localThreads = getThreadsForSession(sid); + // A pulled thread anchors to working-tree lines, so an incoming thread's file must + // still match the PR head. Threads already known anchor nothing, and dirt in files + // no thread lands on shifts nothing. + const dirty = new Set(dirtyPaths()); + const dirtyIncoming = [...new Set( + remoteThreads + .filter(rt => !existingThreadFor(localThreads, rt) && dirty.has(rt.filePath)) + .map(rt => rt.filePath), + )]; + if (dirtyIncoming.length > 0) { + sendError(res, 409, `Uncommitted local changes in ${dirtyIncoming.join(', ')}. Commit or stash them first.`); + 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..1bb6b6f0 --- /dev/null +++ b/packages/cli/tests/dirty-guard.test.ts @@ -0,0 +1,194 @@ +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 () => { + 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 () => { + 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..6a59fc9d 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 { dirtyPaths } 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..f88936a0 100644 --- a/packages/git/src/status.ts +++ b/packages/git/src/status.ts @@ -1,5 +1,21 @@ -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. NUL framing so paths with spaces, quotes or newlines come + * through verbatim. + */ +export function dirtyPaths(): string[] { + const fields = gitUntrimmed(['status', '--porcelain=v1', '-z']).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..379370ec --- /dev/null +++ b/packages/git/tests/dirty-paths.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execSync } from 'node:child_process'; +import { mkdtempSync, 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('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('dirtyPaths', () => { + it('returns nothing in a clean tree', async () => { + const { dirtyPaths } = await import('../src/status.js'); + expect(dirtyPaths()).toEqual([]); + }); + + it('reports unstaged, staged and untracked paths', async () => { + const { dirtyPaths } = 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 = dirtyPaths(); + 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 { dirtyPaths } = 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(dirtyPaths()).toEqual(['committed.txt']); + + git('checkout -- committed.txt'); + }); + + it('reports both endpoints of a staged rename', async () => { + const { dirtyPaths } = await import('../src/status.js'); + git('mv to-rename.txt renamed.txt'); + + const paths = dirtyPaths(); + expect(paths).toContain('renamed.txt'); + expect(paths).toContain('to-rename.txt'); + + git('mv renamed.txt to-rename.txt'); + }); + + it('reports a path with a space verbatim', async () => { + const { dirtyPaths } = await import('../src/status.js'); + writeFile('with space.txt', 'edited\n'); + + expect(dirtyPaths()).toEqual(['with space.txt']); + + git('checkout -- "with space.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": { From accd5efede4840ecc8ec94086af22af47b05cee1 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5)" Date: Tue, 1 Sep 2026 15:49:43 +0200 Subject: [PATCH 2/3] fix: expand untracked directories and share the dirty-file refusal Plain porcelain collapses a fresh directory to one entry while the working-tree diff enumerates its files, so the guard spoke a different language than the diff; --untracked-files=all aligns them. The two routes now share one refusal helper, each test establishes the dirt its name claims, and the parser suite covers deletes, untracked-directory expansion and a newline in a path. getDirtyPaths follows the package's getter naming. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/cli/src/server.ts | 40 +++++++++--------- packages/cli/tests/dirty-guard.test.ts | 3 ++ packages/git/src/index.ts | 2 +- packages/git/src/status.ts | 11 +++-- packages/git/tests/dirty-paths.test.ts | 56 ++++++++++++++++++++------ 5 files changed, 76 insertions(+), 36 deletions(-) diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 87184cf9..d13f9725 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -48,7 +48,7 @@ import { revertHunk, getRefCapabilities, getHeadHash, - dirtyPaths, + getDirtyPaths, getTree, getTreeEntries, getTreeFingerprint, @@ -254,6 +254,22 @@ 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 { + 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 = { @@ -732,13 +748,7 @@ export function startServer(options: ServerOptions): Promise { sendError(res, 400, 'A comment review needs a summary or at least one comment'); return; } - // Comments anchor to working-tree lines, so a commented file must still match the PR - // head. Dirt elsewhere — a scratch file, an unrelated edit — shifts none of them. - const dirty = new Set(dirtyPaths()); - const dirtyCommented = [...new Set(submission.comments.map(comment => comment.filePath))] - .filter(filePath => dirty.has(filePath)); - if (dirtyCommented.length > 0) { - sendError(res, 409, `Uncommitted local changes in ${dirtyCommented.join(', ')}. Commit or stash them first.`); + if (refusedDirtyFiles(res, submission.comments.map(comment => comment.filePath))) { return; } const result = await createGitHubReview( @@ -789,17 +799,9 @@ export function startServer(options: ServerOptions): Promise { ]); const localThreads = getThreadsForSession(sid); - // A pulled thread anchors to working-tree lines, so an incoming thread's file must - // still match the PR head. Threads already known anchor nothing, and dirt in files - // no thread lands on shifts nothing. - const dirty = new Set(dirtyPaths()); - const dirtyIncoming = [...new Set( - remoteThreads - .filter(rt => !existingThreadFor(localThreads, rt) && dirty.has(rt.filePath)) - .map(rt => rt.filePath), - )]; - if (dirtyIncoming.length > 0) { - sendError(res, 409, `Uncommitted local changes in ${dirtyIncoming.join(', ')}. Commit or stash them first.`); + // 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; } diff --git a/packages/cli/tests/dirty-guard.test.ts b/packages/cli/tests/dirty-guard.test.ts index 1bb6b6f0..a0915b7d 100644 --- a/packages/cli/tests/dirty-guard.test.ts +++ b/packages/cli/tests/dirty-guard.test.ts @@ -157,6 +157,8 @@ describe('dirt only blocks the files it can mis-anchor', () => { }, 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); @@ -174,6 +176,7 @@ describe('dirt only blocks the files it can mis-anchor', () => { }, 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 }); diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index 6a59fc9d..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 { dirtyPaths } 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 f88936a0..50b3f28e 100644 --- a/packages/git/src/status.ts +++ b/packages/git/src/status.ts @@ -2,11 +2,14 @@ import { gitUntrimmed } from './exec.js'; /** * Every path `git status` reports as changed — staged, unstaged or untracked. A rename or copy - * contributes both of its endpoints. NUL framing so paths with spaces, quotes or newlines come - * through verbatim. + * 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 dirtyPaths(): string[] { - const fields = gitUntrimmed(['status', '--porcelain=v1', '-z']).split('\0').filter(Boolean); +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); diff --git a/packages/git/tests/dirty-paths.test.ts b/packages/git/tests/dirty-paths.test.ts index 379370ec..132b5e04 100644 --- a/packages/git/tests/dirty-paths.test.ts +++ b/packages/git/tests/dirty-paths.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { execSync } from 'node:child_process'; -import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -25,6 +25,7 @@ beforeAll(() => { 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"'); @@ -37,20 +38,20 @@ afterAll(() => { rmSync(repoDir, { recursive: true, force: true }); }); -describe('dirtyPaths', () => { +describe('getDirtyPaths', () => { it('returns nothing in a clean tree', async () => { - const { dirtyPaths } = await import('../src/status.js'); - expect(dirtyPaths()).toEqual([]); + const { getDirtyPaths } = await import('../src/status.js'); + expect(getDirtyPaths()).toEqual([]); }); it('reports unstaged, staged and untracked paths', async () => { - const { dirtyPaths } = await import('../src/status.js'); + 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 = dirtyPaths(); + const paths = getDirtyPaths(); expect(paths).toContain('committed.txt'); expect(paths).toContain('staged.txt'); expect(paths).toContain('untracked.txt'); @@ -62,32 +63,63 @@ describe('dirtyPaths', () => { }); it('reports an unstaged edit whose status column starts with a space', async () => { - const { dirtyPaths } = await import('../src/status.js'); + 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(dirtyPaths()).toEqual(['committed.txt']); + 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 { dirtyPaths } = await import('../src/status.js'); + const { getDirtyPaths } = await import('../src/status.js'); git('mv to-rename.txt renamed.txt'); - const paths = dirtyPaths(); + 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 { dirtyPaths } = await import('../src/status.js'); + const { getDirtyPaths } = await import('../src/status.js'); writeFile('with space.txt', 'edited\n'); - expect(dirtyPaths()).toEqual(['with space.txt']); + 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')); + }); }); From 06a24eac53bfb2858781c8188f527dbcc0661a05 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5)" Date: Tue, 1 Sep 2026 15:57:51 +0200 Subject: [PATCH 3/3] perf: no git status when nothing anchors A body-only review and the all-known re-pull carry no file for dirt to mis-anchor, so the subprocess has nothing to decide. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/cli/src/server.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index d13f9725..8e4c2906 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -261,6 +261,9 @@ function serveStatic(res: ServerResponse, filePath: string) { * 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) {