From bf147d6adeac7e8fbdece323116a1266754d6504 Mon Sep 17 00:00:00 2001 From: Fusion Agent Date: Tue, 25 Aug 2026 19:46:49 +0000 Subject: [PATCH] test(FN-WF): prove Code Review remediation produces named steps that run and merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/code-review-remediation-proof.md | 7 ++ .changeset/foreach-appended-step-coverage.md | 7 -- .../pipeline-remediation.pipeline.test.ts | 85 +++++++++++++++++++ .../src/workflows/workflow-graph-foreach.ts | 29 +++---- 4 files changed, 104 insertions(+), 24 deletions(-) create mode 100644 .changeset/code-review-remediation-proof.md delete mode 100644 .changeset/foreach-appended-step-coverage.md create mode 100644 packages/engine/src/__tests__/pipeline-smoke/pipeline-remediation.pipeline.test.ts diff --git a/.changeset/code-review-remediation-proof.md b/.changeset/code-review-remediation-proof.md new file mode 100644 index 0000000000..618e5c70e5 --- /dev/null +++ b/.changeset/code-review-remediation-proof.md @@ -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. diff --git a/.changeset/foreach-appended-step-coverage.md b/.changeset/foreach-appended-step-coverage.md deleted file mode 100644 index 39339c331e..0000000000 --- a/.changeset/foreach-appended-step-coverage.md +++ /dev/null @@ -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. diff --git a/packages/engine/src/__tests__/pipeline-smoke/pipeline-remediation.pipeline.test.ts b/packages/engine/src/__tests__/pipeline-smoke/pipeline-remediation.pipeline.test.ts new file mode 100644 index 0000000000..12ff9d1e56 --- /dev/null +++ b/packages/engine/src/__tests__/pipeline-smoke/pipeline-remediation.pipeline.test.ts @@ -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); + }); +}); diff --git a/packages/engine/src/workflows/workflow-graph-foreach.ts b/packages/engine/src/workflows/workflow-graph-foreach.ts index a316398fdd..11815b4f73 100644 --- a/packages/engine/src/workflows/workflow-graph-foreach.ts +++ b/packages/engine/src/workflows/workflow-graph-foreach.ts @@ -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,