root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(engine): address review checkout routing feedback

Browse Source
This commit is contained in:
Phil Larson
2026-07-01 16:38:27 -07:00
parent c46d11f0a3
commit 87ce37831c
3 changed files with 29 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
packages/engine/src/__tests__/review-checkout.test.ts
View File

@@ -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";

48
packages/engine/src/executor.ts
View File

@@ -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<string, unknown>).sourceMetadata : undefined;
const sourceRecord = sourceMetadata && typeof sourceMetadata === "object" ? sourceMetadata as Record<string, unknown> : 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<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}`);
}
}
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<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}`);
}
}
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

11
packages/engine/src/review-checkout.ts
View File

@@ -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<string, unknown>;
// 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();
}