fix(FN-WF): make the smoke mock honest, and match V2 remediation to its reopen policy
The pipeline-smoke executor mock routed a gate turn 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, fell through to the implementation branch, and ended by
emitting a blanket APPROVE — silently discarding the scenario's scripted verdict.
Measured: S07 scripts `codeReviewModes: ["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.
Gate turns are now routed by the step they name (`Execute the workflow step "X"`),
which is present on every gate turn and absent from the implementation session.
Non-review gates approve without consuming review verdicts.
This matters beyond the two scenarios it fixes: the mock was manufacturing false
greens. S05 on builtin:coding-ideas-v2 passed only because its scripted
"revise twice, then approve" was being auto-approved, so the workflow's rework path
was never exercised at all. Making the mock honest reveals that path as genuinely
broken, and S05/S07 accordingly move back to the workflows where they are proven.
A green that came from a mislabel is worse than a red.
Also aligns V2's code-review remediation with `review-remediation-steps`. That is
not cosmetic symmetry: the workflow 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,
the inherited `pre-merge-remediation` returns the card to in-progress with every
step already done and nothing to execute. The earlier revert of this change blamed
the wrong cause: the empty `git merge --squash` ref came from the merger mock
resolving `task.branch`, since fixed at the harness.
Lane green: 6 files, 81 tests, 19/19 scenarios, 107.7s and 107.1s against 150s.
Remaining V2 gap, stated rather than hidden: the Code Review REVISE -> rework path
does not converge ("did not persist completed implementation-step projection"), so
S05, S07, S13 and S17 stay on their original workflows.
This commit is contained in:
@@ -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");
|
||||
});
|
||||
|
||||
/*
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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 "<name>"` 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
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user