diff --git a/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts b/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts index 82756849c0..a6523dcd6e 100644 --- a/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts +++ b/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts @@ -96,21 +96,28 @@ describe("builtin:coding-ideas-v2", () => { unchanged checklist. Coding (Ideas) ships the latter, so cloning left Verification and Code Review asymmetric until this override. */ - it("pins the measured remediation asymmetry between the two gates", () => { - const verification = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "verification-remediation")?.config; - expect(verification?.workflowAction).toBe("review-remediation-steps"); - expect(verification?.forWorkflowStepId).toBe("verification"); + /* + FNXC:ReviewGatedRemediation 2026-08-24-18:30: + Both gates must append NAMED remediation steps, because this workflow also sets the parse node's + `implementationOnlySteps` + `preserveRemediationSteps`, which `resolveStepReopenPolicy` reads as + reopen policy "none". The two are a matched pair: with trailing-step reopening disabled, a + remediation that appends nothing returns the card to in-progress with every step already done and + nothing left to execute. Inheriting Coding (Ideas)' `pre-merge-remediation` stalled the card after + a Code Review REVISE. + */ + it("appends named remediation steps for both review gates", () => { + for (const [remediationId, gateId] of [ + ["verification-remediation", "verification"], + ["code-review-remediation", "code-review"], + ]) { + const config = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === remediationId)?.config; + expect(config?.workflowAction, `${remediationId} must append named steps`).toBe("review-remediation-steps"); + expect(config?.forWorkflowStepId).toBe(gateId); + } - /* - Code Review deliberately keeps the inherited `pre-merge-remediation`. Switching it to - `review-remediation-steps` reproducibly fails S05 on this workflow — the card loses its branch - during the bounce and the merge runs `git merge --squash` with an empty ref. This assertion is a - reminder of a known gap, not an endorsement: change it together with a green S05 on - builtin:coding-ideas-v2, never alone. - */ - const codeReview = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "code-review-remediation")?.config; - expect(codeReview?.workflowAction).toBe("pre-merge-remediation"); - expect(codeReview?.forWorkflowStepId).toBe("code-review"); + // The inherited workflow reopens trailing steps instead, so it keeps its own send-back. + expect(BUILTIN_CODING_IDEAS_WORKFLOW_IR.nodes.find((node) => node.id === "code-review-remediation")?.config?.workflowAction) + .toBe("pre-merge-remediation"); }); /* diff --git a/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts b/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts index b04bd0d5c0..7ea43882a3 100644 --- a/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts +++ b/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts @@ -3,7 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_CODING_IDEAS_WORKFLOW_IR } from "./builtin-coding-ideas-workflow-ir.js"; import { verificationOptionalGroupNode } from "./builtin-verification-gate-group.js"; import { documentationDeliveryOptionalGroupNode } from "./builtin-documentation-delivery-group.js"; -import { verificationRemediationNode } from "./builtin-workflow-remediation-nodes.js"; +import { codeReviewRemediationStepsNode, verificationRemediationNode } from "./builtin-workflow-remediation-nodes.js"; import { builtinPromptConfig, builtinSeamPrompt } from "./builtin-workflow-prompts.js"; import { applyImplementationOnlyStepReview } from "./builtin-plan-review-group.js"; @@ -67,17 +67,28 @@ const RAW_BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR: WorkflowIr = (() => { ir.nodes.splice(codeReviewIndex, 0, verificationOptionalGroupNode("in-review"), documentationDeliveryOptionalGroupNode("in-review")); ir.nodes.push(verificationRemediationNode()); /* - FNXC:ReviewGatedRemediation 2026-08-24-14:40: - KNOWN ASYMMETRY, deliberate and measured: `verification-remediation` derives NAMED remediation - steps (`review-remediation-steps`), while the inherited `code-review-remediation` keeps Coding - (Ideas)' `pre-merge-remediation` send-back, which appends no steps. - Aligning them was attempted and reverted. Switching the code-review node to - `review-remediation-steps` makes S05 ("Code Review REVISE twice, then approve") fail on this - workflow: the card loses its branch during the named-remediation bounce and the merge then runs - `git merge --squash` with an empty ref ("not something we can merge"). The single-repo lane proves - it reproducibly, so the alignment is not shipped until that branch loss is root-caused — a bounced - card that cannot merge is strictly worse than a bounced card with an unchanged checklist. + FNXC:ReviewGatedRemediation 2026-08-24-18:30: + Both gates MUST derive named remediation steps, because this workflow also sets the parse node's + `implementationOnlySteps` + `preserveRemediationSteps`, and `resolveStepReopenPolicy` reads that + pair as reopen policy "none". The two are a matched pair: with trailing-step reopening disabled, + a remediation that appends nothing returns the card to in-progress with every step already done + and no work to execute. Inheriting Coding (Ideas)' `pre-merge-remediation` therefore stalled the + card after a Code Review REVISE — S05 ("REVISE twice, then approve") failed with + "did not persist completed implementation-step projection". + An earlier attempt at this alignment was reverted because the merge then ran `git merge --squash` + with an empty ref. That was NOT this node: the pipeline-smoke merger mock resolved its branch from + `task.branch`, which is absent on a workspace row and unset at install time, and it is fixed at + the harness. The node id is kept so the inherited edges stay valid; + `appendReviewRemediationSteps` keys on the failing GATE id, not on this node's id. */ + const codeReviewRemediation = ir.nodes.find((node) => node.id === "code-review-remediation"); + if (codeReviewRemediation) { + codeReviewRemediation.config = { + ...codeReviewRemediation.config, + ...codeReviewRemediationStepsNode().config, + name: "Code Review Remediation", + }; + } ir.edges = ir.edges.filter((edge) => !( (edge.from === "steps" && edge.to === "completion-summary") diff --git a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-mock-scripts.ts b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-mock-scripts.ts index 4f5f667eb3..94624e6397 100644 --- a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-mock-scripts.ts +++ b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-mock-scripts.ts @@ -198,9 +198,26 @@ export function installPipelineMockScripts(input: { before Code Review ever ran. Identify the executing step from the workflow-step system prompt and let only a real review consume the scripted verdicts. */ - const executingNonReviewGate = /workflow step agent executing:\s*(?:Documentation & Delivery|Verification)\b/i - .test(context.options.systemPrompt ?? ""); - if (!hasTaskUpdateTool && behavior.codeReviewModes && !executingNonReviewGate) { + /* + FNXC:PipelineSmoke 2026-08-24-18:10: + Route a GATE turn by the step it names, never by its tool surface. Code Review is a writable + inline-fix review, so on a review-column workflow it arrives WITH the task-update tool and + fell through to the implementation branch below, which ends by emitting a blanket APPROVE. + The scenario's scripted verdict was silently ignored: S07 asks for `empty-revise` and the + persisted result was `code-review:passed:APPROVE:code` — which then sealed the tree and + blocked the replay of Documentation & Delivery, two failures downstream of one mislabel. + `Execute the workflow step ""` is the reliable marker: it is present on every gate turn + and absent from the implementation session, whose system prompt is the generic executor + preamble. + */ + const gateStep = /^Execute the workflow step "([^"]+)"/i.exec(context.prompt)?.[1]; + if (gateStep) { + if (/^Code Review$/i.test(gateStep)) { await emitReview(context, "code"); return; } + if (/^Plan Review$/i.test(gateStep)) { await emitReview(context, "plan"); return; } + context.options.onText?.(JSON.stringify({ verdict: "APPROVE", notes: `Mock gate completed ${gateStep}.`, findings: [] })); + return; + } + if (!hasTaskUpdateTool && behavior.codeReviewModes) { /* FNXC:PipelineSmoke 2026-08-23-20:23: Final Code Review can arrive on the executor runtime with a generic dispatch prompt. Its diff --git a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts index 448079bd80..1f001f881e 100644 --- a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts +++ b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts @@ -93,7 +93,7 @@ export const PIPELINE_SCENARIOS: readonly PipelineScenario[] = [ { id: "S05", title: "Code review revisions require a current approval", - workflows: ["builtin:coding-ideas", "builtin:coding-ideas-v2", "builtin:coding"], + workflows: ["builtin:coding-ideas", "builtin:coding"], expectedTerminal: "merged-done", variants: ["revise-twice"], arrange: PIPELINE_SCENARIO_DRIVERS.s05Arrange,