From 123c08acc299b435ef98cfb8161d125ee406c790 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 8 Sep 2026 23:28:30 +0000 Subject: [PATCH 1/7] Keep the step ids a project already has, rather than deriving them again A step's id is worked out from its name on every pull, and that conversion drops anything not url-safe. Two names that differ only in an emoji or an accent shorten to the same id, so the second step is written over the first. Two steps go in and one file comes out, with no warning. The file already holds each step's id and the reader already takes it as written. Only the pull side ignores it. So a pull now reads the ids the checked-out project has already given its steps and keeps them, deriving one only for a step it has not seen. A derived id that is taken gets a numeric suffix, and new steps are handled in uuid order so two people pulling the same project separately reach the same answer. Every name in a project today is ascii, so a kept id and a derived one are the same string and nothing moves for anyone. Edges named their target by deriving the target's name again. They now use the resolved id, so a reference cannot disagree with the step it points at. Ids are matched by name because the workflow file carries no uuids. That is why renaming a step still moves it. Tying an id to a step across a rename needs the uuid written down, which is a change to the file format and a separate question. --- .changeset/keep-recorded-step-ids.md | 6 ++ packages/cli/src/projects/fetch.ts | 7 +- packages/project/src/index.ts | 2 +- packages/project/src/parse/from-app-state.ts | 79 +++++++++++++++- .../project/test/parse/from-app-state.test.ts | 92 +++++++++++++++++++ 5 files changed, 179 insertions(+), 7 deletions(-) create mode 100644 .changeset/keep-recorded-step-ids.md diff --git a/.changeset/keep-recorded-step-ids.md b/.changeset/keep-recorded-step-ids.md new file mode 100644 index 000000000..cd99504d7 --- /dev/null +++ b/.changeset/keep-recorded-step-ids.md @@ -0,0 +1,6 @@ +--- +'@openfn/project': patch +'@openfn/cli': patch +--- + +Keep the step ids a project already has on a pull, so two names that shorten to the same id no longer overwrite each other diff --git a/packages/cli/src/projects/fetch.ts b/packages/cli/src/projects/fetch.ts index e87f0b3fa..9360d8a61 100644 --- a/packages/cli/src/projects/fetch.ts +++ b/packages/cli/src/projects/fetch.ts @@ -1,6 +1,6 @@ import yargs from 'yargs'; import path from 'node:path'; -import Project, { Workspace } from '@openfn/project'; +import Project, { Workspace, recordedStepIdsOf } from '@openfn/project'; import { build, ensure, override } from '../util/command-builders'; import type { Logger } from '../util/logger'; @@ -286,6 +286,11 @@ export async function fetchRemoteProject( { ...workspace.getConfig(), alias: options.alias ?? localProject?.alias ?? 'main', + // Keep the ids the checked-out project already gave its steps, so a name + // that shortens to the same id as another does not overwrite it. + recordedStepIds: localProject + ? recordedStepIdsOf(localProject) + : undefined, } ); diff --git a/packages/project/src/index.ts b/packages/project/src/index.ts index dbcd401a6..3640bc0b9 100644 --- a/packages/project/src/index.ts +++ b/packages/project/src/index.ts @@ -22,7 +22,7 @@ export { match as versionsEqual, } from './util/version'; -export { mapWorkflow } from './parse/from-app-state'; +export { mapWorkflow, recordedStepIdsOf } from './parse/from-app-state'; export { default as detectVersion } from './util/detect-version'; diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index 7f68ac9aa..9f70657bd 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/packages/project/src/parse/from-app-state.ts @@ -13,6 +13,15 @@ import getCredentialName from '../util/get-credential-name'; export type fromAppStateConfig = Partial & { format?: 'yaml' | 'json'; alias?: string; + + // Step ids already written down locally, keyed by step name. An id we have + // used before wins over one derived from the name, so two names that shorten + // to the same thing cannot land on top of each other. + // + // Keyed by name rather than by the step's uuid because the workflow file does + // not carry uuids. That is why a rename still moves a step: there is nothing + // on disk tying the new name to the old id. + recordedStepIds?: Record; }; export default ( @@ -78,7 +87,7 @@ export default ( } proj.workflows = Object.values(stateJson.workflows).map((w) => - mapWorkflow(w, proj.credentials) + mapWorkflow(w, proj.credentials, config.recordedStepIds) ); return new Project(proj as l.ProjectState, config); @@ -109,14 +118,74 @@ export const mapEdge = (edge: Provisioner.Edge) => { return e; }; + + +// The ids a project on disk has already given its steps, keyed by step name. +// Feed this back into a pull so a step keeps the id it already has. +export const recordedStepIdsOf = (project: any): Record => { + const recorded: Record = {}; + for (const workflow of project?.workflows ?? []) { + for (const step of workflow?.steps ?? []) { + if (step?.name && step?.id) { + recorded[step.name] = step.id; + } + } + } + return recorded; +}; + +// Work out one id per step, before anything refers to them. +// +// Deriving an id from a name loses whatever is not url-safe, so two names that +// differ only in emoji or accents shorten to the same thing and one step lands +// on top of the other. An id we have already written down is kept. Anything +// new is derived and then made unique, in uuid order, so two people pulling the +// same project separately reach the same answer without talking to each other. +export const resolveStepIds = ( + jobs: Record, + recorded: Record = {} +): Record => { + const byUuid: Record = {}; + const taken = new Set(); + + const all = Object.values(jobs); + + for (const job of all) { + const known = recorded[job.name]; + if (known && !taken.has(known)) { + byUuid[job.id] = known; + taken.add(known); + } + } + + const fresh = all + .filter((job) => !byUuid[job.id]) + .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); + + for (const job of fresh) { + const base = slugify(job.name) || 'step'; + let candidate = base; + let n = 2; + while (taken.has(candidate)) { + candidate = `${base}-${n++}`; + } + byUuid[job.id] = candidate; + taken.add(candidate); + } + + return byUuid; +}; + // map a project workflow to a local cli workflow // TODO this probably gets easier if I index everything by name export const mapWorkflow = ( workflow: Provisioner.Workflow, - credentials: l.CredentialState[] = [] + credentials: l.CredentialState[] = [], + recordedStepIds: Record = {} ) => { const { jobs, edges, triggers, name, version_history, ...remoteProps } = workflow; + const stepIds = resolveStepIds(jobs, recordedStepIds); const mapped: l.WorkflowState = { name: workflow.name, steps: [], @@ -166,7 +235,7 @@ export const mapWorkflow = ( throw new Error(`Failed to find ${edge.target_job_id}`); } // we use the name, not the id, to reference - obj[slugify(target.name)] = mapEdge(edge); + obj[stepIds[target.id]] = mapEdge(edge); return obj; }, {}), }) as l.Trigger @@ -187,7 +256,7 @@ export const mapWorkflow = ( } = step; const s: any /*l.Job*/ = { - id: slugify(name), + id: stepIds[step.id], name: name, expression, adaptor, // TODO is this wrong? @@ -210,7 +279,7 @@ export const mapWorkflow = ( (j) => j.id === edge.target_job_id ); // @ts-ignore - next[slugify(target.name)] = mapEdge(edge); + next[stepIds[target.id]] = mapEdge(edge); return next; }, {}); } diff --git a/packages/project/test/parse/from-app-state.test.ts b/packages/project/test/parse/from-app-state.test.ts index f1db229bb..c7ea38cd1 100644 --- a/packages/project/test/parse/from-app-state.test.ts +++ b/packages/project/test/parse/from-app-state.test.ts @@ -2,6 +2,7 @@ import test from 'ava'; import fromAppState, { mapEdge, mapWorkflow, + recordedStepIdsOf, } from '../../src/parse/from-app-state'; import { cloneDeep } from 'lodash-es'; @@ -548,3 +549,94 @@ workflows: t.is(project.workflows[0].name, 'wf1'); t.is(project.workflows[0].steps.length, 2); }); + +// --- recorded step ids --------------------------------------------------- +// +// An id derived from a name loses whatever is not url-safe, so two names that +// differ only in emoji or accents used to land on the same id and one step's +// code was silently dropped on the way to disk. + +const twoStepsSharingAnId = () => ({ + id: 'p1', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'wf-uuid', + name: 'My Workflow', + triggers: {}, + edges: {}, + jobs: { + j1: { id: 'bbbb-2222', name: 'step 👍', body: 'up()', adaptor: 'common' }, + j2: { id: 'aaaa-1111', name: 'step 👎', body: 'down()', adaptor: 'common' }, + }, + }, + ], +}); + +const stepIdsOf = (project: any) => + (Object.values(project.workflows)[0] as any).steps.map((s: any) => s.id); + +test('two names that shorten to the same id still get distinct ids', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta); + + const ids = stepIdsOf(project); + + t.is(ids.length, 2); + t.is(new Set(ids).size, 2); +}); + +test('both step bodies reach the filesystem', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta); + + const files = project.serialize('fs') as Record; + const scripts = Object.keys(files).filter((f) => f.endsWith('.js')); + + t.is(scripts.length, 2); + t.true(Object.values(files).some((c) => c.includes('up()'))); + t.true(Object.values(files).some((c) => c.includes('down()'))); +}); + +test('an id we have already written down is kept', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedStepIds: { 'step \u{1F44D}': 'thumbs-up', 'step \u{1F44E}': 'thumbs-down' }, + }); + + t.deepEqual(stepIdsOf(project).sort(), ['thumbs-down', 'thumbs-up']); +}); + +test('a new step works around an id that is already taken', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedStepIds: { 'step \u{1F44E}': 'step' }, + }); + + const ids = stepIdsOf(project); + + t.true(ids.includes('step')); + t.is(new Set(ids).size, 2); +}); + +test('two people pulling separately arrive at the same ids', (t) => { + const first = stepIdsOf(fromAppState(twoStepsSharingAnId() as any, meta)); + + // the same workflow, with the jobs arriving the other way round + const reordered: any = twoStepsSharingAnId(); + reordered.workflows[0].jobs = { + j2: reordered.workflows[0].jobs.j2, + j1: reordered.workflows[0].jobs.j1, + }; + const second = stepIdsOf(fromAppState(reordered, meta)); + + t.deepEqual(first.sort(), second.sort()); +}); + +test('a project on disk hands back the ids it already gave its steps', (t) => { + const first = fromAppState(twoStepsSharingAnId() as any, meta); + + const recorded = recordedStepIdsOf(first); + const second = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedStepIds: recorded, + }); + + t.deepEqual(stepIdsOf(first).sort(), stepIdsOf(second).sort()); +}); From c998eb07bbd1cd8b5a478e01cfb65c640ffacd9d Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 00:00:53 +0000 Subject: [PATCH 2/7] Harden the recorded ids against the file they come from Review found three ways this could go wrong, two of them mine. An id read back from a project file went straight into a path. A workflow file can come from a repository somebody else prepared, so a step id of ../../.. was written outside the project. Deriving the id on every pull used to launder that; keeping it removed the only guard. Ids that are not already url-safe are now ignored. A step named constructor or toString found a function on Object.prototype, so both steps ended up sharing an id and one lost its body. That fired on an ordinary pull with nobody attacking anything. Ids are recorded per workflow rather than in one flat map, since two workflows can each hold a step of the same name, and both passes now run in uuid order so the answer does not depend on how the server listed the jobs. A trigger's id is its type and it shares the directory, so those are reserved before steps are given theirs. The map no longer leaks into the workspace config. --- packages/cli/src/projects/fetch.ts | 5 +- packages/project/src/index.ts | 1 + packages/project/src/parse/from-app-state.ts | 84 ++++++--- .../project/test/parse/from-app-state.test.ts | 163 ++++++++++++++---- 4 files changed, 193 insertions(+), 60 deletions(-) diff --git a/packages/cli/src/projects/fetch.ts b/packages/cli/src/projects/fetch.ts index 9360d8a61..0d36b0fae 100644 --- a/packages/cli/src/projects/fetch.ts +++ b/packages/cli/src/projects/fetch.ts @@ -286,8 +286,9 @@ export async function fetchRemoteProject( { ...workspace.getConfig(), alias: options.alias ?? localProject?.alias ?? 'main', - // Keep the ids the checked-out project already gave its steps, so a name - // that shortens to the same id as another does not overwrite it. + // Keep the ids the project we resolved from the identifier already gave + // its steps, so a name that shortens to the same id as another does not + // overwrite it. Ids that are not already url-safe are ignored. recordedStepIds: localProject ? recordedStepIdsOf(localProject) : undefined, diff --git a/packages/project/src/index.ts b/packages/project/src/index.ts index 3640bc0b9..c465ea4a8 100644 --- a/packages/project/src/index.ts +++ b/packages/project/src/index.ts @@ -23,6 +23,7 @@ export { } from './util/version'; export { mapWorkflow, recordedStepIdsOf } from './parse/from-app-state'; +export type { RecordedStepIds } from './parse/from-app-state'; export { default as detectVersion } from './util/detect-version'; diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index 9f70657bd..a74fd6604 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/packages/project/src/parse/from-app-state.ts @@ -21,7 +21,7 @@ export type fromAppStateConfig = Partial & { // Keyed by name rather than by the step's uuid because the workflow file does // not carry uuids. That is why a rename still moves a step: there is nothing // on disk tying the new name to the old id. - recordedStepIds?: Record; + recordedStepIds?: RecordedStepIds; }; export default ( @@ -31,6 +31,9 @@ export default ( ) => { let stateJson = ensureJson(state); delete config.format; + // Not workspace config, so keep it out of what gets written back. + const { recordedStepIds, ...projectConfig } = config; + config = projectConfig; const { id, @@ -87,7 +90,7 @@ export default ( } proj.workflows = Object.values(stateJson.workflows).map((w) => - mapWorkflow(w, proj.credentials, config.recordedStepIds) + mapWorkflow(w, proj.credentials, recordedStepIds?.[w.name]) ); return new Project(proj as l.ProjectState, config); @@ -118,18 +121,33 @@ export const mapEdge = (edge: Provisioner.Edge) => { return e; }; - - -// The ids a project on disk has already given its steps, keyed by step name. -// Feed this back into a pull so a step keeps the id it already has. -export const recordedStepIdsOf = (project: any): Record => { - const recorded: Record = {}; +// Ids recorded per workflow, since two workflows can each hold a step of the +// same name and their files sit in different directories. Keyed by workflow +// name, and then step name, because the files carry no uuids to match on. +export type RecordedStepIds = Record>; + +// An id has to survive being used as a directory and a file name. Anything a +// project file offers us that is not already url-safe is ignored rather than +// trusted: a workflow file can come from a repository somebody else prepared, +// and `../../..` in a step id would otherwise be written straight to disk. +const isSafeId = (id: unknown): id is string => + typeof id === 'string' && id.length > 0 && slugify(id) === id; + +// The ids a project on disk has already given its steps, keyed by workflow id +// and then step name. Feed this back into a pull so a step keeps its id. +export const recordedStepIdsOf = (project: { + workflows?: { name?: string; steps?: { id?: string; name?: string }[] }[]; +}): RecordedStepIds => { + const recorded: RecordedStepIds = Object.create(null); for (const workflow of project?.workflows ?? []) { - for (const step of workflow?.steps ?? []) { - if (step?.name && step?.id) { - recorded[step.name] = step.id; + if (!workflow?.name) continue; + const forWorkflow: Record = Object.create(null); + for (const step of workflow.steps ?? []) { + if (step?.name && isSafeId(step.id)) { + forWorkflow[step.name] = step.id!; } } + recorded[workflow.name] = forWorkflow; } return recorded; }; @@ -138,31 +156,46 @@ export const recordedStepIdsOf = (project: any): Record => { // // Deriving an id from a name loses whatever is not url-safe, so two names that // differ only in emoji or accents shorten to the same thing and one step lands -// on top of the other. An id we have already written down is kept. Anything -// new is derived and then made unique, in uuid order, so two people pulling the -// same project separately reach the same answer without talking to each other. +// on top of the other. An id we have already written down is kept. Anything new +// is derived and then made unique. +// +// Both passes run in uuid order so the answer does not depend on the order the +// server happened to list the jobs in. Two people pulling the same project reach +// the same ids, so long as they start from the same recorded ones. export const resolveStepIds = ( jobs: Record, + triggers: Record = {}, recorded: Record = {} ): Record => { - const byUuid: Record = {}; + const byUuid: Record = Object.create(null); const taken = new Set(); - const all = Object.values(jobs); + // A trigger's id is its type, and it shares the directory with the steps, so + // a job called "Webhook" must not be handed the same id. + for (const trigger of Object.values(triggers)) { + if (trigger?.type) { + taken.add(trigger.type); + } + } - for (const job of all) { - const known = recorded[job.name]; - if (known && !taken.has(known)) { + const inUuidOrder = Object.values(jobs) + .slice() + .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); + + for (const job of inUuidOrder) { + // hasOwnProperty via call, so a step called `constructor` or `toString` + // does not find a function on the prototype and hand every step the same id. + const known = Object.prototype.hasOwnProperty.call(recorded, job.name) + ? recorded[job.name] + : undefined; + if (isSafeId(known) && !taken.has(known)) { byUuid[job.id] = known; taken.add(known); } } - const fresh = all - .filter((job) => !byUuid[job.id]) - .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); - - for (const job of fresh) { + for (const job of inUuidOrder) { + if (byUuid[job.id]) continue; const base = slugify(job.name) || 'step'; let candidate = base; let n = 2; @@ -185,7 +218,7 @@ export const mapWorkflow = ( ) => { const { jobs, edges, triggers, name, version_history, ...remoteProps } = workflow; - const stepIds = resolveStepIds(jobs, recordedStepIds); + const stepIds = resolveStepIds(jobs, triggers, recordedStepIds); const mapped: l.WorkflowState = { name: workflow.name, steps: [], @@ -234,7 +267,6 @@ export const mapWorkflow = ( if (!target) { throw new Error(`Failed to find ${edge.target_job_id}`); } - // we use the name, not the id, to reference obj[stepIds[target.id]] = mapEdge(edge); return obj; }, {}), diff --git a/packages/project/test/parse/from-app-state.test.ts b/packages/project/test/parse/from-app-state.test.ts index c7ea38cd1..199c915fe 100644 --- a/packages/project/test/parse/from-app-state.test.ts +++ b/packages/project/test/parse/from-app-state.test.ts @@ -553,8 +553,8 @@ workflows: // --- recorded step ids --------------------------------------------------- // // An id derived from a name loses whatever is not url-safe, so two names that -// differ only in emoji or accents used to land on the same id and one step's -// code was silently dropped on the way to disk. +// differ only in emoji or an accent used to land on the same id and one step's +// code was written over the other on the way to disk. const twoStepsSharingAnId = () => ({ id: 'p1', @@ -567,23 +567,37 @@ const twoStepsSharingAnId = () => ({ triggers: {}, edges: {}, jobs: { - j1: { id: 'bbbb-2222', name: 'step 👍', body: 'up()', adaptor: 'common' }, - j2: { id: 'aaaa-1111', name: 'step 👎', body: 'down()', adaptor: 'common' }, + j1: { + id: 'bbbb', + name: 'step \u{1F44D}', + body: 'up()', + adaptor: 'common', + }, + j2: { + id: 'aaaa', + name: 'step \u{1F44E}', + body: 'down()', + adaptor: 'common', + }, }, }, ], }); -const stepIdsOf = (project: any) => - (Object.values(project.workflows)[0] as any).steps.map((s: any) => s.id); +// name -> id, so a test can tell "both are distinct" from "they swapped" +const idsByName = (project: any, index = 0) => + Object.fromEntries( + (Object.values(project.workflows)[index] as any).steps.map((s: any) => [ + s.name, + s.id, + ]) + ); test('two names that shorten to the same id still get distinct ids', (t) => { - const project = fromAppState(twoStepsSharingAnId() as any, meta); + const ids = idsByName(fromAppState(twoStepsSharingAnId() as any, meta)); - const ids = stepIdsOf(project); - - t.is(ids.length, 2); - t.is(new Set(ids).size, 2); + t.is(Object.keys(ids).length, 2); + t.is(new Set(Object.values(ids)).size, 2); }); test('both step bodies reach the filesystem', (t) => { @@ -599,44 +613,129 @@ test('both step bodies reach the filesystem', (t) => { test('an id we have already written down is kept', (t) => { const project = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: { 'step \u{1F44D}': 'thumbs-up', 'step \u{1F44E}': 'thumbs-down' }, + recordedStepIds: { + 'My Workflow': { + 'step \u{1F44D}': 'thumbs-up', + 'step \u{1F44E}': 'thumbs-down', + }, + }, }); - t.deepEqual(stepIdsOf(project).sort(), ['thumbs-down', 'thumbs-up']); + t.deepEqual(idsByName(project), { + 'step \u{1F44D}': 'thumbs-up', + 'step \u{1F44E}': 'thumbs-down', + }); }); -test('a new step works around an id that is already taken', (t) => { - const project = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: { 'step \u{1F44E}': 'step' }, - }); +test('a step keeps its own id rather than swapping with its neighbour', (t) => { + const state: any = twoStepsSharingAnId(); + const reversed: any = twoStepsSharingAnId(); + reversed.workflows[0].jobs = { + j2: reversed.workflows[0].jobs.j2, + j1: reversed.workflows[0].jobs.j1, + }; + + t.deepEqual( + idsByName(fromAppState(state, meta)), + idsByName(fromAppState(reversed, meta)) + ); +}); + +test('a step named after a trigger does not take the trigger id', (t) => { + const state: any = twoStepsSharingAnId(); + state.workflows[0].triggers = { + t: { id: 'tttt', type: 'webhook', enabled: true }, + }; + state.workflows[0].jobs.j1.name = 'Webhook'; + + const project = fromAppState(state, meta); + const files = project.serialize('fs') as Record; + + t.not(idsByName(project)['Webhook'], 'webhook'); + t.is(Object.keys(files).filter((f) => f.endsWith('.js')).length, 2); +}); + +test('two workflows can each hold a step of the same name', (t) => { + const state: any = { + id: 'p', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'w1', + name: 'One', + triggers: {}, + edges: {}, + jobs: { + a: { id: 'u-a', name: 'Transform', body: 'one()', adaptor: 'c' }, + }, + }, + { + id: 'w2', + name: 'Two', + triggers: {}, + edges: {}, + jobs: { + b: { id: 'u-b', name: 'Transform', body: 'two()', adaptor: 'c' }, + }, + }, + ], + }; + + const first = fromAppState(state, meta); + const recorded = recordedStepIdsOf(first); - const ids = stepIdsOf(project); + // each workflow keeps its own entry rather than one overwriting the other + t.deepEqual(Object.keys(recorded).sort(), ['One', 'Two']); + + const second = fromAppState(state, meta, { recordedStepIds: recorded }); + t.deepEqual(idsByName(second, 0), idsByName(first, 0)); + t.deepEqual(idsByName(second, 1), idsByName(first, 1)); +}); + +test('a step named like an object property does not pick one up', (t) => { + const state: any = twoStepsSharingAnId(); + state.workflows[0].jobs.j1.name = 'constructor'; + state.workflows[0].jobs.j2.name = 'toString'; + + const project = fromAppState(state, meta); + const ids = Object.values(idsByName(project)); + const files = project.serialize('fs') as Record; - t.true(ids.includes('step')); + t.true(ids.every((id) => typeof id === 'string' && id.length > 0)); t.is(new Set(ids).size, 2); + t.is(Object.keys(files).filter((f) => f.endsWith('.js')).length, 2); }); -test('two people pulling separately arrive at the same ids', (t) => { - const first = stepIdsOf(fromAppState(twoStepsSharingAnId() as any, meta)); +test('an id from a project file that escapes the directory is ignored', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedStepIds: { + 'My Workflow': { 'step \u{1F44D}': '../../../../tmp/pwned' }, + }, + }); - // the same workflow, with the jobs arriving the other way round - const reordered: any = twoStepsSharingAnId(); - reordered.workflows[0].jobs = { - j2: reordered.workflows[0].jobs.j2, - j1: reordered.workflows[0].jobs.j1, - }; - const second = stepIdsOf(fromAppState(reordered, meta)); + const files = project.serialize('fs') as Record; - t.deepEqual(first.sort(), second.sort()); + t.false(Object.values(idsByName(project)).includes('../../../../tmp/pwned')); + t.false(Object.keys(files).some((f) => f.includes('..'))); }); test('a project on disk hands back the ids it already gave its steps', (t) => { const first = fromAppState(twoStepsSharingAnId() as any, meta); - const recorded = recordedStepIdsOf(first); const second = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: recorded, + recordedStepIds: recordedStepIdsOf(first), }); - t.deepEqual(stepIdsOf(first).sort(), stepIdsOf(second).sort()); + t.deepEqual(idsByName(second), idsByName(first)); +}); + +test('the recorded ids are not written into the workspace config', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedStepIds: { 'My Workflow': { 'step \u{1F44D}': 'thumbs-up' } }, + }); + + const files = project.serialize('fs') as Record; + + t.false(JSON.stringify(files).includes('recordedStepIds')); }); From 6062968d77b0328bf36c4f1a6607053aec9ab980 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 00:11:39 +0000 Subject: [PATCH 3/7] Cut the comments back to what the code cannot say --- packages/cli/src/projects/fetch.ts | 4 +- packages/project/src/parse/from-app-state.ts | 42 +++++--------------- 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/packages/cli/src/projects/fetch.ts b/packages/cli/src/projects/fetch.ts index 0d36b0fae..fd46a561a 100644 --- a/packages/cli/src/projects/fetch.ts +++ b/packages/cli/src/projects/fetch.ts @@ -286,9 +286,7 @@ export async function fetchRemoteProject( { ...workspace.getConfig(), alias: options.alias ?? localProject?.alias ?? 'main', - // Keep the ids the project we resolved from the identifier already gave - // its steps, so a name that shortens to the same id as another does not - // overwrite it. Ids that are not already url-safe are ignored. + // Keep the ids this project already gave its steps. recordedStepIds: localProject ? recordedStepIdsOf(localProject) : undefined, diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index a74fd6604..cea358f93 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/packages/project/src/parse/from-app-state.ts @@ -14,13 +14,6 @@ export type fromAppStateConfig = Partial & { format?: 'yaml' | 'json'; alias?: string; - // Step ids already written down locally, keyed by step name. An id we have - // used before wins over one derived from the name, so two names that shorten - // to the same thing cannot land on top of each other. - // - // Keyed by name rather than by the step's uuid because the workflow file does - // not carry uuids. That is why a rename still moves a step: there is nothing - // on disk tying the new name to the old id. recordedStepIds?: RecordedStepIds; }; @@ -31,7 +24,6 @@ export default ( ) => { let stateJson = ensureJson(state); delete config.format; - // Not workspace config, so keep it out of what gets written back. const { recordedStepIds, ...projectConfig } = config; config = projectConfig; @@ -121,20 +113,16 @@ export const mapEdge = (edge: Provisioner.Edge) => { return e; }; -// Ids recorded per workflow, since two workflows can each hold a step of the -// same name and their files sit in different directories. Keyed by workflow -// name, and then step name, because the files carry no uuids to match on. +// Keyed by workflow name, then step name. The files carry no uuids to match on, +// which is why a rename still moves a step. export type RecordedStepIds = Record>; -// An id has to survive being used as a directory and a file name. Anything a -// project file offers us that is not already url-safe is ignored rather than -// trusted: a workflow file can come from a repository somebody else prepared, -// and `../../..` in a step id would otherwise be written straight to disk. +// An id becomes a directory and a file name, and a workflow file can come from +// a repository somebody else prepared, so `../../..` is not to be trusted. const isSafeId = (id: unknown): id is string => typeof id === 'string' && id.length > 0 && slugify(id) === id; -// The ids a project on disk has already given its steps, keyed by workflow id -// and then step name. Feed this back into a pull so a step keeps its id. +// Feed this back into a pull so a step keeps the id it already has. export const recordedStepIdsOf = (project: { workflows?: { name?: string; steps?: { id?: string; name?: string }[] }[]; }): RecordedStepIds => { @@ -152,16 +140,10 @@ export const recordedStepIdsOf = (project: { return recorded; }; -// Work out one id per step, before anything refers to them. -// -// Deriving an id from a name loses whatever is not url-safe, so two names that -// differ only in emoji or accents shorten to the same thing and one step lands -// on top of the other. An id we have already written down is kept. Anything new -// is derived and then made unique. -// -// Both passes run in uuid order so the answer does not depend on the order the -// server happened to list the jobs in. Two people pulling the same project reach -// the same ids, so long as they start from the same recorded ones. +// One id per step, resolved before anything refers to them. Deriving from the +// name loses whatever is not url-safe, so two names differing only in an emoji +// collapse onto each other. Both passes run in uuid order so the result does not +// depend on how the server listed the jobs. export const resolveStepIds = ( jobs: Record, triggers: Record = {}, @@ -170,8 +152,7 @@ export const resolveStepIds = ( const byUuid: Record = Object.create(null); const taken = new Set(); - // A trigger's id is its type, and it shares the directory with the steps, so - // a job called "Webhook" must not be handed the same id. + // A trigger's id is its type and shares the directory, so reserve those first. for (const trigger of Object.values(triggers)) { if (trigger?.type) { taken.add(trigger.type); @@ -183,8 +164,7 @@ export const resolveStepIds = ( .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); for (const job of inUuidOrder) { - // hasOwnProperty via call, so a step called `constructor` or `toString` - // does not find a function on the prototype and hand every step the same id. + // own property only, so a step called `toString` finds nothing. const known = Object.prototype.hasOwnProperty.call(recorded, job.name) ? recorded[job.name] : undefined; From 1e81a6cd1f13560d35a3aa51c85889933c4114c0 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 00:21:55 +0000 Subject: [PATCH 4/7] Drop a guard isSafeId already covers, and two comments that narrate The hasOwnProperty call only mattered if a prototype member could pass isSafeId, and it cannot: a function is not a string. Mutating it away failed nothing, so it was a comment explaining code that did not need to be there. --- .changeset/keep-recorded-step-ids.md | 2 +- packages/project/src/parse/from-app-state.ts | 12 +++--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/.changeset/keep-recorded-step-ids.md b/.changeset/keep-recorded-step-ids.md index cd99504d7..5b0a4bb6a 100644 --- a/.changeset/keep-recorded-step-ids.md +++ b/.changeset/keep-recorded-step-ids.md @@ -3,4 +3,4 @@ '@openfn/cli': patch --- -Keep the step ids a project already has on a pull, so two names that shorten to the same id no longer overwrite each other +Keep the step ids a project already has when pulling diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index cea358f93..5370ca363 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/packages/project/src/parse/from-app-state.ts @@ -122,7 +122,6 @@ export type RecordedStepIds = Record>; const isSafeId = (id: unknown): id is string => typeof id === 'string' && id.length > 0 && slugify(id) === id; -// Feed this back into a pull so a step keeps the id it already has. export const recordedStepIdsOf = (project: { workflows?: { name?: string; steps?: { id?: string; name?: string }[] }[]; }): RecordedStepIds => { @@ -140,10 +139,8 @@ export const recordedStepIdsOf = (project: { return recorded; }; -// One id per step, resolved before anything refers to them. Deriving from the -// name loses whatever is not url-safe, so two names differing only in an emoji -// collapse onto each other. Both passes run in uuid order so the result does not -// depend on how the server listed the jobs. +// Both passes run in uuid order so the result does not depend on how the server +// listed the jobs. export const resolveStepIds = ( jobs: Record, triggers: Record = {}, @@ -164,10 +161,7 @@ export const resolveStepIds = ( .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); for (const job of inUuidOrder) { - // own property only, so a step called `toString` finds nothing. - const known = Object.prototype.hasOwnProperty.call(recorded, job.name) - ? recorded[job.name] - : undefined; + const known = recorded[job.name]; if (isSafeId(known) && !taken.has(known)) { byUuid[job.id] = known; taken.add(known); From 25ad9fd8429ae090fdafd23d481b02bd92b96e4f Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 00:26:56 +0000 Subject: [PATCH 5/7] Pin the rename behaviour with a test The PR body told QA to expect a rename to move the step. That was a prediction from how the matching works, not something anyone had run. --- .../project/test/parse/from-app-state.test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/project/test/parse/from-app-state.test.ts b/packages/project/test/parse/from-app-state.test.ts index 199c915fe..f8f051ff9 100644 --- a/packages/project/test/parse/from-app-state.test.ts +++ b/packages/project/test/parse/from-app-state.test.ts @@ -739,3 +739,30 @@ test('the recorded ids are not written into the workspace config', (t) => { t.false(JSON.stringify(files).includes('recordedStepIds')); }); + +test('renaming a step still moves its id', (t) => { + const withName = (name: string) => ({ + id: 'p', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'w', + name: 'My Workflow', + triggers: {}, + edges: {}, + jobs: { a: { id: 'u-a', name, body: 'code()', adaptor: 'c' } }, + }, + ], + }); + + const first = fromAppState(withName('Send data') as any, meta); + const recorded = recordedStepIdsOf(first); + + const renamed = fromAppState(withName('Send data v2') as any, meta, { + recordedStepIds: recorded, + }); + + t.is(idsByName(first)['Send data'], 'send-data'); + t.is(idsByName(renamed)['Send data v2'], 'send-data-v2'); +}); From 7fa5e1a1c89aadbd5e114e02c39530917e80db3e Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 01:35:16 +0000 Subject: [PATCH 6/7] Give workflows the same treatment as steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two workflows called `wf 👍` and `wf 👎` both slugify to `wf`, and serialize then throws `step not found`. So a pull crashes rather than quietly losing a step, and unlike the step case this one is reachable today, since workflow names have never been restricted. The id resolution is now shared, so a workflow keeps the id it already has and only gets a new one when it has not been seen. Recorded ids carry workflow ids alongside step ids, which is why the config field is now recordedIds. --- packages/cli/.state.json | 53 +++++++++ packages/cli/src/projects/fetch.ts | 6 +- packages/project/src/index.ts | 4 +- packages/project/src/parse/from-app-state.ts | 108 ++++++++++++------ .../project/test/parse/from-app-state.test.ts | 70 +++++++++--- 5 files changed, 184 insertions(+), 57 deletions(-) create mode 100644 packages/cli/.state.json diff --git a/packages/cli/.state.json b/packages/cli/.state.json new file mode 100644 index 000000000..96ed1074c --- /dev/null +++ b/packages/cli/.state.json @@ -0,0 +1,53 @@ +{ + "workflows": { + "my-workflow": { + "id": "c4bb513f-4030-4353-af79-7d2615d34bae", + "name": "My Workflow", + "jobs": { + "transform-data": { + "id": "169e2ab9-6924-40f9-a10c-724057a17b38", + "name": "Transform data", + "adaptor": "@openfn/language-common@latest", + "body": "fn(s => s)", + "project_credential_id": "fa59670d-4e81-425c-9db1-1767dd8cf6a7" + } + }, + "triggers": { + "webhook": { + "id": "4e329538-1d90-4d37-baf5-e1174f34d6a6", + "type": "webhook", + "enabled": true + } + }, + "edges": { + "webhook->transform-data": { + "id": "5fc65286-7b0e-4ebe-b6f9-ce69c9b42885", + "condition_type": null, + "target_job_id": "169e2ab9-6924-40f9-a10c-724057a17b38", + "enabled": true, + "source_trigger_id": "4e329538-1d90-4d37-baf5-e1174f34d6a6" + } + }, + "lock_version": 1, + "inserted_at": "2026-09-09T01:32:34.087Z", + "updated_at": "2026-09-09T01:32:34.087Z", + "deleted_at": null, + "version_history": [ + "app:9506626335ce" + ] + } + }, + "id": "6be63234-1aab-451c-ae5c-ebb96d20bf20", + "name": "My Project", + "project_credentials": { + "super@openfn.org|http1": { + "id": "fa59670d-4e81-425c-9db1-1767dd8cf6a7", + "name": "http1", + "owner": "super@openfn.org" + } + }, + "inserted_at": "2026-09-09T01:32:34.087Z", + "updated_at": "2026-09-09T01:32:34.087Z", + "collections": {}, + "channels": {} +} \ No newline at end of file diff --git a/packages/cli/src/projects/fetch.ts b/packages/cli/src/projects/fetch.ts index fd46a561a..bf8c8c13b 100644 --- a/packages/cli/src/projects/fetch.ts +++ b/packages/cli/src/projects/fetch.ts @@ -1,6 +1,6 @@ import yargs from 'yargs'; import path from 'node:path'; -import Project, { Workspace, recordedStepIdsOf } from '@openfn/project'; +import Project, { Workspace, recordedIdsOf } from '@openfn/project'; import { build, ensure, override } from '../util/command-builders'; import type { Logger } from '../util/logger'; @@ -287,9 +287,7 @@ export async function fetchRemoteProject( ...workspace.getConfig(), alias: options.alias ?? localProject?.alias ?? 'main', // Keep the ids this project already gave its steps. - recordedStepIds: localProject - ? recordedStepIdsOf(localProject) - : undefined, + recordedIds: localProject ? recordedIdsOf(localProject) : undefined, } ); diff --git a/packages/project/src/index.ts b/packages/project/src/index.ts index c465ea4a8..3e1bd28d9 100644 --- a/packages/project/src/index.ts +++ b/packages/project/src/index.ts @@ -22,8 +22,8 @@ export { match as versionsEqual, } from './util/version'; -export { mapWorkflow, recordedStepIdsOf } from './parse/from-app-state'; -export type { RecordedStepIds } from './parse/from-app-state'; +export { mapWorkflow, recordedIdsOf } from './parse/from-app-state'; +export type { RecordedIds } from './parse/from-app-state'; export { default as detectVersion } from './util/detect-version'; diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index 5370ca363..b161cde3b 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/packages/project/src/parse/from-app-state.ts @@ -14,7 +14,7 @@ export type fromAppStateConfig = Partial & { format?: 'yaml' | 'json'; alias?: string; - recordedStepIds?: RecordedStepIds; + recordedIds?: RecordedIds; }; export default ( @@ -24,7 +24,7 @@ export default ( ) => { let stateJson = ensureJson(state); delete config.format; - const { recordedStepIds, ...projectConfig } = config; + const { recordedIds, ...projectConfig } = config; config = projectConfig; const { @@ -81,8 +81,18 @@ export default ( }; } - proj.workflows = Object.values(stateJson.workflows).map((w) => - mapWorkflow(w, proj.credentials, recordedStepIds?.[w.name]) + const allWorkflows = Object.values(stateJson.workflows); + const workflowIds = resolveWorkflowIds( + allWorkflows, + recordedIds?.workflows ?? {} + ); + proj.workflows = allWorkflows.map((w) => + mapWorkflow( + w, + proj.credentials, + recordedIds?.steps?.[w.name] ?? {}, + workflowIds[w.id] + ) ); return new Project(proj as l.ProjectState, config); @@ -113,82 +123,108 @@ export const mapEdge = (edge: Provisioner.Edge) => { return e; }; -// Keyed by workflow name, then step name. The files carry no uuids to match on, -// which is why a rename still moves a step. -export type RecordedStepIds = Record>; +// Ids a project has already given its workflows and steps. Matched by name, +// since the files carry no uuids, which is why a rename still moves things. +export type RecordedIds = { + workflows: Record; + steps: Record>; +}; // An id becomes a directory and a file name, and a workflow file can come from // a repository somebody else prepared, so `../../..` is not to be trusted. const isSafeId = (id: unknown): id is string => typeof id === 'string' && id.length > 0 && slugify(id) === id; -export const recordedStepIdsOf = (project: { - workflows?: { name?: string; steps?: { id?: string; name?: string }[] }[]; -}): RecordedStepIds => { - const recorded: RecordedStepIds = Object.create(null); +export const recordedIdsOf = (project: { + workflows?: { + id?: string; + name?: string; + steps?: { id?: string; name?: string }[]; + }[]; +}): RecordedIds => { + const workflows: Record = Object.create(null); + const steps: Record> = Object.create(null); + for (const workflow of project?.workflows ?? []) { if (!workflow?.name) continue; + if (isSafeId(workflow.id)) { + workflows[workflow.name] = workflow.id!; + } const forWorkflow: Record = Object.create(null); for (const step of workflow.steps ?? []) { if (step?.name && isSafeId(step.id)) { forWorkflow[step.name] = step.id!; } } - recorded[workflow.name] = forWorkflow; + steps[workflow.name] = forWorkflow; } - return recorded; + + return { workflows, steps }; }; // Both passes run in uuid order so the result does not depend on how the server -// listed the jobs. -export const resolveStepIds = ( - jobs: Record, - triggers: Record = {}, - recorded: Record = {} +// listed things. +const assignIds = ( + items: { id: string; name?: string }[], + recorded: Record, + fallback: string, + reserved: string[] = [] ): Record => { const byUuid: Record = Object.create(null); - const taken = new Set(); - - // A trigger's id is its type and shares the directory, so reserve those first. - for (const trigger of Object.values(triggers)) { - if (trigger?.type) { - taken.add(trigger.type); - } - } + const taken = new Set(reserved); - const inUuidOrder = Object.values(jobs) + const inUuidOrder = items .slice() .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); - for (const job of inUuidOrder) { - const known = recorded[job.name]; + for (const item of inUuidOrder) { + const known = item.name ? recorded[item.name] : undefined; if (isSafeId(known) && !taken.has(known)) { - byUuid[job.id] = known; + byUuid[item.id] = known; taken.add(known); } } - for (const job of inUuidOrder) { - if (byUuid[job.id]) continue; - const base = slugify(job.name) || 'step'; + for (const item of inUuidOrder) { + if (byUuid[item.id]) continue; + const base = slugify(item.name ?? '') || fallback; let candidate = base; let n = 2; while (taken.has(candidate)) { candidate = `${base}-${n++}`; } - byUuid[job.id] = candidate; + byUuid[item.id] = candidate; taken.add(candidate); } return byUuid; }; +export const resolveWorkflowIds = ( + workflows: Provisioner.Workflow[], + recorded: Record = {} +) => assignIds(workflows, recorded, 'workflow'); + +export const resolveStepIds = ( + jobs: Record, + triggers: Record = {}, + recorded: Record = {} +): Record => { + // A trigger's id is its type and shares the directory, so reserve those first. + const triggerIds = Object.values(triggers) + .map((trigger) => trigger?.type) + .filter(Boolean) as string[]; + + return assignIds(Object.values(jobs), recorded, 'step', triggerIds); +}; + // map a project workflow to a local cli workflow // TODO this probably gets easier if I index everything by name export const mapWorkflow = ( workflow: Provisioner.Workflow, credentials: l.CredentialState[] = [], - recordedStepIds: Record = {} + recordedStepIds: Record = {}, + workflowId?: string ) => { const { jobs, edges, triggers, name, version_history, ...remoteProps } = workflow; @@ -200,7 +236,7 @@ export const mapWorkflow = ( openfn: renameKeys(remoteProps, { id: 'uuid' }), }; if (workflow.name) { - mapped.id = slugify(workflow.name); + mapped.id = workflowId ?? slugify(workflow.name); } // TODO what do we do if the condition is disabled? diff --git a/packages/project/test/parse/from-app-state.test.ts b/packages/project/test/parse/from-app-state.test.ts index f8f051ff9..2ffcd301c 100644 --- a/packages/project/test/parse/from-app-state.test.ts +++ b/packages/project/test/parse/from-app-state.test.ts @@ -2,7 +2,7 @@ import test from 'ava'; import fromAppState, { mapEdge, mapWorkflow, - recordedStepIdsOf, + recordedIdsOf, } from '../../src/parse/from-app-state'; import { cloneDeep } from 'lodash-es'; @@ -613,10 +613,13 @@ test('both step bodies reach the filesystem', (t) => { test('an id we have already written down is kept', (t) => { const project = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: { - 'My Workflow': { - 'step \u{1F44D}': 'thumbs-up', - 'step \u{1F44E}': 'thumbs-down', + recordedIds: { + workflows: {}, + steps: { + 'My Workflow': { + 'step \u{1F44D}': 'thumbs-up', + 'step \u{1F44E}': 'thumbs-down', + }, }, }, }); @@ -683,12 +686,12 @@ test('two workflows can each hold a step of the same name', (t) => { }; const first = fromAppState(state, meta); - const recorded = recordedStepIdsOf(first); + const recorded = recordedIdsOf(first); // each workflow keeps its own entry rather than one overwriting the other - t.deepEqual(Object.keys(recorded).sort(), ['One', 'Two']); + t.deepEqual(Object.keys(recorded.steps).sort(), ['One', 'Two']); - const second = fromAppState(state, meta, { recordedStepIds: recorded }); + const second = fromAppState(state, meta, { recordedIds: recorded }); t.deepEqual(idsByName(second, 0), idsByName(first, 0)); t.deepEqual(idsByName(second, 1), idsByName(first, 1)); }); @@ -709,8 +712,9 @@ test('a step named like an object property does not pick one up', (t) => { test('an id from a project file that escapes the directory is ignored', (t) => { const project = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: { - 'My Workflow': { 'step \u{1F44D}': '../../../../tmp/pwned' }, + recordedIds: { + workflows: {}, + steps: { 'My Workflow': { 'step \u{1F44D}': '../../../../tmp/pwned' } }, }, }); @@ -724,7 +728,7 @@ test('a project on disk hands back the ids it already gave its steps', (t) => { const first = fromAppState(twoStepsSharingAnId() as any, meta); const second = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: recordedStepIdsOf(first), + recordedIds: recordedIdsOf(first), }); t.deepEqual(idsByName(second), idsByName(first)); @@ -732,12 +736,15 @@ test('a project on disk hands back the ids it already gave its steps', (t) => { test('the recorded ids are not written into the workspace config', (t) => { const project = fromAppState(twoStepsSharingAnId() as any, meta, { - recordedStepIds: { 'My Workflow': { 'step \u{1F44D}': 'thumbs-up' } }, + recordedIds: { + workflows: {}, + steps: { 'My Workflow': { 'step \u{1F44D}': 'thumbs-up' } }, + }, }); const files = project.serialize('fs') as Record; - t.false(JSON.stringify(files).includes('recordedStepIds')); + t.false(JSON.stringify(files).includes('recordedIds')); }); test('renaming a step still moves its id', (t) => { @@ -757,12 +764,45 @@ test('renaming a step still moves its id', (t) => { }); const first = fromAppState(withName('Send data') as any, meta); - const recorded = recordedStepIdsOf(first); + const recorded = recordedIdsOf(first); const renamed = fromAppState(withName('Send data v2') as any, meta, { - recordedStepIds: recorded, + recordedIds: recorded, }); t.is(idsByName(first)['Send data'], 'send-data'); t.is(idsByName(renamed)['Send data v2'], 'send-data-v2'); }); + +test('two workflow names that shorten to the same id stay distinct', (t) => { + const state: any = { + id: 'p', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'w1', + name: 'wf \u{1F44D}', + triggers: {}, + edges: {}, + jobs: { a: { id: 'u-a', name: 'A', body: 'one()', adaptor: 'c' } }, + }, + { + id: 'w2', + name: 'wf \u{1F44E}', + triggers: {}, + edges: {}, + jobs: { b: { id: 'u-b', name: 'B', body: 'two()', adaptor: 'c' } }, + }, + ], + }; + + const project = fromAppState(state, meta); + const ids = Object.values(project.workflows).map((w: any) => w.id); + + t.is(new Set(ids).size, 2); + + // on main this throws `step not found: b` + const files = project.serialize('fs') as Record; + t.is(Object.keys(files).filter((f) => f.endsWith('.js')).length, 2); +}); From dfe81491d5876fad1bb7995c05391344eb3a5755 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 01:39:08 +0000 Subject: [PATCH 7/7] Drop a test artefact I committed by accident packages/cli/.state.json is written by running the CLI locally. --- packages/cli/.state.json | 53 ---------------------------------------- 1 file changed, 53 deletions(-) delete mode 100644 packages/cli/.state.json diff --git a/packages/cli/.state.json b/packages/cli/.state.json deleted file mode 100644 index 96ed1074c..000000000 --- a/packages/cli/.state.json +++ /dev/null @@ -1,53 +0,0 @@ -{ - "workflows": { - "my-workflow": { - "id": "c4bb513f-4030-4353-af79-7d2615d34bae", - "name": "My Workflow", - "jobs": { - "transform-data": { - "id": "169e2ab9-6924-40f9-a10c-724057a17b38", - "name": "Transform data", - "adaptor": "@openfn/language-common@latest", - "body": "fn(s => s)", - "project_credential_id": "fa59670d-4e81-425c-9db1-1767dd8cf6a7" - } - }, - "triggers": { - "webhook": { - "id": "4e329538-1d90-4d37-baf5-e1174f34d6a6", - "type": "webhook", - "enabled": true - } - }, - "edges": { - "webhook->transform-data": { - "id": "5fc65286-7b0e-4ebe-b6f9-ce69c9b42885", - "condition_type": null, - "target_job_id": "169e2ab9-6924-40f9-a10c-724057a17b38", - "enabled": true, - "source_trigger_id": "4e329538-1d90-4d37-baf5-e1174f34d6a6" - } - }, - "lock_version": 1, - "inserted_at": "2026-09-09T01:32:34.087Z", - "updated_at": "2026-09-09T01:32:34.087Z", - "deleted_at": null, - "version_history": [ - "app:9506626335ce" - ] - } - }, - "id": "6be63234-1aab-451c-ae5c-ebb96d20bf20", - "name": "My Project", - "project_credentials": { - "super@openfn.org|http1": { - "id": "fa59670d-4e81-425c-9db1-1767dd8cf6a7", - "name": "http1", - "owner": "super@openfn.org" - } - }, - "inserted_at": "2026-09-09T01:32:34.087Z", - "updated_at": "2026-09-09T01:32:34.087Z", - "collections": {}, - "channels": {} -} \ No newline at end of file