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" );