From 9e76393cfa610629dbbe3ca291f0ed34e90b8cf0 Mon Sep 17 00:00:00 2001 From: Fusion Agent Date: Mon, 24 Aug 2026 10:07:22 +0000 Subject: [PATCH] fix(FN-WF): make review-column workflows actually merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A required pre-merge step is not necessarily a content review. Review-column workflows also require a deterministic verification gate (exit codes) and a documentation/delivery gate; neither records a `reviewInputFingerprint` because neither binds a diff. `evaluatePreMergeApprovals` compared them against the merge content anyway, classified both as `unprovable-content`, and `canMergeTask` answered "task has no provable approval for the content being merged" — an unsatisfiable gate, so NOTHING could ever merge on such a workflow. Cards reached the merge, were refused, and looped through verification-remediation. The carve-out is narrow: a step that is neither `code-review` nor a `reviewKind: "code"` result AND recorded no fingerprint of its own is not diff-bound and passes on its status. A content review that DID record a fingerprint is still compared, and a code review missing one is still refused, so FN-180's guarantee is untouched. Reverting the carve-out fails the new tests. builtin:review-gated-coding carried the identical latent defect and never reached its merge to expose it. Proven end to end: pipeline-smoke now drives S01 on builtin:coding-ideas-v2 from the Ideas intake through promotion, planning, plan review, implementation, verification, documentation, summary and code review to `merged-done` — 63 tests, 19/19 scenarios, 74.7s against the 90s budget. S01 keeps that workflow permanently, because all five defects fixed in this effort passed structural review and only a real card reaching `merged-done` exposed them. --- ...re-merge-approval-non-review-gates.test.ts | 90 +++++++++++++++++++ packages/core/src/merge/pre-merge-approval.ts | 17 ++++ .../pipeline-smoke/_pipeline-harness.ts | 2 +- .../pipeline-smoke/_pipeline-scenarios.ts | 13 ++- 4 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/__tests__/pre-merge-approval-non-review-gates.test.ts diff --git a/packages/core/src/__tests__/pre-merge-approval-non-review-gates.test.ts b/packages/core/src/__tests__/pre-merge-approval-non-review-gates.test.ts new file mode 100644 index 0000000000..8139482b45 --- /dev/null +++ b/packages/core/src/__tests__/pre-merge-approval-non-review-gates.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from "vitest"; +import { evaluatePreMergeApprovals } from "../merge/pre-merge-approval.js"; +import type { Task, WorkflowStepResult } from "../types.js"; + +/* +FNXC:PreMergeApproval 2026-08-24-07:10: +Measured failure this guards: on builtin:coding-ideas-v2 every card reached the merge and was +refused with "task has no provable approval for the content being merged", then looped. The required +pre-merge set for a review-column workflow is +`plan-review, verification, documentation-delivery, code-review`, but only a CONTENT REVIEW records +a `reviewInputFingerprint`. The deterministic verification gate (exit codes) and the +documentation/delivery gate have no diff to bind, so they fell through to the fingerprint comparison +and were classified `unprovable-content` — an unsatisfiable merge gate. +builtin:review-gated-coding carries the identical requirement set and the identical latent defect. +*/ + +const CONTENT_FINGERPRINT = "sha-current-content"; + +function taskWith(results: WorkflowStepResult[]): Pick { + return { workflowStepResults: results } as Pick; +} + +function gate(workflowStepId: string, extra: Partial = {}): WorkflowStepResult { + return { workflowStepId, phase: "pre-merge", status: "passed", ...extra } as WorkflowStepResult; +} + +const singularContent = { + kind: "singular" as const, + diff: { state: "fingerprint" as const, fingerprint: CONTENT_FINGERPRINT }, +}; + +describe("pre-merge approvals for non-review gates", () => { + const required = new Set(["plan-review", "verification", "documentation-delivery", "code-review"]); + + it("approves a review-column workflow whose gates carry no diff fingerprint", () => { + const approvals = evaluatePreMergeApprovals( + taskWith([ + gate("plan-review", { reviewKind: "plan", verdict: "APPROVE" }), + gate("verification"), + gate("documentation-delivery", { verdict: "APPROVE" }), + gate("code-review", { reviewKind: "code", verdict: "APPROVE", reviewInputFingerprint: CONTENT_FINGERPRINT }), + ]), + { requiredPreMergeStepIds: required, mergeContent: singularContent }, + ); + + expect(approvals.filter((approval) => approval.state !== "approved")).toEqual([]); + }); + + /* The FN-180 guarantee must survive the carve-out: a code review is still diff-bound. */ + it("still refuses a code review recorded against different content", () => { + const approvals = evaluatePreMergeApprovals( + taskWith([ + gate("verification"), + gate("code-review", { reviewKind: "code", verdict: "APPROVE", reviewInputFingerprint: "sha-stale" }), + ]), + { requiredPreMergeStepIds: new Set(["verification", "code-review"]), mergeContent: singularContent }, + ); + + expect(approvals.find((approval) => approval.workflowStepId === "code-review")?.state).toBe("stale-content"); + expect(approvals.find((approval) => approval.workflowStepId === "verification")?.state).toBe("approved"); + }); + + it("still refuses a code review that never bound any content", () => { + const approvals = evaluatePreMergeApprovals( + taskWith([gate("code-review", { reviewKind: "code", verdict: "APPROVE" })]), + { requiredPreMergeStepIds: new Set(["code-review"]), mergeContent: singularContent }, + ); + + expect(approvals[0]?.state).toBe("unprovable-content"); + }); + + /* A non-review gate that DID bind content keeps being compared — the carve-out is not a blanket pass. */ + it("still compares a non-review gate that recorded its own fingerprint", () => { + const approvals = evaluatePreMergeApprovals( + taskWith([gate("verification", { reviewInputFingerprint: "sha-stale" })]), + { requiredPreMergeStepIds: new Set(["verification"]), mergeContent: singularContent }, + ); + + expect(approvals[0]?.state).toBe("stale-content"); + }); + + it("still refuses a failed gate regardless of fingerprints", () => { + const approvals = evaluatePreMergeApprovals( + taskWith([gate("verification", { status: "failed" })]), + { requiredPreMergeStepIds: new Set(["verification"]), mergeContent: singularContent }, + ); + + expect(approvals[0]?.state).toBe("not-approved"); + }); +}); diff --git a/packages/core/src/merge/pre-merge-approval.ts b/packages/core/src/merge/pre-merge-approval.ts index 3c60e68393..fce61f902d 100644 --- a/packages/core/src/merge/pre-merge-approval.ts +++ b/packages/core/src/merge/pre-merge-approval.ts @@ -39,6 +39,23 @@ function evaluateStep( if (!approved || !!result.remediationArchivedAt) return { workflowStepId, state: "not-approved" }; // Plan fingerprints bind plan text rather than source diff and must never be cross-compared. if (result.reviewKind === "plan") return { workflowStepId, state: "approved" }; + /* + FNXC:PreMergeApproval 2026-08-24-07:10: + A required pre-merge step is not necessarily a CONTENT REVIEW. Review-column workflows also + require deterministic verification and documentation/delivery gates, which pass on an exit code + or a completed action and never record a `reviewInputFingerprint` — there is no diff for them to + bind. Falling through to the diff comparison classified every one of them as + `unprovable-content`, so `canMergeTask` answered "task has no provable approval for the content + being merged" and NOTHING could ever merge on such a workflow. Measured on + builtin:coding-ideas-v2 via pipeline-smoke S01; builtin:review-gated-coding carries the same + latent defect and simply never reached its merge. + The carve-out is deliberately narrow: it applies only when the step is neither `code-review` nor + a `reviewKind: "code"` result AND recorded no fingerprint of its own. A content review that DID + record one still gets compared, and a code review missing its fingerprint is still refused — the + FN-180 guarantee it exists to protect is untouched. + */ + const bindsContent = requiresExplicitVerdict || result.reviewInputFingerprint !== undefined; + if (!bindsContent) return { workflowStepId, state: "approved" }; } if (!descriptor) return { workflowStepId, state: "approved" }; if (descriptor.kind === "singular") { diff --git a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts index 3a66e12754..fdcd8be03d 100644 --- a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts +++ b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts @@ -44,7 +44,7 @@ import { type PipelineTerminalState, } from "./_pipeline-terminal-state.js"; -export type PipelineBuiltinWorkflow = "builtin:coding-ideas" | "builtin:coding"; +export type PipelineBuiltinWorkflow = "builtin:coding-ideas" | "builtin:coding-ideas-v2" | "builtin:coding"; export type PipelineWorkflowId = PipelineBuiltinWorkflow | string; export type PipelineTaskSeed = { diff --git a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts index b1f06b8eaf..d3892fdea9 100644 --- a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts +++ b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts @@ -2,7 +2,7 @@ import type { PipelineScenarioResult, PipelineSmokeHarness, PipelineTaskSeed, Pi import type { PipelineTerminalState } from "./_pipeline-terminal-state.js"; import { PIPELINE_SCENARIO_DRIVERS } from "./_pipeline-drivers.js"; -export type PipelineWorkflowId = "builtin:coding-ideas" | "builtin:coding" | "renamed-clone"; +export type PipelineWorkflowId = "builtin:coding-ideas" | "builtin:coding-ideas-v2" | "builtin:coding" | "renamed-clone"; export interface PipelineScenarioContext { readonly harness: PipelineSmokeHarness; @@ -44,8 +44,17 @@ not a second inert description of behavior. export const PIPELINE_SCENARIOS: readonly PipelineScenario[] = [ { id: "S01", + /* + FNXC:PipelineSmoke 2026-08-24-07:10: + coding-ideas-v2 is covered here because a review-column workflow exercises merge admission that + the base graph never reaches: its required pre-merge set includes gates that record no diff + fingerprint. Topology assertions cannot see that — four separate defects (an unsupported plan + seam, a self-contradicting planner prompt, a missing workspace session boundary, and an + unsatisfiable approval evaluation) all passed structural review and were only caught by driving + a card to `merged-done` here. + */ title: "Ideas promotion completes the coding pipeline", - workflows: ["builtin:coding-ideas"], + workflows: ["builtin:coding-ideas", "builtin:coding-ideas-v2"], expectedTerminal: "merged-done", arrange: PIPELINE_SCENARIO_DRIVERS.s01Arrange, act: PIPELINE_SCENARIO_DRIVERS.s01Act,