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,