fix(review): harden jj diff and vcs detection

Author: backnotprop

AGENTS.md

@@ -152,7 +152,7 @@ User runs /plannotator-review command
 Claude Code: plannotator review subcommand runs
 OpenCode: event handler intercepts command
         ↓
-git diff captures unstaged changes
+VCS diff captures local changes (git diff or jj diff)
         ↓
 Review server starts, opens browser with diff viewer
         ↓

apps/marketing/src/content/docs/guides/ai-code-review.md

@@ -189,7 +189,7 @@ claude -p \
   --no-session-persistence \
   --model sonnet \
   --tools Agent,Bash,Read,Glob,Grep \
-  --allowedTools Agent,Read,Glob,Grep,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh api repos/*/*/pulls/*),Bash(gh api repos/*/*/pulls/*/files*),Bash(gh api repos/*/*/pulls/*/comments*),Bash(gh api repos/*/*/issues/*/comments*),Bash(glab mr view:*),Bash(glab mr diff:*),Bash(glab mr list:*),Bash(glab api:*),Bash(git status:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),Bash(git blame:*),Bash(git branch:*),Bash(git grep:*),Bash(git ls-remote:*),Bash(git ls-tree:*),Bash(git merge-base:*),Bash(git remote:*),Bash(git rev-parse:*),Bash(git show-ref:*),Bash(jj status:*),Bash(jj diff:*),Bash(jj log:*),Bash(jj show:*),Bash(jj bookmark list:*),Bash(wc:*) \
+  --allowedTools Agent,Read,Glob,Grep,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh api repos/*/*/pulls/*),Bash(gh api repos/*/*/pulls/*/files*),Bash(gh api repos/*/*/pulls/*/comments*),Bash(gh api repos/*/*/issues/*/comments*),Bash(glab mr view:*),Bash(glab mr diff:*),Bash(glab mr list:*),Bash(glab api:*),Bash(git status:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),Bash(git blame:*),Bash(git branch:*),Bash(git grep:*),Bash(git ls-remote:*),Bash(git ls-tree:*),Bash(git merge-base:*),Bash(git remote:*),Bash(git rev-parse:*),Bash(git show-ref:*),Bash(jj status:*),Bash(jj diff:*),Bash(jj log:*),Bash(jj show:*),Bash(jj file show:*),Bash(jj cat:*),Bash(jj bookmark list:*),Bash(wc:*) \
   --disallowedTools Edit,Write,NotebookEdit,WebFetch,WebSearch,Bash(python:*),Bash(python3:*),Bash(node:*),Bash(npx:*),Bash(bun:*),Bash(bunx:*),Bash(sh:*),Bash(bash:*),Bash(zsh:*),Bash(curl:*),Bash(wget:*)
 ```
 

packages/shared/jj-core.test.ts

@@ -4,6 +4,8 @@ import {
   jjLineBaseRevset,
   parseJjBookmarkList,
   parseJjRemoteBookmarkList,
+  type ReviewJjRuntime,
+  runJjDiff,
   selectDefaultJjCompareTarget,
 } from "./jj-core";
 
@@ -40,6 +42,45 @@ describe("jj diff args", () => {
     expect(getJjDiffArgs("jj-all", "trunk()", { hideWhitespace: true })?.args)
       .toEqual(["diff", "--git", "-w", "--from", "root()", "--to", "@"]);
   });
+
+  test("drops hunk-less file chunks after hide-whitespace filtering", async () => {
+    const runtimeForPatch = (stdout: string): ReviewJjRuntime => ({
+      async runJj() {
+        return {
+          stdout,
+          stderr: "",
+          exitCode: 0,
+        };
+      },
+    });
+
+    const hunklessChunk = [
+      "diff --git a/spacey.ts b/spacey.ts",
+      "index 1111111..2222222 100644",
+      "--- a/spacey.ts",
+      "+++ b/spacey.ts",
+      "",
+    ].join("\n");
+    const realChunk = [
+      "diff --git a/real.ts b/real.ts",
+      "index 3333333..4444444 100644",
+      "--- a/real.ts",
+      "+++ b/real.ts",
+      "@@ -1 +1 @@",
+      "-old",
+      "+new",
+      "",
+    ].join("\n");
+
+    const result = await runJjDiff(runtimeForPatch(hunklessChunk + realChunk), "jj-current", "trunk()", undefined, { hideWhitespace: true });
+
+    expect(result.patch).not.toContain("spacey.ts");
+    expect(result.patch).toContain("real.ts");
+    expect(result.patch).toContain("@@ -1 +1 @@");
+
+    const emptyResult = await runJjDiff(runtimeForPatch(hunklessChunk), "jj-current", "trunk()", undefined, { hideWhitespace: true });
+    expect(emptyResult.patch).toBe("");
+  });
 });
 
 describe("jj compare targets", () => {

packages/shared/jj-core.ts

@@ -87,7 +87,26 @@ export async function runJjDiff(
     return { patch: "", label: args.label, error: firstErrorLine(result.stderr) };
   }
 
-  return { patch: result.stdout, label: args.label };
+  const patch = options?.hideWhitespace ? dropHunklessGitDiffChunks(result.stdout) : result.stdout;
+  return { patch, label: args.label };
+}
+
+function dropHunklessGitDiffChunks(patch: string): string {
+  if (!patch.includes("diff --git ")) return patch;
+
+  const chunks = patch.split(/^diff --git /m);
+  const prefix = chunks.shift() ?? "";
+  const filtered = chunks
+    .filter(hasReviewableGitDiffChunk)
+    .map((chunk) => `diff --git ${chunk}`)
+    .join("");
+
+  return `${prefix}${filtered}`;
+}
+
+function hasReviewableGitDiffChunk(chunk: string): boolean {
+  if (/^@@@? /m.test(chunk)) return true;
+  return /^(new file mode|deleted file mode|old mode|new mode|rename from|rename to|copy from|copy to|GIT binary patch|Binary files |similarity index|dissimilarity index)/m.test(chunk);
 }
 
 export async function getJjFileContentsForDiff(

packages/shared/vcs-core.test.ts

@@ -94,6 +94,20 @@ describe("createVcsApi", () => {
     await expect(api.getVcsContext("/repo/packages/tool")).resolves.toMatchObject({ vcsType: "jj" });
   });
 
+  test("continues probing providers when root detection throws", async () => {
+    const brokenGit = {
+      ...provider("git", true, ["uncommitted"]),
+      async getRoot() {
+        throw new Error("git failed");
+      },
+    };
+    const p4 = provider("p4", true, ["p4-default"], {}, "/repo");
+    const api = createVcsApi([brokenGit, p4]);
+
+    await expect(api.detectVcs("/repo")).resolves.toBe(p4);
+    await expect(api.getVcsContext("/repo")).resolves.toMatchObject({ vcsType: "p4" });
+  });
+
   test("routes operations by diff type before falling back to detection", async () => {
     const jj = provider("jj", false, ["jj-current"]);
     const git = provider("git", true, ["uncommitted"]);

packages/shared/vcs-core.ts

@@ -242,8 +242,19 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi {
 
     const candidates: Array<{ provider: VcsProvider; root: string | null; order: number }> = [];
     for (const provider of providerList) {
-      const root = provider.getRoot ? await provider.getRoot(cwd) : null;
-      if (root || (!provider.getRoot && await provider.detect(cwd))) {
+      let root: string | null = null;
+      let detected = false;
+      try {
+        if (provider.getRoot) {
+          root = await provider.getRoot(cwd);
+          detected = root !== null;
+        } else {
+          detected = await provider.detect(cwd);
+        }
+      } catch {
+        continue;
+      }
+      if (detected) {
         candidates.push({ provider, root, order: candidates.length });
       }
     }