From c326b469ad2e9d4c93ea33ed25fb33f7f2bbe4f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 21 Apr 2026 11:00:49 +0800 Subject: [PATCH 01/23] feat(core): add git diff statistics utility Port numstat + unified-diff parsing into `packages/core/src/utils/gitDiff.ts` to surface structured working-tree change summaries (files changed, lines added/removed, per-file hunks) against HEAD. Caps mirror issue #2997: 50 files, 1MB per file, 400 lines per file, with a 500-file short-circuit via `git diff --shortstat` to avoid expensive work on massive diffs. - `fetchGitDiff(cwd)` returns stats + per-file summaries (tracked + untracked). - `fetchGitDiffHunks(cwd)` returns structured hunks on demand. - `resolveGitDir(cwd)` follows `.git` file indirection so linked worktrees and submodules report the correct gitdir. - Transient-state short-circuit covers merge, cherry-pick, revert, and both `rebase-merge` / `rebase-apply` layouts. - `core.quotepath=false` is forced so non-ASCII filenames stay as UTF-8. Refs #2997 --- packages/core/src/index.ts | 1 + packages/core/src/utils/gitDiff.test.ts | 514 ++++++++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 342 ++++++++++++++++ 3 files changed, 857 insertions(+) create mode 100644 packages/core/src/utils/gitDiff.test.ts create mode 100644 packages/core/src/utils/gitDiff.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e672e4adfd0..1e44de8d1d9 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -268,6 +268,7 @@ export * from './utils/filesearch/fileSearch.js'; export * from './utils/formatters.js'; export * from './utils/generateContentResponseUtilities.js'; export * from './utils/getFolderStructure.js'; +export * from './utils/gitDiff.js'; export * from './utils/gitIgnoreParser.js'; export * from './utils/gitUtils.js'; export * from './utils/ignorePatterns.js'; diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts new file mode 100644 index 00000000000..128b531d058 --- /dev/null +++ b/packages/core/src/utils/gitDiff.test.ts @@ -0,0 +1,514 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { execFile } from 'node:child_process'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { promisify } from 'node:util'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + fetchGitDiff, + fetchGitDiffHunks, + MAX_DIFF_SIZE_BYTES, + MAX_FILES, + MAX_LINES_PER_FILE, + parseGitDiff, + parseGitNumstat, + parseShortstat, + resolveGitDir, +} from './gitDiff.js'; + +const execFileAsync = promisify(execFile); + +async function git(cwd: string, ...args: string[]): Promise { + await execFileAsync('git', args, { cwd }); +} + +async function makeRepo(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-test-')); + await git(dir, 'init', '-q', '-b', 'main'); + await git(dir, 'config', 'user.email', 'test@example.com'); + await git(dir, 'config', 'user.name', 'Test'); + await git(dir, 'config', 'commit.gpgsign', 'false'); + return dir; +} + +describe('parseGitNumstat', () => { + it('parses added/removed counts and file totals', () => { + const out = '3\t1\tsrc/a.ts\n10\t0\tsrc/b.ts\n0\t5\tsrc/c.ts\n'; + const { stats, perFileStats } = parseGitNumstat(out); + expect(stats).toEqual({ + filesCount: 3, + linesAdded: 13, + linesRemoved: 6, + }); + expect(perFileStats.get('src/a.ts')).toEqual({ + added: 3, + removed: 1, + isBinary: false, + }); + expect(perFileStats.size).toBe(3); + }); + + it('treats `-` counts as binary with zero line deltas', () => { + const out = '-\t-\timg/logo.png\n'; + const { stats, perFileStats } = parseGitNumstat(out); + expect(stats.filesCount).toBe(1); + expect(stats.linesAdded).toBe(0); + expect(stats.linesRemoved).toBe(0); + expect(perFileStats.get('img/logo.png')).toEqual({ + added: 0, + removed: 0, + isBinary: true, + }); + }); + + it('keeps accurate totals but caps per-file entries at MAX_FILES', () => { + const lines: string[] = []; + const totalFiles = MAX_FILES + 5; + for (let i = 0; i < totalFiles; i++) { + lines.push(`1\t0\tfile${i}.ts`); + } + const { stats, perFileStats } = parseGitNumstat(lines.join('\n')); + expect(stats.filesCount).toBe(totalFiles); + expect(stats.linesAdded).toBe(totalFiles); + expect(perFileStats.size).toBe(MAX_FILES); + }); + + it('ignores malformed rows without crashing', () => { + const out = 'garbage-line\n2\t1\tsrc/a.ts\n'; + const { stats, perFileStats } = parseGitNumstat(out); + expect(stats.filesCount).toBe(1); + expect(perFileStats.has('src/a.ts')).toBe(true); + }); + + it('handles filenames containing tabs', () => { + const out = '1\t2\tweird\tname.ts\n'; + const { perFileStats } = parseGitNumstat(out); + expect(perFileStats.has('weird\tname.ts')).toBe(true); + }); +}); + +describe('parseShortstat', () => { + it('parses the full form', () => { + expect( + parseShortstat(' 3 files changed, 42 insertions(+), 7 deletions(-)'), + ).toEqual({ filesCount: 3, linesAdded: 42, linesRemoved: 7 }); + }); + + it('parses additions-only and deletions-only forms', () => { + expect(parseShortstat(' 1 file changed, 5 insertions(+)')).toEqual({ + filesCount: 1, + linesAdded: 5, + linesRemoved: 0, + }); + expect(parseShortstat(' 2 files changed, 3 deletions(-)')).toEqual({ + filesCount: 2, + linesAdded: 0, + linesRemoved: 3, + }); + }); + + it('returns null on garbage input', () => { + expect(parseShortstat('not a shortstat')).toBeNull(); + }); +}); + +describe('parseGitDiff', () => { + const sampleDiff = `diff --git a/src/a.ts b/src/a.ts +index 1111111..2222222 100644 +--- a/src/a.ts ++++ b/src/a.ts +@@ -1,3 +1,4 @@ + line one +-removed ++added ++added two + line three +diff --git a/src/b.ts b/src/b.ts +new file mode 100644 +index 0000000..3333333 +--- /dev/null ++++ b/src/b.ts +@@ -0,0 +1,2 @@ ++hello ++world +`; + + it('produces structured hunks for each file', () => { + const result = parseGitDiff(sampleDiff); + expect([...result.keys()]).toEqual(['src/a.ts', 'src/b.ts']); + + const aHunks = result.get('src/a.ts')!; + expect(aHunks).toHaveLength(1); + expect(aHunks[0]).toMatchObject({ + oldStart: 1, + oldLines: 3, + newStart: 1, + newLines: 4, + }); + expect(aHunks[0].lines).toEqual([ + ' line one', + '-removed', + '+added', + '+added two', + ' line three', + ]); + + const bHunks = result.get('src/b.ts')!; + expect(bHunks[0].lines).toEqual(['+hello', '+world']); + }); + + it('returns empty map on empty input', () => { + expect(parseGitDiff('').size).toBe(0); + expect(parseGitDiff(' \n').size).toBe(0); + }); + + it('caps per-file lines at MAX_LINES_PER_FILE', () => { + const header = `diff --git a/big.ts b/big.ts +index 1111111..2222222 100644 +--- a/big.ts ++++ b/big.ts +@@ -1,${MAX_LINES_PER_FILE + 50} +1,${MAX_LINES_PER_FILE + 50} @@ +`; + const body = Array.from( + { length: MAX_LINES_PER_FILE + 50 }, + (_, i) => ` line${i}`, + ).join('\n'); + const result = parseGitDiff(header + body + '\n'); + const hunk = result.get('big.ts')![0]; + expect(hunk.lines.length).toBe(MAX_LINES_PER_FILE); + }); +}); + +describe('fetchGitDiff', () => { + let repo: string; + + beforeEach(async () => { + repo = await makeRepo(); + }); + + afterEach(async () => { + await fs.rm(repo, { recursive: true, force: true }); + }); + + it('returns null when not in a git repo', async () => { + const plain = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plain-')); + try { + expect(await fetchGitDiff(plain)).toBeNull(); + } finally { + await fs.rm(plain, { recursive: true, force: true }); + } + }); + + it('captures tracked modifications and untracked files', async () => { + await fs.writeFile(path.join(repo, 'tracked.txt'), 'one\ntwo\nthree\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + await fs.writeFile( + path.join(repo, 'tracked.txt'), + 'one\ntwo\nthree\nfour\n', + ); + await fs.writeFile(path.join(repo, 'new.txt'), 'brand new\n'); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.stats.linesAdded).toBeGreaterThanOrEqual(1); + expect(result!.stats.filesCount).toBe(2); + expect(result!.perFileStats.get('tracked.txt')?.added).toBe(1); + expect(result!.perFileStats.get('new.txt')?.isUntracked).toBe(true); + }); + + it('returns zero stats on a clean working tree', async () => { + await fs.writeFile(path.join(repo, 'a.txt'), 'hello\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.stats).toEqual({ + filesCount: 0, + linesAdded: 0, + linesRemoved: 0, + }); + expect(result!.perFileStats.size).toBe(0); + }); + + it('returns null during a transient merge state', async () => { + await fs.writeFile(path.join(repo, 'a.txt'), 'hello\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + // Fake a merge in progress by writing MERGE_HEAD. + await fs.writeFile( + path.join(repo, '.git', 'MERGE_HEAD'), + '0000000000000000000000000000000000000000\n', + ); + expect(await fetchGitDiff(repo)).toBeNull(); + expect((await fetchGitDiffHunks(repo)).size).toBe(0); + }); +}); + +describe('fetchGitDiffHunks', () => { + let repo: string; + + beforeEach(async () => { + repo = await makeRepo(); + }); + + afterEach(async () => { + await fs.rm(repo, { recursive: true, force: true }); + }); + + it('returns hunks for modified tracked files', async () => { + await fs.writeFile(path.join(repo, 'a.txt'), 'one\ntwo\nthree\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + await fs.writeFile(path.join(repo, 'a.txt'), 'one\nTWO\nthree\n'); + const hunks = await fetchGitDiffHunks(repo); + const fileHunks = hunks.get('a.txt'); + expect(fileHunks).toBeDefined(); + expect(fileHunks![0].lines.some((l: string) => l.startsWith('-two'))).toBe( + true, + ); + expect(fileHunks![0].lines.some((l: string) => l.startsWith('+TWO'))).toBe( + true, + ); + }); + + it('preserves content lines that start with --- / +++ / index', async () => { + await fs.writeFile( + path.join(repo, 'notes.md'), + 'keep\n---a/foo\n+++b/bar\nindex deadbeef\nkeep2\n', + ); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + // Remove every diff-lookalike line; the added/removed lines should still + // round-trip through parseGitDiff even though their prefixes match + // file-header sentinels. + await fs.writeFile(path.join(repo, 'notes.md'), 'keep\nkeep2\n'); + const hunks = await fetchGitDiffHunks(repo); + const fileHunks = hunks.get('notes.md'); + expect(fileHunks).toBeDefined(); + const removed = fileHunks!.flatMap((h) => + h.lines.filter((l: string) => l.startsWith('-')), + ); + expect(removed).toEqual( + expect.arrayContaining(['----a/foo', '-+++b/bar', '-index deadbeef']), + ); + }); + + it('handles multi-hunk diffs', async () => { + const initial = Array.from({ length: 40 }, (_, i) => `line${i}`).join('\n'); + await fs.writeFile(path.join(repo, 'big.txt'), initial + '\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + const lines = initial.split('\n'); + lines[2] = 'CHANGED_EARLY'; + lines[35] = 'CHANGED_LATE'; + await fs.writeFile(path.join(repo, 'big.txt'), lines.join('\n') + '\n'); + + const hunks = await fetchGitDiffHunks(repo); + const fileHunks = hunks.get('big.txt'); + expect(fileHunks).toBeDefined(); + expect(fileHunks!.length).toBeGreaterThanOrEqual(2); + }); +}); + +describe('parseGitDiff edge cases', () => { + it('drops file blocks that have no `@@` hunk header', () => { + const noHunk = `diff --git a/foo.ts b/foo.ts +old mode 100644 +new mode 100755 +`; + expect(parseGitDiff(noHunk).size).toBe(0); + }); + + it('stops collecting once MAX_FILES files have been parsed', () => { + const blocks: string[] = []; + for (let i = 0; i < MAX_FILES + 5; i++) { + blocks.push( + `diff --git a/f${i}.ts b/f${i}.ts +--- a/f${i}.ts ++++ b/f${i}.ts +@@ -1,1 +1,1 @@ +-x ++y +`, + ); + } + const result = parseGitDiff(blocks.join('')); + expect(result.size).toBe(MAX_FILES); + }); +}); + +describe('parseGitDiff size/line caps', () => { + it('skips files whose raw diff exceeds MAX_DIFF_SIZE_BYTES', () => { + const header = `diff --git a/small.ts b/small.ts +--- a/small.ts ++++ b/small.ts +@@ -1,1 +1,1 @@ +-a ++b +`; + const bigBody = 'x'.repeat(MAX_DIFF_SIZE_BYTES + 10); + const bigDiff = `diff --git a/big.ts b/big.ts +--- a/big.ts ++++ b/big.ts +@@ -1,1 +1,1 @@ +-${bigBody} ++b +`; + const result = parseGitDiff(header + bigDiff); + expect(result.has('small.ts')).toBe(true); + expect(result.has('big.ts')).toBe(false); + }); +}); + +describe('resolveGitDir', () => { + it('returns the .git directory for a regular repo', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdir-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: dir }); + const resolved = await resolveGitDir(dir); + expect(resolved).toBe(path.join(dir, '.git')); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it('follows the gitdir pointer for linked worktrees', async () => { + const main = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitmain-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: main }); + await execFileAsync('git', ['config', 'user.email', 'test@example.com'], { + cwd: main, + }); + await execFileAsync('git', ['config', 'user.name', 'Test'], { + cwd: main, + }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: main, + }); + await fs.writeFile(path.join(main, 'a.txt'), 'hi\n'); + await execFileAsync('git', ['add', '.'], { cwd: main }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: main }); + + const wtPath = path.join(main, 'wt'); + await execFileAsync( + 'git', + ['worktree', 'add', '-q', wtPath, '-b', 'side'], + { cwd: main }, + ); + + const resolved = await resolveGitDir(wtPath); + expect(resolved).not.toBeNull(); + expect(resolved).toContain(path.join('.git', 'worktrees')); + + // Fake a merge-in-progress inside the linked worktree's gitdir and + // confirm `fetchGitDiff` short-circuits, which would silently fail if + // transient detection only looked at `/.git/MERGE_HEAD`. + await fs.writeFile( + path.join(resolved!, 'MERGE_HEAD'), + '0000000000000000000000000000000000000000\n', + ); + expect(await fetchGitDiff(wtPath)).toBeNull(); + } finally { + await fs.rm(main, { recursive: true, force: true }); + } + }); +}); + +describe('fetchGitDiff transient-state detection', () => { + let repo: string; + beforeEach(async () => { + repo = await makeRepo(); + await fs.writeFile(path.join(repo, 'a.txt'), 'hi\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + }); + afterEach(async () => { + await fs.rm(repo, { recursive: true, force: true }); + }); + + it.each([ + ['CHERRY_PICK_HEAD', 'file'], + ['REVERT_HEAD', 'file'], + ['rebase-merge', 'dir'], + ['rebase-apply', 'dir'], + ] as const)('short-circuits when %s is present (%s)', async (name, kind) => { + const target = path.join(repo, '.git', name); + if (kind === 'dir') { + await fs.mkdir(target); + } else { + await fs.writeFile(target, '0\n'); + } + expect(await fetchGitDiff(repo)).toBeNull(); + expect((await fetchGitDiffHunks(repo)).size).toBe(0); + }); +}); + +describe('fetchGitDiff non-ASCII filenames', () => { + it('does not octal-escape UTF-8 filenames via core.quotepath', async () => { + const repo = await makeRepo(); + try { + const fname = '日本語.txt'; + await fs.writeFile(path.join(repo, fname), 'alpha\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await fs.writeFile(path.join(repo, fname), 'beta\n'); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.perFileStats.has(fname)).toBe(true); + // Make sure we didn't end up with an octal-escaped key instead. + for (const key of result!.perFileStats.keys()) { + expect(key).not.toMatch(/\\\d{3}/); + } + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); +}); + +describe('fetchGitDiff untracked counting', () => { + let repo: string; + beforeEach(async () => { + repo = await makeRepo(); + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + }); + afterEach(async () => { + await fs.rm(repo, { recursive: true, force: true }); + }); + + it('counts untracked files in filesCount even after the per-file map is full', async () => { + // Create MAX_FILES tracked modifications to fill the per-file map. + for (let i = 0; i < MAX_FILES; i++) { + await fs.writeFile(path.join(repo, `t${i}.txt`), `hello${i}\n`); + } + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'seed'); + for (let i = 0; i < MAX_FILES; i++) { + await fs.writeFile(path.join(repo, `t${i}.txt`), `HELLO${i}\n`); + } + // Add 3 untracked files. + await fs.writeFile(path.join(repo, 'u1.txt'), 'a\n'); + await fs.writeFile(path.join(repo, 'u2.txt'), 'b\n'); + await fs.writeFile(path.join(repo, 'u3.txt'), 'c\n'); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.stats.filesCount).toBe(MAX_FILES + 3); + // Per-file map is still capped at MAX_FILES. + expect(result!.perFileStats.size).toBe(MAX_FILES); + }); +}); diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts new file mode 100644 index 00000000000..f8b71258502 --- /dev/null +++ b/packages/core/src/utils/gitDiff.ts @@ -0,0 +1,342 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { execFile } from 'node:child_process'; +import { access, readFile, stat } from 'node:fs/promises'; +import * as path from 'node:path'; +import { promisify } from 'node:util'; +import type { Hunk } from 'diff'; +import { findGitRoot, isGitRepository } from './gitUtils.js'; + +/** Re-export so consumers don't need to depend on `diff` directly. */ +export type GitDiffHunk = Hunk; + +const execFileAsync = promisify(execFile); + +export interface GitDiffStats { + filesCount: number; + linesAdded: number; + linesRemoved: number; +} + +export interface PerFileStats { + added: number; + removed: number; + isBinary: boolean; + isUntracked?: boolean; +} + +export interface GitDiffResult { + stats: GitDiffStats; + perFileStats: Map; +} + +export interface NumstatResult { + stats: GitDiffStats; + perFileStats: Map; +} + +const GIT_TIMEOUT_MS = 5000; +/** Maximum files retained in per-file results. Matches issue #2997 "50 files" cap. */ +export const MAX_FILES = 50; +/** Per-file diff content cap. Matches issue #2997 "1MB" cap. */ +export const MAX_DIFF_SIZE_BYTES = 1_000_000; +/** Per-file diff line cap (GitHub's auto-load threshold). */ +export const MAX_LINES_PER_FILE = 400; +/** Skip per-file parsing when the diff touches more than this many files. */ +export const MAX_FILES_FOR_DETAILS = 500; + +/** + * Fetch numstat-based git diff stats (files changed, lines added/removed) and + * per-file summaries comparing the working tree to HEAD. Structured hunks are + * available separately via `fetchGitDiffHunks`. + * + * Returns `null` when not inside a git repo, when git itself fails, or when + * the working tree is in a transient state (merge, rebase, cherry-pick, + * revert) — those states carry incoming changes that weren't intentionally + * made by the user. + */ +export async function fetchGitDiff(cwd: string): Promise { + if (!isGitRepository(cwd)) return null; + if (await isInTransientGitState(cwd)) return null; + + // Quick probe first — O(1) memory regardless of diff size. Lets us bail out + // of huge diffs (e.g. generated workspaces) before paying for per-file work. + const shortstatOut = await runGit( + ['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], + cwd, + ); + // Fetch untracked filenames up front so both the shortstat fast path and + // the numstat slow path report the same `filesCount` surface. + const untrackedPaths = (await fetchUntrackedPaths(cwd)) ?? []; + + if (shortstatOut != null) { + const quickStats = parseShortstat(shortstatOut); + if (quickStats && quickStats.filesCount > MAX_FILES_FOR_DETAILS) { + return { + stats: { + ...quickStats, + filesCount: quickStats.filesCount + untrackedPaths.length, + }, + perFileStats: new Map(), + }; + } + } + + const numstatOut = await runGit( + ['--no-optional-locks', 'diff', 'HEAD', '--numstat'], + cwd, + ); + if (numstatOut == null) return null; + + const { stats, perFileStats } = parseGitNumstat(numstatOut); + + if (untrackedPaths.length > 0) { + // Count every untracked file in the totals, even if the per-file map is + // already full. Otherwise `filesCount` under-reports whenever tracked + // changes already fill the `MAX_FILES` slot. + stats.filesCount += untrackedPaths.length; + const remainingSlots = MAX_FILES - perFileStats.size; + for (const filePath of untrackedPaths.slice( + 0, + Math.max(0, remainingSlots), + )) { + perFileStats.set(filePath, { + added: 0, + removed: 0, + isBinary: false, + isUntracked: true, + }); + } + } + + return { stats, perFileStats }; +} + +/** + * Fetch structured hunks for the current working tree vs HEAD. Separate from + * `fetchGitDiff` so callers that only need stats do not pay the full diff cost. + */ +export async function fetchGitDiffHunks( + cwd: string, +): Promise> { + if (!isGitRepository(cwd)) return new Map(); + if (await isInTransientGitState(cwd)) return new Map(); + + const diffOut = await runGit(['--no-optional-locks', 'diff', 'HEAD'], cwd); + if (diffOut == null) return new Map(); + return parseGitDiff(diffOut); +} + +/** + * Parse `git diff --numstat` output. + * Format per line: `\t\t`. Binary files use `-` for + * counts. Only the first `MAX_FILES` entries are kept in `perFileStats`, but + * total stats account for every line. + */ +export function parseGitNumstat(stdout: string): NumstatResult { + const lines = stdout.split('\n').filter(Boolean); + let added = 0; + let removed = 0; + let validFileCount = 0; + const perFileStats = new Map(); + + for (const line of lines) { + const parts = line.split('\t'); + if (parts.length < 3) continue; + + validFileCount++; + const addStr = parts[0]; + const remStr = parts[1]; + const filePath = parts.slice(2).join('\t'); + const isBinary = addStr === '-' || remStr === '-'; + const fileAdded = isBinary ? 0 : parseInt(addStr ?? '0', 10) || 0; + const fileRemoved = isBinary ? 0 : parseInt(remStr ?? '0', 10) || 0; + + added += fileAdded; + removed += fileRemoved; + + if (perFileStats.size < MAX_FILES) { + perFileStats.set(filePath, { + added: fileAdded, + removed: fileRemoved, + isBinary, + }); + } + } + + return { + stats: { + filesCount: validFileCount, + linesAdded: added, + linesRemoved: removed, + }, + perFileStats, + }; +} + +/** + * Parse unified diff output into per-file hunks. + * + * Limits applied: + * - Stop once `MAX_FILES` files have been collected. + * - Skip files whose raw diff exceeds `MAX_DIFF_SIZE_BYTES`. + * - Truncate per-file content at `MAX_LINES_PER_FILE` lines. + */ +export function parseGitDiff(stdout: string): Map { + const result = new Map(); + if (!stdout.trim()) return result; + + const fileDiffs = stdout.split(/^diff --git /m).filter(Boolean); + + for (const fileDiff of fileDiffs) { + if (result.size >= MAX_FILES) break; + if (fileDiff.length > MAX_DIFF_SIZE_BYTES) continue; + + const lines = fileDiff.split('\n'); + const headerMatch = lines[0]?.match(/^a\/(.+?) b\/(.+)$/); + if (!headerMatch) continue; + const filePath = headerMatch[2] ?? headerMatch[1] ?? ''; + + const fileHunks: Hunk[] = []; + let currentHunk: Hunk | null = null; + let lineCount = 0; + + for (let i = 1; i < lines.length; i++) { + const line = lines[i] ?? ''; + const hunkMatch = line.match( + /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/, + ); + if (hunkMatch) { + if (currentHunk) fileHunks.push(currentHunk); + currentHunk = { + oldStart: parseInt(hunkMatch[1] ?? '0', 10), + oldLines: parseInt(hunkMatch[2] ?? '1', 10), + newStart: parseInt(hunkMatch[3] ?? '0', 10), + newLines: parseInt(hunkMatch[4] ?? '1', 10), + lines: [], + }; + continue; + } + + // Pre-hunk metadata is only skipped before the first `@@` header. Once + // inside a hunk, a line like `---foo` is a removed source line whose + // content happens to start with `---`, and must not be dropped. + if (!currentHunk) { + continue; + } + + if ( + line.startsWith('+') || + line.startsWith('-') || + line.startsWith(' ') + ) { + if (lineCount >= MAX_LINES_PER_FILE) break; + // Force a flat string copy to break V8 sliced-string references so the + // whole raw diff can be GC'd once parsing finishes. + currentHunk.lines.push('' + line); + lineCount++; + } + } + + if (currentHunk) fileHunks.push(currentHunk); + if (fileHunks.length > 0) result.set(filePath, fileHunks); + } + + return result; +} + +/** + * Parse `git diff --shortstat` output, e.g. + * ` 3 files changed, 42 insertions(+), 7 deletions(-)`. + */ +export function parseShortstat(stdout: string): GitDiffStats | null { + const match = stdout.match( + /(\d+)\s+files?\s+changed(?:,\s+(\d+)\s+insertions?\(\+\))?(?:,\s+(\d+)\s+deletions?\(-\))?/, + ); + if (!match) return null; + return { + filesCount: parseInt(match[1] ?? '0', 10), + linesAdded: parseInt(match[2] ?? '0', 10), + linesRemoved: parseInt(match[3] ?? '0', 10), + }; +} + +/** + * Resolve the real git directory for a working tree, following `.git` file + * indirection used by linked worktrees (`git worktree add`) and submodules. + * Returns `null` when the location is not inside a git repo. + */ +export async function resolveGitDir(cwd: string): Promise { + const gitRoot = findGitRoot(cwd); + if (!gitRoot) return null; + const dotGit = path.join(gitRoot, '.git'); + try { + const s = await stat(dotGit); + if (s.isDirectory()) return dotGit; + if (!s.isFile()) return null; + const content = await readFile(dotGit, 'utf8'); + const match = content.match(/^gitdir:\s*(.+?)\s*$/m); + if (!match || !match[1]) return null; + const raw = match[1]; + return path.isAbsolute(raw) ? raw : path.resolve(gitRoot, raw); + } catch { + return null; + } +} + +async function isInTransientGitState(cwd: string): Promise { + const gitDir = await resolveGitDir(cwd); + if (!gitDir) return false; + + // Rebase-in-progress is signalled by a directory, not a ref file. Both + // rebase-apply (git-am backed) and rebase-merge (interactive / `-m`) forms + // are covered. REBASE_HEAD alone misses the common case. + const transientPaths = [ + 'MERGE_HEAD', + 'CHERRY_PICK_HEAD', + 'REVERT_HEAD', + 'rebase-merge', + 'rebase-apply', + ]; + + const results = await Promise.all( + transientPaths.map((name) => + access(path.join(gitDir, name)) + .then(() => true) + .catch(() => false), + ), + ); + return results.some(Boolean); +} + +async function fetchUntrackedPaths(cwd: string): Promise { + const stdout = await runGit( + ['--no-optional-locks', 'ls-files', '--others', '--exclude-standard'], + cwd, + ); + if (!stdout || !stdout.trim()) return null; + return stdout.trim().split('\n').filter(Boolean); +} + +async function runGit(args: string[], cwd: string): Promise { + // `core.quotepath=false` keeps non-ASCII filenames as UTF-8 in git's output + // instead of octal-escaping them (`\346\226\207.txt`), which would otherwise + // end up as literal keys in `perFileStats`. + const fullArgs = ['-c', 'core.quotepath=false', ...args]; + try { + const { stdout } = await execFileAsync('git', fullArgs, { + cwd, + timeout: GIT_TIMEOUT_MS, + maxBuffer: 64 * 1024 * 1024, + windowsHide: true, + encoding: 'utf8', + }); + return stdout; + } catch { + return null; + } +} From e0777d22405572ccba2645274521488c44349895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 21 Apr 2026 11:39:54 +0800 Subject: [PATCH 02/23] feat(cli): add /diff slash command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surface the `fetchGitDiff` utility through an interactive `/diff` command. Prints a header (`N files changed, +A / -R`) followed by per-file rows with padded add/remove counts. Untracked files are marked `?`, binary files are marked `~`. When the change set exceeds the per-file cap, a trailing `…and N more` note tells the user how many entries are hidden. Returns a `MessageActionReturn` so it renders the same way in interactive and non-interactive modes. --- .../cli/src/services/BuiltinCommandLoader.ts | 2 + .../cli/src/ui/commands/diffCommand.test.ts | 126 ++++++++++++++++++ packages/cli/src/ui/commands/diffCommand.ts | 109 +++++++++++++++ 3 files changed, 237 insertions(+) create mode 100644 packages/cli/src/ui/commands/diffCommand.test.ts create mode 100644 packages/cli/src/ui/commands/diffCommand.ts diff --git a/packages/cli/src/services/BuiltinCommandLoader.ts b/packages/cli/src/services/BuiltinCommandLoader.ts index 05bd26981ad..3a993f93274 100644 --- a/packages/cli/src/services/BuiltinCommandLoader.ts +++ b/packages/cli/src/services/BuiltinCommandLoader.ts @@ -20,6 +20,7 @@ import { contextCommand } from '../ui/commands/contextCommand.js'; import { copyCommand } from '../ui/commands/copyCommand.js'; import { docsCommand } from '../ui/commands/docsCommand.js'; import { doctorCommand } from '../ui/commands/doctorCommand.js'; +import { diffCommand } from '../ui/commands/diffCommand.js'; import { directoryCommand } from '../ui/commands/directoryCommand.js'; import { editorCommand } from '../ui/commands/editorCommand.js'; import { exportCommand } from '../ui/commands/exportCommand.js'; @@ -97,6 +98,7 @@ export class BuiltinCommandLoader implements ICommandLoader { compressCommand, contextCommand, copyCommand, + diffCommand, docsCommand, doctorCommand, directoryCommand, diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts new file mode 100644 index 00000000000..b32d241b620 --- /dev/null +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -0,0 +1,126 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { Mock } from 'vitest'; +import { vi, describe, it, expect, beforeEach } from 'vitest'; +import { diffCommand } from './diffCommand.js'; +import { type CommandContext } from './types.js'; +import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; +import { fetchGitDiff } from '@qwen-code/qwen-code-core'; + +vi.mock('@qwen-code/qwen-code-core', async () => { + const actual = await vi.importActual< + typeof import('@qwen-code/qwen-code-core') + >('@qwen-code/qwen-code-core'); + return { + ...actual, + fetchGitDiff: vi.fn(), + }; +}); + +describe('diffCommand', () => { + let mockContext: CommandContext; + let mockFetchGitDiff: Mock; + + beforeEach(() => { + vi.clearAllMocks(); + mockFetchGitDiff = vi.mocked(fetchGitDiff) as unknown as Mock; + + mockContext = createMockCommandContext({ + services: { + config: { + getWorkingDir: () => '/tmp/repo', + getProjectRoot: () => '/tmp/repo', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any, + }, + }); + }); + + it('errors when config is unavailable', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const noConfigContext = createMockCommandContext(); + const result = await diffCommand.action(noConfigContext, ''); + expect(result).toMatchObject({ type: 'message', messageType: 'error' }); + }); + + it('reports when not in a git repo or transient state', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue(null); + const result = await diffCommand.action(mockContext, ''); + expect(result).toMatchObject({ + type: 'message', + messageType: 'info', + }); + expect((result as { content: string }).content).toMatch( + /not a git repository|merge|rebase/i, + ); + }); + + it('reports clean working tree when stats show zero changes', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 0, linesAdded: 0, linesRemoved: 0 }, + perFileStats: new Map(), + }); + const result = await diffCommand.action(mockContext, ''); + expect((result as { content: string }).content).toMatch( + /Clean working tree/i, + ); + }); + + it('renders header and per-file rows with +added / -removed', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 2, linesAdded: 7, linesRemoved: 3 }, + perFileStats: new Map([ + ['src/a.ts', { added: 5, removed: 2, isBinary: false }], + ['src/b.ts', { added: 2, removed: 1, isBinary: false }], + ]), + }); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + expect(content).toContain('2 files changed'); + expect(content).toContain('+7'); + expect(content).toContain('-3'); + expect(content).toContain('src/a.ts'); + expect(content).toContain('src/b.ts'); + }); + + it('marks untracked and binary entries distinctly', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 2, linesAdded: 0, linesRemoved: 0 }, + perFileStats: new Map([ + [ + 'new.txt', + { added: 0, removed: 0, isBinary: false, isUntracked: true }, + ], + ['img.png', { added: 0, removed: 0, isBinary: true }], + ]), + }); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + expect(content).toContain('?'); + expect(content).toContain('new.txt'); + expect(content).toContain('(binary)'); + expect(content).toContain('img.png'); + }); + + it('notes how many files were hidden beyond the per-file cap', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 60, linesAdded: 100, linesRemoved: 20 }, + perFileStats: new Map([ + ['src/a.ts', { added: 1, removed: 0, isBinary: false }], + ]), + }); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + expect(content).toContain('60 files changed'); + expect(content).toMatch(/59 more/); + }); +}); diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts new file mode 100644 index 00000000000..4311fefb1f2 --- /dev/null +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -0,0 +1,109 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { fetchGitDiff, type PerFileStats } from '@qwen-code/qwen-code-core'; +import { + CommandKind, + type CommandContext, + type MessageActionReturn, + type SlashCommand, +} from './types.js'; +import { t } from '../../i18n/index.js'; + +async function diffAction( + context: CommandContext, +): Promise { + const { config } = context.services; + if (!config) { + return { + type: 'message', + messageType: 'error', + content: t('Configuration not available.'), + }; + } + + const cwd = config.getWorkingDir() || config.getProjectRoot(); + if (!cwd) { + return { + type: 'message', + messageType: 'error', + content: t('Could not determine current working directory.'), + }; + } + + const result = await fetchGitDiff(cwd); + if (!result) { + return { + type: 'message', + messageType: 'info', + content: t( + 'No diff available. Either this is not a git repository, HEAD is missing, or a merge/rebase/cherry-pick/revert is in progress.', + ), + }; + } + + const { stats, perFileStats } = result; + if (stats.filesCount === 0) { + return { + type: 'message', + messageType: 'info', + content: t('Clean working tree — no changes against HEAD.'), + }; + } + + const fileWord = stats.filesCount === 1 ? 'file' : 'files'; + const header = `${stats.filesCount} ${fileWord} changed, +${stats.linesAdded} / -${stats.linesRemoved}`; + const rows = formatPerFile(perFileStats); + const hidden = stats.filesCount - perFileStats.size; + const capNote = + hidden > 0 + ? `\n …and ${hidden} more (showing first ${perFileStats.size})` + : ''; + + return { + type: 'message', + messageType: 'info', + content: + rows.length > 0 ? `${header}\n${rows.join('\n')}${capNote}` : header, + }; +} + +function formatPerFile(perFileStats: Map): string[] { + const rows: string[] = []; + const maxAdded = Math.max( + 0, + ...[...perFileStats.values()].map((s) => s.added), + ); + const maxRemoved = Math.max( + 0, + ...[...perFileStats.values()].map((s) => s.removed), + ); + const addWidth = String(maxAdded).length; + const remWidth = String(maxRemoved).length; + + for (const [filename, s] of perFileStats) { + if (s.isUntracked) { + rows.push(` ? ${filename}`); + } else if (s.isBinary) { + rows.push(` ~ ${filename} (binary)`); + } else { + const added = `+${String(s.added).padStart(addWidth)}`; + const removed = `-${String(s.removed).padStart(remWidth)}`; + rows.push(` ${added} ${removed} ${filename}`); + } + } + return rows; +} + +export const diffCommand: SlashCommand = { + name: 'diff', + get description() { + return t('Show working-tree change stats versus HEAD'); + }, + kind: CommandKind.BUILT_IN, + commandType: 'local', + action: diffAction, +}; From 48b79efe4783b9f2f727a4ecd6dce261b41e7da6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 21 Apr 2026 14:12:10 +0800 Subject: [PATCH 03/23] fix(cli): harden /diff command after adversarial audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wrap `fetchGitDiff` in try/catch so permission errors on `.git` surface as a friendly error message instead of crashing the action. - Declare `supportedModes: ['interactive', 'non_interactive', 'acp']` so the command is reachable outside the interactive Ink UI — the default for `commandType: 'local'` is interactive-only. - Align `?` (untracked) and `~` (binary) markers with the `+X -Y` stat column via a padded prefix, so filenames line up regardless of row kind. - Drop the "…and N more" hint when no rows are shown (shortstat fast-path with >500 files) — the count alone is sufficient and "showing first 0" is noise. - Switch header to full-phrase i18n templates (separate singular/plural variants) instead of word-by-word `t()` calls that don't survive non-English locales. - Extend tests to 12 scenarios: empty cwd, fetch rejection, singular "file" form, mixed untracked/binary/tracked alignment, 4-digit padding, shortstat fast-path, and supportedModes declaration. Mocks carry a `satisfies GitDiffResult` annotation so shape drift in core breaks the test at compile time. --- .../cli/src/ui/commands/diffCommand.test.ts | 122 +++++++++++++++--- packages/cli/src/ui/commands/diffCommand.ts | 69 +++++++--- 2 files changed, 157 insertions(+), 34 deletions(-) diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index b32d241b620..92ccb40a1d0 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -9,7 +9,7 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import { diffCommand } from './diffCommand.js'; import { type CommandContext } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; -import { fetchGitDiff } from '@qwen-code/qwen-code-core'; +import { fetchGitDiff, type GitDiffResult } from '@qwen-code/qwen-code-core'; vi.mock('@qwen-code/qwen-code-core', async () => { const actual = await vi.importActual< @@ -21,30 +21,58 @@ vi.mock('@qwen-code/qwen-code-core', async () => { }; }); +function makeContextWithCwd(cwd = '/tmp/repo'): CommandContext { + return createMockCommandContext({ + services: { + config: { + getWorkingDir: () => cwd, + getProjectRoot: () => cwd, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any, + }, + }); +} + describe('diffCommand', () => { let mockContext: CommandContext; let mockFetchGitDiff: Mock; beforeEach(() => { vi.clearAllMocks(); - mockFetchGitDiff = vi.mocked(fetchGitDiff) as unknown as Mock; + mockFetchGitDiff = vi.mocked(fetchGitDiff); + mockContext = makeContextWithCwd(); + }); + + it('errors when config is unavailable', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const noConfigContext = createMockCommandContext(); + const result = await diffCommand.action(noConfigContext, ''); + expect(result).toMatchObject({ type: 'message', messageType: 'error' }); + }); - mockContext = createMockCommandContext({ + it('errors when getWorkingDir and getProjectRoot both return empty', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const noCwdContext = createMockCommandContext({ services: { config: { - getWorkingDir: () => '/tmp/repo', - getProjectRoot: () => '/tmp/repo', + getWorkingDir: () => '', + getProjectRoot: () => '', // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any, }, }); + const result = await diffCommand.action(noCwdContext, ''); + expect(result).toMatchObject({ type: 'message', messageType: 'error' }); }); - it('errors when config is unavailable', async () => { + it('surfaces an error when fetchGitDiff throws', async () => { if (!diffCommand.action) throw new Error('Command has no action'); - const noConfigContext = createMockCommandContext(); - const result = await diffCommand.action(noConfigContext, ''); + mockFetchGitDiff.mockRejectedValueOnce(new Error('permission denied')); + const result = await diffCommand.action(mockContext, ''); expect(result).toMatchObject({ type: 'message', messageType: 'error' }); + expect((result as { content: string }).content).toContain( + 'permission denied', + ); }); it('reports when not in a git repo or transient state', async () => { @@ -65,13 +93,27 @@ describe('diffCommand', () => { mockFetchGitDiff.mockResolvedValue({ stats: { filesCount: 0, linesAdded: 0, linesRemoved: 0 }, perFileStats: new Map(), - }); + } satisfies GitDiffResult); const result = await diffCommand.action(mockContext, ''); expect((result as { content: string }).content).toMatch( /Clean working tree/i, ); }); + it('uses singular "file" when exactly one file changed', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1, linesAdded: 3, linesRemoved: 1 }, + perFileStats: new Map([ + ['src/a.ts', { added: 3, removed: 1, isBinary: false }], + ]), + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + expect(content).toMatch(/\b1 file\b/); + expect(content).not.toMatch(/\b1 files\b/); + }); + it('renders header and per-file rows with +added / -removed', async () => { if (!diffCommand.action) throw new Error('Command has no action'); mockFetchGitDiff.mockResolvedValue({ @@ -80,7 +122,7 @@ describe('diffCommand', () => { ['src/a.ts', { added: 5, removed: 2, isBinary: false }], ['src/b.ts', { added: 2, removed: 1, isBinary: false }], ]), - }); + } satisfies GitDiffResult); const result = await diffCommand.action(mockContext, ''); const content = (result as { content: string }).content; expect(content).toContain('2 files changed'); @@ -90,24 +132,46 @@ describe('diffCommand', () => { expect(content).toContain('src/b.ts'); }); - it('marks untracked and binary entries distinctly', async () => { + it('aligns untracked/binary rows with the numeric stat column', async () => { if (!diffCommand.action) throw new Error('Command has no action'); mockFetchGitDiff.mockResolvedValue({ - stats: { filesCount: 2, linesAdded: 0, linesRemoved: 0 }, + stats: { filesCount: 3, linesAdded: 10, linesRemoved: 2 }, perFileStats: new Map([ + ['src/a.ts', { added: 10, removed: 2, isBinary: false }], [ 'new.txt', { added: 0, removed: 0, isBinary: false, isUntracked: true }, ], ['img.png', { added: 0, removed: 0, isBinary: true }], ]), - }); + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const lines = (result as { content: string }).content.split('\n'); + const aLine = lines.find((l) => l.endsWith('src/a.ts'))!; + const newLine = lines.find((l) => l.endsWith('new.txt'))!; + const imgLine = lines.find((l) => l.endsWith('img.png (binary)'))!; + // Filename column starts at the same offset in every row so that `?` / `~` + // markers line up with `+X -Y` entries. + expect(aLine.indexOf('src/a.ts')).toBe(newLine.indexOf('new.txt')); + expect(aLine.indexOf('src/a.ts')).toBe(imgLine.indexOf('img.png')); + }); + + it('pads counts consistently for 4-digit values', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 2, linesAdded: 9999, linesRemoved: 1 }, + perFileStats: new Map([ + ['big.ts', { added: 9999, removed: 0, isBinary: false }], + ['tiny.ts', { added: 0, removed: 1, isBinary: false }], + ]), + } satisfies GitDiffResult); const result = await diffCommand.action(mockContext, ''); const content = (result as { content: string }).content; - expect(content).toContain('?'); - expect(content).toContain('new.txt'); - expect(content).toContain('(binary)'); - expect(content).toContain('img.png'); + // Both rows must use the same prefix width so they align. + const bigLine = content.split('\n').find((l) => l.endsWith('big.ts'))!; + const tinyLine = content.split('\n').find((l) => l.endsWith('tiny.ts'))!; + expect(bigLine.indexOf('big.ts')).toBe(tinyLine.indexOf('tiny.ts')); + expect(content).toContain('+9999'); }); it('notes how many files were hidden beyond the per-file cap', async () => { @@ -117,10 +181,32 @@ describe('diffCommand', () => { perFileStats: new Map([ ['src/a.ts', { added: 1, removed: 0, isBinary: false }], ]), - }); + } satisfies GitDiffResult); const result = await diffCommand.action(mockContext, ''); const content = (result as { content: string }).content; expect(content).toContain('60 files changed'); expect(content).toMatch(/59 more/); }); + + it('shows header only when the shortstat fast path yields no per-file data', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1000, linesAdded: 50_000, linesRemoved: 8_000 }, + perFileStats: new Map(), + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + expect(content).toContain('1000 files changed'); + expect(content).not.toMatch(/more \(showing first/); + }); +}); + +describe('diffCommand registration', () => { + it('declares all execution modes so it works in non-interactive and ACP', () => { + expect(diffCommand.supportedModes).toEqual([ + 'interactive', + 'non_interactive', + 'acp', + ]); + }); }); diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index 4311fefb1f2..af6adcc3d41 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -34,7 +34,19 @@ async function diffAction( }; } - const result = await fetchGitDiff(cwd); + let result: Awaited>; + try { + result = await fetchGitDiff(cwd); + } catch (error) { + return { + type: 'message', + messageType: 'error', + content: `${t('Failed to compute git diff stats')}: ${ + error instanceof Error ? error.message : String(error) + }`, + }; + } + if (!result) { return { type: 'message', @@ -54,13 +66,26 @@ async function diffAction( }; } - const fileWord = stats.filesCount === 1 ? 'file' : 'files'; - const header = `${stats.filesCount} ${fileWord} changed, +${stats.linesAdded} / -${stats.linesRemoved}`; + const header = + stats.filesCount === 1 + ? t('{{count}} file changed, +{{added}} / -{{removed}}', { + count: String(stats.filesCount), + added: String(stats.linesAdded), + removed: String(stats.linesRemoved), + }) + : t('{{count}} files changed, +{{added}} / -{{removed}}', { + count: String(stats.filesCount), + added: String(stats.linesAdded), + removed: String(stats.linesRemoved), + }); const rows = formatPerFile(perFileStats); const hidden = stats.filesCount - perFileStats.size; const capNote = - hidden > 0 - ? `\n …and ${hidden} more (showing first ${perFileStats.size})` + hidden > 0 && perFileStats.size > 0 + ? `\n ${t('…and {{hidden}} more (showing first {{shown}})', { + hidden: String(hidden), + shown: String(perFileStats.size), + })}` : ''; return { @@ -72,23 +97,28 @@ async function diffAction( } function formatPerFile(perFileStats: Map): string[] { - const rows: string[] = []; - const maxAdded = Math.max( - 0, - ...[...perFileStats.values()].map((s) => s.added), - ); - const maxRemoved = Math.max( - 0, - ...[...perFileStats.values()].map((s) => s.removed), - ); + if (perFileStats.size === 0) return []; + + let maxAdded = 0; + let maxRemoved = 0; + for (const s of perFileStats.values()) { + if (s.isBinary || s.isUntracked) continue; + if (s.added > maxAdded) maxAdded = s.added; + if (s.removed > maxRemoved) maxRemoved = s.removed; + } const addWidth = String(maxAdded).length; const remWidth = String(maxRemoved).length; + // Width of the `+X -Y` stat column so `?` / `~` rows line up with it. + const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + const rows: string[] = []; for (const [filename, s] of perFileStats) { if (s.isUntracked) { - rows.push(` ? ${filename}`); + rows.push(` ${padMarker('?', statColumnWidth)} ${filename}`); } else if (s.isBinary) { - rows.push(` ~ ${filename} (binary)`); + rows.push( + ` ${padMarker('~', statColumnWidth)} ${filename} ${t('(binary)')}`, + ); } else { const added = `+${String(s.added).padStart(addWidth)}`; const removed = `-${String(s.removed).padStart(remWidth)}`; @@ -98,6 +128,12 @@ function formatPerFile(perFileStats: Map): string[] { return rows; } +function padMarker(marker: string, width: number): string { + if (marker.length >= width) return marker; + const pad = ' '.repeat(width - marker.length); + return `${marker}${pad}`; +} + export const diffCommand: SlashCommand = { name: 'diff', get description() { @@ -105,5 +141,6 @@ export const diffCommand: SlashCommand = { }, kind: CommandKind.BUILT_IN, commandType: 'local', + supportedModes: ['interactive', 'non_interactive', 'acp'] as const, action: diffAction, }; From 055cd85efaa98565ac404e9c8310073d7cdfc835 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Fri, 24 Apr 2026 15:27:10 +0800 Subject: [PATCH 04/23] fix(cli): clean up /diff feature review issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove invalid `commandType` field from diffCommand (SlashCommand has no such property; caused a TS build failure). - Drop duplicate `NumstatResult` interface in gitDiff.ts — it is structurally identical to `GitDiffResult`. - Register the 9 missing `/diff` i18n strings in en.js / zh.js so the command is translatable (previously only `Configuration not available.` had entries). --- packages/cli/src/i18n/locales/en.js | 16 ++++++++++++++++ packages/cli/src/i18n/locales/zh.js | 15 +++++++++++++++ packages/cli/src/ui/commands/diffCommand.ts | 1 - packages/core/src/utils/gitDiff.ts | 7 +------ scripts/unused-keys-only-in-locales.json | 2 +- 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index c2a427bdb70..01b8a15352d 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -153,6 +153,22 @@ export default { 'Configure authentication information for login', 'Copy the last result or code snippet to clipboard': 'Copy the last result or code snippet to clipboard', + 'Show working-tree change stats versus HEAD': + 'Show working-tree change stats versus HEAD', + 'Could not determine current working directory.': + 'Could not determine current working directory.', + 'Failed to compute git diff stats': 'Failed to compute git diff stats', + 'No diff available. Either this is not a git repository, HEAD is missing, or a merge/rebase/cherry-pick/revert is in progress.': + 'No diff available. Either this is not a git repository, HEAD is missing, or a merge/rebase/cherry-pick/revert is in progress.', + 'Clean working tree — no changes against HEAD.': + 'Clean working tree — no changes against HEAD.', + '{{count}} file changed, +{{added}} / -{{removed}}': + '{{count}} file changed, +{{added}} / -{{removed}}', + '{{count}} files changed, +{{added}} / -{{removed}}': + '{{count}} files changed, +{{added}} / -{{removed}}', + '…and {{hidden}} more (showing first {{shown}})': + '…and {{hidden}} more (showing first {{shown}})', + '(binary)': '(binary)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index 4c3c98ba604..2bfde8c6f2f 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -149,6 +149,21 @@ export default { 'Configure authentication information for login': '配置登录认证信息', 'Copy the last result or code snippet to clipboard': '将最后的结果或代码片段复制到剪贴板', + 'Show working-tree change stats versus HEAD': + '显示工作区相对 HEAD 的变更统计', + 'Could not determine current working directory.': '无法确定当前工作目录。', + 'Failed to compute git diff stats': '计算 git diff 统计失败', + 'No diff available. Either this is not a git repository, HEAD is missing, or a merge/rebase/cherry-pick/revert is in progress.': + '无可用 diff。可能不是 Git 仓库、HEAD 缺失,或正在执行 merge/rebase/cherry-pick/revert。', + 'Clean working tree — no changes against HEAD.': + '工作区干净 —— 与 HEAD 无差异。', + '{{count}} file changed, +{{added}} / -{{removed}}': + '{{count}} 个文件变更,+{{added}} / -{{removed}}', + '{{count}} files changed, +{{added}} / -{{removed}}': + '{{count}} 个文件变更,+{{added}} / -{{removed}}', + '…and {{hidden}} more (showing first {{shown}})': + '…还有 {{hidden}} 个(仅显示前 {{shown}} 个)', + '(binary)': '(二进制)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index af6adcc3d41..cd67b97804c 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -140,7 +140,6 @@ export const diffCommand: SlashCommand = { return t('Show working-tree change stats versus HEAD'); }, kind: CommandKind.BUILT_IN, - commandType: 'local', supportedModes: ['interactive', 'non_interactive', 'acp'] as const, action: diffAction, }; diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index f8b71258502..63fd8756b39 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -34,11 +34,6 @@ export interface GitDiffResult { perFileStats: Map; } -export interface NumstatResult { - stats: GitDiffStats; - perFileStats: Map; -} - const GIT_TIMEOUT_MS = 5000; /** Maximum files retained in per-file results. Matches issue #2997 "50 files" cap. */ export const MAX_FILES = 50; @@ -137,7 +132,7 @@ export async function fetchGitDiffHunks( * counts. Only the first `MAX_FILES` entries are kept in `perFileStats`, but * total stats account for every line. */ -export function parseGitNumstat(stdout: string): NumstatResult { +export function parseGitNumstat(stdout: string): GitDiffResult { const lines = stdout.split('\n').filter(Boolean); let added = 0; let removed = 0; diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index 8c0076ffd32..abf3e862d0b 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-22T15:40:32.318Z", + "generatedAt": "2026-04-24T07:26:06.808Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)", From d88eba17528cc4db535d496802afd67213158c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Fri, 24 Apr 2026 15:37:02 +0800 Subject: [PATCH 05/23] fix(core): harden git diff stats after multi-round review - fetchUntrackedPaths now uses `ls-files -z` so filenames containing newlines, tabs, or non-ASCII bytes round-trip cleanly instead of being C-style quoted and split into phantom entries. - fetchGitDiff runs the `--shortstat` probe and the untracked-paths lookup in parallel, since both are needed regardless of which path the function takes. - parseGitDiff measures per-file diff size via Buffer.byteLength so MAX_DIFF_SIZE_BYTES matches its documented meaning on non-ASCII diffs. - Adds a regression test for an untracked file whose name contains a literal newline. --- packages/core/src/utils/gitDiff.test.ts | 39 +++++++++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 31 +++++++++++--------- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 128b531d058..1953cc2ff74 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -478,6 +478,45 @@ describe('fetchGitDiff non-ASCII filenames', () => { }); }); +describe('fetchGitDiff untracked with special filenames', () => { + it('counts an untracked file whose name contains a newline as one entry', async () => { + // Skip on platforms where the filesystem rejects `\n` in names (e.g. some + // Windows filesystems). POSIX filesystems accept it; we rely on that here. + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-nl-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@example.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'seed'], { cwd: repo }); + + const weirdName = 'line1\nline2.txt'; + try { + await fs.writeFile(path.join(repo, weirdName), 'content\n'); + } catch { + // Filesystem refused newline in name — nothing to assert here. + return; + } + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + // Without `-z`, `ls-files` would quote this as `"line1\nline2.txt"` + // and split-on-\n would produce two phantom entries. With `-z` we get + // exactly one entry, keyed by the real name. + expect(result!.stats.filesCount).toBe(1); + expect(result!.perFileStats.has(weirdName)).toBe(true); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); +}); + describe('fetchGitDiff untracked counting', () => { let repo: string; beforeEach(async () => { diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 63fd8756b39..1fea8d597d7 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -58,15 +58,14 @@ export async function fetchGitDiff(cwd: string): Promise { if (!isGitRepository(cwd)) return null; if (await isInTransientGitState(cwd)) return null; - // Quick probe first — O(1) memory regardless of diff size. Lets us bail out - // of huge diffs (e.g. generated workspaces) before paying for per-file work. - const shortstatOut = await runGit( - ['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], - cwd, - ); - // Fetch untracked filenames up front so both the shortstat fast path and - // the numstat slow path report the same `filesCount` surface. - const untrackedPaths = (await fetchUntrackedPaths(cwd)) ?? []; + // Quick probe + untracked list run in parallel — both are needed regardless + // of which path we take, and shortstat is O(1) memory so it can short-circuit + // huge generated workspaces before we pay the per-file numstat cost. + const [shortstatOut, untrackedPathsRaw] = await Promise.all([ + runGit(['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], cwd), + fetchUntrackedPaths(cwd), + ]); + const untrackedPaths = untrackedPathsRaw ?? []; if (shortstatOut != null) { const quickStats = parseShortstat(shortstatOut); @@ -189,7 +188,10 @@ export function parseGitDiff(stdout: string): Map { for (const fileDiff of fileDiffs) { if (result.size >= MAX_FILES) break; - if (fileDiff.length > MAX_DIFF_SIZE_BYTES) continue; + // Use UTF-8 byte length (not JS string .length, which counts UTF-16 code + // units) so the cap matches the documented `MAX_DIFF_SIZE_BYTES` semantic + // on non-ASCII diffs. + if (Buffer.byteLength(fileDiff, 'utf8') > MAX_DIFF_SIZE_BYTES) continue; const lines = fileDiff.split('\n'); const headerMatch = lines[0]?.match(/^a\/(.+?) b\/(.+)$/); @@ -309,12 +311,15 @@ async function isInTransientGitState(cwd: string): Promise { } async function fetchUntrackedPaths(cwd: string): Promise { + // `-z` emits NUL-delimited paths with no C-style quoting, so filenames + // containing newlines, tabs, or non-ASCII bytes round-trip cleanly. const stdout = await runGit( - ['--no-optional-locks', 'ls-files', '--others', '--exclude-standard'], + ['--no-optional-locks', 'ls-files', '-z', '--others', '--exclude-standard'], cwd, ); - if (!stdout || !stdout.trim()) return null; - return stdout.trim().split('\n').filter(Boolean); + if (!stdout) return null; + const paths = stdout.split('\0').filter(Boolean); + return paths.length > 0 ? paths : null; } async function runGit(args: string[], cwd: string): Promise { From 005f88e2e4ccd457d867a44bd8d73bd0019c384d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Fri, 24 Apr 2026 16:13:32 +0800 Subject: [PATCH 06/23] fix(core): address /diff PR review comments Addresses the five open review threads on #3491: - parseShortstat: anchored and bounded the regex (`^...$` with `\d{1,10}`) so adversarial inputs can no longer drive polynomial backtracking. Closes CodeQL alert #137. - fetchGitDiff: only parse the untracked-path list when we actually need it; the fast path now counts NUL bytes in the raw `ls-files -z` stdout (wenshao P1). - fetchGitDiff: base the `MAX_FILES_FOR_DETAILS` short-circuit on `tracked + untracked`, so repos with few edits but many untracked files still take the summary-only path (wenshao P2). - fetchGitDiff: count newlines in each untracked text file (binary sniff + 1 MB read cap) and fold that into both the header `+N` and the per-file row, so a brand-new file no longer renders as `+0 / -0` (BZ-D P2). - parseGitNumstat: switch to `git diff --numstat -z`. The parser now uses index-based slicing and a rename-pair state machine, so tracked filenames containing tabs/newlines/non-ASCII keep their real bytes (BZ-D P3). Renames collapse into a single `old => new` entry. UI: untracked rows render as `+N filename (new)` (or `~ filename (binary, new)`) instead of the placeholder `?` marker; `/diff` now shows real additions for fresh files. --- packages/cli/src/i18n/locales/en.js | 2 + packages/cli/src/i18n/locales/zh.js | 2 + .../cli/src/ui/commands/diffCommand.test.ts | 42 +++- packages/cli/src/ui/commands/diffCommand.ts | 18 +- packages/core/src/utils/gitDiff.test.ts | 150 +++++++++++- packages/core/src/utils/gitDiff.ts | 229 ++++++++++++++---- scripts/unused-keys-only-in-locales.json | 2 +- 7 files changed, 363 insertions(+), 82 deletions(-) diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index 01b8a15352d..3b6b7f02066 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -169,6 +169,8 @@ export default { '…and {{hidden}} more (showing first {{shown}})': '…and {{hidden}} more (showing first {{shown}})', '(binary)': '(binary)', + '(binary, new)': '(binary, new)', + '(new)': '(new)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index 2bfde8c6f2f..63c26b41226 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -164,6 +164,8 @@ export default { '…and {{hidden}} more (showing first {{shown}})': '…还有 {{hidden}} 个(仅显示前 {{shown}} 个)', '(binary)': '(二进制)', + '(binary, new)': '(二进制,新增)', + '(new)': '(新增)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index 92ccb40a1d0..71c36446d7c 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -132,28 +132,46 @@ describe('diffCommand', () => { expect(content).toContain('src/b.ts'); }); - it('aligns untracked/binary rows with the numeric stat column', async () => { + it('shows untracked text files with their line count and a (new) marker', async () => { if (!diffCommand.action) throw new Error('Command has no action'); mockFetchGitDiff.mockResolvedValue({ - stats: { filesCount: 3, linesAdded: 10, linesRemoved: 2 }, + stats: { filesCount: 2, linesAdded: 12, linesRemoved: 2 }, perFileStats: new Map([ ['src/a.ts', { added: 10, removed: 2, isBinary: false }], [ - 'new.txt', - { added: 0, removed: 0, isBinary: false, isUntracked: true }, + 'notes.md', + { added: 2, removed: 0, isBinary: false, isUntracked: true }, ], - ['img.png', { added: 0, removed: 0, isBinary: true }], ]), } satisfies GitDiffResult); const result = await diffCommand.action(mockContext, ''); - const lines = (result as { content: string }).content.split('\n'); + const content = (result as { content: string }).content; + const lines = content.split('\n'); const aLine = lines.find((l) => l.endsWith('src/a.ts'))!; - const newLine = lines.find((l) => l.endsWith('new.txt'))!; - const imgLine = lines.find((l) => l.endsWith('img.png (binary)'))!; - // Filename column starts at the same offset in every row so that `?` / `~` - // markers line up with `+X -Y` entries. - expect(aLine.indexOf('src/a.ts')).toBe(newLine.indexOf('new.txt')); - expect(aLine.indexOf('src/a.ts')).toBe(imgLine.indexOf('img.png')); + const newLine = lines.find((l) => l.includes('notes.md'))!; + expect(newLine).toContain('+ 2'); + expect(newLine).toContain('(new)'); + // Stat columns stay aligned across tracked and new rows. + expect(aLine.indexOf('src/a.ts')).toBe(newLine.indexOf('notes.md')); + }); + + it('marks binary untracked files with (binary, new) and no line count', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1, linesAdded: 0, linesRemoved: 0 }, + perFileStats: new Map([ + [ + 'blob.bin', + { added: 0, removed: 0, isBinary: true, isUntracked: true }, + ], + ]), + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + const binaryLine = content.split('\n').find((l) => l.includes('blob.bin'))!; + expect(binaryLine).toContain('(binary, new)'); + expect(binaryLine).not.toMatch(/\+\d/); + expect(binaryLine.trimStart().startsWith('~')).toBe(true); }); it('pads counts consistently for 4-digit values', async () => { diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index cd67b97804c..b87be89be0c 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -102,27 +102,27 @@ function formatPerFile(perFileStats: Map): string[] { let maxAdded = 0; let maxRemoved = 0; for (const s of perFileStats.values()) { - if (s.isBinary || s.isUntracked) continue; + if (s.isBinary) continue; if (s.added > maxAdded) maxAdded = s.added; if (s.removed > maxRemoved) maxRemoved = s.removed; } const addWidth = String(maxAdded).length; const remWidth = String(maxRemoved).length; - // Width of the `+X -Y` stat column so `?` / `~` rows line up with it. + // Width of the `+X -Y` stat column so `~` (binary) rows line up with it. const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; const rows: string[] = []; for (const [filename, s] of perFileStats) { - if (s.isUntracked) { - rows.push(` ${padMarker('?', statColumnWidth)} ${filename}`); - } else if (s.isBinary) { - rows.push( - ` ${padMarker('~', statColumnWidth)} ${filename} ${t('(binary)')}`, - ); + if (s.isBinary) { + const suffix = s.isUntracked + ? ` ${t('(binary, new)')}` + : ` ${t('(binary)')}`; + rows.push(` ${padMarker('~', statColumnWidth)} ${filename}${suffix}`); } else { const added = `+${String(s.added).padStart(addWidth)}`; const removed = `-${String(s.removed).padStart(remWidth)}`; - rows.push(` ${added} ${removed} ${filename}`); + const suffix = s.isUntracked ? ` ${t('(new)')}` : ''; + rows.push(` ${added} ${removed} ${filename}${suffix}`); } } return rows; diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 1953cc2ff74..a118d85873e 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -38,8 +38,8 @@ async function makeRepo(): Promise { } describe('parseGitNumstat', () => { - it('parses added/removed counts and file totals', () => { - const out = '3\t1\tsrc/a.ts\n10\t0\tsrc/b.ts\n0\t5\tsrc/c.ts\n'; + it('parses added/removed counts and file totals (NUL-delimited -z format)', () => { + const out = '3\t1\tsrc/a.ts\0' + '10\t0\tsrc/b.ts\0' + '0\t5\tsrc/c.ts\0'; const { stats, perFileStats } = parseGitNumstat(out); expect(stats).toEqual({ filesCount: 3, @@ -55,7 +55,7 @@ describe('parseGitNumstat', () => { }); it('treats `-` counts as binary with zero line deltas', () => { - const out = '-\t-\timg/logo.png\n'; + const out = '-\t-\timg/logo.png\0'; const { stats, perFileStats } = parseGitNumstat(out); expect(stats.filesCount).toBe(1); expect(stats.linesAdded).toBe(0); @@ -68,28 +68,44 @@ describe('parseGitNumstat', () => { }); it('keeps accurate totals but caps per-file entries at MAX_FILES', () => { - const lines: string[] = []; + const tokens: string[] = []; const totalFiles = MAX_FILES + 5; for (let i = 0; i < totalFiles; i++) { - lines.push(`1\t0\tfile${i}.ts`); + tokens.push(`1\t0\tfile${i}.ts`); } - const { stats, perFileStats } = parseGitNumstat(lines.join('\n')); + const { stats, perFileStats } = parseGitNumstat(tokens.join('\0') + '\0'); expect(stats.filesCount).toBe(totalFiles); expect(stats.linesAdded).toBe(totalFiles); expect(perFileStats.size).toBe(MAX_FILES); }); it('ignores malformed rows without crashing', () => { - const out = 'garbage-line\n2\t1\tsrc/a.ts\n'; + const out = 'garbage-token\0' + '2\t1\tsrc/a.ts\0'; const { stats, perFileStats } = parseGitNumstat(out); expect(stats.filesCount).toBe(1); expect(perFileStats.has('src/a.ts')).toBe(true); }); - it('handles filenames containing tabs', () => { - const out = '1\t2\tweird\tname.ts\n'; + it('preserves literal tabs in tracked filenames via the -z wire format', () => { + // With -z, git emits the raw path; no C-style quoting. `split('\t')` + // would mis-attribute characters after the first tab, so the parser has + // to use index-based slicing instead. + const out = '1\t2\tweird\tname.ts\0'; const { perFileStats } = parseGitNumstat(out); expect(perFileStats.has('weird\tname.ts')).toBe(true); + expect(perFileStats.get('weird\tname.ts')).toEqual({ + added: 1, + removed: 2, + isBinary: false, + }); + }); + + it('combines rename-pair tokens into a single entry', () => { + // `-z` rename format: `\t\t\0\0\0`. + const out = '0\t0\t\0' + 'src/old.ts\0' + 'src/new.ts\0'; + const { stats, perFileStats } = parseGitNumstat(out); + expect(stats.filesCount).toBe(1); + expect(perFileStats.has('src/old.ts => src/new.ts')).toBe(true); }); }); @@ -205,7 +221,7 @@ describe('fetchGitDiff', () => { } }); - it('captures tracked modifications and untracked files', async () => { + it('captures tracked modifications and counts lines in untracked text files', async () => { await fs.writeFile(path.join(repo, 'tracked.txt'), 'one\ntwo\nthree\n'); await git(repo, 'add', '.'); await git(repo, 'commit', '-q', '-m', 'init'); @@ -214,14 +230,43 @@ describe('fetchGitDiff', () => { path.join(repo, 'tracked.txt'), 'one\ntwo\nthree\nfour\n', ); - await fs.writeFile(path.join(repo, 'new.txt'), 'brand new\n'); + await fs.writeFile(path.join(repo, 'new.txt'), 'brand new\nsecond\n'); const result = await fetchGitDiff(repo); expect(result).not.toBeNull(); - expect(result!.stats.linesAdded).toBeGreaterThanOrEqual(1); expect(result!.stats.filesCount).toBe(2); + // Tracked: +1 from adding `four`. Untracked `new.txt`: 2 lines. + expect(result!.stats.linesAdded).toBe(3); expect(result!.perFileStats.get('tracked.txt')?.added).toBe(1); - expect(result!.perFileStats.get('new.txt')?.isUntracked).toBe(true); + expect(result!.perFileStats.get('new.txt')).toEqual({ + added: 2, + removed: 0, + isBinary: false, + isUntracked: true, + }); + }); + + it('flags untracked binary files without counting lines', async () => { + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + // A NUL byte in the first few bytes is git's own binary heuristic. + await fs.writeFile( + path.join(repo, 'blob.bin'), + Buffer.from([0x89, 0x00, 0xff, 0x10]), + ); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.perFileStats.get('blob.bin')).toEqual({ + added: 0, + removed: 0, + isBinary: true, + isUntracked: true, + }); + // Binary bytes must not contaminate the linesAdded total. + expect(result!.stats.linesAdded).toBe(0); }); it('returns zero stats on a clean working tree', async () => { @@ -455,6 +500,85 @@ describe('fetchGitDiff transient-state detection', () => { }); }); +describe('fetchGitDiff tracked-file filename robustness', () => { + it('keeps the real filename for tracked files that contain a tab', async () => { + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-tab-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + const weirdName = 'tab\there.txt'; + try { + await fs.writeFile(path.join(repo, weirdName), 'x\n'); + } catch { + return; // Filesystem refused the name; nothing to assert. + } + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: repo }); + await fs.writeFile(path.join(repo, weirdName), 'y\n'); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + // With plain --numstat, git would C-quote this as `"tab\\there.txt"` + // and the map key would not match the real path. `--numstat -z` gives + // us the raw bytes back. + expect(result!.perFileStats.has(weirdName)).toBe(true); + expect(result!.perFileStats.has(`"tab\\there.txt"`)).toBe(false); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); + + it('combines a rename into a single "old => new" per-file entry', async () => { + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-mv-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(path.join(repo, 'old.txt'), 'x\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: repo }); + await execFileAsync('git', ['mv', 'old.txt', 'new.txt'], { cwd: repo }); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + // Rename detection is the git default with -M; we preserve that display + // shape rather than splitting into delete + add rows. + const keys = [...result!.perFileStats.keys()]; + expect(keys.some((k) => k.includes('old.txt => new.txt'))).toBe(true); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); +}); + +describe('parseShortstat ReDoS guard', () => { + it('runs in bounded time on pathological input', () => { + // CodeQL #137 flagged the previous regex as polynomial on many `0`s. + // After hardening (anchors + bounded digit runs), even 1e5 `0`s parse fast. + const adversarial = `${'0'.repeat(100_000)} files changed, ${'0'.repeat( + 100_000, + )} insertions(+)`; + const start = Date.now(); + const result = parseShortstat(adversarial); + const elapsed = Date.now() - start; + // Expect the bounded regex to either reject (too long for \d{1,10}) or + // match trivially. Either way it must not spin. + expect(elapsed).toBeLessThan(250); + expect(result).toBeNull(); + }); +}); + describe('fetchGitDiff non-ASCII filenames', () => { it('does not octal-escape UTF-8 filenames via core.quotepath', async () => { const repo = await makeRepo(); diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 1fea8d597d7..56592784faa 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -5,7 +5,7 @@ */ import { execFile } from 'node:child_process'; -import { access, readFile, stat } from 'node:fs/promises'; +import { access, open, readFile, stat } from 'node:fs/promises'; import * as path from 'node:path'; import { promisify } from 'node:util'; import type { Hunk } from 'diff'; @@ -43,6 +43,10 @@ export const MAX_DIFF_SIZE_BYTES = 1_000_000; export const MAX_LINES_PER_FILE = 400; /** Skip per-file parsing when the diff touches more than this many files. */ export const MAX_FILES_FOR_DETAILS = 500; +/** How much of an untracked file to read when counting its lines. */ +const UNTRACKED_READ_CAP_BYTES = MAX_DIFF_SIZE_BYTES; +/** Scan the first N bytes for NUL to detect binary files (matches git's heuristic). */ +const BINARY_SNIFF_BYTES = 8 * 1024; /** * Fetch numstat-based git diff stats (files changed, lines added/removed) and @@ -58,22 +62,40 @@ export async function fetchGitDiff(cwd: string): Promise { if (!isGitRepository(cwd)) return null; if (await isInTransientGitState(cwd)) return null; - // Quick probe + untracked list run in parallel — both are needed regardless - // of which path we take, and shortstat is O(1) memory so it can short-circuit - // huge generated workspaces before we pay the per-file numstat cost. - const [shortstatOut, untrackedPathsRaw] = await Promise.all([ + // Shortstat probe + untracked scan run in parallel — both are needed + // regardless of which path we take, and shortstat is O(1) memory so it can + // short-circuit huge generated workspaces before we pay the per-file + // numstat cost. For untracked we hold the raw stdout rather than the parsed + // list so the fast path only has to count NUL bytes instead of allocating + // a full path array. + const [shortstatOut, untrackedOut] = await Promise.all([ runGit(['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], cwd), - fetchUntrackedPaths(cwd), + runGit( + [ + '--no-optional-locks', + 'ls-files', + '-z', + '--others', + '--exclude-standard', + ], + cwd, + ), ]); - const untrackedPaths = untrackedPathsRaw ?? []; + const untrackedCount = countNulDelimited(untrackedOut); if (shortstatOut != null) { const quickStats = parseShortstat(shortstatOut); - if (quickStats && quickStats.filesCount > MAX_FILES_FOR_DETAILS) { + // Base the short-circuit on tracked + untracked so repos with relatively + // few edits but a flood of untracked files (e.g. large generated + // workspaces) still take the summary-only path. + if ( + quickStats && + quickStats.filesCount + untrackedCount > MAX_FILES_FOR_DETAILS + ) { return { stats: { ...quickStats, - filesCount: quickStats.filesCount + untrackedPaths.length, + filesCount: quickStats.filesCount + untrackedCount, }, perFileStats: new Map(), }; @@ -81,29 +103,43 @@ export async function fetchGitDiff(cwd: string): Promise { } const numstatOut = await runGit( - ['--no-optional-locks', 'diff', 'HEAD', '--numstat'], + ['--no-optional-locks', 'diff', 'HEAD', '--numstat', '-z'], cwd, ); if (numstatOut == null) return null; const { stats, perFileStats } = parseGitNumstat(numstatOut); - if (untrackedPaths.length > 0) { + if (untrackedCount > 0) { // Count every untracked file in the totals, even if the per-file map is // already full. Otherwise `filesCount` under-reports whenever tracked // changes already fill the `MAX_FILES` slot. - stats.filesCount += untrackedPaths.length; - const remainingSlots = MAX_FILES - perFileStats.size; - for (const filePath of untrackedPaths.slice( - 0, - Math.max(0, remainingSlots), - )) { - perFileStats.set(filePath, { + stats.filesCount += untrackedCount; + const untrackedPaths = splitNulDelimited(untrackedOut); + const remainingSlots = Math.max(0, MAX_FILES - perFileStats.size); + const visiblePaths = untrackedPaths.slice(0, remainingSlots); + // Count lines in each newly-created file so the header's `+N` reflects + // the true additions a user would see if they `git add`'d. Reads are + // capped per-file and binary content is detected so huge log files or + // blobs don't stall /diff. + const untrackedStats = await Promise.all( + visiblePaths.map((relPath) => + countUntrackedLines(path.join(cwd, relPath)), + ), + ); + for (let i = 0; i < visiblePaths.length; i++) { + const relPath = visiblePaths[i] ?? ''; + const u = untrackedStats[i] ?? { added: 0, - removed: 0, isBinary: false, + }; + perFileStats.set(relPath, { + added: u.added, + removed: 0, + isBinary: u.isBinary, isUntracked: true, }); + stats.linesAdded += u.added; } } @@ -126,33 +162,83 @@ export async function fetchGitDiffHunks( } /** - * Parse `git diff --numstat` output. - * Format per line: `\t\t`. Binary files use `-` for - * counts. Only the first `MAX_FILES` entries are kept in `perFileStats`, but - * total stats account for every line. + * Parse `git diff --numstat -z` output. + * + * Wire format (stable per `git-diff(1)`): + * - Non-rename: `\t\t\0` + * - Rename: `\t\t\0\0\0` + * + * Using `-z` (vs the default newline-delimited form) keeps paths byte-accurate: + * tabs, newlines, and non-ASCII characters all round-trip without git's + * C-style quoting, so `perFileStats` keys match the real on-disk filenames. + * + * Binary files use `-` for both counts. Only the first `MAX_FILES` entries are + * retained in `perFileStats`; totals account for every entry. */ export function parseGitNumstat(stdout: string): GitDiffResult { - const lines = stdout.split('\n').filter(Boolean); + // Drop the trailing empty chunk from the terminating NUL. + const tokens = stdout.split('\0'); + if (tokens.length > 0 && tokens[tokens.length - 1] === '') tokens.pop(); + let added = 0; let removed = 0; let validFileCount = 0; const perFileStats = new Map(); - for (const line of lines) { - const parts = line.split('\t'); - if (parts.length < 3) continue; + // Rename entries span three tokens ({counts}, oldPath, newPath). When we + // see an empty path in the counts token we stash the counts here and + // consume the next two tokens as the rename pair. + let pending: { added: number; removed: number; isBinary: boolean } | null = + null; + let renameOld: string | null = null; + + for (const token of tokens) { + if (pending) { + if (renameOld === null) { + renameOld = token; + continue; + } + commitEntry( + `${renameOld} => ${token}`, + pending.added, + pending.removed, + pending.isBinary, + ); + pending = null; + renameOld = null; + continue; + } - validFileCount++; - const addStr = parts[0]; - const remStr = parts[1]; - const filePath = parts.slice(2).join('\t'); + // Index-based parse — `split('\t')` is unsafe because `-z` preserves + // literal tabs inside filenames. + const firstTab = token.indexOf('\t'); + if (firstTab < 0) continue; + const secondTab = token.indexOf('\t', firstTab + 1); + if (secondTab < 0) continue; + const addStr = token.slice(0, firstTab); + const remStr = token.slice(firstTab + 1, secondTab); + const filePath = token.slice(secondTab + 1); const isBinary = addStr === '-' || remStr === '-'; - const fileAdded = isBinary ? 0 : parseInt(addStr ?? '0', 10) || 0; - const fileRemoved = isBinary ? 0 : parseInt(remStr ?? '0', 10) || 0; + const fileAdded = isBinary ? 0 : parseInt(addStr, 10) || 0; + const fileRemoved = isBinary ? 0 : parseInt(remStr, 10) || 0; + + if (filePath === '') { + // Rename header — wait for oldPath and newPath tokens. + pending = { added: fileAdded, removed: fileRemoved, isBinary }; + continue; + } + commitEntry(filePath, fileAdded, fileRemoved, isBinary); + } + function commitEntry( + filePath: string, + fileAdded: number, + fileRemoved: number, + isBinary: boolean, + ): void { + validFileCount++; added += fileAdded; removed += fileRemoved; - if (perFileStats.size < MAX_FILES) { perFileStats.set(filePath, { added: fileAdded, @@ -249,10 +335,15 @@ export function parseGitDiff(stdout: string): Map { /** * Parse `git diff --shortstat` output, e.g. * ` 3 files changed, 42 insertions(+), 7 deletions(-)`. + * + * The regex is anchored (line start/end with the `m` flag) and uses single + * literal spaces plus bounded `\d{1,10}` digit runs. This closes CodeQL alert + * #137: the previous unanchored form with `\s+` and `\d+` in nested optional + * groups could backtrack polynomially on crafted strings of `0`s. */ export function parseShortstat(stdout: string): GitDiffStats | null { const match = stdout.match( - /(\d+)\s+files?\s+changed(?:,\s+(\d+)\s+insertions?\(\+\))?(?:,\s+(\d+)\s+deletions?\(-\))?/, + /^ ?(\d{1,10}) files? changed(?:, (\d{1,10}) insertions?\(\+\))?(?:, (\d{1,10}) deletions?\(-\))?$/m, ); if (!match) return null; return { @@ -262,6 +353,62 @@ export function parseShortstat(stdout: string): GitDiffStats | null { }; } +function countNulDelimited(stdout: string | null): number { + if (!stdout) return 0; + let count = 0; + for (let i = 0; i < stdout.length; i++) { + if (stdout.charCodeAt(i) === 0) count++; + } + return count; +} + +function splitNulDelimited(stdout: string | null): string[] { + if (!stdout) return []; + return stdout.split('\0').filter(Boolean); +} + +interface UntrackedLineStats { + added: number; + isBinary: boolean; +} + +/** + * Count lines in an untracked file so the /diff totals include it. Reads up + * to `UNTRACKED_READ_CAP_BYTES`, bails on NUL in the first `BINARY_SNIFF_BYTES` + * (git's own heuristic), and swallows read errors into `{added:0,isBinary:false}` + * so one unreadable file can't block the whole command. + */ +async function countUntrackedLines( + absPath: string, +): Promise { + let fh; + try { + fh = await open(absPath, 'r'); + } catch { + return { added: 0, isBinary: false }; + } + try { + const buf = Buffer.alloc(UNTRACKED_READ_CAP_BYTES); + const { bytesRead } = await fh.read(buf, 0, UNTRACKED_READ_CAP_BYTES, 0); + if (bytesRead === 0) return { added: 0, isBinary: false }; + const sniffEnd = Math.min(BINARY_SNIFF_BYTES, bytesRead); + for (let i = 0; i < sniffEnd; i++) { + if (buf[i] === 0) return { added: 0, isBinary: true }; + } + let lines = 0; + for (let i = 0; i < bytesRead; i++) { + if (buf[i] === 0x0a) lines++; + } + // If the file does not end in a newline, count the trailing partial line. + if (buf[bytesRead - 1] !== 0x0a) lines++; + return { added: lines, isBinary: false }; + } catch { + return { added: 0, isBinary: false }; + } finally { + await fh.close().catch(() => {}); + } +} + /** * Resolve the real git directory for a working tree, following `.git` file * indirection used by linked worktrees (`git worktree add`) and submodules. @@ -310,18 +457,6 @@ async function isInTransientGitState(cwd: string): Promise { return results.some(Boolean); } -async function fetchUntrackedPaths(cwd: string): Promise { - // `-z` emits NUL-delimited paths with no C-style quoting, so filenames - // containing newlines, tabs, or non-ASCII bytes round-trip cleanly. - const stdout = await runGit( - ['--no-optional-locks', 'ls-files', '-z', '--others', '--exclude-standard'], - cwd, - ); - if (!stdout) return null; - const paths = stdout.split('\0').filter(Boolean); - return paths.length > 0 ? paths : null; -} - async function runGit(args: string[], cwd: string): Promise { // `core.quotepath=false` keeps non-ASCII filenames as UTF-8 in git's output // instead of octal-escaping them (`\346\226\207.txt`), which would otherwise diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index abf3e862d0b..2e7729bf49e 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-24T07:26:06.808Z", + "generatedAt": "2026-04-24T08:13:04.067Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)", From 5075ad5157949dbc5ea06be5a397caa8396dabc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Fri, 24 Apr 2026 17:04:57 +0800 Subject: [PATCH 07/23] fix(core): surface truncated untracked counts and decouple totals from display MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues surfaced during a directionless multi-round audit of the /diff feature: 1. `countUntrackedLines` reads at most `UNTRACKED_READ_CAP_BYTES` (1 MB) per file, so a 10 MB new log was silently reported as `+~20k` when the real count is ~10×. The helper now `fstat`s the file and returns a `truncated: true` flag when size exceeds the read window; `/diff` surfaces it as `(new, partial)` so the `+N` isn't read as exact. 2. Line-count aggregation was coupled to the per-file display cap: when tracked changes filled the `MAX_FILES` slot, untracked line counts beyond the remaining slots were dropped from `stats.linesAdded` entirely (header under-reported additions). Decoupled: we now read up to `MAX_FILES` untracked files for their line counts regardless of display slots, and only restrict the visible rows to `remainingSlots`. Added regression tests for both: a 1.5 MB new file asserts `truncated: true` and a lower-bound line count, and a `MAX_FILES`-saturated tracked set + 5 untracked files asserts that untracked additions still appear in the header totals even though none of them get displayed. --- packages/cli/src/i18n/locales/en.js | 1 + packages/cli/src/i18n/locales/zh.js | 1 + .../cli/src/ui/commands/diffCommand.test.ts | 24 +++++++ packages/cli/src/ui/commands/diffCommand.ts | 7 +- packages/core/src/utils/gitDiff.test.ts | 59 +++++++++++++++ packages/core/src/utils/gitDiff.ts | 72 +++++++++++++------ scripts/unused-keys-only-in-locales.json | 2 +- 7 files changed, 141 insertions(+), 25 deletions(-) diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index 3b6b7f02066..bea6dcda3c3 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -171,6 +171,7 @@ export default { '(binary)': '(binary)', '(binary, new)': '(binary, new)', '(new)': '(new)', + '(new, partial)': '(new, partial)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index 63c26b41226..86fa75a2186 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -166,6 +166,7 @@ export default { '(binary)': '(二进制)', '(binary, new)': '(二进制,新增)', '(new)': '(新增)', + '(new, partial)': '(新增,部分统计)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index 71c36446d7c..45f27ceaf6d 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -155,6 +155,30 @@ describe('diffCommand', () => { expect(aLine.indexOf('src/a.ts')).toBe(newLine.indexOf('notes.md')); }); + it('marks truncated untracked text files with (new, partial)', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1, linesAdded: 10000, linesRemoved: 0 }, + perFileStats: new Map([ + [ + 'big.log', + { + added: 10000, + removed: 0, + isBinary: false, + isUntracked: true, + truncated: true, + }, + ], + ]), + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + const row = content.split('\n').find((l) => l.includes('big.log'))!; + expect(row).toContain('(new, partial)'); + expect(row).not.toContain(' (new)'); + }); + it('marks binary untracked files with (binary, new) and no line count', async () => { if (!diffCommand.action) throw new Error('Command has no action'); mockFetchGitDiff.mockResolvedValue({ diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index b87be89be0c..14c2b86716b 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -121,7 +121,12 @@ function formatPerFile(perFileStats: Map): string[] { } else { const added = `+${String(s.added).padStart(addWidth)}`; const removed = `-${String(s.removed).padStart(remWidth)}`; - const suffix = s.isUntracked ? ` ${t('(new)')}` : ''; + let suffix = ''; + if (s.isUntracked) { + // `truncated` means we only counted part of a large new file — surface + // that so `+N` isn't read as the exact line count. + suffix = s.truncated ? ` ${t('(new, partial)')}` : ` ${t('(new)')}`; + } rows.push(` ${added} ${removed} ${filename}${suffix}`); } } diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index a118d85873e..c92cb346bd4 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -243,9 +243,34 @@ describe('fetchGitDiff', () => { removed: 0, isBinary: false, isUntracked: true, + truncated: false, }); }); + it('marks oversized untracked text files as truncated', async () => { + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + // Write a 1.5 MB text file — larger than UNTRACKED_READ_CAP_BYTES (1 MB), + // so the counter can only see part of the lines. The flag lets the UI + // mark `+N` as a lower bound instead of silently under-reporting. + const line = 'a'.repeat(99) + '\n'; // 100 bytes per line + const totalLines = 15_000; // 1.5 MB + await fs.writeFile(path.join(repo, 'big.log'), line.repeat(totalLines)); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + const entry = result!.perFileStats.get('big.log'); + expect(entry?.isUntracked).toBe(true); + expect(entry?.isBinary).toBe(false); + expect(entry?.truncated).toBe(true); + // We counted at most UNTRACKED_READ_CAP_BYTES / 100 = 10_000 lines, less + // than the file's real line count. + expect(entry?.added).toBeGreaterThan(0); + expect(entry!.added).toBeLessThan(totalLines); + }); + it('flags untracked binary files without counting lines', async () => { await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); await git(repo, 'add', '.'); @@ -264,6 +289,7 @@ describe('fetchGitDiff', () => { removed: 0, isBinary: true, isUntracked: true, + truncated: false, }); // Binary bytes must not contaminate the linesAdded total. expect(result!.stats.linesAdded).toBe(0); @@ -653,6 +679,39 @@ describe('fetchGitDiff untracked counting', () => { await fs.rm(repo, { recursive: true, force: true }); }); + it('aggregates untracked line counts into linesAdded even when the per-file map is full of tracked entries', async () => { + // Seed MAX_FILES tracked files, then modify them so the per-file map + // saturates with tracked entries. Add a handful of untracked files that + // would otherwise be cut out of the display slots — their line counts + // still need to land in `stats.linesAdded`. + for (let i = 0; i < MAX_FILES; i++) { + await fs.writeFile(path.join(repo, `t${i}.txt`), `hello${i}\n`); + } + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'seed'); + for (let i = 0; i < MAX_FILES; i++) { + await fs.writeFile(path.join(repo, `t${i}.txt`), `HELLO${i}\n`); + } + // Each untracked file has 3 lines; 5 files × 3 = 15 lines we must keep. + const untrackedCount = 5; + const linesPerFile = 3; + for (let i = 0; i < untrackedCount; i++) { + await fs.writeFile(path.join(repo, `u${i}.txt`), 'a\nb\nc\n'); + } + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.stats.filesCount).toBe(MAX_FILES + untrackedCount); + // Per-file map is still capped — none of the u* entries will be visible + // because the t* entries filled every slot. But the totals must still + // include the untracked additions. + expect(result!.perFileStats.size).toBe(MAX_FILES); + const trackedLinesAdded = MAX_FILES; // each t* gained 1 char → numstat 1/1 + expect(result!.stats.linesAdded).toBe( + trackedLinesAdded + untrackedCount * linesPerFile, + ); + }); + it('counts untracked files in filesCount even after the per-file map is full', async () => { // Create MAX_FILES tracked modifications to fill the per-file map. for (let i = 0; i < MAX_FILES; i++) { diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 56592784faa..0dce90c4d1f 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -27,6 +27,9 @@ export interface PerFileStats { removed: number; isBinary: boolean; isUntracked?: boolean; + /** Only meaningful for untracked files: `true` when the file exceeded the + * line-counting read cap and `added` is therefore a lower bound. */ + truncated?: boolean; } export interface GitDiffResult { @@ -116,30 +119,34 @@ export async function fetchGitDiff(cwd: string): Promise { // changes already fill the `MAX_FILES` slot. stats.filesCount += untrackedCount; const untrackedPaths = splitNulDelimited(untrackedOut); - const remainingSlots = Math.max(0, MAX_FILES - perFileStats.size); - const visiblePaths = untrackedPaths.slice(0, remainingSlots); - // Count lines in each newly-created file so the header's `+N` reflects - // the true additions a user would see if they `git add`'d. Reads are - // capped per-file and binary content is detected so huge log files or - // blobs don't stall /diff. - const untrackedStats = await Promise.all( - visiblePaths.map((relPath) => - countUntrackedLines(path.join(cwd, relPath)), - ), + // Keep line-counting decoupled from per-file rendering. We read up to + // `MAX_FILES` untracked files for their line counts (bounds worst-case + // I/O at ~50 MB) and fold all of them into `linesAdded`, even if only + // `remainingSlots` of them end up as visible rows. That avoids the + // header silently under-reporting additions when tracked changes have + // already filled the per-file map. + const countable = untrackedPaths.slice(0, MAX_FILES); + const countableStats = await Promise.all( + countable.map((relPath) => countUntrackedLines(path.join(cwd, relPath))), ); - for (let i = 0; i < visiblePaths.length; i++) { - const relPath = visiblePaths[i] ?? ''; - const u = untrackedStats[i] ?? { + for (const s of countableStats) stats.linesAdded += s.added; + + const remainingSlots = Math.max(0, MAX_FILES - perFileStats.size); + const visibleCount = Math.min(remainingSlots, countable.length); + for (let i = 0; i < visibleCount; i++) { + const relPath = countable[i] ?? ''; + const u = countableStats[i] ?? { added: 0, isBinary: false, + truncated: false, }; perFileStats.set(relPath, { added: u.added, removed: 0, isBinary: u.isBinary, isUntracked: true, + truncated: u.truncated, }); - stats.linesAdded += u.added; } } @@ -370,13 +377,18 @@ function splitNulDelimited(stdout: string | null): string[] { interface UntrackedLineStats { added: number; isBinary: boolean; + /** `true` when the file was larger than the read cap so `added` is a lower + * bound (the caller is expected to surface this so the user knows). */ + truncated: boolean; } /** * Count lines in an untracked file so the /diff totals include it. Reads up * to `UNTRACKED_READ_CAP_BYTES`, bails on NUL in the first `BINARY_SNIFF_BYTES` - * (git's own heuristic), and swallows read errors into `{added:0,isBinary:false}` - * so one unreadable file can't block the whole command. + * (git's own heuristic), and swallows read errors into a zero-result so one + * unreadable file can't block the whole command. `truncated` is set when + * `fstat(size) > bytesRead`, so the UI can mark partial counts honestly + * instead of silently under-reporting a 10 MB log as `+20k`. */ async function countUntrackedLines( absPath: string, @@ -385,25 +397,39 @@ async function countUntrackedLines( try { fh = await open(absPath, 'r'); } catch { - return { added: 0, isBinary: false }; + return { added: 0, isBinary: false, truncated: false }; } try { const buf = Buffer.alloc(UNTRACKED_READ_CAP_BYTES); const { bytesRead } = await fh.read(buf, 0, UNTRACKED_READ_CAP_BYTES, 0); - if (bytesRead === 0) return { added: 0, isBinary: false }; + if (bytesRead === 0) { + return { added: 0, isBinary: false, truncated: false }; + } const sniffEnd = Math.min(BINARY_SNIFF_BYTES, bytesRead); for (let i = 0; i < sniffEnd; i++) { - if (buf[i] === 0) return { added: 0, isBinary: true }; + if (buf[i] === 0) { + return { added: 0, isBinary: true, truncated: false }; + } } + // Compare against real file size rather than "bytesRead === cap" — + // a file that happens to be exactly UNTRACKED_READ_CAP_BYTES isn't + // truncated, and a short-read against a larger file still is. + const { size } = await fh.stat(); + const truncated = size > bytesRead; let lines = 0; for (let i = 0; i < bytesRead; i++) { if (buf[i] === 0x0a) lines++; } - // If the file does not end in a newline, count the trailing partial line. - if (buf[bytesRead - 1] !== 0x0a) lines++; - return { added: lines, isBinary: false }; + // If the portion we read does not end on a newline, count the trailing + // partial line — but only when the read reached EOF. When the read was + // cut short by the cap, the "trailing partial" is actually a complete + // line that continues past the buffer and will be counted when the + // following `\n` is eventually seen by a future larger cap; counting it + // here would double-count once the cap is raised. + if (!truncated && buf[bytesRead - 1] !== 0x0a) lines++; + return { added: lines, isBinary: false, truncated }; } catch { - return { added: 0, isBinary: false }; + return { added: 0, isBinary: false, truncated: false }; } finally { await fh.close().catch(() => {}); } diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index 2e7729bf49e..48a3108823c 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-24T08:13:04.067Z", + "generatedAt": "2026-04-24T09:04:13.440Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)", From 54a0b2de1692e663a796358e7e049d365d7e905b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Fri, 24 Apr 2026 17:21:03 +0800 Subject: [PATCH 08/23] fix(core): parse filenames from +++/--- lines to handle paths with ' b/' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `diff --git a/X b/Y` is ambiguous when X contains ` b/` — a file literally named `a b/c.txt` produces `diff --git a/a b/c.txt b/a b/c.txt` with no escape or quoting, and the previous regex `^a\/(.+?) b\/(.+)$` keyed the hunks under the wrong path. Consumers of the exported `fetchGitDiffHunks` API would then fail to correlate hunks with stats or editor paths. Introduces `extractFilePath(lines)` which walks the block for the unambiguous markers (`rename to` / `copy to` / `+++ b/` with a `/dev/null` fallback to `--- a/`) and strips the trailing TAB git appends to paths containing whitespace. Adds unit tests for the `a b/c.txt`, rename, delete, and new-file cases plus an end-to-end test that creates a real `a b/c.txt` file and asserts `fetchGitDiffHunks` keys the hunks correctly. Addresses wenshao review comment #3136657141 on #3491. --- packages/core/src/utils/gitDiff.test.ts | 71 +++++++++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 63 ++++++++++++++++++++-- 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index c92cb346bd4..b87d7a6fadd 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -375,6 +375,19 @@ describe('fetchGitDiffHunks', () => { ); }); + it('keys hunks by the real path for files whose name contains " b/"', async () => { + await fs.mkdir(path.join(repo, 'a b'), { recursive: true }); + await fs.writeFile(path.join(repo, 'a b', 'c.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await fs.writeFile(path.join(repo, 'a b', 'c.txt'), 'y\n'); + + const hunks = await fetchGitDiffHunks(repo); + // `diff --git a/a b/c.txt b/a b/c.txt` is ambiguous to split; the parser + // must anchor on `+++ b/\t` instead. + expect([...hunks.keys()]).toEqual(['a b/c.txt']); + }); + it('handles multi-hunk diffs', async () => { const initial = Array.from({ length: 40 }, (_, i) => `line${i}`).join('\n'); await fs.writeFile(path.join(repo, 'big.txt'), initial + '\n'); @@ -393,6 +406,64 @@ describe('fetchGitDiffHunks', () => { }); }); +describe('parseGitDiff path disambiguation', () => { + it('keys hunks by the real path when the filename contains " b/"', () => { + // `a b/c.txt` produces `diff --git a/a b/c.txt b/a b/c.txt`, which is + // ambiguous to split on ` b/`. Git appends a TAB on the `---`/`+++` lines + // when the path contains whitespace — that's the unambiguous anchor. + const diff = `diff --git a/a b/c.txt b/a b/c.txt +index 111..222 100644 +--- a/a b/c.txt\t ++++ b/a b/c.txt\t +@@ -1 +1 @@ +-x ++y +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual(['a b/c.txt']); + expect(result.get('a b/c.txt')![0].lines).toEqual(['-x', '+y']); + }); + + it('uses `rename to` for renames, ignoring the ambiguous header', () => { + const diff = `diff --git a/old name.txt b/renamed name.txt +similarity index 100% +rename from old name.txt +rename to renamed name.txt +`; + // No hunks — nothing to key — but the extractor should still not confuse + // paths. The file block is dropped because there are no `@@` lines, which + // is the existing behavior for mode-only / rename-only changes. + const result = parseGitDiff(diff); + expect(result.size).toBe(0); + }); + + it('falls back to `--- a/` when the file was deleted', () => { + const diff = `diff --git a/gone.txt b/gone.txt +deleted file mode 100644 +index 111..000 +--- a/gone.txt ++++ /dev/null +@@ -1 +0,0 @@ +-bye +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual(['gone.txt']); + }); + + it('uses `+++ b/` for newly-created files', () => { + const diff = `diff --git a/new.txt b/new.txt +new file mode 100644 +index 000..111 +--- /dev/null ++++ b/new.txt +@@ -0,0 +1 @@ ++hi +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual(['new.txt']); + }); +}); + describe('parseGitDiff edge cases', () => { it('drops file blocks that have no `@@` hunk header', () => { const noHunk = `diff --git a/foo.ts b/foo.ts diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 0dce90c4d1f..331d9a2223e 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -287,9 +287,14 @@ export function parseGitDiff(stdout: string): Map { if (Buffer.byteLength(fileDiff, 'utf8') > MAX_DIFF_SIZE_BYTES) continue; const lines = fileDiff.split('\n'); - const headerMatch = lines[0]?.match(/^a\/(.+?) b\/(.+)$/); - if (!headerMatch) continue; - const filePath = headerMatch[2] ?? headerMatch[1] ?? ''; + // The `diff --git a/X b/Y` header is ambiguous for paths that contain + // ` b/` (e.g. `a b/c.txt` yields `diff --git a/a b/c.txt b/a b/c.txt`). + // Prefer the unambiguous metadata that follows: `rename to`, `copy to`, + // or the `+++ b/` / `--- a/` lines. Git appends a trailing + // TAB to those paths when they contain whitespace — that's our real + // end-of-path marker. + const filePath = extractFilePath(lines); + if (filePath === null) continue; const fileHunks: Hunk[] = []; let currentHunk: Hunk | null = null; @@ -339,6 +344,58 @@ export function parseGitDiff(stdout: string): Map { return result; } +/** + * Extract the real filename from a `diff --git` file block, avoiding the + * ambiguity of `diff --git a/X b/Y` when `X` itself contains ` b/`. + * + * Preference order: + * 1. `rename to ` / `copy to ` — the authoritative new name. + * 2. `+++ b/` — the new-side path for in-place modifications. When + * the file was deleted the line reads `+++ /dev/null`; we then fall back + * to `--- a/` for the old name. + * 3. `--- a/` alone — for the rare case where `+++` is absent. + * + * Git appends a TAB after the path on `---` / `+++` / `rename to` lines when + * the path contains whitespace; `stripTab` cuts at that marker. + * + * Returns `null` when the block has no hunks or no recognizable path line + * (mode-only changes, for example). + */ +function extractFilePath(lines: string[]): string | null { + let plus: string | null = null; + let minus: string | null = null; + let renameTo: string | null = null; + let copyTo: string | null = null; + for (const line of lines) { + if (line.startsWith('@@ ')) break; + if (line.startsWith('+++ ')) plus = line.slice(4); + else if (line.startsWith('--- ')) minus = line.slice(4); + else if (line.startsWith('rename to ')) renameTo = line.slice(10); + else if (line.startsWith('copy to ')) copyTo = line.slice(8); + } + const stripTab = (s: string): string => { + const t = s.indexOf('\t'); + return t >= 0 ? s.slice(0, t) : s; + }; + if (renameTo !== null) return stripTab(renameTo); + if (copyTo !== null) return stripTab(copyTo); + if (plus !== null) { + const p = stripTab(plus); + if (p !== '/dev/null' && p.startsWith('b/')) return p.slice(2); + // Deleted file — fall back to the old path. + if (minus !== null) { + const m = stripTab(minus); + if (m !== '/dev/null' && m.startsWith('a/')) return m.slice(2); + } + return null; + } + if (minus !== null) { + const m = stripTab(minus); + if (m !== '/dev/null' && m.startsWith('a/')) return m.slice(2); + } + return null; +} + /** * Parse `git diff --shortstat` output, e.g. * ` 3 files changed, 42 insertions(+), 7 deletions(-)`. From e9a27210d78dd4d742ca47cbcf59e4152489062d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Fri, 24 Apr 2026 17:45:42 +0800 Subject: [PATCH 09/23] feat(cli): colorize /diff output via a themed Ink component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /diff stats used to come back as a plain-text MessageActionReturn. Pipes and ACP still get that, but in interactive terminals we now dispatch a structured history item so the numbers can carry theme colors. - packages/cli/src/ui/types.ts — new DiffRenderRow / DiffRenderModel / HistoryItemDiffStats, MessageType.DIFF_STATS. - packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx — renders +N in theme.status.success (green), -M in theme.status.error (red), and the (new) / (binary) / (new, partial) markers in theme.text.secondary (dim). Column alignment matches the plain-text fallback. - packages/cli/src/ui/components/HistoryItemDisplay.tsx — routes the new item type. - packages/cli/src/ui/commands/diffCommand.ts — builds a DiffRenderModel once and fans out: interactive calls context.ui.addItem; other modes fall through to renderDiffModelText() for the plain-text path. Error and "clean tree" branches keep the existing info/error MessageActionReturn in every mode. - Tests: existing diffCommand suite moved to an explicit non_interactive context (it was asserting text content); new interactive suite covers addItem dispatch and model shape; DiffStatsDisplay component tests cover the four row variants and the "…and N more" note. --- packages/cli/src/i18n/locales/en.js | 2 + packages/cli/src/i18n/locales/zh.js | 2 + .../cli/src/ui/commands/diffCommand.test.ts | 97 ++++++++++ packages/cli/src/ui/commands/diffCommand.ts | 165 ++++++++++++------ .../src/ui/components/HistoryItemDisplay.tsx | 4 + .../messages/DiffStatsDisplay.test.tsx | 148 ++++++++++++++++ .../components/messages/DiffStatsDisplay.tsx | 130 ++++++++++++++ packages/cli/src/ui/types.ts | 36 +++- scripts/unused-keys-only-in-locales.json | 2 +- 9 files changed, 534 insertions(+), 52 deletions(-) create mode 100644 packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx create mode 100644 packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index bea6dcda3c3..b38a117abb8 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -166,6 +166,8 @@ export default { '{{count}} file changed, +{{added}} / -{{removed}}', '{{count}} files changed, +{{added}} / -{{removed}}': '{{count}} files changed, +{{added}} / -{{removed}}', + '{{count}} file changed': '{{count}} file changed', + '{{count}} files changed': '{{count}} files changed', '…and {{hidden}} more (showing first {{shown}})': '…and {{hidden}} more (showing first {{shown}})', '(binary)': '(binary)', diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index 86fa75a2186..fb2f674e717 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -161,6 +161,8 @@ export default { '{{count}} 个文件变更,+{{added}} / -{{removed}}', '{{count}} files changed, +{{added}} / -{{removed}}': '{{count}} 个文件变更,+{{added}} / -{{removed}}', + '{{count}} file changed': '{{count}} 个文件变更', + '{{count}} files changed': '{{count}} 个文件变更', '…and {{hidden}} more (showing first {{shown}})': '…还有 {{hidden}} 个(仅显示前 {{shown}} 个)', '(binary)': '(二进制)', diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index 45f27ceaf6d..cdab8c8c0fd 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -22,7 +22,24 @@ vi.mock('@qwen-code/qwen-code-core', async () => { }); function makeContextWithCwd(cwd = '/tmp/repo'): CommandContext { + // Non-interactive by default here because these tests assert on the + // plain-text `MessageActionReturn`; interactive mode dispatches via + // `context.ui.addItem` and is covered in a separate describe block. return createMockCommandContext({ + executionMode: 'non_interactive', + services: { + config: { + getWorkingDir: () => cwd, + getProjectRoot: () => cwd, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any, + }, + }); +} + +function makeInteractiveContext(cwd = '/tmp/repo'): CommandContext { + return createMockCommandContext({ + executionMode: 'interactive', services: { config: { getWorkingDir: () => cwd, @@ -243,6 +260,86 @@ describe('diffCommand', () => { }); }); +describe('diffCommand interactive mode', () => { + let mockFetchGitDiff: Mock; + + beforeEach(() => { + vi.clearAllMocks(); + mockFetchGitDiff = vi.mocked(fetchGitDiff); + }); + + it('dispatches a diff_stats history item instead of returning text', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const ctx = makeInteractiveContext(); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 2, linesAdded: 7, linesRemoved: 3 }, + perFileStats: new Map([ + ['src/a.ts', { added: 5, removed: 2, isBinary: false }], + ['src/b.ts', { added: 2, removed: 1, isBinary: false }], + ]), + } satisfies GitDiffResult); + + const result = await diffCommand.action(ctx, ''); + expect(result).toBeUndefined(); + expect(ctx.ui.addItem).toHaveBeenCalledTimes(1); + const call = (ctx.ui.addItem as Mock).mock.calls[0][0]; + expect(call.type).toBe('diff_stats'); + expect(call.model).toMatchObject({ + filesCount: 2, + linesAdded: 7, + linesRemoved: 3, + hiddenCount: 0, + }); + expect(call.model.rows).toHaveLength(2); + expect(call.model.rows[0]).toMatchObject({ + filename: 'src/a.ts', + added: 5, + removed: 2, + isBinary: false, + isUntracked: false, + }); + }); + + it('still returns a plain-text info message for the "clean tree" case', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const ctx = makeInteractiveContext(); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 0, linesAdded: 0, linesRemoved: 0 }, + perFileStats: new Map(), + } satisfies GitDiffResult); + + const result = await diffCommand.action(ctx, ''); + expect(result).toMatchObject({ type: 'message', messageType: 'info' }); + expect(ctx.ui.addItem).not.toHaveBeenCalled(); + }); + + it('still returns an error MessageActionReturn when fetchGitDiff throws', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const ctx = makeInteractiveContext(); + mockFetchGitDiff.mockRejectedValueOnce(new Error('boom')); + + const result = await diffCommand.action(ctx, ''); + expect(result).toMatchObject({ type: 'message', messageType: 'error' }); + expect(ctx.ui.addItem).not.toHaveBeenCalled(); + }); + + it('propagates hiddenCount to the history item for fast-path results', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + const ctx = makeInteractiveContext(); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 60, linesAdded: 100, linesRemoved: 20 }, + perFileStats: new Map([ + ['src/a.ts', { added: 1, removed: 0, isBinary: false }], + ]), + } satisfies GitDiffResult); + + await diffCommand.action(ctx, ''); + const call = (ctx.ui.addItem as Mock).mock.calls[0][0]; + expect(call.model.hiddenCount).toBe(59); + expect(call.model.rows).toHaveLength(1); + }); +}); + describe('diffCommand registration', () => { it('declares all execution modes so it works in non-interactive and ACP', () => { expect(diffCommand.supportedModes).toEqual([ diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index 14c2b86716b..c7337369a51 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -4,7 +4,11 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { fetchGitDiff, type PerFileStats } from '@qwen-code/qwen-code-core'; +import { + fetchGitDiff, + type GitDiffResult, + type PerFileStats, +} from '@qwen-code/qwen-code-core'; import { CommandKind, type CommandContext, @@ -12,10 +16,16 @@ import { type SlashCommand, } from './types.js'; import { t } from '../../i18n/index.js'; +import { + MessageType, + type DiffRenderModel, + type DiffRenderRow, + type HistoryItemDiffStats, +} from '../types.js'; async function diffAction( context: CommandContext, -): Promise { +): Promise { const { config } = context.services; if (!config) { return { @@ -34,7 +44,7 @@ async function diffAction( }; } - let result: Awaited>; + let result: GitDiffResult | null; try { result = await fetchGitDiff(cwd); } catch (error) { @@ -57,8 +67,7 @@ async function diffAction( }; } - const { stats, perFileStats } = result; - if (stats.filesCount === 0) { + if (result.stats.filesCount === 0) { return { type: 'message', messageType: 'info', @@ -66,77 +75,133 @@ async function diffAction( }; } + const model = buildDiffRenderModel(result); + + // Interactive path: dispatch a structured history item so `DiffStatsDisplay` + // can render with theme colors. Non-interactive / ACP stay on the + // plain-text MessageActionReturn path so pipes, logs, and transports that + // don't speak Ink still see legible output. + if (context.executionMode === 'interactive') { + const item: Omit = { + type: MessageType.DIFF_STATS, + model, + }; + context.ui.addItem(item, Date.now()); + return; + } + + return { + type: 'message', + messageType: 'info', + content: renderDiffModelText(model), + }; +} + +/** + * Convert the raw `fetchGitDiff` result into a display-ready structure that + * both the Ink component and the plain-text renderer consume. Order of rows + * mirrors git's numstat output (which uses alphabetical or insertion order). + */ +export function buildDiffRenderModel(result: GitDiffResult): DiffRenderModel { + const rows: DiffRenderRow[] = []; + for (const [filename, s] of result.perFileStats) { + rows.push(toRow(filename, s)); + } + const hiddenCount = Math.max(0, result.stats.filesCount - rows.length); + return { + filesCount: result.stats.filesCount, + linesAdded: result.stats.linesAdded, + linesRemoved: result.stats.linesRemoved, + rows, + hiddenCount, + }; +} + +function toRow(filename: string, s: PerFileStats): DiffRenderRow { + if (s.isBinary) { + return { + filename, + isBinary: true, + isUntracked: Boolean(s.isUntracked), + truncated: false, + }; + } + return { + filename, + added: s.added, + removed: s.isUntracked ? 0 : s.removed, + isBinary: false, + isUntracked: Boolean(s.isUntracked), + truncated: Boolean(s.truncated), + }; +} + +/** + * Plain-text rendering of a `DiffRenderModel`. Used in non-interactive / ACP + * modes where no Ink renderer is available, and as the source of truth for + * the text column layout the Ink component mirrors. + */ +export function renderDiffModelText(model: DiffRenderModel): string { + const { filesCount, linesAdded, linesRemoved, rows, hiddenCount } = model; const header = - stats.filesCount === 1 + filesCount === 1 ? t('{{count}} file changed, +{{added}} / -{{removed}}', { - count: String(stats.filesCount), - added: String(stats.linesAdded), - removed: String(stats.linesRemoved), + count: String(filesCount), + added: String(linesAdded), + removed: String(linesRemoved), }) : t('{{count}} files changed, +{{added}} / -{{removed}}', { - count: String(stats.filesCount), - added: String(stats.linesAdded), - removed: String(stats.linesRemoved), + count: String(filesCount), + added: String(linesAdded), + removed: String(linesRemoved), }); - const rows = formatPerFile(perFileStats); - const hidden = stats.filesCount - perFileStats.size; + const lines = formatRowsText(rows); const capNote = - hidden > 0 && perFileStats.size > 0 + hiddenCount > 0 && rows.length > 0 ? `\n ${t('…and {{hidden}} more (showing first {{shown}})', { - hidden: String(hidden), - shown: String(perFileStats.size), + hidden: String(hiddenCount), + shown: String(rows.length), })}` : ''; - - return { - type: 'message', - messageType: 'info', - content: - rows.length > 0 ? `${header}\n${rows.join('\n')}${capNote}` : header, - }; + return lines.length > 0 ? `${header}\n${lines.join('\n')}${capNote}` : header; } -function formatPerFile(perFileStats: Map): string[] { - if (perFileStats.size === 0) return []; - +function formatRowsText(rows: DiffRenderRow[]): string[] { + if (rows.length === 0) return []; let maxAdded = 0; let maxRemoved = 0; - for (const s of perFileStats.values()) { - if (s.isBinary) continue; - if (s.added > maxAdded) maxAdded = s.added; - if (s.removed > maxRemoved) maxRemoved = s.removed; + for (const r of rows) { + if (r.isBinary) continue; + if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; + if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; } const addWidth = String(maxAdded).length; const remWidth = String(maxRemoved).length; - // Width of the `+X -Y` stat column so `~` (binary) rows line up with it. const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; - const rows: string[] = []; - for (const [filename, s] of perFileStats) { - if (s.isBinary) { - const suffix = s.isUntracked + const out: string[] = []; + for (const r of rows) { + if (r.isBinary) { + const suffix = r.isUntracked ? ` ${t('(binary, new)')}` : ` ${t('(binary)')}`; - rows.push(` ${padMarker('~', statColumnWidth)} ${filename}${suffix}`); - } else { - const added = `+${String(s.added).padStart(addWidth)}`; - const removed = `-${String(s.removed).padStart(remWidth)}`; - let suffix = ''; - if (s.isUntracked) { - // `truncated` means we only counted part of a large new file — surface - // that so `+N` isn't read as the exact line count. - suffix = s.truncated ? ` ${t('(new, partial)')}` : ` ${t('(new)')}`; - } - rows.push(` ${added} ${removed} ${filename}${suffix}`); + out.push(` ${padMarker('~', statColumnWidth)} ${r.filename}${suffix}`); + continue; + } + const added = `+${String(r.added ?? 0).padStart(addWidth)}`; + const removed = `-${String(r.removed ?? 0).padStart(remWidth)}`; + let suffix = ''; + if (r.isUntracked) { + suffix = r.truncated ? ` ${t('(new, partial)')}` : ` ${t('(new)')}`; } + out.push(` ${added} ${removed} ${r.filename}${suffix}`); } - return rows; + return out; } function padMarker(marker: string, width: number): string { if (marker.length >= width) return marker; - const pad = ' '.repeat(width - marker.length); - return `${marker}${pad}`; + return `${marker}${' '.repeat(width - marker.length)}`; } export const diffCommand: SlashCommand = { diff --git a/packages/cli/src/ui/components/HistoryItemDisplay.tsx b/packages/cli/src/ui/components/HistoryItemDisplay.tsx index a37544db035..0939038f9d7 100644 --- a/packages/cli/src/ui/components/HistoryItemDisplay.tsx +++ b/packages/cli/src/ui/components/HistoryItemDisplay.tsx @@ -51,6 +51,7 @@ import { ArenaAgentCard, ArenaSessionCard } from './arena/ArenaCards.js'; import { InsightProgressMessage } from './messages/InsightProgressMessage.js'; import { BtwMessage } from './messages/BtwMessage.js'; import { MemorySavedMessage } from './messages/MemorySavedMessage.js'; +import { DiffStatsDisplay } from './messages/DiffStatsDisplay.js'; import { useCompactMode } from '../contexts/CompactModeContext.js'; interface HistoryItemDisplayProps { @@ -174,6 +175,9 @@ const HistoryItemDisplayComponent: React.FC = ({ {itemForDisplay.type === 'stats' && ( )} + {itemForDisplay.type === 'diff_stats' && ( + + )} {itemForDisplay.type === 'model_stats' && ( )} diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx new file mode 100644 index 00000000000..abf0edc5fde --- /dev/null +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx @@ -0,0 +1,148 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from 'ink-testing-library'; +import { describe, it, expect } from 'vitest'; +import { DiffStatsDisplay } from './DiffStatsDisplay.js'; +import type { DiffRenderModel } from '../../types.js'; + +function stripAnsi(s: string): string { + // eslint-disable-next-line no-control-regex + return s.replace(/\x1b\[[0-9;]*m/g, ''); +} + +describe('DiffStatsDisplay', () => { + it('renders header and per-file rows aligned in columns', () => { + const model: DiffRenderModel = { + filesCount: 2, + linesAdded: 7, + linesRemoved: 3, + hiddenCount: 0, + rows: [ + { + filename: 'src/a.ts', + added: 5, + removed: 2, + isBinary: false, + isUntracked: false, + truncated: false, + }, + { + filename: 'src/b.ts', + added: 2, + removed: 1, + isBinary: false, + isUntracked: false, + truncated: false, + }, + ], + }; + const { lastFrame } = render(); + const visible = stripAnsi(lastFrame() ?? ''); + expect(visible).toContain('2 files changed'); + expect(visible).toContain('+7'); + expect(visible).toContain('-3'); + const aRow = visible.split('\n').find((l) => l.endsWith('src/a.ts'))!; + const bRow = visible.split('\n').find((l) => l.endsWith('src/b.ts'))!; + // Columns align — "src/a.ts" and "src/b.ts" start at the same offset. + expect(aRow.indexOf('src/a.ts')).toBe(bRow.indexOf('src/b.ts')); + }); + + it('renders the (new) marker for untracked text files', () => { + const model: DiffRenderModel = { + filesCount: 1, + linesAdded: 3, + linesRemoved: 0, + hiddenCount: 0, + rows: [ + { + filename: 'notes.md', + added: 3, + removed: 0, + isBinary: false, + isUntracked: true, + truncated: false, + }, + ], + }; + const { lastFrame } = render(); + const visible = stripAnsi(lastFrame() ?? ''); + expect(visible).toContain('notes.md'); + expect(visible).toContain('(new)'); + expect(visible).not.toContain('(new, partial)'); + }); + + it('renders the (new, partial) marker for truncated untracked text files', () => { + const model: DiffRenderModel = { + filesCount: 1, + linesAdded: 10000, + linesRemoved: 0, + hiddenCount: 0, + rows: [ + { + filename: 'big.log', + added: 10000, + removed: 0, + isBinary: false, + isUntracked: true, + truncated: true, + }, + ], + }; + const visible = stripAnsi( + render().lastFrame() ?? '', + ); + expect(visible).toContain('(new, partial)'); + }); + + it('renders binary rows with a ~ marker and no +N/-M', () => { + const model: DiffRenderModel = { + filesCount: 1, + linesAdded: 0, + linesRemoved: 0, + hiddenCount: 0, + rows: [ + { + filename: 'img.png', + isBinary: true, + isUntracked: false, + truncated: false, + }, + ], + }; + const visible = stripAnsi( + render().lastFrame() ?? '', + ); + const rowLine = visible.split('\n').find((l) => l.includes('img.png'))!; + expect(rowLine).toContain('~'); + expect(rowLine).toContain('(binary)'); + expect(rowLine).not.toMatch(/\+\d/); + }); + + it('renders the "…and N more" note when hiddenCount > 0', () => { + const model: DiffRenderModel = { + filesCount: 60, + linesAdded: 100, + linesRemoved: 20, + hiddenCount: 59, + rows: [ + { + filename: 'src/a.ts', + added: 1, + removed: 0, + isBinary: false, + isUntracked: false, + truncated: false, + }, + ], + }; + const visible = stripAnsi( + render().lastFrame() ?? '', + ); + expect(visible).toContain('60 files changed'); + expect(visible).toMatch(/59 more/); + }); +}); diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx new file mode 100644 index 00000000000..caba4d374cc --- /dev/null +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx @@ -0,0 +1,130 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type React from 'react'; +import { Box, Text } from 'ink'; +import { theme } from '../../semantic-colors.js'; +import type { DiffRenderModel, DiffRenderRow } from '../../types.js'; +import { t } from '../../../i18n/index.js'; + +interface DiffStatsDisplayProps { + model: DiffRenderModel; +} + +/** + * Colored rendering of `/diff` output for interactive mode. Mirrors the + * layout of the plain-text fallback (see `renderDiffModelText`) so the two + * modes stay visually aligned, but uses Ink primitives with `theme.status.*` + * tokens instead of baking ANSI into the text. + */ +export const DiffStatsDisplay: React.FC = ({ + model, +}) => { + const { filesCount, linesAdded, linesRemoved, rows, hiddenCount } = model; + + // Reproduce the numeric-column alignment of the text renderer so the + // interactive and non-interactive outputs are visually interchangeable. + let maxAdded = 0; + let maxRemoved = 0; + for (const r of rows) { + if (r.isBinary) continue; + if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; + if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; + } + const addWidth = String(maxAdded).length; + const remWidth = String(maxRemoved).length; + const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + + const headerLabel = + filesCount === 1 + ? t('{{count}} file changed', { count: String(filesCount) }) + : t('{{count}} files changed', { count: String(filesCount) }); + + return ( + + + {headerLabel} + , + +{linesAdded} + / + -{linesRemoved} + + {rows.map((row, i) => ( + + ))} + {hiddenCount > 0 && rows.length > 0 && ( + + + {' '} + {t('…and {{hidden}} more (showing first {{shown}})', { + hidden: String(hiddenCount), + shown: String(rows.length), + })} + + + )} + + ); +}; + +interface DiffRowProps { + row: DiffRenderRow; + addWidth: number; + remWidth: number; + statColumnWidth: number; +} + +const DiffRow: React.FC = ({ + row, + addWidth, + remWidth, + statColumnWidth, +}) => { + if (row.isBinary) { + const marker = padRight('~', statColumnWidth); + const suffix = row.isUntracked ? t('(binary, new)') : t('(binary)'); + return ( + + + {' '} + {marker} + {' '} + {row.filename} + {suffix} + + + ); + } + const added = String(row.added ?? 0).padStart(addWidth); + const removed = String(row.removed ?? 0).padStart(remWidth); + let suffix: string | null = null; + if (row.isUntracked) { + suffix = row.truncated ? t('(new, partial)') : t('(new)'); + } + return ( + + + {' '} + +{added} + + -{removed} + {' '} + {row.filename} + {suffix && {suffix}} + + + ); +}; + +function padRight(s: string, width: number): string { + return s.length >= width ? s : s + ' '.repeat(width - s.length); +} diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 775537da96d..8bf1fae5669 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -175,6 +175,38 @@ export type HistoryItemStats = HistoryItemBase & { duration: string; }; +/** + * Structured payload rendered by `/diff`. Kept as plain data (not React nodes) + * so the same model can feed both the Ink-based interactive display and the + * plain-text non-interactive / ACP output. + */ +export interface DiffRenderRow { + filename: string; + /** `undefined` for binary files; a line count (lower bound if `truncated`) + * otherwise. */ + added?: number; + /** `undefined` for binary and untracked files. */ + removed?: number; + isBinary: boolean; + isUntracked: boolean; + /** Only set for untracked text files that exceeded the read cap. */ + truncated: boolean; +} + +export interface DiffRenderModel { + filesCount: number; + linesAdded: number; + linesRemoved: number; + rows: DiffRenderRow[]; + /** `filesCount - rows.length` when the per-file cap truncated the listing. */ + hiddenCount: number; +} + +export type HistoryItemDiffStats = HistoryItemBase & { + type: 'diff_stats'; + model: DiffRenderModel; +}; + export type HistoryItemModelStats = HistoryItemBase & { type: 'model_stats'; }; @@ -492,7 +524,8 @@ export type HistoryItemWithoutId = | HistoryItemUserPromptSubmitBlocked | HistoryItemStopHookLoop | HistoryItemStopHookSystemMessage - | HistoryItemDoctor; + | HistoryItemDoctor + | HistoryItemDiffStats; export type HistoryItem = HistoryItemWithoutId & { id: number }; @@ -521,6 +554,7 @@ export enum MessageType { ARENA_SESSION_COMPLETE = 'arena_session_complete', INSIGHT_PROGRESS = 'insight_progress', BTW = 'btw', + DIFF_STATS = 'diff_stats', } export interface InsightProgressProps { diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index 48a3108823c..7fbdb5f9893 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-24T09:04:13.440Z", + "generatedAt": "2026-04-24T09:44:54.528Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)", From 239fa7141e2d3cee9fc66197813aa1933b59dacb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Mon, 27 Apr 2026 11:39:19 +0800 Subject: [PATCH 10/23] refactor(cli): factor /diff column widths into a shared helper Audit of the colorize commit found one real DRY hazard: DiffStatsDisplay and renderDiffModelText each independently re-derived addWidth / remWidth / statColumnWidth from the same row list. If anyone later changed one formula, the interactive Ink output and the non-interactive plain text would silently fall out of column alignment. Extract the computation into computeDiffColumnWidths() exported from diffCommand.ts; both renderers now call it. Adds a focused unit test of the contract (empty rows, widest non-binary row wins, binary rows are ignored, untracked text rows count). Drop a redundant `Omit` annotation since the type already has no id field. --- .../cli/src/ui/commands/diffCommand.test.ts | 79 ++++++++++++++++++- packages/cli/src/ui/commands/diffCommand.ts | 45 ++++++++--- .../components/messages/DiffStatsDisplay.tsx | 18 ++--- scripts/unused-keys-only-in-locales.json | 2 +- 4 files changed, 118 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index cdab8c8c0fd..ca267b0671b 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -6,7 +6,7 @@ import type { Mock } from 'vitest'; import { vi, describe, it, expect, beforeEach } from 'vitest'; -import { diffCommand } from './diffCommand.js'; +import { computeDiffColumnWidths, diffCommand } from './diffCommand.js'; import { type CommandContext } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import { fetchGitDiff, type GitDiffResult } from '@qwen-code/qwen-code-core'; @@ -340,6 +340,83 @@ describe('diffCommand interactive mode', () => { }); }); +describe('computeDiffColumnWidths', () => { + // Direct contract test — both the Ink component and the plain-text + // renderer call this helper, so its output binds their column alignment. + // If anyone changes the formula, both paths must shift together. + + it('reports min widths of 1 for an empty row list', () => { + expect(computeDiffColumnWidths([])).toEqual({ + addWidth: 1, + remWidth: 1, + statColumnWidth: 5, // `+_ -_` with single-digit padding + }); + }); + + it('sizes columns to the widest non-binary row', () => { + const widths = computeDiffColumnWidths([ + { + filename: 'a', + added: 9999, + removed: 5, + isBinary: false, + isUntracked: false, + truncated: false, + }, + { + filename: 'b', + added: 2, + removed: 100, + isBinary: false, + isUntracked: false, + truncated: false, + }, + ]); + // 1 (`+`) + 4 (digits) + 1 (` `) + 1 (`-`) + 3 (digits) = 10 + expect(widths).toEqual({ addWidth: 4, remWidth: 3, statColumnWidth: 10 }); + }); + + it('ignores binary rows when computing widths', () => { + // A binary row must not push the numeric column wider, otherwise the + // `~` placeholder ends up padded to a column that no real number ever + // occupies. + const widths = computeDiffColumnWidths([ + { + filename: 'a', + added: 1, + removed: 1, + isBinary: false, + isUntracked: false, + truncated: false, + }, + { + filename: 'b.bin', + isBinary: true, + isUntracked: false, + truncated: false, + }, + ]); + expect(widths).toEqual({ addWidth: 1, remWidth: 1, statColumnWidth: 5 }); + }); + + it('counts untracked text rows in width calculation', () => { + // Untracked rows render as `+N -0 filename (new)`; their `added` + // value must be allowed to widen the column. + const widths = computeDiffColumnWidths([ + { + filename: 'fresh.log', + added: 12345, + removed: 0, + isBinary: false, + isUntracked: true, + truncated: false, + }, + ]); + expect(widths.addWidth).toBe(5); + expect(widths.statColumnWidth).toBe(1 + 5 + 1 + 1 + 1); + }); +}); + describe('diffCommand registration', () => { it('declares all execution modes so it works in non-interactive and ACP', () => { expect(diffCommand.supportedModes).toEqual([ diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index c7337369a51..4d09dc1c86c 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -82,7 +82,7 @@ async function diffAction( // plain-text MessageActionReturn path so pipes, logs, and transports that // don't speak Ink still see legible output. if (context.executionMode === 'interactive') { - const item: Omit = { + const item: HistoryItemDiffStats = { type: MessageType.DIFF_STATS, model, }; @@ -136,6 +136,38 @@ function toRow(filename: string, s: PerFileStats): DiffRenderRow { }; } +/** + * Single source of truth for the per-row column layout. Used by both the + * Ink component and the plain-text renderer so the two paths can never + * silently disagree on alignment. + */ +export interface DiffColumnWidths { + /** Digits in the widest non-binary `added` value (min 1). */ + addWidth: number; + /** Digits in the widest non-binary `removed` value (min 1). */ + remWidth: number; + /** Visual width of the `+X -Y` stat column, used to pad the binary `~` + * marker so it lines up with the numeric rows. */ + statColumnWidth: number; +} + +export function computeDiffColumnWidths( + rows: readonly DiffRenderRow[], +): DiffColumnWidths { + let maxAdded = 0; + let maxRemoved = 0; + for (const r of rows) { + if (r.isBinary) continue; + if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; + if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; + } + const addWidth = String(maxAdded).length; + const remWidth = String(maxRemoved).length; + // `+` + addDigits + ' ' + `-` + remDigits. + const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + return { addWidth, remWidth, statColumnWidth }; +} + /** * Plain-text rendering of a `DiffRenderModel`. Used in non-interactive / ACP * modes where no Ink renderer is available, and as the source of truth for @@ -168,16 +200,7 @@ export function renderDiffModelText(model: DiffRenderModel): string { function formatRowsText(rows: DiffRenderRow[]): string[] { if (rows.length === 0) return []; - let maxAdded = 0; - let maxRemoved = 0; - for (const r of rows) { - if (r.isBinary) continue; - if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; - if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; - } - const addWidth = String(maxAdded).length; - const remWidth = String(maxRemoved).length; - const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + const { addWidth, remWidth, statColumnWidth } = computeDiffColumnWidths(rows); const out: string[] = []; for (const r of rows) { diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx index caba4d374cc..8fc342f862c 100644 --- a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx @@ -8,6 +8,7 @@ import type React from 'react'; import { Box, Text } from 'ink'; import { theme } from '../../semantic-colors.js'; import type { DiffRenderModel, DiffRenderRow } from '../../types.js'; +import { computeDiffColumnWidths } from '../../commands/diffCommand.js'; import { t } from '../../../i18n/index.js'; interface DiffStatsDisplayProps { @@ -24,19 +25,10 @@ export const DiffStatsDisplay: React.FC = ({ model, }) => { const { filesCount, linesAdded, linesRemoved, rows, hiddenCount } = model; - - // Reproduce the numeric-column alignment of the text renderer so the - // interactive and non-interactive outputs are visually interchangeable. - let maxAdded = 0; - let maxRemoved = 0; - for (const r of rows) { - if (r.isBinary) continue; - if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; - if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; - } - const addWidth = String(maxAdded).length; - const remWidth = String(maxRemoved).length; - const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + // Single source of truth shared with `renderDiffModelText`, so the + // interactive Ink output and the non-interactive plain text never drift + // out of column alignment. + const { addWidth, remWidth, statColumnWidth } = computeDiffColumnWidths(rows); const headerLabel = filesCount === 1 diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index 72615d1df06..9d599490e64 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-27T03:28:33.689Z", + "generatedAt": "2026-04-27T03:39:04.133Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)", From bb0164d99f01ff852b07c66754f27c9c2ea51fcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Mon, 27 Apr 2026 14:57:33 +0800 Subject: [PATCH 11/23] fix(core): pin /diff git ops to repo root and lstat untracked entries Two Critical findings on PR #3491: 1. (line 63) When /diff is invoked from a subdirectory of the worktree, `git diff` emits repo-root-relative paths but `git ls-files --others` is scoped to cwd and emits cwd-relative paths. Result: mixed path bases in `perFileStats` and silent omission of untracked files in sibling directories. Resolve `findGitRoot(cwd)` once and run every git invocation (and `path.join(...)` for line counting) from there, so all keys are repo-root-relative and the listing is repo-wide. 2. (line 455) `countUntrackedLines` opened every untracked path with `open(absPath, 'r')`. Git's `ls-files --others` can list FIFOs (whose `open()` blocks indefinitely waiting on a writer) and symlinks (which `open()` dereferences, potentially reading outside the worktree). Add an `lstat` gate: only regular files are counted; symlinks and other special files render as binary `~` rows. Two new integration tests cover both regressions: one creates a sibling untracked file at the repo root and invokes fetchGitDiff from a subdir asserting all three changes (root + sub) come back keyed by repo-root-relative paths; the other creates a symlink pointing at content outside the worktree and asserts it lands as a binary row with no contribution to linesAdded. --- .npmrc | 1 + README.md | 1 + packages/core/src/utils/gitDiff.test.ts | 97 +++++++++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 44 +++++++++-- 4 files changed, 137 insertions(+), 6 deletions(-) diff --git a/.npmrc b/.npmrc index 38f11c645a0..e28d5df67f9 100644 --- a/.npmrc +++ b/.npmrc @@ -1 +1,2 @@ registry=https://registry.npmjs.org +# test comment for diff verification diff --git a/README.md b/README.md index 5358f9d0e1f..7b30fdd4c27 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,4 @@ +
[![npm version](https://img.shields.io/npm/v/@qwen-code/qwen-code.svg)](https://www.npmjs.com/package/@qwen-code/qwen-code) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index b87d7a6fadd..be57f892ca3 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -738,6 +738,103 @@ describe('fetchGitDiff untracked with special filenames', () => { }); }); +describe('fetchGitDiff invocation from a subdirectory', () => { + it('returns repo-wide changes with consistent repo-root-relative path keys', async () => { + // Reproduces wenshao Critical (PR #3491 line 63): when /diff was invoked + // from a subdir, `git diff --numstat` emitted repo-root-relative keys but + // `ls-files --others` was scoped to cwd, so untracked files outside the + // subdir were silently dropped and the path basis was inconsistent. + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-sub-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.mkdir(path.join(repo, 'sub'), { recursive: true }); + await fs.writeFile(path.join(repo, 'sub', 'tracked.txt'), 'x\n'); + await fs.writeFile(path.join(repo, 'rootkeep.txt'), 'k\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: repo }); + + // Modify a tracked file inside the subdir. + await fs.writeFile(path.join(repo, 'sub', 'tracked.txt'), 'y\n'); + // Add an untracked file in a sibling location at the repo root. + await fs.writeFile(path.join(repo, 'rootnew.txt'), 'fresh\n'); + // And one in the subdir for good measure. + await fs.writeFile(path.join(repo, 'sub', 'subnew.txt'), 'a\nb\n'); + + // Invoke fetchGitDiff with cwd pointing at the SUBDIR, not the root. + const result = await fetchGitDiff(path.join(repo, 'sub')); + expect(result).not.toBeNull(); + const keys = [...result!.perFileStats.keys()].sort(); + // All path keys must be repo-root-relative (not "tracked.txt" or + // "subnew.txt" alone). And the root-level untracked file must be + // present even though we asked from sub/. + expect(keys).toEqual([ + 'rootnew.txt', + 'sub/subnew.txt', + 'sub/tracked.txt', + ]); + expect(result!.stats.filesCount).toBe(3); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); +}); + +describe('fetchGitDiff special filetypes among untracked files', () => { + it('marks untracked symlinks as binary and never follows them', async () => { + // Reproduces wenshao Critical (PR #3491 line 455): without an lstat + // gate, `open()` would dereference an untracked symlink and read its + // target — which can live outside the worktree. + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-lnk-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: repo }); + + // Create an outside-worktree target with content that, if followed, + // would push linesAdded up. The lstat gate means we never read it. + const outside = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-outside-')); + try { + await fs.writeFile( + path.join(outside, 'secret.txt'), + 'one\ntwo\nthree\n', + ); + await fs.symlink( + path.join(outside, 'secret.txt'), + path.join(repo, 'link.txt'), + ); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + const entry = result!.perFileStats.get('link.txt'); + expect(entry).toBeDefined(); + expect(entry?.isBinary).toBe(true); + expect(entry?.isUntracked).toBe(true); + // No content from the symlink target leaked into the totals. + expect(result!.stats.linesAdded).toBe(0); + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); +}); + describe('fetchGitDiff untracked counting', () => { let repo: string; beforeEach(async () => { diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 331d9a2223e..d352682f470 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -5,7 +5,7 @@ */ import { execFile } from 'node:child_process'; -import { access, open, readFile, stat } from 'node:fs/promises'; +import { access, lstat, open, readFile, stat } from 'node:fs/promises'; import * as path from 'node:path'; import { promisify } from 'node:util'; import type { Hunk } from 'diff'; @@ -65,6 +65,15 @@ export async function fetchGitDiff(cwd: string): Promise { if (!isGitRepository(cwd)) return null; if (await isInTransientGitState(cwd)) return null; + // Pin every git invocation (and on-disk file probe) to the repo root. + // `git diff` already emits repo-root-relative paths regardless of cwd, but + // `git ls-files --others` is scoped to cwd — running both from the same + // root keeps the path keys consistent and ensures untracked files in + // sibling directories aren't silently dropped when /diff is invoked from + // a subdirectory of the worktree. + const gitRoot = findGitRoot(cwd); + if (!gitRoot) return null; + // Shortstat probe + untracked scan run in parallel — both are needed // regardless of which path we take, and shortstat is O(1) memory so it can // short-circuit huge generated workspaces before we pay the per-file @@ -72,7 +81,7 @@ export async function fetchGitDiff(cwd: string): Promise { // list so the fast path only has to count NUL bytes instead of allocating // a full path array. const [shortstatOut, untrackedOut] = await Promise.all([ - runGit(['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], cwd), + runGit(['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], gitRoot), runGit( [ '--no-optional-locks', @@ -81,7 +90,7 @@ export async function fetchGitDiff(cwd: string): Promise { '--others', '--exclude-standard', ], - cwd, + gitRoot, ), ]); const untrackedCount = countNulDelimited(untrackedOut); @@ -107,7 +116,7 @@ export async function fetchGitDiff(cwd: string): Promise { const numstatOut = await runGit( ['--no-optional-locks', 'diff', 'HEAD', '--numstat', '-z'], - cwd, + gitRoot, ); if (numstatOut == null) return null; @@ -127,7 +136,9 @@ export async function fetchGitDiff(cwd: string): Promise { // already filled the per-file map. const countable = untrackedPaths.slice(0, MAX_FILES); const countableStats = await Promise.all( - countable.map((relPath) => countUntrackedLines(path.join(cwd, relPath))), + countable.map((relPath) => + countUntrackedLines(path.join(gitRoot, relPath)), + ), ); for (const s of countableStats) stats.linesAdded += s.added; @@ -162,8 +173,15 @@ export async function fetchGitDiffHunks( ): Promise> { if (!isGitRepository(cwd)) return new Map(); if (await isInTransientGitState(cwd)) return new Map(); + // Run from the repo root so hunk keys are repo-root-relative regardless of + // which subdirectory the caller is in. + const gitRoot = findGitRoot(cwd); + if (!gitRoot) return new Map(); - const diffOut = await runGit(['--no-optional-locks', 'diff', 'HEAD'], cwd); + const diffOut = await runGit( + ['--no-optional-locks', 'diff', 'HEAD'], + gitRoot, + ); if (diffOut == null) return new Map(); return parseGitDiff(diffOut); } @@ -446,10 +464,24 @@ interface UntrackedLineStats { * unreadable file can't block the whole command. `truncated` is set when * `fstat(size) > bytesRead`, so the UI can mark partial counts honestly * instead of silently under-reporting a 10 MB log as `+20k`. + * + * Uses `lstat` before `open` to gate on regular files only — git's + * `ls-files --others` can list FIFOs (whose `open()` would block forever + * waiting on a writer) and symlinks (whose target may live outside the + * worktree). Symlinks and non-regular files render as binary `~` rows. */ async function countUntrackedLines( absPath: string, ): Promise { + let st; + try { + st = await lstat(absPath); + } catch { + return { added: 0, isBinary: false, truncated: false }; + } + if (!st.isFile()) { + return { added: 0, isBinary: true, truncated: false }; + } let fh; try { fh = await open(absPath, 'r'); From c0d8a5db6e5ca5c4206830fd93ac05fd724c54ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Mon, 27 Apr 2026 14:58:25 +0800 Subject: [PATCH 12/23] chore: revert stray .npmrc/README.md test edits swept into bb0164d99 The previous fix(core) commit accidentally bundled two unrelated working-tree edits (a test comment in .npmrc and a TODO in README.md) that I had used while sanity-testing /diff. They have nothing to do with the fix; restore them to their pre-bb0164d99 state. --- .npmrc | 1 - README.md | 1 - 2 files changed, 2 deletions(-) diff --git a/.npmrc b/.npmrc index e28d5df67f9..38f11c645a0 100644 --- a/.npmrc +++ b/.npmrc @@ -1,2 +1 @@ registry=https://registry.npmjs.org -# test comment for diff verification diff --git a/README.md b/README.md index 7b30fdd4c27..5358f9d0e1f 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,3 @@ -
[![npm version](https://img.shields.io/npm/v/@qwen-code/qwen-code.svg)](https://www.npmjs.com/package/@qwen-code/qwen-code) From 002d17c250351690d5b19d458ae6f942fe151c26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 10:15:13 +0800 Subject: [PATCH 13/23] perf(core): stream untracked-file line counts in 64 KB chunks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `countUntrackedLines` allocated a fresh `UNTRACKED_READ_CAP_BYTES` (1 MB) buffer per file. With up to MAX_FILES (=50) line-counts running concurrently via `Promise.all`, the worst-case heap footprint of a single `/diff` invocation was ~50 MB of transient buffers — avoidable spike on small containers / low-memory hosts flagged by wenshao on PR #3491. Switch to a fixed 64 KB chunk buffer and read in a loop, accumulating line counts and tracking the last byte across iterations. Peak footprint is now ~3.2 MB (50 × 64 KB). Behavior is identical: same binary sniff over the first 8 KB, same truncation flag when the read hits the cap with bytes still on disk, same trailing-partial-line rule. All 44 gitDiff tests pass unchanged, including the 1.5 MB truncation test which now crosses chunk boundaries. --- packages/core/src/utils/gitDiff.ts | 79 +++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index d352682f470..4dd048364ba 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -48,6 +48,10 @@ export const MAX_LINES_PER_FILE = 400; export const MAX_FILES_FOR_DETAILS = 500; /** How much of an untracked file to read when counting its lines. */ const UNTRACKED_READ_CAP_BYTES = MAX_DIFF_SIZE_BYTES; +/** Per-file read buffer for line counting. With up to MAX_FILES (=50) files + * reading concurrently, the worst-case heap footprint is ~3.2 MB instead of + * the ~50 MB a single full-cap allocation per file would cost. */ +const UNTRACKED_READ_CHUNK_BYTES = 64 * 1024; /** Scan the first N bytes for NUL to detect binary files (matches git's heuristic). */ const BINARY_SNIFF_BYTES = 8 * 1024; @@ -489,33 +493,60 @@ async function countUntrackedLines( return { added: 0, isBinary: false, truncated: false }; } try { - const buf = Buffer.alloc(UNTRACKED_READ_CAP_BYTES); - const { bytesRead } = await fh.read(buf, 0, UNTRACKED_READ_CAP_BYTES, 0); - if (bytesRead === 0) { - return { added: 0, isBinary: false, truncated: false }; - } - const sniffEnd = Math.min(BINARY_SNIFF_BYTES, bytesRead); - for (let i = 0; i < sniffEnd; i++) { - if (buf[i] === 0) { - return { added: 0, isBinary: true, truncated: false }; + // Stream the file in fixed-size chunks instead of allocating one full + // `UNTRACKED_READ_CAP_BYTES` buffer per call. With up to MAX_FILES + // line-counts running concurrently the heap footprint stays around + // `MAX_FILES * UNTRACKED_READ_CHUNK_BYTES` (~3.2 MB) rather than the + // ~50 MB a one-shot full-cap alloc would have cost on a constrained + // host. Behavior (line count, binary sniff, truncation flag) is + // identical to the single-shot path. + const buf = Buffer.allocUnsafe(UNTRACKED_READ_CHUNK_BYTES); + let totalRead = 0; + let lines = 0; + let lastByte = -1; + let sniffedBytes = 0; + while (totalRead < UNTRACKED_READ_CAP_BYTES) { + const remaining = UNTRACKED_READ_CAP_BYTES - totalRead; + const toRead = Math.min(buf.length, remaining); + const { bytesRead } = await fh.read(buf, 0, toRead, totalRead); + if (bytesRead === 0) break; + + // Binary sniff on the first BINARY_SNIFF_BYTES across cumulative reads. + // Almost always completes inside the first chunk because chunk size + // (64 KB) is much larger than the sniff window (8 KB). + if (sniffedBytes < BINARY_SNIFF_BYTES) { + const sniffEnd = Math.min(bytesRead, BINARY_SNIFF_BYTES - sniffedBytes); + for (let i = 0; i < sniffEnd; i++) { + if (buf[i] === 0) { + return { added: 0, isBinary: true, truncated: false }; + } + } + sniffedBytes += sniffEnd; + } + + for (let i = 0; i < bytesRead; i++) { + if (buf[i] === 0x0a) lines++; } + lastByte = buf[bytesRead - 1] ?? -1; + totalRead += bytesRead; } - // Compare against real file size rather than "bytesRead === cap" — - // a file that happens to be exactly UNTRACKED_READ_CAP_BYTES isn't - // truncated, and a short-read against a larger file still is. - const { size } = await fh.stat(); - const truncated = size > bytesRead; - let lines = 0; - for (let i = 0; i < bytesRead; i++) { - if (buf[i] === 0x0a) lines++; + + if (totalRead === 0) { + return { added: 0, isBinary: false, truncated: false }; + } + // Truncated only when we hit the cap with more bytes still on disk. + // A `read()` returning 0 means EOF, so we naturally exit untruncated. + let truncated = false; + if (totalRead >= UNTRACKED_READ_CAP_BYTES) { + const { size } = await fh.stat(); + truncated = size > totalRead; } - // If the portion we read does not end on a newline, count the trailing - // partial line — but only when the read reached EOF. When the read was - // cut short by the cap, the "trailing partial" is actually a complete - // line that continues past the buffer and will be counted when the - // following `\n` is eventually seen by a future larger cap; counting it - // here would double-count once the cap is raised. - if (!truncated && buf[bytesRead - 1] !== 0x0a) lines++; + // If the portion we read ends mid-line (no trailing `\n`) and the read + // reached EOF, count that trailing partial line. When the read was cut + // short by the cap, the "trailing partial" is really a line that + // continues past the cap; counting it here would double-count once the + // cap is raised. + if (!truncated && lastByte !== 0x0a) lines++; return { added: lines, isBinary: false, truncated }; } catch { return { added: 0, isBinary: false, truncated: false }; From 6b3ea4181174dc4dbddbcc2fb31c9708c56b10e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 10:33:47 +0800 Subject: [PATCH 14/23] refactor(core): collapse redundant ancestor walks; harden untracked open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-round audit of the recent /diff fixes turned up two real issues: 1. `fetchGitDiff` and `fetchGitDiffHunks` walked worktree ancestors three times each — `isGitRepository(cwd)`, then `isInTransientGitState → resolveGitDir → findGitRoot`, then `findGitRoot(cwd)` to pin git ops to the root. Resolve once at the top, then thread `gitRoot` everywhere. Removes the now-dead `isGitRepository` import and adds a private `resolveGitDirFromRoot(gitRoot)` so the public `resolveGitDir(cwd)` contract stays untouched for the test suite and external callers. 2. `countUntrackedLines` had a TOCTOU window between `lstat` (which gates on regular files) and `open(absPath, 'r')` — if the path was replaced by a symlink in that gap, `open` would silently follow it and read the target. Open with `O_RDONLY | O_NOFOLLOW` (falling back to `O_RDONLY` on platforms that don't expose the flag, e.g. Windows) so the open rejects with `ELOOP` instead. Also unified the five "couldn't read this file" branches (lstat throws / non-regular / open throws / binary sniff / mid-read failure) to all return `{isBinary:true, added:0}` — the row appears in the listing as an opaque `~ (binary, new)` marker rather than masquerading as an empty text file with `+0 (new)`. 44 gitDiff tests pass unchanged. --- packages/core/src/utils/gitDiff.ts | 67 +++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 4dd048364ba..36396dbbaa4 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -5,11 +5,12 @@ */ import { execFile } from 'node:child_process'; +import { constants as fsConstants } from 'node:fs'; import { access, lstat, open, readFile, stat } from 'node:fs/promises'; import * as path from 'node:path'; import { promisify } from 'node:util'; import type { Hunk } from 'diff'; -import { findGitRoot, isGitRepository } from './gitUtils.js'; +import { findGitRoot } from './gitUtils.js'; /** Re-export so consumers don't need to depend on `diff` directly. */ export type GitDiffHunk = Hunk; @@ -54,6 +55,13 @@ const UNTRACKED_READ_CAP_BYTES = MAX_DIFF_SIZE_BYTES; const UNTRACKED_READ_CHUNK_BYTES = 64 * 1024; /** Scan the first N bytes for NUL to detect binary files (matches git's heuristic). */ const BINARY_SNIFF_BYTES = 8 * 1024; +/** Open flags for line counting. `O_NOFOLLOW` closes the TOCTOU window + * between the `lstat` symlink check and `open` — if the path is replaced + * with a symlink in that gap, `open` rejects with `ELOOP` instead of + * silently dereferencing it. Falls back to plain `O_RDONLY` on platforms + * that don't expose the flag (Windows constants omit `O_NOFOLLOW`). */ +const UNTRACKED_OPEN_FLAGS = + fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0); /** * Fetch numstat-based git diff stats (files changed, lines added/removed) and @@ -66,17 +74,17 @@ const BINARY_SNIFF_BYTES = 8 * 1024; * made by the user. */ export async function fetchGitDiff(cwd: string): Promise { - if (!isGitRepository(cwd)) return null; - if (await isInTransientGitState(cwd)) return null; - - // Pin every git invocation (and on-disk file probe) to the repo root. - // `git diff` already emits repo-root-relative paths regardless of cwd, but - // `git ls-files --others` is scoped to cwd — running both from the same - // root keeps the path keys consistent and ensures untracked files in - // sibling directories aren't silently dropped when /diff is invoked from - // a subdirectory of the worktree. + // Walk ancestors once to find the worktree root; reuse the result for the + // transient-state probe and every git invocation below. `findGitRoot` + // doubles as the "is this a git repo" check — a non-null return implies a + // repo. `git diff` already emits repo-root-relative paths regardless of + // cwd, but `git ls-files --others` is scoped to cwd, so pinning everything + // to the same root keeps the path keys consistent and ensures untracked + // files in sibling directories aren't silently dropped when /diff is + // invoked from a subdirectory of the worktree. const gitRoot = findGitRoot(cwd); if (!gitRoot) return null; + if (await isInTransientGitState(gitRoot)) return null; // Shortstat probe + untracked scan run in parallel — both are needed // regardless of which path we take, and shortstat is O(1) memory so it can @@ -175,12 +183,12 @@ export async function fetchGitDiff(cwd: string): Promise { export async function fetchGitDiffHunks( cwd: string, ): Promise> { - if (!isGitRepository(cwd)) return new Map(); - if (await isInTransientGitState(cwd)) return new Map(); - // Run from the repo root so hunk keys are repo-root-relative regardless of - // which subdirectory the caller is in. + // Walk ancestors once; reuse for the transient-state probe and the diff + // call. Running from the repo root also keeps hunk keys repo-root-relative + // regardless of which subdirectory the caller is in. const gitRoot = findGitRoot(cwd); if (!gitRoot) return new Map(); + if (await isInTransientGitState(gitRoot)) return new Map(); const diffOut = await runGit( ['--no-optional-locks', 'diff', 'HEAD'], @@ -481,16 +489,23 @@ async function countUntrackedLines( try { st = await lstat(absPath); } catch { - return { added: 0, isBinary: false, truncated: false }; + // File raced out from under ls-files (deleted, permission revoked, etc.). + // Surface it as a binary row to be consistent with the open-failure / + // non-regular-file branches below — `+0 (new)` would lie about it being + // an empty text file when we genuinely have no signal. + return { added: 0, isBinary: true, truncated: false }; } if (!st.isFile()) { return { added: 0, isBinary: true, truncated: false }; } let fh; try { - fh = await open(absPath, 'r'); + fh = await open(absPath, UNTRACKED_OPEN_FLAGS); } catch { - return { added: 0, isBinary: false, truncated: false }; + // ELOOP from O_NOFOLLOW (path raced into a symlink between lstat and + // open) and any other open error all collapse to a binary row so the + // file appears once in the listing without contributing line counts. + return { added: 0, isBinary: true, truncated: false }; } try { // Stream the file in fixed-size chunks instead of allocating one full @@ -549,7 +564,10 @@ async function countUntrackedLines( if (!truncated && lastByte !== 0x0a) lines++; return { added: lines, isBinary: false, truncated }; } catch { - return { added: 0, isBinary: false, truncated: false }; + // Mid-read failure (EIO, fh.stat throwing, etc.). Discard the partial + // count and surface as binary — same opaque marker as every other + // "we couldn't read this" branch in this function. + return { added: 0, isBinary: true, truncated: false }; } finally { await fh.close().catch(() => {}); } @@ -563,6 +581,15 @@ async function countUntrackedLines( export async function resolveGitDir(cwd: string): Promise { const gitRoot = findGitRoot(cwd); if (!gitRoot) return null; + return resolveGitDirFromRoot(gitRoot); +} + +/** + * Same contract as `resolveGitDir`, but skips the ancestor walk when the + * caller has already resolved the worktree root. Used by `fetchGitDiff` / + * `fetchGitDiffHunks` so they walk ancestors at most once per invocation. + */ +async function resolveGitDirFromRoot(gitRoot: string): Promise { const dotGit = path.join(gitRoot, '.git'); try { const s = await stat(dotGit); @@ -578,8 +605,8 @@ export async function resolveGitDir(cwd: string): Promise { } } -async function isInTransientGitState(cwd: string): Promise { - const gitDir = await resolveGitDir(cwd); +async function isInTransientGitState(gitRoot: string): Promise { + const gitDir = await resolveGitDirFromRoot(gitRoot); if (!gitDir) return false; // Rebase-in-progress is signalled by a directory, not a ref file. Both From 24dfb87268a5514a5c17634812e56348b284ed23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 10:47:13 +0800 Subject: [PATCH 15/23] docs(cli): clarify row-order contract; simplify DiffRow key Two non-blocking suggestions from qqqys's CR on PR #3491: - `buildDiffRenderModel`: expand the JSDoc to call out the implicit row-ordering contract that both renderers depend on (tracked entries first in numstat order, then untracked appended in ls-files order). Future replacements of the underlying Map need to preserve this sequence. - `DiffStatsDisplay`: drop the `${i}-${filename}` React key in favor of bare `filename`. Filenames are unique within a single `DiffRenderModel` (perFileStats is a Map keyed by filename), so the index prefix added no information. --- packages/cli/src/ui/commands/diffCommand.ts | 10 ++++++++-- .../src/ui/components/messages/DiffStatsDisplay.tsx | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index 4d09dc1c86c..aa1e44982db 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -99,8 +99,14 @@ async function diffAction( /** * Convert the raw `fetchGitDiff` result into a display-ready structure that - * both the Ink component and the plain-text renderer consume. Order of rows - * mirrors git's numstat output (which uses alphabetical or insertion order). + * both the Ink component and the plain-text renderer consume. + * + * Row order is the iteration order of `result.perFileStats`, which is a + * `Map` and therefore preserves insertion order: tracked numstat entries + * first (alphabetical, as git emits them), then untracked entries appended + * by `fetchGitDiff` in their `ls-files --others` order. Renderers depend on + * this — if `perFileStats` ever switches to a different container, the row + * sequence must continue to be stable across runs. */ export function buildDiffRenderModel(result: GitDiffResult): DiffRenderModel { const rows: DiffRenderRow[] = []; diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx index 8fc342f862c..8093c837802 100644 --- a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx @@ -44,9 +44,9 @@ export const DiffStatsDisplay: React.FC = ({ / -{linesRemoved} - {rows.map((row, i) => ( + {rows.map((row) => ( Date: Tue, 28 Apr 2026 11:30:02 +0800 Subject: [PATCH 16/23] feat(cli): add (deleted) marker for files removed in the worktree Symmetrical to the (new) marker for untracked files: tracked files that were removed from the worktree relative to HEAD now render with a (deleted) suffix (or (binary, deleted) for binary deletes), so users can tell a delete apart from a heavy edit. Implementation: - core: `fetchGitDiff` now runs `git diff HEAD --name-status -z` in parallel with the existing numstat call. `parseDeletedFromNameStatus` extracts the set of D-status paths (skipping R/C rename and copy pairs, both halves of which still exist on disk under one name or the other). Each `perFileStats` entry whose key is in that set gets `isDeleted: true`. Numstat alone could not distinguish a delete (`0\t10\tpath`) from a heavy edit; the name-status pass disambiguates. - cli: `DiffRenderRow` carries `isDeleted: boolean`; both the plain-text renderer and the Ink component append the new suffix in `theme.text.secondary` (dim). - i18n: new `(deleted)` and `(binary, deleted)` keys in en/zh/zh-TW. Tests: - Unit: `parseDeletedFromNameStatus` covers D-only extraction, R/C pair skipping, NUL-safe paths (tabs / non-ASCII), and empty input. - Integration: real repo deletes a tracked text + a tracked binary plus edits another file; asserts the deleted entries get `isDeleted: true` but the heavy edit does not. Second test verifies neither half of a `git mv` rename gets flagged as deleted. - CLI / component: `(deleted)` and `(binary, deleted)` rendering variants with column alignment intact. --- packages/cli/src/i18n/locales/en.js | 2 + packages/cli/src/i18n/locales/zh-TW.js | 2 + packages/cli/src/i18n/locales/zh.js | 2 + .../cli/src/ui/commands/diffCommand.test.ts | 38 +++++++++ packages/cli/src/ui/commands/diffCommand.ts | 8 +- .../messages/DiffStatsDisplay.test.tsx | 32 +++++++ .../components/messages/DiffStatsDisplay.tsx | 8 +- packages/cli/src/ui/types.ts | 3 + packages/core/src/utils/gitDiff.test.ts | 85 +++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 61 ++++++++++++- scripts/unused-keys-only-in-locales.json | 2 +- 11 files changed, 236 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index b38a117abb8..26a877ad036 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -174,6 +174,8 @@ export default { '(binary, new)': '(binary, new)', '(new)': '(new)', '(new, partial)': '(new, partial)', + '(deleted)': '(deleted)', + '(binary, deleted)': '(binary, deleted)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/i18n/locales/zh-TW.js b/packages/cli/src/i18n/locales/zh-TW.js index d4a9906ba30..f1f3f5ea5fa 100644 --- a/packages/cli/src/i18n/locales/zh-TW.js +++ b/packages/cli/src/i18n/locales/zh-TW.js @@ -156,6 +156,8 @@ export default { '(binary, new)': '(二進位,新增)', '(new)': '(新增)', '(new, partial)': '(新增,部分統計)', + '(deleted)': '(已刪除)', + '(binary, deleted)': '(二進位,已刪除)', 'Manage subagents for specialized task delegation.': '管理用於專門任務委派的子智能體', 'Manage existing subagents (view, edit, delete).': diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index fb2f674e717..cd94abace67 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -169,6 +169,8 @@ export default { '(binary, new)': '(二进制,新增)', '(new)': '(新增)', '(new, partial)': '(新增,部分统计)', + '(deleted)': '(已删除)', + '(binary, deleted)': '(二进制,已删除)', // ============================================================================ // Commands - Agents diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index ca267b0671b..c4f8b1075a7 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -196,6 +196,39 @@ describe('diffCommand', () => { expect(row).not.toContain(' (new)'); }); + it('marks deleted tracked files with (deleted)', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1, linesAdded: 0, linesRemoved: 5 }, + perFileStats: new Map([ + [ + 'gone.txt', + { added: 0, removed: 5, isBinary: false, isDeleted: true }, + ], + ]), + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + const row = content.split('\n').find((l) => l.includes('gone.txt'))!; + expect(row).toContain('(deleted)'); + expect(row).toContain('-5'); + }); + + it('marks deleted binary tracked files with (binary, deleted)', async () => { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1, linesAdded: 0, linesRemoved: 0 }, + perFileStats: new Map([ + ['gone.bin', { added: 0, removed: 0, isBinary: true, isDeleted: true }], + ]), + } satisfies GitDiffResult); + const result = await diffCommand.action(mockContext, ''); + const content = (result as { content: string }).content; + const row = content.split('\n').find((l) => l.includes('gone.bin'))!; + expect(row).toContain('(binary, deleted)'); + expect(row.trimStart().startsWith('~')).toBe(true); + }); + it('marks binary untracked files with (binary, new) and no line count', async () => { if (!diffCommand.action) throw new Error('Command has no action'); mockFetchGitDiff.mockResolvedValue({ @@ -361,6 +394,7 @@ describe('computeDiffColumnWidths', () => { removed: 5, isBinary: false, isUntracked: false, + isDeleted: false, truncated: false, }, { @@ -369,6 +403,7 @@ describe('computeDiffColumnWidths', () => { removed: 100, isBinary: false, isUntracked: false, + isDeleted: false, truncated: false, }, ]); @@ -387,12 +422,14 @@ describe('computeDiffColumnWidths', () => { removed: 1, isBinary: false, isUntracked: false, + isDeleted: false, truncated: false, }, { filename: 'b.bin', isBinary: true, isUntracked: false, + isDeleted: false, truncated: false, }, ]); @@ -409,6 +446,7 @@ describe('computeDiffColumnWidths', () => { removed: 0, isBinary: false, isUntracked: true, + isDeleted: false, truncated: false, }, ]); diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index aa1e44982db..78c20ca4898 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -129,6 +129,7 @@ function toRow(filename: string, s: PerFileStats): DiffRenderRow { filename, isBinary: true, isUntracked: Boolean(s.isUntracked), + isDeleted: Boolean(s.isDeleted), truncated: false, }; } @@ -138,6 +139,7 @@ function toRow(filename: string, s: PerFileStats): DiffRenderRow { removed: s.isUntracked ? 0 : s.removed, isBinary: false, isUntracked: Boolean(s.isUntracked), + isDeleted: Boolean(s.isDeleted), truncated: Boolean(s.truncated), }; } @@ -213,7 +215,9 @@ function formatRowsText(rows: DiffRenderRow[]): string[] { if (r.isBinary) { const suffix = r.isUntracked ? ` ${t('(binary, new)')}` - : ` ${t('(binary)')}`; + : r.isDeleted + ? ` ${t('(binary, deleted)')}` + : ` ${t('(binary)')}`; out.push(` ${padMarker('~', statColumnWidth)} ${r.filename}${suffix}`); continue; } @@ -222,6 +226,8 @@ function formatRowsText(rows: DiffRenderRow[]): string[] { let suffix = ''; if (r.isUntracked) { suffix = r.truncated ? ` ${t('(new, partial)')}` : ` ${t('(new)')}`; + } else if (r.isDeleted) { + suffix = ` ${t('(deleted)')}`; } out.push(` ${added} ${removed} ${r.filename}${suffix}`); } diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx index abf0edc5fde..e36e6862a4a 100644 --- a/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.test.tsx @@ -28,6 +28,7 @@ describe('DiffStatsDisplay', () => { removed: 2, isBinary: false, isUntracked: false, + isDeleted: false, truncated: false, }, { @@ -36,6 +37,7 @@ describe('DiffStatsDisplay', () => { removed: 1, isBinary: false, isUntracked: false, + isDeleted: false, truncated: false, }, ], @@ -64,6 +66,7 @@ describe('DiffStatsDisplay', () => { removed: 0, isBinary: false, isUntracked: true, + isDeleted: false, truncated: false, }, ], @@ -88,6 +91,7 @@ describe('DiffStatsDisplay', () => { removed: 0, isBinary: false, isUntracked: true, + isDeleted: false, truncated: true, }, ], @@ -98,6 +102,32 @@ describe('DiffStatsDisplay', () => { expect(visible).toContain('(new, partial)'); }); + it('renders the (deleted) marker for tracked files removed from the worktree', () => { + const model: DiffRenderModel = { + filesCount: 1, + linesAdded: 0, + linesRemoved: 5, + hiddenCount: 0, + rows: [ + { + filename: 'gone.txt', + added: 0, + removed: 5, + isBinary: false, + isUntracked: false, + isDeleted: true, + truncated: false, + }, + ], + }; + const visible = stripAnsi( + render().lastFrame() ?? '', + ); + const row = visible.split('\n').find((l) => l.includes('gone.txt'))!; + expect(row).toContain('(deleted)'); + expect(row).toContain('-5'); + }); + it('renders binary rows with a ~ marker and no +N/-M', () => { const model: DiffRenderModel = { filesCount: 1, @@ -109,6 +139,7 @@ describe('DiffStatsDisplay', () => { filename: 'img.png', isBinary: true, isUntracked: false, + isDeleted: false, truncated: false, }, ], @@ -135,6 +166,7 @@ describe('DiffStatsDisplay', () => { removed: 0, isBinary: false, isUntracked: false, + isDeleted: false, truncated: false, }, ], diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx index 8093c837802..05c4aeb066c 100644 --- a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx @@ -83,7 +83,11 @@ const DiffRow: React.FC = ({ }) => { if (row.isBinary) { const marker = padRight('~', statColumnWidth); - const suffix = row.isUntracked ? t('(binary, new)') : t('(binary)'); + const suffix = row.isUntracked + ? t('(binary, new)') + : row.isDeleted + ? t('(binary, deleted)') + : t('(binary)'); return ( @@ -101,6 +105,8 @@ const DiffRow: React.FC = ({ let suffix: string | null = null; if (row.isUntracked) { suffix = row.truncated ? t('(new, partial)') : t('(new)'); + } else if (row.isDeleted) { + suffix = t('(deleted)'); } return ( diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 8bf1fae5669..ccd3d19363a 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -189,6 +189,9 @@ export interface DiffRenderRow { removed?: number; isBinary: boolean; isUntracked: boolean; + /** `true` when the file is removed from the worktree relative to HEAD. + * Mutually exclusive with `isUntracked`. */ + isDeleted: boolean; /** Only set for untracked text files that exceeded the read cap. */ truncated: boolean; } diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index be57f892ca3..7ca8d45824e 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -16,6 +16,7 @@ import { MAX_DIFF_SIZE_BYTES, MAX_FILES, MAX_LINES_PER_FILE, + parseDeletedFromNameStatus, parseGitDiff, parseGitNumstat, parseShortstat, @@ -109,6 +110,36 @@ describe('parseGitNumstat', () => { }); }); +describe('parseDeletedFromNameStatus', () => { + it('extracts D-status paths and ignores M/A entries', () => { + const out = 'D\0gone.txt\0M\0kept.txt\0A\0added.txt\0D\0also-gone.txt\0'; + expect(parseDeletedFromNameStatus(out)).toEqual( + new Set(['gone.txt', 'also-gone.txt']), + ); + }); + + it('skips both halves of rename and copy entries', () => { + // Renames/copies span three tokens: `R\0\0\0`. Neither + // path is "deleted" in the user sense — the file still exists under + // the new name. + const out = + 'R100\0old.txt\0new.txt\0' + 'C75\0src.txt\0copy.txt\0' + 'D\0gone.txt\0'; + expect(parseDeletedFromNameStatus(out)).toEqual(new Set(['gone.txt'])); + }); + + it('preserves NUL-safe paths (tabs, non-ASCII)', () => { + // -z keeps raw bytes — same guarantee as the numstat path. + const out = 'D\0tab\there.txt\0D\0日本語.txt\0'; + expect(parseDeletedFromNameStatus(out)).toEqual( + new Set(['tab\there.txt', '日本語.txt']), + ); + }); + + it('handles empty input', () => { + expect(parseDeletedFromNameStatus('')).toEqual(new Set()); + }); +}); + describe('parseShortstat', () => { it('parses the full form', () => { expect( @@ -786,6 +817,60 @@ describe('fetchGitDiff invocation from a subdirectory', () => { }); }); +describe('fetchGitDiff deletion detection', () => { + let repo: string; + beforeEach(async () => { + repo = await makeRepo(); + }); + afterEach(async () => { + await fs.rm(repo, { recursive: true, force: true }); + }); + + it('marks tracked files removed from the worktree as isDeleted', async () => { + await fs.writeFile(path.join(repo, 'kept.txt'), 'one\ntwo\n'); + await fs.writeFile(path.join(repo, 'gone.txt'), 'a\nb\nc\n'); + await fs.writeFile( + path.join(repo, 'gone.bin'), + Buffer.from([0x89, 0x00, 0xff]), + ); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + + // Modify one tracked file (heavy edit), and remove two others. + await fs.writeFile(path.join(repo, 'kept.txt'), ''); + await fs.rm(path.join(repo, 'gone.txt')); + await fs.rm(path.join(repo, 'gone.bin')); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + // Heavy edit must NOT be marked deleted, even though numstat shows + // `0\t2\tkept.txt` (which looks identical to a delete shape). + expect(result!.perFileStats.get('kept.txt')?.isDeleted).toBeFalsy(); + expect(result!.perFileStats.get('gone.txt')?.isDeleted).toBe(true); + expect(result!.perFileStats.get('gone.bin')).toMatchObject({ + isBinary: true, + isDeleted: true, + }); + }); + + it('does not mark either side of a rename as deleted', async () => { + await fs.writeFile(path.join(repo, 'old.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await git(repo, 'mv', 'old.txt', 'new.txt'); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + // The rename collapses to a single "old => new" entry; it must not + // be flagged as deleted. + const keys = [...result!.perFileStats.keys()]; + expect(keys.some((k) => k.includes('=>'))).toBe(true); + for (const s of result!.perFileStats.values()) { + expect(s.isDeleted).toBeFalsy(); + } + }); +}); + describe('fetchGitDiff special filetypes among untracked files', () => { it('marks untracked symlinks as binary and never follows them', async () => { // Reproduces wenshao Critical (PR #3491 line 455): without an lstat diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 36396dbbaa4..2fc0cd93054 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -28,6 +28,12 @@ export interface PerFileStats { removed: number; isBinary: boolean; isUntracked?: boolean; + /** `true` when the file is removed in the worktree relative to HEAD. + * Mutually exclusive with `isUntracked`. Detected via + * `git diff HEAD --name-status -z` (status letter `D`); a row like + * `0\t10\tfoo.ts` from numstat alone is not enough to distinguish + * "deleted" from "heavy edit that drops 10 lines". */ + isDeleted?: boolean; /** Only meaningful for untracked files: `true` when the file exceeded the * line-counting read cap and `added` is therefore a lower bound. */ truncated?: boolean; @@ -126,13 +132,27 @@ export async function fetchGitDiff(cwd: string): Promise { } } - const numstatOut = await runGit( - ['--no-optional-locks', 'diff', 'HEAD', '--numstat', '-z'], - gitRoot, - ); + // Numstat gives us +/- counts; name-status tells us *why* a row exists + // (D = deleted, M = modified, R = rename, etc.). We need both + // because numstat alone can't distinguish a delete (`0\tN\tpath`) from + // a heavy edit that drops N lines. + const [numstatOut, nameStatusOut] = await Promise.all([ + runGit(['--no-optional-locks', 'diff', 'HEAD', '--numstat', '-z'], gitRoot), + runGit( + ['--no-optional-locks', 'diff', 'HEAD', '--name-status', '-z'], + gitRoot, + ), + ]); if (numstatOut == null) return null; const { stats, perFileStats } = parseGitNumstat(numstatOut); + const deletedPaths = + nameStatusOut != null ? parseDeletedFromNameStatus(nameStatusOut) : null; + if (deletedPaths && deletedPaths.size > 0) { + for (const [filename, s] of perFileStats) { + if (deletedPaths.has(filename)) s.isDeleted = true; + } + } if (untrackedCount > 0) { // Count every untracked file in the totals, even if the per-file map is @@ -447,6 +467,39 @@ export function parseShortstat(stdout: string): GitDiffStats | null { }; } +/** + * Parse `git diff HEAD --name-status -z` output and return the paths whose + * status is `D` (deleted in the worktree). + * + * Wire format with `-z`: `\0\0` per entry, except renames and + * copies which span three tokens: `R\0\0\0` (and + * `C\0...`). We only care about deletions here, so renames/copies + * are walked past — neither half of a rename pair is "deleted" in the + * user-facing sense (the file still exists under the new name). + */ +export function parseDeletedFromNameStatus(stdout: string): Set { + const tokens = stdout.split('\0'); + if (tokens.length > 0 && tokens[tokens.length - 1] === '') tokens.pop(); + + const deleted = new Set(); + let i = 0; + while (i < tokens.length) { + const status = tokens[i] ?? ''; + i++; + if (status === '') continue; + const head = status[0]; + // Rename / copy entries are followed by TWO path tokens. + if (head === 'R' || head === 'C') { + i += 2; + continue; + } + const path = tokens[i] ?? ''; + i++; + if (head === 'D' && path !== '') deleted.add(path); + } + return deleted; +} + function countNulDelimited(stdout: string | null): number { if (!stdout) return 0; let count = 0; diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index 9d599490e64..203338abaa1 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-27T03:39:04.133Z", + "generatedAt": "2026-04-28T03:29:36.589Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)", From 667f01a562b42e5e1f7373262d3e97b5d762560b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 16:15:49 +0800 Subject: [PATCH 17/23] fix(core): pin --no-ext-diff on every git diff invocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plain `git diff HEAD` honors `GIT_EXTERNAL_DIFF` and configured `diff..command` drivers, so the exported `fetchGitDiffHunks` utility could execute arbitrary commands when invoked inside a worktree whose user-global or repo-local config registers an external driver. Add `--no-ext-diff` to every `git diff` call: - `fetchGitDiffHunks`'s plain `git diff HEAD` — the actual vulnerability surface. - `fetchGitDiff`'s `--shortstat`, `--numstat`, `--name-status` variants — defense-in-depth. Empirically these stats modes already bypass external drivers in current git, but git's behavior here has shifted between versions before, and pinning the flag everywhere is a zero-cost hardening that keeps the policy uniform across every `git diff` we run. Regression test plants `GIT_EXTERNAL_DIFF=evil.sh` (a driver that writes a sentinel file as its side effect) before calling `fetchGitDiffHunks`, then asserts the sentinel never appears — confirming `--no-ext-diff` actually stops git from spawning the driver. Closes wenshao critical comment on PR #3491. --- packages/core/src/utils/gitDiff.test.ts | 61 +++++++++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 40 ++++++++++++++-- 2 files changed, 97 insertions(+), 4 deletions(-) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 7ca8d45824e..519fda008ed 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -817,6 +817,67 @@ describe('fetchGitDiff invocation from a subdirectory', () => { }); }); +describe('fetchGitDiffHunks ignores external diff drivers', () => { + it('does not invoke GIT_EXTERNAL_DIFF when reading hunks', async () => { + // Reproduces wenshao Critical (PR #3491 line 219). Plain `git diff` + // honors `GIT_EXTERNAL_DIFF` / `diff..command`, so a malicious + // worktree could execute arbitrary commands when a caller of + // `fetchGitDiffHunks` only wants to inspect hunks. The fix is + // `--no-ext-diff`; this test plants an env-var driver that touches a + // sentinel file and asserts it never fires. + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-ext-')); + const sentinel = path.join(os.tmpdir(), `qwen-ext-fired-${Date.now()}`); + const driverScript = path.join(repo, 'evil-diff.sh'); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(path.join(repo, 'a.txt'), 'one\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: repo }); + await fs.writeFile(path.join(repo, 'a.txt'), 'two\n'); + + // Driver writes the sentinel as a side effect when invoked. + await fs.writeFile( + driverScript, + `#!/bin/sh\necho fired > "${sentinel}"\n`, + { mode: 0o755 }, + ); + + // Set GIT_EXTERNAL_DIFF for the rest of this test. fetchGitDiffHunks + // calls runGit which spawns child processes that inherit our env. + const prev = process.env['GIT_EXTERNAL_DIFF']; + process.env['GIT_EXTERNAL_DIFF'] = driverScript; + try { + const hunks = await fetchGitDiffHunks(repo); + expect(hunks.get('a.txt')).toBeDefined(); + } finally { + if (prev === undefined) delete process.env['GIT_EXTERNAL_DIFF']; + else process.env['GIT_EXTERNAL_DIFF'] = prev; + } + + // The sentinel must NOT exist — `--no-ext-diff` should have stopped + // git from running the driver. + let driverFired = false; + try { + await fs.stat(sentinel); + driverFired = true; + } catch { + // ENOENT — driver never ran. Expected. + } + expect(driverFired).toBe(false); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + await fs.rm(sentinel, { force: true }); + } + }); +}); + describe('fetchGitDiff deletion detection', () => { let repo: string; beforeEach(async () => { diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 2fc0cd93054..c12d3bfdf6a 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -98,8 +98,18 @@ export async function fetchGitDiff(cwd: string): Promise { // numstat cost. For untracked we hold the raw stdout rather than the parsed // list so the fast path only has to count NUL bytes instead of allocating // a full path array. + // Every `git diff` invocation passes `--no-ext-diff` so a malicious + // repo-local or user-global config that registers an external diff driver + // (`GIT_EXTERNAL_DIFF` or `diff..command`) can never run when /diff + // touches the worktree. In practice the stats variants (`--shortstat`, + // `--numstat`, `--name-status`) do not invoke external drivers, but + // pinning the flag everywhere is defense-in-depth — git's behavior + // around external drivers has shifted between versions before. const [shortstatOut, untrackedOut] = await Promise.all([ - runGit(['--no-optional-locks', 'diff', 'HEAD', '--shortstat'], gitRoot), + runGit( + ['--no-optional-locks', 'diff', '--no-ext-diff', 'HEAD', '--shortstat'], + gitRoot, + ), runGit( [ '--no-optional-locks', @@ -137,9 +147,26 @@ export async function fetchGitDiff(cwd: string): Promise { // because numstat alone can't distinguish a delete (`0\tN\tpath`) from // a heavy edit that drops N lines. const [numstatOut, nameStatusOut] = await Promise.all([ - runGit(['--no-optional-locks', 'diff', 'HEAD', '--numstat', '-z'], gitRoot), runGit( - ['--no-optional-locks', 'diff', 'HEAD', '--name-status', '-z'], + [ + '--no-optional-locks', + 'diff', + '--no-ext-diff', + 'HEAD', + '--numstat', + '-z', + ], + gitRoot, + ), + runGit( + [ + '--no-optional-locks', + 'diff', + '--no-ext-diff', + 'HEAD', + '--name-status', + '-z', + ], gitRoot, ), ]); @@ -210,8 +237,13 @@ export async function fetchGitDiffHunks( if (!gitRoot) return new Map(); if (await isInTransientGitState(gitRoot)) return new Map(); + // `--no-ext-diff` is critical here: without it, a repo-local or user- + // global config that sets `GIT_EXTERNAL_DIFF` or `diff..command` + // would let `git diff` execute arbitrary commands when this read-only + // utility is invoked. The stats variants in `fetchGitDiff` already + // bypass external drivers, but plain `git diff` honors them. const diffOut = await runGit( - ['--no-optional-locks', 'diff', 'HEAD'], + ['--no-optional-locks', 'diff', '--no-ext-diff', 'HEAD'], gitRoot, ); if (diffOut == null) return new Map(); From efa81db6239062e283c54c90cfc0aa92ad7ef3b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 16:43:00 +0800 Subject: [PATCH 18/23] fix(core): lazy O_NOFOLLOW lookup so vitest fs mocks don't blow up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #3491 CI was failing across all 9 platform/Node combos with: Error: [vitest] No "constants" export is defined on the "node:fs" mock. at gitDiff.ts:70 const UNTRACKED_OPEN_FLAGS = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0) at index.ts:279 export * from './utils/gitDiff.js' Six unrelated test files (`client.test.ts`, `geminiChat.test.ts`, `marketplace.test.ts`, `npm.test.ts`, `mcp-client.test.ts`, `nextSpeakerChecker.test.ts`) `vi.mock('node:fs', ...)` without returning `constants`, and their transitive import of `@qwen-code/qwen-code-core` pulls in `gitDiff.ts`, whose module-load-time `import { constants as fsConstants }` plus the top-level `UNTRACKED_OPEN_FLAGS` constant tripped vitest's strict mock proxy. Two changes: 1. Switch `import { constants }` to `import * as nodeFs from 'node:fs'`. Strict-mock no longer rejects the import statement itself. 2. Move the flag computation out of a module-load constant into a memoized `getUntrackedOpenFlags()` called from inside `countUntrackedLines`. Tests that don't actually invoke `fetchGitDiff` / `fetchGitDiffHunks` (i.e. all six broken ones) never reach the property access, so vitest's proxy never trips. `?? 0` fallback on each constant lookup is preserved so Windows (no `O_NOFOLLOW`) and the genuine "constants is undefined" mock path both degrade to plain `O_RDONLY` without throwing. Locally re-ran all six previously-failing files (199 tests) — all green. Existing 51 gitDiff tests unchanged. --- packages/core/src/utils/gitDiff.ts | 37 ++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index c12d3bfdf6a..9c9a0d230fb 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -5,7 +5,14 @@ */ import { execFile } from 'node:child_process'; -import { constants as fsConstants } from 'node:fs'; +// Namespace import (vs `import { constants }`) so vitest tests that +// `vi.mock('node:fs', ...)` without supplying every named export don't +// blow up in strict-mock mode just because they transitively load this +// file via `@qwen-code/qwen-code-core`. The `constants?.X ?? 0` accesses +// below absorb a missing `constants` field by falling through to plain +// `O_RDONLY` (= 0 on POSIX) — harmless in mock environments where no +// real `open()` ever runs. +import * as nodeFs from 'node:fs'; import { access, lstat, open, readFile, stat } from 'node:fs/promises'; import * as path from 'node:path'; import { promisify } from 'node:util'; @@ -61,13 +68,25 @@ const UNTRACKED_READ_CAP_BYTES = MAX_DIFF_SIZE_BYTES; const UNTRACKED_READ_CHUNK_BYTES = 64 * 1024; /** Scan the first N bytes for NUL to detect binary files (matches git's heuristic). */ const BINARY_SNIFF_BYTES = 8 * 1024; -/** Open flags for line counting. `O_NOFOLLOW` closes the TOCTOU window - * between the `lstat` symlink check and `open` — if the path is replaced - * with a symlink in that gap, `open` rejects with `ELOOP` instead of - * silently dereferencing it. Falls back to plain `O_RDONLY` on platforms - * that don't expose the flag (Windows constants omit `O_NOFOLLOW`). */ -const UNTRACKED_OPEN_FLAGS = - fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0); +/** Memoized open flags for line counting. `O_NOFOLLOW` closes the TOCTOU + * window between the `lstat` symlink check and `open` — if the path is + * replaced with a symlink in that gap, `open` rejects with `ELOOP` instead + * of silently dereferencing it. Falls back to plain `O_RDONLY` on platforms + * that don't expose the flag (Windows constants omit `O_NOFOLLOW`). + * + * Computed lazily on first call (rather than at module load) so test files + * that `vi.mock('node:fs', ...)` without supplying `constants` can still + * load this module transitively via `@qwen-code/qwen-code-core` without + * vitest's strict-mock proxy throwing on the property access. Tests that + * do not actually exercise `countUntrackedLines` never trigger the lookup. */ +let untrackedOpenFlagsCache: number | undefined; +function getUntrackedOpenFlags(): number { + if (untrackedOpenFlagsCache === undefined) { + untrackedOpenFlagsCache = + (nodeFs.constants?.O_RDONLY ?? 0) | (nodeFs.constants?.O_NOFOLLOW ?? 0); + } + return untrackedOpenFlagsCache; +} /** * Fetch numstat-based git diff stats (files changed, lines added/removed) and @@ -585,7 +604,7 @@ async function countUntrackedLines( } let fh; try { - fh = await open(absPath, UNTRACKED_OPEN_FLAGS); + fh = await open(absPath, getUntrackedOpenFlags()); } catch { // ELOOP from O_NOFOLLOW (path raced into a symlink between lstat and // open) and any other open error all collapse to a binary row so the From c1638efeaddf012398cdd857d18d8ec6f317e995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 17:20:40 +0800 Subject: [PATCH 19/23] fix(core): make resolveGitDir worktree assertion platform-agnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI was failing only on: resolveGitDir > follows the gitdir pointer for linked worktrees AssertionError: expected 'C:/Users/runneradmin/.../main/.git/worktrees/wt' to contain '.git\worktrees' Git writes the linked-worktree pointer in the `.git` *file* using forward slashes — `gitdir: C:/Users/.../main/.git/worktrees/wt` — even on Windows. `resolveGitDir` surfaces that string verbatim (intentional, since fs APIs on Windows accept both separators). But the assertion used `path.join('.git', 'worktrees')`, which is `'.git\\worktrees'` on Windows, so the substring-contains check failed despite the value being correct. Switch to a regex that matches either separator: `/[/\\]\.git[/\\]worktrees[/\\]/`. Now the assertion holds on POSIX (where path.join uses `/` anyway) and Windows (where git's value uses `/` but the host uses `\`). 6285/6289 Windows tests already passed before this; only this one assertion was platform-dependent. --- packages/core/src/utils/gitDiff.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 519fda008ed..8db40a84b78 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -583,7 +583,11 @@ describe('resolveGitDir', () => { const resolved = await resolveGitDir(wtPath); expect(resolved).not.toBeNull(); - expect(resolved).toContain(path.join('.git', 'worktrees')); + // Git writes the linked-worktree pointer with forward slashes even on + // Windows (`gitdir: C:/.../main/.git/worktrees/wt`), and we surface + // that string verbatim. Match either separator so the assertion is + // platform-independent. + expect(resolved).toMatch(/[/\\]\.git[/\\]worktrees[/\\]/); // Fake a merge-in-progress inside the linked worktree's gitdir and // confirm `fetchGitDiff` short-circuits, which would silently fail if From 632b25915540aed95551836a942d8808e0c8ba70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Tue, 28 Apr 2026 18:57:46 +0800 Subject: [PATCH 20/23] fix(core): C-unquote diff path headers and fix untracked-only fast path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Critical findings + one suggestion from wenshao on PR #3491: 1. (line 615) `extractFilePath` only accepted unquoted `+++ b/...` / `--- a/...` headers. Git wraps a path in `"..."` and applies C-style escaping (`\t`, `\n`, `\r`, `\"`, `\\`, plus octal `\NNN` for non-ASCII bytes) whenever the raw path contains a character that breaks space-delimited parsing. `core.quotepath=false` only disables the octal form for non-ASCII bytes — control chars and quotes are still escaped — so `fetchGitDiffHunks` silently dropped hunks for any tracked file whose name contained a tab, newline, or quote. Add `unquoteCStylePath()`: detects the surrounding quotes, decodes `\t`/`\n`/`\r`/`\"`/`\\` plus octal `\NNN` to raw bytes, then UTF-8-decodes the byte sequence so multi-byte octal sequences like `\346\226\207` (= `文`) round-trip correctly. `extractFilePath` pipes every candidate through `stripTab` -> `unquoteCStylePath` before checking the `a/` / `b/` prefix. Two unit tests cover the tab and octal cases; one integration test creates a real `tab\there.txt` tracked file, modifies it, and asserts `fetchGitDiffHunks` keys hunks under the real name. The integration test no-ops on filesystems that reject tab-in-name (NTFS). 2. (line 146) The >MAX_FILES_FOR_DETAILS fast path was guarded by `quickStats &&`, which short-circuited to false when shortstat returned an empty string. A workspace with 0 tracked changes plus 501 untracked files therefore slipped past the guardrail and ran the slow path, line-counting only the first MAX_FILES untracked files — header reported `filesCount: 501` but `linesAdded` missed the other 451. Treat empty/null/unparseable shortstat as `EMPTY_STATS` and apply the threshold on `tracked + untracked` uniformly. Integration test plants 501 untracked files + 0 tracked and asserts the result has `filesCount: 501` with an empty perFileStats Map (summary-only). 3. (line 263) `fetchGitDiffHunks` reads the full `git diff HEAD` stdout before parser caps apply. Documented in the JSDoc as a known limitation: streaming the parser to terminate git early at MAX_FILES is a reasonable follow-up but a non-trivial refactor (spawn + incremental parse + UTF-8 boundary handling) and out of scope for this PR. The existing `runGit` 64 MB maxBuffer keeps pathological cases from runaway-allocating. 55 gitDiff tests pass (51 + 4 new). --- packages/core/src/utils/gitDiff.test.ts | 105 ++++++++++++++++ packages/core/src/utils/gitDiff.ts | 156 ++++++++++++++++++++---- 2 files changed, 235 insertions(+), 26 deletions(-) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 8db40a84b78..97331827faf 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -406,6 +406,32 @@ describe('fetchGitDiffHunks', () => { ); }); + it('keys hunks by the real path for files with tabs in the name (C-quoted in diff output)', async () => { + // Real git output for a tracked file named `tab\there.txt` looks like + // `+++ "b/tab\there.txt"` even with `core.quotepath=false` — C-quoting + // for tabs/newlines/quotes is independent of that config. Without the + // unquote step in `extractFilePath`, fetchGitDiffHunks would silently + // drop the file's hunks. + const weirdName = 'tab\there.txt'; + try { + await fs.writeFile(path.join(repo, weirdName), 'x\n'); + } catch { + return; // Filesystem refused tab in name (e.g. Windows NTFS). + } + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await fs.writeFile(path.join(repo, weirdName), 'y\n'); + + const hunks = await fetchGitDiffHunks(repo); + expect([...hunks.keys()]).toEqual([weirdName]); + expect(hunks.get(weirdName)![0].lines.some((l) => l.startsWith('-x'))).toBe( + true, + ); + expect(hunks.get(weirdName)![0].lines.some((l) => l.startsWith('+y'))).toBe( + true, + ); + }); + it('keys hunks by the real path for files whose name contains " b/"', async () => { await fs.mkdir(path.join(repo, 'a b'), { recursive: true }); await fs.writeFile(path.join(repo, 'a b', 'c.txt'), 'x\n'); @@ -437,6 +463,41 @@ describe('fetchGitDiffHunks', () => { }); }); +describe('parseGitDiff C-quoted path support', () => { + it('decodes `+++ "b/..."` headers for files with tabs in the name', () => { + // Reproduces wenshao Critical (PR #3491 line 615): without C-quote + // decoding, `extractFilePath` rejects the quoted +++ line and the + // hunks are silently dropped. + const diff = `diff --git "a/tab\\there.txt" "b/tab\\there.txt" +index 1111111..2222222 100644 +--- "a/tab\\there.txt" ++++ "b/tab\\there.txt" +@@ -1 +1,2 @@ + a ++b +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual(['tab\there.txt']); + expect(result.get('tab\there.txt')![0].lines).toEqual([' a', '+b']); + }); + + it('decodes octal escapes in quoted paths (legacy quotepath=true output)', () => { + // Even with `core.quotepath=false` set on our git invocations, callers + // could feed us output produced by a different command. \346\226\207 + // is the UTF-8 byte sequence for `文`. + const diff = `diff --git "a/\\346\\226\\207.txt" "b/\\346\\226\\207.txt" +index 1111111..2222222 100644 +--- "a/\\346\\226\\207.txt" ++++ "b/\\346\\226\\207.txt" +@@ -1 +1 @@ +-x ++y +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual(['文.txt']); + }); +}); + describe('parseGitDiff path disambiguation', () => { it('keys hunks by the real path when the filename contains " b/"', () => { // `a b/c.txt` produces `diff --git a/a b/c.txt b/a b/c.txt`, which is @@ -821,6 +882,50 @@ describe('fetchGitDiff invocation from a subdirectory', () => { }); }); +describe('fetchGitDiff fast path with untracked-only workspaces', () => { + it('takes the >MAX_FILES_FOR_DETAILS short-circuit when shortstat is empty', async () => { + // Reproduces wenshao Critical (PR #3491 line 146). 0 tracked changes + // + many untracked previously left `quickStats` null, so the fast + // path was skipped and the slow path under-reported `linesAdded` + // because it line-counted only the first MAX_FILES untracked files. + // The fix makes the threshold fire on tracked + untracked even when + // shortstat returns nothing. + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-fp-')); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'seed'], { cwd: repo }); + + // Plant 501 untracked files (just over MAX_FILES_FOR_DETAILS = 500) + // and zero tracked changes. + const N = 501; + const writes: Array> = []; + for (let i = 0; i < N; i++) { + writes.push(fs.writeFile(path.join(repo, `u${i}.txt`), 'a\n')); + } + await Promise.all(writes); + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + // Header includes every untracked file in `filesCount`. + expect(result!.stats.filesCount).toBe(N); + // `perFileStats` is empty because we took the summary-only path, + // which is the whole point of the guardrail. + expect(result!.perFileStats.size).toBe(0); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); +}); + describe('fetchGitDiffHunks ignores external diff drivers', () => { it('does not invoke GIT_EXTERNAL_DIFF when reading hunks', async () => { // Reproduces wenshao Critical (PR #3491 line 219). Plain `git diff` diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 9c9a0d230fb..c5473c21cc9 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -60,6 +60,14 @@ export const MAX_DIFF_SIZE_BYTES = 1_000_000; export const MAX_LINES_PER_FILE = 400; /** Skip per-file parsing when the diff touches more than this many files. */ export const MAX_FILES_FOR_DETAILS = 500; +/** Sentinel used when `git diff --shortstat` returns nothing — most often + * because there are no tracked changes at all. The fast-path threshold + * is then driven entirely by the untracked count. */ +const EMPTY_STATS: GitDiffStats = { + filesCount: 0, + linesAdded: 0, + linesRemoved: 0, +}; /** How much of an untracked file to read when counting its lines. */ const UNTRACKED_READ_CAP_BYTES = MAX_DIFF_SIZE_BYTES; /** Per-file read buffer for line counting. With up to MAX_FILES (=50) files @@ -142,23 +150,24 @@ export async function fetchGitDiff(cwd: string): Promise { ]); const untrackedCount = countNulDelimited(untrackedOut); - if (shortstatOut != null) { - const quickStats = parseShortstat(shortstatOut); - // Base the short-circuit on tracked + untracked so repos with relatively - // few edits but a flood of untracked files (e.g. large generated - // workspaces) still take the summary-only path. - if ( - quickStats && - quickStats.filesCount + untrackedCount > MAX_FILES_FOR_DETAILS - ) { - return { - stats: { - ...quickStats, - filesCount: quickStats.filesCount + untrackedCount, - }, - perFileStats: new Map(), - }; - } + // Apply the >500-file fast path on tracked + untracked, treating "no + // shortstat output" (no tracked changes) and "shortstat unparseable" + // both as zero tracked stats. Without this fall-through, a workspace + // with 0 tracked + 501 untracked files would slip past the guardrail: + // shortstat would be empty, parseShortstat would return null, and the + // slow path would only line-count the first MAX_FILES untracked + // entries — leaving `filesCount: 501` paired with a `linesAdded` that + // missed the other 451 files. + const quickStats = + (shortstatOut != null && parseShortstat(shortstatOut)) || EMPTY_STATS; + if (quickStats.filesCount + untrackedCount > MAX_FILES_FOR_DETAILS) { + return { + stats: { + ...quickStats, + filesCount: quickStats.filesCount + untrackedCount, + }, + perFileStats: new Map(), + }; } // Numstat gives us +/- counts; name-status tells us *why* a row exists @@ -243,8 +252,16 @@ export async function fetchGitDiff(cwd: string): Promise { } /** - * Fetch structured hunks for the current working tree vs HEAD. Separate from - * `fetchGitDiff` so callers that only need stats do not pay the full diff cost. + * Fetch structured hunks for the current working tree vs HEAD. Separate + * from `fetchGitDiff` so callers that only need stats do not pay the full + * diff cost. + * + * NOTE on memory: this reads the full `git diff HEAD` stdout via `execFile` + * before applying parser caps (`MAX_FILES`, `MAX_DIFF_SIZE_BYTES`, + * `MAX_LINES_PER_FILE`). For very large diffs we can buffer up to the + * `runGit` `maxBuffer` (64 MB) before dropping content. Streaming the + * parser would let us terminate `git` early at `MAX_FILES`; that's a + * reasonable follow-up but out of scope for this utility's first cut. */ export async function fetchGitDiffHunks( cwd: string, @@ -445,6 +462,86 @@ export function parseGitDiff(stdout: string): Map { return result; } +/** + * Decode a path field from a `diff --git` header — handles both unquoted + * (`b/foo.txt`) and C-style quoted (`"b/tab\there.txt"`) forms. + * + * Git wraps a path in `"..."` and applies C-style escaping (`\t`, `\n`, + * `\r`, `\"`, `\\`, plus octal `\NNN` for non-ASCII bytes) whenever the + * raw path contains a character that breaks the simple space-delimited + * format. `core.quotepath=false` disables ONLY the octal escaping for + * non-ASCII bytes; control chars and quotes are still escaped, so we + * must decode them ourselves to preserve the real on-disk filename. + * + * Octal escapes are decoded as raw byte values then UTF-8-decoded en + * masse so multi-byte sequences like `\346\226\207` (文) round-trip + * correctly even though we never set quotepath=true ourselves. + */ +function unquoteCStylePath(s: string): string { + if (!s.startsWith('"') || !s.endsWith('"') || s.length < 2) return s; + const inner = s.slice(1, -1); + // Build raw bytes first so octal `\NNN` sequences (each one byte of a + // potentially multi-byte UTF-8 character) reassemble correctly. + const bytes: number[] = []; + let i = 0; + while (i < inner.length) { + const c = inner.charCodeAt(i); + if (c !== 0x5c /* '\' */) { + // Encode this character (which may be a multi-byte UTF-8 char from + // the input string) into bytes. JS strings are UTF-16; use a helper. + bytes.push(...Buffer.from(inner[i] ?? '', 'utf8')); + i++; + continue; + } + const next = inner[i + 1]; + if (next === undefined) { + bytes.push(0x5c); + i++; + continue; + } + switch (next) { + case 't': + bytes.push(0x09); + i += 2; + break; + case 'n': + bytes.push(0x0a); + i += 2; + break; + case 'r': + bytes.push(0x0d); + i += 2; + break; + case '"': + bytes.push(0x22); + i += 2; + break; + case '\\': + bytes.push(0x5c); + i += 2; + break; + default: + if (next >= '0' && next <= '7') { + let octal = ''; + while ( + octal.length < 3 && + i + 1 + octal.length < inner.length && + (inner[i + 1 + octal.length] ?? '') >= '0' && + (inner[i + 1 + octal.length] ?? '') <= '7' + ) { + octal += inner[i + 1 + octal.length]; + } + bytes.push(parseInt(octal, 8) & 0xff); + i += 1 + octal.length; + } else { + bytes.push(...Buffer.from(next, 'utf8')); + i += 2; + } + } + } + return Buffer.from(bytes).toString('utf8'); +} + /** * Extract the real filename from a `diff --git` file block, avoiding the * ambiguity of `diff --git a/X b/Y` when `X` itself contains ` b/`. @@ -456,8 +553,12 @@ export function parseGitDiff(stdout: string): Map { * to `--- a/` for the old name. * 3. `--- a/` alone — for the rare case where `+++` is absent. * - * Git appends a TAB after the path on `---` / `+++` / `rename to` lines when - * the path contains whitespace; `stripTab` cuts at that marker. + * Each candidate path goes through `stripTab` (cut at the trailing TAB git + * appends after whitespace-containing paths) and `unquoteCStylePath` + * (decode `"..."` C-quoted form for paths whose raw bytes include tabs, + * newlines, quotes, or non-ASCII characters that core.quotepath does not + * suppress). Without the unquote step, fetchGitDiffHunks would silently + * drop hunks for any tracked file whose name contains those characters. * * Returns `null` when the block has no hunks or no recognizable path line * (mode-only changes, for example). @@ -478,20 +579,23 @@ function extractFilePath(lines: string[]): string | null { const t = s.indexOf('\t'); return t >= 0 ? s.slice(0, t) : s; }; - if (renameTo !== null) return stripTab(renameTo); - if (copyTo !== null) return stripTab(copyTo); + // Strip the TAB-end-of-path marker first, then C-unquote — git emits the + // TAB AFTER the closing quote on quoted paths. + const normalize = (s: string): string => unquoteCStylePath(stripTab(s)); + if (renameTo !== null) return normalize(renameTo); + if (copyTo !== null) return normalize(copyTo); if (plus !== null) { - const p = stripTab(plus); + const p = normalize(plus); if (p !== '/dev/null' && p.startsWith('b/')) return p.slice(2); // Deleted file — fall back to the old path. if (minus !== null) { - const m = stripTab(minus); + const m = normalize(minus); if (m !== '/dev/null' && m.startsWith('a/')) return m.slice(2); } return null; } if (minus !== null) { - const m = stripTab(minus); + const m = normalize(minus); if (m !== '/dev/null' && m.startsWith('a/')) return m.slice(2); } return null; From 4b371a4070c1cffd2742d59cb5fe0b7b7af3a634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Wed, 29 Apr 2026 10:56:09 +0800 Subject: [PATCH 21/23] fix(core): also pass --no-textconv to block .gitattributes textconv drivers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on the earlier `--no-ext-diff` hardening. wenshao pointed out that `--no-ext-diff` covers `GIT_EXTERNAL_DIFF` and `diff..command`, but it does NOT block textconv filters registered via `.gitattributes` + `diff..textconv` — those run on a separate code path inside `git diff`. Verified locally: git config diff.evil.textconv /tmp/evil.sh echo '*.pdf diff=evil' > .gitattributes # ... commit + modify doc.pdf ... git diff HEAD --no-ext-diff -> /tmp/evil.sh fires git diff HEAD --no-ext-diff --no-textconv -> driver does NOT fire Add `--no-textconv` to all four `git diff` invocations (shortstat / numstat / name-status / plain hunks). As with `--no-ext-diff`, only the plain-diff call (`fetchGitDiffHunks`) is known to invoke textconv in current git, but pinning both flags uniformly is defense-in-depth and keeps the policy declarative. Regression test plants a real textconv driver in a worktree's `.git/config` + `.gitattributes` and asserts the driver's sentinel file is NOT written when `fetchGitDiffHunks` runs. Without the new flag the test fails with the sentinel present. --- packages/core/src/utils/gitDiff.test.ts | 60 +++++++++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 42 +++++++++++------ 2 files changed, 88 insertions(+), 14 deletions(-) diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 97331827faf..28cfccc9bf5 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -985,6 +985,66 @@ describe('fetchGitDiffHunks ignores external diff drivers', () => { await fs.rm(sentinel, { force: true }); } }); + + it('does not invoke textconv drivers when reading hunks', async () => { + // Reproduces wenshao Critical (PR #3491 line 282). `--no-ext-diff` + // blocks GIT_EXTERNAL_DIFF / diff..command but is INDEPENDENT + // of textconv filters configured via .gitattributes + + // `diff..textconv`. Without `--no-textconv`, a malicious + // worktree can still execute commands when this utility runs. + const repo = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-gitdiff-tc-')); + const sentinel = path.join(os.tmpdir(), `qwen-tc-fired-${Date.now()}`); + const driverScript = path.join(repo, 'evil-textconv.sh'); + try { + await execFileAsync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + await execFileAsync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + await execFileAsync('git', ['config', 'user.name', 'T'], { cwd: repo }); + await execFileAsync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + + // Driver writes the sentinel as a side effect when invoked. + await fs.writeFile( + driverScript, + `#!/bin/sh\necho fired > "${sentinel}"\ncat "$1" 2>/dev/null\n`, + { mode: 0o755 }, + ); + + // Wire the driver up: register textconv command + attribute. + await execFileAsync( + 'git', + ['config', 'diff.evil.textconv', driverScript], + { + cwd: repo, + }, + ); + await fs.writeFile( + path.join(repo, '.gitattributes'), + '*.pdf diff=evil\n', + ); + await fs.writeFile(path.join(repo, 'doc.pdf'), 'a\n'); + await execFileAsync('git', ['add', '.'], { cwd: repo }); + await execFileAsync('git', ['commit', '-q', '-m', 'init'], { cwd: repo }); + await fs.writeFile(path.join(repo, 'doc.pdf'), 'b\n'); + + const hunks = await fetchGitDiffHunks(repo); + expect(hunks.get('doc.pdf')).toBeDefined(); + + let driverFired = false; + try { + await fs.stat(sentinel); + driverFired = true; + } catch { + // ENOENT — driver never ran. Expected. + } + expect(driverFired).toBe(false); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + await fs.rm(sentinel, { force: true }); + } + }); }); describe('fetchGitDiff deletion detection', () => { diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index c5473c21cc9..081010a89ec 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -125,16 +125,27 @@ export async function fetchGitDiff(cwd: string): Promise { // numstat cost. For untracked we hold the raw stdout rather than the parsed // list so the fast path only has to count NUL bytes instead of allocating // a full path array. - // Every `git diff` invocation passes `--no-ext-diff` so a malicious - // repo-local or user-global config that registers an external diff driver - // (`GIT_EXTERNAL_DIFF` or `diff..command`) can never run when /diff - // touches the worktree. In practice the stats variants (`--shortstat`, - // `--numstat`, `--name-status`) do not invoke external drivers, but - // pinning the flag everywhere is defense-in-depth — git's behavior - // around external drivers has shifted between versions before. + // Every `git diff` invocation passes both `--no-ext-diff` AND + // `--no-textconv` so the worktree's config can never run user-supplied + // commands while /diff is only inspecting changes. The two flags cover + // independent attack surfaces: `--no-ext-diff` blocks `GIT_EXTERNAL_DIFF` + // and `diff..command`, while `--no-textconv` blocks the textconv + // filter that .gitattributes + `diff..textconv` register (e.g. + // `pdftotext` to render PDFs). In practice the stats variants + // (`--shortstat`, `--numstat`, `--name-status`) do not invoke either + // mechanism, but pinning both flags everywhere is defense-in-depth — + // git's behavior around these drivers has shifted between versions + // before. const [shortstatOut, untrackedOut] = await Promise.all([ runGit( - ['--no-optional-locks', 'diff', '--no-ext-diff', 'HEAD', '--shortstat'], + [ + '--no-optional-locks', + 'diff', + '--no-ext-diff', + '--no-textconv', + 'HEAD', + '--shortstat', + ], gitRoot, ), runGit( @@ -180,6 +191,7 @@ export async function fetchGitDiff(cwd: string): Promise { '--no-optional-locks', 'diff', '--no-ext-diff', + '--no-textconv', 'HEAD', '--numstat', '-z', @@ -191,6 +203,7 @@ export async function fetchGitDiff(cwd: string): Promise { '--no-optional-locks', 'diff', '--no-ext-diff', + '--no-textconv', 'HEAD', '--name-status', '-z', @@ -273,13 +286,14 @@ export async function fetchGitDiffHunks( if (!gitRoot) return new Map(); if (await isInTransientGitState(gitRoot)) return new Map(); - // `--no-ext-diff` is critical here: without it, a repo-local or user- - // global config that sets `GIT_EXTERNAL_DIFF` or `diff..command` - // would let `git diff` execute arbitrary commands when this read-only - // utility is invoked. The stats variants in `fetchGitDiff` already - // bypass external drivers, but plain `git diff` honors them. + // Plain `git diff` honors both `GIT_EXTERNAL_DIFF` / `diff..command` + // (blocked by `--no-ext-diff`) AND .gitattributes-driven textconv filters + // like `diff..textconv` (blocked by `--no-textconv`) — independent + // command-execution surfaces, both of which we have to disable on this + // read-only utility. The stats variants in `fetchGitDiff` already bypass + // both, but plain diff fires both unless told not to. const diffOut = await runGit( - ['--no-optional-locks', 'diff', '--no-ext-diff', 'HEAD'], + ['--no-optional-locks', 'diff', '--no-ext-diff', '--no-textconv', 'HEAD'], gitRoot, ); if (diffOut == null) return new Map(); From ecb7743cc2afd7cbe6f564c3be7fdec2185152fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Wed, 29 Apr 2026 14:52:36 +0800 Subject: [PATCH 22/23] fix(diff): close untracked-line undercount and ANSI injection in /diff output Two Critical issues from PR #3491 review: 1. fetchGitDiff slow path only line-counted the first MAX_FILES (50) untracked paths via `untrackedPaths.slice(0, MAX_FILES)`. With 51-500 untracked files in a clean tree the header reported the full file count but only ~50 files' worth of additions, materially under-reporting the total. Now read every untracked path that survived the >MAX_FILES_FOR_DETAILS fast-path filter, with concurrency bounded to MAX_FILES so peak heap stays around MAX_FILES * UNTRACKED_READ_CHUNK_BYTES (~3.2 MB) regardless of input size. 2. renderDiffModelText interpolated raw filenames into the non-interactive / ACP text path. The interactive history is sanitized via escapeAnsiCtrlCodes(item) inside HistoryItemDisplay, but the text path streams to stdout / logs / transports with no equivalent hop, so a tracked or untracked filename containing \x1b[2J etc. could inject color resets, cursor moves, or full screen clears into CI logs and downstream terminals. Pipe r.filename through escapeAnsiCtrlCodes at the rendering boundary on every row variant (binary, untracked, deleted, modified). Tests: - gitDiff.test.ts: regression that asserts every one of MAX_FILES + 10 untracked one-line files contributes to stats.linesAdded (would be 50 pre-fix vs 60 actual). - diffCommand.test.ts: two new specs covering ANSI escapes in modified-file rows and in binary / untracked / deleted suffix rows. Verifies raw \x1b never reaches stdout while suffix markers ((binary), (new), (deleted)) still render. --- .../cli/src/ui/commands/diffCommand.test.ts | 63 ++++++++++++++++++ packages/cli/src/ui/commands/diffCommand.ts | 13 +++- packages/core/src/utils/gitDiff.test.ts | 26 ++++++++ packages/core/src/utils/gitDiff.ts | 65 ++++++++++++++----- 4 files changed, 150 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index c4f8b1075a7..a9950d82ff7 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -464,3 +464,66 @@ describe('diffCommand registration', () => { ]); }); }); + +describe('renderDiffModelText filename sanitization', () => { + // Regression for the non-interactive ANSI-injection vector: the + // interactive path runs the full HistoryItem through + // `escapeAnsiCtrlCodes(item)` in `HistoryItemDisplay`, but text output + // (non-interactive / ACP) was streaming `r.filename` straight into + // stdout / logs / transports without that hop. A hostile filename like + // `evil\x1b[31m.txt` could therefore inject color resets, cursor moves, + // or full screen clears into CI logs. The renderer now pipes filenames + // through `escapeAnsiCtrlCodes` at the text boundary. + let mockFetchGitDiff: Mock; + beforeEach(() => { + vi.clearAllMocks(); + mockFetchGitDiff = vi.mocked(fetchGitDiff); + }); + + async function renderText(perFileStats: Map) { + if (!diffCommand.action) throw new Error('Command has no action'); + mockFetchGitDiff.mockResolvedValue({ + stats: { filesCount: 1, linesAdded: 1, linesRemoved: 0 }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + perFileStats: perFileStats as any, + } satisfies GitDiffResult); + const result = await diffCommand.action(makeContextWithCwd(), ''); + return (result as { content: string }).content; + } + + it('escapes raw ANSI escape sequences embedded in tracked filenames', async () => { + const evil = 'safe\x1b[31mEVIL\x1b[0m.txt'; + const content = await renderText( + new Map([[evil, { added: 1, removed: 0, isBinary: false }]]), + ); + // The raw ESC byte must not survive into the text output — it would + // otherwise be interpreted as an SGR by any downstream terminal. + expect(content).not.toContain('\x1b['); + // The literal escaped form is what `escapeAnsiCtrlCodes` produces. + expect(content).toContain('\\u001b[31m'); + }); + + it('escapes ANSI sequences in untracked / binary / deleted suffix rows', async () => { + const evilBinary = 'img\x1b[2J.png'; + const evilUntracked = 'note\x1b[H.md'; + const evilDeleted = 'gone\x1b[0K.txt'; + const content = await renderText( + new Map([ + [evilBinary, { added: 0, removed: 0, isBinary: true }], + [ + evilUntracked, + { added: 1, removed: 0, isBinary: false, isUntracked: true }, + ], + [ + evilDeleted, + { added: 0, removed: 1, isBinary: false, isDeleted: true }, + ], + ]), + ); + expect(content).not.toContain('\x1b['); + // All three suffix branches still render their markers. + expect(content).toContain('(binary)'); + expect(content).toContain('(new)'); + expect(content).toContain('(deleted)'); + }); +}); diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index 78c20ca4898..e29eb590bcd 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -22,6 +22,7 @@ import { type DiffRenderRow, type HistoryItemDiffStats, } from '../types.js'; +import { escapeAnsiCtrlCodes } from '../utils/textUtils.js'; async function diffAction( context: CommandContext, @@ -212,13 +213,21 @@ function formatRowsText(rows: DiffRenderRow[]): string[] { const out: string[] = []; for (const r of rows) { + // Escape ANSI / control-byte sequences in the filename. Git permits + // bytes like `\x1b` in tracked / untracked paths, and the + // non-interactive (and ACP) text path streams straight to stdout / logs + // / transports without going through the interactive history's + // `escapeAnsiCtrlCodes(item)` sanitizer in `HistoryItemDisplay`. Without + // this hop, a hostile filename could inject color resets, cursor moves, + // or full screen clears into CI logs and any consumer's terminal. + const safeName = escapeAnsiCtrlCodes(r.filename); if (r.isBinary) { const suffix = r.isUntracked ? ` ${t('(binary, new)')}` : r.isDeleted ? ` ${t('(binary, deleted)')}` : ` ${t('(binary)')}`; - out.push(` ${padMarker('~', statColumnWidth)} ${r.filename}${suffix}`); + out.push(` ${padMarker('~', statColumnWidth)} ${safeName}${suffix}`); continue; } const added = `+${String(r.added ?? 0).padStart(addWidth)}`; @@ -229,7 +238,7 @@ function formatRowsText(rows: DiffRenderRow[]): string[] { } else if (r.isDeleted) { suffix = ` ${t('(deleted)')}`; } - out.push(` ${added} ${removed} ${r.filename}${suffix}`); + out.push(` ${added} ${removed} ${safeName}${suffix}`); } return out; } diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 28cfccc9bf5..199a4692a4d 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -1216,4 +1216,30 @@ describe('fetchGitDiff untracked counting', () => { // Per-file map is still capped at MAX_FILES. expect(result!.perFileStats.size).toBe(MAX_FILES); }); + + it('line-counts every untracked file in the slow path, not just the first MAX_FILES', async () => { + // Regression for the under-counted-totals bug: with 0 tracked changes + // and 51-500 untracked files, the slow path used to read line counts + // for `untrackedPaths.slice(0, MAX_FILES)` only, so files beyond the + // per-file display cap silently dropped out of `stats.linesAdded`. + // This test puts MAX_FILES + 10 = 60 untracked one-line files in a + // clean repo and asserts every one of them contributes to the total. + const extra = 10; + const totalUntracked = MAX_FILES + extra; + for (let i = 0; i < totalUntracked; i++) { + // Padded filenames so `ls-files --others` returns them in stable + // order — otherwise the "first MAX_FILES" slicing in the bug case + // could randomly cover the test files. + const name = `u${String(i).padStart(3, '0')}.txt`; + await fs.writeFile(path.join(repo, name), 'one-line\n'); + } + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.stats.filesCount).toBe(totalUntracked); + // Every untracked file is one line, so totals must equal totalUntracked. + // Pre-fix this would have been MAX_FILES (= 50). + expect(result!.stats.linesAdded).toBe(totalUntracked); + // Visible per-file rows still cap at MAX_FILES. + expect(result!.perFileStats.size).toBe(MAX_FILES); + }); }); diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 081010a89ec..c5196a35835 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -228,25 +228,31 @@ export async function fetchGitDiff(cwd: string): Promise { // changes already fill the `MAX_FILES` slot. stats.filesCount += untrackedCount; const untrackedPaths = splitNulDelimited(untrackedOut); - // Keep line-counting decoupled from per-file rendering. We read up to - // `MAX_FILES` untracked files for their line counts (bounds worst-case - // I/O at ~50 MB) and fold all of them into `linesAdded`, even if only - // `remainingSlots` of them end up as visible rows. That avoids the - // header silently under-reporting additions when tracked changes have - // already filled the per-file map. - const countable = untrackedPaths.slice(0, MAX_FILES); - const countableStats = await Promise.all( - countable.map((relPath) => - countUntrackedLines(path.join(gitRoot, relPath)), - ), + // Read line counts for *every* untracked path that survived the + // `>MAX_FILES_FOR_DETAILS` fast-path filter (so up to ~500 files at the + // outer cap, not just the first MAX_FILES). Otherwise a workspace with + // 51-500 untracked files would surface in the header as e.g. "60 files + // changed, +50 lines" — the +50 only covering the first 50 files, + // bypassing the contributions of the remaining 10. Concurrency is + // bounded to MAX_FILES so peak heap stays around + // `MAX_FILES * UNTRACKED_READ_CHUNK_BYTES` (~3.2 MB) regardless of how + // many untracked files are in the slow-path window. + const lineStats = await mapWithConcurrency( + untrackedPaths, + MAX_FILES, + (relPath) => countUntrackedLines(path.join(gitRoot, relPath)), ); - for (const s of countableStats) stats.linesAdded += s.added; + for (const s of lineStats) stats.linesAdded += s.added; + // Per-file rendering still caps at MAX_FILES — only the first + // `remainingSlots` untracked entries become visible rows. The rest are + // already folded into `linesAdded` above and into `filesCount`, so + // `hiddenCount` covers them faithfully on the renderer side. const remainingSlots = Math.max(0, MAX_FILES - perFileStats.size); - const visibleCount = Math.min(remainingSlots, countable.length); + const visibleCount = Math.min(remainingSlots, untrackedPaths.length); for (let i = 0; i < visibleCount; i++) { - const relPath = countable[i] ?? ''; - const u = countableStats[i] ?? { + const relPath = untrackedPaths[i] ?? ''; + const u = lineStats[i] ?? { added: 0, isBinary: false, truncated: false, @@ -852,6 +858,35 @@ async function isInTransientGitState(gitRoot: string): Promise { return results.some(Boolean); } +/** + * Run an async mapper over `items` with at most `limit` operations in + * flight at once. Used for untracked-file line counting so a workspace with + * a few hundred untracked files doesn't open 500 file descriptors in + * parallel — peak heap stays at `limit * UNTRACKED_READ_CHUNK_BYTES` + * regardless of `items.length`. + * + * Order-preserving: `results[i]` corresponds to `items[i]`. Failures + * propagate as thrown errors (`countUntrackedLines` already swallows its + * own I/O errors, so callers here see no rejections in practice). + */ +async function mapWithConcurrency( + items: readonly T[], + limit: number, + fn: (item: T) => Promise, +): Promise { + if (items.length === 0) return []; + const effective = Math.max(1, limit); + const results: R[] = new Array(items.length); + for (let i = 0; i < items.length; i += effective) { + const slice = items.slice(i, i + effective); + const batch = await Promise.all(slice.map((item) => fn(item))); + for (let j = 0; j < batch.length; j++) { + results[i + j] = batch[j] as R; + } + } + return results; +} + async function runGit(args: string[], cwd: string): Promise { // `core.quotepath=false` keeps non-ASCII filenames as UTF-8 in git's output // instead of octal-escaping them (`\346\226\207.txt`), which would otherwise From 4f7e8dde567df3d0c57c3cfff226d259255f6333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Thu, 7 May 2026 14:38:29 +0800 Subject: [PATCH 23/23] fix(diff): harden quoted-path decoder and filename sanitizer - unquoteCStylePath now walks Unicode code points so non-BMP characters (e.g. emoji) inside a forced-quoted path no longer get split into lone surrogates and decoded as replacement characters. - Add explicit C-escape mappings for \a, \b, \f, \v so paths using those control bytes decode to BEL/BS/FF/VT instead of dropping the backslash. - Replace escapeAnsiCtrlCodes(filename) at the /diff text-rendering boundary with a sanitizer that also escapes standalone C0/C1 control bytes plus DEL, closing newline / CR / BS / BEL injection vectors that ansi-regex does not match. --- .../cli/src/ui/commands/diffCommand.test.ts | 30 +++++++++ packages/cli/src/ui/commands/diffCommand.ts | 54 +++++++++++++--- packages/core/src/utils/gitDiff.test.ts | 61 +++++++++++++++++++ packages/core/src/utils/gitDiff.ts | 33 ++++++++-- 4 files changed, 165 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index a9950d82ff7..a2ab127f3b8 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -526,4 +526,34 @@ describe('renderDiffModelText filename sanitization', () => { expect(content).toContain('(new)'); expect(content).toContain('(deleted)'); }); + + it('escapes standalone control bytes that ansi-regex does not match', async () => { + // Filenames git permits but that aren't part of an ANSI escape sequence: + // raw newline, carriage return, backspace, BEL, and DEL. Each one would + // otherwise reorder, overwrite, or beep its way through the rendered + // diff in the non-interactive / ACP path. + const newline = 'bad\nINJECTED.txt'; + const cr = 'bad\roverwrite.txt'; + const bs = 'noisy\x08\x08\x08gone.txt'; + const bel = 'beep\x07.txt'; + const del = 'tail\x7f.txt'; + const content = await renderText( + new Map([ + [newline, { added: 1, removed: 0, isBinary: false }], + [cr, { added: 1, removed: 0, isBinary: false }], + [bs, { added: 1, removed: 0, isBinary: false }], + [bel, { added: 1, removed: 0, isBinary: false }], + [del, { added: 1, removed: 0, isBinary: false }], + ]), + ); + // None of the raw control bytes should survive into the rendered text. + expect(content).not.toContain('\n\nINJECTED.txt'); + expect(content).not.toMatch(/\roverwrite\.txt/); + expect(content).not.toContain('\x08'); + expect(content).not.toContain('\x07'); + expect(content).not.toContain('\x7f'); + // The escaped forms (JSON-style) are what we render instead. + expect(content).toContain('bad\\nINJECTED.txt'); + expect(content).toContain('bad\\roverwrite.txt'); + }); }); diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index e29eb590bcd..8607ec324b3 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -207,20 +207,58 @@ export function renderDiffModelText(model: DiffRenderModel): string { return lines.length > 0 ? `${header}\n${lines.join('\n')}${capNote}` : header; } +// Match standalone C0 controls (incl. TAB/CR/LF/BEL/BS), DEL, and C1 controls. +// `escapeAnsiCtrlCodes` only neutralizes multi-byte ANSI sequences, so a +// filename like `bad\nINJECTED.txt` or `bad\roverwrite.txt` would otherwise +// still break layout in the non-interactive / ACP rendering path. +// eslint-disable-next-line no-control-regex +const FILENAME_CONTROL_CHARS_REGEX = /[\x00-\x1f\x7f-\x9f]/g; + +function escapeControlChar(ch: string): string { + switch (ch) { + case '\b': + return '\\b'; + case '\t': + return '\\t'; + case '\n': + return '\\n'; + case '\f': + return '\\f'; + case '\r': + return '\\r'; + default: { + // JSON.stringify only escapes 0x00-0x1F (and `"` / `\`); DEL (0x7F) and + // C1 controls (0x80-0x9F) are returned as raw bytes, which is exactly + // what we are trying to keep out of the rendered output. Hand-roll the + // \uXXXX escape so every matched code point becomes printable. + const code = ch.charCodeAt(0); + return `\\u${code.toString(16).padStart(4, '0')}`; + } + } +} + +function sanitizeFilenameForDisplay(name: string): string { + return escapeAnsiCtrlCodes(name).replace( + FILENAME_CONTROL_CHARS_REGEX, + escapeControlChar, + ); +} + function formatRowsText(rows: DiffRenderRow[]): string[] { if (rows.length === 0) return []; const { addWidth, remWidth, statColumnWidth } = computeDiffColumnWidths(rows); const out: string[] = []; for (const r of rows) { - // Escape ANSI / control-byte sequences in the filename. Git permits - // bytes like `\x1b` in tracked / untracked paths, and the - // non-interactive (and ACP) text path streams straight to stdout / logs - // / transports without going through the interactive history's - // `escapeAnsiCtrlCodes(item)` sanitizer in `HistoryItemDisplay`. Without - // this hop, a hostile filename could inject color resets, cursor moves, - // or full screen clears into CI logs and any consumer's terminal. - const safeName = escapeAnsiCtrlCodes(r.filename); + // Escape ANSI sequences AND standalone control bytes in the filename. Git + // permits raw bytes like `\x1b`, `\n`, `\r`, BEL, BS in tracked / untracked + // paths, and the non-interactive (and ACP) text path streams straight to + // stdout / logs / transports without going through the interactive + // history's `escapeAnsiCtrlCodes(item)` sanitizer in `HistoryItemDisplay`. + // Without this hop, a hostile filename could inject color resets, cursor + // moves, full screen clears, or layout-breaking newlines into CI logs and + // any consumer's terminal. + const safeName = sanitizeFilenameForDisplay(r.filename); if (r.isBinary) { const suffix = r.isUntracked ? ` ${t('(binary, new)')}` diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index 199a4692a4d..e21bd0ac719 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -496,6 +496,67 @@ index 1111111..2222222 100644 const result = parseGitDiff(diff); expect([...result.keys()]).toEqual(['文.txt']); }); + + it('preserves non-BMP code points in quoted paths instead of splitting surrogates', () => { + // Reproduces wenshao Critical (PR #3491 line 504): the previous walker + // advanced one UTF-16 code unit at a time, so a non-BMP codepoint such + // as the rocket emoji 🚀 (U+1F680) coexisting with a forced-quoting byte + // (here a TAB) was decoded as two lone surrogates → two replacement + // characters, corrupting the hunk key. + const diff = `diff --git "a/\\t🚀.txt" "b/\\t🚀.txt" +index 1111111..2222222 100644 +--- "a/\\t🚀.txt" ++++ "b/\\t🚀.txt" +@@ -1 +1 @@ +-x ++y +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual(['\t🚀.txt']); + }); + + it('decodes the remaining C-style escapes (\\a, \\b, \\f, \\v)', () => { + // Reproduces wenshao Critical (PR #3491 line 552): the previous switch + // dropped the leading backslash for these escapes, turning `\a` / `\b` + // / `\f` / `\v` into ordinary `a` / `b` / `f` / `v` and yielding a + // hunk key that did not match the real on-disk filename. + const diff = `diff --git "a/bell\\afile.txt" "b/bell\\afile.txt" +index 1111111..2222222 100644 +--- "a/bell\\afile.txt" ++++ "b/bell\\afile.txt" +@@ -1 +1 @@ +-x ++y +diff --git "a/back\\bspace.txt" "b/back\\bspace.txt" +index 3333333..4444444 100644 +--- "a/back\\bspace.txt" ++++ "b/back\\bspace.txt" +@@ -1 +1 @@ +-x ++y +diff --git "a/form\\ffeed.txt" "b/form\\ffeed.txt" +index 5555555..6666666 100644 +--- "a/form\\ffeed.txt" ++++ "b/form\\ffeed.txt" +@@ -1 +1 @@ +-x ++y +diff --git "a/vert\\vtab.txt" "b/vert\\vtab.txt" +index 7777777..8888888 100644 +--- "a/vert\\vtab.txt" ++++ "b/vert\\vtab.txt" +@@ -1 +1 @@ +-x ++y +`; + const result = parseGitDiff(diff); + expect([...result.keys()]).toEqual([ + 'bell\x07file.txt', + 'back\x08space.txt', + 'form\x0cfeed.txt', + 'vert\x0btab.txt', + ]); + }); }); describe('parseGitDiff path disambiguation', () => { diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index c5196a35835..f6b9288987e 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -501,16 +501,23 @@ function unquoteCStylePath(s: string): string { if (!s.startsWith('"') || !s.endsWith('"') || s.length < 2) return s; const inner = s.slice(1, -1); // Build raw bytes first so octal `\NNN` sequences (each one byte of a - // potentially multi-byte UTF-8 character) reassemble correctly. + // potentially multi-byte UTF-8 character) reassemble correctly. We walk by + // Unicode code points (not UTF-16 code units), so non-BMP characters such as + // emoji that may appear inside a quoted path under `core.quotepath=false` + // round-trip through UTF-8 instead of being split into lone surrogates. const bytes: number[] = []; let i = 0; while (i < inner.length) { const c = inner.charCodeAt(i); if (c !== 0x5c /* '\' */) { - // Encode this character (which may be a multi-byte UTF-8 char from - // the input string) into bytes. JS strings are UTF-16; use a helper. - bytes.push(...Buffer.from(inner[i] ?? '', 'utf8')); - i++; + const cp = inner.codePointAt(i); + if (cp === undefined) { + i++; + continue; + } + const ch = String.fromCodePoint(cp); + bytes.push(...Buffer.from(ch, 'utf8')); + i += ch.length; continue; } const next = inner[i + 1]; @@ -520,6 +527,22 @@ function unquoteCStylePath(s: string): string { continue; } switch (next) { + case 'a': + bytes.push(0x07); + i += 2; + break; + case 'b': + bytes.push(0x08); + i += 2; + break; + case 'f': + bytes.push(0x0c); + i += 2; + break; + case 'v': + bytes.push(0x0b); + i += 2; + break; case 't': bytes.push(0x09); i += 2;