feat(FN-WF): make the V2 review lane Code Review -> Documentation -> merge
One gate that can hold a card, one milestone that reports, then the merge.
REMOVED — the separate deterministic `verification` group. It duplicated the
executor's own verification, it showed a green badge on projects that had
configured no command, and it split merge evidence across two authorities that
could disagree. Code Review now runs lint/test/build itself, so exit codes still
decide and a single node owns the verdict. Its prompt is APPENDED to rather than
edited, leaving the shared reviewer used by builtin:coding and
builtin:coding-ideas exactly as it was.
The evidence rule is the point: the reviewer must quote each command with its exit
code and output tail, and a verdict with no execution evidence is invalid. A
reviewer free to assert "tests pass" in prose reproduces the false green a silently
passing gate produced mechanically — and the fluent version is harder to spot.
Absent commands are reported, never treated as failure: a project that never
configured verification has never been refused a merge on that basis.
REMOVED — `completion-summary` as its own milestone. Documentation writes the card
summary in the same pass as the delivery note. One model call, not two.
CHANGED — Documentation now runs AFTER the review, which is the ordering its own
author intended ("runs after passing verification and code review") and which the
review seal previously forbade. It is legal because it no longer writes the
repository: it is advisory, read-only, and records a Fusion-side delivery note,
artifacts, follow-ups and the summary. Repository documentation belongs to the
executor during implementation — a docs change is a code change, and writing it
after approval put it outside the diff the reviewer signed off.
It also cannot veto any more. As a blocking gate it bounced a task whose own plan
forbade implementing anything, and that card looped through the review lane every
five minutes indefinitely.
The seal invariant got STRONGER, not weaker: no node other than the reviewer itself
writes anywhere in the review lane, so nothing can change after an approval. The
test asserts exactly that, and names the reviewer exclusion rather than filtering it
away silently.
pnpm lint 0 errors, test:gate, verify:fast, engine-pipeline-smoke 90/90, and three
consecutive full runs: 137.4s, 141.0s, 144.6s of the 175s budget. The 3 remaining
core failures are pre-existing and reproduce without this diff.
This commit is contained in:
Fusion Agent
7
.changeset/review-lane-three-milestones.md
Normal file
7
.changeset/review-lane-three-milestones.md
Normal file
@@ -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.
|
||||
@@ -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<string, unknown> }> } | 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)
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@@ -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<string, unknown> }> } | 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:
|
||||
|
||||
@@ -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",
|
||||
},
|
||||
}],
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user