From 87ce37831c4deefa304e9f91bfcec5e2991e18ec Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Wed, 1 Jul 2026 16:38:27 -0700 Subject: [PATCH] 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 a7cfa7a67d..ef8b9c0971 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 68d0b50583..817cfed622 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 83cffd1b69..c9eb5ab476 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(); }