From b6390caf157e59d3280a629c340633f3ae1df6f9 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Wed, 21 Oct 2026 00:34:53 +0200 Subject: [PATCH 1/3] fix: parse local diffs whatever the user's git diff config says `git diff` follows the user's git config. An external diff tool (`diff.external`, for example difftastic) replaces the unified diff, forced color puts escape codes before `diff --git`, and `diff.noprefix` or `diff.mnemonicPrefix` drop or change the `a/` and `b/` prefixes. With any of them, a local `ecr review` parsed no files and reported "No reviewable changes". Every `git diff` that LocalGitSource parses now passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/`. CI is unaffected: it reads the PR diff from GitHub's API. --- llp/0008-sources-and-reporters.explainer.md | 4 +- src/__tests__/local-git.test.ts | 75 +++++++++++++++++++++ src/sources/local-git.ts | 24 +++++-- 3 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 src/__tests__/local-git.test.ts diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index fa4b559..c62b2f5 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -47,7 +47,7 @@ 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:17-21](../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)). @@ -55,6 +55,8 @@ The base OID is fetchable by SHA even though the base ref may have moved since t **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)). +**Every parsed `git diff` overrides the user's diff config.** `git diff` follows the user's git config, and three 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". Each call therefore passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/` [observed] ([local-git.ts](../src/sources/local-git.ts) `PARSEABLE_DIFF_FLAGS`; tests `src/__tests__/local-git.test.ts`). + ## GitHub Reporter Identity The reporter maintains exactly one PR comment and is comment-only — it never touches the review-thread / GitHub "reviews" APIs, by explicit design [observed] ([github.ts:77-82](../src/reporters/github.ts)). The hard problem is: which comment on the PR is *ours*? diff --git a/src/__tests__/local-git.test.ts b/src/__tests__/local-git.test.ts new file mode 100644 index 0000000..4e3f184 --- /dev/null +++ b/src/__tests__/local-git.test.ts @@ -0,0 +1,75 @@ +import { test, expect, afterEach } from "bun:test"; +import { chmod, 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"; + +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); +} + +// 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 tempDir("ecr-local-git-"); + const tools = await tempDir("ecr-local-git-tools-"); + const external = path.join(tools, "external-diff"); + await writeFile(external, '#!/bin/sh\necho "external diff of $1"\n', "utf8"); + await chmod(external, 0o755); + await git(["init", "-q"], root); + await writeFile(path.join(root, "a.txt"), "one\n", "utf8"); + await git(["add", "a.txt"], root); + await commit(root, "base"); + await git(["config", "diff.external", external], root); + await git(["config", "color.diff", "always"], root); + await git(["config", "diff.noprefix", "true"], root); + return root; +} + +test("LocalGitSource: working-tree diffs parse despite the user's diff config", async () => { + const root = await repoWithDiffConfig(); + await writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); + await writeFile(path.join(root, "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", "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 writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); + await git(["add", "a.txt"], 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"]); + expect(files[0]!.patch).toContain("+two"); +}); + +test("LocalGitSource: a staged diff parses despite the user's diff config", async () => { + const root = await repoWithDiffConfig(); + await writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); + await git(["add", "a.txt"], root); + const files = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles(); + expect(files.map((file) => file.path)).toEqual(["a.txt"]); + expect(files[0]!.patch).toContain("+two"); +}); diff --git a/src/sources/local-git.ts b/src/sources/local-git.ts index 3567504..fe37fa4 100644 --- a/src/sources/local-git.ts +++ b/src/sources/local-git.ts @@ -4,6 +4,12 @@ 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, and other path prefixes (`diff.noprefix`, `diff.mnemonicPrefix`). These flags +// restore the plain unified diff with `a/` and `b/` prefixes that parseUnifiedDiff reads. +// @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/"]; + export interface LocalGitOptions { /** Base ref to diff against. Defaults to the merge-base with the default branch. */ base?: string; @@ -83,13 +89,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 +120,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); } From 1c58670da8c8325bdaef4590eefe4a32cd7babc2 Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Tue, 20 Oct 2026 16:53:40 -0700 Subject: [PATCH 2/3] fix: also disable textconv and copy detection in local diffs, and harden the tests A textconv driver showed converted text instead of the real file, or hid a change when both sides converted alike, and `diff.renames=copies` turned a copied file into a hunk-less entry whose contents went unreviewed. `--no-textconv` and `--find-renames` join the shared flags. The tests now fail when any one flag is removed: the fixture has a `b/` path that `diff.noprefix` breaks, and new cases cover `diff.mnemonicPrefix`, textconv and copy detection. Fixture commits ignore global signing and hooks, and LLP 0008's line references are current again. --- llp/0008-sources-and-reporters.explainer.md | 10 +- src/__tests__/local-git.test.ts | 110 ++++++++++++++++---- src/sources/local-git.ts | 15 ++- 3 files changed, 108 insertions(+), 27 deletions(-) diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index c62b2f5..d8ec9a5 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -47,15 +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)). Four 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:32-36](../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:46-65](../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:68-83](../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:119-121, 134](../src/sources/local-git.ts)). The listing uses `git ls-files -z` (NUL-terminated) so filenames containing newlines parse correctly [observed] ([local-git.ts:124-125](../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:130, 134](../src/sources/local-git.ts)). -**Every parsed `git diff` overrides the user's diff config.** `git diff` follows the user's git config, and three 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". Each call therefore passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/` [observed] ([local-git.ts](../src/sources/local-git.ts) `PARSEABLE_DIFF_FLAGS`; tests `src/__tests__/local-git.test.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. Each call therefore passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/ --no-textconv --find-renames` [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 index 4e3f184..bb2f414 100644 --- a/src/__tests__/local-git.test.ts +++ b/src/__tests__/local-git.test.ts @@ -1,5 +1,5 @@ import { test, expect, afterEach } from "bun:test"; -import { chmod, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { chmod, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -19,57 +19,129 @@ async function tempDir(prefix: string): Promise { return dir; } +// Fixture commits must not depend on the developer's global git config: signing or a +// global hooks path would fail them before any assertion runs. function commit(root: string, message: string): Promise { - return git(["-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", message], root); + const identity = ["-c", "user.name=t", "-c", "user.email=t@t"]; + return git( + [...identity, "-c", "commit.gpgsign=false", "commit", "-q", "--no-verify", "-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 tempDir("ecr-local-git-"); + const root = await baseRepo(); const tools = await tempDir("ecr-local-git-tools-"); - const external = path.join(tools, "external-diff"); - await writeFile(external, '#!/bin/sh\necho "external diff of $1"\n', "utf8"); - await chmod(external, 0o755); - await git(["init", "-q"], root); - await writeFile(path.join(root, "a.txt"), "one\n", "utf8"); - await git(["add", "a.txt"], root); - await commit(root, "base"); + 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 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 writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); - await writeFile(path.join(root, "new.txt"), "new\n", "utf8"); + 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", "new.txt"]); + 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 writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); - await git(["add", "a.txt"], root); + 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"]); + 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 writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8"); - await git(["add", "a.txt"], root); + 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"]); + 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 baseRepo(); + await git(["config", "diff.mnemonicPrefix", "true"], root); + 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"]); +}); + +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 --git a/src/sources/local-git.ts b/src/sources/local-git.ts index fe37fa4..ecc5433 100644 --- a/src/sources/local-git.ts +++ b/src/sources/local-git.ts @@ -5,10 +5,19 @@ 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, and other path prefixes (`diff.noprefix`, `diff.mnemonicPrefix`). These flags -// restore the plain unified diff with `a/` and `b/` prefixes that parseUnifiedDiff reads. +// color, other path prefixes (`diff.noprefix`, `diff.mnemonicPrefix`), textconv +// drivers, and copy detection (`diff.renames=copies`). These flags restore the plain +// unified diff of the real file contents, with the `a/` and `b/` prefixes that +// parseUnifiedDiff reads. // @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/"]; +const PARSEABLE_DIFF_FLAGS = [ + "--no-ext-diff", + "--no-color", + "--src-prefix=a/", + "--dst-prefix=b/", + "--no-textconv", + "--find-renames", +]; export interface LocalGitOptions { /** Base ref to diff against. Defaults to the merge-base with the default branch. */ From eb72d19b7a180ea2b033e21623d0499c28f68c3c Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Wed, 21 Oct 2026 14:28:38 -0700 Subject: [PATCH 3/3] fix: show submodule pointer changes whatever the diff config, and isolate the local-git tests - Local diffs also pass --submodule=short and --ignore-submodules=dirty, so diff.submodule=log|diff, diff.ignoreSubmodules=all and ignore = all in .gitmodules can't hide or rewrite a submodule pointer change, and edits inside a submodule whose pointer didn't move aren't sent for review. - The tests remove every GIT_* variable and point HOME and XDG_CONFIG_HOME at an empty directory, so a hook's GIT_DIR/GIT_INDEX_FILE or the user's global git files can't touch another repo or fail the fixtures. - New tests: deletions under diff.noprefix and diff.mnemonicPrefix (the only case --src-prefix protects), and the submodule settings above. - LLP 0004 and 0008 line references updated. --- llp/0004-diff-noise-and-prompts.explainer.md | 4 +- llp/0008-sources-and-reporters.explainer.md | 10 +- src/__tests__/local-git.test.ts | 143 +++++++++++++++++-- src/sources/local-git.ts | 10 +- 4 files changed, 147 insertions(+), 20 deletions(-) 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 d8ec9a5..28f47d6 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -47,15 +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:32-36](../src/sources/local-git.ts)). Four 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:46-65](../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:68-83](../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:119-121, 134](../src/sources/local-git.ts)). The listing uses `git ls-files -z` (NUL-terminated) so filenames containing newlines parse correctly [observed] ([local-git.ts:124-125](../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:130, 134](../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. Each call therefore passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/ --no-textconv --find-renames` [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. +**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 index bb2f414..66029ff 100644 --- a/src/__tests__/local-git.test.ts +++ b/src/__tests__/local-git.test.ts @@ -1,4 +1,4 @@ -import { test, expect, afterEach } from "bun:test"; +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"; @@ -6,6 +6,46 @@ 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)) { @@ -19,14 +59,8 @@ async function tempDir(prefix: string): Promise { return dir; } -// Fixture commits must not depend on the developer's global git config: signing or a -// global hooks path would fail them before any assertion runs. function commit(root: string, message: string): Promise { - const identity = ["-c", "user.name=t", "-c", "user.email=t@t"]; - return git( - [...identity, "-c", "commit.gpgsign=false", "commit", "-q", "--no-verify", "-m", message], - root, - ); + 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 { @@ -66,6 +100,12 @@ async function repoWithDiffConfig(): Promise { 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"); @@ -106,8 +146,7 @@ test("LocalGitSource: a staged diff parses despite the user's diff config", asyn 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 baseRepo(); - await git(["config", "diff.mnemonicPrefix", "true"], root); + 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(); @@ -117,6 +156,29 @@ test("LocalGitSource: diff.mnemonicPrefix doesn't rename working-tree, untracked 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. @@ -145,3 +207,64 @@ test("LocalGitSource: copy detection can't turn a new file into an empty modific 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 ecc5433..1bb3bf4 100644 --- a/src/sources/local-git.ts +++ b/src/sources/local-git.ts @@ -6,9 +6,11 @@ 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, and copy detection (`diff.renames=copies`). These flags restore the plain -// unified diff of the real file contents, with the `a/` and `b/` prefixes that -// parseUnifiedDiff reads. +// 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", @@ -17,6 +19,8 @@ const PARSEABLE_DIFF_FLAGS = [ "--dst-prefix=b/", "--no-textconv", "--find-renames", + "--submodule=short", + "--ignore-submodules=dirty", ]; export interface LocalGitOptions {