From 658b3511c78acfcef7f9af795dea678fdab351e6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 30 Jun 2026 12:25:43 -0700 Subject: [PATCH] FN-7293: scope external evidence to per-step review --- .../plan-review-external-evidence-scope.md | 7 ++ .../src/__tests__/builtin-workflows.test.ts | 13 ++++ .../core/src/builtin-plan-review-group.ts | 25 ++++--- .../builtin-stepwise-coding-workflow-ir.ts | 2 +- ...tepwise-final-review-coding-workflow-ir.ts | 13 ++++ .../executor-browser-verification.test.ts | 47 ++++++++++++ .../src/__tests__/executor-test-helpers.ts | 10 +++ ...e-review-spec-external-integration.test.ts | 67 ++++++++++++++++- packages/engine/src/executor.ts | 35 +++++++++ packages/engine/src/triage.ts | 72 ++++++++++++++++--- 10 files changed, 270 insertions(+), 21 deletions(-) create mode 100644 .changeset/plan-review-external-evidence-scope.md diff --git a/.changeset/plan-review-external-evidence-scope.md b/.changeset/plan-review-external-evidence-scope.md new file mode 100644 index 0000000000..ebc5bff2ac --- /dev/null +++ b/.changeset/plan-review-external-evidence-scope.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Scope external-integration plan evidence checks to Coding (per-step review). +category: fix +dev: Triage no longer blocks generated plans for missing external evidence; the per-step review Plan Review gate does. diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 43eaf22197..9f79b6d66c 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -9,6 +9,7 @@ import { isBuiltinWorkflowPluginGated, } from "../builtin-workflows.js"; import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; import { BROWSER_VERIFICATION_GROUP_ID, BROWSER_VERIFICATION_STEP_NODE_ID } from "../builtin-browser-verification-group.js"; import { CODE_REVIEW_STEP_NODE_ID } from "../builtin-code-review-group.js"; import { PLAN_REVIEW_GROUP_ID, PLAN_REVIEW_STEP_NODE_ID } from "../builtin-plan-review-group.js"; @@ -101,6 +102,18 @@ describe("built-in workflows", () => { } }); + it("scopes deterministic external-integration plan validation to Coding (per-step review)", () => { + const perStepPlanReview = planReviewInnerConfig(BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + const defaultCodingPlanReview = planReviewInnerConfig(BUILTIN_CODING_WORKFLOW_IR); + const legacyCodingPlanReview = planReviewInnerConfig(getBuiltinWorkflow("builtin:legacy-coding")!.ir); + const quickFixPlanReview = planReviewInnerConfig(getBuiltinWorkflow("builtin:quick-fix")!.ir); + + expect(perStepPlanReview.requireExternalIntegrationEvidence).toBe(true); + expect(defaultCodingPlanReview.requireExternalIntegrationEvidence).toBeUndefined(); + expect(legacyCodingPlanReview.requireExternalIntegrationEvidence).toBeUndefined(); + expect(quickFixPlanReview.requireExternalIntegrationEvidence).toBeUndefined(); + }); + it("all built-in Code Review optional groups are blocking gates", () => { for (const workflow of BUILTIN_WORKFLOWS) { const codeReview = workflow.ir.nodes.find((node) => node.id === "code-review"); diff --git a/packages/core/src/builtin-plan-review-group.ts b/packages/core/src/builtin-plan-review-group.ts index 8f008f96f2..237ecb227c 100644 --- a/packages/core/src/builtin-plan-review-group.ts +++ b/packages/core/src/builtin-plan-review-group.ts @@ -41,8 +41,23 @@ Be specific: cite the plan section or file path for every finding and explain th /** Build the `plan-review` optional-group node placed between planning and execution. */ export function planReviewOptionalGroupNode( column: string, - options: { defaultOn?: boolean; maxRevisions?: number | "unbounded" } = {}, + options: { defaultOn?: boolean; maxRevisions?: number | "unbounded"; requireExternalIntegrationEvidence?: boolean } = {}, ): WorkflowIrNode { + const promptConfig: Record = { + name: PLAN_REVIEW_NAME, + description: PLAN_REVIEW_DESCRIPTION, + prompt: PLAN_REVIEW_PROMPT, + toolMode: "readonly", + gateMode: "gate", + }; + if (options.requireExternalIntegrationEvidence === true) { + /* + * FNXC:PlanValidation 2026-06-30-08:56: + * Only Coding (per-step review) keeps deterministic external-integration evidence as part of Plan Review. Default Coding must not inherit this pre-review blocker; graph execution reads this flag from the Plan Review template node and turns missing evidence into a REVISE outcome. + */ + promptConfig.requireExternalIntegrationEvidence = true; + } + return { id: PLAN_REVIEW_GROUP_ID, kind: "optional-group", @@ -66,13 +81,7 @@ export function planReviewOptionalGroupNode( { id: PLAN_REVIEW_STEP_NODE_ID, kind: "prompt", - config: { - name: PLAN_REVIEW_NAME, - description: PLAN_REVIEW_DESCRIPTION, - prompt: PLAN_REVIEW_PROMPT, - toolMode: "readonly", - gateMode: "gate", - }, + config: promptConfig, }, ], edges: [], diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index 8d427771fc..730d17eba8 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -86,7 +86,7 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { { id: "start", kind: "start", column: "triage" }, // Planning seam: produces PROMPT.md (the declared step-source artifact). { id: "plan", kind: "prompt", column: "in-progress", config: builtinPromptConfig("planning", "Plan") }, - planReviewOptionalGroupNode("in-progress"), + planReviewOptionalGroupNode("in-progress", { requireExternalIntegrationEvidence: true }), planReplanNode("triage"), // KTD-12: parse the planned PROMPT.md into the task step list. This node must // dominate the foreach (validator-enforced). diff --git a/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts index 567514bf95..edeaae1e39 100644 --- a/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts @@ -33,6 +33,19 @@ const RAW_BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR: WorkflowIr = (() => throw new Error("stepwise final-review built-in requires the stepwise foreach template"); } + const inheritedPlanReview = ir.nodes.find((node) => node.id === "plan-review"); + const inheritedPlanReviewTemplate = inheritedPlanReview?.config?.template as + | { nodes?: Array<{ id: string; config?: Record }> } + | undefined; + const inheritedPlanReviewStep = inheritedPlanReviewTemplate?.nodes?.find((node) => node.id === "plan-review-step"); + if (inheritedPlanReviewStep?.config) { + /* + * FNXC:PlanValidation 2026-06-30-09:00: + * Default Coding is cloned from Coding (per-step review), but the deterministic external-integration evidence check belongs only to the review-heavy/per-step workflow. Remove the inherited flag here so default Coding relies on the normal Plan Review agent rather than pre-agent deterministic rejection. + */ + delete inheritedPlanReviewStep.config.requireExternalIntegrationEvidence; + } + const planIndex = ir.nodes.findIndex((node) => node.id === "plan"); if (planIndex < 0) { throw new Error("stepwise final-review built-in requires a plan node"); diff --git a/packages/engine/src/__tests__/executor-browser-verification.test.ts b/packages/engine/src/__tests__/executor-browser-verification.test.ts index 5e716b759f..a0170a0705 100644 --- a/packages/engine/src/__tests__/executor-browser-verification.test.ts +++ b/packages/engine/src/__tests__/executor-browser-verification.test.ts @@ -95,6 +95,23 @@ function browserVerificationStep(overrides: Record = {}) { }; } +function planReviewStep(overrides: Record = {}) { + return { + id: "graph:plan-review-step", + name: "Plan Review", + description: "", + mode: "prompt", + phase: "pre-merge", + gateMode: "gate", + prompt: "Review the plan.", + toolMode: "readonly", + enabled: true, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + }; +} + describe("browser-verification workflow-step browser capability", () => { beforeEach(() => { resetExecutorMocks(); @@ -264,4 +281,34 @@ describe("browser-verification workflow-step browser capability", () => { expect(store.logEntry.mock.calls.some(([, message]: [string, string]) => message.includes("[browser-verification]"))).toBe(false); expect(store.appendAgentLog.mock.calls.some(([, message]: [string, string]) => message.includes("[browser-verification]"))).toBe(false); }); + + it("returns a Plan Review revision for flagged external-integration evidence gaps without launching a session", async () => { + const store = createMockStore(); + store.getTask.mockResolvedValue({ + ...baseTask(), + prompt: "## Mission\nAdd an external CLI.\n\n## Steps\n- Download and run `wt` from https://github.com/worktrunk/worktrunk/releases/latest/download/wt-linux-x64.tar.gz\n", + }); + const executor = makeExecutor(store); + + const result = await (executor as any).executeWorkflowStep( + baseTask(), + planReviewStep({ requireExternalIntegrationEvidence: true }), + "/tmp/wt", + {}, + undefined, + undefined, + ); + + expect(result).toMatchObject({ + success: false, + revisionRequested: true, + verdict: "REVISE", + }); + expect(result.notes).toContain("External-integration evidence gaps"); + expect(mockedCreateFnAgent).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-7130", + expect.stringContaining("Plan Review deterministic external-integration evidence check requested revision"), + ); + }); }); diff --git a/packages/engine/src/__tests__/executor-test-helpers.ts b/packages/engine/src/__tests__/executor-test-helpers.ts index 9eb42edaa9..62f38e0bca 100644 --- a/packages/engine/src/__tests__/executor-test-helpers.ts +++ b/packages/engine/src/__tests__/executor-test-helpers.ts @@ -239,6 +239,16 @@ vi.mock("../step-session-executor.js", () => ({ steerActiveSessions: mockSteerActiveSessions, }; }), + extractSection: (prompt: string, sectionName: string) => { + const escaped = sectionName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const match = new RegExp(`^## ${escaped}\\s*$`, "m").exec(prompt); + if (!match) return ""; + const start = match.index; + const afterStart = start + match[0].length; + const nextHeading = prompt.indexOf("\n## ", afterStart); + const end = nextHeading === -1 ? prompt.length : nextHeading; + return prompt.slice(start, end).trim(); + }, })); vi.mock("../rate-limit-retry.js", () => ({ diff --git a/packages/engine/src/__tests__/triage-review-spec-external-integration.test.ts b/packages/engine/src/__tests__/triage-review-spec-external-integration.test.ts index 79993c9401..b5a144acfc 100644 --- a/packages/engine/src/__tests__/triage-review-spec-external-integration.test.ts +++ b/packages/engine/src/__tests__/triage-review-spec-external-integration.test.ts @@ -4,6 +4,7 @@ import { join } from "node:path"; import { tmpdir } from "node:os"; import type { TaskStore, TaskDetail, Settings } from "@fusion/core"; import { TriageProcessor } from "../triage.js"; +import { reviewStep } from "../reviewer.js"; vi.mock("@fusion/core", async (importOriginal) => { const { createEngineCoreMock } = await import("../test/mockCore.js"); @@ -12,6 +13,10 @@ vi.mock("@fusion/core", async (importOriginal) => { }); }); +vi.mock("../reviewer.js", () => ({ + reviewStep: vi.fn().mockResolvedValue({ verdict: "APPROVE", review: "ok", summary: "ok" }), +})); + function createMockStore(overrides: Partial = {}): TaskStore { return { getTask: vi.fn(), @@ -58,7 +63,7 @@ const mockTaskDetail: TaskDetail = { }; describe("triage deterministic plan validation for external integration evidence", () => { - it("rejects incomplete evidence without invoking reviewer", async () => { + it("does not reject incomplete evidence during deterministic triage validation", async () => { const rootDir = await mkdtemp(join(tmpdir(), "fusion-triage-ext-evidence-")); try { const taskId = "FN-5321"; @@ -71,7 +76,11 @@ describe("triage deterministic plan validation for external integration evidence `## Mission\nAdd third-party external binary integration.\n## Steps\n- install and probe \`worktrunk\` from release URL https://github.com/${fabricatedRepo}/releases/latest/download/worktrunk.tar.gz\n`, ); - expect(failure).toContain("External-integration evidence gaps"); + expect(failure).toBeNull(); + expect(store.logEntry).not.toHaveBeenCalledWith( + taskId, + expect.stringContaining("external-integration evidence gaps"), + ); } finally { await rm(rootDir, { recursive: true, force: true }); } @@ -110,4 +119,58 @@ describe("triage deterministic plan validation for external integration evidence await rm(rootDir, { recursive: true, force: true }); } }); + + it("blocks the per-step review workflow during Plan Review when external evidence is missing", async () => { + const rootDir = await mkdtemp(join(tmpdir(), "fusion-triage-ext-evidence-plan-review-")); + try { + const taskId = "FN-5321"; + const prompt = "## Mission\nAdd an external CLI.\n\n## Steps\n- Download and run `wt` from https://github.com/worktrunk/worktrunk/releases/latest/download/wt-linux-x64.tar.gz\n"; + const task = { + ...mockTaskDetail, + id: taskId, + enabledWorkflowSteps: ["plan-review"], + workflowStepResults: [], + }; + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(task), + getTaskWorkflowSelection: vi.fn().mockReturnValue({ workflowId: "builtin:stepwise-coding", stepIds: ["plan-review"] }), + getWorkflowDefinition: vi.fn().mockResolvedValue({ + id: "builtin:stepwise-coding", + ir: { + nodes: [ + { + id: "plan-review", + kind: "optional-group", + config: { + template: { + nodes: [ + { + id: "plan-review-step", + kind: "prompt", + config: { requireExternalIntegrationEvidence: true }, + }, + ], + }, + }, + }, + ], + }, + }), + } as Partial); + const processor = new TriageProcessor(store, rootDir); + + const result = await (processor as any).runPlanReviewBeforeExecution(task, prompt, {} as Settings); + + expect(result).toBe("blocked"); + expect(reviewStep).not.toHaveBeenCalled(); + expect(store.updateTask).toHaveBeenCalledWith(taskId, expect.objectContaining({ status: "needs-replan" })); + expect(store.logEntry).toHaveBeenCalledWith( + taskId, + "[pre-merge] Workflow step failed: Plan Review", + expect.stringContaining("External-integration evidence gaps"), + ); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index b6b3942c06..46c99f1529 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -148,6 +148,10 @@ import { TokenCapDetector } from "./token-cap-detector.js"; import { isUsageLimitError, checkSessionError, type UsageLimitPauser } from "./usage-limit-detector.js"; import { isNonContinuableSessionError, isTransientError, isSilentTransientError } from "./transient-error-detector.js"; import { withRateLimitRetry } from "./rate-limit-retry.js"; +import { + detectExternalIntegrationEvidenceGaps, + formatExternalIntegrationEvidenceDiagnostic, +} from "./spec-validation/external-integration-evidence.js"; import { computeRecoveryDecision, formatDelay, MAX_RECOVERY_RETRIES } from "./recovery-policy.js"; import type { StuckTaskDetector, StuckTaskEvent } from "./stuck-task-detector.js"; import type { PluginRunner } from "./plugin-runner.js"; @@ -7174,6 +7178,9 @@ export class TaskExecutor { if (cfg.summaryTarget === "task") { (step as WorkflowStep & { summaryTarget?: "task" }).summaryTarget = "task"; } + if (cfg.requireExternalIntegrationEvidence === true) { + (step as WorkflowStep & { requireExternalIntegrationEvidence?: boolean }).requireExternalIntegrationEvidence = true; + } // (U8a) Thread the plugin-injected runtime env (FUSION_CE_SKILLS_DIR / // FUSION_CE_AGENTS_DIR + PATH contribution) into prompt-mode skill/model @@ -13945,6 +13952,34 @@ ${scopeGuard} // only (default false = board run); see runGraphCustomNode / KTD-3. const unattended = stepOptions?.unattended === true; const isPlanReviewStep = workflowStep.id === "graph:plan-review-step" || workflowStep.name === "Plan Review"; + const requireExternalIntegrationEvidence = + (workflowStep as WorkflowStep & { requireExternalIntegrationEvidence?: boolean }).requireExternalIntegrationEvidence === true; + + if (isPlanReviewStep && requireExternalIntegrationEvidence) { + /* + * FNXC:PlanValidation 2026-06-30-09:03: + * Coding (per-step review) intentionally keeps external-integration evidence as a Plan Review gate. Enforce it here, not in triage, so only workflows that set `requireExternalIntegrationEvidence` block and failures route through the graph's normal plan-replan loop. + */ + const promptContent = await this.readTaskArtifact(task.id, "PROMPT.md"); + const evidenceGaps = detectExternalIntegrationEvidenceGaps({ + promptContent: typeof promptContent === "string" ? promptContent : "", + }); + if (evidenceGaps.length > 0) { + const diagnostic = formatExternalIntegrationEvidenceDiagnostic(evidenceGaps); + const output = `REVISE: ${diagnostic}`; + await this.store.logEntry( + task.id, + `[pre-merge] Plan Review deterministic external-integration evidence check requested revision: ${diagnostic}`, + ); + return { + success: false, + revisionRequested: true, + output, + verdict: "REVISE", + notes: diagnostic, + }; + } + } // Compute the diff scope so the workflow step agent reviews only what THIS // task changed — not unrelated files it might wander into. Without this, diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index e57d1e4335..4d143e09d8 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -7,6 +7,7 @@ import type { TaskAttachment, Settings, WorkflowStepResult, + WorkflowIr, } from "@fusion/core"; import { DUPLICATE_OF_METADATA_KEY, @@ -36,6 +37,8 @@ import { type NearDuplicateCandidate, } from "@fusion/core"; +const PLAN_REVIEW_TEMPLATE_STEP_NODE_ID = "plan-review-step"; + type TaskListClamp = (lines: string[], opts?: { maxChars?: number }) => string; type TaskListFormatter = ( lines: string[], @@ -1837,6 +1840,9 @@ export class TriageProcessor { /* FNXC:PlanReview 2026-06-29-01:52: Triage owns only deterministic PROMPT.md hygiene. AI plan quality review is graph-owned by the optional Plan Review step, so this helper must never call reviewer agents or require a fn_review_spec APPROVE verdict. + + FNXC:PlanValidation 2026-06-30-08:42: + External-integration evidence is a planning/review expectation, not a deterministic triage blocker. Operators saw valid generated plans fail before Plan Review with "Generated plan failed deterministic validation"; keep this local validator limited to structural task-file references the engine can prove. */ if (!promptContent.trim()) { return "PROMPT.md file not found or empty"; @@ -1853,16 +1859,6 @@ export class TriageProcessor { return diagnostic; } - const evidenceGaps = detectExternalIntegrationEvidenceGaps({ - promptContent, - }); - if (evidenceGaps.length > 0) { - const diagnostic = formatExternalIntegrationEvidenceDiagnostic(evidenceGaps); - planLog.warn(`${taskId}: ${diagnostic}`); - await this.store.logEntry(taskId, "Generated plan validation failed: external-integration evidence gaps"); - return diagnostic; - } - return null; } @@ -1874,6 +1870,30 @@ export class TriageProcessor { return Array.isArray(task.enabledWorkflowSteps) && task.enabledWorkflowSteps.includes(PLAN_REVIEW_GROUP_ID); } + private async shouldRequireExternalIntegrationEvidenceForPlanReview(task: Task): Promise { + /* + * FNXC:PlanValidation 2026-06-30-09:20: + * Triage may run Plan Review before the graph reaches `plan-review`; the graph later skips an already-passed Plan Review result. Read the selected workflow's Plan Review template flag here so Coding (per-step review) enforces external-integration evidence in the same Plan Review gate, while default Coding and other workflows stay unblocked. + */ + const selection = typeof this.store.getTaskWorkflowSelection === "function" + ? this.store.getTaskWorkflowSelection(task.id) + : undefined; + const workflowId = selection?.workflowId; + if (!workflowId || typeof this.store.getWorkflowDefinition !== "function") return false; + const definition = await this.store.getWorkflowDefinition(workflowId).catch((error: unknown) => { + const message = error instanceof Error ? error.message : String(error); + planLog.warn(`${task.id}: failed to resolve workflow '${workflowId}' for Plan Review evidence policy: ${message}`); + return undefined; + }); + const ir = definition?.ir as WorkflowIr | undefined; + const planReview = ir?.nodes.find((node) => node.id === PLAN_REVIEW_GROUP_ID); + const template = planReview?.config?.template as + | { nodes?: Array<{ id: string; config?: Record }> } + | undefined; + const planReviewStep = template?.nodes?.find((node) => node.id === PLAN_REVIEW_TEMPLATE_STEP_NODE_ID); + return planReviewStep?.config?.requireExternalIntegrationEvidence === true; + } + private async recordPlanReviewWorkflowResult(task: Task, result: WorkflowStepResult): Promise { const live = await this.store.getTask(task.id).catch((error: unknown) => { const message = error instanceof Error ? error.message : String(error); @@ -1911,6 +1931,38 @@ export class TriageProcessor { }); await this.store.logEntry(task.id, "[pre-merge] Starting workflow step: Plan Review"); + if (await this.shouldRequireExternalIntegrationEvidenceForPlanReview(task)) { + const evidenceGaps = detectExternalIntegrationEvidenceGaps({ promptContent }); + if (evidenceGaps.length > 0) { + const completedAt = new Date().toISOString(); + const diagnostic = formatExternalIntegrationEvidenceDiagnostic(evidenceGaps); + await this.recordPlanReviewWorkflowResult(task, { + workflowStepId: PLAN_REVIEW_GROUP_ID, + workflowStepName: "Plan Review", + phase: "pre-merge", + status: "failed", + verdict: "REVISE", + output: diagnostic, + notes: diagnostic, + startedAt, + completedAt, + }); + await this.store.logEntry(task.id, "[pre-merge] Workflow step failed: Plan Review", diagnostic); + await this.store.logEntry( + task.id, + "AI spec revision requested", + `Plan Review deterministic external-integration evidence check requested a planning revision before execution.\n\nFeedback:\n${diagnostic}`, + ); + await this.store.updateTask(task.id, { + status: "needs-replan", + error: null, + recoveryRetryCount: null, + nextRecoveryAt: null, + }); + return "blocked"; + } + } + const review = await reviewStep( this.rootDir, task.id,