From 7b5b43563ac101295e6d0316f39aad5da2f72eb7 Mon Sep 17 00:00:00 2001 From: Rafael-Silva-Oliveira Date: Wed, 12 Aug 2026 13:28:28 +0200 Subject: [PATCH 1/3] feat(git): add commit context collector Adds `getCommitContext()`, which gathers the changes a commit message should describe. Part 1 of 4 for AI commit-message generation; nothing consumes it yet. Every command runs through `execFile` with an argument array, so no path is ever interpolated into a shell string, and both listings are read NUL-delimited: `git diff --cached --name-status -z` for the index and `git status --porcelain=v1 -z --untracked-files=all` for the working tree. Their rename records disagree on field order - the diff form emits the original path first, porcelain the new one - so each has its own parser. Copy records carry two paths as well and appear whenever `diff.renames = copies` is configured, so they are consumed correctly even though copy detection is never requested; reading one path where there are two would shift every later record onto the wrong file. The result is a typed `CommitContextResult` rather than a string. Failures that are expected rather than exceptional - an oversized diff exceeding `maxBuffer`, a repository git refuses to describe - come back as a reason, so the function never rejects. Branch and recent subjects are collected as context, and tolerate the unborn-HEAD case where `git log` fails outright. Untracked files have no diff, so a bounded head of each one is read directly: without it an untracked-only change reaches the model as a bare list of filenames. Only the first 2KB of each file is read, so an enormous file costs nothing, and anything containing a NUL byte is skipped as binary. Output is capped by characters as well as lines. A line limit alone is not a bound - one minified or generated file can be a single line of several megabytes. Staged changes are collected first, since that is what a commit will actually contain. When nothing is staged it falls back to the working tree so callers still have something to summarize before staging. That fallback deliberately runs `git diff` rather than `git diff HEAD`: the index is known to be empty at that point so the output is identical, but `HEAD` does not resolve in a repository without an initial commit, where it would fail. Co-Authored-By: Claude Opus 5 --- src/utils/__tests__/git.spec.ts | 291 ++++++++++++++++++++++++++++--- src/utils/git.ts | 297 +++++++++++++++++++++++++++++++- 2 files changed, 565 insertions(+), 23 deletions(-) diff --git a/src/utils/__tests__/git.spec.ts b/src/utils/__tests__/git.spec.ts index 95040a3d01..d0a5d28f16 100644 --- a/src/utils/__tests__/git.spec.ts +++ b/src/utils/__tests__/git.spec.ts @@ -13,20 +13,14 @@ import { getWorkspaceGitInfo, convertGitUrlToHttps, getGitStatus, + getCommitContext, } from "../git" import { truncateOutput } from "../../integrations/misc/extract-text" -type ExecFunction = ( - command: string, - options: { cwd?: string }, - callback: (error: ExecException | null, result?: { stdout: string; stderr: string }) => void, -) => void - -type PromisifiedExec = (command: string, options?: { cwd?: string }) => Promise<{ stdout: string; stderr: string }> - // Mock child_process.exec vitest.mock("child_process", () => ({ exec: vitest.fn(), + execFile: vitest.fn(), })) // Mock fs.promises @@ -34,6 +28,7 @@ vitest.mock("fs", () => ({ promises: { access: vitest.fn(), readFile: vitest.fn(), + open: vitest.fn(), }, })) @@ -49,21 +44,27 @@ vitest.mock("vscode", () => ({ // Mock util.promisify to return our own mock function vitest.mock("util", () => ({ - promisify: vitest.fn((fn: ExecFunction): PromisifiedExec => { - return async (command: string, options?: { cwd?: string }) => { + promisify: vitest.fn((fn: (...args: unknown[]) => void) => { + return async (...args: unknown[]) => { // Call the original mock to maintain the mock implementation return new Promise((resolve, reject) => { - fn( - command, - options || {}, - (error: ExecException | null, result?: { stdout: string; stderr: string }) => { - if (error) { - reject(error) - } else { - resolve(result!) - } - }, - ) + const callback = (error: ExecException | null, result?: { stdout: string; stderr: string }) => { + if (error) { + reject(error) + } else { + resolve(result!) + } + } + + // `exec(command, options, cb)` and `execFile(file, args, options, cb)` differ in + // arity, so both shapes are normalized here rather than mocking promisify twice. + const [first, second, third] = args + + if (Array.isArray(second)) { + fn(first, second, third || {}, callback) + } else { + fn(first, second || {}, callback) + } }) } }), @@ -76,7 +77,7 @@ vitest.mock("../../integrations/misc/extract-text", () => ({ }), })) -import { exec } from "child_process" +import { exec, execFile } from "child_process" describe("git utils", () => { const cwd = "/test/path" @@ -351,6 +352,252 @@ describe("git utils", () => { }) }) + describe("getCommitContext", () => { + const NUL = "\0" + const mockDiff = "@@ -1,1 +1,2 @@\n-old line\n+new line" + + type ExecResult = { stdout: string; stderr: string } + type ExecCallback = (error: Error | null, result?: ExecResult) => void + + // `checkGitInstalled` and `checkGitRepo` are fixed strings, so they still run through `exec`. + const mockProbes = ({ installed = true, repo = true } = {}) => { + vitest.mocked(exec).mockImplementation(((command: string, _options: unknown, callback: ExecCallback) => { + const available = command === "git --version" ? installed : repo + + if (available) { + callback(null, { stdout: "ok", stderr: "" }) + } else { + callback(new Error(`unavailable: ${command}`)) + } + + return {} as ReturnType + }) as unknown as typeof exec) + } + + // Keyed by the joined argument array, since that is what the collector passes now. Anything + // not listed rejects, which is how the failure paths are exercised. + const mockGit = (responses: Record) => { + const calls: Array<{ file: string; args: string[] }> = [] + + vitest.mocked(execFile).mockImplementation((( + file: string, + args: string[], + _options: unknown, + callback: ExecCallback, + ) => { + calls.push({ file, args }) + const stdout = responses[args.join(" ")] + + if (stdout === undefined) { + callback(new Error(`unexpected command: git ${args.join(" ")}`)) + } else { + callback(null, { stdout, stderr: "" }) + } + + return {} as ReturnType + }) as unknown as typeof execFile) + + return calls + } + + const staged = (nameStatus: string, diff = mockDiff): Record => ({ + "diff --cached --name-status -z": nameStatus, + "diff --cached --unified=1": diff, + "branch --show-current": "feature/x\n", + "log -n5 --format=%s": "earlier subject\n", + }) + + const workingTree = (status: string, diff = mockDiff): Record => ({ + "diff --cached --name-status -z": "", + "status --porcelain=v1 -z --untracked-files=all": status, + "diff --unified=1": diff, + "rev-parse --show-toplevel": `${cwd}\n`, + "branch --show-current": "main\n", + "log -n5 --format=%s": "earlier subject\n", + }) + + // Narrows the result so a failure reports its reason instead of a property-of-undefined. + const expectContext = async () => { + const result = await getCommitContext(cwd) + + if (!result.ok) { + throw new Error(`expected a context, got "${result.reason}"`) + } + + return result.context + } + + const mockUntrackedFile = (contents: Buffer | null) => { + vitest.mocked(fs.promises.open).mockImplementation((async () => { + if (!contents) { + throw new Error("ENOENT") + } + + return { + read: async (buffer: Buffer) => ({ bytesRead: contents.copy(buffer) }), + close: async () => {}, + } + }) as unknown as typeof fs.promises.open) + } + + it("should collect staged changes as structured entries", async () => { + mockProbes() + mockGit(staged(`M${NUL}src/file1.ts${NUL}A${NUL}src/new.ts${NUL}D${NUL}src/gone.ts${NUL}`)) + + const context = await expectContext() + expect(context.staged).toBe(true) + expect(context.files).toEqual([ + { status: "modified", path: "src/file1.ts" }, + { status: "added", path: "src/new.ts" }, + { status: "deleted", path: "src/gone.ts" }, + ]) + expect(context.branch).toBe("feature/x") + expect(context.recentCommits).toEqual(["earlier subject"]) + expect(context.diff).toContain("+new line") + }) + + // A rename or copy record carries two paths. Reading one where there are two would shift + // every later record onto the wrong file, so the trailing entry is the real assertion. + it("should parse renames and copies without desyncing later entries", async () => { + mockProbes() + mockGit( + staged( + `R100${NUL}old name.ts${NUL}new name.ts${NUL}` + + `C075${NUL}src/base.ts${NUL}src/copy.ts${NUL}` + + `M${NUL}src/after.ts${NUL}`, + ), + ) + + expect((await expectContext()).files).toEqual([ + { status: "renamed", path: "new name.ts", oldPath: "old name.ts" }, + { status: "copied", path: "src/copy.ts", oldPath: "src/base.ts" }, + { status: "modified", path: "src/after.ts" }, + ]) + }) + + it("should keep paths with spaces and unusual characters verbatim", async () => { + mockProbes() + mockGit(staged(`A${NUL}src/a "quoted" & odd (file).ts${NUL}`)) + + expect((await expectContext()).files).toEqual([{ status: "added", path: 'src/a "quoted" & odd (file).ts' }]) + }) + + // Replaces an older test that checked the command string for shell metacharacters. With + // `execFile` there is no shell at all, so the guard is that arguments stay separate values. + it("should pass every argument as an array element rather than a shell string", async () => { + mockProbes() + const calls = mockGit(staged(`M${NUL}src/file1.ts${NUL}`)) + + await getCommitContext(cwd) + + expect(calls.length).toBeGreaterThan(0) + expect(calls.every((call) => call.file === "git")).toBe(true) + expect(calls.map((call) => call.args)).toContainEqual(["diff", "--cached", "--name-status", "-z"]) + expect(calls.map((call) => call.args)).toContainEqual(["diff", "--cached", "--unified=1"]) + }) + + it("should fall back to the working tree when nothing is staged", async () => { + mockProbes() + mockGit(workingTree(` M src/file1.ts${NUL}?? src/untracked.ts${NUL}`)) + mockUntrackedFile(Buffer.from("export const value = 1\n")) + + const context = await expectContext() + expect(context.staged).toBe(false) + expect(context.files).toEqual([ + { status: "modified", path: "src/file1.ts" }, + { status: "untracked", path: "src/untracked.ts" }, + ]) + }) + + // Porcelain reverses the field order of `diff --name-status`: here the new path comes first. + it("should parse porcelain renames, where the new path comes first", async () => { + mockProbes() + mockGit(workingTree(`R new name.ts${NUL}old name.ts${NUL}M after.ts${NUL}`)) + + expect((await expectContext()).files).toEqual([ + { status: "renamed", path: "new name.ts", oldPath: "old name.ts" }, + { status: "modified", path: "after.ts" }, + ]) + }) + + it("should include bounded contents for untracked files", async () => { + mockProbes() + mockGit(workingTree(`?? src/untracked.ts${NUL}`)) + mockUntrackedFile(Buffer.from("export const answer = 42\n")) + + const context = await expectContext() + expect(context.diff).toContain("New file: src/untracked.ts") + expect(context.diff).toContain("export const answer = 42") + }) + + it("should skip untracked files that look binary", async () => { + mockProbes() + mockGit(workingTree(`?? assets/logo.png${NUL}`)) + mockUntrackedFile(Buffer.from([0x89, 0x50, 0x00, 0x4e, 0x47])) + + const context = await expectContext() + expect(context.files).toEqual([{ status: "untracked", path: "assets/logo.png" }]) + expect(context.diff).not.toContain("New file: assets/logo.png") + }) + + it("should work in a repository without an initial commit", async () => { + mockProbes() + // `git log` fails before the first commit, and must not take the collection down with it. + const responses = workingTree(`?? file.txt${NUL}`) + delete responses["log -n5 --format=%s"] + mockGit(responses) + mockUntrackedFile(Buffer.from("hello\n")) + + const context = await expectContext() + expect(context.recentCommits).toEqual([]) + expect(context.files).toEqual([{ status: "untracked", path: "file.txt" }]) + }) + + // A line limit alone is not a bound: one generated file can be a single enormous line. + it("should cap output by characters as well as by lines", async () => { + mockProbes() + mockGit(staged(`M${NUL}dist/bundle.js${NUL}`, `+${"a".repeat(200_000)}`)) + + await getCommitContext(cwd) + + expect(vitest.mocked(truncateOutput)).toHaveBeenCalledWith(expect.any(String), 500, 102_400) + }) + + it("should report no-changes on a clean tree", async () => { + mockProbes() + mockGit(workingTree("")) + + expect(await getCommitContext(cwd)).toEqual({ ok: false, reason: "no-changes" }) + }) + + it("should report git-missing when git is not installed", async () => { + mockProbes({ installed: false }) + mockGit({}) + + expect(await getCommitContext(cwd)).toEqual({ ok: false, reason: "git-missing" }) + }) + + it("should report not-a-repo outside a repository", async () => { + mockProbes({ repo: false }) + mockGit({}) + + expect(await getCommitContext(cwd)).toEqual({ ok: false, reason: "not-a-repo" }) + }) + + // An oversized diff exceeding `maxBuffer` is expected, not exceptional: the documented + // contract is a reason, never a rejection. + it("should report failed instead of rejecting when a git command fails", async () => { + mockProbes() + const responses = staged(`M${NUL}src/file1.ts${NUL}`) + delete responses["diff --cached --unified=1"] + mockGit(responses) + + const result = await getCommitContext(cwd) + expect(result.ok).toBe(false) + expect(result).toMatchObject({ reason: "failed" }) + }) + }) + describe("getWorkingState", () => { const mockStatus = " M src/file1.ts\n?? src/file2.ts" const mockDiff = "@@ -1,1 +1,2 @@\n-old line\n+new line" diff --git a/src/utils/git.ts b/src/utils/git.ts index 04c028c3d1..4b240d46c5 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -1,7 +1,7 @@ import * as vscode from "vscode" import * as path from "path" import { promises as fs } from "fs" -import { exec } from "child_process" +import { exec, execFile } from "child_process" import { promisify } from "util" import type { GitRepositoryInfo, GitCommit } from "@roo-code/types" @@ -10,8 +10,31 @@ import { truncateOutput } from "../integrations/misc/extract-text" const execAsync = promisify(exec) +// Used for the commit-context commands: arguments are passed as an array, so no shell is +// involved and paths never need quoting. +const execFileAsync = promisify(execFile) + const GIT_OUTPUT_LINE_LIMIT = 500 +// A line limit alone is not a bound: one minified or generated file can be a single line of +// several megabytes. This caps the payload regardless of how it is distributed across lines. +const GIT_OUTPUT_CHARACTER_LIMIT = 100 * 1024 + +// Node's default `exec` buffer is 1MB, which real-world diffs routinely exceed. +const GIT_DIFF_MAX_BUFFER = 10 * 1024 * 1024 + +// A commit message needs to know what changed, not every line of how. One line of surrounding +// context per hunk is enough to tell the model where an edit landed, and shrinking the prompt is +// the one latency factor we control without affecting the model's output. +const COMMIT_DIFF_ARGS = ["--unified=1"] + +// Untracked files have no diff, so their contents are read directly. Enough to tell the model +// what a new file is for, not enough for a large one to crowd out the rest of the context. +const UNTRACKED_FILE_BYTE_LIMIT = 2 * 1024 +const UNTRACKED_TOTAL_CHARACTER_LIMIT = 20 * 1024 + +const RECENT_COMMIT_COUNT = 5 + /** * Extracts git repository information from the workspace's .git directory * @param workspaceRoot The root path of the workspace @@ -346,6 +369,278 @@ export async function getWorkingState(cwd: string): Promise { } } +export type GitFileStatus = "added" | "modified" | "deleted" | "renamed" | "copied" | "untracked" | "unknown" + +export interface GitFileChange { + status: GitFileStatus + /** Path relative to the repository root, exactly as git reported it. */ + path: string + /** Where the file came from. Only set for renames and copies. */ + oldPath?: string +} + +export interface CommitContext { + /** True when describing the index, false when describing the whole working tree. */ + staged: boolean + /** Undefined when HEAD is detached. */ + branch?: string + recentCommits: string[] + files: GitFileChange[] + /** The diff, followed by the contents of any untracked files. Truncated to fit a prompt. */ + diff: string +} + +export type CommitContextResult = + | { ok: true; context: CommitContext } + | { ok: false; reason: "git-missing" | "not-a-repo" | "no-changes" | "failed"; error?: string } + +async function runGit(args: string[], cwd: string): Promise { + const { stdout } = await execFileAsync("git", args, { cwd, maxBuffer: GIT_DIFF_MAX_BUFFER }) + return stdout +} + +function toFileStatus(code: string): GitFileStatus { + switch (code) { + case "A": + return "added" + case "M": + return "modified" + case "D": + return "deleted" + case "R": + return "renamed" + case "C": + return "copied" + case "?": + return "untracked" + default: + return "unknown" + } +} + +/** + * Parses `git diff --name-status -z`: a NUL-terminated status field followed by one path, or - + * for renames and copies - by two paths, the original first. + * + * Copy records appear whenever the user has `diff.renames = copies` configured, so they have to + * be consumed correctly even though we never ask for copy detection: reading one path where + * there are two would shift every later record onto the wrong file. + */ +function parseNameStatus(stdout: string): GitFileChange[] { + const fields = stdout.split("\0") + const files: GitFileChange[] = [] + + for (let index = 0; index < fields.length; index++) { + const code = fields[index] + + // The final NUL leaves an empty trailing field. + if (!code) { + continue + } + + const status = toFileStatus(code[0]) + const first = fields[++index] + + if (status === "renamed" || status === "copied") { + const second = fields[++index] + + if (!first || !second) { + break + } + + files.push({ status, path: second, oldPath: first }) + continue + } + + if (!first) { + break + } + + files.push({ status, path: first }) + } + + return files +} + +/** + * Parses `git status --porcelain=v1 -z`: `XY`, with renames and copies adding the + * original path as a second NUL-terminated field. + * + * Note the field order is the reverse of `git diff --name-status -z` - here the new path comes + * first. Both formats are NUL-delimited, so paths are emitted verbatim and never quoted. + */ +function parsePorcelainStatus(stdout: string): GitFileChange[] { + const records = stdout.split("\0") + const files: GitFileChange[] = [] + + for (let index = 0; index < records.length; index++) { + const record = records[index] + + // The shortest valid record is two status characters, a space and a single-character path. + if (record.length < 4) { + continue + } + + const indexCode = record[0] + const worktreeCode = record[1] + const filePath = record.slice(3) + + // The index takes precedence, since that is what a commit would contain. + const status = toFileStatus(indexCode === " " ? worktreeCode : indexCode) + + if (status === "renamed" || status === "copied") { + files.push({ status, path: filePath, oldPath: records[++index] }) + continue + } + + files.push({ status, path: filePath }) + } + + return files +} + +/** + * Reads up to `UNTRACKED_FILE_BYTE_LIMIT` bytes of a file, or null if it cannot be read or looks + * binary. Only the head of the file is read, so an enormous untracked file costs nothing. + */ +async function readBoundedText(filePath: string): Promise { + const handle = await fs.open(filePath, "r").catch(() => null) + + if (!handle) { + return null + } + + try { + const buffer = Buffer.alloc(UNTRACKED_FILE_BYTE_LIMIT) + const { bytesRead } = await handle.read(buffer, 0, buffer.length, 0) + const contents = buffer.subarray(0, bytesRead) + + // An embedded NUL is the same heuristic git itself uses to call a file binary. + return contents.includes(0) ? null : contents.toString("utf8") + } catch { + return null + } finally { + await handle.close().catch(() => {}) + } +} + +/** + * Collects the contents of untracked files, which no diff would show. Without this an + * untracked-only change reaches the model as a bare list of filenames. + */ +async function getUntrackedContents(cwd: string, files: GitFileChange[]): Promise { + const untracked = files.filter((file) => file.status === "untracked") + + if (untracked.length === 0) { + return "" + } + + // Porcelain paths are relative to the repository root, which is not necessarily `cwd`. + const root = (await runGit(["rev-parse", "--show-toplevel"], cwd).catch(() => "")).trim() || cwd + const sections: string[] = [] + let total = 0 + + for (const file of untracked) { + if (total >= UNTRACKED_TOTAL_CHARACTER_LIMIT) { + break + } + + const contents = await readBoundedText(path.join(root, file.path)) + + if (contents === null) { + continue + } + + sections.push(`--- New file: ${file.path} ---\n${contents}`) + total += contents.length + } + + return sections.join("\n\n") +} + +/** Both of these are context, not the payload, so a repository without commits still works. */ +async function getCurrentBranch(cwd: string): Promise { + const branch = await runGit(["branch", "--show-current"], cwd).catch(() => "") + return branch.trim() || undefined +} + +async function getRecentCommits(cwd: string): Promise { + const log = await runGit(["log", `-n${RECENT_COMMIT_COUNT}`, "--format=%s"], cwd).catch(() => "") + return log + .split("\n") + .map((line) => line.trim()) + .filter(Boolean) +} + +/** + * Collects the changes to describe in a commit message. + * + * Prefers staged changes, since that is what a commit will actually contain. When nothing is + * staged, falls back to the whole working tree so the caller still has something to summarize. + * + * Every command runs through `execFile` with an argument array, so no path is ever interpolated + * into a shell string, and every listing is read in NUL-delimited form. + * + * @param cwd The repository root to inspect + * @returns The collected context, or the reason there is none. Never rejects. + */ +export async function getCommitContext(cwd: string): Promise { + if (!(await checkGitInstalled())) { + return { ok: false, reason: "git-missing" } + } + + if (!(await checkGitRepo(cwd))) { + return { ok: false, reason: "not-a-repo" } + } + + try { + const staged = parseNameStatus(await runGit(["diff", "--cached", "--name-status", "-z"], cwd)) + + if (staged.length > 0) { + const diff = await runGit(["diff", "--cached", ...COMMIT_DIFF_ARGS], cwd) + return { ok: true, context: await buildContext(cwd, true, staged, diff) } + } + + // Nothing staged - describe the working tree instead. `--untracked-files=all` lists files + // inside new directories individually, which the default summarized form would collapse. + const files = parsePorcelainStatus( + await runGit(["status", "--porcelain=v1", "-z", "--untracked-files=all"], cwd), + ) + + if (files.length === 0) { + return { ok: false, reason: "no-changes" } + } + + // Deliberately `git diff` rather than `git diff HEAD`: we only reach this branch when the + // index is empty, so the two produce identical output - but `HEAD` does not resolve in a + // repository without an initial commit, where it would fail outright. + const diff = await runGit(["diff", ...COMMIT_DIFF_ARGS], cwd) + const untracked = await getUntrackedContents(cwd, files) + + return { ok: true, context: await buildContext(cwd, false, files, `${diff}\n\n${untracked}`) } + } catch (error) { + // Failures here are expected rather than exceptional - an oversized diff exceeding + // `maxBuffer`, a repository in a state git refuses to describe - so the caller gets a + // reason rather than a rejection. + return { ok: false, reason: "failed", error: error instanceof Error ? error.message : String(error) } + } +} + +async function buildContext( + cwd: string, + staged: boolean, + files: GitFileChange[], + diff: string, +): Promise { + return { + staged, + branch: await getCurrentBranch(cwd), + recentCommits: await getRecentCommits(cwd), + files, + diff: truncateOutput(diff.trim(), GIT_OUTPUT_LINE_LIMIT, GIT_OUTPUT_CHARACTER_LIMIT), + } +} + /** * Gets git status output with configurable file limit * @param cwd The working directory to check git status in From f182cd61b7fbab93ae5bf8489c8b6a488514b5c6 Mon Sep 17 00:00:00 2001 From: Rafael-Silva-Oliveira Date: Thu, 13 Aug 2026 17:44:25 +0200 Subject: [PATCH 2/3] feat(git): describe only staged changes Falling back to the working tree meant the message could describe changes the commit would not contain. An empty index now returns `nothing-staged`, which the caller turns into advice to stage something, and `no-changes` is reserved for a genuinely clean tree. Removes the untracked-file reading that only the fallback needed. --- src/utils/__tests__/git.spec.ts | 65 ++++---------------- src/utils/git.ts | 104 ++++---------------------------- 2 files changed, 25 insertions(+), 144 deletions(-) diff --git a/src/utils/__tests__/git.spec.ts b/src/utils/__tests__/git.spec.ts index d0a5d28f16..6c623bef99 100644 --- a/src/utils/__tests__/git.spec.ts +++ b/src/utils/__tests__/git.spec.ts @@ -427,25 +427,11 @@ describe("git utils", () => { return result.context } - const mockUntrackedFile = (contents: Buffer | null) => { - vitest.mocked(fs.promises.open).mockImplementation((async () => { - if (!contents) { - throw new Error("ENOENT") - } - - return { - read: async (buffer: Buffer) => ({ bytesRead: contents.copy(buffer) }), - close: async () => {}, - } - }) as unknown as typeof fs.promises.open) - } - it("should collect staged changes as structured entries", async () => { mockProbes() mockGit(staged(`M${NUL}src/file1.ts${NUL}A${NUL}src/new.ts${NUL}D${NUL}src/gone.ts${NUL}`)) const context = await expectContext() - expect(context.staged).toBe(true) expect(context.files).toEqual([ { status: "modified", path: "src/file1.ts" }, { status: "added", path: "src/new.ts" }, @@ -496,61 +482,36 @@ describe("git utils", () => { expect(calls.map((call) => call.args)).toContainEqual(["diff", "--cached", "--unified=1"]) }) - it("should fall back to the working tree when nothing is staged", async () => { + // Only the index is described, so a dirty working tree with an empty index is a distinct + // outcome: the user can fix it by staging, and the caller says so. + it("should report nothing-staged when the working tree is dirty but the index is empty", async () => { mockProbes() mockGit(workingTree(` M src/file1.ts${NUL}?? src/untracked.ts${NUL}`)) - mockUntrackedFile(Buffer.from("export const value = 1\n")) - const context = await expectContext() - expect(context.staged).toBe(false) - expect(context.files).toEqual([ - { status: "modified", path: "src/file1.ts" }, - { status: "untracked", path: "src/untracked.ts" }, - ]) + expect(await getCommitContext(cwd)).toEqual({ ok: false, reason: "nothing-staged" }) }) - // Porcelain reverses the field order of `diff --name-status`: here the new path comes first. - it("should parse porcelain renames, where the new path comes first", async () => { + it("should describe only the index when both it and the working tree have changes", async () => { mockProbes() - mockGit(workingTree(`R new name.ts${NUL}old name.ts${NUL}M after.ts${NUL}`)) - - expect((await expectContext()).files).toEqual([ - { status: "renamed", path: "new name.ts", oldPath: "old name.ts" }, - { status: "modified", path: "after.ts" }, - ]) - }) - - it("should include bounded contents for untracked files", async () => { - mockProbes() - mockGit(workingTree(`?? src/untracked.ts${NUL}`)) - mockUntrackedFile(Buffer.from("export const answer = 42\n")) - - const context = await expectContext() - expect(context.diff).toContain("New file: src/untracked.ts") - expect(context.diff).toContain("export const answer = 42") - }) - - it("should skip untracked files that look binary", async () => { - mockProbes() - mockGit(workingTree(`?? assets/logo.png${NUL}`)) - mockUntrackedFile(Buffer.from([0x89, 0x50, 0x00, 0x4e, 0x47])) + // `workingTree` blanks the staged listing, so the staged responses have to win. + mockGit({ + ...workingTree(` M src/unstaged.ts${NUL}`), + ...staged(`M${NUL}src/staged.ts${NUL}`), + }) - const context = await expectContext() - expect(context.files).toEqual([{ status: "untracked", path: "assets/logo.png" }]) - expect(context.diff).not.toContain("New file: assets/logo.png") + expect((await expectContext()).files).toEqual([{ status: "modified", path: "src/staged.ts" }]) }) it("should work in a repository without an initial commit", async () => { mockProbes() // `git log` fails before the first commit, and must not take the collection down with it. - const responses = workingTree(`?? file.txt${NUL}`) + const responses = staged(`A${NUL}file.txt${NUL}`) delete responses["log -n5 --format=%s"] mockGit(responses) - mockUntrackedFile(Buffer.from("hello\n")) const context = await expectContext() expect(context.recentCommits).toEqual([]) - expect(context.files).toEqual([{ status: "untracked", path: "file.txt" }]) + expect(context.files).toEqual([{ status: "added", path: "file.txt" }]) }) // A line limit alone is not a bound: one generated file can be a single enormous line. diff --git a/src/utils/git.ts b/src/utils/git.ts index 4b240d46c5..a660f296f4 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -28,11 +28,6 @@ const GIT_DIFF_MAX_BUFFER = 10 * 1024 * 1024 // the one latency factor we control without affecting the model's output. const COMMIT_DIFF_ARGS = ["--unified=1"] -// Untracked files have no diff, so their contents are read directly. Enough to tell the model -// what a new file is for, not enough for a large one to crowd out the rest of the context. -const UNTRACKED_FILE_BYTE_LIMIT = 2 * 1024 -const UNTRACKED_TOTAL_CHARACTER_LIMIT = 20 * 1024 - const RECENT_COMMIT_COUNT = 5 /** @@ -380,19 +375,17 @@ export interface GitFileChange { } export interface CommitContext { - /** True when describing the index, false when describing the whole working tree. */ - staged: boolean /** Undefined when HEAD is detached. */ branch?: string recentCommits: string[] files: GitFileChange[] - /** The diff, followed by the contents of any untracked files. Truncated to fit a prompt. */ + /** The staged diff, truncated to fit a prompt. */ diff: string } export type CommitContextResult = | { ok: true; context: CommitContext } - | { ok: false; reason: "git-missing" | "not-a-repo" | "no-changes" | "failed"; error?: string } + | { ok: false; reason: "git-missing" | "not-a-repo" | "no-changes" | "nothing-staged" | "failed"; error?: string } async function runGit(args: string[], cwd: string): Promise { const { stdout } = await execFileAsync("git", args, { cwd, maxBuffer: GIT_DIFF_MAX_BUFFER }) @@ -499,65 +492,6 @@ function parsePorcelainStatus(stdout: string): GitFileChange[] { return files } -/** - * Reads up to `UNTRACKED_FILE_BYTE_LIMIT` bytes of a file, or null if it cannot be read or looks - * binary. Only the head of the file is read, so an enormous untracked file costs nothing. - */ -async function readBoundedText(filePath: string): Promise { - const handle = await fs.open(filePath, "r").catch(() => null) - - if (!handle) { - return null - } - - try { - const buffer = Buffer.alloc(UNTRACKED_FILE_BYTE_LIMIT) - const { bytesRead } = await handle.read(buffer, 0, buffer.length, 0) - const contents = buffer.subarray(0, bytesRead) - - // An embedded NUL is the same heuristic git itself uses to call a file binary. - return contents.includes(0) ? null : contents.toString("utf8") - } catch { - return null - } finally { - await handle.close().catch(() => {}) - } -} - -/** - * Collects the contents of untracked files, which no diff would show. Without this an - * untracked-only change reaches the model as a bare list of filenames. - */ -async function getUntrackedContents(cwd: string, files: GitFileChange[]): Promise { - const untracked = files.filter((file) => file.status === "untracked") - - if (untracked.length === 0) { - return "" - } - - // Porcelain paths are relative to the repository root, which is not necessarily `cwd`. - const root = (await runGit(["rev-parse", "--show-toplevel"], cwd).catch(() => "")).trim() || cwd - const sections: string[] = [] - let total = 0 - - for (const file of untracked) { - if (total >= UNTRACKED_TOTAL_CHARACTER_LIMIT) { - break - } - - const contents = await readBoundedText(path.join(root, file.path)) - - if (contents === null) { - continue - } - - sections.push(`--- New file: ${file.path} ---\n${contents}`) - total += contents.length - } - - return sections.join("\n\n") -} - /** Both of these are context, not the payload, so a repository without commits still works. */ async function getCurrentBranch(cwd: string): Promise { const branch = await runGit(["branch", "--show-current"], cwd).catch(() => "") @@ -575,8 +509,9 @@ async function getRecentCommits(cwd: string): Promise { /** * Collects the changes to describe in a commit message. * - * Prefers staged changes, since that is what a commit will actually contain. When nothing is - * staged, falls back to the whole working tree so the caller still has something to summarize. + * Only the index is described, since that is exactly what a commit will contain. An empty index + * returns `nothing-staged` rather than falling back to the working tree, so the message can never + * describe changes the commit would not include. * * Every command runs through `execFile` with an argument array, so no path is ever interpolated * into a shell string, and every listing is read in NUL-delimited form. @@ -598,26 +533,17 @@ export async function getCommitContext(cwd: string): Promise 0) { const diff = await runGit(["diff", "--cached", ...COMMIT_DIFF_ARGS], cwd) - return { ok: true, context: await buildContext(cwd, true, staged, diff) } + return { ok: true, context: await buildContext(cwd, staged, diff) } } - // Nothing staged - describe the working tree instead. `--untracked-files=all` lists files - // inside new directories individually, which the default summarized form would collapse. - const files = parsePorcelainStatus( + // Only the index is described, so an empty one has nothing to summarize. Whether the + // working tree is dirty decides which of the two messages the caller shows: "stage + // something first" is only useful advice when there is in fact something to stage. + const worktree = parsePorcelainStatus( await runGit(["status", "--porcelain=v1", "-z", "--untracked-files=all"], cwd), ) - if (files.length === 0) { - return { ok: false, reason: "no-changes" } - } - - // Deliberately `git diff` rather than `git diff HEAD`: we only reach this branch when the - // index is empty, so the two produce identical output - but `HEAD` does not resolve in a - // repository without an initial commit, where it would fail outright. - const diff = await runGit(["diff", ...COMMIT_DIFF_ARGS], cwd) - const untracked = await getUntrackedContents(cwd, files) - - return { ok: true, context: await buildContext(cwd, false, files, `${diff}\n\n${untracked}`) } + return { ok: false, reason: worktree.length > 0 ? "nothing-staged" : "no-changes" } } catch (error) { // Failures here are expected rather than exceptional - an oversized diff exceeding // `maxBuffer`, a repository in a state git refuses to describe - so the caller gets a @@ -626,14 +552,8 @@ export async function getCommitContext(cwd: string): Promise { +async function buildContext(cwd: string, files: GitFileChange[], diff: string): Promise { return { - staged, branch: await getCurrentBranch(cwd), recentCommits: await getRecentCommits(cwd), files, From 518a8fb2c1b898f2bc39b0acc9dcb3e8ed5a05ee Mon Sep 17 00:00:00 2001 From: Rafael-Silva-Oliveira Date: Fri, 14 Aug 2026 11:50:14 +0200 Subject: [PATCH 3/3] feat(git): describe the working tree when nothing is staged Staged changes are still what a message describes whenever there are any. Only when the index is empty does collection now fall back to the working tree, so an unstaged or untracked-only change is described instead of being refused, as issue #282 requires. The two are never mixed: staged wins outright. Untracked files carry no diff, so their contents are inlined. That is bounded on every axis that can grow without limit - at most ten files, at most 8KB read from each without loading the rest, and anything with a NUL byte marked binary rather than pasted in. Files past the limit are still named, since an added file is part of the change even when there is no room to show it. Rename and copy detection is also now requested explicitly instead of inheriting `diff.renames`, which decided whether a moved file reached the model as a rename or as an unrelated delete plus add depending on the user's git configuration. Co-Authored-By: Claude Opus 5 --- src/utils/__tests__/git.spec.ts | 127 ++++++++++++++++++++++++++++---- src/utils/git.ts | 98 +++++++++++++++++++++--- 2 files changed, 202 insertions(+), 23 deletions(-) diff --git a/src/utils/__tests__/git.spec.ts b/src/utils/__tests__/git.spec.ts index 6c623bef99..fc6c5bcd5d 100644 --- a/src/utils/__tests__/git.spec.ts +++ b/src/utils/__tests__/git.spec.ts @@ -401,17 +401,16 @@ describe("git utils", () => { } const staged = (nameStatus: string, diff = mockDiff): Record => ({ - "diff --cached --name-status -z": nameStatus, - "diff --cached --unified=1": diff, + "diff --cached --name-status -z --find-renames --find-copies": nameStatus, + "diff --cached --unified=1 --find-renames --find-copies": diff, "branch --show-current": "feature/x\n", "log -n5 --format=%s": "earlier subject\n", }) const workingTree = (status: string, diff = mockDiff): Record => ({ - "diff --cached --name-status -z": "", + "diff --cached --name-status -z --find-renames --find-copies": "", "status --porcelain=v1 -z --untracked-files=all": status, - "diff --unified=1": diff, - "rev-parse --show-toplevel": `${cwd}\n`, + "diff --unified=1 --find-renames --find-copies": diff, "branch --show-current": "main\n", "log -n5 --format=%s": "earlier subject\n", }) @@ -478,17 +477,119 @@ describe("git utils", () => { expect(calls.length).toBeGreaterThan(0) expect(calls.every((call) => call.file === "git")).toBe(true) - expect(calls.map((call) => call.args)).toContainEqual(["diff", "--cached", "--name-status", "-z"]) - expect(calls.map((call) => call.args)).toContainEqual(["diff", "--cached", "--unified=1"]) + expect(calls.map((call) => call.args)).toContainEqual([ + "diff", + "--cached", + "--name-status", + "-z", + "--find-renames", + "--find-copies", + ]) + expect(calls.map((call) => call.args)).toContainEqual([ + "diff", + "--cached", + "--unified=1", + "--find-renames", + "--find-copies", + ]) + }) + + // Left to `diff.renames`, a moved file reaches the model as a delete plus an add for some + // users and as a rename for others. + it("should ask for rename and copy detection rather than relying on git configuration", async () => { + mockProbes() + const calls = mockGit(staged(`R100${NUL}src/old.ts${NUL}src/new.ts${NUL}`)) + + await getCommitContext(cwd) + + expect( + calls.every( + (call) => + !call.args.includes("diff") || + (call.args.includes("--find-renames") && call.args.includes("--find-copies")), + ), + ).toBe(true) + }) + + it("should describe the working tree when the index is empty", async () => { + mockProbes() + mockGit(workingTree(` M src/file1.ts${NUL}`)) + + const context = await expectContext() + + expect(context.files).toEqual([{ status: "modified", path: "src/file1.ts" }]) + expect(context.diff).toContain("+new line") + }) + + // Reads `bytes` into the caller's buffer, the way a real file handle would. + const mockUntrackedFile = (bytes: Buffer) => { + const close = vitest.fn().mockResolvedValue(undefined) + + vitest.mocked(fs.promises.open).mockResolvedValue({ + read: vitest.fn().mockImplementation(async (buffer: Buffer, offset: number, length: number) => { + const written = bytes.copy(buffer, offset, 0, Math.min(length, bytes.length)) + return { bytesRead: written } + }), + close, + } as never) + + return { close } + } + + // A path alone does not say what an added file is for, which is most of what a commit + // message about a new file has to convey. + it("should inline the contents of untracked files", async () => { + mockProbes() + mockGit(workingTree(`?? src/added.ts${NUL}`, "")) + mockUntrackedFile(Buffer.from("export const answer = 42\n")) + + const context = await expectContext() + + expect(context.files).toEqual([{ status: "untracked", path: "src/added.ts" }]) + expect(context.diff).toContain("+++ b/src/added.ts") + expect(context.diff).toContain("export const answer = 42") + }) + + it("should mark binary untracked files instead of inlining them", async () => { + mockProbes() + mockGit(workingTree(`?? assets/logo.png${NUL}`, "")) + mockUntrackedFile(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00, 0x01, 0x02])) + + expect((await expectContext()).diff).toContain("(binary file)") + }) + + it("should read only the beginning of a large untracked file", async () => { + mockProbes() + mockGit(workingTree(`?? data/big.txt${NUL}`, "")) + mockUntrackedFile(Buffer.from("x".repeat(64 * 1024))) + + const context = await expectContext() + + expect(context.diff).toContain("(truncated)") + // The cap is what bounds this, not the number of bytes the file happens to hold. + expect(context.diff.length).toBeLessThan(32 * 1024) + }) + + it("should list untracked files past the limit by path only", async () => { + mockProbes() + const status = Array.from({ length: 12 }, (_, index) => `?? src/file${index}.ts${NUL}`).join("") + mockGit(workingTree(status, "")) + mockUntrackedFile(Buffer.from("contents\n")) + + const context = await expectContext() + + expect(context.files).toHaveLength(12) + expect(context.diff).toContain("(contents omitted)") + // Ten are read; the remaining two are named without being opened. + expect(fs.promises.open).toHaveBeenCalledTimes(10) }) - // Only the index is described, so a dirty working tree with an empty index is a distinct - // outcome: the user can fix it by staging, and the caller says so. - it("should report nothing-staged when the working tree is dirty but the index is empty", async () => { + it("should still describe an untracked file it cannot read", async () => { mockProbes() - mockGit(workingTree(` M src/file1.ts${NUL}?? src/untracked.ts${NUL}`)) + mockGit(workingTree(`?? src/vanished.ts${NUL}`, "")) + vitest.mocked(fs.promises.open).mockRejectedValue(new Error("ENOENT")) - expect(await getCommitContext(cwd)).toEqual({ ok: false, reason: "nothing-staged" }) + expect((await expectContext()).diff).toContain("(unreadable)") }) it("should describe only the index when both it and the working tree have changes", async () => { @@ -550,7 +651,7 @@ describe("git utils", () => { it("should report failed instead of rejecting when a git command fails", async () => { mockProbes() const responses = staged(`M${NUL}src/file1.ts${NUL}`) - delete responses["diff --cached --unified=1"] + delete responses["diff --cached --unified=1 --find-renames --find-copies"] mockGit(responses) const result = await getCommitContext(cwd) diff --git a/src/utils/git.ts b/src/utils/git.ts index a660f296f4..a3a8b00bef 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -23,13 +23,24 @@ const GIT_OUTPUT_CHARACTER_LIMIT = 100 * 1024 // Node's default `exec` buffer is 1MB, which real-world diffs routinely exceed. const GIT_DIFF_MAX_BUFFER = 10 * 1024 * 1024 +// Rename and copy detection is asked for explicitly rather than left to `diff.renames`, so a file +// that moved is classified the same way for everyone. With the user's configuration deciding it, a +// rename reaches the model as an unrelated delete plus add wherever that setting is off. +const RENAME_DETECTION_ARGS = ["--find-renames", "--find-copies"] + // A commit message needs to know what changed, not every line of how. One line of surrounding // context per hunk is enough to tell the model where an edit landed, and shrinking the prompt is // the one latency factor we control without affecting the model's output. -const COMMIT_DIFF_ARGS = ["--unified=1"] +const COMMIT_DIFF_ARGS = ["--unified=1", ...RENAME_DETECTION_ARGS] const RECENT_COMMIT_COUNT = 5 +// An untracked file has no diff to read, so its contents are inlined instead. These bounds are what +// keep a dropped build directory or a stray archive from becoming the entire prompt: a file count, +// a per-file byte cap read without loading the whole file, and a skip for anything binary. +const UNTRACKED_FILE_LIMIT = 10 +const UNTRACKED_FILE_BYTE_LIMIT = 8 * 1024 + /** * Extracts git repository information from the workspace's .git directory * @param workspaceRoot The root path of the workspace @@ -385,7 +396,7 @@ export interface CommitContext { export type CommitContextResult = | { ok: true; context: CommitContext } - | { ok: false; reason: "git-missing" | "not-a-repo" | "no-changes" | "nothing-staged" | "failed"; error?: string } + | { ok: false; reason: "git-missing" | "not-a-repo" | "no-changes" | "failed"; error?: string } async function runGit(args: string[], cwd: string): Promise { const { stdout } = await execFileAsync("git", args, { cwd, maxBuffer: GIT_DIFF_MAX_BUFFER }) @@ -509,9 +520,9 @@ async function getRecentCommits(cwd: string): Promise { /** * Collects the changes to describe in a commit message. * - * Only the index is described, since that is exactly what a commit will contain. An empty index - * returns `nothing-staged` rather than falling back to the working tree, so the message can never - * describe changes the commit would not include. + * Staged changes are described whenever there are any, since that is exactly what a commit will + * contain. Only when the index is empty does this fall back to the working tree - unstaged edits + * plus untracked files - so the two are never mixed into one message. * * Every command runs through `execFile` with an argument array, so no path is ever interpolated * into a shell string, and every listing is read in NUL-delimited form. @@ -529,21 +540,33 @@ export async function getCommitContext(cwd: string): Promise 0) { const diff = await runGit(["diff", "--cached", ...COMMIT_DIFF_ARGS], cwd) return { ok: true, context: await buildContext(cwd, staged, diff) } } - // Only the index is described, so an empty one has nothing to summarize. Whether the - // working tree is dirty decides which of the two messages the caller shows: "stage - // something first" is only useful advice when there is in fact something to stage. + // Nothing is staged, so fall back to the working tree rather than refusing: a commit made + // from here would stage these files first, so they are what the message has to describe. const worktree = parsePorcelainStatus( await runGit(["status", "--porcelain=v1", "-z", "--untracked-files=all"], cwd), ) - return { ok: false, reason: worktree.length > 0 ? "nothing-staged" : "no-changes" } + if (worktree.length === 0) { + return { ok: false, reason: "no-changes" } + } + + // Tracked edits still come from `git diff`. Untracked files are absent from it by + // definition, so their contents are appended separately or the model would be naming files + // it has never seen. + const tracked = await runGit(["diff", ...COMMIT_DIFF_ARGS], cwd) + const untracked = await readUntrackedFiles(cwd, worktree) + const diff = [tracked.trim(), untracked].filter(Boolean).join("\n\n") + + return { ok: true, context: await buildContext(cwd, worktree, diff) } } catch (error) { // Failures here are expected rather than exceptional - an oversized diff exceeding // `maxBuffer`, a repository in a state git refuses to describe - so the caller gets a @@ -552,6 +575,61 @@ export async function getCommitContext(cwd: string): Promise { + const untracked = files.filter((file) => file.status === "untracked") + const blocks: string[] = [] + + for (const file of untracked.slice(0, UNTRACKED_FILE_LIMIT)) { + blocks.push(`--- /dev/null\n+++ b/${file.path}\n${await readUntrackedFile(cwd, file.path)}`) + } + + // The rest are still worth naming - that files were added is part of the change even when there + // is no room to show what is in them. + const remaining = untracked.slice(UNTRACKED_FILE_LIMIT) + + if (remaining.length > 0) { + blocks.push(remaining.map((file) => `+++ b/${file.path} (contents omitted)`).join("\n")) + } + + return blocks.join("\n\n") +} + +/** Reads at most `UNTRACKED_FILE_BYTE_LIMIT` bytes, so a huge file costs one bounded read. */ +async function readUntrackedFile(cwd: string, filePath: string): Promise { + let handle + + try { + handle = await fs.open(path.resolve(cwd, filePath), "r") + + const buffer = Buffer.alloc(UNTRACKED_FILE_BYTE_LIMIT) + const { bytesRead } = await handle.read(buffer, 0, UNTRACKED_FILE_BYTE_LIMIT, 0) + const contents = buffer.subarray(0, bytesRead) + + // The same heuristic git uses: a NUL byte early in the file means it is not text. + if (contents.includes(0)) { + return "(binary file)" + } + + const text = contents.toString("utf8") + + return bytesRead < UNTRACKED_FILE_BYTE_LIMIT ? text : `${text}\n(truncated)` + } catch { + // A file listed a moment ago can be gone, or unreadable. Its path is already in the changed + // files list, so the message can still mention it. + return "(unreadable)" + } finally { + await handle?.close().catch(() => {}) + } +} + async function buildContext(cwd: string, files: GitFileChange[], diff: string): Promise { return { branch: await getCurrentBranch(cwd),