diff --git a/.changeset/review-lane-three-milestones.md b/.changeset/review-lane-three-milestones.md new file mode 100644 index 0000000000..cdb767110d --- /dev/null +++ b/.changeset/review-lane-three-milestones.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Coding (Ideas) V2 review lane is now Code Review, Documentation, then merge. +category: feature +dev: Removes the separate deterministic `verification` optional group and the `completion-summary` node from `builtin:coding-ideas-v2`. Code Review runs lint/test/build itself via an appended prompt section (the shared reviewer prompt is untouched, so `builtin:coding` and `builtin:coding-ideas` keep their reviewer) and must quote command output as verdict evidence. Documentation moves after the review, becomes `gateMode: "advisory"` and `toolMode: "readonly"`, no longer writes repository files, and absorbs the card summary via `fn_task_done(summary=...)`. Repository documentation is the executor's judgement during implementation, where it is reviewed with the code it documents. Net effect: two fewer model calls per card and one blocking gate instead of four. diff --git a/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts b/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts index f1ee19a95e..b9db6bc334 100644 --- a/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts +++ b/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts @@ -45,103 +45,100 @@ describe("builtin:coding-ideas-v2", () => { between the review and the merge invalidates FN-180's review-diff fingerprint: "task has no provable approval for the content being merged". Measured in pipeline-smoke S01. */ - it("runs verify -> document -> summarize -> review -> merge in review", () => { + /* + FNXC:CodingIdeasV2Workflow 2026-08-25-10:20: + In-review is THREE milestones: Code Review -> Documentation -> Delivery (the merge nodes). + The previous shape ran a separate deterministic `verification` gate and a `completion-summary` + node. Both are gone: Code Review runs the commands itself so one node owns the verdict, and + Documentation writes the card summary in the same pass as the delivery note. + */ + it("runs review -> document -> merge in review", () => { expect(successChainFrom("steps")).toEqual([ - "verification", - "documentation-delivery", - "completion-summary", "code-review", + "documentation-delivery", "merge-gate", ]); - for (const nodeId of ["verification", "documentation-delivery", "code-review", "completion-summary"]) { + for (const nodeId of ["code-review", "documentation-delivery"]) { expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === nodeId)?.column) .toBe("in-review"); } + for (const removed of ["verification", "verification-remediation", "completion-summary"]) { + expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.some((node) => node.id === removed), `${removed} must be gone`).toBe(false); + expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.edges.some((edge) => edge.from === removed || edge.to === removed)).toBe(false); + } - // Coding (Ideas) carries no post-merge-verification node, so V2 inherits none either. expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR).map((step) => step.templateId)) - .toEqual(["plan-review", "verification", "documentation-delivery", "code-review"]); - expect(resolveRequiredPreMergeStepIds(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR, undefined)) - .toEqual(new Set(["plan-review", "verification", "documentation-delivery", "code-review"])); + .toEqual(["plan-review", "code-review", "documentation-delivery"]); }); /* - FNXC:CodingIdeasV2Workflow 2026-08-24-05:35: - Both remediation loops must re-enter at `verification`, never at `code-review`. A REVISE has to - replay documentation-delivery so the docs and changeset are regenerated to include what the review - demanded; re-entering at the review would merge documentation describing a superseded tree. + FNXC:CodingIdeasV2Workflow 2026-08-25-10:20: + Code Review is the ONLY gate that can hold a card, so its verdict must rest on commands it ran + itself. The prompt is augmented rather than edited so the shared reviewer used by builtin:coding + and builtin:coding-ideas is untouched; the evidence rule is what stops a reviewer asserting "tests + pass" in prose, which is the same false green a silently-passing gate produced mechanically. */ - /* - FNXC:ReviewGatedRemediation 2026-08-24-20:10: - Verification rework re-enters `verification` — a failing test needs re-running, and the doc node - downstream is replayed with it. Code Review rework returns to `code-review`, as the inherited graph - does: its remediation node is a coding session that completes the trailing steps the REVISE - reopened, and routing it through `verification` walked past the foreach so the reopened step was - never re-executed and the merge boundary refused with `merge-boundary-unproven` (measured on S05). - Cost, stated rather than hidden: a Code Review REVISE does not regenerate the documentation. - */ - it("routes each rework to the stage that can actually redo the work", () => { - expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.edges).toEqual(expect.arrayContaining([ - { from: "verification", to: "verification-remediation", condition: "failure" }, - { from: "code-review", to: "code-review-remediation", condition: "failure" }, - { from: "verification-remediation", to: "verification", condition: "success", kind: "rework" }, - { from: "code-review-remediation", to: "code-review", condition: "success", kind: "rework" }, - ])); - // Verification rework replays the docs, because the doc node sits downstream of it. - expect(successChainFrom("verification")).toContain("documentation-delivery"); + it("makes Code Review run the checks and forbids a verdict without execution evidence", () => { + const template = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "code-review")?.config?.template as + { nodes?: Array<{ id: string; config?: { prompt?: string } }> } | undefined; + const prompt = template?.nodes?.find((node) => node.id === "code-review-step")?.config?.prompt ?? ""; - for (const remediationId of ["verification-remediation", "code-review-remediation"]) { - expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === remediationId)?.column) - .toBe("in-progress"); + expect(prompt).toContain("fn_run_verification"); + expect(prompt).toContain("NEVER claim a check passed without its output"); + expect(prompt).toContain("A non-zero exit is REVISE"); + // Absent commands are reported, never treated as a failure: that was never the merge contract. + expect(prompt).toContain("do not treat the absence as failure"); + + // The shared reviewer keeps its original prompt for the other built-ins. + const inherited = BUILTIN_CODING_IDEAS_WORKFLOW_IR.nodes.find((node) => node.id === "code-review")?.config?.template as + { nodes?: Array<{ id: string; config?: { prompt?: string } }> } | undefined; + expect(inherited?.nodes?.find((node) => node.id === "code-review-step")?.config?.prompt ?? "") + .not.toContain("fn_run_verification"); + }); + + /* + FNXC:DocumentationMilestone 2026-08-25-10:20: + Documentation REPORTS: it never vetoes and never writes the repository. Both properties are load + bearing. As a blocking gate it bounced a task whose own plan forbade implementing anything, and the + card then looped through the review lane every five minutes indefinitely. As a repository writer it + had to be forced ahead of the review, because content changing after approval is exactly what the + review seal refuses. + */ + it("makes Documentation advisory and repository-read-only, after the review", () => { + const template = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "documentation-delivery")?.config?.template as + { nodes?: Array<{ id: string; config?: Record }> } | undefined; + const config = template?.nodes?.find((node) => node.id === "documentation-delivery-step")?.config ?? {}; + + expect(config.gateMode).toBe("advisory"); + expect(config.toolMode).toBe("readonly"); + expect(String(config.prompt)).toContain("Do NOT modify repository files"); + // It absorbs the former completion-summary milestone. + expect(String(config.prompt)).toContain("fn_task_done(summary="); + + // Failure reaches the merge exactly like success: a delivery note cannot strand approved code. + for (const condition of ["success", "failure"]) { + expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.edges).toContainEqual( + { from: "documentation-delivery", to: "merge-gate", condition }, + ); } }); - /* - FNXC:ReviewGatedRemediation 2026-08-24-14:40: - BOTH gates must derive named remediation steps. `review-remediation-steps` appends numbered work - to `task.steps` from the reviewer's findings and widens the PROMPT.md File Scope, so a bounced card - arrives in in-progress with visible work; the inherited `pre-merge-remediation` returns it with an - unchanged checklist. Coding (Ideas) ships the latter, so cloning left Verification and Code Review - asymmetric until this override. - */ - /* - FNXC:ReviewGatedRemediation 2026-08-24-18:30: - Both gates must append NAMED remediation steps, because this workflow also sets the parse node's - `implementationOnlySteps` + `preserveRemediationSteps`, which `resolveStepReopenPolicy` reads as - reopen policy "none". The two are a matched pair: with trailing-step reopening disabled, a - remediation that appends nothing returns the card to in-progress with every step already done and - nothing left to execute. Inheriting Coding (Ideas)' `pre-merge-remediation` stalled the card after - a Code Review REVISE. - */ - /* - FNXC:ReviewGatedRemediation 2026-08-24-20:10: - Named remediation (`review-remediation-steps`) is UNAVAILABLE to a foreach-executed workflow: the - parse node preserves an appended step and then answers `already-expanded`, because the foreach is - pinned to the list it first expanded, so that step never receives an instance and stays `pending` - forever. Code Review therefore keeps the inherited `pre-merge-remediation`, which reopens trailing - steps the foreach already owns. Change this only together with a foreach that can re-expand. - */ - /* - FNXC:ReviewGatedRemediation 2026-08-24-22:10: - A rejected review appends NAMED work derived from its findings, so a bounced card arrives in - in-progress showing exactly what must be fixed. This depends on the foreach covering steps - appended after expansion (`FNXC:WorkflowForeachGrowth`); with the count pinned, an appended step - never received an instance and stayed `pending` forever. - */ - it("appends named remediation steps for both review gates", () => { + it("returns a rejected review to in-progress as named work", () => { + expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.edges).toEqual(expect.arrayContaining([ + { from: "code-review", to: "code-review-remediation", condition: "failure" }, + { from: "code-review-remediation", to: "code-review", condition: "success", kind: "rework" }, + ])); + expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "code-review-remediation")?.column) + .toBe("in-progress"); + const parse = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "parse")?.config; expect(parse?.preserveRemediationSteps).toBe(true); expect(parse?.implementationOnlySteps).toBe(true); - for (const [remediationId, gateId] of [ - ["verification-remediation", "verification"], - ["code-review-remediation", "code-review"], - ]) { - const config = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === remediationId)?.config; - expect(config?.workflowAction, `${remediationId} must append named steps`).toBe("review-remediation-steps"); - expect(config?.forWorkflowStepId).toBe(gateId); - } + const config = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "code-review-remediation")?.config; + expect(config?.workflowAction, "code-review-remediation must append named steps").toBe("review-remediation-steps"); + expect(config?.forWorkflowStepId).toBe("code-review"); // The inherited workflow reopens trailing steps instead, and must stay that way. expect(BUILTIN_CODING_IDEAS_WORKFLOW_IR.nodes.find((node) => node.id === "code-review-remediation")?.config?.workflowAction) diff --git a/packages/core/src/__tests__/builtin-review-gated-coding-workflow.test.ts b/packages/core/src/__tests__/builtin-review-gated-coding-workflow.test.ts index 6708b2ed7f..92360acbe2 100644 --- a/packages/core/src/__tests__/builtin-review-gated-coding-workflow.test.ts +++ b/packages/core/src/__tests__/builtin-review-gated-coding-workflow.test.ts @@ -17,7 +17,7 @@ describe("builtin:review-gated-coding", () => { { templateId: "plan-review", name: "Plan Review", description: "", phase: "pre-merge", defaultOn: true }, { templateId: "verification", name: "Verification", description: "", phase: "pre-merge", defaultOn: true }, { templateId: "code-review", name: "Code Review", description: "", phase: "pre-merge", defaultOn: true }, - { templateId: "documentation-delivery", name: "Documentation & Delivery", description: "", phase: "pre-merge", defaultOn: true }, + { templateId: "documentation-delivery", name: "Documentation", description: "", phase: "pre-merge", defaultOn: true }, { templateId: "post-merge-verification", name: "Post-merge verification", description: "", phase: "post-merge", defaultOn: false }, ]); expect(resolveRequiredPreMergeStepIds(BUILTIN_REVIEW_GATED_CODING_WORKFLOW_IR, undefined)) diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 511d49aecd..005f80ea0b 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -237,14 +237,36 @@ describe("built-in workflows", () => { } }); - it("all built-in workflows generate a task completion summary as a graph node", () => { + /* + FNXC:WorkflowCompletion 2026-08-25-10:20: + The invariant is that every built-in PRODUCES a card summary, not that it owns a node called + `completion-summary`. `builtin:coding-ideas-v2` folds the summary into its Documentation milestone, + writing it in the same pass as the delivery note and saving a model call per card. Pinning the node + id would have forced a second read-only session that exists only to satisfy a test. + What must NOT weaken: a workflow with neither a summary node nor a summary-writing milestone still + fails here, so a future built-in cannot ship with no card summary at all. + */ + it("all built-in workflows produce a task completion summary", () => { for (const workflow of BUILTIN_WORKFLOWS) { if (workflow.kind === "fragment") continue; const summaryNodes = workflow.ir.nodes.filter((node) => node.id === "completion-summary"); - expect(summaryNodes, workflow.id).toHaveLength(1); - expect(summaryNodes[0]?.kind, workflow.id).toBe("prompt"); - expect(summaryNodes[0]?.config?.summaryTarget, workflow.id).toBe("task"); - expect(summaryNodes[0]?.config?.toolMode, workflow.id).toBe("readonly"); + if (summaryNodes.length > 0) { + expect(summaryNodes, workflow.id).toHaveLength(1); + expect(summaryNodes[0]?.kind, workflow.id).toBe("prompt"); + expect(summaryNodes[0]?.config?.summaryTarget, workflow.id).toBe("task"); + expect(summaryNodes[0]?.config?.toolMode, workflow.id).toBe("readonly"); + continue; + } + + const writesSummary = workflow.ir.nodes.some((node) => { + const template = node.config?.template as { nodes?: Array<{ config?: { prompt?: string } }> } | undefined; + const prompts = [ + typeof node.config?.prompt === "string" ? node.config.prompt : "", + ...(template?.nodes ?? []).map((child) => child.config?.prompt ?? ""), + ]; + return prompts.some((prompt) => prompt.includes("fn_task_done(summary=")); + }); + expect(writesSummary, `${workflow.id} has no completion-summary node and no milestone that writes the card summary`).toBe(true); } }); diff --git a/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts b/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts index 1e33732c28..087700798b 100644 --- a/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts +++ b/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts @@ -1,9 +1,9 @@ import type { WorkflowIr } from "./workflow-ir-types.js"; import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_CODING_IDEAS_WORKFLOW_IR } from "./builtin-coding-ideas-workflow-ir.js"; -import { verificationOptionalGroupNode } from "./builtin-verification-gate-group.js"; + import { documentationDeliveryOptionalGroupNode } from "./builtin-documentation-delivery-group.js"; -import { codeReviewRemediationStepsNode, verificationRemediationNode } from "./builtin-workflow-remediation-nodes.js"; +import { codeReviewRemediationStepsNode } from "./builtin-workflow-remediation-nodes.js"; import { builtinPromptConfig, builtinSeamPrompt } from "./builtin-workflow-prompts.js"; import { applyImplementationOnlyStepReview } from "./builtin-plan-review-group.js"; @@ -94,10 +94,58 @@ const RAW_BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR: WorkflowIr = (() => { The planner is still constrained — that is the SEAM PROMPT's job, not this flag, which only audits. */ + /* + FNXC:CodingIdeasV2Workflow 2026-08-25-10:20: + In-review is THREE milestones: Code Review -> Documentation -> Delivery (the merge). + + A separate deterministic `Verification` gate is deliberately GONE. It duplicated the executor's + own verification, it produced a green badge on projects that had configured no command, and it + split the merge evidence across two authorities that could disagree. Code Review now RUNS the + commands itself and rules on their real output, so exit codes still decide and one node owns the + verdict. + + `completion-summary` is gone as a milestone too: the card summary is written by Documentation in + the same pass as the delivery note, which removes one model call per card. + + Documentation runs AFTER the review — the ordering the original documentation-delivery node always + intended ("runs after passing verification and code review") and which the review seal previously + forbade. It is legal now because Documentation no longer writes the repository: it records a + Fusion-side delivery note, artifacts, follow-ups, and the card summary. Repository documentation + is the EXECUTOR's call during implementation, where it is reviewed with the code it documents. + */ const codeReviewIndex = ir.nodes.findIndex((node) => node.id === "code-review"); if (codeReviewIndex < 0) throw new Error("coding-ideas-v2 requires the inherited code-review gate"); - ir.nodes.splice(codeReviewIndex, 0, verificationOptionalGroupNode("in-review"), documentationDeliveryOptionalGroupNode("in-review")); - ir.nodes.push(verificationRemediationNode()); + /* + FNXC:CodingIdeasV2Workflow 2026-08-25-10:20: + Code Review RUNS the checks here; it does not receive someone else's verdict. The shared prompt is + AUGMENTED rather than edited, so `builtin:coding` and `builtin:coding-ideas` keep the reviewer they + have always had. + The evidence requirement is the whole point. A reviewer that may state "tests pass" without + running anything reproduces, in prose, the false green a silently-passing gate produced + mechanically — and a fluent claim is harder to spot than a 46ms step. Absent commands report that + fact instead of blocking: a project that never configured verification has never been refused a + merge on that basis, and this is not the place to change that contract. + */ + const codeReviewStep = (ir.nodes[codeReviewIndex]?.config?.template as { nodes?: Array<{ id: string; config?: Record }> } | undefined) + ?.nodes?.find((node) => node.id === "code-review-step"); + if (!codeReviewStep?.config) throw new Error("coding-ideas-v2 requires the inherited code-review-step template node"); + codeReviewStep.config.prompt = `${String(codeReviewStep.config.prompt ?? "")} + +## Step 0: Run the checks yourself (do this FIRST) + +This review is the only gate before merge, so the verdict must rest on real command output. + +1. Determine the project's lint, test, and build commands. Prefer explicitly configured commands; otherwise infer them from the repository (package scripts, Makefile, CI config). +2. Run them with \`fn_run_verification\`, scoped to what the change touches. Do NOT run a full workspace suite as your normal path. +3. Quote, in your review, each command you ran with its exit code and the tail of its output. + +Rules that are not negotiable: +- A non-zero exit is REVISE. State which command failed and the failing output. +- NEVER claim a check passed without its output in your review. A verdict with no execution evidence is invalid. +- If no command is determinable, say so explicitly ("no lint/test/build command could be determined") and review the diff on its merits. Do not invent a command, and do not treat the absence as failure.`; + ir.nodes.splice(codeReviewIndex + 1, 0, documentationDeliveryOptionalGroupNode("in-review")); + const summaryIndex = ir.nodes.findIndex((node) => node.id === "completion-summary"); + if (summaryIndex >= 0) ir.nodes.splice(summaryIndex, 1); /* FNXC:ReviewGatedRemediation 2026-08-24-18:30: Both gates MUST derive named remediation steps, because this workflow also sets the parse node's @@ -116,9 +164,8 @@ const RAW_BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR: WorkflowIr = (() => { /* Code Review keeps the inherited `pre-merge-remediation`, which reopens trailing steps the foreach already owns. See the parse-node note above. */ - ir.edges = ir.edges.filter((edge) => !( - (edge.from === "steps" && edge.to === "completion-summary") - )); + /* Every inherited edge touching `completion-summary` dies with the node; the lane is rebuilt below. */ + ir.edges = ir.edges.filter((edge) => edge.from !== "completion-summary" && edge.to !== "completion-summary"); /* FNXC:CodingIdeasV2Workflow 2026-08-24-05:35: @@ -136,12 +183,21 @@ const RAW_BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR: WorkflowIr = (() => { graph carried each of them twice — a duplicated success edge out of a review gate is a second, competing traversal of the same lane. */ + /* `code-review -> merge-gate` is inherited and must not survive: Documentation now sits between them. */ + ir.edges = ir.edges.filter((edge) => !(edge.from === "code-review" && edge.to === "merge-gate")); ir.edges.push( - { from: "steps", to: "verification", condition: "success" }, - { from: "verification", to: "documentation-delivery", condition: "success" }, - { from: "documentation-delivery", to: "completion-summary", condition: "success" }, - { from: "verification", to: "verification-remediation", condition: "failure" }, - { from: "verification-remediation", to: "verification", condition: "success", kind: "rework" }, + { from: "steps", to: "code-review", condition: "success" }, + { from: "code-review", to: "documentation-delivery", condition: "success" }, + { from: "documentation-delivery", to: "merge-gate", condition: "success" }, + /* + FNXC:CodingIdeasV2Workflow 2026-08-25-10:20: + Documentation is ADVISORY: it reports, it never vetoes. Its failure edge reaches the merge gate + exactly like its success edge, so a delivery note that could not be written cannot strand a + card whose code is already approved. Measured why: as a blocking gate it bounced a task whose + own plan said not to implement anything ("No task-specific implementation is present"), and the + card looped through the review lane every five minutes indefinitely. + */ + { from: "documentation-delivery", to: "merge-gate", condition: "failure" }, ); /* FNXC:ReviewGatedRemediation 2026-08-24-20:10: diff --git a/packages/core/src/workflows/builtin-documentation-delivery-group.ts b/packages/core/src/workflows/builtin-documentation-delivery-group.ts index 0329094ebe..493c98405a 100644 --- a/packages/core/src/workflows/builtin-documentation-delivery-group.ts +++ b/packages/core/src/workflows/builtin-documentation-delivery-group.ts @@ -2,26 +2,45 @@ import type { WorkflowIrNode } from "./workflow-ir-types.js"; export const DOCUMENTATION_DELIVERY_GROUP_ID = "documentation-delivery"; -const DOCUMENTATION_DELIVERY_PROMPT = `Document and deliver the accepted implementation exactly once. Update relevant operator documentation, save a concise delivery note with fn_task_document_write(key="docs", ...), register visual or media deliverables with fn_artifact_register when present, and record only genuine out-of-scope follow-ups.`; +/* +FNXC:DocumentationMilestone 2026-08-25-10:20: +This milestone reports on accepted work; it does NOT write the repository and does NOT judge. + +Repository documentation belongs to the EXECUTOR during implementation: a docs change is a code +change, and writing it here put it outside the diff the reviewer approved — the exact content-drift +the review seal exists to prevent, which is why this node used to be forced ahead of the review it +was authored to follow. Whether the change warrants a docs update is the executor's judgement, not a +mandatory stage. + +It also absorbs the former `completion-summary` milestone: the card summary and the delivery note are +one pass, one model call. +*/ +const DOCUMENTATION_DELIVERY_PROMPT = `Report on the accepted implementation exactly once. Do NOT modify repository files: this milestone records, it does not implement, and the code has already been reviewed. + +1. Write the card summary with fn_task_done(summary=...): 2-4 sentences an operator can read on the card to know what shipped and why. +2. Save a concise delivery note with fn_task_document_write(key="docs", ...): what shipped, how to verify it, and anything an operator must know. +3. Register visual or media deliverables with fn_artifact_register when the work produced any. +4. Record genuine out-of-scope follow-ups as new tasks. Only real ones -- do not invent work. + +You cannot block this card. If you cannot complete a point above, say so plainly in the summary and finish.`; -/** Documentation runs after passing verification and code review, never as an implementation step. */ export function documentationDeliveryOptionalGroupNode(column: string): WorkflowIrNode { return { id: DOCUMENTATION_DELIVERY_GROUP_ID, kind: "optional-group", column, config: { - name: "Documentation & Delivery", + name: "Documentation", defaultOn: true, template: { nodes: [{ id: "documentation-delivery-step", kind: "prompt", config: { - name: "Documentation & Delivery", + name: "Documentation", prompt: DOCUMENTATION_DELIVERY_PROMPT, - toolMode: "coding", - gateMode: "gate", + toolMode: "readonly", + gateMode: "advisory", workflowAction: "documentation-delivery", }, }], diff --git a/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts b/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts index 5d68d24aab..93cf738fc2 100644 --- a/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts +++ b/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts @@ -57,40 +57,47 @@ describe("builtin:coding-ideas-v2 review seal", () => { expect(offenders).toEqual([]); }); - it("keeps both write-capable gates strictly before Code Review", () => { + /* + FNXC:WorkflowReviewSeal 2026-08-25-10:20: + The seal invariant became STRONGER, not weaker: the review lane now contains NO write-capable node + at all, so there is nothing to order against Code Review. + Previously two gates ran in in-review and had to be forced ahead of the review: a deterministic + `verification` gate (classified write-capable only because the old predicate matched its display + NAME), and `documentation-delivery`, which wrote repository docs. Verification is gone — Code + Review runs the commands itself — and Documentation no longer touches the repository, so it may + legally follow the review, which is the ordering its own author always intended. + Assert the PREMISE too: an empty review lane would make this vacuously true. + */ + it("runs no write-capable node anywhere in the review lane", () => { + const reviewLane = ir.nodes.filter((node) => node.column === "in-review"); + expect(reviewLane.map((node) => node.id)).toEqual(expect.arrayContaining([ + "code-review", "documentation-delivery", "merge-gate", + ])); + /* - FNXC:WorkflowReviewSeal 2026-08-25-02:10: - Assert the PREMISE, not just the outcome, or this ratchet measures nothing. - `documentation-delivery` is genuinely write-capable (`toolMode: "coding"`) and is what the seal - exists to order. `verification` is NOT: it is a deterministic gate that runs commands and reads - exit codes, and it was only ever classified write-capable because the old predicate matched its - display NAME. It still runs before the review — not for the seal, but because anything between - the review and the merge invalidates the review-diff fingerprint. + `code-review` is itself write-capable and must stay so: the reviewer fixes findings inline, which + happens BEFORE it issues its own verdict, so it cannot invalidate an approval that does not yet + exist. The seal governs what runs AFTER the sealer — which is why it is the one exclusion here, + and why the exclusion is named rather than filtered away silently. */ + const offenders = reviewLane.filter((node) => node.id !== "code-review" && isWriteCapable(node)).map((node) => node.id); + expect(offenders, "no node other than the reviewer may write in the review lane").toEqual([]); + expect(isWriteCapable(ir.nodes.find((node) => node.id === "code-review")!), "the reviewer keeps its inline-fix capability").toBe(true); + }); + + /* + FNXC:WorkflowReviewSeal 2026-08-25-10:20: + Documentation may follow the review ONLY because it cannot write the repository. If someone + restores `toolMode: "coding"` here, the card silently regains the FN-175 shape: content mutating + after the approval that is supposed to cover it. This is the assertion that catches that. + */ + it("lets Documentation follow the review because it cannot write the repository", () => { const documentation = ir.nodes.find((node) => node.id === "documentation-delivery"); expect(documentation, "documentation-delivery is missing").toBeDefined(); - expect(isWriteCapable(documentation!), "documentation-delivery must be write-capable").toBe(true); - - const verification = ir.nodes.find((node) => node.id === "verification"); - expect(verification, "verification is missing").toBeDefined(); - expect(isWriteCapable(verification!), "a deterministic gate must not be classified write-capable").toBe(false); + expect(isWriteCapable(documentation!), "Documentation must not be write-capable if it runs after the review").toBe(false); const chain = successChainFrom(ir, "steps").map((node) => node.id); - expect(chain.indexOf("verification")).toBeLessThan(chain.indexOf("code-review")); - expect(chain.indexOf("documentation-delivery")).toBeLessThan(chain.indexOf("code-review")); - /* - Not seal-driven but merge-driven: `completion-summary` escapes the write-capable classifier - (readonly) yet still acquires a worktree, and any node between the review and the merge - invalidates FN-180's review-diff fingerprint ("no provable approval for the content being - merged"). Nothing may sit between them. - */ - expect(chain.indexOf("completion-summary")).toBeLessThan(chain.indexOf("code-review")); - expect(chain[chain.indexOf("code-review") + 1]).toBe("merge-gate"); - }); - - it("keeps the completion summary readonly so it may run after the seal", () => { - const summary = ir.nodes.find((node) => node.id === "completion-summary"); - expect(summary?.config?.toolMode).toBe("readonly"); - expect(isWriteCapable(summary!)).toBe(false); + expect(chain.indexOf("code-review")).toBeLessThan(chain.indexOf("documentation-delivery")); + expect(chain[chain.indexOf("documentation-delivery") + 1]).toBe("merge-gate"); }); });