diff --git a/AGENTS.md b/AGENTS.md index fa30584a5..4e3a9aee0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -163,7 +163,9 @@ User runs /plannotator-review command Claude Code: plannotator review subcommand runs OpenCode: event handler intercepts command ↓ -VCS diff captures local changes (git diff or jj diff) +VCS diff captures local changes (git diff or jj diff). When review runs from a +non-VCS parent that contains nested Git repos, child diffs are combined with +folder-prefixed paths. ↓ Review server starts, opens browser with diff viewer ↓ @@ -266,7 +268,7 @@ During normal plan review, an Archive sidebar tab provides the same browsing via | Endpoint | Method | Purpose | | --------------------- | ------ | ------------------------------------------ | -| `/api/diff` | GET | Returns `{ rawPatch, gitRef, origin, diffType, base, hideWhitespace, gitContext }` | +| `/api/diff` | GET | Returns `{ rawPatch, gitRef, origin, mode?, diffType, base, hideWhitespace, gitContext, agentCwd? }`. Workspace mode returns `mode: "workspace"` with folder-prefixed paths and no `gitContext`. | | `/api/diff/switch` | POST | Switch diff type, base branch, or whitespace mode (body: `{ diffType, base?, hideWhitespace? }`) | | `/api/file-content` | GET | Returns `{ oldContent, newContent }` for expandable diff context (`?path=&oldPath=&base=`) | | `/api/git-add` | POST | Stage/unstage a file (body: `{ filePath, undo? }`) | diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index c43c9a02c..29850f09d 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -72,7 +72,7 @@ import { startGoalSetupServer, handleGoalSetupServerReady, } from "@plannotator/server/goal-setup"; -import { type DiffType, prepareLocalReviewDiff, gitRuntime } from "@plannotator/server/vcs"; +import { type DiffType, gitRuntime, prepareLocalReviewDiff, detectManagedVcs } from "@plannotator/server/vcs"; import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; import { parseReviewArgs } from "@plannotator/shared/review-args"; import { @@ -127,6 +127,7 @@ import { } from "./cli"; import path from "path"; import { tmpdir } from "os"; +import { buildLocalWorkspaceReview, type WorkspaceDiffType } from "@plannotator/server/review-workspace"; // Embed the built HTML at compile time // @ts-ignore - Bun import attribute for text @@ -401,10 +402,11 @@ if (args[0] === "sessions") { let diffError: string | undefined; let gitContext: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; - let initialDiffType: DiffType | undefined; + let initialDiffType: DiffType | WorkspaceDiffType | undefined; let agentCwd: string | undefined; let worktreePool: WorktreePool | undefined; let worktreeCleanup: (() => void | Promise) | undefined; + let workspace: Awaited> | undefined; if (isPRMode) { // --- PR Review Mode --- @@ -595,16 +597,35 @@ if (args[0] === "sessions") { } else { // --- Local Review Mode --- const config = loadConfig(); - const diffResult = await prepareLocalReviewDiff({ - vcsType: reviewArgs.vcsType, - configuredDiffType: resolveDefaultDiffType(config), - hideWhitespace: config.diffOptions?.hideWhitespace ?? false, - }); - gitContext = diffResult.gitContext; - initialDiffType = diffResult.diffType; - rawPatch = diffResult.rawPatch; - gitRef = diffResult.gitRef; - diffError = diffResult.error; + const managedVcs = await detectManagedVcs(process.cwd(), reviewArgs.vcsType); + const forcedVcs = !!reviewArgs.vcsType && reviewArgs.vcsType !== "auto"; + + if (managedVcs || forcedVcs) { + const diffResult = await prepareLocalReviewDiff({ + vcsType: reviewArgs.vcsType, + configuredDiffType: resolveDefaultDiffType(config), + hideWhitespace: config.diffOptions?.hideWhitespace ?? false, + }); + gitContext = diffResult.gitContext; + initialDiffType = diffResult.diffType; + rawPatch = diffResult.rawPatch; + gitRef = diffResult.gitRef; + diffError = diffResult.error; + } else { + workspace = await buildLocalWorkspaceReview(process.cwd(), { + configuredDiffType: resolveDefaultDiffType(config), + hideWhitespace: config.diffOptions?.hideWhitespace ?? false, + }); + if (workspace.repos.length === 0) { + console.error("Not in a VCS repo and no nested Git/JJ repositories were found."); + process.exit(1); + } + rawPatch = workspace.rawPatch; + gitRef = workspace.gitRef; + diffError = workspace.error; + initialDiffType = workspace.diffType; + agentCwd = workspace.root; + } } const reviewProject = (await detectProjectName()) ?? "_unknown"; @@ -615,9 +636,10 @@ if (args[0] === "sessions") { gitRef, error: diffError, origin: detectedOrigin, - diffType: gitContext ? (initialDiffType ?? "unstaged") : undefined, + diffType: workspace ? (initialDiffType ?? workspace.diffType) : gitContext ? (initialDiffType ?? "unstaged") : undefined, gitContext, prMetadata, + workspace, agentCwd, worktreePool, sharingEnabled, diff --git a/apps/opencode-plugin/commands.ts b/apps/opencode-plugin/commands.ts index 7a50c75a4..dfea4018d 100644 --- a/apps/opencode-plugin/commands.ts +++ b/apps/opencode-plugin/commands.ts @@ -18,7 +18,7 @@ import { startAnnotateServer, handleAnnotateServerReady, } from "@plannotator/server/annotate"; -import { type DiffType, prepareLocalReviewDiff } from "@plannotator/server/vcs"; +import { type DiffType, prepareLocalReviewDiff, detectManagedVcs } from "@plannotator/server/vcs"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; import { @@ -32,6 +32,7 @@ import { htmlToMarkdown } from "@plannotator/shared/html-to-markdown"; import { parseAnnotateArgs } from "@plannotator/shared/annotate-args"; import { parseReviewArgs } from "@plannotator/shared/review-args"; import { urlToMarkdown, isConvertedSource } from "@plannotator/shared/url-to-markdown"; +import { buildLocalWorkspaceReview, type WorkspaceDiffType } from "@plannotator/server/review-workspace"; import { statSync } from "fs"; import path from "path"; @@ -60,9 +61,11 @@ export async function handleReviewCommand( let rawPatch: string; let gitRef: string; let diffError: string | undefined; - let userDiffType: DiffType | undefined; + let userDiffType: DiffType | WorkspaceDiffType | undefined; let gitContext: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; + let workspace: Awaited> | undefined; + let agentCwd: string | undefined; if (isPRMode) { const prRef = parsePRUrl(urlArg); @@ -94,17 +97,41 @@ export async function handleReviewCommand( client.app.log({ level: "info", message: "Opening code review UI..." }); const config = loadConfig(); - const diffResult = await prepareLocalReviewDiff({ - cwd: directory, - vcsType: reviewArgs.vcsType, - configuredDiffType: resolveDefaultDiffType(config), - hideWhitespace: config.diffOptions?.hideWhitespace ?? false, - }); - gitContext = diffResult.gitContext; - userDiffType = diffResult.diffType; - rawPatch = diffResult.rawPatch; - gitRef = diffResult.gitRef; - diffError = diffResult.error; + const cwd = directory ?? process.cwd(); + const managedVcs = await detectManagedVcs(cwd, reviewArgs.vcsType); + const forcedVcs = !!reviewArgs.vcsType && reviewArgs.vcsType !== "auto"; + if (managedVcs || forcedVcs) { + try { + const diffResult = await prepareLocalReviewDiff({ + cwd, + vcsType: reviewArgs.vcsType, + configuredDiffType: resolveDefaultDiffType(config), + hideWhitespace: config.diffOptions?.hideWhitespace ?? false, + }); + gitContext = diffResult.gitContext; + userDiffType = diffResult.diffType; + rawPatch = diffResult.rawPatch; + gitRef = diffResult.gitRef; + diffError = diffResult.error; + } catch (err) { + client.app.log({ level: "error", message: err instanceof Error ? err.message : "Failed to prepare local review diff" }); + return; + } + } else { + workspace = await buildLocalWorkspaceReview(cwd, { + configuredDiffType: resolveDefaultDiffType(config), + hideWhitespace: config.diffOptions?.hideWhitespace ?? false, + }); + if (workspace.repos.length === 0) { + client.app.log({ level: "error", message: "Not in a VCS repo and no nested Git/JJ repositories were found." }); + return; + } + rawPatch = workspace.rawPatch; + gitRef = workspace.gitRef; + diffError = workspace.error; + userDiffType = workspace.diffType; + agentCwd = workspace.root; + } } const server = await startReviewServer({ @@ -115,6 +142,8 @@ export async function handleReviewCommand( diffType: isPRMode ? undefined : userDiffType, gitContext, prMetadata, + workspace, + agentCwd, sharingEnabled: await getSharingEnabled(), shareBaseUrl: getShareBaseUrl(), htmlContent: reviewHtmlContent, diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index e71b20a24..3bcb21b6b 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -8,11 +8,18 @@ import type { ExtensionContext } from "@earendil-works/pi-coding-agent"; import { prepareLocalReviewDiff, reviewRuntime, + detectManagedVcs, + getVcsContext, + getVcsFileContentsForDiff, + canStageFiles, + runVcsDiff, + stageFile, startAnnotateServer, startPlanReviewServer, startReviewServer, type DiffType, type VcsSelection, + unstageFile, } from "./server.js"; import { openBrowser, isRemoteSession } from "./server/network.js"; import { parsePRUrl, checkPRAuth, fetchPR } from "./server/pr.js"; @@ -26,6 +33,10 @@ import { import { parseRemoteUrl } from "./generated/repo.js"; import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "./generated/worktree.js"; import { loadConfig, resolveDefaultDiffType } from "./generated/config.js"; +import { + WorkspaceReviewSession, + type WorkspaceDiffType, +} from "./generated/review-workspace.js"; export { getLastAssistantMessageText } from "./assistant-message.js"; export type AnnotateMode = "annotate" | "annotate-folder" | "annotate-last"; @@ -89,6 +100,20 @@ function openBrowserForServer(serverUrl: string, ctx: ExtensionContext): void { } } +async function buildLocalWorkspaceReview( + root: string, + options: { requestedDiffType?: DiffType | WorkspaceDiffType; configuredDiffType?: DiffType; hideWhitespace?: boolean } = {}, +): Promise { + return WorkspaceReviewSession.create({ + getVcsContext, + runVcsDiff, + getVcsFileContentsForDiff, + canStageFiles, + stageFile, + unstageFile, + }, root, options); +} + async function openBrowserAndWait( server: { url: string; stop: () => void }, ctx: ExtensionContext, @@ -226,12 +251,13 @@ export async function startCodeReviewBrowserSession( let diffError: string | undefined; let gitCtx: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; - let diffType: DiffType | undefined; + let diffType: DiffType | WorkspaceDiffType | undefined; let agentCwd: string | undefined; let initialBase: string | undefined; let worktreeCleanup: (() => void | Promise) | undefined; let worktreePool: WorktreePool | undefined; let exitHandler: (() => void) | undefined; + let workspace: WorkspaceReviewSession | undefined; if (isPRMode && urlArg) { // --- PR Review Mode --- @@ -399,23 +425,41 @@ export async function startCodeReviewBrowserSession( // --- Local Review Mode --- const cwd = options.cwd ?? ctx.cwd; const config = loadConfig(); - const result = await prepareLocalReviewDiff({ - cwd, - vcsType: options.vcsType, - requestedDiffType: options.diffType, - requestedBase: options.defaultBranch, - configuredDiffType: resolveDefaultDiffType(config), - hideWhitespace: config.diffOptions?.hideWhitespace ?? false, - }); - gitCtx = result.gitContext; - diffType = result.diffType; - rawPatch = result.rawPatch; - gitRef = result.gitRef; - diffError = result.error; - // Remember which base the initial diff was computed against so it can - // be forwarded to the server below. Only matters when the caller - // overrode the detected default; otherwise it matches gitCtx already. - initialBase = result.base; + const managedVcs = await detectManagedVcs(cwd, options.vcsType); + const forcedVcs = !!options.vcsType && options.vcsType !== "auto"; + if (managedVcs || forcedVcs) { + const result = await prepareLocalReviewDiff({ + cwd, + vcsType: options.vcsType, + requestedDiffType: options.diffType, + requestedBase: options.defaultBranch, + configuredDiffType: resolveDefaultDiffType(config), + hideWhitespace: config.diffOptions?.hideWhitespace ?? false, + }); + gitCtx = result.gitContext; + diffType = result.diffType; + rawPatch = result.rawPatch; + gitRef = result.gitRef; + diffError = result.error; + // Remember which base the initial diff was computed against so it can + // be forwarded to the server below. Only matters when the caller + // overrode the detected default; otherwise it matches gitCtx already. + initialBase = result.base; + } else { + workspace = await buildLocalWorkspaceReview(cwd, { + requestedDiffType: options.diffType, + configuredDiffType: resolveDefaultDiffType(config), + hideWhitespace: config.diffOptions?.hideWhitespace ?? false, + }); + if (workspace.repos.length === 0) { + throw new Error("Not in a VCS repo and no nested Git/JJ repositories were found."); + } + rawPatch = workspace.rawPatch; + gitRef = workspace.gitRef; + diffError = workspace.error; + diffType = workspace.diffType; + agentCwd = workspace.root; + } } const server = await startReviewServer({ @@ -427,6 +471,7 @@ export async function startCodeReviewBrowserSession( gitContext: gitCtx, initialBase, prMetadata, + workspace, agentCwd, worktreePool, htmlContent: reviewHtmlContent, diff --git a/apps/pi-extension/server.test.ts b/apps/pi-extension/server.test.ts index 3981ee565..be371fb5d 100644 --- a/apps/pi-extension/server.test.ts +++ b/apps/pi-extension/server.test.ts @@ -1,16 +1,22 @@ import { afterEach, describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { createServer as createNetServer } from "node:net"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { + canStageFiles, getGitContext, getVcsContext, + getVcsFileContentsForDiff, prepareLocalReviewDiff, runGitDiff, + runVcsDiff, + stageFile, startReviewServer, + unstageFile, } from "./server"; +import { WorkspaceReviewSession } from "./generated/review-workspace.js"; const tempDirs: string[] = []; const originalCwd = process.cwd(); @@ -358,6 +364,102 @@ describe("pi review server", () => { } }, 15_000); + test("workspace mode maps prefixed paths to child repos", async () => { + const homeDir = makeTempDir("plannotator-pi-home-"); + const root = makeTempDir("plannotator-pi-workspace-"); + const apiDir = join(root, "api"); + mkdirSync(apiDir, { recursive: true }); + process.env.HOME = homeDir; + process.env.PLANNOTATOR_PORT = String(await reservePort()); + + git(apiDir, ["init"]); + git(apiDir, ["branch", "-M", "main"]); + git(apiDir, ["config", "user.email", "pi-review@example.com"]); + git(apiDir, ["config", "user.name", "Pi Review"]); + writeFileSync(join(apiDir, "tracked.txt"), "before\n", "utf-8"); + git(apiDir, ["add", "tracked.txt"]); + git(apiDir, ["commit", "-m", "initial"]); + writeFileSync(join(apiDir, "tracked.txt"), "after\n", "utf-8"); + + const workspace = await WorkspaceReviewSession.create({ + getVcsContext, + runVcsDiff, + getVcsFileContentsForDiff, + canStageFiles, + stageFile, + unstageFile, + }, root); + + const server = await startReviewServer({ + rawPatch: workspace.rawPatch, + gitRef: workspace.gitRef, + error: workspace.error, + diffType: workspace.diffType, + origin: "pi", + htmlContent: "review", + workspace, + agentCwd: root, + }); + + try { + const diffResponse = await fetch(`${server.url}/api/diff`); + const diffPayload = await diffResponse.json() as { + mode?: string; + agentCwd?: string; + diffType?: string; + diffOptions?: Array<{ id: string }>; + }; + expect(diffPayload.mode).toBe("workspace"); + expect(diffPayload.diffType).toBe("workspace-current"); + expect(diffPayload.diffOptions?.map((option) => option.id)).toContain("workspace-last"); + expect(diffPayload.agentCwd).toBe(root); + expect("workspace" in diffPayload).toBe(false); + + const switchResponse = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "workspace-last", hideWhitespace: true }), + }); + expect(switchResponse.status).toBe(200); + const switched = await switchResponse.json() as { diffType?: string; diffOptions?: Array<{ id: string }> }; + expect(switched.diffType).toBe("workspace-last"); + expect(switched.diffOptions?.map((option) => option.id)).toContain("workspace-current"); + + const currentResponse = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "workspace-current", hideWhitespace: false }), + }); + expect(currentResponse.status).toBe(200); + + const fileContentResponse = await fetch(`${server.url}/api/file-content?path=api/tracked.txt`); + expect(fileContentResponse.status).toBe(200); + const fileContent = await fileContentResponse.json() as { + oldContent: string | null; + newContent: string | null; + }; + expect(fileContent.oldContent).toBe("before\n"); + expect(fileContent.newContent).toBe("after\n"); + + const stageResponse = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "api/tracked.txt" }), + }); + expect(stageResponse.status).toBe(200); + expect(git(apiDir, ["diff", "--staged", "--name-only"])).toContain("tracked.txt"); + + const invalidStageResponse = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "api/../tracked.txt" }), + }); + expect(invalidStageResponse.status).toBe(400); + } finally { + server.stop(); + } + }, 15_000); + test("round-trips the active base branch through /api/diff and /api/diff/switch", async () => { const homeDir = makeTempDir("plannotator-pi-home-"); const repoDir = initRepo(); diff --git a/apps/pi-extension/server.ts b/apps/pi-extension/server.ts index bbdc3c83a..b1ad59522 100644 --- a/apps/pi-extension/server.ts +++ b/apps/pi-extension/server.ts @@ -11,6 +11,7 @@ export type { DiffType, GitContext, } from "./generated/review-core.js"; +export type { WorkspaceDiffType } from "./generated/review-workspace.js"; export type { VcsSelection } from "./server/vcs.js"; export { type AnnotateServerResult, @@ -26,6 +27,7 @@ export { } from "./server/serverReview.js"; export { canStageFiles, + detectManagedVcs, detectRemoteDefaultCompareTarget, detectVcs, getGitContext, diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index f3436f016..7dc78f3c6 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -2,6 +2,7 @@ import { spawn } from "node:child_process"; import { readFileSync, existsSync } from "node:fs"; import { createServer } from "node:http"; import os from "node:os"; +import { basename } from "node:path"; import { contentHash, deleteDraft } from "../generated/draft.js"; import { loadConfig, saveConfig, detectGitUser, getServerConfig } from "../generated/config.js"; @@ -75,7 +76,7 @@ import { parseCodexOutput, transformReviewFindings, } from "../generated/codex-review.js"; -import { buildAgentReviewUserMessage } from "../generated/agent-review-message.js"; +import { buildAgentReviewUserMessage, buildAgentReviewUserMessageForTarget, type WorkspaceReviewPromptContext } from "../generated/agent-review-message.js"; import { CLAUDE_REVIEW_PROMPT, buildClaudeCommand, @@ -83,6 +84,10 @@ import { transformClaudeFindings, } from "../generated/claude-review.js"; import { createTourSession, TOUR_EMPTY_OUTPUT_ERROR } from "../generated/tour-review.js"; +import { + WorkspaceReviewSession, + type WorkspaceDiffType, +} from "../generated/review-workspace.js"; import { type CodeNavRequest, type CodeNavRuntime, @@ -166,7 +171,7 @@ export async function startReviewServer(options: { gitRef: string; htmlContent: string; origin?: string; - diffType?: DiffType; + diffType?: DiffType | WorkspaceDiffType; gitContext?: GitContext; /** * Initial base branch the caller used to compute `rawPatch`. When a caller @@ -183,6 +188,8 @@ export async function startReviewServer(options: { prMetadata?: PRMetadata; /** Working directory for agent processes (e.g., --local worktree). Independent of diff pipeline. */ agentCwd?: string; + /** Local parent directory containing multiple child VCS repositories. */ + workspace?: WorkspaceReviewSession; /** Per-PR worktree pool. When set, pr-switch creates worktrees instead of checking out. */ worktreePool?: WorktreePool; /** Cleanup callback invoked when server stops (e.g., remove temp worktree) */ @@ -194,6 +201,8 @@ export async function startReviewServer(options: { let draftKey = contentHash(options.rawPatch); let prMeta = options.prMetadata; const isPRMode = !!prMeta; + const workspace = options.workspace; + const isWorkspaceMode = !!workspace; const hasLocalAccess = !!options.gitContext; const sessionVcsType = options.gitContext?.vcsType; const isRemote = isRemoteSession(); @@ -245,13 +254,15 @@ export async function startReviewServer(options: { display: getDisplayRepo(prMeta), branch: `${getMRLabel(prMeta)} ${getMRNumberLabel(prMeta)}`, } + : workspace + ? { display: basename(workspace.root), branch: "Workspace" } : getRepoInfo(); const editorAnnotations = createEditorAnnotationHandler(); const externalAnnotations = createExternalAnnotationHandler("review"); let currentPatch = options.rawPatch; let currentGitRef = options.gitRef; - let currentDiffType: DiffType = options.diffType || "uncommitted"; + let currentDiffType: DiffType | WorkspaceDiffType = options.diffType || workspace?.diffType || "uncommitted"; let currentError = options.error; let currentHideWhitespace = loadConfig().diffOptions?.hideWhitespace ?? false; let originalPRPatch = options.rawPatch; @@ -277,12 +288,17 @@ export async function startReviewServer(options: { // Agent jobs — background process manager (late-binds serverUrl via getter) let serverUrl = ""; function resolveAgentCwd(): string { + if (workspace) return workspace.root; if (options.worktreePool && prMeta) { const poolPath = options.worktreePool.resolve(prMeta.url); if (poolPath) return poolPath; } if (options.agentCwd) return options.agentCwd; - return resolveVcsCwd(currentDiffType, options.gitContext?.cwd) ?? process.cwd(); + return resolveVcsCwd(currentDiffType as DiffType, options.gitContext?.cwd) ?? process.cwd(); + } + function getWorkspacePromptContext(): WorkspaceReviewPromptContext | undefined { + if (!workspace) return undefined; + return workspace.getPromptContext(); } const tour = createTourSession(); @@ -293,18 +309,26 @@ export async function startReviewServer(options: { async buildCommand(provider, config) { const cwd = resolveAgentCwd(); - const hasAgentLocalAccess = !!options.worktreePool || !!options.agentCwd || !!options.gitContext; - const userMessageOptions = { defaultBranch: currentBase, hasLocalAccess: hasAgentLocalAccess, prDiffScope: currentPRDiffScope }; + const workspacePrompt = getWorkspacePromptContext(); + const hasAgentLocalAccess = !!workspacePrompt || !!options.worktreePool || !!options.agentCwd || !!options.gitContext; + const userMessageOptions = { + defaultBranch: currentBase, + hasLocalAccess: hasAgentLocalAccess, + prDiffScope: currentPRDiffScope, + ...(workspacePrompt && { workspace: workspacePrompt }), + }; // Snapshot the diff context at launch (see review.ts buildCommand // for the rationale — keeps downstream "Copy All" honest across // subsequent context switches). - const worktreeParts = currentDiffType.startsWith("worktree:") - ? parseWorktreeDiffType(currentDiffType) + const worktreeParts = String(currentDiffType).startsWith("worktree:") + ? parseWorktreeDiffType(currentDiffType as DiffType) : null; const launchPrUrl = prMeta?.url; const launchDiffScope = isPRMode ? currentPRDiffScope : undefined; - const diffContext: AgentJobInfo["diffContext"] | undefined = prMeta + const diffContext: AgentJobInfo["diffContext"] | undefined = workspacePrompt + ? { mode: "workspace", worktreePath: null } + : prMeta ? undefined : { mode: (worktreeParts?.subType ?? currentDiffType) as string, @@ -316,7 +340,7 @@ export async function startReviewServer(options: { const built = await tour.buildCommand({ cwd, patch: currentPatch, - diffType: currentDiffType, + diffType: currentDiffType as DiffType, options: userMessageOptions, prMetadata: prMeta, config, @@ -324,7 +348,14 @@ export async function startReviewServer(options: { return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext } : built; } - const userMessage = buildAgentReviewUserMessage(currentPatch, currentDiffType, userMessageOptions, prMeta); + const userMessage = workspacePrompt + ? buildAgentReviewUserMessageForTarget({ + kind: "workspace", + patch: currentPatch, + workspace: workspacePrompt, + }) + : buildAgentReviewUserMessage(currentPatch, currentDiffType as DiffType, userMessageOptions, prMeta); + const jobLabel = workspacePrompt ? "Workspace Review" : "Code Review"; if (provider === "codex") { const model = typeof config?.model === "string" && config.model ? config.model : undefined; @@ -333,7 +364,7 @@ export async function startReviewServer(options: { const outputPath = generateOutputPath(); const prompt = CODEX_REVIEW_SYSTEM_PROMPT + "\n\n---\n\n" + userMessage; const command = await buildCodexCommand({ cwd, outputPath, prompt, model, reasoningEffort, fastMode }); - return { command, outputPath, prompt, cwd, label: "Code Review", model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; + return { command, outputPath, prompt, cwd, label: jobLabel, model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; } if (provider === "claude") { @@ -341,7 +372,7 @@ export async function startReviewServer(options: { const effort = typeof config?.effort === "string" && config.effort ? config.effort : undefined; const prompt = CLAUDE_REVIEW_PROMPT + "\n\n---\n\n" + userMessage; const { command, stdinPrompt } = buildClaudeCommand(prompt, model, effort); - return { command, stdinPrompt, prompt, cwd, label: "Code Review", captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; + return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; } return null; @@ -371,7 +402,13 @@ export async function startReviewServer(options: { }; if (output.findings.length > 0) { - const annotations = transformReviewFindings(output.findings, job.source, cwd, "Codex") + const annotations = transformReviewFindings( + output.findings, + job.source, + cwd, + "Codex", + workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined, + ) .map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) })); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[codex-review] addAnnotations error:`, result.error); @@ -394,7 +431,12 @@ export async function startReviewServer(options: { }; if (output.findings.length > 0) { - const annotations = transformClaudeFindings(output.findings, job.source, cwd) + const annotations = transformClaudeFindings( + output.findings, + job.source, + cwd, + workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined, + ) .map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) })); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[claude-review] addAnnotations error:`, result.error); @@ -476,11 +518,13 @@ export async function startReviewServer(options: { rawPatch: currentPatch, gitRef: currentGitRef, origin: options.origin ?? "pi", - diffType: hasLocalAccess ? currentDiffType : undefined, + mode: isWorkspaceMode ? "workspace" : undefined, + diffType: hasLocalAccess || isWorkspaceMode ? currentDiffType : undefined, // Echo the active base so page refresh/reconnect rehydrates the // picker to what the server is actually using, not the detected default. base: hasLocalAccess ? currentBase : undefined, hideWhitespace: currentHideWhitespace, + ...(workspace && { diffOptions: workspace.diffOptions }), gitContext: hasLocalAccess ? options.gitContext : undefined, sharingEnabled, shareBaseUrl, @@ -488,6 +532,7 @@ export async function startReviewServer(options: { repoInfo, isWSL: wslFlag, ...(options.agentCwd && { agentCwd: options.agentCwd }), + ...(workspace && { agentCwd: workspace.root }), ...(isPRMode && { prMetadata: prMeta, platformUser, @@ -501,13 +546,13 @@ export async function startReviewServer(options: { serverConfig: getServerConfig(gitUser), }); } else if (url.pathname === "/api/diff/switch" && req.method === "POST") { - if (!hasLocalAccess) { + if (!hasLocalAccess && !workspace) { json(res, { error: "Not available without local file access" }, 400); return; } try { const body = await parseBody(req); - const newType = body.diffType as DiffType; + const newType = body.diffType as DiffType | WorkspaceDiffType; if (!newType) { json(res, { error: "Missing diffType" }, 400); return; @@ -515,13 +560,34 @@ export async function startReviewServer(options: { if (typeof body.hideWhitespace === "boolean") { currentHideWhitespace = body.hideWhitespace; } + if (workspace) { + const snapshot = await workspace.rebuild({ + diffType: newType, + hideWhitespace: currentHideWhitespace, + }); + currentPatch = snapshot.rawPatch; + currentGitRef = snapshot.gitRef; + currentDiffType = workspace.diffType; + currentError = snapshot.error; + draftKey = contentHash(currentPatch); + + json(res, { + rawPatch: currentPatch, + gitRef: currentGitRef, + diffType: currentDiffType, + diffOptions: workspace.diffOptions, + hideWhitespace: currentHideWhitespace, + ...(currentError ? { error: currentError } : {}), + }); + return; + } const detectedBase = detectedCompareTarget(); const base = resolveBaseBranch( typeof body.base === "string" ? body.base : undefined, detectedBase, ); const defaultCwd = options.gitContext?.cwd; - const result = await runVcsDiff(newType, base, defaultCwd, { + const result = await runVcsDiff(newType as DiffType, base, defaultCwd, { hideWhitespace: currentHideWhitespace, }); currentPatch = result.patch; @@ -537,7 +603,7 @@ export async function startReviewServer(options: { let updatedContext: GitContext | undefined; if (options.gitContext) { try { - const effectiveCwd = resolveVcsCwd(newType, options.gitContext.cwd); + const effectiveCwd = resolveVcsCwd(newType as DiffType, options.gitContext.cwd); updatedContext = await getVcsContext(effectiveCwd, sessionVcsType); } catch { /* best-effort */ @@ -821,6 +887,20 @@ export async function startReviewServer(options: { } } + if (workspace) { + try { + const result = await workspace.getFileContents(filePath, oldPath); + json(res, result); + } catch (error) { + json( + res, + { error: error instanceof Error ? error.message : "No file access available" }, + 400, + ); + } + return; + } + const fileContentCwd = (options.worktreePool && prMeta) ? options.worktreePool.resolve(prMeta.url) : options.agentCwd; if ( isPRMode && @@ -858,7 +938,7 @@ export async function startReviewServer(options: { ); const defaultCwd = options.gitContext?.cwd; const result = await getVcsFileContentsForDiff( - currentDiffType, + currentDiffType as DiffType, base, filePath, oldPath, @@ -894,7 +974,7 @@ export async function startReviewServer(options: { json(res, { error: "No file access available" }, 400); } else if (url.pathname === "/api/code-nav/resolve" && req.method === "POST") { - const hasCodeNavAccess = !!options.gitContext || !!options.agentCwd || !!options.worktreePool; + const hasCodeNavAccess = !!workspace || !!options.gitContext || !!options.agentCwd || !!options.worktreePool; if (!hasCodeNavAccess) { json(res, { error: "Code navigation requires local access" }, 400); return; @@ -914,7 +994,7 @@ export async function startReviewServer(options: { json(res, { error: err instanceof Error ? err.message : "Code navigation failed" }, 500); } } else if (url.pathname === "/api/code-nav/file" && req.method === "GET") { - const hasCodeNavAccess = !!options.gitContext || !!options.agentCwd || !!options.worktreePool; + const hasCodeNavAccess = !!workspace || !!options.gitContext || !!options.agentCwd || !!options.worktreePool; if (!hasCodeNavAccess) { json(res, { error: "Code navigation requires local access" }, 400); return; @@ -954,22 +1034,45 @@ export async function startReviewServer(options: { } else if (url.pathname === "/api/agents" && req.method === "GET") { json(res, { agents: [] }); } else if (url.pathname === "/api/git-add" && req.method === "POST") { - const stageCwd = resolveVcsCwd(currentDiffType, options.gitContext?.cwd); - if (isPRMode || !(await canStageFiles(currentDiffType, stageCwd))) { - json(res, { error: "Staging not available" }, 400); - return; - } try { const body = await parseBody(req); const filePath = body.filePath as string | undefined; - if (!filePath) { + if (typeof filePath !== "string" || !filePath) { json(res, { error: "Missing filePath" }, 400); return; } - if (body.undo) { - await unstageFile(currentDiffType, filePath, stageCwd); + try { + validateFilePath(filePath); + } catch { + json(res, { error: "Invalid path" }, 400); + return; + } + const undo = body.undo === true; + + if (workspace) { + try { + await workspace.stageFile(filePath, undo); + json(res, { ok: true }); + } catch (error) { + json( + res, + { error: error instanceof Error ? error.message : "Failed to stage file" }, + 400, + ); + } + return; + } + + const stageCwd = resolveVcsCwd(currentDiffType as DiffType, options.gitContext?.cwd); + if (isPRMode || !(await canStageFiles(currentDiffType as DiffType, stageCwd))) { + json(res, { error: "Staging not available" }, 400); + return; + } + + if (undo) { + await unstageFile(currentDiffType as DiffType, filePath, stageCwd); } else { - await stageFile(currentDiffType, filePath, stageCwd); + await stageFile(currentDiffType as DiffType, filePath, stageCwd); } json(res, { ok: true }); } catch (err) { diff --git a/apps/pi-extension/server/vcs.ts b/apps/pi-extension/server/vcs.ts index 628b72e38..f72e1da58 100644 --- a/apps/pi-extension/server/vcs.ts +++ b/apps/pi-extension/server/vcs.ts @@ -92,6 +92,7 @@ const api = createVcsApi([ export const { detectVcs, + detectManagedVcs, getVcsContext, detectRemoteDefaultCompareTarget, prepareLocalReviewDiff, diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 1c95e2b70..34dfd747c 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -7,7 +7,7 @@ cd "$(dirname "$0")" rm -rf generated mkdir -p generated generated/ai/providers -for f in feedback-templates prompts review-core jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file config external-annotation agent-jobs worktree worktree-pool html-to-markdown url-to-markdown tour annotate-args at-reference pfm-reminder improvement-hooks code-nav; do +for f in feedback-templates prompts review-core jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file config external-annotation agent-jobs worktree worktree-pool html-to-markdown url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav; do src="../../packages/shared/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/shared/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index dde907718..7b2c31047 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -56,6 +56,7 @@ import { usePRSession, type PRSessionUpdate } from './hooks/usePRSession'; import { useAnnotationFactory } from './hooks/useAnnotationFactory'; import { DEMO_DIFF } from './demoData'; import { exportReviewFeedback } from './utils/exportFeedback'; +import { parseDiffToFiles } from './utils/diffParser'; import { ReviewSubmissionDialog, buildReviewSubmission, type ReviewSubmission, type SubmissionTarget } from './components/ReviewSubmissionDialog'; import { ReviewStateProvider, type ReviewState } from './dock/ReviewStateContext'; import { JobLogsProvider } from './dock/JobLogsContext'; @@ -91,45 +92,13 @@ interface DiffData { origin?: Origin; diffType?: string; gitContext?: GitContext; + diffOptions?: DiffOption[]; sharingEnabled?: boolean; prStackInfo?: PRStackInfo | null; prDiffScope?: PRDiffScope; prDiffScopeOptions?: PRDiffScopeOption[]; } -// Simple diff parser to extract files from unified diff -function parseDiffToFiles(rawPatch: string): DiffFile[] { - const files: DiffFile[] = []; - const fileChunks = rawPatch.split(/^diff --git /m).filter(Boolean); - - for (const chunk of fileChunks) { - const lines = chunk.split('\n'); - const headerMatch = lines[0]?.match(/a\/(.+) b\/(.+)/); - if (!headerMatch) continue; - - const oldPath = headerMatch[1]; - const newPath = headerMatch[2]; - - let additions = 0; - let deletions = 0; - - for (const line of lines) { - if (line.startsWith('+') && !line.startsWith('+++')) additions++; - if (line.startsWith('-') && !line.startsWith('---')) deletions++; - } - - files.push({ - path: newPath, - oldPath: oldPath !== newPath ? oldPath : undefined, - patch: 'diff --git ' + chunk, - additions, - deletions, - }); - } - - return files; -} - function getFileTabTitle(filePath: string): string { return filePath.split('/').pop() ?? filePath; } @@ -186,8 +155,10 @@ const ReviewApp: React.FC = () => { const [origin, setOrigin] = useState(null); const [gitUser, setGitUser] = useState(); const [isWSL, setIsWSL] = useState(false); + const [reviewMode, setReviewMode] = useState(null); const [diffType, setDiffType] = useState('uncommitted'); const [gitContext, setGitContext] = useState(null); + const [workspaceDiffOptions, setWorkspaceDiffOptions] = useState(null); // Two bases: // selectedBase — what the picker is currently showing (UI intent). // Updates immediately when the user picks, so the chip @@ -364,6 +335,7 @@ const ReviewApp: React.FC = () => { const hasSearchableFiles = files.length > 0; const shouldShowFileTree = hasSearchableFiles || + (reviewMode === 'workspace' && !!workspaceDiffOptions?.length) || !!gitContext?.diffOptions?.length || !!gitContext?.worktrees?.length; @@ -791,9 +763,11 @@ const ReviewApp: React.FC = () => { rawPatch: string; gitRef: string; origin?: Origin; + mode?: string; diffType?: string; base?: string; gitContext?: GitContext; + diffOptions?: DiffOption[]; agentCwd?: string; sharingEnabled?: boolean; repoInfo?: { display: string; branch?: string }; @@ -820,9 +794,12 @@ const ReviewApp: React.FC = () => { origin: data.origin, diffType: data.diffType, gitContext: data.gitContext, + diffOptions: data.diffOptions, sharingEnabled: data.sharingEnabled, }); setFiles(apiFiles); + setReviewMode(data.mode ?? null); + setWorkspaceDiffOptions(data.mode === 'workspace' ? (data.diffOptions ?? []) : null); if (data.origin) setOrigin(data.origin); if (data.diffType) setDiffType(data.diffType); if (data.gitContext) { @@ -852,7 +829,7 @@ const ReviewApp: React.FC = () => { if (data.error) setDiffError(data.error); if (data.isWSL) setIsWSL(true); // Mark diff type setup as pending on first run (local mode only) - if (data.diffType && !data.prMetadata && data.gitContext?.vcsType !== 'p4' && data.gitContext?.vcsType !== 'jj' && needsDiffTypeSetup()) { + if (data.diffType && data.mode !== 'workspace' && !data.prMetadata && data.gitContext && data.gitContext.vcsType !== 'p4' && data.gitContext.vcsType !== 'jj' && needsDiffTypeSetup()) { setDiffTypeSetupPending(true); } }) @@ -865,6 +842,7 @@ const ReviewApp: React.FC = () => { gitRef: 'demo', }); setFiles(demoFiles); + setWorkspaceDiffOptions(null); }) .finally(() => setIsLoading(false)); }, []); @@ -1097,7 +1075,8 @@ const ReviewApp: React.FC = () => { onFileViewed: handleFileViewedFromStage, }); // Staging is never available in PR review mode — the server rejects it and the UI shouldn't offer it. - const canStageFiles = canStageRaw && !prMetadata; + const canStageInWorkspace = reviewMode !== 'workspace' || workspaceDiffOptions?.some((option) => option.id === 'workspace-staged'); + const canStageFiles = canStageRaw && !prMetadata && canStageInWorkspace; useEffect(() => { const handler = (e: KeyboardEvent) => { @@ -1184,6 +1163,7 @@ const ReviewApp: React.FC = () => { diffType: string; base?: string; gitContext?: GitContext; + diffOptions?: DiffOption[]; error?: string; }; @@ -1194,6 +1174,7 @@ const ReviewApp: React.FC = () => { // If the current file was removed (whitespace-only), retarget the // dock panel to the first remaining file. setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef } : prev); + if (data.diffOptions) setWorkspaceDiffOptions(data.diffOptions); setFiles(nextFiles); const currentPath = files[activeFileIndex]?.path; const nextIdx = currentPath ? nextFiles.findIndex(f => f.path === currentPath) : -1; @@ -1209,6 +1190,7 @@ const ReviewApp: React.FC = () => { setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef, diffType: data.diffType } : prev); setFiles(nextFiles); setDiffType(data.diffType); + if (data.diffOptions) setWorkspaceDiffOptions(data.diffOptions); if (data.base) { setSelectedBase(data.base); setCommittedBase(data.base); @@ -1311,13 +1293,13 @@ const ReviewApp: React.FC = () => { // Preserves the active file since only whitespace hunks change. const hideWhitespaceInitialized = useRef(false); useEffect(() => { - if (!origin || !gitContext) return; + if (!origin || (!gitContext && reviewMode !== 'workspace')) return; if (!hideWhitespaceInitialized.current) { hideWhitespaceInitialized.current = true; return; } fetchDiffSwitch(diffType, selectedBase, { preserveFile: true }); - }, [diffHideWhitespace, origin]); // eslint-disable-line react-hooks/exhaustive-deps + }, [diffHideWhitespace, origin, reviewMode]); // eslint-disable-line react-hooks/exhaustive-deps // Select annotation - switches file if needed and scrolls to it const handleSelectAnnotation = useCallback((id: string | null) => { @@ -1987,6 +1969,15 @@ const ReviewApp: React.FC = () => { )} + {reviewMode === 'workspace' && diffError && files.length > 0 && ( +
+ Some workspace changes could not be loaded +
+ )} + {/* Agent mode: Close/SendFeedback flip + Approve */} {!platformMode ? ( { hideViewedFiles={hideViewedFiles} onToggleHideViewed={() => setHideViewedFiles(prev => !prev)} enableKeyboardNav={!showExportModal && hasSearchableFiles} - diffOptions={gitContext?.diffOptions} + diffOptions={reviewMode === 'workspace' ? (workspaceDiffOptions ?? undefined) : gitContext?.diffOptions} activeDiffType={activeDiffBase} onSelectDiff={handleDiffSwitch} isLoadingDiff={isLoadingDiff} @@ -2250,6 +2241,10 @@ const ReviewApp: React.FC = () => { {activeDiffBase === 'last-commit' && `No changes in the last commit${activeWorktreePath ? ' in this worktree' : ''}.`} {activeDiffBase === 'jj-current' && "No changes in the current jj change."} {activeDiffBase === 'jj-last' && "No changes in the last jj change."} + {activeDiffBase === 'workspace-current' && "No current changes in the workspace repositories."} + {activeDiffBase === 'workspace-staged' && "No staged changes in the workspace repositories."} + {activeDiffBase === 'workspace-unstaged' && "No unstaged changes in the workspace repositories."} + {activeDiffBase === 'workspace-last' && "No changes in the last change across workspace repositories."} {activeDiffBase === 'jj-line' && `No changes in your line of work vs ${selectedBase || gitContext?.defaultBranch || '@-'}.`} {activeDiffBase === 'jj-evolog' && `No changes since evolution ${selectedBase ? selectedBase.slice(0, 8) : 'previous'} — the change looks the same as before.`} {activeDiffBase === 'jj-all' && "No files at the current jj change."} @@ -2260,7 +2255,7 @@ const ReviewApp: React.FC = () => { )} - {gitContext?.diffOptions && gitContext.diffOptions.length > 1 && ( + {((reviewMode === 'workspace' ? workspaceDiffOptions : gitContext?.diffOptions)?.length ?? 0) > 1 && (

Try selecting a different view from the dropdown.

diff --git a/packages/review-editor/components/DiffTypePicker.tsx b/packages/review-editor/components/DiffTypePicker.tsx index c8f7e0e84..52b1d3f2e 100644 --- a/packages/review-editor/components/DiffTypePicker.tsx +++ b/packages/review-editor/components/DiffTypePicker.tsx @@ -20,6 +20,10 @@ const OPTION_HINTS: Record = { staged: "Only what you've run `git add` on.", unstaged: "What `git diff` shows with no arguments.", 'last-commit': "Just your most recent commit.", + 'workspace-current': "Current local changes in every workspace repo.", + 'workspace-staged': "Staged Git changes in every workspace repo.", + 'workspace-unstaged': "Unstaged Git changes in every workspace repo.", + 'workspace-last': "The last committed Git change or previous jj change in every workspace repo.", 'jj-current': "Changes in the current jj change.", 'jj-last': "Changes in the previous jj change.", 'jj-line': "Changes in your line of work from the selected bookmark or revision.", diff --git a/packages/review-editor/hooks/useGitAdd.ts b/packages/review-editor/hooks/useGitAdd.ts index bb544d3ea..c4948dd5c 100644 --- a/packages/review-editor/hooks/useGitAdd.ts +++ b/packages/review-editor/hooks/useGitAdd.ts @@ -14,7 +14,7 @@ interface UseGitAddReturn { stageError: string | null; } -const STAGEABLE_DIFF_TYPES = new Set(['uncommitted', 'unstaged']); +const STAGEABLE_DIFF_TYPES = new Set(['uncommitted', 'unstaged', 'workspace-current', 'workspace-unstaged']); export function useGitAdd({ activeDiffBase, onFileViewed }: UseGitAddOptions): UseGitAddReturn { const [stagedFiles, setStagedFiles] = useState>(new Set()); diff --git a/packages/review-editor/utils/buildFileTree.workspace.test.ts b/packages/review-editor/utils/buildFileTree.workspace.test.ts new file mode 100644 index 000000000..c3d22c3e1 --- /dev/null +++ b/packages/review-editor/utils/buildFileTree.workspace.test.ts @@ -0,0 +1,243 @@ +import { describe, it, expect } from "bun:test"; +import { buildFileTree, getAncestorPaths, getAllFolderPaths } from "./buildFileTree"; +import type { DiffFile } from "../types"; + +const diffFile = (path: string, overrides: Partial = {}): DiffFile => ({ + path, + patch: "", + additions: 0, + deletions: 0, + ...overrides, +}); + +describe("buildFileTree - workspace mode with repo-prefixed paths", () => { + it("builds separate trees for different repo prefixes", () => { + const files = [ + diffFile("repo-a/src/index.ts"), + diffFile("repo-b/src/index.ts"), + ]; + const tree = buildFileTree(files); + + // With flat fallback: single root folder with only file children gets unwrapped + // But here we have two repos at root level, so they stay as folders + expect(tree.length).toBeGreaterThanOrEqual(2); + // After collapseSingleChild, paths like repo-a/src/index.ts become: + // folder: "repo-a/src" with file child "index.ts" + const names = tree.map(n => n.name); + expect(names).toContain("repo-a/src"); + expect(names).toContain("repo-b/src"); + }); + + it("handles same relative paths in different repos", () => { + const files = [ + diffFile("repo-a/src/utils/helper.ts", { additions: 5, deletions: 2 }), + diffFile("repo-b/src/utils/helper.ts", { additions: 3, deletions: 1 }), + ]; + const tree = buildFileTree(files); + + // After collapseSingleChild: repo-a/src/utils becomes a single folder node + const repoA = tree.find(n => n.name === "repo-a/src/utils"); + const repoB = tree.find(n => n.name === "repo-b/src/utils"); + + expect(repoA).toBeDefined(); + expect(repoB).toBeDefined(); + + // Each should have the helper.ts file as a child + const repoAFile = repoA?.children?.find(n => n.name === "helper.ts"); + const repoBFile = repoB?.children?.find(n => n.name === "helper.ts"); + + expect(repoAFile).toBeDefined(); + expect(repoBFile).toBeDefined(); + expect(repoAFile?.path).toBe("repo-a/src/utils/helper.ts"); + expect(repoBFile?.path).toBe("repo-b/src/utils/helper.ts"); + expect(repoAFile?.additions).toBe(5); + expect(repoBFile?.additions).toBe(3); + }); + + it("handles nested repo labels (longest prefix)", () => { + // Simulates repos like "apps", "apps/api", "apps/web" + const files = [ + diffFile("apps/src/main.ts"), + diffFile("apps/api/src/server.ts"), + diffFile("apps/web/src/app.ts"), + ]; + const tree = buildFileTree(files); + + // All under single "apps" root, with children for each sub-repo + // After collapseSingleChild: api/src and web/src collapse + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("apps"); + expect(tree[0].type).toBe("folder"); + + // Children: "api/src" (collapsed), "src" (from apps/src), "web/src" (collapsed) + const childNames = tree[0].children?.map(n => n.name).sort(); + expect(childNames).toEqual(["api/src", "src", "web/src"]); + }); + + it("handles deeply nested repo labels", () => { + const files = [ + diffFile("packages/shared/utils/helpers/string.ts"), + diffFile("packages/core/src/index.ts"), + ]; + const tree = buildFileTree(files); + + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("packages"); + expect(tree[0].type).toBe("folder"); + + // After collapseSingleChild, packages/core/src collapses to "core/src" + // and packages/shared/utils/helpers collapses to "shared/utils/helpers" + const children = tree[0].children?.map(n => n.name).sort(); + expect(children).toEqual(["core/src", "shared/utils/helpers"]); + }); + + it("collapses single-child folders correctly with repo prefixes", () => { + const files = [ + diffFile("repo-a/src/components/Button.tsx"), + ]; + const tree = buildFileTree(files); + + // After collapseSingleChild: repo-a/src/components collapses to single path + // Then flat fallback kicks in: single folder with only file children gets unwrapped + // Result: just the file at root level + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("Button.tsx"); + expect(tree[0].type).toBe("file"); + expect(tree[0].path).toBe("repo-a/src/components/Button.tsx"); + }); + + it("aggregates stats correctly across repo boundaries", () => { + const files = [ + diffFile("repo-a/src/index.ts", { additions: 10, deletions: 5 }), + diffFile("repo-a/src/utils.ts", { additions: 5, deletions: 2 }), + diffFile("repo-b/src/index.ts", { additions: 8, deletions: 3 }), + ]; + const tree = buildFileTree(files); + + // After collapseSingleChild: repo-a/src contains both files + const repoA = tree.find(n => n.name === "repo-a/src"); + const repoB = tree.find(n => n.name === "repo-b/src"); + + expect(repoA?.additions).toBe(15); // 10 + 5 + expect(repoA?.deletions).toBe(7); // 5 + 2 + expect(repoB?.additions).toBe(8); + expect(repoB?.deletions).toBe(3); + }); + + it("handles repo labels with special characters", () => { + const files = [ + diffFile("my-repo_2.0/src/index.ts"), + diffFile("my-repo_2.0-beta/src/app.ts"), + ]; + const tree = buildFileTree(files); + + // Two separate root-level folders after collapse + expect(tree.length).toBeGreaterThanOrEqual(2); + const names = tree.map(n => n.name); + expect(names).toContain("my-repo_2.0/src"); + expect(names).toContain("my-repo_2.0-beta/src"); + }); + + it("preserves full prefixed path in node path property", () => { + const files = [ + diffFile("owner/repo/src/index.ts"), + ]; + const tree = buildFileTree(files); + + // Collapses to "owner/repo/src" folder, then flat fallback unwraps + // Result: just the file with full path preserved + expect(tree[0].name).toBe("index.ts"); + expect(tree[0].path).toBe("owner/repo/src/index.ts"); + }); + + it("handles empty file list", () => { + const tree = buildFileTree([]); + expect(tree).toHaveLength(0); + }); + + it("handles single file in repo (flat fallback)", () => { + const files = [diffFile("repo-a/README.md")]; + const tree = buildFileTree(files); + + // Flat fallback: single root folder with only file children gets unwrapped + // But first collapseSingleChild collapses repo-a to contain README.md + // Then flat fallback sees single folder "repo-a" with file child, unwraps it + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("README.md"); + expect(tree[0].type).toBe("file"); + expect(tree[0].path).toBe("repo-a/README.md"); + }); + + it("handles multiple files in same repo subdirectories", () => { + const files = [ + diffFile("repo-a/src/index.ts"), + diffFile("repo-a/src/app.ts"), + diffFile("repo-a/lib/helpers.ts"), + ]; + const tree = buildFileTree(files); + + // repo-a has two children: src (with 2 files) and lib (with 1 file) + // So it doesn't get unwrapped by flat fallback + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("repo-a"); + expect(tree[0].type).toBe("folder"); + + const children = tree[0].children?.map(n => n.name).sort(); + expect(children).toEqual(["lib", "src"]); + }); +}); + +describe("getAncestorPaths - workspace mode", () => { + it("returns ancestor paths for repo-prefixed file", () => { + const paths = getAncestorPaths("repo-a/src/utils/helper.ts"); + expect(paths).toEqual([ + "repo-a", + "repo-a/src", + "repo-a/src/utils", + ]); + }); + + it("handles deeply nested repo labels", () => { + const paths = getAncestorPaths("packages/shared/utils/helpers/string.ts"); + expect(paths).toEqual([ + "packages", + "packages/shared", + "packages/shared/utils", + "packages/shared/utils/helpers", + ]); + }); + + it("handles flat repo structure", () => { + const paths = getAncestorPaths("repo-a/file.ts"); + expect(paths).toEqual(["repo-a"]); + }); +}); + +describe("getAllFolderPaths - workspace mode", () => { + it("collects all folder paths from repo-prefixed tree", () => { + const files = [ + diffFile("repo-a/src/index.ts"), + diffFile("repo-b/src/app.ts"), + ]; + const tree = buildFileTree(files); + const folders = getAllFolderPaths(tree); + + // After collapseSingleChild, we get "repo-a/src" and "repo-b/src" + expect(folders).toContain("repo-a/src"); + expect(folders).toContain("repo-b/src"); + }); + + it("collects nested repo label folders", () => { + const files = [ + diffFile("apps/api/src/server.ts"), + diffFile("apps/web/src/app.ts"), + ]; + const tree = buildFileTree(files); + const folders = getAllFolderPaths(tree); + + // After collapseSingleChild: apps stays, apps/api/src and apps/web/src + expect(folders).toContain("apps"); + expect(folders).toContain("apps/api/src"); + expect(folders).toContain("apps/web/src"); + }); +}); diff --git a/packages/review-editor/utils/diffParser.test.ts b/packages/review-editor/utils/diffParser.test.ts new file mode 100644 index 000000000..28e9c1664 --- /dev/null +++ b/packages/review-editor/utils/diffParser.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "bun:test"; + +import { parseDiffToFiles } from "./diffParser"; + +describe("parseDiffToFiles", () => { + it("uses file header lines so paths containing separator text stay intact", () => { + const files = parseDiffToFiles([ + 'diff --git "a/api/foo b/bar.ts" "b/api/foo b/bar.ts"', + '--- "a/api/foo b/bar.ts"', + '+++ "b/api/foo b/bar.ts"', + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n")); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("api/foo b/bar.ts"); + expect(files[0].oldPath).toBeUndefined(); + expect(files[0].additions).toBe(1); + expect(files[0].deletions).toBe(1); + }); + + it("handles renamed quoted paths", () => { + const files = parseDiffToFiles([ + 'diff --git "a/api/old name.ts" "b/api/new name.ts"', + '--- "a/api/old name.ts"', + '+++ "b/api/new name.ts"', + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n")); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("api/new name.ts"); + expect(files[0].oldPath).toBe("api/old name.ts"); + }); + + it("parses unquoted headers from the right when file lines are absent", () => { + const files = parseDiffToFiles([ + "diff --git a/api/foo b/old.bin b/api/new.bin", + "new file mode 100644", + "index 0000000..1234567", + "GIT binary patch", + ].join("\n")); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("api/new.bin"); + expect(files[0].oldPath).toBe("api/foo b/old.bin"); + }); + + it("does not treat hunk body lines as file headers", () => { + const files = parseDiffToFiles([ + "diff --git a/api/file.txt b/api/file.txt", + "--- a/api/file.txt", + "+++ b/api/file.txt", + "@@ -1,2 +1,2 @@", + "---- a/not-a-header.txt", + "++++ b/not-a-header.txt", + ].join("\n")); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("api/file.txt"); + expect(files[0].oldPath).toBeUndefined(); + }); +}); diff --git a/packages/review-editor/utils/diffParser.ts b/packages/review-editor/utils/diffParser.ts new file mode 100644 index 000000000..4e4da2766 --- /dev/null +++ b/packages/review-editor/utils/diffParser.ts @@ -0,0 +1,132 @@ +import type { DiffFile } from '../types'; + +function unquoteGitPath(value: string): string { + if (!value.startsWith('"') || !value.endsWith('"')) return value; + try { + return JSON.parse(value) as string; + } catch { + return value.slice(1, -1) + .replace(/\\"/g, '"') + .replace(/\\\\/g, '\\') + .replace(/\\t/g, '\t') + .replace(/\\n/g, '\n'); + } +} + +function parsePatchPathToken(token: string, side: 'a' | 'b'): string | null { + if (token === '/dev/null') return '/dev/null'; + const unquoted = unquoteGitPath(token); + const prefix = `${side}/`; + return unquoted.startsWith(prefix) ? unquoted.slice(prefix.length) : null; +} + +function scanHeaderToken(input: string): { token: string; rest: string } | null { + const trimmed = input.trimStart(); + if (!trimmed) return null; + + if (trimmed.startsWith('"')) { + let escaped = false; + for (let i = 1; i < trimmed.length; i += 1) { + const char = trimmed[i]; + if (escaped) { + escaped = false; + continue; + } + if (char === '\\') { + escaped = true; + continue; + } + if (char === '"') { + return { token: trimmed.slice(0, i + 1), rest: trimmed.slice(i + 1) }; + } + } + return null; + } + + const space = trimmed.indexOf(' '); + if (space === -1) return { token: trimmed, rest: '' }; + return { token: trimmed.slice(0, space), rest: trimmed.slice(space + 1) }; +} + +function parseDiffGitHeader(header: string): { oldPath?: string; newPath?: string } { + const prefix = 'diff --git '; + if (!header.startsWith(prefix)) return {}; + + const rest = header.slice(prefix.length); + if (rest.trimStart().startsWith('"')) { + const first = scanHeaderToken(rest); + const second = first ? scanHeaderToken(first.rest) : null; + if (first && second) { + const oldPath = parsePatchPathToken(first.token, 'a'); + const newPath = parsePatchPathToken(second.token, 'b'); + return { + oldPath: oldPath && oldPath !== '/dev/null' ? oldPath : undefined, + newPath: newPath && newPath !== '/dev/null' ? newPath : undefined, + }; + } + } + + const match = header.match(/^diff --git a\/(.+) b\/(.+)$/); + if (!match) return {}; + return { oldPath: match[1], newPath: match[2] }; +} + +function parseDiffFilePathLines(lines: string[]): { oldPath?: string; newPath?: string } { + let oldPath: string | undefined; + let newPath: string | undefined; + + for (const line of lines) { + if (line.startsWith('@@ ') || line === 'GIT binary patch') break; + if (line.startsWith('--- ')) { + const parsed = parsePatchPathToken(line.slice(4), 'a'); + if (parsed && parsed !== '/dev/null') oldPath = parsed; + } else if (line.startsWith('+++ ')) { + const parsed = parsePatchPathToken(line.slice(4), 'b'); + if (parsed && parsed !== '/dev/null') newPath = parsed; + } + } + + return { oldPath, newPath }; +} + +function splitDiffChunks(rawPatch: string): string[] { + const matches = [...rawPatch.matchAll(/^diff --git /gm)]; + if (matches.length === 0) return []; + + return matches.map((match, index) => { + const start = match.index ?? 0; + const end = matches[index + 1]?.index ?? rawPatch.length; + return rawPatch.slice(start, end); + }); +} + +export function parseDiffToFiles(rawPatch: string): DiffFile[] { + const files: DiffFile[] = []; + + for (const chunk of splitDiffChunks(rawPatch)) { + const lines = chunk.split('\n'); + const fromFileLines = parseDiffFilePathLines(lines); + const fromHeader = parseDiffGitHeader(lines[0] ?? ''); + const oldPath = fromFileLines.oldPath ?? fromFileLines.newPath ?? fromHeader.oldPath; + const newPath = fromFileLines.newPath ?? fromFileLines.oldPath ?? fromHeader.newPath; + if (!oldPath || !newPath) continue; + + let additions = 0; + let deletions = 0; + + for (const line of lines) { + if (line.startsWith('+') && !line.startsWith('+++')) additions += 1; + if (line.startsWith('-') && !line.startsWith('---')) deletions += 1; + } + + files.push({ + path: newPath, + oldPath: oldPath !== newPath ? oldPath : undefined, + patch: chunk, + additions, + deletions, + }); + } + + return files; +} diff --git a/packages/review-editor/utils/exportFeedback.workspace.test.ts b/packages/review-editor/utils/exportFeedback.workspace.test.ts new file mode 100644 index 000000000..0a57ef16b --- /dev/null +++ b/packages/review-editor/utils/exportFeedback.workspace.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from "bun:test"; +import { exportReviewFeedback } from "./exportFeedback"; +import type { CodeAnnotation } from "@plannotator/ui/types"; + +const ann = (overrides: Partial = {}): CodeAnnotation => ({ + id: "1", + type: "comment", + filePath: "src/index.ts", + lineStart: 10, + lineEnd: 10, + side: "new", + text: "This looks wrong", + createdAt: Date.now(), + ...overrides, +}); + +describe("exportReviewFeedback - workspace mode", () => { + it("workspace mode: uses generic header, no PR content (same as local mode)", () => { + // In workspace mode, prMetadata is explicitly undefined even if workspace exists + const result = exportReviewFeedback([ann()], undefined); + expect(result).toStartWith("# Code Review Feedback\n\n"); + expect(result).not.toContain("PR Review"); + expect(result).not.toContain("github.com"); + expect(result).not.toContain("Branch:"); + }); + + it("groups annotations by repo-prefixed file paths", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", lineStart: 5, text: "first" }), + ann({ filePath: "repo-b/src/index.ts", lineStart: 1, text: "second" }), + ]); + // Different repos with same relative path should be separate groups + expect(result).toContain("## repo-a/src/index.ts"); + expect(result).toContain("## repo-b/src/index.ts"); + }); + + it("sorts annotations by line number within each repo-prefixed file", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", lineStart: 20, text: "later" }), + ann({ filePath: "repo-a/src/index.ts", lineStart: 5, text: "earlier" }), + ann({ filePath: "repo-b/src/index.ts", lineStart: 15, text: "middle in repo-b" }), + ]); + const earlierIdx = result.indexOf("earlier"); + const laterIdx = result.indexOf("later"); + const middleInRepoB = result.indexOf("middle in repo-b"); + expect(earlierIdx).toBeLessThan(laterIdx); + // Both repo-a annotations should come before repo-b (alphabetical by path) + expect(laterIdx).toBeLessThan(middleInRepoB); + }); + + it("handles nested repo labels with overlapping paths", () => { + // Tests the longest-prefix matching behavior from resolveWorkspaceFilePath + const result = exportReviewFeedback([ + ann({ filePath: "apps/api/src/server.ts", text: "in nested repo" }), + ann({ filePath: "apps/web/src/app.ts", text: "in sibling repo" }), + ann({ filePath: "apps/src/main.ts", text: "in parent repo" }), + ]); + expect(result).toContain("## apps/api/src/server.ts"); + expect(result).toContain("## apps/web/src/app.ts"); + expect(result).toContain("## apps/src/main.ts"); + }); + + it("handles deeply nested repo labels", () => { + const result = exportReviewFeedback([ + ann({ filePath: "packages/shared/utils/helpers/string.ts", text: "deep path" }), + ]); + expect(result).toContain("## packages/shared/utils/helpers/string.ts"); + expect(result).toContain("### Line 10 (new)"); + }); + + it("groups multiple annotations on same repo-prefixed file together", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", lineStart: 5, text: "first comment" }), + ann({ filePath: "repo-b/src/index.ts", lineStart: 10, text: "second comment" }), + ann({ filePath: "repo-a/src/index.ts", lineStart: 15, text: "third comment" }), + ]); + // All repo-a comments should be grouped together + const repoAHeaderIdx = result.indexOf("## repo-a/src/index.ts"); + const repoBHeaderIdx = result.indexOf("## repo-b/src/index.ts"); + const firstCommentIdx = result.indexOf("first comment"); + const thirdCommentIdx = result.indexOf("third comment"); + const secondCommentIdx = result.indexOf("second comment"); + + expect(repoAHeaderIdx).toBeLessThan(repoBHeaderIdx); + expect(firstCommentIdx).toBeLessThan(thirdCommentIdx); + expect(thirdCommentIdx).toBeLessThan(repoBHeaderIdx); + expect(repoBHeaderIdx).toBeLessThan(secondCommentIdx); + }); + + it("handles file-scoped annotations with repo-prefixed paths", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", scope: "file", text: "file comment" }), + ann({ filePath: "repo-a/src/index.ts", lineStart: 1, lineEnd: 1, text: "line comment" }), + ]); + expect(result).toContain("## repo-a/src/index.ts"); + expect(result).toContain("### File Comment"); + expect(result).toContain("### Line 1"); + const fileIdx = result.indexOf("File Comment"); + const lineIdx = result.indexOf("Line 1"); + expect(fileIdx).toBeLessThan(lineIdx); + }); + + it("handles repo labels with special characters in paths", () => { + const result = exportReviewFeedback([ + ann({ filePath: "my-repo_2.0/src/index.ts", text: "special chars" }), + ]); + expect(result).toContain("## my-repo_2.0/src/index.ts"); + }); + + it("empty annotations returns generic message regardless of workspace mode", () => { + expect(exportReviewFeedback([], undefined)).toBe("# Code Review\n\nNo feedback provided."); + }); + + it("contains exactly one top-level heading in workspace mode", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/a.ts" }), + ann({ filePath: "repo-b/src/b.ts" }), + ]); + const headingMatches = result.match(/^# /gm) || []; + expect(headingMatches).toHaveLength(1); + }); +}); diff --git a/packages/review-editor/utils/reviewSearch.workspace.test.ts b/packages/review-editor/utils/reviewSearch.workspace.test.ts new file mode 100644 index 000000000..400727cc5 --- /dev/null +++ b/packages/review-editor/utils/reviewSearch.workspace.test.ts @@ -0,0 +1,206 @@ +import { describe, it, expect } from "bun:test"; +import { + buildSearchIndex, + findMatchesInIndex, + findReviewSearchMatches, + groupReviewSearchMatches, +} from "./reviewSearch"; +import type { ReviewSearchableDiffFile } from "./reviewSearch"; + +const patchFile = (path: string, patch: string): ReviewSearchableDiffFile => ({ + path, + patch, + additions: 0, + deletions: 0, +}); + +describe("reviewSearch - workspace mode with repo-prefixed paths", () => { + const samplePatch = [ + "diff --git a/src/index.ts b/src/index.ts", + "--- a/src/index.ts", + "+++ b/src/index.ts", + "@@ -1,3 +1,3 @@", + " function greet() {", + "- return 'hello';", + "+ return 'hello world';", + " }", + ].join("\n"); + + it("builds search index with repo-prefixed file paths", () => { + const files = [ + patchFile("repo-a/src/index.ts", samplePatch), + patchFile("repo-b/src/index.ts", samplePatch), + ]; + const index = buildSearchIndex(files); + + // All lines should have repo-prefixed file paths + expect(index.every(line => line.filePath.startsWith("repo-"))).toBe(true); + expect(index.some(line => line.filePath === "repo-a/src/index.ts")).toBe(true); + expect(index.some(line => line.filePath === "repo-b/src/index.ts")).toBe(true); + }); + + it("finds matches across different repos with same relative path", () => { + const files = [ + patchFile("repo-a/src/utils.ts", [ + "diff --git a/src/utils.ts b/src/utils.ts", + "@@ -1 +1 @@", + "-const x = 1;", + "+const x = 2;", + ].join("\n")), + patchFile("repo-b/src/utils.ts", [ + "diff --git a/src/utils.ts b/src/utils.ts", + "@@ -1 +1 @@", + "-const y = 1;", + "+const y = 2;", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "const"); + + // Should find matches in both repos + const repoAMatches = matches.filter(m => m.filePath === "repo-a/src/utils.ts"); + const repoBMatches = matches.filter(m => m.filePath === "repo-b/src/utils.ts"); + + expect(repoAMatches.length).toBeGreaterThan(0); + expect(repoBMatches.length).toBeGreaterThan(0); + }); + + it("distinguishes same content in different repos", () => { + const files = [ + patchFile("repo-a/src/index.ts", [ + "diff --git a/src/index.ts b/src/index.ts", + "@@ -1 +1 @@", + "-old content", + "+new content", + ].join("\n")), + patchFile("repo-b/src/index.ts", [ + "diff --git a/src/index.ts b/src/index.ts", + "@@ -1 +1 @@", + "-old content", + "+new content", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "content"); + + // Should have separate match entries for each repo + const repoAIds = matches.filter(m => m.filePath === "repo-a/src/index.ts"); + const repoBIds = matches.filter(m => m.filePath === "repo-b/src/index.ts"); + + expect(repoAIds.length).toBe(2); // "old content" and "new content" + expect(repoBIds.length).toBe(2); + + // IDs should be different + const repoAIdSet = new Set(repoAIds.map(m => m.id)); + const repoBIdSet = new Set(repoBIds.map(m => m.id)); + expect(repoAIdSet.intersection(repoBIdSet).size).toBe(0); + }); + + it("handles deeply nested repo labels in search", () => { + const files = [ + patchFile("packages/shared/utils/helpers.ts", [ + "diff --git a/utils/helpers.ts b/utils/helpers.ts", + "@@ -1 +1 @@", + "-helper function", + "+improved helper", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "helper"); + + expect(matches.length).toBe(2); + expect(matches.every(m => m.filePath === "packages/shared/utils/helpers.ts")).toBe(true); + }); + + it("groups matches by repo-prefixed file path", () => { + const files = [ + patchFile("repo-a/src/index.ts", samplePatch), + patchFile("repo-b/src/other.ts", [ + "diff --git a/src/other.ts b/src/other.ts", + "@@ -1 +1 @@", + "-hello there", + "+goodbye there", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "hello"); + const groups = groupReviewSearchMatches(files, matches); + + // "hello" appears in both files (repo-a: "hello world", repo-b: "hello there") + expect(groups).toHaveLength(2); + const paths = groups.map(g => g.filePath).sort(); + expect(paths).toEqual(["repo-a/src/index.ts", "repo-b/src/other.ts"]); + }); + + it("maintains correct file indices with repo-prefixed paths", () => { + const files = [ + patchFile("repo-a/src/a.ts", samplePatch), + patchFile("repo-b/src/b.ts", samplePatch), + patchFile("repo-c/src/c.ts", samplePatch), + ]; + const matches = findReviewSearchMatches(files, "hello"); + const groups = groupReviewSearchMatches(files, matches); + + // Each group should have correct file index + const groupA = groups.find(g => g.filePath === "repo-a/src/a.ts"); + const groupB = groups.find(g => g.filePath === "repo-b/src/b.ts"); + const groupC = groups.find(g => g.filePath === "repo-c/src/c.ts"); + + expect(groupA?.fileIndex).toBe(0); + expect(groupB?.fileIndex).toBe(1); + expect(groupC?.fileIndex).toBe(2); + }); + + it("handles search in nested repo labels (longest prefix)", () => { + const files = [ + patchFile("apps/api/src/server.ts", [ + "diff --git a/src/server.ts b/src/server.ts", + "@@ -1 +1 @@", + "-server code", + "+better server", + ].join("\n")), + patchFile("apps/web/src/app.ts", [ + "diff --git a/src/app.ts b/src/app.ts", + "@@ -1 +1 @@", + "-app code", + "+better app", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "code"); + + const apiMatch = matches.find(m => m.filePath === "apps/api/src/server.ts"); + const webMatch = matches.find(m => m.filePath === "apps/web/src/app.ts"); + + expect(apiMatch).toBeDefined(); + expect(webMatch).toBeDefined(); + }); + + it("returns empty results for non-matching query in workspace", () => { + const files = [ + patchFile("repo-a/src/index.ts", samplePatch), + ]; + const matches = findReviewSearchMatches(files, "nonexistent"); + + expect(matches).toHaveLength(0); + }); + + it("handles empty file list in workspace mode", () => { + const index = buildSearchIndex([]); + expect(index).toHaveLength(0); + + const matches = findMatchesInIndex(index, "query"); + expect(matches).toHaveLength(0); + }); + + it("handles multiple matches on same line in same repo", () => { + const files = [ + patchFile("repo-a/src/index.ts", [ + "diff --git a/src/index.ts b/src/index.ts", + "@@ -1 +1 @@", + "-foo bar foo", + "+foo baz foo", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "foo"); + + // Should find 4 matches (2 on old line, 2 on new line) + expect(matches.length).toBe(4); + expect(matches.every(m => m.filePath === "repo-a/src/index.ts")).toBe(true); + }); +}); diff --git a/packages/server/agent-review-message.test.ts b/packages/server/agent-review-message.test.ts index 3effc8075..700db22ff 100644 --- a/packages/server/agent-review-message.test.ts +++ b/packages/server/agent-review-message.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { buildAgentReviewUserMessage, getLocalDiffInstruction } from "./agent-review-message"; +import { buildAgentReviewUserMessage, buildAgentReviewUserMessageForTarget, getLocalDiffInstruction } from "./agent-review-message"; import { buildClaudeCommand } from "./claude-review"; const patch = "diff --git a/src/large.ts b/src/large.ts\n+const value = 1;\n"; @@ -66,6 +66,29 @@ describe("buildAgentReviewUserMessage", () => { expect(message).toContain("Review the following code changes"); expect(message).toContain(patch); }); + + test("builds workspace review instructions with prefixed paths and inline patch", () => { + const message = buildAgentReviewUserMessageForTarget({ + kind: "workspace", + patch, + workspace: { + root: "/tmp/workspace", + repos: [ + { label: "api", cwd: "/tmp/workspace/api", vcsType: "git", gitRef: "Uncommitted changes" }, + { label: "web", cwd: "/tmp/workspace/web", vcsType: "jj", gitRef: "Uncommitted changes" }, + ], + }, + }); + + expect(message).toContain("multiple nested VCS repositories"); + expect(message).toContain("workspace root: /tmp/workspace"); + expect(message).toContain("api/src/file.ts"); + expect(message).toContain("- api/ [git] -> /tmp/workspace/api"); + expect(message).toContain("- web/ [jj] -> /tmp/workspace/web"); + expect(message).toContain("git -C "); + expect(message).toContain("JJ child repos"); + expect(message).toContain(patch); + }); }); describe("getLocalDiffInstruction", () => { @@ -86,5 +109,6 @@ describe("buildClaudeCommand", () => { expect(allowedTools).toContain("Bash(jj file show:*)"); expect(allowedTools).toContain("Bash(jj cat:*)"); expect(allowedTools).toContain("Bash(jj bookmark list:*)"); + expect(allowedTools).toContain("Bash(git -C:*)"); }); }); diff --git a/packages/server/agent-review-message.ts b/packages/server/agent-review-message.ts index bfa9c1e52..cd4a72c96 100644 --- a/packages/server/agent-review-message.ts +++ b/packages/server/agent-review-message.ts @@ -12,11 +12,83 @@ export interface AgentReviewUserMessageOptions { prDiffScope?: string; } +export interface WorkspaceReviewPromptContext { + root: string; + repos: Array<{ + label: string; + cwd: string; + vcsType?: "git" | "jj"; + gitRef?: string; + }>; +} + +export type AgentReviewTarget = + | { + kind: "local"; + patch: string; + diffType: DiffType; + options?: AgentReviewUserMessageOptions; + } + | { + kind: "pr"; + patch: string; + diffType: DiffType; + options?: AgentReviewUserMessageOptions; + prMetadata: PRMetadata; + } + | { + kind: "workspace"; + patch: string; + workspace: WorkspaceReviewPromptContext; + }; + export interface LocalDiffInstruction { target: string; inspect: string; } +export function buildWorkspacePromptContextLines( + workspace: WorkspaceReviewPromptContext, + options: { includeReportingInstruction?: boolean } = {}, +): string[] { + const repoList = workspace.repos.length > 0 + ? workspace.repos + .map((repo) => `- ${repo.label}/${repo.vcsType ? ` [${repo.vcsType}]` : ""} -> ${repo.cwd}${repo.gitRef ? ` (${repo.gitRef})` : ""}`) + .join("\n") + : "- No changed child repositories were detected."; + + const lines = [ + `You are starting in the workspace root: ${workspace.root}`, + "The workspace root is not itself the VCS repository for these changes.", + "Each changed path in the diff is prefixed with the child repository folder, such as `api/src/file.ts`.", + "For Git child repos, inspect with `git -C ...` from the workspace root.", + "For JJ child repos, treat the inline diff and prefixed files as authoritative review context.", + ]; + + if (options.includeReportingInstruction) { + lines.push("When reporting findings, use the same prefixed paths shown in the diff."); + } + + return [ + ...lines, + "", + "Repositories:", + repoList, + ]; +} + +export function buildAgentReviewUserMessageForTarget(target: AgentReviewTarget): string { + if (target.kind === "workspace") { + return buildWorkspaceReviewUserMessage(target.patch, target.workspace); + } + return buildAgentReviewUserMessage( + target.patch, + target.diffType, + target.options, + target.kind === "pr" ? target.prMetadata : undefined, + ); +} + /** Build the dynamic user message shared by local Claude and Codex review jobs. */ export function buildAgentReviewUserMessage( patch: string, @@ -63,6 +135,21 @@ export function buildAgentReviewUserMessage( ].join("\n"); } +function buildWorkspaceReviewUserMessage( + patch: string, + workspace: WorkspaceReviewPromptContext, +): string { + return [ + "Review the local workspace changes across multiple nested VCS repositories.", + "", + ...buildWorkspacePromptContextLines(workspace, { includeReportingInstruction: true }), + "", + "```diff", + patch, + "```", + ].join("\n"); +} + export function getLocalDiffInstruction( diffType: DiffType, defaultBranch?: string, diff --git a/packages/server/claude-review.ts b/packages/server/claude-review.ts index 5b0ec0d9c..bbf6513b9 100644 --- a/packages/server/claude-review.ts +++ b/packages/server/claude-review.ts @@ -205,7 +205,7 @@ export function buildClaudeCommand(prompt: string, model: string = "claude-opus- "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(git show-ref:*)", "Bash(git -C:*)", // JJ (read-only) "Bash(jj status:*)", "Bash(jj diff:*)", "Bash(jj log:*)", "Bash(jj show:*)", "Bash(jj file show:*)", "Bash(jj cat:*)", @@ -282,6 +282,7 @@ export function transformClaudeFindings( findings: ClaudeFinding[], source: string, cwd?: string, + pathTransform?: (path: string) => string, ): Array<{ source: string; filePath: string; @@ -299,7 +300,9 @@ export function transformClaudeFindings( .filter(f => f.file && typeof f.line === "number") .map(f => ({ source, - filePath: toRelativePath(f.file, cwd), + filePath: pathTransform + ? pathTransform(toRelativePath(f.file, cwd)) + : toRelativePath(f.file, cwd), lineStart: f.line, lineEnd: f.end_line ?? f.line, type: "comment", diff --git a/packages/server/codex-review.ts b/packages/server/codex-review.ts index 25619cc94..7c1f19eb7 100644 --- a/packages/server/codex-review.ts +++ b/packages/server/codex-review.ts @@ -291,6 +291,7 @@ export function transformReviewFindings( source: string, cwd?: string, author?: string, + pathTransform?: (path: string) => string, ): ReviewAnnotationInput[] { const annotations = findings .filter((f) => @@ -300,7 +301,9 @@ export function transformReviewFindings( ) .map((f) => ({ source, - filePath: toRelativePath(f.code_location.absolute_file_path, cwd), + filePath: pathTransform + ? pathTransform(toRelativePath(f.code_location.absolute_file_path, cwd)) + : toRelativePath(f.code_location.absolute_file_path, cwd), lineStart: f.code_location.line_range.start, lineEnd: f.code_location.line_range.end, type: "comment", diff --git a/packages/server/package.json b/packages/server/package.json index 31540fdfa..0242e2502 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -17,6 +17,7 @@ "./p4": "./p4.ts", "./vcs": "./vcs.ts", "./repo": "./repo.ts", + "./review-workspace": "./review-workspace.ts", "./share-url": "./share-url.ts", "./sessions": "./sessions.ts", "./project": "./project.ts", diff --git a/packages/server/review-workspace.test.ts b/packages/server/review-workspace.test.ts new file mode 100644 index 000000000..25d741bad --- /dev/null +++ b/packages/server/review-workspace.test.ts @@ -0,0 +1,751 @@ +/** + * Workspace Review Tests + * + * Tests for workspace repo discovery, label building, and path resolution. + * Run: bun test packages/server/review-workspace.test.ts + */ + +import { afterEach, describe, expect, it } from "bun:test"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { spawnSync } from "node:child_process"; + +import { + aggregateWorkspacePatch, + buildLocalWorkspaceReview, + prefixPatchPaths, + resolveWorkspaceFilePath, + discoverWorkspaceRepoPaths, + WorkspaceReviewSession, + type WorkspaceRepoRuntimeState, +} from "./review-workspace"; +import { startReviewServer } from "./review"; +import type { DiffType, GitContext } from "./vcs"; + +const tempDirs: string[] = []; + +function makeTempDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +function git(cwd: string, args: string[]): string { + const result = spawnSync("git", args, { cwd, encoding: "utf-8" }); + if (result.status !== 0) { + throw new Error(result.stderr || `git ${args.join(" ")} failed`); + } + return result.stdout.trim(); +} + +function initRepo(dir: string, initialBranch = "main"): void { + git(dir, ["init"]); + git(dir, ["branch", "-M", initialBranch]); + git(dir, ["config", "user.email", "test@example.com"]); + git(dir, ["config", "user.name", "Test User"]); + writeFileSync(join(dir, "README.md"), "# Test\n", "utf-8"); + git(dir, ["add", "README.md"]); + git(dir, ["commit", "-m", "initial"]); +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("review-workspace", () => { + describe("prefixPatchPaths", () => { + it("prefixes diff headers with the repo label", () => { + const patch = [ + "diff --git a/src/index.ts b/src/index.ts", + "--- a/src/index.ts", + "+++ b/src/index.ts", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("diff --git a/repo-a/src/index.ts b/repo-a/src/index.ts"); + expect(result).toContain("--- a/repo-a/src/index.ts"); + expect(result).toContain("+++ b/repo-a/src/index.ts"); + }); + + it("preserves paths containing the diff header separator text", () => { + const patch = [ + "diff --git a/foo b/bar.ts b/foo b/bar.ts", + "--- a/foo b/bar.ts", + "+++ b/foo b/bar.ts", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"); + + const result = prefixPatchPaths(patch, "api"); + + expect(result).toContain("diff --git \"a/api/foo b/bar.ts\" \"b/api/foo b/bar.ts\""); + expect(result).toContain("--- \"a/api/foo b/bar.ts\""); + expect(result).toContain("+++ \"b/api/foo b/bar.ts\""); + }); + + it("keeps quoted paths valid when prefixing workspace paths", () => { + const patch = [ + "diff --git \"a/path with space.ts\" \"b/path with space.ts\"", + "--- \"a/path with space.ts\"", + "+++ \"b/path with space.ts\"", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"); + + const result = prefixPatchPaths(patch, "web"); + + expect(result).toContain("diff --git \"a/web/path with space.ts\" \"b/web/path with space.ts\""); + expect(result).toContain("--- \"a/web/path with space.ts\""); + expect(result).toContain("+++ \"b/web/path with space.ts\""); + }); + + it("handles /dev/null paths correctly", () => { + const patch = [ + "diff --git a/src/index.ts b/src/index.ts", + "--- a/src/index.ts", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-content", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("+++ /dev/null"); + expect(result).not.toContain("+++ b/repo-a/dev/null"); + }); + + it("handles empty patches", () => { + expect(prefixPatchPaths("", "repo-a")).toBe(""); + expect(prefixPatchPaths(" ", "repo-a")).toBe(" "); + }); + + it("handles nested paths correctly", () => { + const patch = [ + "diff --git a/packages/ui/src/index.ts b/packages/ui/src/index.ts", + "--- a/packages/ui/src/index.ts", + "+++ b/packages/ui/src/index.ts", + ].join("\n"); + + const result = prefixPatchPaths(patch, "frontend"); + + expect(result).toContain("diff --git a/frontend/packages/ui/src/index.ts b/frontend/packages/ui/src/index.ts"); + }); + + it("prefixes rename and copy metadata without corrupting the header keywords", () => { + const patch = [ + "diff --git a/src/old.ts b/src/new.ts", + "similarity index 100%", + "rename from src/old.ts", + "rename to src/new.ts", + "diff --git a/src/source.ts b/src/copy.ts", + "similarity index 100%", + "copy from src/source.ts", + "copy to src/copy.ts", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("rename from repo-a/src/old.ts"); + expect(result).toContain("rename to repo-a/src/new.ts"); + expect(result).toContain("copy from repo-a/src/source.ts"); + expect(result).toContain("copy to repo-a/src/copy.ts"); + expect(result).not.toContain("rename a/repo-a/from"); + expect(result).not.toContain("copy a/repo-a/from"); + }); + + it("prefixes pure rename headers when there are no file path lines", () => { + const patch = [ + "diff --git a/src/old.ts b/src/new.ts", + "similarity index 100%", + "rename from src/old.ts", + "rename to src/new.ts", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("diff --git a/repo-a/src/old.ts b/repo-a/src/new.ts"); + expect(result).toContain("rename from repo-a/src/old.ts"); + expect(result).toContain("rename to repo-a/src/new.ts"); + }); + + it("prefixes unquoted headers from the right when file path lines are absent", () => { + const patch = [ + "diff --git a/foo b/old.bin b/new.bin", + "new file mode 100644", + "index 0000000..1234567", + "GIT binary patch", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("diff --git \"a/repo-a/foo b/old.bin\" b/repo-a/new.bin"); + }); + + it("does not treat hunk body lines as file headers", () => { + const patch = [ + "diff --git a/src/file.txt b/src/file.txt", + "--- a/src/file.txt", + "+++ b/src/file.txt", + "@@ -1,2 +1,2 @@", + "---- a/not-a-header.txt", + "++++ b/not-a-header.txt", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("diff --git a/repo-a/src/file.txt b/repo-a/src/file.txt"); + expect(result).toContain("---- a/not-a-header.txt"); + expect(result).toContain("++++ b/not-a-header.txt"); + }); + + it("does not prefix /dev/null when it appears in metadata", () => { + const result = prefixPatchPaths("rename from /dev/null", "repo-a"); + + expect(result).toBe("rename from /dev/null"); + }); + }); + + describe("aggregateWorkspacePatch", () => { + it("preserves real trailing spaces in patch lines", () => { + const aggregate = aggregateWorkspacePatch([{ + label: "api", + selected: true, + rawPatch: [ + "diff --git a/api/file.txt b/api/file.txt", + "@@ -1 +1 @@", + "-before", + "+after ", + "", + ].join("\n"), + gitRef: "Uncommitted changes", + }]); + + expect(aggregate.rawPatch).toEndWith("+after "); + }); + }); + + describe("resolveWorkspaceFilePath", () => { + it("resolves the longest matching repo label first", () => { + const repos = [ + { id: "1", label: "apps", cwd: "/tmp/apps", selected: true, source: "local", rawPatch: "", gitRef: "" }, + { id: "2", label: "apps/api", cwd: "/tmp/apps-api", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "apps/api/src/index.ts"); + + expect(resolved?.repo.id).toBe("2"); + expect(resolved?.repoRelativePath).toBe("src/index.ts"); + }); + + it("returns null when no repo matches", () => { + const repos = [ + { id: "1", label: "frontend", cwd: "/tmp/frontend", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "backend/src/index.ts"); + + expect(resolved).toBeNull(); + }); + + it("handles exact label matches", () => { + const repos = [ + { id: "1", label: "repo-a", cwd: "/tmp/repo-a", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "repo-a/file.ts"); + + expect(resolved?.repo.id).toBe("1"); + expect(resolved?.repoRelativePath).toBe("file.ts"); + }); + + it("rejects bare repo labels", () => { + const repos = [ + { id: "1", label: "repo-a", cwd: "/tmp/repo-a", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "repo-a"); + + expect(resolved).toBeNull(); + }); + + it("validates file paths for directory traversal attacks", () => { + const repos = [ + { id: "1", label: "repo", cwd: "/tmp/repo", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + expect(() => resolveWorkspaceFilePath(repos, "repo/../../../etc/passwd")).toThrow(); + }); + }); + + describe("discoverWorkspaceRepoPaths", () => { + it("excludes the root itself even if it is a git repo", () => { + // The function is designed to discover repos WITHIN a workspace root, + // not the root itself. This allows the workspace root to be a git repo + // (e.g., a meta-repo) while still discovering nested repos. + const root = makeTempDir("plannotator-workspace-root-repo-"); + initRepo(root); + + const repos = discoverWorkspaceRepoPaths(root); + + // Root itself is excluded even though it's a git repo + expect(repos).toHaveLength(0); + expect(repos).not.toContain(root); + }); + + it("discovers multiple nested VCS repos", () => { + const root = makeTempDir("plannotator-workspace-multi-"); + + // Create nested repos + const frontend = join(root, "frontend"); + const backend = join(root, "backend"); + const backendApi = join(backend, "api"); + + mkdirSync(frontend, { recursive: true }); + mkdirSync(backendApi, { recursive: true }); + + initRepo(frontend); + initRepo(backendApi); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(2); + expect(repos).toContain(frontend); + expect(repos).toContain(backendApi); + expect(repos).not.toContain(root); + expect(repos).not.toContain(backend); // backend itself is not a repo + }); + + it("stops recursion at git repo boundaries (does not discover nested repos inside other repos)", () => { + const root = makeTempDir("plannotator-workspace-boundary-"); + + // Create a repo with a nested directory that would be a repo + const parentRepo = join(root, "parent"); + const childDir = join(parentRepo, "child"); + + mkdirSync(childDir, { recursive: true }); + initRepo(parentRepo); + + // Create a git repo inside the child (should NOT be discovered separately + // because parent repo stops recursion - we don't traverse into git repos) + const grandchildRepo = join(childDir, "grandchild"); + mkdirSync(grandchildRepo, { recursive: true }); + initRepo(grandchildRepo); + + const repos = discoverWorkspaceRepoPaths(root); + + // Only the parent should be discovered - grandchild is inside a git repo + expect(repos).toHaveLength(1); + expect(repos).toContain(parentRepo); + expect(repos).not.toContain(grandchildRepo); + }); + + it("discovers nested jj repos", () => { + const root = makeTempDir("plannotator-workspace-jj-"); + const jjRepo = join(root, "jj-app"); + mkdirSync(join(jjRepo, ".jj"), { recursive: true }); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toEqual([jjRepo]); + }); + + it("skips ignored directories", () => { + const root = makeTempDir("plannotator-workspace-skip-"); + + // Create node_modules with a fake .git (should be skipped) + const nodeModules = join(root, "node_modules", "some-pkg"); + mkdirSync(nodeModules, { recursive: true }); + mkdirSync(join(nodeModules, ".git"), { recursive: true }); + + // Create a real repo + const realRepo = join(root, "src"); + mkdirSync(realRepo, { recursive: true }); + initRepo(realRepo); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(1); + expect(repos[0]).toBe(realRepo); + }); + + it("returns empty array when root has no git repos", () => { + const root = makeTempDir("plannotator-workspace-empty-"); + + // Create some non-git directories + mkdirSync(join(root, "src"), { recursive: true }); + mkdirSync(join(root, "docs"), { recursive: true }); + writeFileSync(join(root, "README.md"), "# Project\n", "utf-8"); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(0); + }); + + it("sorts results alphabetically", () => { + const root = makeTempDir("plannotator-workspace-sort-"); + + const zebra = join(root, "zebra"); + const alpha = join(root, "alpha"); + const beta = join(root, "beta"); + + mkdirSync(zebra, { recursive: true }); + mkdirSync(alpha, { recursive: true }); + mkdirSync(beta, { recursive: true }); + + initRepo(zebra); + initRepo(alpha); + initRepo(beta); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toEqual([alpha, beta, zebra]); + }); + + it("handles deeply nested repos", () => { + const root = makeTempDir("plannotator-workspace-deep-"); + + const deepRepo = join(root, "a", "b", "c", "d", "repo"); + mkdirSync(deepRepo, { recursive: true }); + initRepo(deepRepo); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(1); + expect(repos[0]).toBe(deepRepo); + }); + }); + + describe("buildRepoLabel (via discoverWorkspaceRepoPaths integration)", () => { + it("uses relative path as label when possible", () => { + // This is tested indirectly through the full workspace flow + // The label building logic is internal, but we verify it works + // through resolveWorkspaceFilePath tests with realistic labels + const repos = [ + { id: "1", label: "packages/frontend", cwd: "/tmp/packages/frontend", selected: true, source: "local", rawPatch: "", gitRef: "" }, + { id: "2", label: "packages/backend", cwd: "/tmp/packages/backend", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved1 = resolveWorkspaceFilePath(repos, "packages/frontend/src/index.ts"); + const resolved2 = resolveWorkspaceFilePath(repos, "packages/backend/api.ts"); + + expect(resolved1?.repo.id).toBe("1"); + expect(resolved2?.repo.id).toBe("2"); + }); + + it("handles duplicate basename fallback", () => { + // When two repos have the same basename but different paths, + // the second should get a numbered suffix + const repos = [ + { id: "1", label: "api", cwd: "/tmp/apps/api", selected: true, source: "local", rawPatch: "", gitRef: "" }, + { id: "2", label: "api-2", cwd: "/tmp/services/api", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "api-2/src/index.ts"); + + expect(resolved?.repo.id).toBe("2"); + }); + }); + + describe("workspace review server integration", () => { + it("maps one workspace mode across mixed Git and JJ repos", async () => { + const root = makeTempDir("plannotator-workspace-mixed-vcs-"); + const gitRepo = join(root, "api"); + const jjRepo = join(root, "web"); + mkdirSync(join(gitRepo, ".git"), { recursive: true }); + mkdirSync(join(jjRepo, ".jj"), { recursive: true }); + const calls: Array<{ cwd?: string; diffType: DiffType }> = []; + + const runtime = { + async getVcsContext(cwd?: string): Promise { + const isJj = cwd === jjRepo; + return { + vcsType: isJj ? "jj" : "git", + currentBranch: "main", + defaultBranch: "main", + cwd: cwd ?? root, + worktrees: [], + availableBranches: { local: [], remote: [] }, + diffOptions: isJj + ? [{ id: "jj-current", label: "Current change" }, { id: "jj-last", label: "Last change" }] + : [{ id: "uncommitted", label: "Uncommitted changes" }, { id: "last-commit", label: "Last commit" }], + }; + }, + async runVcsDiff(diffType: DiffType, _defaultBranch?: string, cwd?: string) { + calls.push({ cwd, diffType }); + return { + patch: [ + "diff --git a/file.txt b/file.txt", + "--- a/file.txt", + "+++ b/file.txt", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"), + label: diffType, + }; + }, + async getVcsFileContentsForDiff() { + return { oldContent: null, newContent: null }; + }, + async canStageFiles() { + return true; + }, + async stageFile() {}, + async unstageFile() {}, + }; + + const workspace = await WorkspaceReviewSession.create(runtime, root, { + requestedDiffType: "staged", + }); + + expect(workspace.diffType).toBe("workspace-current"); + expect(workspace.diffOptions.map((option) => option.id)).toEqual([ + "workspace-current", + "workspace-last", + ]); + expect(calls).toEqual( + expect.arrayContaining([ + { cwd: gitRepo, diffType: "uncommitted" }, + { cwd: jjRepo, diffType: "jj-current" }, + ]), + ); + expect(workspace.rawPatch).toContain("diff --git a/api/file.txt b/api/file.txt"); + expect(workspace.rawPatch).toContain("diff --git a/web/file.txt b/web/file.txt"); + + calls.length = 0; + await workspace.rebuild({ diffType: "workspace-last" }); + expect(calls).toEqual( + expect.arrayContaining([ + { cwd: gitRepo, diffType: "last-commit" }, + { cwd: jjRepo, diffType: "jj-last" }, + ]), + ); + await expect(workspace.rebuild({ diffType: "workspace-staged" })).rejects.toThrow( + "Workspace diff mode is not available", + ); + }); + + it("normalizes agent annotation paths to workspace-prefixed paths", async () => { + const root = makeTempDir("plannotator-workspace-agent-path-"); + const api = join(root, "api"); + mkdirSync(join(api, ".git"), { recursive: true }); + + const runtime = { + async getVcsContext(cwd?: string): Promise { + return { + vcsType: "git", + currentBranch: "main", + defaultBranch: "main", + cwd: cwd ?? api, + worktrees: [], + availableBranches: { local: [], remote: [] }, + diffOptions: [{ id: "uncommitted", label: "Uncommitted changes" }], + }; + }, + async runVcsDiff() { + return { + patch: [ + "diff --git a/src/file.ts b/src/file.ts", + "--- a/src/file.ts", + "+++ b/src/file.ts", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"), + label: "Uncommitted changes", + }; + }, + async getVcsFileContentsForDiff() { + return { oldContent: null, newContent: null }; + }, + async canStageFiles() { + return true; + }, + async stageFile() {}, + async unstageFile() {}, + }; + + const workspace = await WorkspaceReviewSession.create(runtime, root); + + expect(workspace.normalizeAnnotationPath("api/src/file.ts")).toBe("api/src/file.ts"); + expect(workspace.normalizeAnnotationPath("src/file.ts")).toBe("api/src/file.ts"); + expect(workspace.normalizeAnnotationPath(join(api, "src/file.ts"))).toBe("api/src/file.ts"); + }); + + it("keeps requested Git-only workspace modes available when another child repo fails detection", async () => { + const root = makeTempDir("plannotator-workspace-partial-failure-"); + const api = join(root, "api"); + const broken = join(root, "broken"); + mkdirSync(join(api, ".git"), { recursive: true }); + mkdirSync(join(broken, ".git"), { recursive: true }); + + const runtime = { + async getVcsContext(cwd?: string): Promise { + if (cwd === broken) throw new Error("broken repo"); + return { + vcsType: "git", + currentBranch: "main", + defaultBranch: "main", + cwd: cwd ?? api, + worktrees: [], + availableBranches: { local: [], remote: [] }, + diffOptions: [{ id: "staged", label: "Staged changes" }], + }; + }, + async runVcsDiff() { + return { + patch: [ + "diff --git a/src/file.ts b/src/file.ts", + "--- a/src/file.ts", + "+++ b/src/file.ts", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"), + label: "Staged changes", + }; + }, + async getVcsFileContentsForDiff() { + return { oldContent: null, newContent: null }; + }, + async canStageFiles() { + return true; + }, + async stageFile() {}, + async unstageFile() {}, + }; + + const workspace = await WorkspaceReviewSession.create(runtime, root, { + requestedDiffType: "staged", + }); + + expect(workspace.diffType).toBe("workspace-staged"); + expect(workspace.diffOptions.map((option) => option.id)).toContain("workspace-staged"); + expect(workspace.rawPatch).toContain("diff --git a/api/src/file.ts b/api/src/file.ts"); + expect(workspace.error).toContain("broken repo"); + }); + + it("passes hide-whitespace through child repo diffs", async () => { + const root = makeTempDir("plannotator-workspace-whitespace-"); + const api = join(root, "api"); + mkdirSync(api, { recursive: true }); + initRepo(api); + + writeFileSync(join(api, "tracked.txt"), "const value = 1;\n", "utf-8"); + git(api, ["add", "tracked.txt"]); + git(api, ["commit", "-m", "add tracked"]); + writeFileSync(join(api, "tracked.txt"), "const value = 1;\n", "utf-8"); + + const workspace = await buildLocalWorkspaceReview(root, { hideWhitespace: true }); + const aggregate = aggregateWorkspacePatch(workspace.repos); + + expect(workspace.repos[0]?.selected).toBe(false); + expect(aggregate.rawPatch).toBe(""); + }, 15_000); + + it("serves combined diffs and maps prefixed paths back to child repos", async () => { + const root = makeTempDir("plannotator-workspace-server-"); + const api = join(root, "api"); + const web = join(root, "web"); + mkdirSync(api, { recursive: true }); + mkdirSync(web, { recursive: true }); + initRepo(api); + initRepo(web); + + writeFileSync(join(api, "tracked.txt"), "before\n", "utf-8"); + git(api, ["add", "tracked.txt"]); + git(api, ["commit", "-m", "add tracked"]); + writeFileSync(join(api, "tracked.txt"), "after\n", "utf-8"); + writeFileSync(join(web, "new.txt"), "new file\n", "utf-8"); + + const workspace = await buildLocalWorkspaceReview(root); + const aggregate = aggregateWorkspacePatch(workspace.repos); + const server = await startReviewServer({ + rawPatch: aggregate.rawPatch, + gitRef: aggregate.gitRef, + error: aggregate.errors.join("\n") || undefined, + origin: "claude-code", + workspace, + agentCwd: workspace.root, + htmlContent: "review", + }); + + try { + const diffResponse = await fetch(`${server.url}/api/diff`); + expect(diffResponse.status).toBe(200); + const diffPayload = await diffResponse.json() as { + mode?: string; + rawPatch: string; + diffType?: string; + diffOptions?: Array<{ id: string }>; + agentCwd?: string; + }; + expect(diffPayload.mode).toBe("workspace"); + expect(diffPayload.diffType).toBe("workspace-current"); + expect(diffPayload.diffOptions?.map((option) => option.id)).toEqual([ + "workspace-current", + "workspace-staged", + "workspace-unstaged", + "workspace-last", + ]); + expect(diffPayload.agentCwd).toBe(root); + expect("workspace" in diffPayload).toBe(false); + expect(diffPayload.rawPatch).toContain("diff --git a/api/tracked.txt b/api/tracked.txt"); + expect(diffPayload.rawPatch).toContain("diff --git a/web/new.txt b/web/new.txt"); + + const lastResponse = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "workspace-last", hideWhitespace: true }), + }); + expect(lastResponse.status).toBe(200); + const lastPayload = await lastResponse.json() as { diffType?: string; rawPatch: string; diffOptions?: Array<{ id: string }> }; + expect(lastPayload.diffType).toBe("workspace-last"); + expect(lastPayload.diffOptions?.map((option) => option.id)).toContain("workspace-current"); + expect(lastPayload.rawPatch).toContain("diff --git a/api/tracked.txt b/api/tracked.txt"); + + const currentResponse = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "workspace-current", hideWhitespace: false }), + }); + expect(currentResponse.status).toBe(200); + + const fileContentResponse = await fetch(`${server.url}/api/file-content?path=api/tracked.txt`); + expect(fileContentResponse.status).toBe(200); + const fileContent = await fileContentResponse.json() as { + oldContent: string | null; + newContent: string | null; + }; + expect(fileContent.oldContent).toBe("before\n"); + expect(fileContent.newContent).toBe("after\n"); + + const stageResponse = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "web/new.txt" }), + }); + expect(stageResponse.status).toBe(200); + expect(git(web, ["diff", "--staged", "--name-only"])).toContain("new.txt"); + + const invalidStageResponse = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "api/../web/new.txt" }), + }); + expect(invalidStageResponse.status).toBe(400); + } finally { + server.stop(); + } + }, 15_000); + }); +}); diff --git a/packages/server/review-workspace.ts b/packages/server/review-workspace.ts new file mode 100644 index 000000000..c474c7ad4 --- /dev/null +++ b/packages/server/review-workspace.ts @@ -0,0 +1,57 @@ +import { + canStageFiles, + getVcsContext, + getVcsFileContentsForDiff, + runVcsDiff, + stageFile, + unstageFile, +} from "./vcs"; +import { + WorkspaceReviewSession, + type WorkspaceReviewBuildOptions, + type WorkspaceRepoRuntimeState, +} from "@plannotator/shared/review-workspace"; + +export { + WorkspaceReviewSession, + mapRepoDiffTypeToWorkspaceMode, + mapWorkspaceModeToRepoDiffType, + resolveWorkspaceInitialDiffType, + type WorkspaceDiffType, + type WorkspaceRepoRuntimeState, + type WorkspaceReviewPromptContext, +} from "@plannotator/shared/review-workspace"; + +export { + aggregateWorkspacePatch, + discoverWorkspaceRepoPaths, + prefixWorkspacePatchPaths as prefixPatchPaths, + resolveWorkspaceFilePath, + type WorkspacePatchAggregate, +} from "@plannotator/shared/review-workspace-node"; + +export type LocalWorkspaceReview = WorkspaceReviewSession; + +const workspaceRuntime = { + getVcsContext, + runVcsDiff, + getVcsFileContentsForDiff, + canStageFiles, + stageFile, + unstageFile, +}; + +export async function buildLocalWorkspaceReview( + root: string, + options: WorkspaceReviewBuildOptions = {}, +): Promise { + return WorkspaceReviewSession.create(workspaceRuntime, root, options); +} + +export async function buildWorkspaceLocalRepos( + root: string, + options: WorkspaceReviewBuildOptions = {}, +): Promise { + const session = await buildLocalWorkspaceReview(root, options); + return session.repos; +} diff --git a/packages/server/review.ts b/packages/server/review.ts index a084b98a3..8ec66f6f0 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -12,6 +12,7 @@ import { isRemoteSession, getServerHostname, getServerPort } from "./remote"; import type { Origin } from "@plannotator/shared/agents"; import { type DiffType, type GitContext, runVcsDiff, getVcsFileContentsForDiff, canStageFiles, stageFile, unstageFile, resolveVcsCwd, validateFilePath, getVcsContext, detectRemoteDefaultCompareTarget, gitRuntime } from "./vcs"; +import { basename } from "node:path"; import { parseWorktreeDiffType, resolveBaseBranch } from "@plannotator/shared/review-core"; import { getPRDiffScopeOptions, @@ -36,7 +37,7 @@ import { parseCodexOutput, transformReviewFindings, } from "./codex-review"; -import { buildAgentReviewUserMessage } from "./agent-review-message"; +import { buildAgentReviewUserMessage, buildAgentReviewUserMessageForTarget, type WorkspaceReviewPromptContext } from "./agent-review-message"; import { CLAUDE_REVIEW_PROMPT, buildClaudeCommand, @@ -49,6 +50,7 @@ import { type PRMetadata, type PRReviewFileComment, type PRStackTree, type PRLis import { AI_QUERY_ENDPOINT, createAIRuntime } from "./ai-runtime"; import type { AIEndpoints } from "@plannotator/ai"; import { isWSL } from "./browser"; +import type { LocalWorkspaceReview, WorkspaceDiffType } from "./review-workspace"; import { handleCodeNavResolve, extractChangedFiles } from "./code-nav"; // Re-export utilities @@ -72,9 +74,11 @@ export interface ReviewServerOptions { /** Origin identifier for UI customization */ origin?: Origin; /** Current diff type being displayed */ - diffType?: DiffType; + diffType?: DiffType | WorkspaceDiffType; /** Git context with branch info and available diff options */ gitContext?: GitContext; + /** Local parent directory containing multiple child VCS repositories. */ + workspace?: LocalWorkspaceReview; /** * Initial base branch the caller used to compute `rawPatch`. When a caller * overrides the detected default (e.g. Pi's `openCodeReview` accepting a @@ -140,6 +144,8 @@ export async function startReviewServer( let prMetadata = options.prMetadata; const isPRMode = !!prMetadata; + const workspace = options.workspace; + const isWorkspaceMode = !!workspace; const hasLocalAccess = !!gitContext; const sessionVcsType = gitContext?.vcsType; let draftKey = contentHash(options.rawPatch); @@ -151,7 +157,7 @@ export async function startReviewServer( // Mutable state for diff switching let currentPatch = options.rawPatch; let currentGitRef = options.gitRef; - let currentDiffType: DiffType = options.diffType || "uncommitted"; + let currentDiffType: DiffType | WorkspaceDiffType = options.diffType || workspace?.diffType || "uncommitted"; let currentError = options.error; let currentHideWhitespace = loadConfig().diffOptions?.hideWhitespace ?? false; let originalPRPatch = options.rawPatch; @@ -188,11 +194,16 @@ export async function startReviewServer( // Agent jobs — background process manager (late-binds serverUrl via getter) let serverUrl = ""; const resolveAgentCwd = (): string => { + if (workspace) return workspace.root; if (options.worktreePool && prMetadata) { const poolPath = options.worktreePool.resolve(prMetadata.url); if (poolPath) return poolPath; } - return options.agentCwd ?? resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); + return options.agentCwd ?? resolveVcsCwd(currentDiffType as DiffType, gitContext?.cwd) ?? process.cwd(); + }; + const getWorkspacePromptContext = (): WorkspaceReviewPromptContext | undefined => { + if (!workspace) return undefined; + return workspace.getPromptContext(); }; const agentJobs = createAgentJobHandler({ mode: "review", @@ -201,19 +212,27 @@ export async function startReviewServer( async buildCommand(provider, config) { const cwd = resolveAgentCwd(); - const hasAgentLocalAccess = !!options.worktreePool || !!options.agentCwd || !!gitContext; - const userMessageOptions = { defaultBranch: currentBase, hasLocalAccess: hasAgentLocalAccess, prDiffScope: currentPRDiffScope }; + const workspacePrompt = getWorkspacePromptContext(); + const hasAgentLocalAccess = !!workspacePrompt || !!options.worktreePool || !!options.agentCwd || !!gitContext; + const userMessageOptions = { + defaultBranch: currentBase, + hasLocalAccess: hasAgentLocalAccess, + prDiffScope: currentPRDiffScope, + ...(workspacePrompt && { workspace: workspacePrompt }), + }; // Snapshot the diff context at launch — stored on the job so // downstream "Copy All" produces the same markdown as /api/feedback // would right now, even if the reviewer switches modes/bases later. // Skipped in PR mode (prMetadata carries equivalent context). - const worktreeParts = currentDiffType.startsWith("worktree:") - ? parseWorktreeDiffType(currentDiffType) + const worktreeParts = String(currentDiffType).startsWith("worktree:") + ? parseWorktreeDiffType(currentDiffType as DiffType) : null; const launchPrUrl = prMetadata?.url; const launchDiffScope = isPRMode ? currentPRDiffScope : undefined; - const diffContext: AgentJobInfo["diffContext"] | undefined = prMetadata + const diffContext: AgentJobInfo["diffContext"] | undefined = workspacePrompt + ? { mode: "workspace", worktreePath: null } + : prMetadata ? undefined : { mode: (worktreeParts?.subType ?? currentDiffType) as string, @@ -225,7 +244,7 @@ export async function startReviewServer( const built = await tour.buildCommand({ cwd, patch: currentPatch, - diffType: currentDiffType, + diffType: currentDiffType as DiffType, options: userMessageOptions, prMetadata, config, @@ -233,7 +252,14 @@ export async function startReviewServer( return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext } : built; } - const userMessage = buildAgentReviewUserMessage(currentPatch, currentDiffType, userMessageOptions, prMetadata); + const userMessage = workspacePrompt + ? buildAgentReviewUserMessageForTarget({ + kind: "workspace", + patch: currentPatch, + workspace: workspacePrompt, + }) + : buildAgentReviewUserMessage(currentPatch, currentDiffType as DiffType, userMessageOptions, prMetadata); + const jobLabel = workspacePrompt ? "Workspace Review" : "Code Review"; if (provider === "codex") { const model = typeof config?.model === "string" && config.model ? config.model : undefined; @@ -242,7 +268,7 @@ export async function startReviewServer( const outputPath = generateOutputPath(); const prompt = CODEX_REVIEW_SYSTEM_PROMPT + "\n\n---\n\n" + userMessage; const command = await buildCodexCommand({ cwd, outputPath, prompt, model, reasoningEffort, fastMode }); - return { command, outputPath, prompt, cwd, label: "Code Review", model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; + return { command, outputPath, prompt, cwd, label: jobLabel, model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; } if (provider === "claude") { @@ -250,7 +276,7 @@ export async function startReviewServer( const effort = typeof config?.effort === "string" && config.effort ? config.effort : undefined; const prompt = CLAUDE_REVIEW_PROMPT + "\n\n---\n\n" + userMessage; const { command, stdinPrompt } = buildClaudeCommand(prompt, model, effort); - return { command, stdinPrompt, prompt, cwd, label: "Code Review", captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; + return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext }; } return null; @@ -283,7 +309,13 @@ export async function startReviewServer( }; if (output.findings.length > 0) { - const annotations = transformReviewFindings(output.findings, job.source, cwd, "Codex") + const annotations = transformReviewFindings( + output.findings, + job.source, + cwd, + "Codex", + workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined, + ) .map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) })); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[codex-review] addAnnotations error:`, result.error); @@ -307,7 +339,12 @@ export async function startReviewServer( }; if (output.findings.length > 0) { - const annotations = transformClaudeFindings(output.findings, job.source, cwd) + const annotations = transformClaudeFindings( + output.findings, + job.source, + cwd, + workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined, + ) .map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) })); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[claude-review] addAnnotations error:`, result.error); @@ -344,6 +381,8 @@ export async function startReviewServer( // In PR mode, derive from metadata instead of local git let repoInfo = isPRMode && prMetadata ? { display: getDisplayRepo(prMetadata), branch: `${getMRLabel(prMetadata)} ${getMRNumberLabel(prMetadata)}` } + : workspace + ? { display: basename(workspace.root), branch: "Workspace" } : await getRepoInfo(); if (gitContext?.repository?.displayFallback) { repoInfo = { @@ -447,18 +486,21 @@ export async function startReviewServer( rawPatch: currentPatch, gitRef: currentGitRef, origin, - diffType: hasLocalAccess ? currentDiffType : undefined, + mode: isWorkspaceMode ? "workspace" : undefined, + diffType: hasLocalAccess || isWorkspaceMode ? currentDiffType : undefined, // Echo the active base so a page refresh or reconnect rehydrates // the picker to what the server is actually using — not the // detected default. base: hasLocalAccess ? currentBase : undefined, hideWhitespace: currentHideWhitespace, + ...(workspace && { diffOptions: workspace.diffOptions }), gitContext: hasLocalAccess ? gitContext : undefined, sharingEnabled, shareBaseUrl, repoInfo, isWSL: wslFlag, ...(options.agentCwd && { agentCwd: options.agentCwd }), + ...(workspace && { agentCwd: workspace.root }), ...(isPRMode && { prMetadata, platformUser, @@ -475,14 +517,14 @@ export async function startReviewServer( // API: Switch diff type (requires local file access) if (url.pathname === "/api/diff/switch" && req.method === "POST") { - if (!hasLocalAccess) { + if (!hasLocalAccess && !workspace) { return Response.json( { error: "Not available without local file access" }, { status: 400 }, ); } try { - const body = (await req.json()) as { diffType: DiffType; base?: string; hideWhitespace?: boolean }; + const body = (await req.json()) as { diffType: DiffType | WorkspaceDiffType; base?: string; hideWhitespace?: boolean }; let newDiffType = body.diffType; if (!newDiffType) { @@ -496,6 +538,27 @@ export async function startReviewServer( currentHideWhitespace = body.hideWhitespace; } + if (workspace) { + const snapshot = await workspace.rebuild({ + diffType: newDiffType, + hideWhitespace: currentHideWhitespace, + }); + currentPatch = snapshot.rawPatch; + currentGitRef = snapshot.gitRef; + currentDiffType = workspace.diffType; + currentError = snapshot.error; + draftKey = contentHash(currentPatch); + + return Response.json({ + rawPatch: currentPatch, + gitRef: currentGitRef, + diffType: currentDiffType, + diffOptions: workspace.diffOptions, + hideWhitespace: currentHideWhitespace, + ...(currentError && { error: currentError }), + }); + } + // Guard against non-string payloads — resolveBaseBranch calls // string methods and would throw a TypeError otherwise. Mirrors // Pi's guard so both runtimes validate identically. @@ -504,7 +567,7 @@ export async function startReviewServer( const defaultCwd = gitContext?.cwd; // Run the new diff - const result = await runVcsDiff(newDiffType, base, defaultCwd, { + const result = await runVcsDiff(newDiffType as DiffType, base, defaultCwd, { hideWhitespace: currentHideWhitespace, }); @@ -524,7 +587,7 @@ export async function startReviewServer( let updatedContext: GitContext | undefined; if (gitContext) { try { - const effectiveCwd = resolveVcsCwd(newDiffType, gitContext.cwd); + const effectiveCwd = resolveVcsCwd(newDiffType as DiffType, gitContext.cwd); updatedContext = await getVcsContext(effectiveCwd, sessionVcsType); } catch { /* best-effort */ @@ -767,6 +830,18 @@ export async function startReviewServer( } } + if (workspace) { + try { + const result = await workspace.getFileContents(filePath, oldPath); + return Response.json(result); + } catch (error) { + return Response.json( + { error: error instanceof Error ? error.message : "No file access available" }, + { status: 400 }, + ); + } + } + // Full-stack PR mode uses local git for file expansion because // the patch is no longer the platform's layer diff. const fileContentCwd = (options.worktreePool && prMetadata) ? options.worktreePool.resolve(prMetadata.url) : options.agentCwd; @@ -802,7 +877,7 @@ export async function startReviewServer( const base = resolveReviewBase(requestedBase); const defaultCwd = gitContext?.cwd; const result = await getVcsFileContentsForDiff( - currentDiffType, + currentDiffType as DiffType, base, filePath, oldPath, @@ -828,7 +903,7 @@ export async function startReviewServer( // API: Code navigation (search-based symbol resolution) if (url.pathname === "/api/code-nav/resolve" && req.method === "POST") { - const hasCodeNavAccess = !!gitContext || !!options.agentCwd || !!options.worktreePool; + const hasCodeNavAccess = !!workspace || !!gitContext || !!options.agentCwd || !!options.worktreePool; if (!hasCodeNavAccess) { return Response.json( { error: "Code navigation requires local access" }, @@ -842,7 +917,7 @@ export async function startReviewServer( // API: Code navigation file preview (read file from working tree) if (url.pathname === "/api/code-nav/file" && req.method === "GET") { - const hasCodeNavAccess = !!gitContext || !!options.agentCwd || !!options.worktreePool; + const hasCodeNavAccess = !!workspace || !!gitContext || !!options.agentCwd || !!options.worktreePool; if (!hasCodeNavAccess) { return Response.json({ error: "Code navigation requires local access" }, { status: 400 }); } @@ -864,23 +939,39 @@ export async function startReviewServer( // API: Stage / unstage a file (disabled when VCS doesn't support it) if (url.pathname === "/api/git-add" && req.method === "POST") { - const stageCwd = resolveVcsCwd(currentDiffType, gitContext?.cwd); - if (isPRMode || !(await canStageFiles(currentDiffType, stageCwd))) { - return Response.json( - { error: "Staging not available" }, - { status: 400 }, - ); - } try { - const body = (await req.json()) as { filePath: string; undo?: boolean }; - if (!body.filePath) { + const body = (await req.json()) as { filePath?: unknown; undo?: boolean }; + if (typeof body.filePath !== "string" || !body.filePath) { return Response.json({ error: "Missing filePath" }, { status: 400 }); } + try { validateFilePath(body.filePath); } catch { + return Response.json({ error: "Invalid path" }, { status: 400 }); + } + + if (workspace) { + try { + await workspace.stageFile(body.filePath, body.undo); + return Response.json({ ok: true }); + } catch (error) { + return Response.json( + { error: error instanceof Error ? error.message : "Failed to stage file" }, + { status: 400 }, + ); + } + } + + const stageCwd = resolveVcsCwd(currentDiffType as DiffType, gitContext?.cwd); + if (isPRMode || !(await canStageFiles(currentDiffType as DiffType, stageCwd))) { + return Response.json( + { error: "Staging not available" }, + { status: 400 }, + ); + } if (body.undo) { - await unstageFile(currentDiffType, body.filePath, stageCwd); + await unstageFile(currentDiffType as DiffType, body.filePath, stageCwd); } else { - await stageFile(currentDiffType, body.filePath, stageCwd); + await stageFile(currentDiffType as DiffType, body.filePath, stageCwd); } return Response.json({ ok: true }); diff --git a/packages/server/tour/tour-review.test.ts b/packages/server/tour/tour-review.test.ts index ec485c198..5bd3ea94f 100644 --- a/packages/server/tour/tour-review.test.ts +++ b/packages/server/tour/tour-review.test.ts @@ -141,5 +141,6 @@ describe("buildTourClaudeCommand", () => { expect(allowedTools).toContain("Bash(jj file show:*)"); expect(allowedTools).toContain("Bash(jj cat:*)"); expect(allowedTools).toContain("Bash(jj bookmark list:*)"); + expect(allowedTools).toContain("Bash(git -C:*)"); }); }); diff --git a/packages/server/tour/tour-review.ts b/packages/server/tour/tour-review.ts index c2b378b62..da23dafd0 100644 --- a/packages/server/tour/tour-review.ts +++ b/packages/server/tour/tour-review.ts @@ -3,7 +3,7 @@ import { homedir, tmpdir } from "node:os"; import { mkdir, writeFile, readFile, unlink } from "node:fs/promises"; import type { DiffType } from "../vcs"; import type { PRMetadata } from "../pr"; -import { getLocalDiffInstruction } from "../agent-review-message"; +import { buildWorkspacePromptContextLines, getLocalDiffInstruction, type WorkspaceReviewPromptContext } from "../agent-review-message"; import type { CodeTourOutput, TourDiffAnchor, @@ -288,9 +288,13 @@ callouts. The primary question is "what does this change do and why?" not export function buildTourUserMessage( patch: string, diffType: DiffType, - options?: { defaultBranch?: string; hasLocalAccess?: boolean; prDiffScope?: string }, + options?: { defaultBranch?: string; hasLocalAccess?: boolean; prDiffScope?: string; workspace?: WorkspaceReviewPromptContext }, prMetadata?: PRMetadata, ): string { + if (options?.workspace) { + return buildWorkspaceTourUserMessage(patch, options.workspace); + } + if (prMetadata) { if (options?.prDiffScope === "full-stack") { return [ @@ -332,6 +336,21 @@ export function buildTourUserMessage( ].join("\n"); } +function buildWorkspaceTourUserMessage( + patch: string, + workspace: WorkspaceReviewPromptContext, +): string { + return [ + "Walk the reviewer through the local workspace changes across multiple nested VCS repositories.", + "", + ...buildWorkspacePromptContextLines(workspace), + "", + "```diff", + patch, + "```", + ].join("\n"); +} + export interface TourClaudeCommandResult { command: string[]; stdinPrompt: string; @@ -344,7 +363,7 @@ export function buildTourClaudeCommand(prompt: string, model: string = "sonnet", "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(git show-ref:*)", "Bash(git -C:*)", "Bash(jj status:*)", "Bash(jj diff:*)", "Bash(jj log:*)", "Bash(jj show:*)", "Bash(jj file show:*)", "Bash(jj cat:*)", "Bash(jj bookmark list:*)", @@ -474,7 +493,7 @@ export interface TourSessionBuildCommandOptions { cwd: string; patch: string; diffType: DiffType; - options?: { defaultBranch?: string; hasLocalAccess?: boolean }; + options?: { defaultBranch?: string; hasLocalAccess?: boolean; prDiffScope?: string; workspace?: WorkspaceReviewPromptContext }; prMetadata?: PRMetadata; config?: Record; } diff --git a/packages/server/vcs.ts b/packages/server/vcs.ts index 08871fd3d..ee3ea6b3d 100644 --- a/packages/server/vcs.ts +++ b/packages/server/vcs.ts @@ -46,6 +46,7 @@ const api = createVcsApi([ export const { detectVcs, + detectManagedVcs, getVcsContext, detectRemoteDefaultCompareTarget, prepareLocalReviewDiff, @@ -77,3 +78,4 @@ export { parseWorktreeDiffType, validateFilePath, } from "@plannotator/shared/vcs-core"; + diff --git a/packages/shared/agent-jobs.ts b/packages/shared/agent-jobs.ts index 1cfedf391..993f10519 100644 --- a/packages/shared/agent-jobs.ts +++ b/packages/shared/agent-jobs.ts @@ -75,6 +75,10 @@ export interface AgentJobInfo { diffScope?: string; /** Diff context at launch time (see AgentJobDiffContext). */ diffContext?: AgentJobDiffContext; + /** Workspace repo snapshot at launch — prevents race with UI selection changes. */ + repoId?: string; + repoLabel?: string; + repoCwd?: string; } export interface AgentCapability { diff --git a/packages/shared/package.json b/packages/shared/package.json index 21c1f17b8..4bd440939 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -9,6 +9,8 @@ "./feedback-templates": "./feedback-templates.ts", "./jj-core": "./jj-core.ts", "./review-core": "./review-core.ts", + "./review-workspace": "./review-workspace.ts", + "./review-workspace-node": "./review-workspace-node.ts", "./review-args": "./review-args.ts", "./vcs-core": "./vcs-core.ts", "./checklist": "./checklist.ts", diff --git a/packages/shared/review-workspace-node.ts b/packages/shared/review-workspace-node.ts new file mode 100644 index 000000000..7f685047d --- /dev/null +++ b/packages/shared/review-workspace-node.ts @@ -0,0 +1,327 @@ +import { existsSync, readdirSync, type Dirent } from "node:fs"; +import { basename, relative, resolve } from "node:path"; + +import { validateFilePath } from "./review-core"; + +const SKIP_DIRS = new Set([ + ".git", + ".jj", + "node_modules", + ".turbo", + ".next", + "dist", + "build", + "coverage", +]); + +const VCS_MARKERS = [".jj", ".git"] as const; + +export interface WorkspacePathEntry { + label: string; +} + +export interface WorkspacePatchEntry { + label: string; + selected: boolean; + rawPatch: string; + gitRef?: string; + error?: string; +} + +export interface WorkspacePathResolution { + repo: T; + repoRelativePath: string; +} + +export interface WorkspacePatchAggregate { + rawPatch: string; + gitRef: string; + errors: string[]; +} + +export function normalizeWorkspacePath(path: string): string { + return path.replace(/\\/g, "/").replace(/^\/+/, ""); +} + +function unquoteGitPath(value: string): string { + if (!value.startsWith('"') || !value.endsWith('"')) return value; + try { + return JSON.parse(value) as string; + } catch { + return value.slice(1, -1) + .replace(/\\"/g, '"') + .replace(/\\\\/g, "\\") + .replace(/\\t/g, "\t") + .replace(/\\n/g, "\n"); + } +} + +function quoteGitPath(value: string): string { + if (!/[\s"\\]/.test(value)) return value; + return JSON.stringify(value); +} + +function parsePatchPathToken(token: string, side: "a" | "b"): string | null { + if (token === "/dev/null") return "/dev/null"; + const unquoted = unquoteGitPath(token); + const prefix = `${side}/`; + return unquoted.startsWith(prefix) ? unquoted.slice(prefix.length) : null; +} + +function scanHeaderToken(input: string): { token: string; rest: string } | null { + const trimmed = input.trimStart(); + if (!trimmed) return null; + + if (trimmed.startsWith('"')) { + let escaped = false; + for (let i = 1; i < trimmed.length; i += 1) { + const char = trimmed[i]; + if (escaped) { + escaped = false; + continue; + } + if (char === "\\") { + escaped = true; + continue; + } + if (char === '"') { + return { token: trimmed.slice(0, i + 1), rest: trimmed.slice(i + 1) }; + } + } + return null; + } + + const space = trimmed.indexOf(" "); + if (space === -1) return { token: trimmed, rest: "" }; + return { token: trimmed.slice(0, space), rest: trimmed.slice(space + 1) }; +} + +function parseDiffGitHeader(line: string): { oldPath?: string; newPath?: string } { + const prefix = "diff --git "; + if (!line.startsWith(prefix)) return {}; + + const rest = line.slice(prefix.length); + if (rest.trimStart().startsWith('"')) { + const first = scanHeaderToken(rest); + const second = first ? scanHeaderToken(first.rest) : null; + if (first && second) { + const oldPath = parsePatchPathToken(first.token, "a"); + const newPath = parsePatchPathToken(second.token, "b"); + return { + oldPath: oldPath && oldPath !== "/dev/null" ? oldPath : undefined, + newPath: newPath && newPath !== "/dev/null" ? newPath : undefined, + }; + } + } + + const match = line.match(/^diff --git a\/(.+) b\/(.+)$/); + if (!match) return {}; + return { oldPath: match[1], newPath: match[2] }; +} + +function formatPatchPathToken(side: "a" | "b", filePath: string): string { + if (filePath === "/dev/null") return filePath; + return quoteGitPath(`${side}/${filePath}`); +} + +function prefixRepoPath(label: string, filePath: string): string { + if (filePath === "/dev/null") return filePath; + const normalizedFilePath = normalizeWorkspacePath(filePath); + return `${normalizeWorkspacePath(label)}/${normalizedFilePath}`; +} + +export function parseDiffFilePathLines(lines: string[]): { oldPath?: string; newPath?: string } { + let oldPath: string | undefined; + let newPath: string | undefined; + + for (const line of lines) { + if (line.startsWith("@@ ") || line === "GIT binary patch") break; + if (line.startsWith("--- ")) { + const parsed = parsePatchPathToken(line.slice(4), "a"); + if (parsed && parsed !== "/dev/null") oldPath = parsed; + } else if (line.startsWith("+++ ")) { + const parsed = parsePatchPathToken(line.slice(4), "b"); + if (parsed && parsed !== "/dev/null") newPath = parsed; + } + } + + return { oldPath, newPath }; +} + +function parseDiffMetadataPathLines(lines: string[]): { oldPath?: string; newPath?: string } { + let oldPath: string | undefined; + let newPath: string | undefined; + + for (const line of lines) { + if (line.startsWith("rename from ") || line.startsWith("copy from ")) { + oldPath = line.slice(line.indexOf(" from ") + " from ".length); + } else if (line.startsWith("rename to ") || line.startsWith("copy to ")) { + newPath = line.slice(line.indexOf(" to ") + " to ".length); + } + } + + return { oldPath, newPath }; +} + +function rewritePatchLine(line: string, label: string): string { + if (line.startsWith("--- ")) { + const parsed = parsePatchPathToken(line.slice(4), "a"); + if (parsed === "/dev/null") return line; + if (parsed) return `--- ${formatPatchPathToken("a", prefixRepoPath(label, parsed))}`; + return line; + } + + if (line.startsWith("+++ ")) { + const parsed = parsePatchPathToken(line.slice(4), "b"); + if (parsed === "/dev/null") return line; + if (parsed) return `+++ ${formatPatchPathToken("b", prefixRepoPath(label, parsed))}`; + return line; + } + + if (line.startsWith("rename from ")) { + return `rename from ${prefixRepoPath(label, line.slice("rename from ".length))}`; + } + if (line.startsWith("rename to ")) { + return `rename to ${prefixRepoPath(label, line.slice("rename to ".length))}`; + } + if (line.startsWith("copy from ")) { + return `copy from ${prefixRepoPath(label, line.slice("copy from ".length))}`; + } + if (line.startsWith("copy to ")) { + return `copy to ${prefixRepoPath(label, line.slice("copy to ".length))}`; + } + + return line; +} + +function rewritePatchChunk(chunk: string, label: string): string { + const lines = chunk.split("\n"); + const fromFileLines = parseDiffFilePathLines(lines); + const fromMetadata = parseDiffMetadataPathLines(lines); + const fromHeader = parseDiffGitHeader(lines[0] ?? ""); + const oldPath = fromFileLines.oldPath ?? fromMetadata.oldPath ?? fromHeader.oldPath; + const newPath = fromFileLines.newPath ?? fromMetadata.newPath ?? fromHeader.newPath; + const headerOldPath = oldPath ?? newPath; + const headerNewPath = newPath ?? oldPath; + + if (lines[0]?.startsWith("diff --git ") && headerOldPath && headerNewPath) { + const prefixedOld = prefixRepoPath(label, headerOldPath); + const prefixedNew = prefixRepoPath(label, headerNewPath); + lines[0] = `diff --git ${formatPatchPathToken("a", prefixedOld)} ${formatPatchPathToken("b", prefixedNew)}`; + } + + return lines.map((line, index) => index === 0 ? line : rewritePatchLine(line, label)).join("\n"); +} + +export function prefixWorkspacePatchPaths(rawPatch: string, label: string): string { + if (!rawPatch.trim()) return rawPatch; + if (!rawPatch.includes("diff --git ")) { + return rawPatch + .split("\n") + .map((line) => rewritePatchLine(line, label)) + .join("\n"); + } + + const chunks = rawPatch.split(/^diff --git /m); + const prefix = chunks.shift() ?? ""; + return prefix + chunks.map((chunk) => rewritePatchChunk(`diff --git ${chunk}`, label)).join(""); +} + +export function resolveWorkspaceFilePath( + repos: T[], + prefixedPath: string, +): WorkspacePathResolution | null { + const normalizedPath = normalizeWorkspacePath(prefixedPath); + validateFilePath(normalizedPath); + + const sorted = [...repos].sort((a, b) => b.label.length - a.label.length); + + for (const repo of sorted) { + const label = normalizeWorkspacePath(repo.label); + const prefix = `${label}/`; + if (normalizedPath.startsWith(prefix)) { + const repoRelativePath = normalizedPath.slice(prefix.length); + if (!repoRelativePath) return null; + return { + repo, + repoRelativePath, + }; + } + } + + return null; +} + +function hasVcsMarker(dirPath: string): boolean { + return VCS_MARKERS.some((marker) => existsSync(resolve(dirPath, marker))); +} + +function collectWorkspaceRepos(root: string, current: string, results: string[]): void { + let entries: Dirent[]; + try { + entries = readdirSync(current, { withFileTypes: true }); + } catch { + return; + } + + if (current !== root && hasVcsMarker(current)) { + results.push(current); + return; + } + + for (const entry of entries) { + if (!entry.isDirectory()) continue; + if (SKIP_DIRS.has(entry.name)) continue; + collectWorkspaceRepos(root, resolve(current, entry.name), results); + } +} + +export function discoverWorkspaceRepoPaths(root: string): string[] { + const resolvedRoot = resolve(root); + const results: string[] = []; + collectWorkspaceRepos(resolvedRoot, resolvedRoot, results); + return results.sort(); +} + +function buildRepoLabel(root: string, cwd: string, used: Set): string { + const rel = normalizeWorkspacePath(relative(root, cwd)); + const preferred = rel && rel !== "" ? rel : basename(cwd); + if (!used.has(preferred)) { + used.add(preferred); + return preferred; + } + + const fallback = normalizeWorkspacePath(basename(cwd)); + if (!used.has(fallback)) { + used.add(fallback); + return fallback; + } + + let counter = 2; + let next = `${fallback}-${counter}`; + while (used.has(next)) { + counter += 1; + next = `${fallback}-${counter}`; + } + used.add(next); + return next; +} + +export function buildWorkspaceRepoLabels(root: string, repoPaths: string[]): string[] { + const resolvedRoot = resolve(root); + const usedLabels = new Set(); + return repoPaths.map((cwd) => buildRepoLabel(resolvedRoot, cwd, usedLabels)); +} + +export function aggregateWorkspacePatch(repos: WorkspacePatchEntry[]): WorkspacePatchAggregate { + const selected = repos.filter((repo) => repo.selected); + const trimmedPatches = selected + .map((repo) => repo.rawPatch) + .filter((patch) => patch.trim().length > 0) + .map((patch) => patch.replace(/\n+$/, "")); + return { + rawPatch: trimmedPatches.join("\n\n"), + gitRef: selected.map((repo) => repo.gitRef || repo.label).filter(Boolean).join(" | ") || "Workspace review", + errors: repos.flatMap((repo) => repo.error ? [`${repo.label}: ${repo.error}`] : []), + }; +} diff --git a/packages/shared/review-workspace.ts b/packages/shared/review-workspace.ts new file mode 100644 index 000000000..de612209c --- /dev/null +++ b/packages/shared/review-workspace.ts @@ -0,0 +1,438 @@ +import { isAbsolute, relative, resolve } from "node:path"; + +import type { DiffOption, DiffResult, DiffType, GitContext, GitDiffOptions } from "./review-core"; +import { + aggregateWorkspacePatch, + buildWorkspaceRepoLabels, + discoverWorkspaceRepoPaths, + normalizeWorkspacePath, + prefixWorkspacePatchPaths, + resolveWorkspaceFilePath, +} from "./review-workspace-node"; + +export type WorkspaceDiffType = + | "workspace-current" + | "workspace-staged" + | "workspace-unstaged" + | "workspace-last"; + +export type WorkspaceChildVcsType = "git" | "jj"; + +export interface WorkspaceRepoState { + id: string; + label: string; + cwd: string; + selected: boolean; + source: "local"; + vcsType?: WorkspaceChildVcsType; + diffType?: DiffType; + gitContext?: GitContext; + diffOptions?: DiffOption[]; + platformUser: string | null; + error?: string; +} + +export interface WorkspaceRepoRuntimeState extends WorkspaceRepoState { + rawPatch: string; + gitRef: string; +} + +export interface WorkspaceReviewState { + mode: "workspace"; + root: string; + diffType: WorkspaceDiffType; + diffOptions: DiffOption[]; + repos: WorkspaceRepoState[]; +} + +export interface WorkspaceReviewRuntime { + getVcsContext(cwd?: string): Promise; + runVcsDiff( + diffType: DiffType, + defaultBranch?: string, + cwd?: string, + options?: GitDiffOptions, + ): Promise; + getVcsFileContentsForDiff( + diffType: DiffType, + defaultBranch: string, + filePath: string, + oldPath?: string, + cwd?: string, + ): Promise<{ oldContent: string | null; newContent: string | null }>; + canStageFiles(diffType: string, cwd?: string): Promise; + stageFile(diffType: string, filePath: string, cwd?: string): Promise; + unstageFile(diffType: string, filePath: string, cwd?: string): Promise; +} + +export interface WorkspaceReviewBuildOptions { + requestedDiffType?: DiffType | WorkspaceDiffType; + configuredDiffType?: DiffType; + hideWhitespace?: boolean; +} + +export interface WorkspacePromptRepoContext { + label: string; + cwd: string; + vcsType?: WorkspaceChildVcsType; + gitRef?: string; +} + +export interface WorkspaceReviewPromptContext { + root: string; + repos: WorkspacePromptRepoContext[]; +} + +export interface WorkspaceDiffSnapshot { + rawPatch: string; + gitRef: string; + error?: string; +} + +const WORKSPACE_CURRENT: DiffOption = { id: "workspace-current", label: "Current changes" }; +const WORKSPACE_STAGED: DiffOption = { id: "workspace-staged", label: "Staged changes" }; +const WORKSPACE_UNSTAGED: DiffOption = { id: "workspace-unstaged", label: "Unstaged changes" }; +const WORKSPACE_LAST: DiffOption = { id: "workspace-last", label: "Last change" }; + +const WORKSPACE_DIFF_TYPES = new Set([ + "workspace-current", + "workspace-staged", + "workspace-unstaged", + "workspace-last", +]); + +function isWorkspaceDiffType(value: string | undefined): value is WorkspaceDiffType { + return !!value && WORKSPACE_DIFF_TYPES.has(value as WorkspaceDiffType); +} + +function normalizeVcsType(value: string | undefined): WorkspaceChildVcsType | undefined { + return value === "git" || value === "jj" ? value : undefined; +} + +export function mapWorkspaceModeToRepoDiffType( + workspaceDiffType: WorkspaceDiffType, + vcsType: WorkspaceChildVcsType | undefined, +): DiffType | null { + if (vcsType === "jj") { + switch (workspaceDiffType) { + case "workspace-current": + return "jj-current"; + case "workspace-last": + return "jj-last"; + default: + return null; + } + } + + if (vcsType === "git") { + switch (workspaceDiffType) { + case "workspace-current": + return "uncommitted"; + case "workspace-staged": + return "staged"; + case "workspace-unstaged": + return "unstaged"; + case "workspace-last": + return "last-commit"; + } + } + + return null; +} + +export function mapRepoDiffTypeToWorkspaceMode( + diffType: DiffType | WorkspaceDiffType | undefined, +): WorkspaceDiffType | undefined { + if (isWorkspaceDiffType(diffType)) return diffType; + switch (diffType) { + case "uncommitted": + case "jj-current": + return "workspace-current"; + case "staged": + return "workspace-staged"; + case "unstaged": + return "workspace-unstaged"; + case "last-commit": + case "jj-last": + return "workspace-last"; + default: + return undefined; + } +} + +export function resolveWorkspaceInitialDiffType( + repos: WorkspaceRepoRuntimeState[], + requested?: DiffType | WorkspaceDiffType, + configured?: DiffType, +): WorkspaceDiffType { + for (const candidate of [ + mapRepoDiffTypeToWorkspaceMode(requested), + mapRepoDiffTypeToWorkspaceMode(configured), + "workspace-current" as const, + ]) { + if (candidate && workspaceModeAvailable(repos, candidate)) return candidate; + } + return "workspace-current"; +} + +export function workspaceModeAvailable( + repos: WorkspaceRepoRuntimeState[], + diffType: WorkspaceDiffType, +): boolean { + if (diffType === "workspace-staged" || diffType === "workspace-unstaged") { + const detectedRepos = repos.filter((repo) => repo.vcsType); + return detectedRepos.length > 0 && detectedRepos.every((repo) => repo.vcsType === "git"); + } + return true; +} + +export function getWorkspaceDiffOptions(repos: WorkspaceRepoRuntimeState[]): DiffOption[] { + const options = [WORKSPACE_CURRENT]; + if (workspaceModeAvailable(repos, "workspace-staged")) { + options.push(WORKSPACE_STAGED, WORKSPACE_UNSTAGED); + } + options.push(WORKSPACE_LAST); + return options; +} + +function aggregateRepos(repos: WorkspaceRepoRuntimeState[]): WorkspaceDiffSnapshot { + const aggregate = aggregateWorkspacePatch(repos); + return { + rawPatch: aggregate.rawPatch, + gitRef: aggregate.gitRef, + error: aggregate.errors.length > 0 ? aggregate.errors.join("\n") : undefined, + }; +} + +function normalizeAgentPath(root: string, repos: WorkspaceRepoRuntimeState[], filePath: string): string { + const normalized = normalizeWorkspacePath(filePath); + if (resolveWorkspaceFilePath(repos, normalized)) return normalized; + + const sorted = [...repos].sort((a, b) => b.cwd.length - a.cwd.length); + for (const repo of sorted) { + const rel = normalizeWorkspacePath(relative(repo.cwd, filePath)); + if (rel && !rel.startsWith("..") && !rel.startsWith("/")) { + return `${normalizeWorkspacePath(repo.label)}/${rel}`; + } + } + + const rootRel = normalizeWorkspacePath(relative(root, filePath)); + if (rootRel && !rootRel.startsWith("..") && !rootRel.startsWith("/")) { + if (resolveWorkspaceFilePath(repos, rootRel)) return rootRel; + } + + const changedRepos = repos.filter((repo) => repo.selected && repo.rawPatch.trim()); + if (!isAbsolute(filePath) && changedRepos.length === 1 && normalized && !normalized.startsWith("..")) { + return `${normalizeWorkspacePath(changedRepos[0].label)}/${normalized}`; + } + + if (rootRel && !rootRel.startsWith("..") && !rootRel.startsWith("/")) return rootRel; + return normalized; +} + +export class WorkspaceReviewSession implements WorkspaceReviewState { + readonly mode = "workspace" as const; + readonly root: string; + repos: WorkspaceRepoRuntimeState[]; + diffType: WorkspaceDiffType; + diffOptions: DiffOption[]; + rawPatch: string; + gitRef: string; + error?: string; + hideWhitespace: boolean; + + private constructor( + private readonly runtime: WorkspaceReviewRuntime, + root: string, + repos: WorkspaceRepoRuntimeState[], + diffType: WorkspaceDiffType, + hideWhitespace: boolean, + ) { + this.root = resolve(root); + this.repos = repos; + this.diffType = diffType; + this.hideWhitespace = hideWhitespace; + this.diffOptions = getWorkspaceDiffOptions(repos); + const snapshot = aggregateRepos(repos); + this.rawPatch = snapshot.rawPatch; + this.gitRef = snapshot.gitRef; + this.error = snapshot.error; + } + + static async create( + runtime: WorkspaceReviewRuntime, + root: string, + options: WorkspaceReviewBuildOptions = {}, + ): Promise { + const resolvedRoot = resolve(root); + const repoPaths = discoverWorkspaceRepoPaths(resolvedRoot); + const labels = buildWorkspaceRepoLabels(resolvedRoot, repoPaths); + + const repos = await Promise.all(repoPaths.map(async (cwd, index) => { + const label = labels[index]; + try { + const gitContext = await runtime.getVcsContext(cwd); + const vcsType = normalizeVcsType(gitContext.vcsType); + return { + id: `repo-${index + 1}`, + label, + cwd, + selected: false, + source: "local", + vcsType, + gitContext, + diffOptions: gitContext.diffOptions, + platformUser: null, + rawPatch: "", + gitRef: "", + } satisfies WorkspaceRepoRuntimeState; + } catch (error) { + return { + id: `repo-${index + 1}`, + label, + cwd, + selected: false, + source: "local", + platformUser: null, + rawPatch: "", + gitRef: "", + error: error instanceof Error ? error.message : String(error), + } satisfies WorkspaceRepoRuntimeState; + } + })); + + const diffType = resolveWorkspaceInitialDiffType( + repos.filter((repo) => repo.vcsType), + options.requestedDiffType, + options.configuredDiffType, + ); + const session = new WorkspaceReviewSession( + runtime, + resolvedRoot, + repos, + diffType, + options.hideWhitespace ?? false, + ); + await session.rebuild({ diffType, hideWhitespace: options.hideWhitespace }); + return session; + } + + async rebuild(options: { + diffType?: DiffType | WorkspaceDiffType; + hideWhitespace?: boolean; + } = {}): Promise { + const requestedMode = mapRepoDiffTypeToWorkspaceMode(options.diffType) ?? this.diffType; + if (!workspaceModeAvailable(this.repos, requestedMode)) { + throw new Error(`Workspace diff mode is not available: ${requestedMode}`); + } + + if (typeof options.hideWhitespace === "boolean") { + this.hideWhitespace = options.hideWhitespace; + } + + const repos = await Promise.all(this.repos.map(async (repo) => { + if (!repo.vcsType || !repo.gitContext) { + return { ...repo, selected: false, rawPatch: "", gitRef: "" }; + } + + const repoDiffType = mapWorkspaceModeToRepoDiffType(requestedMode, repo.vcsType); + if (!repoDiffType) { + return { + ...repo, + selected: false, + diffType: undefined, + rawPatch: "", + gitRef: "", + error: `Workspace diff mode ${requestedMode} is not available for ${repo.vcsType}`, + }; + } + + try { + const diff = await this.runtime.runVcsDiff(repoDiffType, repo.gitContext.defaultBranch, repo.cwd, { + hideWhitespace: this.hideWhitespace, + }); + return { + ...repo, + selected: !!diff.patch.trim(), + diffType: repoDiffType, + rawPatch: prefixWorkspacePatchPaths(diff.patch, repo.label), + gitRef: diff.label, + error: diff.error, + }; + } catch (error) { + return { + ...repo, + selected: false, + diffType: repoDiffType, + rawPatch: "", + gitRef: "", + error: error instanceof Error ? error.message : String(error), + }; + } + })); + + this.repos = repos; + this.diffType = requestedMode; + this.diffOptions = getWorkspaceDiffOptions(repos); + const snapshot = aggregateRepos(repos); + this.rawPatch = snapshot.rawPatch; + this.gitRef = snapshot.gitRef; + this.error = snapshot.error; + return snapshot; + } + + getPromptContext(): WorkspaceReviewPromptContext { + return { + root: this.root, + repos: this.repos + .filter((repo) => repo.selected && repo.rawPatch.trim()) + .map((repo) => ({ + label: repo.label, + cwd: repo.cwd, + vcsType: repo.vcsType, + gitRef: repo.gitRef, + })), + }; + } + + normalizeAnnotationPath(filePath: string): string { + return normalizeAgentPath(this.root, this.repos, filePath); + } + + async getFileContents( + filePath: string, + oldPath?: string, + ): Promise<{ oldContent: string | null; newContent: string | null }> { + const resolved = resolveWorkspaceFilePath(this.repos, filePath); + if (!resolved) throw new Error("File is not part of this workspace review"); + + const resolvedOld = oldPath ? resolveWorkspaceFilePath(this.repos, oldPath) : null; + if (oldPath && (!resolvedOld || resolvedOld.repo.id !== resolved.repo.id)) { + throw new Error("Old path is not part of the same workspace repository"); + } + + return this.runtime.getVcsFileContentsForDiff( + resolved.repo.diffType ?? mapWorkspaceModeToRepoDiffType(this.diffType, resolved.repo.vcsType) ?? "uncommitted", + resolved.repo.gitContext?.defaultBranch ?? "main", + resolved.repoRelativePath, + resolvedOld?.repoRelativePath, + resolved.repo.cwd, + ); + } + + async stageFile(filePath: string, undo?: boolean): Promise { + const resolved = resolveWorkspaceFilePath(this.repos, filePath); + if (!resolved) throw new Error("File is not part of this workspace review"); + + const diffType = resolved.repo.diffType ?? mapWorkspaceModeToRepoDiffType(this.diffType, resolved.repo.vcsType) ?? "uncommitted"; + if (!(await this.runtime.canStageFiles(diffType, resolved.repo.cwd))) { + throw new Error("Staging not available"); + } + + if (undo) { + await this.runtime.unstageFile(diffType, resolved.repoRelativePath, resolved.repo.cwd); + } else { + await this.runtime.stageFile(diffType, resolved.repoRelativePath, resolved.repo.cwd); + } + } +} diff --git a/packages/shared/types.ts b/packages/shared/types.ts index ac07ae68e..dcb76c38f 100644 --- a/packages/shared/types.ts +++ b/packages/shared/types.ts @@ -21,3 +21,9 @@ export type { CompareTargetPickerCopy, RepositoryContext, } from "./review-core"; + +export type { + WorkspaceDiffType, + WorkspaceRepoState, + WorkspaceReviewState, +} from "./review-workspace"; diff --git a/packages/shared/vcs-core.test.ts b/packages/shared/vcs-core.test.ts index 892b21e5c..8df03d892 100644 --- a/packages/shared/vcs-core.test.ts +++ b/packages/shared/vcs-core.test.ts @@ -108,6 +108,35 @@ describe("createVcsApi", () => { await expect(api.getVcsContext("/repo")).resolves.toMatchObject({ vcsType: "p4" }); }); + test("detectManagedVcs returns null instead of falling back when no provider detects a workspace", async () => { + const git = provider("git", false, ["uncommitted"]); + const api = createVcsApi([git]); + + await expect(api.detectManagedVcs("/not-a-repo")).resolves.toBeNull(); + await expect(api.detectVcs("/not-a-repo")).resolves.toBe(git); + }); + + test("detectManagedVcs respects forced VCS selection without throwing", async () => { + const jj = provider("jj", true, ["jj-current"], {}, "/repo"); + const git = provider("git", false, ["uncommitted"]); + const api = createVcsApi([jj, git]); + + await expect(api.detectManagedVcs("/repo", "git")).resolves.toBeNull(); + await expect(api.detectManagedVcs("/repo", "jj")).resolves.toBe(jj); + }); + + test("detectManagedVcs returns null when forced provider detection throws", async () => { + const git = { + ...provider("git", true, ["uncommitted"]), + async detect() { + throw new Error("git failed"); + }, + }; + const api = createVcsApi([git]); + + await expect(api.detectManagedVcs("/repo", "git")).resolves.toBeNull(); + }); + 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"]); diff --git a/packages/shared/vcs-core.ts b/packages/shared/vcs-core.ts index 0f81324ff..bdcef059b 100644 --- a/packages/shared/vcs-core.ts +++ b/packages/shared/vcs-core.ts @@ -64,6 +64,7 @@ export type VcsSelection = "auto" | "git" | "jj" | "p4"; export interface VcsApi { detectVcs(cwd?: string): Promise; + detectManagedVcs(cwd?: string, vcsType?: VcsSelection): Promise; getVcsContext(cwd?: string, vcsType?: VcsSelection): Promise; detectRemoteDefaultCompareTarget(cwd?: string, vcsType?: VcsSelection): Promise; prepareLocalReviewDiff(options: PrepareLocalReviewDiffOptions): Promise; @@ -230,18 +231,16 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { const providerList = [...providers]; const defaultProvider = providerList.find((provider) => provider.id === "git") ?? providerList[0]; const vcsCache = new Map(); + const managedVcsCache = new Map(); if (!defaultProvider) { throw new Error("createVcsApi requires at least one provider"); } - async function detectVcs(cwd?: string): Promise { - const key = cwd ?? process.cwd(); - const cached = vcsCache.get(key); - if (cached) return cached; - + async function collectDetectedProviders(cwd?: string): Promise> { const candidates: Array<{ provider: VcsProvider; root: string | null; order: number }> = []; - for (const provider of providerList) { + for (let index = 0; index < providerList.length; index++) { + const provider = providerList[index]; let root: string | null = null; let detected = false; try { @@ -255,11 +254,42 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { continue; } if (detected) { - candidates.push({ provider, root, order: candidates.length }); + candidates.push({ provider, root, order: index }); } } + return candidates; + } + + async function detectManagedVcs(cwd?: string, vcsType?: VcsSelection): Promise { + const key = `${vcsType ?? "auto"}:${cwd ?? process.cwd()}`; + const cached = managedVcsCache.get(key); + if (cached) return cached; + + if (vcsType && vcsType !== "auto") { + const provider = getProviderById(vcsType); + let detected = false; + try { + detected = provider ? await provider.detect(cwd) : false; + } catch { + detected = false; + } + const result = detected ? provider : null; + if (result) managedVcsCache.set(key, result); + return result; + } + + const candidates = await collectDetectedProviders(cwd); + const detected = selectNearestProvider(candidates, cwd); + if (detected) managedVcsCache.set(key, detected); + return detected; + } + + async function detectVcs(cwd?: string): Promise { + const key = cwd ?? process.cwd(); + const cached = vcsCache.get(key); + if (cached) return cached; - const detected = selectNearestProvider(candidates, cwd) ?? defaultProvider; + const detected = (await detectManagedVcs(cwd)) ?? defaultProvider; vcsCache.set(key, detected); return detected; } @@ -348,6 +378,7 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { return { detectVcs, + detectManagedVcs, async getVcsContext(cwd?: string, vcsType?: VcsSelection): Promise { return (await getContextWithProvider(cwd, vcsType)).gitContext;