From 063978c28993e0b343c4490d94ef3b6eb6cf8952 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 28 Jul 2026 19:09:06 -0700 Subject: [PATCH] =?UTF-8?q?U12=20part=203:=20make=20the=20v1-IR=20persiste?= =?UTF-8?q?nce=20unconditional=20=E2=80=94=20after=20this,=20every=20raw-f?= =?UTF-8?q?lag=20read=20is=20on=20the=20move=20path=20(U2b)=20(#2513)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## U12 part 3 — every remaining raw-flag read is now on the move path **Stacks on #2512** (shares a line in `workflow-ops.ts`). Merge that first. **Behaviour-preserving. Not a single persisted byte changes.** ### What changed The three v1-IR rollback-compat persist sites (#1405) all read `flagOn ? ir : downgradeIrToV1IfPure(ir)`, where `flagOn` came from the retired raw `experimentalFeatures.workflowColumns` key. No production writer sets it, so **every real project has always taken the downgrade arm**. Removing the branch is a runtime no-op; it deletes three flag reads. Sites: `createWorkflowDefinitionImpl`, `updateWorkflowDefinitionImpl`, and `insertWorkflowDefinitionSyncImpl` — whose `flagOn` *parameter* is gone too, along with the plumbing that resolved it in `migrateLegacyWorkflowStepsImpl`. With those gone, **`TaskStore.workflowColumnsFlagOn()` has no callers and is deleted.** Its six readers were the three U5 guards (part 2) and these three persist sites. ### The decision I made, and why I went the other way I had this slice scoped as "retire the v1 downgrade." **I rejected that.** It is a compatibility affordance, not cutover machinery: it fires only for a graph exactly equivalent to pure v1 (default columns, default placements, no v2-only features), and `upgradeV1ToV2` re-reads it into an identical v2 graph, so the runtime never sees a difference. Retiring it would break a binary downgrade for zero benefit — and stale binaries opening these databases is an **observed event** in this project, not a hypothetical. So the slice became the strictly better version of itself: same three flag reads removed, no compat surface touched. ### Why this matters for sequencing `isWorkflowColumnsCompatibilityFlagEnabled` survives. It is still read by `moves.ts:363` and by `workflow-task-create-ops.ts:351`'s move-policy preflight that feeds it. Removing those reads **is** the U2b move-path convergence with its equivalence-proof obligation. The point of deleting the wrapper is that it makes the remainder enumerable: ``` $ grep -rn isWorkflowColumnsCompatibilityFlagEnabled --include=*.ts packages/ | grep -v __tests__ packages/core/src/store.ts:38 <- the definition packages/core/src/task-store/moves.ts:9,363 <- U2b packages/core/src/task-store/workflow-task-create-ops.ts:11,351 <- U2b (feeds moves.ts) ``` **Every surviving read is on the move path.** U2b deletes the definition and the unit closes. ### On coverage — stated honestly This change is behaviour-preserving, so it has **no revert-proof test**, and I am not going to claim one. `flagOn ? ir : downgrade(ir)` with an always-false flag *is* `downgrade(ir)`. What needed a guard is the next edit someone is tempted to make — deleting `downgradeIrToV1IfPure` as dead cutover machinery. New `workflow-ir-v1-rollback-persistence.test.ts` fails if it is removed, and pins the exact boundary: the built-in coding workflow (named columns + traits) stays v2; a pure-v1-equivalent graph stores as v1 without the synthesized `columns`; a downgraded graph re-parses to an **identical** runtime graph (the property that makes unconditional application safe); a graph with a custom column stays v2. ### Verification `pnpm test:gate` (307 + 10 + 71), `pnpm lint`, `pnpm verify:fast` (17 steps), typecheck green. Core workflow-named suites: 383 passed, 1 failed — `workflow-ir-settings.test.ts > moved-key catalog ...` (`expected 10 to strictly equal 3`), which I confirmed fails identically on a stashed clean tree. Pre-existing, unrelated. No Fusion instance booted. ## Summary by CodeRabbit * **Bug Fixes** * Improved workflow persistence compatibility by consistently storing pure v1-equivalent workflows in the compatible format. * Preserved v2 workflows and custom column information when they are not v1-equivalent. * Retired obsolete feature-flag checks without changing stored workflow or board behavior. * **Tests** * Added coverage for workflow version preservation, rollback-compatible serialization, and custom columns. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../u12-unconditional-v1-persistence.md | 7 ++ ...kflow-definition-id-allocator-sync.test.ts | 4 +- ...orkflow-ir-v1-rollback-persistence.test.ts | 83 +++++++++++++++++++ packages/core/src/store.ts | 19 +++-- packages/core/src/task-store/lifecycle-ops.ts | 16 ++-- .../core/src/task-store/project-store-ops.ts | 23 ++++- .../src/task-store/workflow-definitions.ts | 3 +- packages/core/src/task-store/workflow-ops.ts | 18 ++-- 8 files changed, 141 insertions(+), 32 deletions(-) create mode 100644 .changeset/u12-unconditional-v1-persistence.md create mode 100644 packages/core/src/__tests__/workflow-ir-v1-rollback-persistence.test.ts diff --git a/.changeset/u12-unconditional-v1-persistence.md b/.changeset/u12-unconditional-v1-persistence.md new file mode 100644 index 0000000000..93bdaa0860 --- /dev/null +++ b/.changeset/u12-unconditional-v1-persistence.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Internal cleanup of retired workflow-columns flag reads; no change to stored workflows or board behavior. +category: internal +dev: The three v1-IR rollback-compat persist sites (`createWorkflowDefinition`, `updateWorkflowDefinition`, `insertWorkflowDefinitionSync`) branched on the retired raw `experimentalFeatures.workflowColumns` key, which is always false, so the downgrade arm was always taken. The branch and the `flagOn` parameter are removed; `downgradeIrToV1IfPure` is kept as a binary-downgrade affordance and pinned by `workflow-ir-v1-rollback-persistence.test.ts`. `TaskStore.workflowColumnsFlagOn()` is deleted (no callers). `isWorkflowColumnsCompatibilityFlagEnabled` survives; every remaining read is on the move path (U2b). diff --git a/packages/core/src/__tests__/workflow-definition-id-allocator-sync.test.ts b/packages/core/src/__tests__/workflow-definition-id-allocator-sync.test.ts index 0a87c9e2b4..6be3021078 100644 --- a/packages/core/src/__tests__/workflow-definition-id-allocator-sync.test.ts +++ b/packages/core/src/__tests__/workflow-definition-id-allocator-sync.test.ts @@ -51,10 +51,10 @@ describe("workflow definition id allocator (sync materialization path)", () => { const created = insertWorkflowDefinitionSyncImpl(store, { name: "fresh workflow", ir: BUILTIN_CODING_WORKFLOW_IR, - }, true); + }); expect(created.id).toBe("WF-003"); - const second = insertWorkflowDefinitionSyncImpl(store, { name: "second workflow", ir: BUILTIN_CODING_WORKFLOW_IR }, true); + const second = insertWorkflowDefinitionSyncImpl(store, { name: "second workflow", ir: BUILTIN_CODING_WORKFLOW_IR }); expect(second.id).toBe("WF-004"); }); }); diff --git a/packages/core/src/__tests__/workflow-ir-v1-rollback-persistence.test.ts b/packages/core/src/__tests__/workflow-ir-v1-rollback-persistence.test.ts new file mode 100644 index 0000000000..93f0bfcdfe --- /dev/null +++ b/packages/core/src/__tests__/workflow-ir-v1-rollback-persistence.test.ts @@ -0,0 +1,83 @@ +/* +FNXC:WorkflowColumns 2026-07-28-00:00 (U12 — R9): +Pins the v1-IR rollback-compat PERSISTENCE SHAPE (#1405), which U12 made +unconditional by deleting the retired-flag ternary around it. + +WHY THIS FILE EXISTS. The U12 change at the three persist sites is +behaviour-preserving and therefore has NO revert-proof test — `flagOn ? ir : +downgrade(ir)` with a flag that is always false is the same thing as +`downgrade(ir)`, so re-adding the branch changes nothing observable. Saying "covered +by tests" about that edit would be false. + +What DOES need a guard is the next edit someone is tempted to make: deleting the +downgrade outright as "dead cutover machinery". It is not cutover machinery — it is a +compatibility affordance that lets a binary downgrade still load a row, and stale +binaries opening these databases is an observed event in this project. These cases +fail if the downgrade is removed, and they pin the exact boundary of when it applies. +*/ +import { describe, expect, it } from "vitest"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { downgradeIrToV1IfPure, parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; +import type { WorkflowIrV2 } from "../workflow-ir-types.js"; + +describe("v1 IR rollback-compat persistence shape", () => { + it("stores a pure-v1-equivalent graph in the v1 shape", () => { + const stored = downgradeIrToV1IfPure(BUILTIN_CODING_WORKFLOW_IR); + // Fails if the downgrade is deleted: the built-in coding workflow declares named + // columns with traits, so it is NOT pure v1 and must stay v2. + expect(BUILTIN_CODING_WORKFLOW_IR.version).toBe("v2"); + expect(stored.version).toBe("v2"); + }); + + it("downgrades a graph that carries only default columns and default placements", () => { + // A v1 graph upgraded to v2 is by construction pure-v1-equivalent, so it must + // round-trip back down to v1 on persist. + const upgraded = parseWorkflowIr({ + version: "v1", + name: "pure", + nodes: [ + { id: "start", kind: "start" }, + { id: "n1", kind: "prompt", config: { prompt: "hi" } }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "n1" }, + { from: "n1", to: "end" }, + ], + }); + expect(upgraded.version).toBe("v2"); + + const stored = downgradeIrToV1IfPure(upgraded); + expect(stored.version).toBe("v1"); + // The stored v1 row must not carry the synthesized placement fields. + expect(JSON.parse(serializeWorkflowIr(stored))).not.toHaveProperty("columns"); + }); + + it("round-trips a downgraded graph back to an IDENTICAL runtime graph", () => { + // This is the property that makes the downgrade safe to keep and safe to have + // always applied: what the runtime sees is unchanged by the stored shape. + const upgraded = parseWorkflowIr({ + version: "v1", + name: "pure", + nodes: [ + { id: "start", kind: "start" }, + { id: "n1", kind: "prompt", config: { prompt: "hi" } }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "n1" }, + { from: "n1", to: "end" }, + ], + }); + const rehydrated = parseWorkflowIr(JSON.parse(serializeWorkflowIr(downgradeIrToV1IfPure(upgraded)))); + expect(rehydrated).toEqual(upgraded); + }); + + it("keeps a graph with a CUSTOM column on v2", () => { + const ir = structuredClone(BUILTIN_CODING_WORKFLOW_IR) as WorkflowIrV2; + ir.columns.push({ id: "custom-hold", name: "Custom hold", traits: [] }); + const stored = downgradeIrToV1IfPure(ir); + expect(stored.version).toBe("v2"); + expect((stored as WorkflowIrV2).columns.map((c) => c.id)).toContain("custom-hold"); + }); +}); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 09c2b357e2..cd176ea636 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -2397,9 +2397,18 @@ Issue #2149 requires read-only type filtering to occur in the file-store before // never a raw column write, so capacity (KTD-10) and single transition authority // (KTD-3) are honored. Only consulted when `workflowColumns` flag is ON. - public async workflowColumnsFlagOn(): Promise { - return isWorkflowColumnsCompatibilityFlagEnabled(await this.getSettingsFast()); - } + /* + FNXC:WorkflowColumns 2026-07-28-00:00 (U12 — R9): + `workflowColumnsFlagOn()` is DELETED — it has no callers left. Its six readers were + the three U5 reconciliation guards (now unconditional) and the three v1-IR + rollback-compat persistence sites (now unconditional, same stored bytes). + + The underlying `isWorkflowColumnsCompatibilityFlagEnabled` SURVIVES for now: it is + still read by `moves.ts` and `workflow-task-create-ops.ts`, and removing those reads + IS the U2b move-path convergence, which carries an equivalence-proof obligation. + Deleting this wrapper is what makes the remaining reads easy to enumerate: after + this change, every surviving read of the raw flag is on the move path. + */ public async listWorkflowOccupantTaskIds(workflowId: string, includeNullSelection: boolean): Promise { return listWorkflowOccupantTaskIdsImpl(this, workflowId, includeNullSelection); } @@ -2474,8 +2483,8 @@ Issue #2149 requires read-only type filtering to occur in the file-store before } /** Synchronous workflow-definition insert used by migration (U2/KTD-3). */ - public insertWorkflowDefinitionSync( input: WorkflowDefinitionInput, flagOn: boolean, ): WorkflowDefinition { - return insertWorkflowDefinitionSyncImpl(this, input, flagOn); + public insertWorkflowDefinitionSync( input: WorkflowDefinitionInput ): WorkflowDefinition { + return insertWorkflowDefinitionSyncImpl(this, input); } async migrateLegacyWorkflowSteps(): Promise<{ migrated: number; skipped: number; combinedWorkflowId?: string; }> { return migrateLegacyWorkflowStepsImpl(this); diff --git a/packages/core/src/task-store/lifecycle-ops.ts b/packages/core/src/task-store/lifecycle-ops.ts index 119f07058b..f333d6414d 100644 --- a/packages/core/src/task-store/lifecycle-ops.ts +++ b/packages/core/src/task-store/lifecycle-ops.ts @@ -1112,11 +1112,15 @@ export async function recoverStaleTransitionPendingImpl(store: TaskStore): Promi } export async function migrateLegacyWorkflowStepsImpl(store: TaskStore): Promise<{ migrated: number; skipped: number; combinedWorkflowId?: string; }> { - // Resolve async prerequisites BEFORE the synchronous transaction: the - // workflow-columns flag (for flag-aware persistence). The project default is - // re-read AFTER the transaction (compare-and-set) so a concurrently-set - // default is never clobbered. - const flagOn = await store.workflowColumnsFlagOn(); + /* + FNXC:WorkflowColumns 2026-07-28-00:00 (U12 — R9, BEHAVIOUR-PRESERVING): + The workflow-columns flag read is DELETED. It was resolved here only to thread + "flag-aware persistence" into `insertWorkflowDefinitionSync`, where it chose + between the v2 shape and the pure-v1 downgrade. The flag is retired and always + false, so the downgrade arm was always taken; it is now unconditional inside the + insert and the parameter is gone. The project default is still re-read AFTER the + transaction (compare-and-set) so a concurrently-set default is never clobbered. + */ const result = store.db.transactionImmediate(() => { // Write lock is now held. Read the raw step rows directly (the cached, @@ -1152,7 +1156,6 @@ export async function migrateLegacyWorkflowStepsImpl(store: TaskStore): Promise< ir: fragmentIr, layout: layoutForIr(fragmentIr), }, - flagOn, ); store.db .prepare("UPDATE workflow_steps SET migrated_fragment_id = ?, updatedAt = ? WHERE id = ?") @@ -1174,7 +1177,6 @@ export async function migrateLegacyWorkflowStepsImpl(store: TaskStore): Promise< ir, layout: layoutForIr(ir), }, - flagOn, ); combinedWorkflowId = combined.id; } diff --git a/packages/core/src/task-store/project-store-ops.ts b/packages/core/src/task-store/project-store-ops.ts index 7b31b60441..ba705deac0 100644 --- a/packages/core/src/task-store/project-store-ops.ts +++ b/packages/core/src/task-store/project-store-ops.ts @@ -682,9 +682,24 @@ export function __setWorkflowDefinitionBeforeInsertForTesting( } export async function createWorkflowDefinitionImpl(store: TaskStore, input: WorkflowDefinitionInput,): Promise { - // Rollback compat (#1405): with the flag OFF, persist a pure-v1-equivalent - // graph in the v1 shape so a binary downgrade can still load the row. - const flagOnForCreate = await store.workflowColumnsFlagOn(); + /* + Rollback compat (#1405): persist a pure-v1-equivalent graph in the v1 shape so a + binary downgrade can still load the row. + + FNXC:WorkflowColumns 2026-07-28-00:00 (U12 — R9, BEHAVIOUR-PRESERVING): + The `flagOnForCreate` read is DELETED and the downgrade is now unconditional — + which is what it already was. The ternary was `flagOn ? ir : downgrade(ir)`, and + `flagOn` reads the retired raw `experimentalFeatures.workflowColumns` key that no + production writer sets, so every real project has ALWAYS taken the downgrade arm. + Removing the branch changes no persisted byte; it deletes a flag read. + + The downgrade itself is KEPT deliberately. It is a compatibility affordance, not + cutover machinery: it only fires for a graph that is exactly equivalent to pure v1 + (default columns, default placements, no v2-only features), and `upgradeV1ToV2` + re-reads it into an identical v2 graph. Retiring it would break a binary downgrade + for no benefit, and stale binaries opening these databases is an observed event in + this project, not a hypothetical. + */ return store.withConfigLock(async () => { const name = input.name?.trim(); if (!name) throw new Error("Workflow name is required"); @@ -724,7 +739,7 @@ export async function createWorkflowDefinitionImpl(store: TaskStore, input: Work name: definition.name, description: definition.description, icon: definition.icon ?? null, - ir: (flagOnForCreate ? definition.ir : downgradeIrToV1IfPure(definition.ir)) as unknown as object, + ir: downgradeIrToV1IfPure(definition.ir) as unknown as object, layout: definition.layout as unknown as object, kind: definition.kind, createdAt: definition.createdAt, diff --git a/packages/core/src/task-store/workflow-definitions.ts b/packages/core/src/task-store/workflow-definitions.ts index abb63a16cb..47c4396f7b 100644 --- a/packages/core/src/task-store/workflow-definitions.ts +++ b/packages/core/src/task-store/workflow-definitions.ts @@ -356,7 +356,6 @@ export async function occupantsByColumnForWorkflowImpl(store: TaskStore, export function insertWorkflowDefinitionSyncImpl(store: TaskStore, input: WorkflowDefinitionInput, - flagOn: boolean, ): WorkflowDefinition { const name = input.name?.trim(); if (!name) throw new Error("Workflow name is required"); @@ -387,7 +386,7 @@ export function insertWorkflowDefinitionSyncImpl(store: TaskStore, definition.name, definition.description, definition.icon ?? null, - serializeWorkflowIr(flagOn ? definition.ir : downgradeIrToV1IfPure(definition.ir)), + serializeWorkflowIr(downgradeIrToV1IfPure(definition.ir)), JSON.stringify(definition.layout), definition.kind, definition.createdAt, diff --git a/packages/core/src/task-store/workflow-ops.ts b/packages/core/src/task-store/workflow-ops.ts index e32895b907..eb7e9a1d83 100644 --- a/packages/core/src/task-store/workflow-ops.ts +++ b/packages/core/src/task-store/workflow-ops.ts @@ -311,19 +311,13 @@ export async function updateWorkflowDefinitionImpl(store: TaskStore, id: string, description: next.description, icon: next.icon ?? null, /* - FNXC:WorkflowColumns 2026-07-28-00:00 (U12 — deliberately NOT flipped here): - This is the v1-IR rollback-compat persistence decision (#1405), not an - enforcement gate — it chooses the STORED SHAPE of the graph so an older binary - could still load the row. It shared the U5 `flagOn` variable that this change - deletes, which is why it is spelled out separately now rather than silently - inheriting the flip: one flag read was feeding two unrelated decisions, so the - flag has more decision sites than call sites. - - Retiring the downgrade is a persistence-format change with a different blast - radius than a guard, and it needs its own round-trip evidence. Left reading the - raw flag, unchanged in behaviour, for a follow-up. + FNXC:WorkflowColumns 2026-07-28-00:00 (U12 — R9, BEHAVIOUR-PRESERVING): + v1-IR rollback-compat persistence (#1405), now unconditional — which is what it + already was. The flag read it used to branch on is retired and always false, so + every real project has always taken the downgrade arm. See the fuller note on + the create path in `project-store-ops.ts`; the downgrade is kept on purpose. */ - ir: (await store.workflowColumnsFlagOn()) ? next.ir : downgradeIrToV1IfPure(next.ir), + ir: downgradeIrToV1IfPure(next.ir), layout: next.layout, updatedAt: next.updatedAt, }).where(eq(schema.project.workflows.id, id));