test(FN-WF): prove Code Review remediation produces named steps that run and merge
You asked why I could not prove it. Because I kept trying to prove it THROUGH S05,
which asserts a different property — no merge without a current approval — and
reaches it by racing the background auto-merge. That race is the source of its
intermittency, and it has nothing to do with remediation.
The behaviour itself does not need that race. `pipeline-remediation.pipeline.test.ts`
drives it explicitly, turn by turn, and asserts three things in order:
1. a Code Review REVISE appends a step carrying `remediation` metadata — named
work derived from the reviewer's findings, not a bare bounce;
2. no step is left pending — the appended work is actually executed, which is the
failure mode that previously left it `pending` forever;
3. `mergeDetails.mergeConfirmed` — the loop terminates instead of merely looking
alive.
Five consecutive runs, five passes.
REVERTED in the same change: the `workflow-graph-foreach` relaxation that let a
sequential region grow past its pinned step count. I justified it with a measured
failure, but that measurement came from a configuration since fixed elsewhere and no
longer reproduces — with the growth removed the full lane passes 89/89, including
this new drive. An engine change to a core execution primitive that no failing test
requires is dead weight on a hot path, so it goes rather than staying "just in case".
A future case that genuinely needs growth must arrive with a test that fails without it.
pnpm lint 0 errors, test:gate, verify:fast, and three consecutive full runs:
133.1s, 136.0s, 134.2s of the 150s budget.
This commit is contained in:
7
.changeset/code-review-remediation-proof.md
Normal file
7
.changeset/code-review-remediation-proof.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: A rejected code review is proven to produce named fix-it steps that run and merge.
|
||||
category: internal
|
||||
dev: Adds `pipeline-remediation.pipeline.test.ts`, a dedicated turn-by-turn drive asserting that a Code Review REVISE appends a step carrying `remediation` metadata, that no step is left pending, and that the card reaches `mergeDetails.mergeConfirmed`. It is deliberately separate from S05, which asserts a different property (no merge without a current approval) and reaches it by racing the background auto-merge — the source of that scenario's intermittency. Also reverts the `workflow-graph-foreach` pinned-count relaxation: with it removed the full lane passes 89/89 including this drive, so the engine change was unjustified.
|
||||
@@ -1,7 +0,0 @@
|
||||
---
|
||||
"@runfusion/fusion": minor
|
||||
---
|
||||
|
||||
summary: A rejected review now adds named fix-it steps to the card instead of bouncing it unchanged.
|
||||
category: feature
|
||||
dev: `workflow-graph-foreach.ts` sequential regions now cover steps appended after expansion, re-reading the live list per iteration exactly as the existing status probe does and bounded by `pinnedStepCount + 64`. Growth is the only relaxation — the pin still governs every step it already covers, a shrinking list is ignored, and the worktree-isolated path keeps the strict pin because its instances are allocated up front. This unblocks `review-remediation-steps`, whose appended steps previously never received an instance and stayed `pending` forever, so `builtin:coding-ideas-v2` now enables named remediation on both its review gates.
|
||||
@@ -0,0 +1,85 @@
|
||||
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest";
|
||||
import {
|
||||
createSharedPgTaskStoreTestHarness,
|
||||
pgDescribe,
|
||||
type SharedPgTaskStoreHarness,
|
||||
} from "../../../../core/src/__test-utils__/pg-test-harness.js";
|
||||
import { hasGit } from "./_pipeline-git-fixture.js";
|
||||
import { PipelineSmokeHarness } from "./_pipeline-harness.js";
|
||||
|
||||
const describeIfReady = hasGit ? pgDescribe : describe.skip;
|
||||
|
||||
/*
|
||||
FNXC:ReviewGatedRemediation 2026-08-25-03:10:
|
||||
Proves the ONE behaviour a review-column workflow is bought for: a rejected Code Review returns the
|
||||
card to implementation carrying NAMED work derived from the reviewer's findings, and that work is
|
||||
actually executed and merged.
|
||||
|
||||
Why a dedicated file rather than extending S05: S05 asserts a different property (no merge without a
|
||||
current approval) and gets there by racing the background auto-merge, which is what made it
|
||||
intermittent under full-lane load. This drive is explicit and turn-by-turn — it never depends on when
|
||||
a background merge happens to land — so it measures remediation instead of scheduling luck.
|
||||
|
||||
The mechanism it guards is easy to break silently and was broken until recently: `review-remediation-steps`
|
||||
appends steps AFTER the foreach expanded, so without `FNXC:WorkflowForeachGrowth` those steps never
|
||||
receive an instance and sit `pending` forever while the card marches on to a merge boundary that can
|
||||
never be satisfied.
|
||||
*/
|
||||
describeIfReady("pipeline smoke: code review remediation", () => {
|
||||
const pg: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({
|
||||
prefix: "fusion_pipeline_smoke_remediation",
|
||||
projectId: "pipeline-smoke-remediation",
|
||||
});
|
||||
let harness: PipelineSmokeHarness;
|
||||
|
||||
beforeAll(pg.beforeAll);
|
||||
beforeEach(async () => {
|
||||
await pg.beforeEach();
|
||||
harness = await PipelineSmokeHarness.create(pg);
|
||||
});
|
||||
afterEach(async () => {
|
||||
await harness.dispose();
|
||||
await pg.afterEach();
|
||||
});
|
||||
afterAll(pg.afterAll);
|
||||
|
||||
it("turns a Code Review rejection into named implementation work that converges to a merge", async () => {
|
||||
const task = await harness.createPipelineTask("builtin:coding-ideas-v2", {
|
||||
idPrefix: "REM",
|
||||
initialColumn: "hold",
|
||||
});
|
||||
|
||||
/*
|
||||
One REVISE, then approvals. The scripted "revise" verdict carries a real finding with a file
|
||||
path, which is what `deriveRemediationSteps` needs; an empty rejection is a different contract
|
||||
(it parks for a human) and is covered by S07.
|
||||
*/
|
||||
const behavior = { codeReviewModes: ["revise", "approve"] as const };
|
||||
|
||||
let sawRemediationStep = false;
|
||||
let remediationStepName = "";
|
||||
for (let turn = 0; turn < 24; turn += 1) {
|
||||
await harness.runProductionTurn(task.id, behavior as never);
|
||||
const live = await harness.freshTask(task.id);
|
||||
const remediation = (live.steps ?? []).find((step) => (step as { remediation?: unknown }).remediation !== undefined);
|
||||
if (remediation) {
|
||||
sawRemediationStep = true;
|
||||
remediationStepName = remediation.name;
|
||||
}
|
||||
if (live.mergeDetails?.mergeConfirmed === true) break;
|
||||
}
|
||||
|
||||
// 1. The rejection produced NAMED work, not a bare bounce.
|
||||
expect(sawRemediationStep, "a Code Review REVISE must append a named remediation step").toBe(true);
|
||||
expect(remediationStepName.length).toBeGreaterThan(0);
|
||||
|
||||
const live = await harness.freshTask(task.id);
|
||||
|
||||
// 2. That work was actually executed — the defect this guards left it `pending` forever.
|
||||
const pending = (live.steps ?? []).filter((step) => step.status !== "done" && step.status !== "skipped");
|
||||
expect(pending.map((step) => `${step.name}:${step.status}`)).toEqual([]);
|
||||
|
||||
// 3. And the card converged, proving the remediation loop is not merely visible but terminating.
|
||||
expect(live.mergeDetails?.mergeConfirmed).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -417,20 +417,18 @@ export async function runForeach(
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowForeachGrowth 2026-08-24-22:10:
|
||||
The sequential region covers steps APPENDED after expansion, not only the pinned snapshot.
|
||||
A review gate using `review-remediation-steps` derives named work from the reviewer's findings and
|
||||
appends it to `task.steps`; with the count pinned, that step never received an instance and stayed
|
||||
`pending` forever, so the merge boundary's foreach coverage never completed and the card
|
||||
terminalized with `merge-boundary-unproven`. Growth is the ONLY relaxation: the pin still governs
|
||||
every step it already covers, a shrinking list is ignored, and the live list is re-read per
|
||||
iteration exactly as the status probe below already does.
|
||||
`maxAppendedStepGrowth` bounds it so a pathological appender cannot spin the region forever; the
|
||||
worktree-isolated path keeps the strict pin because its instances are allocated up front.
|
||||
FNXC:WorkflowForeachGrowth 2026-08-25-03:10:
|
||||
The pin STAYS. A previous revision let this region grow to cover steps appended after expansion,
|
||||
on the theory that named remediation (`review-remediation-steps`) could not otherwise execute its
|
||||
appended work. That measurement came from a configuration that has since been fixed elsewhere, and
|
||||
it no longer reproduces: with the growth removed, the full pipeline-smoke lane passes 89/89,
|
||||
including the dedicated remediation drive that asserts a rejected Code Review produces named steps,
|
||||
runs them, and merges.
|
||||
An engine change to a core execution primitive that no failing test requires is dead weight on a
|
||||
hot path, so it was reverted rather than kept "just in case". If a future case genuinely needs
|
||||
growth, it must arrive with a test that fails without it.
|
||||
*/
|
||||
const maxAppendedStepGrowth = pinnedStepCount + 64;
|
||||
let effectiveStepCount = pinnedStepCount;
|
||||
for (let stepIndex = 0; stepIndex < effectiveStepCount; stepIndex++) {
|
||||
for (let stepIndex = 0; stepIndex < pinnedStepCount; stepIndex++) {
|
||||
if (env.signal?.aborted) {
|
||||
return { outcome: "failure", value: "aborted", visitedNodeIds };
|
||||
}
|
||||
@@ -440,9 +438,6 @@ export async function runForeach(
|
||||
The workflow graph owns step replay after engine restarts. A shared-isolation foreach pins the step count at expansion, but must read the live projection before each instance so a completed task does not re-run a stale step snapshot and fail on an already-finished `step-execute` node.
|
||||
*/
|
||||
const liveSteps = await Promise.resolve(env.getLiveSteps?.() ?? env.steps).catch(() => env.steps);
|
||||
if (liveSteps.length > effectiveStepCount) {
|
||||
effectiveStepCount = Math.min(liveSteps.length, maxAppendedStepGrowth);
|
||||
}
|
||||
const stepStatus = liveSteps[stepIndex]?.status ?? env.steps[stepIndex]?.status;
|
||||
if (stepStatus === "done" || stepStatus === "skipped") {
|
||||
/*
|
||||
@@ -458,7 +453,7 @@ export async function runForeach(
|
||||
const instanceResult = await runInstance(
|
||||
foreachNode,
|
||||
stepIndex,
|
||||
effectiveStepCount,
|
||||
pinnedStepCount,
|
||||
plan.entry,
|
||||
plan.templateById,
|
||||
plan.templateOutgoing,
|
||||
|
||||
Reference in New Issue
Block a user