diff --git a/apps/server/src/review/ReviewService.test.ts b/apps/server/src/review/ReviewService.test.ts index 01a4692264e0..5f560032f695 100644 --- a/apps/server/src/review/ReviewService.test.ts +++ b/apps/server/src/review/ReviewService.test.ts @@ -3,131 +3,94 @@ import { assert, describe, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; -import * as PlatformError from "effect/PlatformError"; +import * as Path from "effect/Path"; import { ServerConfig } from "../config.ts"; import * as GitVcsDriver from "../vcs/GitVcsDriver.ts"; import * as VcsDriverRegistry from "../vcs/VcsDriverRegistry.ts"; +import * as VcsProcess from "../vcs/VcsProcess.ts"; import * as ReviewService from "./ReviewService.ts"; -function makeLayer(input: { - readonly workspaceRoot: string; - readonly baseDir: string; - readonly detectCalls?: Array<{ readonly cwd: string }>; -}) { - return ReviewService.layer.pipe( - Layer.provide( - Layer.mock(VcsDriverRegistry.VcsDriverRegistry)({ - get: () => Effect.die("unexpected VCS registry get"), - resolve: () => Effect.die("unexpected VCS registry resolve"), - detect: (request) => - Effect.sync(() => { - input.detectCalls?.push({ cwd: request.cwd }); - return null; - }), - }), - ), - Layer.provide(Layer.mock(GitVcsDriver.GitVcsDriver)({})), - Layer.provide(ServerConfig.layerTest(input.workspaceRoot, input.baseDir)), - Layer.provideMerge(NodeServices.layer), - ); -} +const services = ReviewService.layer.pipe( + Layer.provide(VcsDriverRegistry.layer), + Layer.provide(VcsProcess.layer), + Layer.provideMerge(GitVcsDriver.layer), +); describe("ReviewService", () => { - it.effect("rejects diff preview cwd outside the configured workspace roots", () => + it.effect("switches previews and file contents between projects and external worktrees", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; - const workspaceRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-workspace-" }); - const outsideRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-outside-" }); - const baseDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-base-" }); - const detectCalls: Array<{ readonly cwd: string }> = []; - - const error = yield* Effect.gen(function* () { + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-switch-" }); + const launchDir = path.join(root, "server"); + const projectA = path.join(root, "project-a"); + const projectB = path.join(root, "project-b"); + const worktree = path.join(root, "external-worktree"); + yield* fs.makeDirectory(launchDir); + yield* Effect.gen(function* () { + const git = yield* GitVcsDriver.GitVcsDriver; const review = yield* ReviewService.ReviewService; - return yield* review.getDiffPreview({ cwd: outsideRoot }).pipe(Effect.flip); - }).pipe(Effect.provide(makeLayer({ workspaceRoot, baseDir, detectCalls }))); - - assert.strictEqual(error._tag, "VcsRepositoryDetectionError"); - assert.strictEqual(error.operation, "ReviewService.getDiffPreview"); - assert.match( - "detail" in error ? error.detail : "", - /must stay within the configured workspace root/, - ); - assert.deepStrictEqual(detectCalls, []); - }).pipe(Effect.provide(NodeServices.layer)), - ); - - it.effect("attributes file-content workspace violations to the file-content operation", () => - Effect.gen(function* () { - const fs = yield* FileSystem.FileSystem; - const workspaceRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-workspace-" }); - const outsideRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-outside-" }); - const baseDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-base-" }); - const detectCalls: Array<{ readonly cwd: string }> = []; + const runGit = (cwd: string, args: ReadonlyArray) => + git.execute({ operation: "ReviewService.test.git", cwd, args }); + for (const cwd of [projectA, projectB]) { + yield* fs.makeDirectory(cwd); + yield* runGit(cwd, ["init"]); + yield* runGit(cwd, ["config", "user.email", "test@example.com"]); + yield* runGit(cwd, ["config", "user.name", "Test"]); + yield* fs.writeFileString(path.join(cwd, "file.txt"), "original\n"); + yield* runGit(cwd, ["add", "."]); + yield* runGit(cwd, ["commit", "-m", "initial"]); + } + yield* runGit(projectA, ["worktree", "add", "-b", "external", worktree]); + const changes = new Map([ + [projectA, "project A\n"], + [projectB, "project B\nsecond line\n"], + [worktree, "external worktree\nsecond line\nthird line\n"], + ]); + for (const [cwd, content] of changes) { + yield* fs.writeFileString(path.join(cwd, "file.txt"), content); + } + for (const cwd of [projectA, projectB, worktree, projectA]) { + const content = changes.get(cwd)!; + const preview = yield* review.getDiffPreview({ cwd }); + assert.strictEqual(preview.cwd, cwd); + const dirty = preview.sources.find((source) => source.kind === "working-tree")!; + assert.include(dirty.diff, `+${content.split("\n")[0]}`); + const contents = yield* review.getDiffFileContents({ + cwd, + sourceKind: "working-tree", + changeType: "change", + baseRef: "HEAD", + headRef: null, + oldPath: "file.txt", + newPath: "file.txt", + }); + assert.strictEqual(contents.oldContents, "original\n"); + assert.strictEqual(contents.newContents, content); + } + const empty = yield* review.getDiffPreview({ cwd: launchDir }); + assert.strictEqual(empty.cwd, launchDir); + assert.deepStrictEqual(empty.sources, []); - const error = yield* Effect.gen(function* () { - const review = yield* ReviewService.ReviewService; - return yield* review + const escaped = yield* review .getDiffFileContents({ - cwd: outsideRoot, + cwd: projectA, sourceKind: "working-tree", - changeType: "change", + changeType: "new", baseRef: "HEAD", headRef: null, - oldPath: "file.ts", - newPath: "file.ts", + oldPath: "../project-b/file.txt", + newPath: "../project-b/file.txt", }) .pipe(Effect.flip); - }).pipe(Effect.provide(makeLayer({ workspaceRoot, baseDir, detectCalls }))); - - assert.strictEqual(error._tag, "VcsRepositoryDetectionError"); - assert.strictEqual(error.operation, "ReviewService.getDiffFileContents"); - assert.match( - "detail" in error ? error.detail : "", - /must stay within the configured workspace root/, + assert.strictEqual(escaped._tag, "GitCommandError"); + if (escaped._tag === "GitCommandError") assert.include(escaped.detail, "outside"); + }).pipe( + Effect.provide( + services.pipe(Layer.provide(ServerConfig.layerTest(launchDir, path.join(root, "state")))), + ), ); - assert.deepStrictEqual(detectCalls, []); - }).pipe(Effect.provide(NodeServices.layer)), - ); - - it.effect("allows diff preview cwd inside the configured workspace root", () => - Effect.gen(function* () { - const fs = yield* FileSystem.FileSystem; - const workspaceRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-workspace-" }); - const baseDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-base-" }); - const detectCalls: Array<{ readonly cwd: string }> = []; - - const result = yield* Effect.gen(function* () { - const review = yield* ReviewService.ReviewService; - return yield* review.getDiffPreview({ cwd: workspaceRoot }); - }).pipe(Effect.provide(makeLayer({ workspaceRoot, baseDir, detectCalls }))); - - assert.strictEqual(result.cwd, workspaceRoot); - assert.deepStrictEqual(result.sources, []); - assert.deepStrictEqual(detectCalls, [{ cwd: workspaceRoot }]); - }).pipe(Effect.provide(NodeServices.layer)), - ); - - it.effect("preserves unexpected path-resolution failures", () => - Effect.gen(function* () { - const fs = yield* FileSystem.FileSystem; - const workspaceRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-workspace-" }); - const baseDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-base-" }); - const invalidCwd = `${workspaceRoot}\0invalid`; - const detectCalls: Array<{ readonly cwd: string }> = []; - - const error = yield* Effect.gen(function* () { - const review = yield* ReviewService.ReviewService; - return yield* review.getDiffPreview({ cwd: invalidCwd }).pipe(Effect.flip); - }).pipe(Effect.provide(makeLayer({ workspaceRoot, baseDir, detectCalls }))); - - assert.strictEqual(error._tag, "VcsRepositoryDetectionError"); - if (error._tag !== "VcsRepositoryDetectionError") return; - assert.strictEqual(error.operation, "ReviewService.assertWorkspaceBoundCwd.canonicalizePath"); - assert.strictEqual(error.cwd, invalidCwd); - assert.match(error.detail, /Failed to resolve a path/); - assert.instanceOf(error.cause, PlatformError.PlatformError); - assert.deepStrictEqual(detectCalls, []); }).pipe(Effect.provide(NodeServices.layer)), ); }); diff --git a/apps/server/src/review/ReviewService.ts b/apps/server/src/review/ReviewService.ts index 275dcb416610..3a5fc5e89894 100644 --- a/apps/server/src/review/ReviewService.ts +++ b/apps/server/src/review/ReviewService.ts @@ -1,12 +1,9 @@ import * as Context from "effect/Context"; import * as DateTime from "effect/DateTime"; import * as Effect from "effect/Effect"; -import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; -import * as Path from "effect/Path"; import { - VcsRepositoryDetectionError, VcsUnsupportedOperationError, type ReviewDiffFileContentsInput, type ReviewDiffFileContentsResult, @@ -15,7 +12,6 @@ import { type ReviewDiffPreviewResult, } from "@t3tools/contracts"; -import * as ServerConfig from "../config.ts"; import * as GitVcsDriver from "../vcs/GitVcsDriver.ts"; import * as VcsDriverRegistry from "../vcs/VcsDriverRegistry.ts"; @@ -32,65 +28,14 @@ export class ReviewService extends Context.Service< >()("t3/review/ReviewService") {} export const make = Effect.gen(function* () { - const config = yield* ServerConfig.ServerConfig; - const fileSystem = yield* FileSystem.FileSystem; - const path = yield* Path.Path; const vcsRegistry = yield* VcsDriverRegistry.VcsDriverRegistry; const git = yield* GitVcsDriver.GitVcsDriver; - const canonicalizePath = (value: string) => { - const resolvedPath = path.resolve(value); - return fileSystem.realPath(resolvedPath).pipe( - Effect.catchTags({ - PlatformError: (cause) => - cause.reason._tag === "NotFound" - ? Effect.succeed(resolvedPath) - : Effect.fail( - new VcsRepositoryDetectionError({ - operation: "ReviewService.assertWorkspaceBoundCwd.canonicalizePath", - cwd: resolvedPath, - detail: "Failed to resolve a path while validating the review workspace.", - cause, - }), - ), - }), - ); - }; - - const isWithinRoot = (candidate: string, root: string) => { - const relative = path.relative(root, candidate); - return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); - }; - - const assertWorkspaceBoundCwd = Effect.fn("ReviewService.assertWorkspaceBoundCwd")(function* ( - operation: "ReviewService.getDiffPreview" | "ReviewService.getDiffFileContents", - cwd: string, - ) { - const [candidate, workspaceRoot, worktreesRoot] = yield* Effect.all([ - canonicalizePath(cwd), - canonicalizePath(config.cwd), - canonicalizePath(config.worktreesDir), - ]); - - if (isWithinRoot(candidate, workspaceRoot) || isWithinRoot(candidate, worktreesRoot)) { - return; - } - - return yield* new VcsRepositoryDetectionError({ - operation, - cwd, - detail: - operation === "ReviewService.getDiffPreview" - ? "Review diff preview cwd must stay within the configured workspace root." - : "Review diff file contents cwd must stay within the configured workspace root.", - }); - }); - + // Review uses the requested repository, like the other VCS operations. The + // server's launch directory is only a default workspace, not a filesystem boundary. const getDiffPreview: ReviewService["Service"]["getDiffPreview"] = Effect.fn( "ReviewService.getDiffPreview", )(function* (input) { - yield* assertWorkspaceBoundCwd("ReviewService.getDiffPreview", input.cwd); - const handle = yield* vcsRegistry.detect({ cwd: input.cwd, requestedKind: "auto" }); if (!handle) { return { @@ -118,8 +63,6 @@ export const make = Effect.gen(function* () { const getDiffFileContents: ReviewService["Service"]["getDiffFileContents"] = Effect.fn( "ReviewService.getDiffFileContents", )(function* (input) { - yield* assertWorkspaceBoundCwd("ReviewService.getDiffFileContents", input.cwd); - const handle = yield* vcsRegistry.detect({ cwd: input.cwd, requestedKind: "auto" }); if (handle?.kind !== "git") { return yield* new VcsUnsupportedOperationError({ diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index d764b9c6a9e2..6d04607a1be3 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -256,7 +256,7 @@ export default function DiffPanel({ }, { enabled: isGitRepo && selectedTurn !== undefined }, ); - const primaryBranchDiffPreview = useEnvironmentQuery( + const branchDiffPreview = useEnvironmentQuery( selectedTurnId === null && activeThread && activeCwd ? reviewEnvironment.diffPreview({ environmentId: activeThread.environmentId, @@ -268,26 +268,6 @@ export default function DiffPanel({ }) : null, ); - const shouldRetryBranchDiffAtEnvironmentCwd = - selectedTurnId === null && - primaryBranchDiffPreview.error?.includes("configured workspace root") === true && - serverConfig?.cwd !== undefined && - serverConfig.cwd !== activeCwd; - const fallbackBranchDiffPreview = useEnvironmentQuery( - shouldRetryBranchDiffAtEnvironmentCwd && activeThread && serverConfig - ? reviewEnvironment.diffPreview({ - environmentId: activeThread.environmentId, - input: { - cwd: serverConfig.cwd, - ...(selectedBaseRef ? { baseRef: selectedBaseRef } : {}), - ignoreWhitespace: diffIgnoreWhitespace, - }, - }) - : null, - ); - const branchDiffPreview = shouldRetryBranchDiffAtEnvironmentCwd - ? fallbackBranchDiffPreview - : primaryBranchDiffPreview; const refreshBranchDiffPreview = branchDiffPreview.refresh; const canRefreshGitDiff = isGitRepo && selectedTurnId === null && activeThread != null && activeCwd != null;