fix(engine): route review via explicit external checkout metadata
This commit is contained in:
Phil Larson
266
packages/engine/src/__tests__/review-checkout.test.ts
Normal file
266
packages/engine/src/__tests__/review-checkout.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, unknown> | 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<string, unknown> | 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
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
/*
|
||||
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<string, unknown>;
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user