From 3b1b3473f1c9362c4d7c10d5fdc141ddb11a2282 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 28 May 2026 10:12:40 +0100 Subject: [PATCH] fix(web): avoid trailing-context crash in multi-file diff panel (#1097) --- apps/backend/internal/worktree/manager.go | 43 ++++-- .../internal/worktree/manager_timeout_test.go | 53 ++++++- .../diff/use-expandable-diff.test.ts | 143 ++++++++++++++++++ .../components/diff/use-expandable-diff.ts | 47 +++++- .../components/review/review-diff-list.tsx | 12 +- 5 files changed, 278 insertions(+), 20 deletions(-) create mode 100644 apps/web/components/diff/use-expandable-diff.test.ts diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index d7dce6920c..47c4e772c9 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/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 } diff --git a/apps/backend/internal/worktree/manager_timeout_test.go b/apps/backend/internal/worktree/manager_timeout_test.go index 8c904d53ad..daf9c61605 100644 --- a/apps/backend/internal/worktree/manager_timeout_test.go +++ b/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 ` (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) + } +} diff --git a/apps/web/components/diff/use-expandable-diff.test.ts b/apps/web/components/diff/use-expandable-diff.test.ts new file mode 100644 index 0000000000..9b7c2ae5d5 --- /dev/null +++ b/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 { + 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) { + 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(); + }); +}); diff --git a/apps/web/components/diff/use-expandable-diff.ts b/apps/web/components/diff/use-expandable-diff.ts index 3e511af01e..7cc40171a9 100644 --- a/apps/web/components/diff/use-expandable-diff.ts +++ b/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(() => { - 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, }; } diff --git a/apps/web/components/review/review-diff-list.tsx b/apps/web/components/review/review-diff-list.tsx index 91fafeb515..8a3b1f624e 100644 --- a/apps/web/components/review/review-diff-list.tsx +++ b/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 ( <>