U12 part 8: delete the lossy normalizeColumn + behaviour ratchet — and the definitive answer on the raw flag (2 reads left, both U2b's) (#2535)
## U12 part 8 — deletes the lossy `normalizeColumn`, and ratchets it shut Independent of the #2525 → #2528 → #2530 stack; touches only `@fusion/core` exports. This closes **one of the two `@deprecated (workflowColumns, U12)` markers** the unit was named for. ### The hazard `normalizeColumn` coerced an arbitrary value to a **legacy** column, rewriting every workflow-defined custom id to `triage`. Silent data loss for any project whose workflow declares a column outside the six built-ins — and it sat one line away from `normalizeColumnId`, which sanitises structurally and passes real ids through. The dashboard picked the wrong one for its entire task-ingest path until that was diagnosed; `useTasks.ts` and `routes-trait-rekey.test.ts` still carry the notes from that fix. So this is not a hypothetical footgun — it already fired once, on the surface where it mattered most. Deleted rather than left deprecated because it has **zero callers anywhere in the workspace**. It was pure exported hazard: a lossy coercion next to its safe twin, waiting to be picked again. ### The ratchet is the point `no-lossy-column-coercion-export.test.ts` bans the **behaviour, not the identifier**: it walks every exported single-argument function whose name mentions "column" and fails if one maps a valid custom id onto a different legacy id. Re-adding `normalizeColumn` under any name trips it. Verified by actually reintroducing the function — **two of the three cases fail, including the name-agnostic one**. That last detail is what stops it being a guard that checks nothing. Coverage stated plainly: deleting an unused export has no behaviour to revert-check. The compile is the proof it had no callers; the ratchet is the proof it cannot return. --- ## Answering the standing question: does anything still read the raw `workflowColumns` flag? **Yes. Exactly two sites, and both are U2b's.** I am not able to close this out, and here is the complete list rather than a summary: ``` packages/core/src/store.ts:38,43 ← the definition packages/core/src/task-store/moves.ts:9,363 ← `useWorkflow` packages/core/src/task-store/workflow-task-create-ops.ts:11,351 ← move-policy preflight ``` That is the whole list in production code. Everything else that greps is a comment, a test that writes the flag deliberately to exercise the dead path, or the unrelated `workflowColumns.*` i18n namespace for the Columns editor panel. **Why I have not deleted the settings key.** It cannot go while those two read it — the key is what they read. And the two are not separable from each other: `workflow-task-create-ops.ts:351` computes the `movePolicyPreflight` that `moves.ts` consumes and validates, and un-gating the preflight alone would start evaluating workflow move policies (with their plugin-gate side effects) while the branch that consumes the result stays off. That is a behaviour change with no consumer, which is worse than either state. **Status of the blocker.** U2b has not landed. `main` at `919f68f9b` still has both reads; the program's merged history goes `#2466 → #2467 → #2468 (characterisation only) → #2469 → #2479 → #2500 → #2512 → #2513`, with no convergence PR. PR #2468 was Phase A2 **steps 1–2 only** — the differential characterisation — and the convergence that deletes one of the two move paths was never merged. So the honest state of the unit: everything U12 owns is done except the two reads that U2b owns, and the settings key that cannot be deleted until they are gone. If you want me to take U2b itself, say so — I have the inventory and the divergence list, and I would want the current U2b worker stood down from `moves.ts` first. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
7
.changeset/u12-delete-lossy-normalize-column.md
Normal file
7
.changeset/u12-delete-lossy-normalize-column.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Custom board columns can no longer be silently rewritten to Planning by an internal helper.
|
||||
category: internal
|
||||
dev: Deletes `normalizeColumn` from `@fusion/core` (zero callers; the dashboard migrated to the non-lossy `normalizeColumnId` when the data loss was diagnosed) and adds `no-lossy-column-coercion-export.test.ts`, which bans any exported single-argument column helper that maps a valid custom id onto a legacy one — by behaviour, not by name.
|
||||
@@ -0,0 +1,94 @@
|
||||
/*
|
||||
FNXC:WorkflowColumns 2026-07-29-00:00 (U12 — R8):
|
||||
Ratchet: `@fusion/core` must not export a column coercion that discards
|
||||
workflow-defined ids.
|
||||
|
||||
`normalizeColumn` did exactly that — it answered "is this one of the SIX legacy ids"
|
||||
and rewrote everything else to `triage`, so any project with a custom column silently
|
||||
lost it. It sat one line away from `normalizeColumnId`, which sanitises structurally
|
||||
and passes real ids through, and the dashboard picked the wrong one for its whole task
|
||||
ingest path until that was diagnosed (see `routes-trait-rekey.test.ts`).
|
||||
|
||||
U12 deleted it once it had zero callers. This test is what stops it — or an equivalent
|
||||
under a new name — coming back: an exported helper that maps a valid custom column id
|
||||
to a legacy one is the defect, regardless of what it is called.
|
||||
*/
|
||||
import { describe, expect, it } from "vitest";
|
||||
import * as core from "../index.js";
|
||||
import { normalizeColumnId } from "../types/board.js";
|
||||
|
||||
describe("no lossy column coercion on the core public surface", () => {
|
||||
it("does not export the deleted normalizeColumn", () => {
|
||||
expect(Object.keys(core)).not.toContain("normalizeColumn");
|
||||
});
|
||||
|
||||
it("keeps normalizeColumnId non-lossy for workflow-defined ids", () => {
|
||||
// The property that made normalizeColumn wrong: a real custom column id must
|
||||
// survive. If this ever fails, the safe helper has acquired the lossy behaviour.
|
||||
for (const customId of ["ideas", "merging", "custom-hold", "signoff"]) {
|
||||
expect(normalizeColumnId(customId)).toBe(customId);
|
||||
}
|
||||
// Structural sanitisation is still expected.
|
||||
expect(normalizeColumnId("")).toBe("triage");
|
||||
expect(normalizeColumnId(undefined)).toBe("triage");
|
||||
expect(normalizeColumnId(null, "todo")).toBe("todo");
|
||||
});
|
||||
|
||||
it("catches a two-required-argument lossy coercer, not just one-argument ones", () => {
|
||||
/*
|
||||
The arity hole, pinned directly rather than only in prose: this is the shape that
|
||||
used to slip through, so the guard's own coverage is now measurable instead of
|
||||
asserted.
|
||||
*/
|
||||
const twoRequiredArgs = (value: unknown, fallback: string) =>
|
||||
(["triage", "todo", "in-progress", "in-review", "done", "archived"] as string[]).includes(value as string)
|
||||
? (value as string)
|
||||
: fallback;
|
||||
expect(twoRequiredArgs.length).toBe(2);
|
||||
// Probed WITH the fallback, a lossy coercer returns the legacy id — the signal the
|
||||
// scan keys on. Probed without it, this same function returns undefined and the
|
||||
// scan skipped it, which is the hole that made the arity filter's removal
|
||||
// insufficient on its own.
|
||||
expect(twoRequiredArgs("custom-hold", "triage")).toBe("triage");
|
||||
expect(twoRequiredArgs("custom-hold", undefined as unknown as string)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("exports no OTHER helper that maps a custom column id onto a legacy one", () => {
|
||||
/*
|
||||
Name-agnostic: exercise every exported single-argument function whose name mentions
|
||||
"column" and fail if it turns a valid custom id into a different, legacy id. That is
|
||||
the behaviour being banned, not the identifier.
|
||||
*/
|
||||
const legacy = new Set(["triage", "todo", "in-progress", "in-review", "done", "archived"]);
|
||||
const offenders: string[] = [];
|
||||
for (const [name, value] of Object.entries(core)) {
|
||||
if (typeof value !== "function" || !/column/i.test(name)) continue;
|
||||
/*
|
||||
NO ARITY FILTER (PR #2535 review — greptile). An earlier version skipped anything
|
||||
whose `Function.length !== 1`, which is the exact signature family this is meant to
|
||||
police: `normalizeColumnId(value, fallback = DEFAULT)` reports length 1 because
|
||||
defaults do not count, but a new coercer written `(value, fallback)` with both
|
||||
required reports 2 and would have walked straight past the guard. A ratchet that
|
||||
silently skips the shape it exists to catch is worse than no ratchet.
|
||||
|
||||
Probe with BOTH a custom id and a legacy fallback. One argument alone was not
|
||||
enough either: a two-required-argument coercer returns its (undefined) fallback,
|
||||
which is not a string, so the check skipped it — I verified that by injecting one.
|
||||
Supplying the fallback makes a lossy coercer return the LEGACY id, which is the
|
||||
signal; a passthrough returns the custom id; a one-argument function ignores the
|
||||
extra parameter harmlessly. Anything that cannot take this shape throws, and a
|
||||
function that rejects an unknown column is not silently losing it.
|
||||
*/
|
||||
let result: unknown;
|
||||
try {
|
||||
result = (value as (input: unknown, fallback: unknown) => unknown)("custom-hold", "triage");
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (typeof result === "string" && result !== "custom-hold" && legacy.has(result)) {
|
||||
offenders.push(name);
|
||||
}
|
||||
}
|
||||
expect(offenders).toEqual([]);
|
||||
});
|
||||
});
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -63,7 +63,7 @@ import {
|
||||
COLUMNS,
|
||||
DEFAULT_COLUMN,
|
||||
isColumn,
|
||||
normalizeColumn, normalizeColumnId,
|
||||
normalizeColumnId,
|
||||
TASK_PRIORITIES,
|
||||
DEFAULT_TASK_PRIORITY,
|
||||
} from "./types/board.js";
|
||||
@@ -73,7 +73,7 @@ export {
|
||||
COLUMNS,
|
||||
DEFAULT_COLUMN,
|
||||
isColumn,
|
||||
normalizeColumn, normalizeColumnId,
|
||||
normalizeColumnId,
|
||||
TASK_PRIORITIES,
|
||||
DEFAULT_TASK_PRIORITY,
|
||||
};
|
||||
|
||||
@@ -52,22 +52,32 @@ export function isColumn(value: unknown): value is Column {
|
||||
return typeof value === "string" && (COLUMNS as readonly string[]).includes(value);
|
||||
}
|
||||
|
||||
/**
|
||||
* @deprecated (workflowColumns, U12) Coerces an arbitrary value to a legacy
|
||||
* column, DISCARDING workflow-defined custom column ids — lossy under the
|
||||
* flag. Resolve and validate against the task's workflow instead. Retained
|
||||
* for the legacy flag-OFF path while the flag exists.
|
||||
*/
|
||||
export function normalizeColumn(value: unknown, fallback: Column = DEFAULT_COLUMN): Column {
|
||||
return isColumn(value) ? value : fallback;
|
||||
}
|
||||
/*
|
||||
FNXC:WorkflowColumns 2026-07-29-00:00 (U12 — R8):
|
||||
`normalizeColumn` is DELETED. It carried one of the two `@deprecated (workflowColumns,
|
||||
U12)` markers this unit was named for.
|
||||
|
||||
It coerced an arbitrary value to a LEGACY column, rewriting every workflow-defined
|
||||
custom id to `triage` — silent data loss for any project whose workflow declares a
|
||||
column outside the six built-ins. `normalizeColumnId` (retained, just below) is the
|
||||
non-lossy replacement: it sanitises structurally (non-string/empty -> fallback) and
|
||||
passes real ids through untouched.
|
||||
|
||||
Deleted rather than left deprecated because it had ZERO callers — the dashboard's
|
||||
ingest path and move handler both migrated to `normalizeColumnId` when the lossy
|
||||
behaviour was diagnosed (see `useTasks.ts` and `routes-trait-rekey.test.ts`, which pin
|
||||
that migration). Leaving an exported lossy coercion next to its safe twin is an
|
||||
invitation to pick the wrong one; `__tests__/no-lossy-column-coercion-export.test.ts`
|
||||
now ratchets that shut, by behaviour rather than by name.
|
||||
*/
|
||||
|
||||
|
||||
/*
|
||||
FNXC:WorkflowColumns 2026-07-19-2b:00 (U12 / R2 / R11):
|
||||
The workflow-aware counterpart to `normalizeColumn`, and the one client code should use when
|
||||
The workflow-aware column sanitiser — the one client code should use when
|
||||
sanitizing a column id off the wire.
|
||||
|
||||
`normalizeColumn` answers "is this one of the SIX legacy ids", so it silently rewrites every
|
||||
The deleted `normalizeColumn` answered "is this one of the SIX legacy ids", so it silently rewrote every
|
||||
workflow-defined id to `triage`. That is correct only for the closed default-workflow set; applied
|
||||
to a real board it teleports cards. A custom `merging` column's cards rendered in Triage because
|
||||
the dashboard ran every task through the legacy coercion on ingest.
|
||||
|
||||
@@ -82,7 +82,7 @@ function writeTaskCacheSnapshot(cacheKey: string, tasks: Task[]): boolean {
|
||||
FNXC:WorkflowColumns 2026-07-19-2b:05 (U12 / R2 / R11):
|
||||
Every task the dashboard ingests — initial list, SWR revalidation, and each SSE event — passes
|
||||
through here, so this one line decided whether custom columns exist in the UI at all. It used
|
||||
`normalizeColumn`, which keeps only the six legacy ids and rewrites everything else to `triage`:
|
||||
`normalizeColumn` (since DELETED in U12), which kept only the six legacy ids and rewrote everything else to `triage`:
|
||||
a card sitting in a user-authored `Merging` column rendered in Triage, and dragging it appeared to
|
||||
do nothing. The move handler below already worked around this for its own `to` id ("normalizeColumn
|
||||
alone would drop custom ids"), which fixed the symptom for one event and left the ingest path lossy.
|
||||
@@ -817,7 +817,7 @@ export function useTasks(options?: UseTasksOptions) {
|
||||
return;
|
||||
}
|
||||
// Preserve a custom (non-legacy) target id verbatim; only coerce empty/garbage
|
||||
// back to the task's current column. normalizeColumn alone would drop custom ids.
|
||||
// back to the task's current column. The old normalizeColumn (deleted in U12) would drop custom ids.
|
||||
const nextColumn: ColumnId = typeof to === "string" && to ? to : normalizedTask.column;
|
||||
const movedTask = { ...normalizedTask, column: nextColumn };
|
||||
setTasks((prev) => {
|
||||
|
||||
Reference in New Issue
Block a user