fix(web): avoid trailing-context crash in multi-file diff panel (#1097)

apps/backend/internal/worktree/manager.go

@@ -293,7 +293,13 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err
 	// was anchored to a PR head or fresh branch that has been deleted upstream
 	// or never existed in a sibling repo.
 	fallbackWarning, fallbackDetail := "", ""
-	if !m.branchExists(ctx, req.RepositoryPath, baseRef) {
+	baseExists, baseErr := m.branchExists(ctx, req.RepositoryPath, baseRef)
+	if baseErr != nil {
+		// Could not determine existence (timeout / fs stall). Surface the
+		// real cause instead of pretending the branch is missing.
+		return nil, fmt.Errorf("could not verify base branch %q: %w", baseRef, baseErr)
+	}
+	if !baseExists {
 		fallback := strings.TrimSpace(req.FallbackBaseBranch)
 		if fallback == "" || fallback == baseRef {
 			return nil, fmt.Errorf("%w: %s", ErrInvalidBaseBranch, baseRef)
@@ -307,7 +313,11 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err
 		if req.PullBeforeWorktree {
 			resolvedFallback = m.pullBaseBranch(ctx, req.RepositoryPath, fallback, nil)
 		}
-		if !m.branchExists(ctx, req.RepositoryPath, resolvedFallback) {
+		fallbackExists, fallbackErr := m.branchExists(ctx, req.RepositoryPath, resolvedFallback)
+		if fallbackErr != nil {
+			return nil, fmt.Errorf("could not verify fallback base branch %q: %w", resolvedFallback, fallbackErr)
+		}
+		if !fallbackExists {
 			return nil, fmt.Errorf("%w: %s (fallback %q also not found)", ErrInvalidBaseBranch, baseRef, fallback)
 		}
 		m.logger.Warn("requested base branch not found, falling back",
@@ -529,7 +539,11 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin
 			zap.Error(err))
 
 		// Fall back to local branch if it exists.
-		if !m.branchExists(ctx, repoPath, branch) {
+		exists, existsErr := m.branchExists(ctx, repoPath, branch)
+		if existsErr != nil {
+			return nil, fmt.Errorf("could not verify local branch %q after fetch failure (%s): %w", branch, strings.TrimSpace(outputStr), existsErr)
+		}
+		if !exists {
 			return nil, fmt.Errorf("branch %q not found locally or on remote: %s", branch, outputStr)
 		}
 
@@ -1329,10 +1343,16 @@ func (m *Manager) isGitRepo(path string) bool {
 
 // branchExists checks if a branch exists in the repository.
 // Bounded by m.inspectTimeout so a hung git (credential prompt, stuck filter,
-// filesystem stall) cannot deadlock the caller while holding repoLock. When
-// the bound fires, the ctx error is logged so the root cause is visible in
-// logs rather than surfacing only as a misleading "branch not found".
-func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) bool {
+// filesystem stall) cannot deadlock the caller while holding repoLock.
+//
+// Returns:
+//   - (true, nil)  branch exists
+//   - (false, nil) git ran and reported the branch absent
+//   - (false, err) check could not be completed (timeout, fs stall); err
+//     carries the underlying ctx error so callers can distinguish a real
+//     "missing branch" from a "could not tell" and avoid surfacing a
+//     misleading ErrInvalidBaseBranch.
+func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) (bool, error) {
 	inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout)
 	defer cancel()
 	cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--verify", branch)
@@ -1342,10 +1362,11 @@ func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) boo
 				zap.String("repository_path", repoPath),
 				zap.String("branch", branch),
 				zap.Error(ctxErr))
+			return false, fmt.Errorf("branch check timed out for %q after %s: %w", branch, m.inspectTimeout, ctxErr)
 		}
-		return false
+		return false, nil
 	}
-	return true
+	return true, nil
 }
 
 func (m *Manager) currentBranch(ctx context.Context, repoPath string) string {
@@ -1469,7 +1490,9 @@ func (m *Manager) resolveLocalBaseRef(
 	if m.currentBranch(ctx, repoPath) == baseBranch {
 		return m.pullCurrentBranchOrFallback(ctx, repoPath, baseBranch, remoteRef, stepName, onProgress)
 	}
-	if m.branchExists(ctx, repoPath, remoteRef) {
+	// Best-effort sync: a timeout / error here is treated the same as a
+	// missing remote ref — caller falls back to the local baseBranch.
+	if exists, _ := m.branchExists(ctx, repoPath, remoteRef); exists {
 		m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", remoteRef), "")
 		return remoteRef
 	}

apps/backend/internal/worktree/manager_timeout_test.go

@@ -2,6 +2,7 @@ package worktree
 
 import (
 	"context"
+	"errors"
 	"os"
 	"testing"
 	"time"
@@ -39,12 +40,15 @@ func TestBranchExists_RespectsContextDeadline(t *testing.T) {
 	defer cancel()
 
 	start := time.Now()
-	exists := mgr.branchExists(ctx, t.TempDir(), "main")
+	exists, err := mgr.branchExists(ctx, t.TempDir(), "main")
 	elapsed := time.Since(start)
 
 	if exists {
 		t.Fatalf("branchExists() = true, want false on hanging git")
 	}
+	if err == nil {
+		t.Fatalf("branchExists() err = nil, want non-nil on ctx cancellation")
+	}
 	if elapsed > 2*time.Second {
 		t.Fatalf("branchExists() took %v, want <2s (ctx not propagated to subprocess)", elapsed)
 	}
@@ -94,12 +98,15 @@ func TestBranchExists_BoundedWhenCallerHasNoDeadline(t *testing.T) {
 	mgr.inspectTimeout = 300 * time.Millisecond
 
 	start := time.Now()
-	exists := mgr.branchExists(context.Background(), t.TempDir(), "main")
+	exists, err := mgr.branchExists(context.Background(), t.TempDir(), "main")
 	elapsed := time.Since(start)
 
 	if exists {
 		t.Fatalf("branchExists() = true, want false on hanging git")
 	}
+	if err == nil {
+		t.Fatalf("branchExists() err = nil, want non-nil on inspectTimeout firing")
+	}
 	// Budget = inspectTimeout + WaitDelay for subprocess pipe cleanup + slack.
 	if elapsed > 2*time.Second {
 		t.Fatalf("branchExists() took %v, want <2s (inspectTimeout not applied)", elapsed)
@@ -177,3 +184,45 @@ func TestCreate_HangingRevParseReleasesRepoLock(t *testing.T) {
 		t.Fatalf("second Create() stuck on repo lock (elapsed %v)", time.Since(lockStart))
 	}
 }
+
+// TestCreate_InspectTimeoutDoesNotSurfaceAsInvalidBaseBranch pins the bug
+// where a hung `git rev-parse --verify <base>` (e.g. a stalled NFS / SMB
+// mount) caused branchExists to return false and Create to surface
+// ErrInvalidBaseBranch — falsely claiming the base branch did not exist when
+// in fact the check timed out. Callers must see the underlying timeout, not
+// a misleading "branch not found" error that triggers task FAILED states.
+func TestCreate_InspectTimeoutDoesNotSurfaceAsInvalidBaseBranch(t *testing.T) {
+	scriptDir := writeFakeGitScript(t, hangOnRevParseScript)
+	t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	cfg := newTestConfig(t)
+	mgr, err := NewManager(cfg, newMockStore(), newTestLogger())
+	if err != nil {
+		t.Fatalf("NewManager failed: %v", err)
+	}
+	// Tight inspect timeout so the test completes quickly.
+	mgr.inspectTimeout = 200 * time.Millisecond
+
+	repoPath := t.TempDir()
+	if err := os.MkdirAll(repoPath+"/.git", 0755); err != nil {
+		t.Fatalf("failed to create .git dir: %v", err)
+	}
+
+	req := CreateRequest{
+		TaskID:         "task-a",
+		SessionID:      "sess-a",
+		TaskTitle:      "inspect timeout repro",
+		RepositoryPath: repoPath,
+		BaseBranch:     "master",
+		TaskDirName:    "task-a",
+		RepoName:       "repo-a",
+	}
+
+	_, err = mgr.Create(context.Background(), req)
+	if err == nil {
+		t.Fatalf("Create() err = nil, want timeout error on hanging git rev-parse")
+	}
+	if errors.Is(err, ErrInvalidBaseBranch) {
+		t.Fatalf("Create() err = %v, must NOT be ErrInvalidBaseBranch on inspect timeout (regression: misleading 'base branch does not exist' surfaced to users)", err)
+	}
+}

apps/web/components/diff/use-expandable-diff.test.ts

@@ -0,0 +1,143 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { renderHook, act } from "@testing-library/react";
+import type { FileDiffMetadata } from "@pierre/diffs";
+
+const processFileMock = vi.fn();
+const requestFileContentMock = vi.fn();
+const requestFileContentAtRefMock = vi.fn();
+const getWebSocketClientMock = vi.fn(() => ({}) as unknown);
+
+vi.mock("@pierre/diffs", () => ({
+  processFile: (...args: unknown[]) => processFileMock(...args),
+}));
+vi.mock("@/lib/ws/connection", () => ({
+  getWebSocketClient: () => getWebSocketClientMock(),
+}));
+vi.mock("@/lib/ws/workspace-files", () => ({
+  requestFileContent: (...args: unknown[]) => requestFileContentMock(...args),
+  requestFileContentAtRef: (...args: unknown[]) => requestFileContentAtRefMock(...args),
+}));
+
+type UseExpandableDiffFn = typeof import("./use-expandable-diff").useExpandableDiff;
+
+let useExpandableDiff: UseExpandableDiffFn;
+beforeEach(async () => {
+  vi.resetModules();
+  processFileMock.mockReset();
+  requestFileContentMock.mockReset();
+  requestFileContentAtRefMock.mockReset();
+  getWebSocketClientMock.mockReset().mockReturnValue({});
+  ({ useExpandableDiff } = await import("./use-expandable-diff"));
+});
+
+/** Build a minimal FileDiffMetadata where the trailing-context counts match. */
+function consistentMeta(overrides: Partial<FileDiffMetadata> = {}): FileDiffMetadata {
+  return {
+    name: "src/foo.ts",
+    type: "modified" as FileDiffMetadata["type"],
+    hunks: [
+      {
+        additionStart: 1,
+        additionCount: 3,
+        additionLines: 3,
+        additionLineIndex: 0,
+        deletionStart: 1,
+        deletionCount: 3,
+        deletionLines: 3,
+        deletionLineIndex: 0,
+        hunkContent: [],
+      },
+    ],
+    // 5 lines total, hunk uses 3 starting at 0, leaves 2 trailing on each side
+    additionLines: ["a", "b", "c", "d", "e"],
+    deletionLines: ["a", "b", "c", "d", "e"],
+    splitLineCount: 0,
+    unifiedLineCount: 0,
+    isPartial: false,
+    ...overrides,
+  } as FileDiffMetadata;
+}
+
+/** Build a FileDiffMetadata whose trailing additions vs deletions disagree. */
+function inconsistentMeta(): FileDiffMetadata {
+  const m = consistentMeta();
+  // Same hunk indices/counts, but extend deletionLines so trailing remainders
+  // differ (2 trailing additions vs 4 trailing deletions). This is exactly
+  // the shape @pierre/diffs' iterateOverDiff asserts on and throws over.
+  return { ...m, deletionLines: [...m.deletionLines, "f", "g"] };
+}
+
+const PARTIAL = consistentMeta({ isPartial: true });
+
+const baseProps = {
+  sessionId: "sess-1",
+  filePath: "src/foo.ts",
+  baseRef: "HEAD",
+  diff: "diff --git a/src/foo.ts b/src/foo.ts\n--- a/src/foo.ts\n+++ b/src/foo.ts\n@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n",
+  enableExpansion: true,
+};
+
+async function loadAndSettle(loadContent: () => Promise<void>) {
+  await act(async () => {
+    await loadContent();
+  });
+}
+
+describe("useExpandableDiff", () => {
+  it("returns the partial fileDiffMetadata before content is loaded", () => {
+    const { result } = renderHook(() =>
+      useExpandableDiff({ ...baseProps, fileDiffMetadata: PARTIAL }),
+    );
+    expect(result.current.metadata).toBe(PARTIAL);
+    expect(result.current.isContentLoaded).toBe(false);
+  });
+
+  it("returns the reparsed metadata when trailing-context counts agree", async () => {
+    requestFileContentMock.mockResolvedValue({ content: "new", is_binary: false });
+    requestFileContentAtRefMock.mockResolvedValue({ content: "old", is_binary: false });
+    const reparsed = consistentMeta();
+    processFileMock.mockReturnValue(reparsed);
+
+    const { result } = renderHook(() =>
+      useExpandableDiff({ ...baseProps, fileDiffMetadata: PARTIAL }),
+    );
+    await loadAndSettle(result.current.loadContent);
+    expect(result.current.metadata).toBe(reparsed);
+  });
+
+  it("falls back to the partial metadata when the reparse is inconsistent", async () => {
+    requestFileContentMock.mockResolvedValue({ content: "new", is_binary: false });
+    requestFileContentAtRefMock.mockResolvedValue({ content: "new", is_binary: false });
+    processFileMock.mockReturnValue(inconsistentMeta());
+
+    const { result } = renderHook(() =>
+      useExpandableDiff({ ...baseProps, fileDiffMetadata: PARTIAL }),
+    );
+    await loadAndSettle(result.current.loadContent);
+    expect(result.current.metadata).toBe(PARTIAL);
+    // canExpand must be false: clicking the toolbar button after a rejected
+    // reparse is a silent no-op because the partial metadata can't drive
+    // @pierre/diffs' iterateOverDiff expansion path.
+    expect(result.current.canExpand).toBe(false);
+  });
+
+  it("preserves the lang override from the partial metadata on a successful reparse", async () => {
+    requestFileContentMock.mockResolvedValue({ content: "new", is_binary: false });
+    requestFileContentAtRefMock.mockResolvedValue({ content: "old", is_binary: false });
+    processFileMock.mockReturnValue(consistentMeta({ lang: "go" as never }));
+
+    const overridden = consistentMeta({ isPartial: true, lang: "text" as never });
+    const { result } = renderHook(() =>
+      useExpandableDiff({ ...baseProps, fileDiffMetadata: overridden }),
+    );
+    await loadAndSettle(result.current.loadContent);
+    expect((result.current.metadata as { lang?: string }).lang).toBe("text");
+  });
+
+  it("returns null when no fileDiffMetadata was provided", () => {
+    const { result } = renderHook(() =>
+      useExpandableDiff({ ...baseProps, fileDiffMetadata: null }),
+    );
+    expect(result.current.metadata).toBeNull();
+  });
+});

apps/web/components/diff/use-expandable-diff.ts

@@ -32,6 +32,25 @@ function isFileNotFoundError(error: string): boolean {
   return /file not found|not found|no such file|does not exist/i.test(error);
 }
 
+/**
+ * Validate that a reparsed metadata won't trip @pierre/diffs' iterateOverDiff
+ * trailing-context check (which throws and tears down the renderer). When
+ * fetched contents are out of sync with the patch, processFile still produces
+ * a metadata object whose final-hunk indices leave a different number of
+ * trailing addition vs deletion lines — the exact condition iterateOverDiff
+ * asserts on. Detecting it here lets the caller fall back to the partial
+ * metadata instead of crashing.
+ */
+function isReparseConsistent(meta: FileDiffMetadata): boolean {
+  const lastHunk = meta.hunks?.[meta.hunks.length - 1];
+  if (!lastHunk) return true;
+  const addRemain =
+    meta.additionLines.length - (lastHunk.additionLineIndex + lastHunk.additionCount);
+  const delRemain =
+    meta.deletionLines.length - (lastHunk.deletionLineIndex + lastHunk.deletionCount);
+  return addRemain === delRemain;
+}
+
 /** Fetch old file content at a git ref. Returns empty string for new files. */
 async function fetchOldContent(
   client: WsClient,
@@ -149,18 +168,31 @@ export function useExpandableDiff({
     }
   }, [sessionId, filePath, baseRef, repo, enableExpansion, loadedContent, isLoading]);
 
-  const metadata = useMemo<FileDiffMetadata | null>(() => {
-    if (!fileDiffMetadata) return null;
-    if (!loadedContent || !diff) return fileDiffMetadata;
+  const { metadata, reparseAccepted } = useMemo<{
+    metadata: FileDiffMetadata | null;
+    reparseAccepted: boolean;
+  }>(() => {
+    if (!fileDiffMetadata) return { metadata: null, reparseAccepted: false };
+    if (!loadedContent || !diff) return { metadata: fileDiffMetadata, reparseAccepted: false };
     const reparsed = processFile(diff, {
       oldFile: { name: filePath, contents: loadedContent.oldContent },
       newFile: { name: filePath, contents: loadedContent.newContent },
     });
-    if (!reparsed) return fileDiffMetadata;
+    if (!reparsed) return { metadata: fileDiffMetadata, reparseAccepted: false };
+    // Reject a reparse whose trailing-context lengths don't match: when the
+    // fetched contents are out of sync with the patch (stale snapshot, wrong
+    // base ref for a committed diff, file edited mid-flight), processFile
+    // still returns a metadata object, but @pierre/diffs' iterateOverDiff
+    // will throw "trailing context mismatch" the moment it tries to render.
+    // Falling back to the original partial metadata loses expansion controls
+    // but renders the patch correctly — same as the dedicated Diff tab.
+    if (!isReparseConsistent(reparsed))
+      return { metadata: fileDiffMetadata, reparseAccepted: false };
     // Preserve the lang override that useDiffMetadata sets (e.g. lang:'text'
     // for Go files that hit the Shiki backtracking guard). processFile would
     // otherwise infer "go" from the filename and silently re-enable Shiki.
-    return fileDiffMetadata.lang ? { ...reparsed, lang: fileDiffMetadata.lang } : reparsed;
+    const final = fileDiffMetadata.lang ? { ...reparsed, lang: fileDiffMetadata.lang } : reparsed;
+    return { metadata: final, reparseAccepted: true };
   }, [fileDiffMetadata, loadedContent, diff, filePath]);
 
   const isContentLoaded = loadedContent !== null;
@@ -171,6 +203,9 @@ export function useExpandableDiff({
     isLoading,
     error,
     loadContent,
-    canExpand: enableExpansion && isContentLoaded && !error,
+    // Gate canExpand on a successful reparse so the toolbar button doesn't
+    // appear when we fell back to the partial metadata — clicking it would
+    // be a silent no-op since the metadata can't drive iterateOverDiff.
+    canExpand: enableExpansion && isContentLoaded && !error && reparseAccepted,
   };
 }

apps/web/components/review/review-diff-list.tsx

@@ -374,7 +374,15 @@ function renderDiffContent(opts: {
     // belongs to the local branch, not the PR head — when the same file has
     // both PR and local changes, the wrong content gets paired with the PR
     // patch and @pierre/diffs 1.1.x renders nothing/errors. Disable expansion
-    // for PR-sourced rows; uncommitted/committed rows still get it.
+    // for PR-sourced rows.
+    //
+    // Also disable expansion for committed-source rows: the backend computes
+    // the diff against the session's stored base commit, not HEAD. HEAD
+    // already contains the committed changes, so fetching old content at
+    // baseRef="HEAD" returns the same content as the working tree, leaving
+    // processFile's trailing-context counts inconsistent and crashing the
+    // renderer. Using the right base would need the session's base SHA
+    // plumbed in here; that's a follow-up.
     //
     // Also disable expansion for untracked files: the backend synthesizes a
     // single all-additions hunk against /dev/null, so there is no real
@@ -382,7 +390,7 @@ function renderDiffContent(opts: {
     // races with concurrent edits — if the file shrinks after the snapshot,
     // the cached hunk header advertises more lines than the fetched content
     // has and @pierre/diffs' DiffHunksRenderer throws.
-    const enableExpansion = file.source !== "pr" && file.status !== "untracked";
+    const enableExpansion = file.source === "uncommitted" && file.status !== "untracked";
     return (
       <>
         <DiffErrorBoundary filePath={file.path}>