diff --git a/llp/0004-diff-noise-and-prompts.explainer.md b/llp/0004-diff-noise-and-prompts.explainer.md index 2f8e7a5..5a0b3c4 100644 --- a/llp/0004-diff-noise-and-prompts.explainer.md +++ b/llp/0004-diff-noise-and-prompts.explainer.md @@ -77,8 +77,8 @@ path. `parseUnifiedDiff` has three independent callers with different trust needs, so a parsing bug is not contained to the reviewer feed: the two sources -(`src/sources/local-git.ts:97`, `src/sources/github-pr.ts:98`) turn raw diff -into the review input, and the GitHub reporter (`src/reporters/github.ts:214`, +(`src/sources/local-git.ts:118`, `src/sources/github-pr.ts:233`) turn raw diff +into the review input, and the GitHub reporter (`src/reporters/github.ts:1094`, `buildDiffLineIndex`) re-parses to anchor inline PR comments to line numbers [observed] (grep of `parseUnifiedDiff` call sites). A path or hunk-offset bug would corrupt comment placement, not just review content. diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index fa4b559..28f47d6 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -47,13 +47,15 @@ The base OID is fetchable by SHA even though the base ref may have moved since t ## Local Git Source -`LocalGitSource` has no network calls; it reads local git state and computes its own diff base [observed] ([local-git.ts:17-21](../src/sources/local-git.ts)). Three small decisions carry the weight. +`LocalGitSource` has no network calls; it reads local git state and computes its own diff base [observed] ([local-git.ts:35-39](../src/sources/local-git.ts)). Four small decisions carry the weight. -**Default branch resolution** tries `origin/HEAD` via `symbolic-ref` first, then falls back through a fixed guess list (`origin/main`, `origin/master`, `main`, `master`), then `main`, because not every checkout has `origin/HEAD` set [observed] ([local-git.ts:31-50](../src/sources/local-git.ts)). +**Default branch resolution** tries `origin/HEAD` via `symbolic-ref` first, then falls back through a fixed guess list (`origin/main`, `origin/master`, `main`, `master`), then `main`, because not every checkout has `origin/HEAD` set [observed] ([local-git.ts:49-68](../src/sources/local-git.ts)). -**Diff base is `merge-base(defaultBranch, HEAD)`, not the branch tip.** This scopes the review to the current branch's own changes and excludes commits merged into the default branch after the feature branch diverged — you review what you wrote, not what trunk moved on to [observed] ([local-git.ts:53-68](../src/sources/local-git.ts)). +**Diff base is `merge-base(defaultBranch, HEAD)`, not the branch tip.** This scopes the review to the current branch's own changes and excludes commits merged into the default branch after the feature branch diverged — you review what you wrote, not what trunk moved on to [observed] ([local-git.ts:71-86](../src/sources/local-git.ts)). -**Untracked files are diffed without mutating the index.** Running a review must have no side effects on the user's working state, so untracked files are synthesized as add-diffs via `git diff --no-index -- /dev/null ` rather than being staged [observed] ([local-git.ts:101-103, 114](../src/sources/local-git.ts)). The listing uses `git ls-files -z` (NUL-terminated) so filenames containing newlines parse correctly [observed] ([local-git.ts:106-107](../src/sources/local-git.ts)), and the `--` separator before the filename stops a name beginning with `-` from being read as a git option — argument injection through an untracked filename [observed] ([local-git.ts:112, 114](../src/sources/local-git.ts)). +**Untracked files are diffed without mutating the index.** Running a review must have no side effects on the user's working state, so untracked files are synthesized as add-diffs via `git diff --no-index -- /dev/null ` rather than being staged [observed] ([local-git.ts:122-124, 137](../src/sources/local-git.ts)). The listing uses `git ls-files -z` (NUL-terminated) so filenames containing newlines parse correctly [observed] ([local-git.ts:127-128](../src/sources/local-git.ts)), and the `--` separator before the filename stops a name beginning with `-` from being read as a git option — argument injection through an untracked filename [observed] ([local-git.ts:133, 137](../src/sources/local-git.ts)). + +**Every parsed `git diff` overrides the user's diff config.** `git diff` follows the user's git config, and several settings change what it prints. An external diff tool (`diff.external`) replaces the unified diff entirely, forced color (`color.diff=always`) puts escape codes before `diff --git`, and `diff.noprefix` or `diff.mnemonicPrefix` drop or change the `a/` and `b/` prefixes that `parseUnifiedDiff` matches. Any of them made a local review parse no files and report "No reviewable changes". Two more change the content rather than break the parse: a textconv driver shows converted text instead of the file the reviewers and verifier read (or nothing, when both sides convert alike), and copy detection (`diff.renames=copies`) turns a copied file into a hunk-less `copy from`/`copy to` entry, so its contents go unreviewed. `diff.submodule` does both to a submodule change: `log` prints commit subjects that parse as nothing, and `diff` prints the submodule's own file diffs instead of the pointer change. `diff.ignoreSubmodules=all`, or `ignore = all` for a submodule in `.gitmodules`, drops the pointer change entirely. Each call therefore passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/ --no-textconv --find-renames --submodule=short --ignore-submodules=dirty` [observed]. `dirty` rather than git's default `none`: a submodule then shows only when its pointer moves, so edits or untracked files inside one never reach the reviewers as a `-dirty` pseudo-commit [observed] ([local-git.ts](../src/sources/local-git.ts) `PARSEABLE_DIFF_FLAGS`; tests `src/__tests__/local-git.test.ts`, which fail when any one flag is removed). Repository attributes (`binary`, `-diff`) still apply: in local mode the checkout is trusted (see [LLP 0001](./0001-trust-model.principles.md)), and a file they mark binary is dropped by noise filtering as before. ## GitHub Reporter Identity diff --git a/src/__tests__/local-git.test.ts b/src/__tests__/local-git.test.ts new file mode 100644 index 0000000..66029ff --- /dev/null +++ b/src/__tests__/local-git.test.ts @@ -0,0 +1,270 @@ +import { test, expect, afterAll, afterEach, beforeAll } from "bun:test"; +import { chmod, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { git } from "../core/exec.js"; +import { LocalGitSource } from "../sources/local-git.js"; + +// This file's fixture git must stay inside its temp repos. Git exports GIT_DIR, +// GIT_INDEX_FILE and friends to hooks, so `bun test` run from a pre-commit hook would +// otherwise write these fixtures into the commit being made. The user's global and +// system git files (signing, a hooks path, init templates, a global ignore or +// attributes file) could also fail the fixtures before any assertion runs, so HOME and +// XDG_CONFIG_HOME point at an empty directory, which works on every git version. +const savedEnv: Record = {}; +// Set only once mkdtemp succeeds: afterAll runs even when beforeAll throws, and must +// never remove whatever HOME still points at. +let fakeHome: string | undefined; +beforeAll(async () => { + for (const [name, value] of Object.entries(process.env)) { + if (name.startsWith("GIT_")) { + savedEnv[name] = value; + delete process.env[name]; + } + } + fakeHome = await mkdtemp(path.join(tmpdir(), "ecr-local-git-home-")); + for (const [name, value] of [ + ["HOME", fakeHome], + ["XDG_CONFIG_HOME", fakeHome], + ["GIT_CONFIG_NOSYSTEM", "1"], + ] as const) { + savedEnv[name] ??= process.env[name]; + process.env[name] = value; + } +}); +afterAll(async () => { + for (const [name, value] of Object.entries(savedEnv)) { + if (value === undefined) { + delete process.env[name]; + } else { + process.env[name] = value; + } + } + if (fakeHome) { + await rm(fakeHome, { recursive: true, force: true }); + } +}); + +const dirs: string[] = []; +afterEach(async () => { + for (const dir of dirs.splice(0)) { + await rm(dir, { recursive: true, force: true }); + } +}); + +async function tempDir(prefix: string): Promise { + const dir = await mkdtemp(path.join(tmpdir(), prefix)); + dirs.push(dir); + return dir; +} + +function commit(root: string, message: string): Promise { + return git(["-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", message], root); +} + +async function executable(dir: string, name: string, script: string): Promise { + const file = path.join(dir, name); + await writeFile(file, script, "utf8"); + await chmod(file, 0o755); + return file; +} + +// A committed repo with `a.txt` and `b/inner.txt`. The `b/` path matters: with no +// prefixes, `+++ b/inner.txt` loses its directory to the parser's `b/` strip. +async function baseRepo(): Promise { + const root = await tempDir("ecr-local-git-"); + await git(["init", "-q"], root); + await mkdir(path.join(root, "b")); + await writeFile(path.join(root, "a.txt"), "one\n", "utf8"); + await writeFile(path.join(root, "b", "inner.txt"), "inner\n", "utf8"); + await git(["add", "-A"], root); + await commit(root, "base"); + return root; +} + +// A repo whose local git config changes what `git diff` prints: an external diff +// tool, forced color, and no `a/` and `b/` prefixes. All of them break the parser +// unless the engine overrides them. +async function repoWithDiffConfig(): Promise { + const root = await baseRepo(); + const tools = await tempDir("ecr-local-git-tools-"); + const external = await executable( + tools, + "external-diff", + '#!/bin/sh\necho "external diff of $1"\n', + ); + await git(["config", "diff.external", external], root); + await git(["config", "color.diff", "always"], root); + await git(["config", "diff.noprefix", "true"], root); + return root; +} + +async function repoWithMnemonicPrefix(): Promise { + const root = await baseRepo(); + await git(["config", "diff.mnemonicPrefix", "true"], root); + return root; +} + +async function editBoth(root: string): Promise { + await writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); + await writeFile(path.join(root, "b", "inner.txt"), "inner\nmore\n", "utf8"); +} + +test("LocalGitSource: working-tree diffs parse despite the user's diff config", async () => { + const root = await repoWithDiffConfig(); + await editBoth(root); + await writeFile(path.join(root, "b", "new.txt"), "new\n", "utf8"); + const files = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles(); + expect(files.map((file) => file.path).sort()).toEqual(["a.txt", "b/inner.txt", "b/new.txt"]); + expect(files.every((file) => file.patch.includes("@@"))).toBe(true); +}); + +test("LocalGitSource: a base...head diff parses despite the user's diff config", async () => { + const root = await repoWithDiffConfig(); + await editBoth(root); + await git(["add", "-A"], root); + await commit(root, "change"); + const files = await new LocalGitSource({ + cwd: root, + base: "HEAD~1", + head: "HEAD", + }).getChangedFiles(); + expect(files.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt"]); + expect(files[0]!.patch).toContain("+two"); +}); + +test("LocalGitSource: a staged diff parses despite the user's diff config", async () => { + const root = await repoWithDiffConfig(); + await editBoth(root); + await git(["add", "-A"], root); + const files = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles(); + expect(files.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt"]); + expect(files[0]!.patch).toContain("+two"); +}); + +test("LocalGitSource: diff.mnemonicPrefix doesn't rename working-tree, untracked or staged paths", async () => { + // Mnemonic prefixes are `c/` and `w/` for the working tree, `2/` for --no-index, + // and `i/` for the index, none of which the parser strips. + const root = await repoWithMnemonicPrefix(); + await editBoth(root); + await writeFile(path.join(root, "new.txt"), "new\n", "utf8"); + const working = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles(); + expect(working.map((file) => file.path).sort()).toEqual(["a.txt", "b/inner.txt", "new.txt"]); + await git(["add", "-A"], root); + const staged = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles(); + expect(staged.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt", "new.txt"]); +}); + +// Only a deletion takes its path from the `---` line, so only a deletion shows a wrong +// source prefix: `--- b/inner.txt` would lose its directory, and `--- c/b/inner.txt` +// would keep a `c/` that isn't part of the path. +test.each([ + ["diff.noprefix", repoWithDiffConfig], + ["diff.mnemonicPrefix", repoWithMnemonicPrefix], +])("LocalGitSource: a deleted file keeps its path under %s", async (_config, makeRepo) => { + const root = await makeRepo(); + await rm(path.join(root, "b", "inner.txt")); + const working = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles(); + await git(["add", "-A"], root); + const staged = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles(); + await commit(root, "delete"); + const range = await new LocalGitSource({ + cwd: root, + base: "HEAD~1", + head: "HEAD", + }).getChangedFiles(); + for (const files of [working, staged, range]) { + expect(files.map((file) => [file.status, file.path])).toEqual([["D", "b/inner.txt"]]); + } +}); + +test("LocalGitSource: a textconv driver can't replace or hide the real change", async () => { + // A driver that prints a constant makes both sides identical, so with textconv on + // the change to a.txt vanishes from the review. + const root = await baseRepo(); + const tools = await tempDir("ecr-local-git-tools-"); + const mask = await executable(tools, "mask", "#!/bin/sh\necho masked\n"); + await writeFile(path.join(root, ".git", "info", "attributes"), "*.txt diff=mask\n", "utf8"); + await git(["config", "diff.mask.textconv", mask], root); + await editBoth(root); + const files = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles(); + expect(files.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt"]); + expect(files[0]!.patch).toContain("+two"); + expect(files[0]!.patch).not.toContain("masked"); +}); + +test("LocalGitSource: copy detection can't turn a new file into an empty modification", async () => { + // With diff.renames=copies, an exact copy of a file the change also edits comes out + // as `copy from`/`copy to` with no hunk, so the new file's contents go unreviewed. + const root = await baseRepo(); + await git(["config", "diff.renames", "copies"], root); + await writeFile(path.join(root, "b", "copy.txt"), "inner\n", "utf8"); + await writeFile(path.join(root, "b", "inner.txt"), "inner\nmore\n", "utf8"); + await git(["add", "-A"], root); + const files = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles(); + const copy = files.find((file) => file.path === "b/copy.txt"); + expect(copy?.status).toBe("A"); + expect(copy?.patch).toContain("+inner"); +}); + +// `diff.submodule=log` prints `Submodule sub ..:` and commit subjects instead +// of a diff, so the change parses as nothing. `diff.submodule=diff` prints the +// submodule's own file diffs, so the review sees `sub/s.txt` instead of the pointer +// change the commit makes. `diff.ignoreSubmodules=all`, or `ignore = all` in +// `.gitmodules`, drops the change entirely. +async function repoWithSubmodule(): Promise<{ root: string; sub: string }> { + const root = await baseRepo(); + const sub = path.join(root, "sub"); + await mkdir(sub); + await git(["init", "-q"], sub); + await writeFile(path.join(sub, "s.txt"), "one\n", "utf8"); + await git(["add", "-A"], sub); + await commit(sub, "s1"); + await git(["add", "sub"], root); + await commit(root, "add sub"); + return { root, sub }; +} + +test.each([ + ["diff.submodule=log", ["config", "diff.submodule", "log"]], + ["diff.submodule=diff", ["config", "diff.submodule", "diff"]], + ["diff.ignoreSubmodules=all", ["config", "diff.ignoreSubmodules", "all"]], + [".gitmodules ignore = all", ["config", "-f", ".gitmodules", "submodule.sub.ignore", "all"]], +])("LocalGitSource: %s still shows a submodule change as its pointer", async (_name, setting) => { + const { root, sub } = await repoWithSubmodule(); + await writeFile(path.join(sub, "s.txt"), "two\n", "utf8"); + await git(["add", "-A"], sub); + await commit(sub, "s2"); + await git(setting, root); + if (setting.includes(".gitmodules")) { + await git(["config", "-f", ".gitmodules", "submodule.sub.path", "sub"], root); + await git(["add", ".gitmodules"], root); + await commit(root, "ignore sub"); + } + const working = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles(); + // `git add` skips a submodule that `.gitmodules` ignores unless forced. + await git(["add", "--force", "sub"], root); + const staged = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles(); + await commit(root, "bump sub"); + const range = await new LocalGitSource({ + cwd: root, + base: "HEAD~1", + head: "HEAD", + }).getChangedFiles(); + for (const files of [working, staged, range]) { + expect(files.map((file) => [file.status, file.path])).toEqual([["M", "sub"]]); + expect(files[0]!.patch).toContain("+Subproject commit"); + } +}); + +// Edits inside a submodule whose pointer didn't move aren't a change to review, even +// under a user's `diff.ignoreSubmodules=none`. +test("LocalGitSource: a submodule with local edits but the same pointer isn't a change", async () => { + const { root, sub } = await repoWithSubmodule(); + await git(["config", "diff.ignoreSubmodules", "none"], root); + await writeFile(path.join(sub, "s.txt"), "edited\n", "utf8"); + await writeFile(path.join(sub, "untracked.txt"), "new\n", "utf8"); + const files = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles(); + expect(files).toEqual([]); +}); diff --git a/src/sources/local-git.ts b/src/sources/local-git.ts index 3567504..1bb3bf4 100644 --- a/src/sources/local-git.ts +++ b/src/sources/local-git.ts @@ -4,6 +4,25 @@ import { parseUnifiedDiff } from "../core/diff.js"; import type { DiffEntry, ReviewMetadata } from "../core/schema.js"; import type { ReviewSource } from "./source.js"; +// `git diff` honors the user's config: an external diff tool (`diff.external`), forced +// color, other path prefixes (`diff.noprefix`, `diff.mnemonicPrefix`), textconv +// drivers, copy detection (`diff.renames=copies`), submodule logs or inline diffs +// (`diff.submodule`), and ignored submodules (`diff.ignoreSubmodules`, or `ignore` +// in `.gitmodules`). These flags restore the plain unified diff of the real file +// contents, with the `a/` and `b/` prefixes that parseUnifiedDiff reads. A submodule +// shows only when its pointer moves, never for edits inside it. +// @ref LLP 0008#local-git-source [implements] — every parsed `git diff` overrides the user's diff config +const PARSEABLE_DIFF_FLAGS = [ + "--no-ext-diff", + "--no-color", + "--src-prefix=a/", + "--dst-prefix=b/", + "--no-textconv", + "--find-renames", + "--submodule=short", + "--ignore-submodules=dirty", +]; + export interface LocalGitOptions { /** Base ref to diff against. Defaults to the merge-base with the default branch. */ base?: string; @@ -83,13 +102,16 @@ export class LocalGitSource implements ReviewSource { async getChangedFiles(): Promise { let raw: string; if (this.options.staged) { - raw = await git(["diff", "--staged"], this.cwd); + raw = await git(["diff", ...PARSEABLE_DIFF_FLAGS, "--staged"], this.cwd); } else { const base = await this.resolveBase(); if (this.options.head) { - raw = await git(["diff", `${base}...${this.options.head}`], this.cwd); + raw = await git( + ["diff", ...PARSEABLE_DIFF_FLAGS, `${base}...${this.options.head}`], + this.cwd, + ); } else { - const tracked = await git(["diff", base], this.cwd); + const tracked = await git(["diff", ...PARSEABLE_DIFF_FLAGS, base], this.cwd); const untracked = await this.untrackedDiffs(); raw = [tracked, untracked].filter((chunk) => chunk.trim()).join("\n"); } @@ -111,10 +133,11 @@ export class LocalGitSource implements ReviewSource { for (const file of files) { // `--` so a filename beginning with `-` can't be read as a git option. const gitPath = await resolveTrustedTool("git"); - const { stdout } = await run(gitPath, ["diff", "--no-index", "--", "/dev/null", file], { - cwd: this.cwd, - check: false, - }); + const { stdout } = await run( + gitPath, + ["diff", ...PARSEABLE_DIFF_FLAGS, "--no-index", "--", "/dev/null", file], + { cwd: this.cwd, check: false }, + ); if (stdout.trim()) { chunks.push(stdout); }