fix: preserve exact delegated review diffs

Author: HenryQW

package-lock.json

@@ -31,7 +31,7 @@
     "@earendil-works/pi-coding-agent": "^0.84.3"
   },
   "dependencies": {
-    "@henryqw/pi-subagent": "^4.0.0",
+    "@henryqw/pi-subagent": "^4.1.0",
     "proper-lockfile": "^4.1.2",
     "typebox": "1.3.15"
   },

packages/pi-auto-dag/package.json

@@ -133,8 +133,9 @@ export async function persistReviewPatch(input: ReviewPatchInput): Promise<Revie
 	const path = join(artifactDirectory, `${basename(temporaryDirectory)}.patch`);
 	assertInsideRunDirectory(root, path);
 	try {
-		const result = await input.runner("git", ["diff", "--binary", `--output=${temporary}`, base, commit], { cwd: input.worktree });
-		if (result.code !== 0) throw new Error(commandFailure("git", ["diff", "--binary", `--output=${temporary}`, base, commit], result));
+		const args = ["diff", "--no-textconv", "--no-ext-diff", "--ignore-submodules=none", "--binary", `--output=${temporary}`, base, commit];
+		const result = await input.runner("git", args, { cwd: input.worktree });
+		if (result.code !== 0) throw new Error(commandFailure("git", args, result));
 		await chmod(temporary, 0o600);
 		const expected = await patchDigest(temporary);
 		try {

packages/pi-auto-dag/src/review.ts

@@ -3,20 +3,20 @@ import { execFile as execFileCallback, spawn } from "node:child_process";
 import { createHash } from "node:crypto";
 import { once } from "node:events";
 import { readFileSync } from "node:fs";
-import { mkdtemp, mkdir, readFile, rm, stat, utimes, writeFile } from "node:fs/promises";
+import { chmod, mkdtemp, mkdir, readFile, rm, stat, utimes, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import test, { type TestContext } from "node:test";
 import { promisify } from "node:util";
 import { fakeHerdr } from "./support/fake-herdr.ts";
 import { testLaunchResolver } from "./support/roles.ts";
-import { recordedGateEvidence, type CommandRunner } from "../src/command.ts";
+import { recordedGateEvidence, runCommand, type CommandRunner } from "../src/command.ts";
 import { createCoreLifecycle, type CoreLifecycle } from "../src/lifecycle.ts";
 import { readDeliveryGraph } from "../src/graph.ts";
 import { preflightLocalRun } from "../src/intake.ts";
 import { childWorktreePath } from "../src/implementation-workers.ts";
 import { type RunState } from "../src/model.ts";
-import { reviewPromptMode } from "../src/review.ts";
+import { persistReviewPatch, reviewPromptMode } from "../src/review.ts";
 import { parseWorkerEnvelope } from "../src/orchestration.ts";
 import { actionTicketPath, eventReceiptPath, readActionTicket, readWorkerReceipt, reviewId, type ActionTicket, WorkerEnvelopeRejectedError, writeWorkerReceipt } from "../src/review-ticket.ts";
 import { readActiveRunId, readRunState, recordAcceptedWorkerEvent, runDirectory, stateRoot, writeRunState } from "../src/state.ts";
@@ -28,6 +28,54 @@ test("stale reviewer packet identity forces a full recovery packet", () => {
 	assert.equal(reviewPromptMode(false, "existing", "old-base", "old-commit", "base", "commit"), "full");
 });
 
+test("review patches ignore configured diff transforms and submodule omission", async (t) => {
+	const root = await mkdtemp(join(tmpdir(), "pi-auto-dag-review-patch-"));
+	t.after(async () => { await rm(root, { recursive: true, force: true }); });
+	await git(root, "init", "-b", "main");
+	await git(root, "config", "user.email", "test@example.com");
+	await git(root, "config", "user.name", "Test User");
+	await writeFile(join(root, ".gitattributes"), "fixture.txt diff=fixture\n");
+	await writeFile(join(root, "fixture.txt"), "native base\n");
+	await git(root, "add", ".");
+	await git(root, "commit", "-m", "base");
+	const base = await git(root, "rev-parse", "HEAD");
+	const external = join(root, "external-diff");
+	await writeFile(external, "#!/bin/sh\nprintf 'external diff\\n'\n");
+	await chmod(external, 0o755);
+	await git(root, "config", "diff.fixture.textconv", "sed s/native/transformed/");
+	await git(root, "config", "diff.external", external);
+	await git(root, "config", "diff.ignoreSubmodules", "all");
+	await writeFile(join(root, "fixture.txt"), "native changed\n");
+	const gitlink = "1".repeat(40);
+	await git(root, "update-index", "--add", "--cacheinfo", `160000,${gitlink},vendor/review-fixture`);
+	await git(root, "add", "fixture.txt");
+	await git(root, "commit", "-m", "change");
+	const commit = await git(root, "rev-parse", "HEAD");
+	assert.match((await execFile("git", ["diff", "--textconv", "--no-ext-diff", "--ignore-submodules=none", base, commit], { cwd: root })).stdout, /transformed/);
+	assert.match((await execFile("git", ["diff", "--ext-diff", base, commit], { cwd: root })).stdout, /external diff/);
+	assert.doesNotMatch((await execFile("git", ["diff", "--no-textconv", "--no-ext-diff", "--ignore-submodules=all", base, commit], { cwd: root })).stdout, new RegExp(gitlink));
+	await mkdir(runDirectory(root, RUN_ID), { recursive: true });
+	const calls: ReadonlyArray<string>[] = [];
+	const patch = await persistReviewPatch({
+		runner: async (command, args, options) => {
+			if (command === "git") calls.push(args);
+			return await runCommand(command, args, options);
+		},
+		mainWorktree: root,
+		runId: RUN_ID,
+		worktree: root,
+		base,
+		commit,
+		context: { type: "integration_head" },
+	});
+	const actual = await readFile(patch.path);
+	const expected = (await execFile("git", ["diff", "--no-textconv", "--no-ext-diff", "--ignore-submodules=none", "--binary", base, commit], { cwd: root, encoding: "buffer" })).stdout;
+	assert.deepEqual(actual, expected);
+	assert.match(actual.toString(), /native changed/);
+	assert.match(actual.toString(), new RegExp(gitlink));
+	assert.ok(calls.some((args) => args.includes("--no-textconv") && args.includes("--no-ext-diff") && args.includes("--ignore-submodules=none")));
+});
+
 test("a successor takes over after the starter is killed with a durably identified live worker", async (t) => {
 	const project = await makeProject(t, graph(["alpha"]), 1, 1);
 	const child = spawn(process.execPath, ["--input-type=module", "--eval", `
@@ -1267,7 +1315,7 @@ async function assertReviewPatch(
 	assert.equal(patch.base, base);
 	assert.equal(patch.commit, commit);
 	const actual = await readFile(patch.path);
-	const expected = (await execFile("git", ["diff", "--binary", base, commit], { cwd: root, encoding: "buffer" })).stdout;
+	const expected = (await execFile("git", ["diff", "--no-textconv", "--no-ext-diff", "--ignore-submodules=none", "--binary", base, commit], { cwd: root, encoding: "buffer" })).stdout;
 	assert.deepEqual(actual, expected);
 	assert.equal(patch.bytes, actual.length);
 	assert.equal(patch.sha256, createHash("sha256").update(actual).digest("hex"));

packages/pi-auto-dag/test/orchestration.test.ts

@@ -785,7 +785,7 @@ async function assertReviewPatch(
 	assert.equal(patch.base, base);
 	assert.equal(patch.commit, commit);
 	const actual = await readFile(patch.path);
-	const expected = (await execFile("git", ["diff", "--binary", base, commit], { cwd: root, encoding: "buffer" })).stdout;
+	const expected = (await execFile("git", ["diff", "--no-textconv", "--no-ext-diff", "--ignore-submodules=none", "--binary", base, commit], { cwd: root, encoding: "buffer" })).stdout;
 	assert.deepEqual(actual, expected);
 	assert.equal(patch.bytes, actual.length);
 	assert.equal(patch.sha256, createHash("sha256").update(actual).digest("hex"));

packages/pi-auto-dag/test/pr-lifecycle.test.ts

@@ -33,15 +33,15 @@ For each bounded unit:
 1. **Implement** — one `delegate_task` single call to `implementer`. The packet states the objective, touched scope, required validation, and recorded base commit. Require its output to identify the retained worktree path, branch, base commit, tip commit, changed files from the base-to-tip committed diff, and clean `git status --porcelain=v1 --untracked-files=all` result.
 2. **Verify and review** — refuse review or merge unless Main independently verifies that the worktree was retained and is clean (including untracked files), the reported base/branch/tip identities are complete and match Git, the tip is descended from the recorded base, and the reported changed files equal `git diff --name-only "$base" "$tip"`. All intended changes must be in that committed base-to-tip diff. If any evidence is missing or any check fails, send the work to a fresh implementer repair; never review dirty or uncommitted work.
 
-   Create a private temporary exact-patch artifact outside the repository from `git diff --binary "$base" "$tip"`. Independently regenerate that diff and byte-compare it with the artifact, then record its path, byte count, and SHA-256. If creation, regeneration, comparison, byte count, or checksum fails, stop and report the artifact failure clearly; do not review or merge. Do not put any complete patch content in `delegate_task` text or argv.
+   Create a private temporary exact-patch artifact outside the repository from `git diff --no-textconv --no-ext-diff --ignore-submodules=none --binary "$base" "$tip"`. Independently regenerate that same diff and byte-compare it with the artifact, then record its path, byte count, and SHA-256. If creation, regeneration, comparison, byte count, or checksum fails, stop and report the artifact failure clearly; do not review or merge. Do not put any complete patch content in `delegate_task` text or argv.
 
    Make one `delegate_task` single call to `reviewer` with only a bounded metadata packet: base, tip, review context `{type:'child_branch', branch}`, the verified complete patch file reference (`path`, `bytes`, `sha256`), and the verified changed paths. If that complete metadata cannot fit the task transport bound, stop and report it rather than truncating or inlining patch content. Chain entries do not share files: `{previous}` passes text only.
 3. **Merge** — only after an approving review, re-check Main's clean status, verify the branch tip still equals the reviewed tip commit, then merge that exact commit (not the branch name) into Main's current worktree and run focused validation there. Never merge on unresolved findings.
 4. **Clean up after success** — only after the exact reviewed tip is integrated and focused validation passes, remove each reported retained worktree, then safely delete its task branch. Include a superseded repair-round worktree only when its exact tip is an ancestor of integrated `HEAD`. For each candidate, verify ancestry first, use non-forced worktree removal followed by `git branch -d`, and stop/report cleanup failure without deleting later evidence. On any integration or validation failure, preserve every temporary patch artifact, retained worktree, and task branch for recovery. After integration and validation succeed, remove the temporary reviewer patch artifacts.
 
 ## Findings
 
-Any reviewer finding goes back as a **fresh** `implementer` delegation containing the findings plus the reviewed base/branch/tip identities and complete exact patch file reference, followed by a fresh review of the new state. A fresh repair implementer starts in a new worktree from Main HEAD and does not contain the prior unit commit: the repair packet must first bring the reported predecessor commit into its fresh worktree (merge or cherry-pick as appropriate), then address the findings. Dirty or uncommitted work also goes only to this fresh repair path. Bound the loop (e.g. three rounds); past the bound, stop and report to the user instead of merging.
+Any reviewer finding goes back as a **fresh** `implementer` delegation containing the findings plus the reviewed base/branch/tip identities and complete exact patch file reference, followed by a fresh review of the new state. A fresh repair implementer starts in a new worktree from Main HEAD and does not contain the prior unit commit: the repair packet must first bring the whole reviewed `$base..$tip` range into its fresh worktree by merging the exact `$tip`, or cherry-picking every range commit in order. Cherry-pick `$tip` alone only after `git rev-list --count "$base..$tip"` verifies the range is exactly one commit; then address the findings. Dirty or uncommitted work also goes only to this fresh repair path. Bound the loop (e.g. three rounds); past the bound, stop and report to the user instead of merging.
 
 ## Parallelism
 

packages/pi-subagent/skills/pi-subagent-delegated-development/SKILL.md

@@ -24,7 +24,11 @@ test("bundled pi-subagent-delegated-development Skill is valid and registered",
 	assert.match(skill, /git rev-parse --verify -q HEAD\^\{commit\}/);
 	assert.match(skill, /refuse review or merge unless/i);
 	assert.match(skill, /never review dirty or uncommitted work/i);
-	assert.match(skill, /git diff --binary "\$base" "\$tip"/);
+	assert.match(skill, /git diff --no-textconv --no-ext-diff --ignore-submodules=none --binary "\$base" "\$tip"/);
+	assert.match(skill, /same diff and byte-compare it with the artifact/i);
+	assert.match(skill, /whole reviewed `\$base\.\.\$tip` range/i);
+	assert.match(skill, /cherry-picking every range commit in order/i);
+	assert.match(skill, /git rev-list --count "\$base\.\.\$tip"/);
 	assert.match(skill, /private temporary exact-patch artifact/i);
 	assert.match(skill, /byte-compare it with the artifact/i);
 	assert.match(skill, /review context `\{type:'child_branch', branch\}`/i);