From f76d2f3609bd8a76d84cd4a3eadc65fa5d35baee Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Wed, 1 Jul 2026 16:26:54 -0700 Subject: [PATCH 1/4] fix(engine): route review via explicit external checkout metadata --- .../src/__tests__/review-checkout.test.ts | 266 ++++++++++++++++++ .../src/__tests__/reviewer-workspace.test.ts | 136 ++++++++- packages/engine/src/executor.ts | 24 ++ packages/engine/src/review-checkout.ts | 16 +- 4 files changed, 434 insertions(+), 8 deletions(-) create mode 100644 packages/engine/src/__tests__/review-checkout.test.ts diff --git a/packages/engine/src/__tests__/review-checkout.test.ts b/packages/engine/src/__tests__/review-checkout.test.ts new file mode 100644 index 000000000..a7cfa7a67 --- /dev/null +++ b/packages/engine/src/__tests__/review-checkout.test.ts @@ -0,0 +1,266 @@ +/* +Explicit external review checkout metadata contract tests. + +Resolves review checkout cwd from task metadata with fail-closed defaults: +- absent/blank/relative/non-git metadata → fallback (task worktree) +- valid explicit absolute git checkout → resolved realpath +- sourceMetadata.externalReviewCheckout is the canonical external field +- invalid higher-priority metadata → fallback, not silent lower-priority widening +*/ +import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, writeFileSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { resolveReviewCheckoutCwd, getTaskReviewCheckoutPath } from "../review-checkout.js"; + +const FALLBACK = "/some/fallback/worktree"; +const cleanupDirs: string[] = []; + +function makeGitCheckout(): string { + const dir = mkdtempSync(join(tmpdir(), "review-git-checkout-")); + cleanupDirs.push(dir); + execFileSync("git", ["init"], { cwd: dir, stdio: "ignore" }); + return dir; +} + +function makeNonGitDir(): string { + const dir = mkdtempSync(join(tmpdir(), "review-nongit-")); + cleanupDirs.push(dir); + return dir; +} + +function makeRegularFile(): string { + const dir = mkdtempSync(join(tmpdir(), "review-file-")); + cleanupDirs.push(dir); + const file = join(dir, "file.txt"); + writeFileSync(file, "not a directory"); + return file; +} + +beforeEach(() => { + cleanupDirs.length = 0; +}); +afterEach(() => { + while (cleanupDirs.length > 0) { + const dir = cleanupDirs.pop(); + if (dir) rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("getTaskReviewCheckoutPath — extracts metadata path candidates", () => { + it("returns undefined for null/undefined task", () => { + expect(getTaskReviewCheckoutPath(null)).toBeUndefined(); + expect(getTaskReviewCheckoutPath(undefined)).toBeUndefined(); + }); + + it("returns undefined when no metadata fields are present", () => { + expect(getTaskReviewCheckoutPath({})).toBeUndefined(); + expect(getTaskReviewCheckoutPath({ id: "TASK-1", title: "T" })).toBeUndefined(); + }); + + it("returns undefined for blank/whitespace-only reviewCheckoutPath", () => { + expect(getTaskReviewCheckoutPath({ customFields: { reviewCheckoutPath: "" } })).toBeUndefined(); + expect(getTaskReviewCheckoutPath({ customFields: { reviewCheckoutPath: " " } })).toBeUndefined(); + expect(getTaskReviewCheckoutPath({ customFields: { externalReviewCheckoutPath: "" } })).toBeUndefined(); + }); + + it("returns undefined for non-string reviewCheckoutPath", () => { + expect(getTaskReviewCheckoutPath({ customFields: { reviewCheckoutPath: 42 } })).toBeUndefined(); + expect(getTaskReviewCheckoutPath({ customFields: { reviewCheckoutPath: true } })).toBeUndefined(); + expect(getTaskReviewCheckoutPath({ customFields: { reviewCheckoutPath: {} } })).toBeUndefined(); + }); + + it("reads reviewCheckoutPath from customFields", () => { + const task = { customFields: { reviewCheckoutPath: "/custom/path" } }; + expect(getTaskReviewCheckoutPath(task)).toBe("/custom/path"); + }); + + it("reads externalReviewCheckoutPath from customFields", () => { + const task = { customFields: { externalReviewCheckoutPath: "/external/path" } }; + expect(getTaskReviewCheckoutPath(task)).toBe("/external/path"); + }); + + it("reads nested reviewCheckout.path from customFields", () => { + const task = { customFields: { reviewCheckout: { path: "/nested/path" } } }; + expect(getTaskReviewCheckoutPath(task)).toBe("/nested/path"); + }); + + it("reads sourceMetadata.externalReviewCheckout as a candidate", () => { + const task = { sourceMetadata: { externalReviewCheckout: "/meta/checkout" } }; + expect(getTaskReviewCheckoutPath(task)).toBe("/meta/checkout"); + }); + + it("trims whitespace from metadata values", () => { + const task = { customFields: { reviewCheckoutPath: " /trimmed/path " } }; + expect(getTaskReviewCheckoutPath(task)).toBe("/trimmed/path"); + }); + + it("customFields takes priority over branchContext, sourceMetadata, and root", () => { + const task = { + customFields: { reviewCheckoutPath: "/custom" }, + branchContext: { reviewCheckoutPath: "/branch" }, + sourceMetadata: { externalReviewCheckout: "/meta" }, + reviewCheckoutPath: "/root", + }; + expect(getTaskReviewCheckoutPath(task)).toBe("/custom"); + }); + + it("branchContext takes priority over sourceMetadata and root-level fields", () => { + const task = { + branchContext: { reviewCheckoutPath: "/branch" }, + sourceMetadata: { externalReviewCheckout: "/meta" }, + reviewCheckoutPath: "/root", + }; + expect(getTaskReviewCheckoutPath(task)).toBe("/branch"); + }); + + it("sourceMetadata takes priority over root-level fields", () => { + const task = { + sourceMetadata: { externalReviewCheckout: "/meta" }, + reviewCheckoutPath: "/root", + }; + expect(getTaskReviewCheckoutPath(task)).toBe("/meta"); + }); +}); + +describe("resolveReviewCheckoutCwd — fail-closed defaults", () => { + it("returns fallback when task has no metadata", () => { + expect(resolveReviewCheckoutCwd({}, FALLBACK)).toBe(FALLBACK); + }); + + it("returns fallback for null/undefined task", () => { + expect(resolveReviewCheckoutCwd(null, FALLBACK)).toBe(FALLBACK); + expect(resolveReviewCheckoutCwd(undefined, FALLBACK)).toBe(FALLBACK); + }); + + it("returns fallback when reviewCheckoutPath is blank", () => { + const task = { customFields: { reviewCheckoutPath: "" } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("returns fallback for relative path (not absolute)", () => { + const task = { customFields: { reviewCheckoutPath: "relative/path" } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("returns fallback for non-existent path", () => { + const task = { customFields: { reviewCheckoutPath: "/nonexistent/path/that/does/not/exist" } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("returns fallback for path that is a regular file, not a directory", () => { + const file = makeRegularFile(); + const task = { customFields: { reviewCheckoutPath: file } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("returns fallback for non-git directory", () => { + const dir = makeNonGitDir(); + const task = { customFields: { reviewCheckoutPath: dir } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("returns resolved realpath for valid git checkout", () => { + const checkout = makeGitCheckout(); + const expected = realpathSync(checkout); + const task = { customFields: { reviewCheckoutPath: checkout } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(expected); + }); + + it("resolves sourceMetadata.externalReviewCheckout for valid git checkout", () => { + const checkout = makeGitCheckout(); + const expected = realpathSync(checkout); + const task = { sourceMetadata: { externalReviewCheckout: checkout } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(expected); + }); + + it("does not infer external checkout from prompt text or task description", () => { + const task = { + description: "Please review changes in /tmp/external-runtime", + prompt: "Look at /tmp/some-checkout", + }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("conflicting metadata between customFields and sourceMetadata: customFields wins when valid", () => { + const checkoutA = makeGitCheckout(); + const checkoutB = makeGitCheckout(); + const expectedA = realpathSync(checkoutA); + const task = { + customFields: { reviewCheckoutPath: checkoutA }, + sourceMetadata: { externalReviewCheckout: checkoutB }, + }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(expectedA); + }); + + it("valid customFields + invalid sourceMetadata: uses customFields", () => { + const checkout = makeGitCheckout(); + const expected = realpathSync(checkout); + const task = { + customFields: { reviewCheckoutPath: checkout }, + sourceMetadata: { externalReviewCheckout: "/nonexistent" }, + }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(expected); + }); + + // Priority-based resolution means the first candidate from the highest-priority + // source (customFields > branchContext > sourceMetadata > root) is the only + // candidate validated. When that candidate is invalid, the resolver fails + // closed to the fallback instead of trying lower-priority sources. That keeps a + // stale high-priority value from being bypassed by a coincidentally valid lower + // priority path. + it("invalid customFields → falls back even when sourceMetadata has a valid checkout (fail-closed priority)", () => { + const checkout = makeGitCheckout(); + const task = { + customFields: { reviewCheckoutPath: "/nonexistent" }, + sourceMetadata: { externalReviewCheckout: checkout }, + }; + // customFields wins priority; /nonexistent fails validation → fallback + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("workspace-mode task without explicit metadata: returns fallback (task worktree)", () => { + const task = { + workspaceWorktrees: { + "repo-a": { worktreePath: "/tmp/ws/repo-a/.worktrees/task-1" }, + }, + }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); +}); + +describe("resolveReviewCheckoutCwd — does NOT fabricate approval or widen scope", () => { + it("metadata pointing to a parent directory of the fallback: still validates as git dir", () => { + const parent = makeGitCheckout(); + const task = { customFields: { reviewCheckoutPath: parent } }; + const result = resolveReviewCheckoutCwd(task, FALLBACK); + expect(result).toBe(realpathSync(parent)); + expect(result).not.toBe(FALLBACK); + }); + + it("empty sourceMetadata object: returns fallback", () => { + const task = { sourceMetadata: {} }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("sourceMetadata with unrelated fields only: returns fallback", () => { + const task = { sourceMetadata: { fileScope: ["src/**"], contentFingerprint: "abc" } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("blank sourceMetadata.externalReviewCheckout: returns fallback", () => { + const task = { sourceMetadata: { externalReviewCheckout: "" } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("non-string sourceMetadata.externalReviewCheckout: returns fallback", () => { + const task = { sourceMetadata: { externalReviewCheckout: 42 } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); + + it("relative sourceMetadata.externalReviewCheckout: returns fallback", () => { + const task = { sourceMetadata: { externalReviewCheckout: "relative/path" } }; + expect(resolveReviewCheckoutCwd(task, FALLBACK)).toBe(FALLBACK); + }); +}); diff --git a/packages/engine/src/__tests__/reviewer-workspace.test.ts b/packages/engine/src/__tests__/reviewer-workspace.test.ts index 9b68473ba..c00b8adcb 100644 --- a/packages/engine/src/__tests__/reviewer-workspace.test.ts +++ b/packages/engine/src/__tests__/reviewer-workspace.test.ts @@ -229,7 +229,7 @@ describe("U2 KTD3 — in-session fn_review_step (createReviewStepTool) loops per expect(seen).toEqual([WT_A]); }); - it("explicit external review checkout overrides the Atlas task worktree for fn_review_step", async () => { + it("explicit external review checkout overrides the task worktree for fn_review_step", async () => { const externalCheckout = makeGitCheckout(); const expectedCheckout = realpathSync(externalCheckout); const task = makeTask({ customFields: { reviewCheckoutPath: externalCheckout } } as any); @@ -334,3 +334,137 @@ describe("U2 KTD3 — step-inversion review seam (executor.ts:5668) loops per su expect(seen).toEqual([expectedCheckout]); }); }); + +describe("sourceMetadata.externalReviewCheckout for fn_review_step", () => { + it("sourceMetadata.externalReviewCheckout overrides the task worktree for fn_review_step", async () => { + const externalCheckout = makeGitCheckout(); + const expectedCheckout = realpathSync(externalCheckout); + const task = makeTask({ sourceMetadata: { externalReviewCheckout: externalCheckout } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [expectedCheckout]: { 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]); + }); + + it("no metadata → fn_review_step reviews the task worktree (default fallback)", async () => { + const task = makeTask(); // no customFields, no sourceMetadata + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "ok", summary: "ok" } }); + 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([WT_A]); + }); + + it("invalid external metadata (non-existent path) → falls back to task worktree", async () => { + const task = makeTask({ sourceMetadata: { externalReviewCheckout: "/nonexistent/path/918" } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "ok", summary: "ok" } }); + 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([WT_A]); + }); + + it("invalid external metadata (non-git directory) → falls back to task worktree", async () => { + const nonGitDir = mkdtempSync(join(tmpdir(), "review-nongit-integ-")); + cleanupDirs.push(nonGitDir); + const task = makeTask({ sourceMetadata: { externalReviewCheckout: nonGitDir } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "ok", summary: "ok" } }); + 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([WT_A]); + }); +}); + +describe("sourceMetadata.externalReviewCheckout for workflow stepReview", () => { + it("sourceMetadata.externalReviewCheckout overrides the active graph worktree for stepReview", async () => { + const externalCheckout = makeGitCheckout(); + const expectedCheckout = realpathSync(externalCheckout); + const task = makeTask({ worktree: WT_A, sourceMetadata: { externalReviewCheckout: externalCheckout } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [expectedCheckout]: { 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]); + }); + + it("no metadata → stepReview reviews the task worktree (default fallback)", async () => { + const task = makeTask({ worktree: WT_A }); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "a", summary: "a" } }); + 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([WT_A]); + }); + + it("invalid external metadata (non-existent path) → stepReview falls back to task worktree", async () => { + const task = makeTask({ worktree: WT_A, sourceMetadata: { externalReviewCheckout: "/nonexistent/path/918b" } } as any); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "a", summary: "a" } }); + 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([WT_A]); + }); + + it("workspace-mode task without explicit metadata: stepReview still reviews per sub-repo", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES, worktree: ROOT }); + const store = makeStore(task); + const executor = workspaceExecutor(store); + const seen = scriptReviewByCwd({ + [WT_A]: { verdict: "APPROVE", review: "a", summary: "a" }, + [WT_B]: { verdict: "APPROVE", review: "b", summary: "b" }, + }); + const seams = executor.createAuthoritativeWorkflowSeams({ autoMerge: false } as any); + const context = { [FOREACH_ACTIVE_CONTEXT_KEY]: { stepIndex: 1, worktreePath: ROOT, baselineSha: "base" } } as any; + await seams.stepReview!(task as any, context, { type: "code", advisory: true } as any); + expect(seen).toEqual([WT_A, WT_B]); + expect(seen).not.toContain(ROOT); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index c85547eba..68d0b5058 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -6363,6 +6363,18 @@ export class TaskExecutor { // 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); + // Make the actual review target visible: explicit + // sourceMetadata.externalReviewCheckout routes review to an external + // checkout; without valid metadata, review defaults to the task worktree. + if (reviewCwd !== worktreePath) { + reviewerLog.log(`${seamTask.id}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); + } else { + const sm = detail.sourceMetadata as Record | undefined; + const hasExternalMeta = sm && typeof sm.externalReviewCheckout === "string" && sm.externalReviewCheckout.trim(); + if (hasExternalMeta) { + reviewerLog.warn(`${seamTask.id}: external review checkout metadata present (${sm!.externalReviewCheckout}) but invalid — reviewing task worktree ${worktreePath}`); + } + } const stepName = detail.steps[stepIndex]?.name ?? `Step ${stepIndex}`; const promptContent = detail.prompt ?? ""; const userComments = selectUserCommentsForAgentContext(detail, { limit: null }); @@ -12979,6 +12991,18 @@ export class TaskExecutor { const userComments = selectUserCommentsForAgentContext(latestDetailForReview, { limit: null }); const settings = await mergeEffectiveSettings(store, latestDetailForReview, await store.getSettings()); const reviewCwd = resolveReviewCheckoutCwd(latestDetailForReview, worktreePath); + // Make the actual review target visible: explicit + // sourceMetadata.externalReviewCheckout routes review to an external + // checkout; without valid metadata, review defaults to the task worktree. + if (reviewCwd !== worktreePath) { + reviewerLog.log(`${taskId}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); + } else { + const sm = latestDetailForReview.sourceMetadata as Record | undefined; + const hasExternalMeta = sm && typeof sm.externalReviewCheckout === "string" && sm.externalReviewCheckout.trim(); + if (hasExternalMeta) { + reviewerLog.warn(`${taskId}: external review checkout metadata present (${sm!.externalReviewCheckout}) but invalid — reviewing task worktree ${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 diff --git a/packages/engine/src/review-checkout.ts b/packages/engine/src/review-checkout.ts index 344950545..83cffd1b6 100644 --- a/packages/engine/src/review-checkout.ts +++ b/packages/engine/src/review-checkout.ts @@ -5,12 +5,14 @@ import { isAbsolute } from "node:path"; function readMetadataPath(value: unknown): string | undefined { if (!value || typeof value !== "object") return undefined; const record = value as Record; - /* - FNXC:ReviewCheckout 2026-06-29-14:05: - Explicit external review checkout metadata must survive legacy empty reviewCheckoutPath fields and symlinked checkout directories. - Treat blank/non-string direct metadata as absent before falling back, then verify the resolved target directory so external worktrees mounted via symlinks can still be reviewed. - */ - for (const direct of [record.reviewCheckoutPath, record.externalReviewCheckoutPath]) { + // External review routing must be explicit metadata, not inferred from prompt + // text or task descriptions. The resolver only accepts known metadata fields, + // treats blank/non-string values as absent, and later validates that the chosen + // path is an absolute git checkout. Source priority is fixed + // (customFields > branchContext > sourceMetadata > root); an invalid higher + // priority candidate fails closed to the task worktree rather than silently + // falling through to a lower-priority path. + for (const direct of [record.reviewCheckoutPath, record.externalReviewCheckoutPath, record.externalReviewCheckout]) { if (typeof direct === "string" && direct.trim()) return direct.trim(); } const nested = record.reviewCheckout; @@ -24,7 +26,7 @@ function readMetadataPath(value: unknown): string | 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); + return readMetadataPath(record.customFields) ?? readMetadataPath(record.branchContext) ?? readMetadataPath(record.sourceMetadata) ?? readMetadataPath(record); } export function resolveReviewCheckoutCwd(task: unknown, fallbackCwd: string): string { From b58d9b5d2fcfff71bfe0dd72075f7e2c73c7114d Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Wed, 1 Jul 2026 16:27:21 -0700 Subject: [PATCH 2/4] chore: add review checkout routing changeset --- .changeset/review-checkout-routing.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/review-checkout-routing.md diff --git a/.changeset/review-checkout-routing.md b/.changeset/review-checkout-routing.md new file mode 100644 index 000000000..4b194dadd --- /dev/null +++ b/.changeset/review-checkout-routing.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Enforce explicit external checkout metadata for review routing. +category: fix +dev: Reviews now use sourceMetadata.externalReviewCheckout only when it points at a valid git checkout, otherwise they fail closed to the task worktree and log the selected review target. From c46d11f0a3164487dbb748e9c534d436f7c00d6c Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Wed, 1 Jul 2026 16:34:02 -0700 Subject: [PATCH 3/4] test(cli): keep extension engine mocks current --- .../__tests__/extension-experiment-finalize.test.ts | 10 ++++++++++ .../cli/src/__tests__/extension-fn-secret-get.test.ts | 10 ++++++++++ .../src/__tests__/extension-github-tracking.test.ts | 10 ++++++++++ packages/cli/src/__tests__/extension-web-fetch.test.ts | 10 ++++++++++ 4 files changed, 40 insertions(+) diff --git a/packages/cli/src/__tests__/extension-experiment-finalize.test.ts b/packages/cli/src/__tests__/extension-experiment-finalize.test.ts index 657167889..c30b2caa5 100644 --- a/packages/cli/src/__tests__/extension-experiment-finalize.test.ts +++ b/packages/cli/src/__tests__/extension-experiment-finalize.test.ts @@ -57,6 +57,16 @@ vi.mock("@fusion/dashboard", () => ({ vi.mock("@fusion/engine", () => ({ createFnAgent: vi.fn(), fetchWebContent: vi.fn(), + emitGoalRetrievalAudit: vi.fn(), + createWorkflowAuthoringTools: vi.fn(() => ({})), + workflowListParams: {}, + workflowGetParams: {}, + workflowSelectParams: {}, + workflowCreateParams: {}, + workflowUpdateParams: {}, + workflowDeleteParams: {}, + workflowSettingsParams: {}, + traitListParams: {}, defaultGitOps: vi.fn(() => ({})), ExperimentFinalizeService: makeConstructibleMock(() => ({ previewPlan: previewPlanMock, finalize: finalizeMock })), ExperimentFinalizeStateError: mockErrors.StateError, diff --git a/packages/cli/src/__tests__/extension-fn-secret-get.test.ts b/packages/cli/src/__tests__/extension-fn-secret-get.test.ts index 8f5cedfc6..986d56cdd 100644 --- a/packages/cli/src/__tests__/extension-fn-secret-get.test.ts +++ b/packages/cli/src/__tests__/extension-fn-secret-get.test.ts @@ -13,6 +13,16 @@ vi.mock("@fusion/engine", () => ({ createFnAgent: vi.fn(), fetchWebContent: vi.fn(), assertNoSecretPlaintext: assertNoSecretPlaintextMock, + emitGoalRetrievalAudit: vi.fn(), + createWorkflowAuthoringTools: vi.fn(() => ({})), + workflowListParams: {}, + workflowGetParams: {}, + workflowSelectParams: {}, + workflowCreateParams: {}, + workflowUpdateParams: {}, + workflowDeleteParams: {}, + workflowSettingsParams: {}, + traitListParams: {}, })); vi.mock("@fusion/core", async () => { diff --git a/packages/cli/src/__tests__/extension-github-tracking.test.ts b/packages/cli/src/__tests__/extension-github-tracking.test.ts index d25d2f433..561f6e434 100644 --- a/packages/cli/src/__tests__/extension-github-tracking.test.ts +++ b/packages/cli/src/__tests__/extension-github-tracking.test.ts @@ -31,6 +31,16 @@ vi.mock("@fusion/engine", () => ({ createFnAgent: vi.fn(), fetchWebContent: vi.fn(), assertNoSecretPlaintext: vi.fn(), + emitGoalRetrievalAudit: vi.fn(), + createWorkflowAuthoringTools: vi.fn(() => ({})), + workflowListParams: {}, + workflowGetParams: {}, + workflowSelectParams: {}, + workflowCreateParams: {}, + workflowUpdateParams: {}, + workflowDeleteParams: {}, + workflowSettingsParams: {}, + traitListParams: {}, })); async function loadExtension() { diff --git a/packages/cli/src/__tests__/extension-web-fetch.test.ts b/packages/cli/src/__tests__/extension-web-fetch.test.ts index f85cc11eb..fc7add2ad 100644 --- a/packages/cli/src/__tests__/extension-web-fetch.test.ts +++ b/packages/cli/src/__tests__/extension-web-fetch.test.ts @@ -9,6 +9,16 @@ vi.mock("@fusion/dashboard", () => ({ vi.mock("@fusion/engine", () => ({ createFnAgent: vi.fn(), fetchWebContent: fetchWebContentMock, + emitGoalRetrievalAudit: vi.fn(), + createWorkflowAuthoringTools: vi.fn(() => ({})), + workflowListParams: {}, + workflowGetParams: {}, + workflowSelectParams: {}, + workflowCreateParams: {}, + workflowUpdateParams: {}, + workflowDeleteParams: {}, + workflowSettingsParams: {}, + traitListParams: {}, })); import kbExtension from "../extension.js"; From 87ce37831c4deefa304e9f91bfcec5e2991e18ec Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Wed, 1 Jul 2026 16:38:27 -0700 Subject: [PATCH 4/4] fix(engine): address review checkout routing feedback --- .../src/__tests__/review-checkout.test.ts | 9 +--- packages/engine/src/executor.ts | 48 +++++++++---------- packages/engine/src/review-checkout.ts | 11 ++--- 3 files changed, 29 insertions(+), 39 deletions(-) diff --git a/packages/engine/src/__tests__/review-checkout.test.ts b/packages/engine/src/__tests__/review-checkout.test.ts index a7cfa7a67..ef8b9c097 100644 --- a/packages/engine/src/__tests__/review-checkout.test.ts +++ b/packages/engine/src/__tests__/review-checkout.test.ts @@ -1,11 +1,6 @@ /* -Explicit external review checkout metadata contract tests. - -Resolves review checkout cwd from task metadata with fail-closed defaults: -- absent/blank/relative/non-git metadata → fallback (task worktree) -- valid explicit absolute git checkout → resolved realpath -- sourceMetadata.externalReviewCheckout is the canonical external field -- invalid higher-priority metadata → fallback, not silent lower-priority widening +FNXC:ReviewRouting 2026-07-01-16:36: +External review checkout contract tests pin fail-closed resolution: absent, blank, relative, or non-git metadata resolves to the task worktree fallback; valid explicit absolute git checkouts resolve to their realpath; sourceMetadata.externalReviewCheckout is the canonical external field; invalid higher-priority metadata must not silently widen to lower-priority metadata. */ import { describe, expect, it, beforeEach, afterEach } from "vitest"; import { execFileSync } from "node:child_process"; diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 68d0b5058..817cfed62 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -24,7 +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 { getTaskReviewCheckoutPath, 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"; @@ -1736,6 +1736,26 @@ export class TaskExecutor { } } + /** + * FNXC:ReviewRouting 2026-07-01-16:36: + * Review routing must expose whether the reviewer is using an explicit external checkout or the task worktree, but the invalid-sourceMetadata warning is only valid when sourceMetadata supplied the selected candidate. Higher-priority metadata can fail closed before sourceMetadata is considered, so centralize the logging to keep both review seams consistent and avoid false invalid-path warnings. + */ + private logReviewCheckoutRouting(taskId: string, task: unknown, reviewCwd: string, worktreePath: string): void { + if (reviewCwd !== worktreePath) { + reviewerLog.log(`${taskId}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); + return; + } + + const selectedCandidate = getTaskReviewCheckoutPath(task); + const sourceMetadata = task && typeof task === "object" ? (task as Record).sourceMetadata : undefined; + const sourceRecord = sourceMetadata && typeof sourceMetadata === "object" ? sourceMetadata as Record : undefined; + const sourceExternalReviewCheckout = sourceRecord?.externalReviewCheckout; + const sourceExternalReviewCheckoutPath = typeof sourceExternalReviewCheckout === "string" ? sourceExternalReviewCheckout.trim() : undefined; + if (sourceExternalReviewCheckoutPath && selectedCandidate === sourceExternalReviewCheckoutPath) { + reviewerLog.warn(`${taskId}: external review checkout metadata present (${sourceExternalReviewCheckoutPath}) but invalid — reviewing task worktree ${worktreePath}`); + } + } + private markCompletionFinalized(taskId: string): void { this.markPausedAborted(taskId, "completion-finalize", "completion-finalize"); this.completionFinalizedTaskIds.add(taskId); @@ -6363,18 +6383,7 @@ export class TaskExecutor { // 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); - // Make the actual review target visible: explicit - // sourceMetadata.externalReviewCheckout routes review to an external - // checkout; without valid metadata, review defaults to the task worktree. - if (reviewCwd !== worktreePath) { - reviewerLog.log(`${seamTask.id}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); - } else { - const sm = detail.sourceMetadata as Record | undefined; - const hasExternalMeta = sm && typeof sm.externalReviewCheckout === "string" && sm.externalReviewCheckout.trim(); - if (hasExternalMeta) { - reviewerLog.warn(`${seamTask.id}: external review checkout metadata present (${sm!.externalReviewCheckout}) but invalid — reviewing task worktree ${worktreePath}`); - } - } + this.logReviewCheckoutRouting(seamTask.id, detail, reviewCwd, worktreePath); const stepName = detail.steps[stepIndex]?.name ?? `Step ${stepIndex}`; const promptContent = detail.prompt ?? ""; const userComments = selectUserCommentsForAgentContext(detail, { limit: null }); @@ -12991,18 +13000,7 @@ export class TaskExecutor { const userComments = selectUserCommentsForAgentContext(latestDetailForReview, { limit: null }); const settings = await mergeEffectiveSettings(store, latestDetailForReview, await store.getSettings()); const reviewCwd = resolveReviewCheckoutCwd(latestDetailForReview, worktreePath); - // Make the actual review target visible: explicit - // sourceMetadata.externalReviewCheckout routes review to an external - // checkout; without valid metadata, review defaults to the task worktree. - if (reviewCwd !== worktreePath) { - reviewerLog.log(`${taskId}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); - } else { - const sm = latestDetailForReview.sourceMetadata as Record | undefined; - const hasExternalMeta = sm && typeof sm.externalReviewCheckout === "string" && sm.externalReviewCheckout.trim(); - if (hasExternalMeta) { - reviewerLog.warn(`${taskId}: external review checkout metadata present (${sm!.externalReviewCheckout}) but invalid — reviewing task worktree ${worktreePath}`); - } - } + this.logReviewCheckoutRouting(taskId, latestDetailForReview, reviewCwd, 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 diff --git a/packages/engine/src/review-checkout.ts b/packages/engine/src/review-checkout.ts index 83cffd1b6..c9eb5ab47 100644 --- a/packages/engine/src/review-checkout.ts +++ b/packages/engine/src/review-checkout.ts @@ -5,13 +5,10 @@ import { isAbsolute } from "node:path"; function readMetadataPath(value: unknown): string | undefined { if (!value || typeof value !== "object") return undefined; const record = value as Record; - // External review routing must be explicit metadata, not inferred from prompt - // text or task descriptions. The resolver only accepts known metadata fields, - // treats blank/non-string values as absent, and later validates that the chosen - // path is an absolute git checkout. Source priority is fixed - // (customFields > branchContext > sourceMetadata > root); an invalid higher - // priority candidate fails closed to the task worktree rather than silently - // falling through to a lower-priority path. + /* + FNXC:ReviewRouting 2026-07-01-16:36: + External review routing must be explicit metadata, not inferred from prompt text or task descriptions. The resolver only accepts known metadata fields, treats blank/non-string values as absent, and later validates that the chosen path is an absolute git checkout. Source priority is fixed (customFields > branchContext > sourceMetadata > root); an invalid higher-priority candidate fails closed to the task worktree rather than silently falling through to a lower-priority path. + */ for (const direct of [record.reviewCheckoutPath, record.externalReviewCheckoutPath, record.externalReviewCheckout]) { if (typeof direct === "string" && direct.trim()) return direct.trim(); }