fix(FN-WF): make review-column workflows actually merge
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.
This commit is contained in:
@@ -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<Task, "workflowStepResults" | "repositoryScope"> {
|
||||
return { workflowStepResults: results } as Pick<Task, "workflowStepResults" | "repositoryScope">;
|
||||
}
|
||||
|
||||
function gate(workflowStepId: string, extra: Partial<WorkflowStepResult> = {}): 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");
|
||||
});
|
||||
});
|
||||
@@ -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") {
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user