From 4dbf8737238ccc4dc2c67365e6d43f04f46c75b5 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 24 Jun 2026 16:40:07 +0200 Subject: [PATCH] fix(frontend): stop flow step id generation from being poisoned by non-canonical keys (#9766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(frontend): stop flow step id generation from being poisoned by non-canonical keys nextId computed the next step id from the max of charsToNumber over every module id and flowState key. Only canonical auto-ids (a, b, ... aa, ab) have a meaningful charsToNumber value, but flowState also holds copy ids ("z2"), subflow result keys ("subflow:..."), reserved keys ("failure"/"preprocessor") and user-renamed ids. The old `length >= 4` guard filtered long junk but let short junk through, so e.g. duplicating step "z" (key "z2", charsToNumber 629) made the next new step jump to "xg" and escalate from there. nextId now only counts a key if it round-trips through numberToChars and is not reserved, and the broken length cap is removed so large flows still get correct ids. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(frontend): keep length cap in nextId to avoid regressing long renames Address CI review: removing the length cap made all-lowercase renamed step ids (e.g. "process", which round-trips through numberToChars) feed into the max and poison id generation again — a regression versus the prior behavior, since step ids can be renamed to ^[a-zA-Z][a-zA-Z0-9_]*$. Restore the length>=4 skip and pair it with the round-trip canonical check, so short non-canonical keys (copy ids "z2"/"c10", reserved/renamed short ids) no longer poison the max while long renames stay out of the sequence. Update the tests to reflect the actual coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../components/flows/flowModuleNextId.test.ts | 53 +++++++++++++++++++ .../lib/components/flows/flowModuleNextId.ts | 30 ++++++++--- 2 files changed, 76 insertions(+), 7 deletions(-) create mode 100644 frontend/src/lib/components/flows/flowModuleNextId.test.ts diff --git a/frontend/src/lib/components/flows/flowModuleNextId.test.ts b/frontend/src/lib/components/flows/flowModuleNextId.test.ts new file mode 100644 index 0000000000..dcf007e925 --- /dev/null +++ b/frontend/src/lib/components/flows/flowModuleNextId.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest' + +import type { OpenFlow } from '$lib/gen' +import type { FlowState } from './flowState' +import { nextId } from './flowModuleNextId' + +function flowWith(ids: string[]): OpenFlow { + return { + summary: '', + value: { + modules: ids.map((id) => ({ id, value: { type: 'identity' } as any })) + } + } as OpenFlow +} + +function stateWith(keys: string[]): FlowState { + return Object.fromEntries(keys.map((k) => [k, {}])) as FlowState +} + +describe('nextId', () => { + it('produces a, b, c, ... for a fresh flow', () => { + expect(nextId(stateWith(['failure']), flowWith([]))).toBe('a') + expect(nextId(stateWith(['a', 'failure']), flowWith(['a']))).toBe('b') + expect(nextId(stateWith(['a', 'b', 'c', 'failure']), flowWith(['a', 'b', 'c']))).toBe('d') + }) + + it('ignores the reserved failure/preprocessor keys always present in flowState', () => { + expect(nextId(stateWith(['failure', 'preprocessor']), flowWith([]))).toBe('a') + }) + + // Regression: copy ids ("z2"), subflow result keys and other non-canonical keys land in + // flowState; charsToNumber on them used to leak into the max and made new steps jump to + // garbage ids like "bzw". + it('is not poisoned by copy ids', () => { + const ids = ['a', 'b', 'c'] + const state = stateWith([...ids, 'c2', 'a2', 'z2', 'c10', 'failure']) + expect(nextId(state, flowWith(ids))).toBe('d') + }) + + it('is not poisoned by subflow result keys', () => { + const ids = ['a', 'b'] + const state = stateWith([...ids, 'subflow:abcd', 'Result', 'failure']) + expect(nextId(state, flowWith(ids))).toBe('c') + }) + + // A step renamed to a long lowercase word ("process") is a valid base-26 string and would + // otherwise inflate the max; the length cutoff keeps such renames out of the sequence. + it('is not poisoned by renames to long lowercase words or underscored ids', () => { + const ids = ['a', 'b'] + const state = stateWith([...ids, 'process', 'my_step', 'failure']) + expect(nextId(state, flowWith(ids))).toBe('c') + }) +}) diff --git a/frontend/src/lib/components/flows/flowModuleNextId.ts b/frontend/src/lib/components/flows/flowModuleNextId.ts index e10c900982..48b2eb5ac2 100644 --- a/frontend/src/lib/components/flows/flowModuleNextId.ts +++ b/frontend/src/lib/components/flows/flowModuleNextId.ts @@ -1,19 +1,35 @@ import type { OpenFlow } from '$lib/gen' import { dfs } from './dfs' import type { FlowState } from './flowState' -import { charsToNumber, numberToChars } from './idUtils' +import { charsToNumber, forbiddenIds, numberToChars } from './idUtils' + +const reservedIds = new Set(forbiddenIds) + +// Returns the base-26 value of a key only if it is a short, auto-generated step id +// (a, b, ..., z, aa, ...). flowState/module-id keys also include copy ids ("a2"), subflow +// result keys ("subflow:..."), reserved keys and user-renamed ids; feeding those through +// charsToNumber yields meaningless (often huge) numbers that would poison id generation and +// make new steps jump to ids like "bzw". Short non-canonical keys are rejected via a +// round-trip check; longer keys are skipped entirely, which also leaves user renames to long +// lowercase words (e.g. "process") out of the sequence. +function autoIdNumber(key: string): number | undefined { + if (key.length >= 4 || reservedIds.has(key)) { + return undefined + } + const num = charsToNumber(key) + if (num < 0 || numberToChars(num) !== key) { + return undefined + } + return num +} // Computes the next available id export function nextId(flowState: FlowState, fullFlow: OpenFlow): string { const allIds = dfs(fullFlow.value.modules, (fm) => fm.id) const max = allIds.concat(Object.keys(flowState)).reduce((acc, key) => { - if (key.length >= 4) { - return acc - } else { - const num = charsToNumber(key) - return Math.max(acc, num + 1) - } + const num = autoIdNumber(key) + return num === undefined ? acc : Math.max(acc, num + 1) }, 0) return numberToChars(max) }