From b829821dd9972921f3dcf412e90c9dd4c84e6f62 Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Mon, 29 Jun 2026 13:29:49 -0700 Subject: [PATCH] fix(engine): review explicit external checkouts --- .changeset/safe-ducks-review.md | 7 +++ .../src/__tests__/reviewer-workspace.test.ts | 50 +++++++++++++++++++ packages/engine/src/executor.ts | 11 ++-- packages/engine/src/review-checkout.ts | 40 +++++++++++++++ 4 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 .changeset/safe-ducks-review.md create mode 100644 packages/engine/src/review-checkout.ts diff --git a/.changeset/safe-ducks-review.md b/.changeset/safe-ducks-review.md new file mode 100644 index 0000000000..2075503f06 --- /dev/null +++ b/.changeset/safe-ducks-review.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Allow review steps to target a validated external checkout. +category: fix +dev: Resolves explicit review checkout metadata before spawning read-only step reviewers. diff --git a/packages/engine/src/__tests__/reviewer-workspace.test.ts b/packages/engine/src/__tests__/reviewer-workspace.test.ts index 4f5306d190..03f00163aa 100644 --- a/packages/engine/src/__tests__/reviewer-workspace.test.ts +++ b/packages/engine/src/__tests__/reviewer-workspace.test.ts @@ -14,6 +14,10 @@ the cwd of each call. Coverage: */ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { EventEmitter } from "node:events"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { ReviewResult } from "../reviewer.js"; // Narrow AI seam: only reviewStep (the agent boundary) is mocked. Everything else is the real executor. @@ -32,6 +36,14 @@ const mockedReviewStep = vi.mocked(mockedReviewStepFn); const ROOT = "/tmp/ws-root"; // NON-git workspace root — must never be a review cwd in workspace mode. const WT_A = "/tmp/ws-root/repo-a/.worktrees/fn-1"; const WT_B = "/tmp/ws-root/repo-b/.worktrees/fn-1"; +const cleanupDirs: string[] = []; + +function makeGitCheckout(): string { + const dir = mkdtempSync(join(tmpdir(), "fusion-review-checkout-")); + cleanupDirs.push(dir); + execFileSync("git", ["init", "-b", "main"], { cwd: dir, stdio: "ignore" }); + return dir; +} function makeStore(task: Task): TaskStore & EventEmitter { const emitter = new EventEmitter(); @@ -93,6 +105,10 @@ beforeEach(() => { }); afterEach(() => { vi.clearAllMocks(); + while (cleanupDirs.length > 0) { + const dir = cleanupDirs.pop(); + if (dir) rmSync(dir, { recursive: true, force: true }); + } }); describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared loop both call sites use)", () => { @@ -212,6 +228,27 @@ describe("U2 KTD3 — in-session fn_review_step (createReviewStepTool) loops per await tool.execute("call-1", { step: 1, type: "code", step_name: "Step 1", baseline: "base" }); expect(seen).toEqual([WT_A]); }); + + it("explicit external review checkout overrides the Atlas task worktree for fn_review_step", async () => { + const externalCheckout = makeGitCheckout(); + const expectedCheckout = realpathSync(externalCheckout); + const task = makeTask({ customFields: { reviewCheckoutPath: externalCheckout } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [externalCheckout]: { verdict: "APPROVE", review: "external ok", summary: "external" } }); + const tool = (executor as any).createReviewStepTool( + task.id, + WT_A, + "PROMPT", + new Map(), + { current: null }, + new Map(), + task, + undefined, + ); + await tool.execute("call-1", { step: 1, type: "code", step_name: "Step 1", baseline: "base" }); + expect(seen).toEqual([expectedCheckout]); + }); }); describe("U2 KTD3 — step-inversion review seam (executor.ts:5668) loops per sub-repo", () => { @@ -244,4 +281,17 @@ describe("U2 KTD3 — step-inversion review seam (executor.ts:5668) loops per su await seams.stepReview!(task as any, context, { type: "code", advisory: true } as any); expect(seen).toEqual([WT_A]); }); + + it("explicit external review checkout overrides the active graph worktree", async () => { + const externalCheckout = makeGitCheckout(); + const expectedCheckout = realpathSync(externalCheckout); + const task = makeTask({ worktree: WT_A, customFields: { reviewCheckoutPath: externalCheckout } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [externalCheckout]: { verdict: "APPROVE", review: "external", summary: "external" } }); + const seams = executor.createAuthoritativeWorkflowSeams({ autoMerge: false } as any); + const context = { [FOREACH_ACTIVE_CONTEXT_KEY]: { stepIndex: 1, worktreePath: WT_A, baselineSha: "base" } } as any; + await seams.stepReview!(task as any, context, { type: "code", advisory: true } as any); + expect(seen).toEqual([expectedCheckout]); + }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 5c636bcd01..c08b821f7f 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -24,6 +24,7 @@ import { import { WorkflowGraphTaskRunner, type WorkflowGraphTaskRunResult } from "./workflow-graph-task-runner.js"; import { ensureWorkflowCompletionSummary } from "./workflow-completion-summary.js"; import { createCodeNodeRunner } from "./code-node-runner.js"; +import { resolveReviewCheckoutCwd } from "./review-checkout.js"; import { getActiveNotificationService } from "./notifier.js"; import type { ParseStepsHandlerDeps, CodeNodeRunner } from "./workflow-node-handlers.js"; import type { WorkflowBranchPersistence, WorkflowBranchRunState } from "./workflow-graph-branches.js"; @@ -6135,6 +6136,7 @@ export class TaskExecutor { const detail = await this.store.getTask(seamTask.id); // Worktree isolation (KTD-11): review the instance's OWN worktree when set. const worktreePath = active.worktreePath || detail.worktree || this.rootDir; + const reviewCwd = resolveReviewCheckoutCwd(detail, worktreePath); const stepName = detail.steps[stepIndex]?.name ?? `Step ${stepIndex}`; const promptContent = detail.prompt ?? ""; // Merge per-task effective workflow settings (U3, KTD-3) so the validator @@ -6190,9 +6192,9 @@ export class TaskExecutor { return sem ? sem.runNested(invoke) : invoke(); }; const invokeReviewer = () => - this.workspaceConfig + this.workspaceConfig && reviewCwd === worktreePath ? this.reviewWorkspacePerRepo(detail, (cwd) => runForCwd(cwd)) - : runForCwd(worktreePath); + : runForCwd(reviewCwd); let review: { verdict: ReviewVerdict; review: string; summary: string }; try { @@ -12634,6 +12636,7 @@ export class TaskExecutor { const latestDetailForReview = await store.getTask(taskId); const userComments = selectUserCommentsForAgentContext(latestDetailForReview); const settings = await mergeEffectiveSettings(store, latestDetailForReview, await store.getSettings()); + const reviewCwd = resolveReviewCheckoutCwd(latestDetailForReview, worktreePath); // Run the reviewer via semaphore.runNested so its slot accounting // is honest: activeCount transiently bumps to reflect the second // agent session, but the reviewer doesn't enter the wait queue @@ -12693,9 +12696,9 @@ export class TaskExecutor { const invoke = () => invokeReviewerForCwd(cwd); return sem ? sem.runNested(invoke) : invoke(); }; - const result = this.workspaceConfig + const result = this.workspaceConfig && reviewCwd === worktreePath ? await this.reviewWorkspacePerRepo(currentTask, (cwd) => runForCwd(cwd)) - : await runForCwd(worktreePath); + : await runForCwd(reviewCwd); await store.logEntry( taskId, diff --git a/packages/engine/src/review-checkout.ts b/packages/engine/src/review-checkout.ts new file mode 100644 index 0000000000..172cc42ceb --- /dev/null +++ b/packages/engine/src/review-checkout.ts @@ -0,0 +1,40 @@ +import { execFileSync } from "node:child_process"; +import { existsSync, lstatSync, realpathSync } from "node:fs"; +import { isAbsolute } from "node:path"; + +function readMetadataPath(value: unknown): string | undefined { + if (!value || typeof value !== "object") return undefined; + const record = value as Record; + const direct = record.reviewCheckoutPath ?? record.externalReviewCheckoutPath; + if (typeof direct === "string" && direct.trim()) return direct.trim(); + const nested = record.reviewCheckout; + if (nested && typeof nested === "object") { + const path = (nested as Record).path; + if (typeof path === "string" && path.trim()) return path.trim(); + } + return undefined; +} + +export function getTaskReviewCheckoutPath(task: unknown): string | undefined { + if (!task || typeof task !== "object") return undefined; + const record = task as Record; + return readMetadataPath(record.customFields) ?? readMetadataPath(record.branchContext) ?? readMetadataPath(record); +} + +export function resolveReviewCheckoutCwd(task: unknown, fallbackCwd: string): string { + const candidate = getTaskReviewCheckoutPath(task); + if (!candidate || !isAbsolute(candidate)) return fallbackCwd; + try { + if (!existsSync(candidate) || !lstatSync(candidate).isDirectory()) return fallbackCwd; + const realCandidate = realpathSync(candidate); + const topLevel = execFileSync("git", ["rev-parse", "--show-toplevel"], { + cwd: realCandidate, + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + if (!topLevel) return fallbackCwd; + return realpathSync(topLevel); + } catch { + return fallbackCwd; + } +}