From 4750b689ea6985fb8a47c62c64c8fde27a34e488 Mon Sep 17 00:00:00 2001 From: Fusion Agent Date: Tue, 25 Aug 2026 12:31:43 +0000 Subject: [PATCH] refactor(FN-WF): classify workflow gates structurally, not by display name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects of the same family, all of which let a LABEL decide BEHAVIOUR. 1. `workflowNodeRequiresWorktree` matched `/(?:^|\b)(?:review|verification)(?:\b|$)/i` against `config.name`. A deterministic verification gate — exit codes only, no mutation path — was therefore classified write-capable purely because it is called "Verification", and the review seal refused it on every post-approval replay. It now keys on `reviewKind`, `workflowAction` and the optional-group id. 2. The review seal's `isCodeReview` also matched `/code review/i`, so its central question — "is this THE review that seals the tree?" — depended on a name an operator may change. Renaming the gate to "Final Review" would have silently stopped it being recognised while every other gate kept being sealed against it. That is why the rename was blocked; it no longer is. 3. `getRunningOptionalGateBadge` gated on a closed list of three step ids, so gates a workflow adds showed no badge at all: the operator watched an apparently idle card until "Merging" appeared at the end. It now asks whether the running step is a lane-owned gate. Also: task cards arrive in the review lane with their step list EXPANDED, as they already were in in-progress. The initial state is computed once per mount and a column move remounts the card, so a card the operator had open collapsed itself exactly when its review gates started running. Lifecycle-column ratchet ceilings lowered to the measured counts (todo 64→12, in-progress 197→72, in-review 213→28). They had drifted so far above reality that the ratchet was inert for the thing it exists to stop — the TaskCard guard that hid review-lane progress was one of those tolerated sites, and it had no way to notice. Tests updated to the new truth rather than around it: the seal ratchet now asserts a deterministic gate must NOT be write-capable, and the badge tests assert the expanded list. pnpm lint 0 errors, test:gate, verify:fast, dashboard 695, core 38, engine 8, and three consecutive smoke runs: 122.4s, 126.3s, 124.8s of the 150s budget. --- .changeset/structural-workflow-predicates.md | 7 ++++++ .../no-hardcoded-lifecycle-columns.test.ts | 14 ++++++++--- .../dashboard/app/components/TaskCard.tsx | 7 ++++++ .../components/__tests__/TaskCard.test.tsx | 9 ++++++-- packages/dashboard/app/utils/taskProgress.ts | 19 +++++++++------ .../coding-ideas-v2-review-seal.test.ts | 23 +++++++++++++------ .../src/executor/execute-workflow-graph.ts | 14 +++++++++-- .../workflow-node-execution-needs.ts | 20 +++++++++++++--- 8 files changed, 89 insertions(+), 24 deletions(-) create mode 100644 .changeset/structural-workflow-predicates.md diff --git a/.changeset/structural-workflow-predicates.md b/.changeset/structural-workflow-predicates.md new file mode 100644 index 0000000000..9b78ba3db9 --- /dev/null +++ b/.changeset/structural-workflow-predicates.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Workflow gates are classified by what they are, not by what they are called. +category: fix +dev: Three name/id-coupling defects removed. `workflowNodeRequiresWorktree` matched `/review|verification/i` against `config.name`, so a deterministic verification gate was classified write-capable purely because of its label and the review seal refused it on every post-approval replay; it now keys on `reviewKind`, `workflowAction` and the optional-group id. The review seal's `isCodeReview` likewise matched `/code review/i`, which would have silently unrecognised a gate renamed to "Final Review". `getRunningOptionalGateBadge` replaced a closed three-id allowlist with `isNonImplementationWorkflowStepId`, so review-lane gates added by a workflow surface a running badge instead of leaving the card apparently idle until "Merging". Lifecycle-column ratchet ceilings lowered to measured counts (todo 64→12, in-progress 197→72, in-review 213→28). diff --git a/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts b/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts index d1c190c63a..6af7dcefc5 100644 --- a/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts +++ b/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts @@ -220,11 +220,19 @@ these values the same single-guard probe takes the gate red. LOWER these as conversions land; a raise means a literal came back OR detection improved — and if it is the latter, say which sites in the commit, as previous revisions of this file did. */ +/* +FNXC:LifecycleColumnRatchet 2026-08-25-02:10: +Ceilings lowered to the MEASURED counts. They had drifted far above reality (todo 64 vs 12, +in-progress 197 vs 72, in-review 213 vs 28), which made the ratchet inert for the very thing it +exists to stop: a new hardcoded guard could be added freely inside that headroom. Not hypothetical — +the TaskCard guard that hid review-lane progress from operators was one of those tolerated sites, +and the ratchet had no way to notice it. A ceiling that sits far above the count measures nothing. +*/ const CEILINGS: Record = { triage: 11, - todo: 64, - "in-progress": 197, - "in-review": 213, + todo: 12, + "in-progress": 72, + "in-review": 28, }; describe("lifecycle-column literal ratchet (AST)", () => { diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index 4850511260..e4e6d03ed0 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -1115,8 +1115,15 @@ function TaskCardComponent({ : TIME_INDICATOR_COLUMNS.has(task.column); const [isSaving, setIsSaving] = useState(false); + /* + FNXC:TaskCardWorkflowProgress 2026-08-25-02:10: + The review lane starts expanded too. This initial state is computed once per mount, and moving a + card between columns remounts it, so a card that was expanded in in-progress collapsed the moment + it reached review — exactly where a review-column workflow has gates worth watching. + */ const [showSteps, setShowSteps] = useState( isWipColumn || + isReviewColumn || (isIntakeColumn && task.steps.some(s => s.status === "done" || s.status === "skipped")) ); const [missionTitle, setMissionTitle] = useState(null); diff --git a/packages/dashboard/app/components/__tests__/TaskCard.test.tsx b/packages/dashboard/app/components/__tests__/TaskCard.test.tsx index 1a51ceec5f..b98eba1ff3 100644 --- a/packages/dashboard/app/components/__tests__/TaskCard.test.tsx +++ b/packages/dashboard/app/components/__tests__/TaskCard.test.tsx @@ -2797,8 +2797,13 @@ describe("TaskCard", () => { distinct, additive affordance — it is not replaced by the bar. */ expect(container.querySelector(".card-progress")).not.toBeNull(); - // The expandable step list stays collapsed until the operator opens it. - expect(container.querySelector(".card-steps-list")).toBeNull(); + /* + FNXC:TaskCardWorkflowProgress 2026-08-25-02:10: + The list is EXPANDED on arrival in the review lane, as it already was in in-progress. The + initial state is computed once per mount and a column move remounts the card, so a card the + operator had open collapsed itself at exactly the point its review gates start running. + */ + expect(container.querySelector(".card-steps-list")).not.toBeNull(); }); /* diff --git a/packages/dashboard/app/utils/taskProgress.ts b/packages/dashboard/app/utils/taskProgress.ts index a80d9192a3..2d6d00766b 100644 --- a/packages/dashboard/app/utils/taskProgress.ts +++ b/packages/dashboard/app/utils/taskProgress.ts @@ -288,13 +288,18 @@ export function getRunningOptionalGateBadge( } if (!(columnFlags ? isReviewColumnRole(columnFlags, task.column) : REVIEW_LANE_COLUMNS.has(task.column))) return undefined; - if ( - workflowStepId !== "code-review" - && workflowStepId !== "browser-verification" - && workflowStepId !== "post-merge-verification" - ) { - return undefined; - } + /* + FNXC:TaskCardOptionalGateBadge 2026-08-25-02:10: + Badge whatever review-lane gate is RUNNING, instead of a closed list of three ids. The old test + named `code-review`, `browser-verification` and `post-merge-verification` explicitly, so a + workflow that adds gates — builtin:coding-ideas-v2 runs Verification and Documentation & Delivery + in review — showed no badge at all for them: the operator watched an apparently idle card until + "Merging" appeared at the very end. The running gate's own state is the signal; a hardcoded id + list can only ever describe the gates that existed when it was written. + `isNonImplementationWorkflowStepId` already distinguishes a lane-owned gate from an + implementation step, and the review-lane check above bounds this to the right column. + */ + if (!isNonImplementationWorkflowStepId(workflowStepId)) return undefined; return { workflowStepId, diff --git a/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts b/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts index 0d8f1da354..5d68d24aab 100644 --- a/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts +++ b/packages/engine/src/__tests__/coding-ideas-v2-review-seal.test.ts @@ -58,13 +58,22 @@ describe("builtin:coding-ideas-v2 review seal", () => { }); it("keeps both write-capable gates strictly before Code Review", () => { - // If these ever stop being write-capable the ordering constraint is moot and this - // whole ratchet is measuring nothing — so assert the premise, not just the outcome. - for (const gateId of ["verification", "documentation-delivery"]) { - const gate = ir.nodes.find((node) => node.id === gateId); - expect(gate, `${gateId} is missing`).toBeDefined(); - expect(isWriteCapable(gate!), `${gateId} is expected to be write-capable`).toBe(true); - } + /* + FNXC:WorkflowReviewSeal 2026-08-25-02:10: + Assert the PREMISE, not just the outcome, or this ratchet measures nothing. + `documentation-delivery` is genuinely write-capable (`toolMode: "coding"`) and is what the seal + exists to order. `verification` is NOT: it is a deterministic gate that runs commands and reads + exit codes, and it was only ever classified write-capable because the old predicate matched its + display NAME. It still runs before the review — not for the seal, but because anything between + the review and the merge invalidates the review-diff fingerprint. + */ + const documentation = ir.nodes.find((node) => node.id === "documentation-delivery"); + expect(documentation, "documentation-delivery is missing").toBeDefined(); + expect(isWriteCapable(documentation!), "documentation-delivery must be write-capable").toBe(true); + + const verification = ir.nodes.find((node) => node.id === "verification"); + expect(verification, "verification is missing").toBeDefined(); + expect(isWriteCapable(verification!), "a deterministic gate must not be classified write-capable").toBe(false); const chain = successChainFrom(ir, "steps").map((node) => node.id); expect(chain.indexOf("verification")).toBeLessThan(chain.indexOf("code-review")); diff --git a/packages/engine/src/executor/execute-workflow-graph.ts b/packages/engine/src/executor/execute-workflow-graph.ts index d47ca397d8..9dab1606df 100644 --- a/packages/engine/src/executor/execute-workflow-graph.ts +++ b/packages/engine/src/executor/execute-workflow-graph.ts @@ -684,8 +684,18 @@ export async function executeWorkflowGraph( ); if (principalAdmission) return principalAdmission; const live = await deps.store.getTask(nodeTask.id); - const name = typeof node.config?.name === "string" ? node.config.name : ""; - const isCodeReview = node.id === "code-review" || node.config?.reviewKind === "code" || /code review/i.test(name); + /* + FNXC:WorkflowReviewSeal 2026-08-25-02:10: + Structural signals only. The old test also matched `/code review/i` against the display + name, which made the seal's central question — "is this THE review that seals the tree?" — + depend on a label an operator is free to change. Renaming the gate to "Final Review" would + have silently stopped it being recognised as the sealing review while every other gate kept + being sealed against it. `reviewKind: "code"` and the node/group id are carried by every + built-in and are what the rest of the merge path already keys on. + */ + const isCodeReview = node.id === "code-review" + || node.id === "code-review-step" + || node.config?.reviewKind === "code"; /* FNXC:WorkflowReviewSeal 2026-08-24-16:20: A DETERMINISTIC verification gate is not a writer. It needs a worktree because it runs the diff --git a/packages/engine/src/workflows/workflow-node-execution-needs.ts b/packages/engine/src/workflows/workflow-node-execution-needs.ts index e140804991..52ca717610 100644 --- a/packages/engine/src/workflows/workflow-node-execution-needs.ts +++ b/packages/engine/src/workflows/workflow-node-execution-needs.ts @@ -25,14 +25,28 @@ export function workflowNodeRequiresWorktree( const rawCliCommand = executorKind === "cli" && typeof cfg.cliCommand === "string" && cfg.cliCommand.trim() ? cfg.cliCommand : undefined; - const nodeName = typeof cfg.name === "string" && cfg.name.trim() ? cfg.name.trim() : node.id; - const isPlanReview = node.id === "plan-review-step" || nodeName === "Plan Review" || optionalGroupId === "plan-review"; + /* + FNXC:WorkflowNodeNeeds 2026-08-25-02:10: + Classify by STRUCTURE, never by display name. The old test matched + `/(?:^|\b)(?:review|verification)(?:\b|$)/i` against `config.name`, which made behaviour hostage to + a label: a DETERMINISTIC verification gate — exit codes only, no mutation path whatsoever — was + classified write-capable purely because it is called "Verification", and the review seal then + refused it on every post-approval replay. It also silently blocked renaming a gate, since + "Final Review" and "Code Review" would classify differently for no structural reason. + `reviewKind`, `workflowAction` and the optional-group id are the real signals and are already + carried by every built-in node; a hand-authored node opts in explicitly with `reviewCanFixInline`. + */ + const isPlanReview = node.id === "plan-review-step" + || optionalGroupId === "plan-review" + || cfg.reviewKind === "plan"; + const isDeterministicGate = cfg.workflowAction === "deterministic-verification"; const isInlineFixReview = reviewerInlineFixes !== false && executorKind !== "cli" && !isPlanReview + && !isDeterministicGate && ( cfg.reviewCanFixInline === true - || /(?:^|\b)(?:review|verification)(?:\b|$)/i.test(nodeName) + || cfg.reviewKind === "code" || optionalGroupId === "code-review" || optionalGroupId === "browser-verification" );