diff --git a/.changeset/fn-8841-plan-review-no-op.md b/.changeset/fn-8841-plan-review-no-op.md new file mode 100644 index 0000000000..900356efcf --- /dev/null +++ b/.changeset/fn-8841-plan-review-no-op.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Let Plan Review close stale or duplicate work before implementation starts. +category: feature +dev: Adds the Plan-Review-only CLOSE_NO_OP sentinel verdict and terminal graph route. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 6f9b9d2583..89ca4f35b9 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -222,6 +222,20 @@ If the Plan Review reviewer is unavailable before producing a verdict, the task Workflow Plan Review is separate from manual plan approval. Project `planApprovalMode: "auto-approve-all"` bypasses only the final manual `awaiting-approval` plan gate after the plan is specified and any enabled Plan Review passes; it does not disable Plan Review or other explicit safety gates. +### Closing stale or duplicate work from Plan Review + +The built-in **Plan Review** optional group alone accepts a fourth structured verdict: + +```json +{"verdict":"CLOSE_NO_OP","notes":"DUPLICATE: FN-1234 already covered"} +``` + +Use it only when implementation should not begin because the premise is stale, the work is already satisfied or redundant, or another task already covers it. `notes` must begin with one of the existing no-op sentinels: `PREMISE STALE:`, `NO-OP:`, `NOOP:`, `REDUNDANT:`, or `DUPLICATE:`. For known duplicates, write `DUPLICATE: FN-NNNN ...`; Fusion preserves that canonical task ID in the Plan Review result. + +A valid close follows the graph's explicit terminal no-op route before parsing or implementation: it records a passed `CLOSE_NO_OP` Plan Review result, marks the task as no-commits-expected, records the reason, and completes it without replan or an implementation agent session. This is distinct from `REVISE`, which requests a corrected plan and follows the bounded replan route. + +`CLOSE_NO_OP` is not a generic review verdict. Code Review, Browser Verification, post-merge review, and custom non-Plan groups treat it as malformed/unknown output under their existing gate or advisory policy. A Plan Review close with empty/non-sentinel notes, or a custom workflow that has no explicit `outcome:close-no-op` terminal route, stays held at Plan Review with failed close evidence; it does not execute, replan, or fabricate a task error. + **FN-7559 (superseded by FN-7732) — telling the holds apart:** Plan Review parks a task with its own distinct statuses (`needs-replan` for a revision verdict, `plan-review-unavailable` for a reviewer-outage retry), so it never renders identically to a plan-approval hold. A separate triage release-authorization gate used to also use `status: "awaiting-approval"` with a distinct `awaitingApprovalReason: "release-authorization"` discriminator and its own dashboard label; that gate was removed (it over-fired on AI-authored specs that merely mentioned release tooling — see `b5b0458`, FN-7732). Releases are kept out of Fusion by agent instruction instead (AGENTS.md → "Releasing"), not by an engine/UI gate. The `Task.awaitingApprovalReason` field and its `"release-authorization"` value are kept only so legacy rows deserialize; any task that still carries the legacy value now renders as an ordinary manual plan-approval hold. **FN-7569 / FN-8008 — manual plan approval is idempotent against unchanged plans:** approving a plan under the manual gate records a fingerprint of the approved `PROMPT.md`, normalized to ignore deterministic `## Original Description` and Frontend UX hygiene sections. If the same task is later re-specified — a `needs-replan` replan, a Plan Review reviewer-outage retry, or a self-healing rebound back to `triage` — the manual gate detects the unchanged operator-authored plan and proceeds straight to `todo` instead of re-parking at `status: "awaiting-approval"`, regardless of whether those generated sections were injected before either fingerprint was calculated. A genuinely revised Mission, Steps, or File Scope still produces a different fingerprint and re-asks as before, and using Reject Plan clears the fingerprint so the regenerated plan is always treated as new. This idempotency check runs only inside the manual gate, strictly after Plan Review has already made its independent decision, and never applies under `planApprovalMode: "auto-approve-all"` (which bypasses the manual gate entirely). diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index a495828fb7..181da0c0d1 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -122,6 +122,30 @@ describe("built-in workflows", () => { expect(quickFixPlanReview.requireExternalIntegrationEvidence).toBeUndefined(); }); + it("routes Plan Review CLOSE_NO_OP to the terminal no-op action in every executable consumer", () => { + const workflows = [ + BUILTIN_CODING_WORKFLOW_IR, + BUILTIN_STEPWISE_CODING_WORKFLOW_IR, + BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR, + ]; + for (const ir of workflows) { + expect(ir.nodes.find((node) => node.id === "plan-review-no-op"), ir.name).toMatchObject({ + kind: "gate", + config: { workflowAction: "plan-review-no-op" }, + }); + expect(ir.edges, ir.name).toContainEqual({ + from: PLAN_REVIEW_GROUP_ID, + to: "plan-review-no-op", + condition: "outcome:close-no-op", + }); + expect(ir.edges, ir.name).toContainEqual({ + from: "plan-review-no-op", + to: "end", + condition: "success", + }); + } + }); + 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/types/task/task-review.ts b/packages/core/src/types/task/task-review.ts index d9c2aa5f28..69e9c295ba 100644 --- a/packages/core/src/types/task/task-review.ts +++ b/packages/core/src/types/task/task-review.ts @@ -6,7 +6,12 @@ export type TaskReviewMode = "pull-request" | "direct"; export type TaskReviewSource = "github-pr" | "reviewer-agent"; export type TaskReviewDecision = "approved" | "changes-requested" | "commented" | "pending"; -export type TaskReviewVerdict = "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "RETHINK" | "UNAVAILABLE"; +/* + * FNXC:PlanReviewNoOp 2026-08-09-01:17: + * The Review data projection must retain the Plan-Review-only close verdict as audit evidence; + * consumers render it as a terminal review decision rather than converting it into a revision. + */ +export type TaskReviewVerdict = "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "RETHINK" | "UNAVAILABLE" | "CLOSE_NO_OP"; export type TaskReviewerType = "plan" | "code"; export type TaskReviewItemStatus = "queued" | "in-progress" | "addressed" | "failed"; export type TaskReviewFindingSeverity = "low" | "medium" | "high" | "critical"; diff --git a/packages/core/src/types/workflow/workflow-steps.ts b/packages/core/src/types/workflow/workflow-steps.ts index a4baa571da..83c32f6d88 100644 --- a/packages/core/src/types/workflow/workflow-steps.ts +++ b/packages/core/src/types/workflow/workflow-steps.ts @@ -281,7 +281,7 @@ export interface WorkflowStepResult { * Machine-readable verdict from prompt-mode structured output. * Absent for script-mode steps and legacy prose-only prompt outputs. */ - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; /** * Optional notes from prompt-mode structured output. * Absent for script-mode steps and legacy prose-only prompt outputs. diff --git a/packages/core/src/workflows/builtin-coding-workflow-ir.ts b/packages/core/src/workflows/builtin-coding-workflow-ir.ts index cd2448a46f..872cd592d6 100644 --- a/packages/core/src/workflows/builtin-coding-workflow-ir.ts +++ b/packages/core/src/workflows/builtin-coding-workflow-ir.ts @@ -82,6 +82,7 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { // builtin-stepwise-coding-workflow-ir.ts. planReviewOptionalGroupNode("todo"), planReplanNode("todo"), + { id: "plan-review-no-op", kind: "gate", column: "todo", config: { workflowAction: "plan-review-no-op" } }, { id: "execute", kind: "prompt", @@ -147,6 +148,8 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { { from: "start", to: "planning" }, { from: "planning", to: "plan-review", condition: "success" }, { from: "plan-review", to: "execute", condition: "success" }, + { from: "plan-review", to: "plan-review-no-op", condition: "outcome:close-no-op" }, + { from: "plan-review-no-op", to: "end", condition: "success" }, // execute → browser-verification (optional-group) → review. When the group is // disabled it passes through with outcome=success and routes straight to review. { from: "execute", to: "browser-verification", condition: "success" }, diff --git a/packages/core/src/workflows/builtin-plan-review-group.ts b/packages/core/src/workflows/builtin-plan-review-group.ts index e3cea126f7..e94d18fdfd 100644 --- a/packages/core/src/workflows/builtin-plan-review-group.ts +++ b/packages/core/src/workflows/builtin-plan-review-group.ts @@ -38,8 +38,9 @@ Be specific: cite the plan section or file path for every finding and explain th - APPROVE: the plan is ready for execution. - APPROVE_WITH_NOTES: execution may proceed, but include non-blocking advisory notes. - REVISE: the plan should be corrected before execution; include every blocking finding and needed change in the JSON notes, not only in preceding prose. +- CLOSE_NO_OP: implementation must not proceed because the premise is stale, the work is already satisfied, redundant, or a duplicate. The notes field MUST begin with exactly one existing completion sentinel: PREMISE STALE:, NO-OP:, NOOP:, REDUNDANT:, or DUPLICATE:. For duplicates, use DUPLICATE: FN-NNNN ... when the canonical task is known. - Final output: output exactly one trailing JSON object on the final line (no markdown fences, no surrounding prose): -{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE","notes":"..."}`; +{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE|CLOSE_NO_OP","notes":"..."}`; /* FNXC:PlanReviewStep 2026-07-27-06:10: diff --git a/packages/core/src/workflows/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/workflows/builtin-stepwise-coding-workflow-ir.ts index 44e80cd642..37a0e2ee57 100644 --- a/packages/core/src/workflows/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/workflows/builtin-stepwise-coding-workflow-ir.ts @@ -153,6 +153,7 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { { id: "plan", kind: "prompt", column: "todo", config: builtinPromptConfig("planning", "Plan") }, planReviewOptionalGroupNode("todo", { requireExternalIntegrationEvidence: true }), planReplanNode("todo"), + { id: "plan-review-no-op", kind: "gate", column: "todo", config: { workflowAction: "plan-review-no-op" } }, // KTD-12: parse the planned PROMPT.md into the task step list. This node must // dominate the foreach (validator-enforced). { @@ -302,6 +303,8 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { { from: "plan", to: "plan-review", condition: "success" }, { from: "plan", to: "end", condition: "failure" }, { from: "plan-review", to: "parse", condition: "success" }, + { from: "plan-review", to: "plan-review-no-op", condition: "outcome:close-no-op" }, + { from: "plan-review-no-op", to: "end", condition: "success" }, { from: "plan-review", to: "plan-replan", condition: "failure" }, { from: "plan-replan", to: "plan-review", condition: "success", kind: "rework" }, { from: "parse", to: "steps", condition: "success" }, diff --git a/packages/core/src/workflows/builtin-workflows.ts b/packages/core/src/workflows/builtin-workflows.ts index 7ddb990180..f0a2b2ec07 100644 --- a/packages/core/src/workflows/builtin-workflows.ts +++ b/packages/core/src/workflows/builtin-workflows.ts @@ -453,6 +453,7 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ plan: { x: 230, y: 160 }, "plan-review": { x: 400, y: 160 }, "plan-replan": { x: 400, y: 320 }, + "plan-review-no-op": { x: 570, y: 320 }, parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ @@ -491,6 +492,7 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ plan: { x: 230, y: 160 }, "plan-review": { x: 400, y: 160 }, "plan-replan": { x: 400, y: 320 }, + "plan-review-no-op": { x: 570, y: 320 }, parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ @@ -528,6 +530,7 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ planning: { x: 230, y: 160 }, "plan-review": { x: 400, y: 160 }, "plan-replan": { x: 400, y: 320 }, + "plan-review-no-op": { x: 570, y: 320 }, execute: { x: 570, y: 160 }, "browser-verification": { x: 740, y: 160 }, "browser-verification-remediation": { x: 740, y: 320 }, @@ -719,6 +722,7 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ plan: { x: 230, y: 160 }, "plan-review": { x: 400, y: 160 }, "plan-replan": { x: 400, y: 320 }, + "plan-review-no-op": { x: 570, y: 320 }, parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ @@ -859,6 +863,7 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ plan: { x: 740, y: 160 }, "plan-review": { x: 910, y: 160 }, "plan-replan": { x: 910, y: 320 }, + "plan-review-no-op": { x: 1080, y: 320 }, parse: { x: 1080, y: 160 }, steps: { x: 1250, y: 160 }, /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ diff --git a/packages/engine/src/__tests__/plan-review-no-op.test.ts b/packages/engine/src/__tests__/plan-review-no-op.test.ts new file mode 100644 index 0000000000..ee504908e0 --- /dev/null +++ b/packages/engine/src/__tests__/plan-review-no-op.test.ts @@ -0,0 +1,289 @@ +import { describe, expect, it, vi } from "vitest"; +import type { TaskDetail, WorkflowIr, WorkflowStepResult } from "@fusion/core"; +import { + BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR, + PLAN_REVIEW_GROUP_ID, + upsertWorkflowStepResult, +} from "@fusion/core"; +import { TaskExecutor } from "../executor.js"; +import { WorkflowGraphExecutor } from "../workflows/workflow-graph-executor.js"; +import { WorkflowGraphTaskRunner } from "../workflows/workflow-graph-task-runner.js"; + +function planReviewIr(withRoute = true): WorkflowIr { + return { + version: "v2", + name: "plan-review-no-op-test", + columns: [{ id: "todo", name: "Todo", traits: [] }, { id: "done", name: "Done", traits: [] }], + nodes: [ + { id: "start", kind: "start" }, + { id: PLAN_REVIEW_GROUP_ID, kind: "optional-group", config: { name: "Plan Review", reviewKind: "plan", defaultOn: true, template: { nodes: [{ id: "review", kind: "prompt", config: { prompt: "review" } }], edges: [] } } }, + ...(withRoute ? [{ id: "plan-review-no-op", kind: "gate" as const, config: { workflowAction: "plan-review-no-op" } }] : []), + { id: "execute", kind: "prompt", config: { prompt: "execute" } }, + { id: "plan-replan", kind: "prompt", config: { prompt: "replan" } }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: PLAN_REVIEW_GROUP_ID }, + { from: PLAN_REVIEW_GROUP_ID, to: "execute", condition: "success" }, + { from: PLAN_REVIEW_GROUP_ID, to: "plan-replan", condition: "failure" }, + ...(withRoute ? [{ from: PLAN_REVIEW_GROUP_ID, to: "plan-review-no-op", condition: "outcome:close-no-op" }, { from: "plan-review-no-op", to: "end", condition: "success" }] : []), + { from: "execute", to: "end" }, { from: "plan-replan", to: "end" }, + ], + }; +} + +const task = (): TaskDetail => ({ id: "FN-123", enabledWorkflowSteps: [PLAN_REVIEW_GROUP_ID] } as TaskDetail); +const close = { outcome: "success" as const, value: "CLOSE_NO_OP", contextPatch: { notes: "DUPLICATE: FN-1234 already covered" } }; + +describe("Plan Review CLOSE_NO_OP", () => { + it("completes a valid duplicate close without execution or replan", async () => { + const executed: string[] = []; + const results: WorkflowStepResult[] = []; + const complete = vi.fn(async () => true); + const executor = new WorkflowGraphExecutor({ + handlers: { prompt: async (node) => { executed.push(node.id); return close; } }, + recordWorkflowStepResult: async (_id, result) => { results.push(result); }, + completePlanReviewNoOp: complete, + }); + const result = await executor.run(task(), { experimentalFeatures: { workflowGraphExecutor: true } }, planReviewIr()); + + expect(result.outcome).toBe("success"); + expect(executed).toEqual(["review"]); + expect(result.visitedNodeIds).toEqual(["start", PLAN_REVIEW_GROUP_ID, "plan-review::review", "plan-review-no-op"]); + expect(complete).toHaveBeenCalledWith(expect.objectContaining({ id: "FN-123" }), { + kind: "duplicate", reason: "FN-1234 already covered", canonicalId: "FN-1234", + }); + expect(results.at(-1)).toMatchObject({ status: "passed", verdict: "CLOSE_NO_OP", notes: "DUPLICATE: FN-1234 already covered" }); + }); + + it("takes the terminal close route in every executable built-in without implementation dispatch", async () => { + const workflows = [ + { id: "builtin:coding" }, + { id: "builtin:stepwise-coding" }, + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:52: + * The derived final-review topology is the catalog's builtin:coding IR. Load it + * directly as a custom definition too, ratcheting the clone independently. + */ + { id: "WF-stepwise-final", ir: BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR }, + ]; + for (const workflow of workflows) { + const workflowId = workflow.id; + const persisted: WorkflowStepResult[] = []; + const complete = vi.fn(async () => true); + const implementation = vi.fn(async () => ({ outcome: "success" as const })); + const runner = new WorkflowGraphTaskRunner({ + store: { + getTaskWorkflowSelection: () => ({ workflowId, stepIds: [PLAN_REVIEW_GROUP_ID] }), + getTaskWorkflowSelectionAsync: async () => ({ workflowId, stepIds: [PLAN_REVIEW_GROUP_ID] }), + getWorkflowDefinition: async () => workflow.ir ? ({ id: workflowId, ir: workflow.ir } as never) : undefined, + }, + seams: { + planning: async () => ({ outcome: "success" }), + execute: implementation, + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + }, + runCustomNode: async (node) => node.id === "plan-review-step" + ? close + : { outcome: "failure", value: `unexpected:${node.id}` }, + recordWorkflowStepResult: async (_taskId, stepResult) => { persisted.push(stepResult); }, + completePlanReviewNoOp: complete, + }); + const result = await runner.run(task(), { experimentalFeatures: { workflowGraphExecutor: true } }, PLAN_REVIEW_GROUP_ID); + + expect(result.disposition, workflowId).toBe("completed"); + expect(result.visitedNodeIds, workflowId).toContain("plan-review-no-op"); + expect(implementation, workflowId).not.toHaveBeenCalled(); + expect(complete, workflowId).toHaveBeenCalledWith(expect.objectContaining({ id: "FN-123" }), { + kind: "duplicate", reason: "FN-1234 already covered", canonicalId: "FN-1234", + }); + expect(persisted.at(-1), workflowId).toMatchObject({ + status: "passed", verdict: "CLOSE_NO_OP", notes: "DUPLICATE: FN-1234 already covered", + }); + } + }); + + it("holds invalid close notes without execution, replan, or completion", async () => { + const executed: string[] = []; + const complete = vi.fn(async () => true); + const executor = new WorkflowGraphExecutor({ + handlers: { prompt: async (node) => { executed.push(node.id); return { ...close, contextPatch: { notes: "already done" } }; } }, + completePlanReviewNoOp: complete, + }); + const result = await executor.run(task(), { experimentalFeatures: { workflowGraphExecutor: true } }, planReviewIr()); + expect(result.outcome).toBe("success"); + expect(result.suspended).toMatchObject({ reason: "hold", nodeId: PLAN_REVIEW_GROUP_ID }); + expect(executed).toEqual(["review"]); + expect(result.visitedNodeIds).toEqual(["start", PLAN_REVIEW_GROUP_ID, "plan-review::review"]); + expect(complete).not.toHaveBeenCalled(); + }); + + it("holds a valid close when an authored workflow has no terminal route", async () => { + const complete = vi.fn(async () => true); + const results: WorkflowStepResult[] = []; + const executor = new WorkflowGraphExecutor({ + handlers: { prompt: async () => close }, + recordWorkflowStepResult: async (_id, result) => { results.push(result); }, + completePlanReviewNoOp: complete, + }); + const result = await executor.run(task(), { experimentalFeatures: { workflowGraphExecutor: true } }, planReviewIr(false)); + expect(result.outcome).toBe("success"); + expect(result.suspended).toMatchObject({ reason: "hold", nodeId: PLAN_REVIEW_GROUP_ID }); + expect(result.visitedNodeIds).toEqual(["start", PLAN_REVIEW_GROUP_ID, "plan-review::review"]); + expect(complete).not.toHaveBeenCalled(); + expect(results.at(-1)).toMatchObject({ status: "failed", verdict: "CLOSE_NO_OP", output: "Plan Review CLOSE_NO_OP terminal route unavailable." }); + }); + + it("replaces passed close evidence and holds when terminalization fails", async () => { + const results: WorkflowStepResult[] = []; + const executor = new WorkflowGraphExecutor({ + handlers: { prompt: async () => close }, + recordWorkflowStepResult: async (_id, result) => { results.push(result); }, + completePlanReviewNoOp: async () => false, + }); + const result = await executor.run(task(), { experimentalFeatures: { workflowGraphExecutor: true } }, planReviewIr()); + + expect(result.suspended).toMatchObject({ reason: "hold", nodeId: PLAN_REVIEW_GROUP_ID }); + expect(result.visitedNodeIds).toEqual(["start", PLAN_REVIEW_GROUP_ID, "plan-review::review", "plan-review-no-op"]); + expect(results.at(-1)).toMatchObject({ + status: "failed", + verdict: "CLOSE_NO_OP", + notes: "DUPLICATE: FN-1234 already covered", + output: "Plan Review CLOSE_NO_OP terminalization failed.", + }); + }); + + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:52: + * A close can race an operator pause after the reviewer result is durable but before + * the terminal action starts. Exercise the real runner → TaskExecutor completion + * handoff so this fence cannot regress into a direct helper-only ordering test. + */ + it("holds the real runner continuation when a pause wins after close evidence", async () => { + const live = { + id: "FN-123", + title: "Duplicate task", + column: "todo", + steps: [], + enabledWorkflowSteps: [PLAN_REVIEW_GROUP_ID], + workflowStepResults: [], + } as unknown as TaskDetail; + const continuations: Array> = []; + const store = { + on: vi.fn(), + getTask: vi.fn(async () => live), + getSettings: vi.fn(async () => ({ experimentalFeatures: { workflowGraphExecutor: true } })), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "builtin:coding", stepIds: [PLAN_REVIEW_GROUP_ID] })), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "builtin:coding", stepIds: [PLAN_REVIEW_GROUP_ID] })), + updateTask: vi.fn(async (_taskId: string, patch: Record) => { + Object.assign(live, patch); + return live; + }), + updateStep: vi.fn(), + logEntry: vi.fn(), + moveTask: vi.fn(), + replaceActiveTaskWorkflowContinuation: vi.fn(async (input: Record) => { + continuations.push(input); + return { id: "held-close", ...input }; + }), + }; + const taskExecutor = new TaskExecutor(store as never, "/tmp/plan-review-no-op-real-race"); + const recordResult = vi.fn(async (_taskId: string, result: WorkflowStepResult) => { + live.workflowStepResults = upsertWorkflowStepResult(live.workflowStepResults, result); + /* FNXC:PlanReviewNoOp 2026-08-09-02:52: Pause after durable evidence to prove the completion fence retains the review hold. */ + live.paused = true; + live.userPaused = true; + }); + const runner = new WorkflowGraphTaskRunner({ + store: store as never, + seams: { + planning: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + }, + runCustomNode: vi.fn(async (node) => { + expect(node.id).toBe("plan-review-step"); + return close; + }), + recordWorkflowStepResult: recordResult, + completePlanReviewNoOp: (nodeTask, marker) => (taskExecutor as unknown as { + completePlanReviewNoOp: (task: TaskDetail, parsedMarker: typeof marker) => Promise; + }).completePlanReviewNoOp(nodeTask, marker), + holdPlanReviewNoOp: async (nodeTask, suspension) => { + await (taskExecutor as unknown as { + holdPlanReviewNoOpContinuation: ( + task: TaskDetail, + heldSuspension: typeof suspension, + continuation: undefined, + runId: string, + ) => Promise; + }).holdPlanReviewNoOpContinuation(nodeTask, suspension, undefined, "FN-123:builtin:coding"); + }, + }); + + const result = await runner.run(live, { experimentalFeatures: { workflowGraphExecutor: true } }, PLAN_REVIEW_GROUP_ID); + + expect(result.disposition).toBe("suspended"); + expect(result.suspension).toMatchObject({ reason: "hold", nodeId: PLAN_REVIEW_GROUP_ID }); + expect(live).toMatchObject({ column: "todo", paused: true, userPaused: true }); + expect(live.workflowStepResults.at(-1)).toMatchObject({ + status: "failed", + verdict: "CLOSE_NO_OP", + notes: "DUPLICATE: FN-1234 already covered", + output: "Plan Review CLOSE_NO_OP terminalization failed.", + }); + expect(continuations.at(-1)).toMatchObject({ + nodeId: PLAN_REVIEW_GROUP_ID, + state: "held", + waitReason: "planning", + blockedReason: "plan-review-close-terminalization-failed", + }); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + + it("retains the held continuation when a user pause wins the close race", async () => { + const pausedTask = { + id: "FN-123", + column: "todo", + paused: true, + userPaused: true, + } as TaskDetail; + const replace = vi.fn(async (input: Record) => ({ id: "held-close", ...input })); + const store = { + on: vi.fn(), + getTask: vi.fn(async () => pausedTask), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "builtin:coding", stepIds: [] })), + replaceActiveTaskWorkflowContinuation: replace, + }; + const executor = new TaskExecutor(store as never, "/tmp/plan-review-no-op-race"); + + const held = await (executor as unknown as { + holdPlanReviewNoOpContinuation: ( + task: TaskDetail, + suspension: { reason: "terminalization-failed"; nodeId: string; fromColumn: string; toColumn: string; irHash: string }, + continuation: undefined, + runId: string, + ) => Promise<{ state: string; waitReason: string; nodeId: string; taskId: string }>; + }).holdPlanReviewNoOpContinuation(pausedTask, { + reason: "terminalization-failed", + nodeId: PLAN_REVIEW_GROUP_ID, + fromColumn: "todo", + toColumn: "todo", + irHash: "test-ir", + }, undefined, "FN-123:builtin:coding"); + + expect(replace).toHaveBeenCalledWith(expect.objectContaining({ + taskId: "FN-123", + nodeId: PLAN_REVIEW_GROUP_ID, + state: "held", + waitReason: "planning", + blockedReason: "plan-review-close-terminalization-failed", + })); + expect(held).toMatchObject({ state: "held", waitReason: "planning" }); + expect(pausedTask).toMatchObject({ paused: true, userPaused: true }); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts b/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts index c42ae44adc..508b04b69e 100644 --- a/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts +++ b/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts @@ -19,6 +19,24 @@ describe("parseWorkflowStepVerdict", () => { expect(parseWorkflowStepVerdict('{"verdict":"PASS"}')).toBeNull(); }); + it("recognizes CLOSE_NO_OP only for the Plan Review optional group", () => { + const response = '{"verdict":"CLOSE_NO_OP","notes":"DUPLICATE: FN-1234 already covered"}'; + expect(parseWorkflowStepVerdict(response, { optionalGroupId: "plan-review" })).toMatchObject({ + verdict: "CLOSE_NO_OP", + notes: "DUPLICATE: FN-1234 already covered", + }); + expect(parseWorkflowStepVerdict(response, { optionalGroupId: "code-review" })).toBeNull(); + expect(parseWorkflowStepVerdict(response)).toBeNull(); + }); + + it("prefers a trailing Plan Review close JSON payload", () => { + const response = 'Example: {"verdict":"REVISE"}\n{"verdict":"CLOSE_NO_OP","notes":"PREMISE STALE: already shipped"}'; + expect(parseWorkflowStepVerdict(response, { optionalGroupId: "plan-review" })).toEqual({ + verdict: "CLOSE_NO_OP", + notes: "PREMISE STALE: already shipped", + }); + }); + /* FNXC:ReviewLeniency 2026-07-01-23:30: Models often emit reasoning PROSE (sometimes containing braces) then a trailing diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 33e282d9de..47d3e741d6 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1319,7 +1319,7 @@ export interface WorkflowStepOutcome { output?: string; error?: string; /** Machine-readable verdict extracted from structured JSON output. */ - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; /** Notes extracted from structured JSON output (distinct from raw output). */ notes?: string; /** Normalized independently actionable feedback from a review-kind node. */ @@ -1344,7 +1344,10 @@ export type WorkflowStepResult = | { allPassed: false; revisionRequested: false; feedback: string; stepName: string } | { allPassed: false; revisionRequested: true; feedback: string; stepName: string }; -export function parseWorkflowStepVerdict(rawOutput: string): { verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; notes: string; findings?: WorkflowReviewFinding[] } | null { +export function parseWorkflowStepVerdict( + rawOutput: string, + options: { optionalGroupId?: string } = {}, +): { verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; notes: string; findings?: WorkflowReviewFinding[] } | null { const trimmed = rawOutput.trim(); const candidates: string[] = []; const fencedMatches = [...trimmed.matchAll(/```(?:json)?\s*([\s\S]*?)```/g)]; @@ -1366,9 +1369,16 @@ export function parseWorkflowStepVerdict(rawOutput: string): { verdict: "APPROVE "Any approved" — accept approval-family verdict variants (APPROVE, APPROVED, APPROVE_WITH_NOTES, approve_with_verdict, …), not just the exact WORKFLOW_STEP_VERDICTS strings. A token starting with APPROVE maps to APPROVE_WITH_NOTES when it mentions notes, else APPROVE; REVISE-family → REVISE; anything else (e.g. "PASS") is not a verdict and the candidate is skipped. */ const token = parsed.verdict.trim().toUpperCase(); - let verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | null = null; + let verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP" | null = null; if (token.startsWith("APPROVE") || token.startsWith("APPROVAL")) { verdict = token.includes("NOTE") ? "APPROVE_WITH_NOTES" : "APPROVE"; + } else if (token === "CLOSE_NO_OP" && options.optionalGroupId === PLAN_REVIEW_GROUP_ID) { + /* + * FNXC:PlanReviewNoOp 2026-08-09-01:17: + * Only the built-in Plan Review protocol may request a no-op close. Exact matching + * prevents prose or unrelated review groups from acquiring a terminal lifecycle path. + */ + verdict = "CLOSE_NO_OP"; } else if (token.startsWith("REVISE") || token.startsWith("REQUEST_REVISION") || token.startsWith("REJECT")) { verdict = "REVISE"; } @@ -1423,27 +1433,34 @@ export function inferWorkflowStepVerdictFromProse(rawOutput: string): { verdict: */ export function parseWorkflowStepOutput(rawOutput: string): { output: string; - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; notes?: string; findings?: WorkflowReviewFinding[]; malformed?: boolean; }; -export function parseWorkflowStepOutput(rawOutput: string, options: { requireVerdict: false }): { +export function parseWorkflowStepOutput(rawOutput: string, options: { optionalGroupId?: string }): { output: string; - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; notes?: string; findings?: WorkflowReviewFinding[]; malformed?: boolean; }; -export function parseWorkflowStepOutput(rawOutput: string, options: { requireVerdict?: boolean } = {}): { +export function parseWorkflowStepOutput(rawOutput: string, options: { requireVerdict: false; optionalGroupId?: string }): { output: string; - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; + notes?: string; + findings?: WorkflowReviewFinding[]; + malformed?: boolean; +}; +export function parseWorkflowStepOutput(rawOutput: string, options: { requireVerdict?: boolean; optionalGroupId?: string } = {}): { + output: string; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; notes?: string; findings?: WorkflowReviewFinding[]; malformed?: boolean; } { const trimmed = rawOutput.trim(); - const parsed = parseWorkflowStepVerdict(trimmed); + const parsed = parseWorkflowStepVerdict(trimmed, options); if (parsed) { return { output: parsed.notes || "", @@ -5723,6 +5740,205 @@ export class TaskExecutor { ); } + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:08: + * Accepted no-op completion has one lifecycle handoff for both fn_task_done and Plan Review. + * The close path must not emulate completion with a column patch: this primitive records the + * canonical marker, completes steps, and uses the same watchdog-owned handoff as an executor. + */ + private async finalizeAcceptedNoOpCompletion(params: { + task: TaskDetail; + marker: { kind: string; reason: string; canonicalId?: string }; + summary: string; + recommendations?: TaskRecommendation[]; + onDone?: () => void; + rejectIfPaused?: boolean; + }): Promise<{ completed: boolean; hardPauseActive: boolean }> { + const { task, marker, summary, recommendations, onDone, rejectIfPaused = false } = params; + const isRejectedCloseState = async (): Promise => { + const current = await this.store.getTask(task.id); + return !current + || Boolean(current.deletedAt) + || (await resolveTerminalColumnsFor(this.store, task.id)).includes(current.column) + || (rejectIfPaused && (current.paused === true || current.userPaused === true)); + }; + const live = await this.store.getTask(task.id); + if (!live || live.deletedAt || (await resolveTerminalColumnsFor(this.store, task.id)).includes(live.column)) { + return { completed: false, hardPauseActive: false }; + } + if (rejectIfPaused && (live.paused || live.userPaused)) return { completed: false, hardPauseActive: false }; + + const runContext = this.getRunContextFor(task.id); + const restoreNoCommitsExpected = async (): Promise => { + if (live.noCommitsExpected !== true) { + await this.store.updateTask(task.id, { noCommitsExpected: false }).catch(() => undefined); + } + }; + try { + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:28: + * A reviewer close must lose to a concurrent user pause, deletion, or terminal handoff. + * Re-read immediately before each lifecycle boundary and never clear pause fields on this + * path, so accepting a close cannot resurrect or complete operator-withdrawn work. + */ + if (await isRejectedCloseState()) return { completed: false, hardPauseActive: false }; + await this.store.updateTask(task.id, { noCommitsExpected: true }); + await this.store.logEntry( + task.id, + `Verified ${marker.kind} completion sentinel accepted; no commits expected for terminal handoff`, + JSON.stringify({ kind: marker.kind, reason: marker.reason, canonicalId: marker.canonicalId, summary, runId: runContext?.runId, agentId: runContext?.agentId }), + runContext, + ); + const recordActivity = (this.store as typeof this.store & { + recordActivity?: (entry: { type: "task:updated"; taskId: string; taskTitle?: string; details: string; metadata?: Record }) => Promise; + }).recordActivity; + if (recordActivity) { + await recordActivity.call(this.store, { + type: "task:updated", + taskId: task.id, + taskTitle: live.title, + details: `Task marked as verified ${marker.kind}; no commits expected`, + metadata: { taskId: task.id, kind: marker.kind, reason: marker.reason, canonicalId: marker.canonicalId, summary, runId: runContext?.runId, agentId: runContext?.agentId }, + }).catch((error: unknown) => { + executorLog.warn(`${task.id}: failed to record no-op completion activity: ${error instanceof Error ? error.message : String(error)}`); + }); + } + onDone?.(); + for (let index = 0; index < live.steps.length; index += 1) { + if (live.steps[index]?.status !== "done" && live.steps[index]?.status !== "skipped") { + if (await isRejectedCloseState()) { + await restoreNoCommitsExpected(); + return { completed: false, hardPauseActive: false }; + } + await this.store.updateStep(task.id, index, "done"); + } + } + if (await isRejectedCloseState()) { + await restoreNoCommitsExpected(); + return { completed: false, hardPauseActive: false }; + } + const currentTask = await this.store.getTask(task.id); + const existingSummary = currentTask.summary?.trim(); + const hasRunWorkflowSteps = (currentTask.workflowStepResults?.length ?? 0) > 0; + const rerunSuffix = `---\nRerun after workflow step revision:\n${summary}`; + if (existingSummary && hasRunWorkflowSteps && !existingSummary.endsWith(rerunSuffix)) { + await this.store.updateTask(task.id, { summary: `${currentTask.summary}\n\n${rerunSuffix}` }); + await this.store.logEntry(task.id, "fn_task_done summary appended to existing summary (workflow-step rerun)", undefined, runContext); + } else if (!existingSummary || !hasRunWorkflowSteps) { + await this.store.updateTask(task.id, { summary }); + } + if (recommendations !== undefined) { + await this.store.updateTask(task.id, { recommendations }); + } + const settings = await this.store.getSettings(); + const hardPauseActive = Boolean(settings.globalPause); + if (await isRejectedCloseState()) { + await restoreNoCommitsExpected(); + return { completed: false, hardPauseActive: false }; + } + await this.store.updateTask(task.id, { + ...(rejectIfPaused ? {} : { paused: false, pausedByAgentId: null }), + status: null, + bulkCompletionRefusalAt: null, + }, runContext); + await this.store.logEntry(task.id, "Task marked done by agent", undefined, runContext); + const refreshed = await this.store.getTask(task.id); + if (!refreshed || refreshed.deletedAt || (await resolveTerminalColumnsFor(this.store, task.id)).includes(refreshed.column) + || (rejectIfPaused && (refreshed.paused || refreshed.userPaused))) { + await restoreNoCommitsExpected(); + return { completed: false, hardPauseActive: false }; + } + let latestColumn = refreshed.column; + if (latestColumn === await resolveReboundColumnFor(this.store, task.id)) { + const wipTarget = await resolveWipTargetForTask(this.store, task.id); + await this.store.moveTask(task.id, wipTarget); + latestColumn = wipTarget; + } + const beforeWatchdog = await this.store.getTask(task.id); + if (latestColumn === await resolveWipTargetForTask(this.store, task.id) + && !hardPauseActive + && beforeWatchdog + && !beforeWatchdog.deletedAt + && !(rejectIfPaused && (beforeWatchdog.paused || beforeWatchdog.userPaused))) { + this.scheduleCompletedTaskWatchdog(task.id, "fn_task_done"); + } + return { completed: true, hardPauseActive }; + } catch (error) { + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:24: + * `noCommitsExpected` is a completion-only exemption. A failed handoff returns to + * Plan Review, so restore its prior value rather than allowing a later approval to + * execute implementation without the normal no-commit invariant. + */ + await restoreNoCommitsExpected(); + await this.store.logEntry(task.id, `Plan Review CLOSE_NO_OP terminalization failed: ${error instanceof Error ? error.message : String(error)}`); + return { completed: false, hardPauseActive: false }; + } + } + + private async completePlanReviewNoOp( + task: TaskDetail, + marker: { kind: string; reason: string; canonicalId?: string }, + ): Promise { + const summaryPrefix = marker.kind === "premise-stale" ? "PREMISE STALE" : marker.kind.toUpperCase(); + const completion = await this.finalizeAcceptedNoOpCompletion({ + task, + marker, + summary: `${summaryPrefix}: ${marker.reason}`, + rejectIfPaused: true, + }); + return completion.completed; + } + + private async holdPlanReviewNoOpContinuation( + task: Task, + suspension: { + reason: "invalid" | "terminal-route-unavailable" | "terminalization-failed"; + nodeId: string; + fromColumn: string; + toColumn: string; + irHash: string; + }, + continuation: WorkflowWorkItem | undefined, + resolvedRunId: string | undefined, + ): Promise { + const live = await this.store.getTask(task.id).catch(() => undefined); + if (!live || live.deletedAt || (await resolveTerminalColumnsFor(this.store, task.id)).includes(live.column)) return continuation; + const blockedReason = `plan-review-close-${suspension.reason}`; + if (typeof this.store.replaceActiveTaskWorkflowContinuation === "function") { + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:37: + * A user pause wins terminal completion, but it must not discard the reviewer-close + * continuation that makes the paused card resumable. Replace the active continuation + * atomically even after observing a pause; holding it never clears pause fields or + * schedules execution, while omitting it strands durable failed close evidence. + */ + return await this.store.replaceActiveTaskWorkflowContinuation({ + runId: continuation?.runId ?? `${resolvedRunId ?? `${task.id}:workflow`}:plan-review-close:${suspension.reason}`, + taskId: task.id, + nodeId: suspension.nodeId, + kind: "task", + state: "held", + stableWorkflowRunId: continuation?.stableWorkflowRunId ?? resolvedRunId ?? `${task.id}:workflow`, + waitReason: "planning", + blockedReason, + lastError: blockedReason, + sourceColumn: suspension.fromColumn, + targetColumn: suspension.toColumn, + irHash: suspension.irHash, + }); + } + if (continuation && typeof this.store.transitionWorkflowWorkItem === "function") { + return await this.store.transitionWorkflowWorkItem(continuation.id, "held", { + leaseOwner: null, + leaseExpiresAt: null, + lastError: blockedReason, + blockedReason, + }).catch(() => continuation); + } + return continuation; + } + private async requestPreMergeOptionalStepFix( taskId: string, fallbackTask: Task, @@ -6822,6 +7038,9 @@ export class TaskExecutor { this.runGraphCustomNode(node, nodeTask, nodeSettings, columnBinding, context), resolveColumnBinding: resolveBindingForNode, }); + // Assigned from the active work item before runner.run(). The close-hold callback + // closes over this binding so it can retain the exact resumable continuation. + let continuation: WorkflowWorkItem | undefined; const runner = new WorkflowGraphTaskRunner({ localNodeId: this.options.getLocalNodeId?.(), store: { @@ -7260,6 +7479,16 @@ export class TaskExecutor { no-op when the store lacks updateTask, and swallow read/write errors (the executor wrapper also swallows) so result recording never affects the run. */ + completePlanReviewNoOp: (nodeTask, marker) => this.completePlanReviewNoOp(nodeTask, marker), + /* + FNXC:PlanReviewNoOp 2026-08-09-01:55: + Invalid, unroutable, or failed Plan Review closes are explicit waits, not graph failures. + Keep one held continuation at plan-review so scheduler resume preserves the audited close + evidence without changing the task's column or manufacturing a task error. + */ + holdPlanReviewNoOp: async (nodeTask, suspension) => { + continuation = await this.holdPlanReviewNoOpContinuation(nodeTask, suspension, continuation, resolvedRunId); + }, recordWorkflowStepResult: async (taskId: string, result: CoreWorkflowStepResult) => { if (typeof this.store.updateTask !== "function") return; try { @@ -7302,7 +7531,6 @@ export class TaskExecutor { columnBoundaryHooks: this.buildColumnBoundaryHooks(task, resolvedRunId), }); let result: WorkflowGraphTaskRunResult; - let continuation: WorkflowWorkItem | undefined; try { const loadedDetail = await this.store.getTask(task.id); /* @@ -18507,49 +18735,25 @@ export class TaskExecutor { } if (noOpMarker) { - const runContext = this.getRunContextFor(taskId); - await store.updateTask(taskId, { noCommitsExpected: true }); - await store.logEntry( - taskId, - `Verified ${noOpMarker.kind} completion sentinel accepted; no commits expected for terminal handoff`, - JSON.stringify({ - kind: noOpMarker.kind, - reason: noOpMarker.reason, - canonicalId: noOpMarker.canonicalId, - summary: params.summary, - runId: runContext?.runId, - agentId: runContext?.agentId, - }), - runContext, - ); - const recordActivity = (store as typeof store & { - recordActivity?: (entry: { - type: "task:updated"; - taskId: string; - taskTitle?: string; - details: string; - metadata?: Record; - }) => Promise; - }).recordActivity; - if (recordActivity) { - await recordActivity.call(store, { - type: "task:updated", - taskId, - taskTitle: task.title, - details: `Task marked as verified ${noOpMarker.kind}; no commits expected`, - metadata: { - taskId, - kind: noOpMarker.kind, - reason: noOpMarker.reason, - canonicalId: noOpMarker.canonicalId, - summary: params.summary, - runId: runContext?.runId, - agentId: runContext?.agentId, - }, - }).catch((error: unknown) => { - executorLog.warn(`${taskId}: failed to record no-op completion activity: ${error instanceof Error ? error.message : String(error)}`); - }); + const completion = await this.finalizeAcceptedNoOpCompletion({ + task, + marker: noOpMarker, + summary: params.summary?.trim() || `${noOpMarker.kind.toUpperCase()}: ${noOpMarker.reason}`, + recommendations: completionRecommendations, + onDone, + }); + if (!completion.completed) { + return { + content: [{ type: "text" as const, text: "Cannot mark task done because completion handoff was interrupted." }], + details: { error: "no-op-completion-interrupted" }, + }; } + const successMessage = completion.hardPauseActive + ? "Task marked complete. Completion handoff deferred until pause is cleared." + : params.summary + ? "Task marked complete with summary. All steps done. Moving to in-review." + : "Task marked complete. All steps done. Moving to in-review."; + return { content: [{ type: "text" as const, text: successMessage }], details: {} }; } onDone(); @@ -19583,13 +19787,13 @@ ${scopeGuard} } /** Parse structured JSON verdict from workflow step output. */ - private parseWorkflowStepOutput(rawOutput: string): { + private parseWorkflowStepOutput(rawOutput: string, optionalGroupId?: string): { output: string; - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; + verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "CLOSE_NO_OP"; notes?: string; malformed?: boolean; } { - return parseWorkflowStepOutput(rawOutput); + return parseWorkflowStepOutput(rawOutput, { optionalGroupId }); } private workflowInputRepliesAfterWatermark(task: TaskDetail, marker: string): Array<{ createdAt?: string }> { @@ -20287,7 +20491,9 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB session.dispose(); await agentLogger.flush(); - const parsed = requireVerdict ? parseWorkflowStepOutput(output) : parseWorkflowStepOutput(output, { requireVerdict: false }); + const parsed = requireVerdict + ? parseWorkflowStepOutput(output, { optionalGroupId }) + : parseWorkflowStepOutput(output, { requireVerdict: false, optionalGroupId }); if (parsed.verdict) { const revisionRequested = parsed.verdict === "REVISE"; if (workflowStep.requiresBrowser === true) { diff --git a/packages/engine/src/workflows/workflow-graph-executor.ts b/packages/engine/src/workflows/workflow-graph-executor.ts index 72c89d51ab..56f101e5b6 100644 --- a/packages/engine/src/workflows/workflow-graph-executor.ts +++ b/packages/engine/src/workflows/workflow-graph-executor.ts @@ -9,7 +9,7 @@ import type { WorkflowNodeExtensionResult, WorkflowStepResult, } from "@fusion/core"; -import { BUILTIN_CODING_WORKFLOW_IR, PLAN_REVIEW_GROUP_ID, WorkflowIrError, getWorkflowExtensionRegistry, instanceNodeId, resolveMaxReworkCycles, isExperimentalFeatureEnabled, GRAPH_NATIVE_POST_MERGE_FLAG, isCompletionSummaryNode, classifyReviewLease, isWorkflowOptionalGroupEnabled, isPlanReviewSatisfied } from "@fusion/core"; +import { BUILTIN_CODING_WORKFLOW_IR, PLAN_REVIEW_GROUP_ID, WorkflowIrError, computeWorkflowIrPin, getWorkflowExtensionRegistry, instanceNodeId, resolveMaxReworkCycles, isExperimentalFeatureEnabled, GRAPH_NATIVE_POST_MERGE_FLAG, isCompletionSummaryNode, classifyReviewLease, isWorkflowOptionalGroupEnabled, isPlanReviewSatisfied, parseNoOpCompletionMarker } from "@fusion/core"; import { isNonPlanDefectPlanReviewFailure } from "../errors/transient-error-detector.js"; import { isSessionContentionError } from "../errors/transient-error-patterns.js"; import { isRequiredArtifactReadFailedValue, parseRequiredArtifactMissingValue } from "../execution/required-workflow-artifacts.js"; @@ -300,6 +300,16 @@ export interface WorkflowGraphExecutorDeps { * FNXC:WorkflowRevisionBudget 2026-06-30-20:46: * Forward the optional-group id for every failure context because Plan Review/spec and Code Review budget resolution is keyed by that id. The graph does not read workflow setting values directly; live execution and self-healing share the core resolver at the remediation boundary. */ + /** Completes an accepted Plan Review close through the authoritative task lifecycle. */ + completePlanReviewNoOp?: (task: TaskDetail, marker: { kind: string; reason: string; canonicalId?: string }) => Promise | boolean; + /** Persists a resumable Plan Review hold before an invalid or unroutable close returns control. */ + holdPlanReviewNoOp?: (task: TaskDetail, suspension: { + nodeId: string; + fromColumn: string; + toColumn: string; + irHash: string; + reason: "invalid" | "terminal-route-unavailable" | "terminalization-failed"; + }) => Promise | void; requestPreMergeOptionalStepFix?: (taskId: string, info: { stepName: string; feedback: string; @@ -339,7 +349,7 @@ export interface WorkflowGraphExecutorResult { context: Record; visitedNodeIds: string[]; suspended?: { - reason: "capacity" | "pause"; + reason: "capacity" | "pause" | "hold"; nodeId: string; fromColumn: string; toColumn: string; @@ -1003,6 +1013,7 @@ export class WorkflowGraphExecutor { : undefined; const verdict = verdictRaw === "APPROVE" || verdictRaw === "APPROVE_WITH_NOTES" || verdictRaw === "REVISE" + || (node.id === PLAN_REVIEW_GROUP_ID && verdictRaw === "CLOSE_NO_OP") ? verdictRaw : undefined; let stepStatus: WorkflowStepResult["status"]; @@ -1013,6 +1024,11 @@ export class WorkflowGraphExecutor { const exitContextPatch = exitResult?.contextPatch; let stepOutput = typeof exitContextPatch?.output === "string" ? exitContextPatch.output : undefined; const stepNotes = typeof exitContextPatch?.notes === "string" ? exitContextPatch.notes : undefined; + const closeMarker = verdict === "CLOSE_NO_OP" ? parseNoOpCompletionMarker(stepNotes) : null; + if (verdict === "CLOSE_NO_OP" && !closeMarker) { + stepStatus = "failed"; + stepOutput = "Plan Review CLOSE_NO_OP requires notes beginning with a no-op completion sentinel."; + } const stepFindings = this.workflowReviewKind(node) && Array.isArray(exitContextPatch?.findings) ? exitContextPatch.findings as WorkflowStepResult["findings"] : undefined; @@ -1093,6 +1109,47 @@ export class WorkflowGraphExecutor { * edge to plan-replan (nothing about the plan is wrong) nor be labeled a provider failure * (nothing about the provider is wrong). It becomes a contention hold the executor waits out. */ + /* + * FNXC:PlanReviewNoOp 2026-08-09-01:17: + * A close is valid only with the shared leading sentinel. Invalid requests retain + * auditable failed evidence and suspend; they never enter remediation or execution. + */ + if (verdict === "CLOSE_NO_OP") { + const holdClose = async (reason: "invalid" | "terminal-route-unavailable" | "terminalization-failed"): Promise => { + const column = this.deps.columnBoundary?.currentColumn() ?? task.column; + const suspension = { + reason: "hold" as const, + nodeId: node.id, + fromColumn: column, + toColumn: column, + irHash: computeWorkflowIrPin(ir, node.id).irHash, + }; + await this.deps.holdPlanReviewNoOp?.(task, { ...suspension, reason }); + throw new WorkflowGraphSuspended(suspension); + }; + if (!closeMarker) { + context[`node:${node.id}:outcome`] = "failure"; + context[`node:${node.id}:value`] = "plan-review-close-invalid"; + return await holdClose("invalid"); + } + const hasTerminalRoute = (outgoingMap.get(node.id) ?? []).some((edge) => + edge.condition === "outcome:close-no-op" + && nodeMap.get(edge.to)?.config?.workflowAction === "plan-review-no-op", + ); + if (!hasTerminalRoute) { + await this.recordOptionalGroupStepResult(task.id, { + workflowStepId: node.id, workflowStepName: groupName, phase: stepPhase, source: "optional-group", + status: "failed", reviewKind: "plan", verdict, notes: stepNotes, + output: "Plan Review CLOSE_NO_OP terminal route unavailable.", startedAt: stepStartedAt, completedAt: new Date().toISOString(), + }); + context[`node:${node.id}:outcome`] = "failure"; + context[`node:${node.id}:value`] = "plan-review-close-route-unavailable"; + return await holdClose("terminal-route-unavailable"); + } + context.noOpMarker = closeMarker; + context.noOpCloseNotes = stepNotes; + return await traverseChildren(node, { outcome: "success", value: "close-no-op" }); + } const sessionContentionFailure = stepStatus === "failed" && ( @@ -1217,6 +1274,48 @@ export class WorkflowGraphExecutor { return { outcome: "success", value: "remediation-scheduled" }; } + if (workflowAction === "plan-review-no-op") { + const marker = context.noOpMarker as { kind?: unknown; reason?: unknown; canonicalId?: unknown } | undefined; + if (!marker || typeof marker.kind !== "string" || typeof marker.reason !== "string") { + return { outcome: "failure", value: "plan-review-close-marker-missing" }; + } + const completed = await this.deps.completePlanReviewNoOp?.(task, { + kind: marker.kind, + reason: marker.reason, + ...(typeof marker.canonicalId === "string" ? { canonicalId: marker.canonicalId } : {}), + }); + if (completed) return await traverseChildren(node, { outcome: "success", value: "close-no-op-completed" }); + /* + * FNXC:PlanReviewNoOp 2026-08-09-02:24: + * A lifecycle handoff failure must replace the optimistic passed close evidence. + * Leaving that result passed would let a held, non-terminal task look completed even + * though its accepted no-op mutation never committed. + */ + await this.recordOptionalGroupStepResult(task.id, { + workflowStepId: PLAN_REVIEW_GROUP_ID, + workflowStepName: "Plan Review", + phase: "pre-merge", + source: "optional-group", + status: "failed", + reviewKind: "plan", + verdict: "CLOSE_NO_OP", + notes: typeof context.noOpCloseNotes === "string" ? context.noOpCloseNotes : marker.reason, + output: "Plan Review CLOSE_NO_OP terminalization failed.", + startedAt: new Date().toISOString(), + completedAt: new Date().toISOString(), + }); + const column = this.deps.columnBoundary?.currentColumn() ?? task.column; + const suspension = { + reason: "hold" as const, + nodeId: PLAN_REVIEW_GROUP_ID, + fromColumn: column, + toColumn: column, + irHash: computeWorkflowIrPin(ir, PLAN_REVIEW_GROUP_ID).irHash, + }; + await this.deps.holdPlanReviewNoOp?.(task, { ...suspension, reason: "terminalization-failed" }); + throw new WorkflowGraphSuspended(suspension); + } + const result = await this.executeNodeWithRetries(node, task, settings, context, ir, this.deps.signal); if (result.contextPatch) Object.assign(context, result.contextPatch); context[`node:${node.id}:outcome`] = result.outcome; diff --git a/packages/engine/src/workflows/workflow-graph-task-runner.ts b/packages/engine/src/workflows/workflow-graph-task-runner.ts index 0ef6e15202..08ffa4d9c1 100644 --- a/packages/engine/src/workflows/workflow-graph-task-runner.ts +++ b/packages/engine/src/workflows/workflow-graph-task-runner.ts @@ -141,6 +141,10 @@ export interface WorkflowGraphTaskRunnerDeps { * node's outcome into `task.workflowStepResults` keyed by node id. Additive; * absent → graph records nothing (disabled groups + unwired stores byte-inert). */ recordWorkflowStepResult?: (taskId: string, result: WorkflowStepResult) => void | Promise; + /** Completes an accepted Plan Review close through the authoritative task lifecycle. */ + completePlanReviewNoOp?: WorkflowGraphExecutorDeps["completePlanReviewNoOp"]; + /** Persists the resumable Plan Review hold for a rejected close request. */ + holdPlanReviewNoOp?: WorkflowGraphExecutorDeps["holdPlanReviewNoOp"]; /** Enabled pre-merge optional-step REVISE remediation seam. Additive; absent preserves prior graph traversal. */ requestPreMergeOptionalStepFix?: WorkflowGraphExecutorDeps["requestPreMergeOptionalStepFix"]; /** Project node-published task metadata onto the task row for dispatcher/UI. */ @@ -411,6 +415,8 @@ export class WorkflowGraphTaskRunner { resumeReconcile: this.deps.resumeReconcile, logTaskEntry: this.deps.logTaskEntry, recordWorkflowStepResult: this.deps.recordWorkflowStepResult, + completePlanReviewNoOp: this.deps.completePlanReviewNoOp, + holdPlanReviewNoOp: this.deps.holdPlanReviewNoOp, requestPreMergeOptionalStepFix: this.deps.requestPreMergeOptionalStepFix, publishTaskProjection: this.deps.publishTaskProjection, signal: this.deps.signal,