Skip to content

Commit f5f6d0e

Browse files
pranaygpTooTallNateVaguelySerious
authored
Validate unique workflow step IDs at build time (#2018)
* Validate unique step ids at build time * Fall back to file-path IDs for non-exported package files Instead of synthesizing a 'name/dist/<path>@Version' specifier (which hardcoded the dist/ output convention), non-exported workspace/node_modules files now return moduleSpecifier: undefined and let the SWC plugin's './{filepath}' fallback produce per-file IDs. This is the same path local app files have always taken and avoids the dist/ assumption flagged in review. The build-time duplicate-ID check stays as the safety net. * Dedupe virtual-entry imports by canonical module identity When both the source and the compiled-dist copies of the same workspace package export end up in discoveredSteps/discoveredWorkflows (e.g. the 'workflow' package's internal/builtins in monorepo dev), they resolve to the same module via esbuild's package resolution. The virtual entry was emitting BOTH 'import "workflow/internal/builtins";' (the built-in preamble) and 'import "../../packages/workflow/src/internal/builtins.ts";' (via the isWorkspaceSourceBackedPackageFile carve-out in createImport), which made the swc plugin transform both copies and generate duplicate step IDs. Track a per-bundle set of emitted module identities (package specifier when reachable, otherwise the file path) and skip files whose identity has already been imported. The steps bundle pre-seeds the set with the built-in steps specifier so workspace step files at that path don't emit a competing relative-path import. * Stop rewriting workspace package /dist/ -> /src/ during Next.js discovery The Next.js deferred builder's `resolveSourceBackedPackagePath` rewrote any discovered `/dist/` path to its `/src/` sibling for workspace packages and for `workflow`/`@workflow/*` tarballs. That made the discovered step file list point at source files while base-builder's esbuild bundle (which builds the workflow VM and step registrations) resolved the same package imports through `pkg.exports` to `/dist/`. The workflow proxy ID — generated from the dist path — didn't match the step bundle's registration ID — generated from the src path — producing "Step function not registered" failures at runtime, most visibly with @workflow/ai's doStreamStep on Vercel and Windows Next.js deployments. App code that imports a package by name should resolve naturally through pkg.exports; the loader has no business reaching into the package's source tree. Drop the rewrite (and the now-unused `resolveCopiedStepImportTargetPath` helper that supported it). Workspace packages are still discovered — that's a separate predicate (`shouldPreferSourceBackedPackagePath`) which only gates inclusion, not path translation. Verified locally with the nextjs-turbopack workbench: agent e2e suite (19 tests, including the failing `agentBasicE2e`) and the addTenWorkflow duplicate-name suite all pass. * Address review nits: extract stripPackageVersion, expand duplicate-ID hint, note new build-time check in changeset --------- Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: Peter Wielander <mittgfu@gmail.com>
1 parent b5396bc commit f5f6d0e

8 files changed

Lines changed: 323 additions & 86 deletions
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@workflow/next": patch
3+
---
4+
5+
Stop rewriting workspace-package `/dist/` paths to `/src/` during workflow/step discovery so that the discovered file paths agree with how base-builder resolves the same packages through `pkg.exports`, fixing `Step function not registered` errors at runtime.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@workflow/builders": patch
3+
---
4+
5+
Generate per-file IDs for non-exported workspace package files (previously they collapsed to `name@version` and silently overwrote each other at runtime) and fail the build when two transformed files emit the same step or workflow ID — collisions that used to register silently last-write-wins now surface as a build error.

‎packages/builders/src/base-builder.ts‎

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@ import {
1616
} from './apply-swc-transform.js';
1717
import { createDiscoverEntriesPlugin } from './discover-entries-esbuild-plugin.js';
1818
import { getEsbuildTsconfigOptions } from './esbuild-tsconfig.js';
19-
import { getImportPath } from './module-specifier.js';
19+
import {
20+
getImportPath,
21+
resolveModuleSpecifier,
22+
stripPackageVersion,
23+
} from './module-specifier.js';
2024
import { createNodeModuleErrorPlugin } from './node-module-esbuild-plugin.js';
2125
import { createPseudoPackagePlugin } from './pseudo-package-esbuild-plugin.js';
2226
import { createSwcPlugin } from './swc-esbuild-plugin.js';
@@ -90,6 +94,31 @@ async function withRealpaths(entries: string[]): Promise<string[]> {
9094
);
9195
}
9296

97+
/**
98+
* Canonical "what module does this file represent?" key used to dedupe
99+
* virtual-entry imports.
100+
*
101+
* If the file resolves to a real package specifier (`workflow/internal/builtins`,
102+
* `@internal/agent/server`, etc.), we return the bare specifier — version
103+
* stripped — because esbuild's package resolution will collapse all
104+
* importers of that specifier to the same physical module regardless of
105+
* which on-disk copy (src vs dist) any one importer wrote.
106+
*
107+
* Otherwise we fall back to the absolute file path. Distinct local-app
108+
* files have distinct paths, so this still dedupes a file against itself
109+
* (e.g. if it shows up in both `stepFiles` and `serdeOnlyFiles`) without
110+
* conflating unrelated files.
111+
*/
112+
function moduleIdentityKey(file: string, projectRoot: string): string {
113+
const { moduleSpecifier } = resolveModuleSpecifier(file, projectRoot);
114+
if (moduleSpecifier) {
115+
// Strip the "@<version>" suffix so source and dist copies of the same
116+
// export collapse to the same key.
117+
return stripPackageVersion(moduleSpecifier);
118+
}
119+
return file.replace(/\\/g, '/');
120+
}
121+
93122
export interface DiscoveredEntries {
94123
discoveredSteps: Set<string>;
95124
discoveredWorkflows: Set<string>;
@@ -696,10 +725,29 @@ export abstract class BaseBuilder {
696725

697726
// Create a virtual entry that imports all files. All step definitions
698727
// will get registered thanks to the swc transform.
699-
const stepImports = stepFiles.map(createImport).join('\n');
728+
//
729+
// Dedupe imports by canonical module identity so we never emit two
730+
// import lines that resolve to the same physical module. Pre-seed the
731+
// set with the built-in steps import so a workspace step file at
732+
// `packages/workflow/src/internal/builtins.ts` doesn't emit a second,
733+
// relative-path competing import — esbuild would otherwise transform
734+
// both copies and the swc plugin would generate duplicate step IDs.
735+
const emittedImportIdentities = new Set<string>([builtInSteps]);
736+
const buildImports = (files: string[]): string =>
737+
files
738+
.filter((file) => {
739+
const identity = moduleIdentityKey(file, this.transformProjectRoot);
740+
if (emittedImportIdentities.has(identity)) return false;
741+
emittedImportIdentities.add(identity);
742+
return true;
743+
})
744+
.map(createImport)
745+
.join('\n');
746+
747+
const stepImports = buildImports(stepFiles);
700748

701749
// Include serde-only files for class registration side effects
702-
const serdeImports = serdeOnlyFiles.map(createImport).join('\n');
750+
const serdeImports = buildImports(serdeOnlyFiles);
703751

704752
const entryContent = `
705753
// Built in steps
@@ -934,13 +982,28 @@ export abstract class BaseBuilder {
934982
return `import '${relativePath}';`;
935983
};
936984

937-
// Create a virtual entry that imports all workflow files
985+
// Create a virtual entry that imports all workflow files. Dedupe by
986+
// canonical module identity so source/dist copies of the same workspace
987+
// package export don't both get imported (which would make the swc
988+
// plugin generate duplicate workflow IDs).
989+
const emittedImportIdentities = new Set<string>();
990+
const buildImports = (files: string[]): string =>
991+
files
992+
.filter((file) => {
993+
const identity = moduleIdentityKey(file, this.transformProjectRoot);
994+
if (emittedImportIdentities.has(identity)) return false;
995+
emittedImportIdentities.add(identity);
996+
return true;
997+
})
998+
.map(createImport)
999+
.join('\n');
1000+
9381001
// The SWC plugin in workflow mode emits `globalThis.__private_workflows.set(workflowId, fn)`
9391002
// calls directly, so we just need to import the files (Map is initialized via banner)
940-
const workflowImports = workflowFiles.map(createImport).join('\n');
1003+
const workflowImports = buildImports(workflowFiles);
9411004

9421005
// Include serde-only files for class registration side effects
943-
const serdeImports = serdeOnlyFiles.map(createImport).join('\n');
1006+
const serdeImports = buildImports(serdeOnlyFiles);
9441007

9451008
const imports = serdeImports
9461009
? `${workflowImports}\n// Serde files for cross-context class registration\n${serdeImports}`

‎packages/builders/src/module-specifier.test.ts‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,30 @@ describe('getImportPath', () => {
281281
});
282282
});
283283

284-
it('uses the consuming app root to resolve workspace package workflow ids', () => {
284+
it('uses package specifiers for workspace package root entrypoint ids', () => {
285+
const projectRoot = join(testRoot, 'apps/chat');
286+
const packageDir = join(testRoot, 'packages/vade');
287+
const filePath = join(packageDir, 'src/index.ts');
288+
289+
writeJson(join(projectRoot, 'package.json'), {
290+
name: 'chat',
291+
dependencies: { vade: 'workspace:*' },
292+
});
293+
294+
writeJson(join(packageDir, 'package.json'), {
295+
name: 'vade',
296+
version: '0.0.0',
297+
main: './src/index.ts',
298+
});
299+
300+
writeFile(filePath, `'use workflow';\n`);
301+
302+
expect(resolveModuleSpecifier(filePath, projectRoot)).toEqual({
303+
moduleSpecifier: 'vade@0.0.0',
304+
});
305+
});
306+
307+
it('returns undefined for non-exported workspace package files', () => {
285308
const projectRoot = join(testRoot, 'apps/chat');
286309
const packageDir = join(testRoot, 'packages/vade');
287310
const filePath = join(
@@ -301,8 +324,12 @@ describe('getImportPath', () => {
301324

302325
writeFile(filePath, `'use workflow';\n`);
303326

327+
// Non-exported package files fall back to the relative-file-path ID
328+
// (the SWC plugin uses "./{file}" when moduleSpecifier is undefined).
329+
// This keeps IDs unique per file across the build instead of collapsing
330+
// every non-exported file in `vade` to the same "vade@0.0.0" specifier.
304331
expect(resolveModuleSpecifier(filePath, projectRoot)).toEqual({
305-
moduleSpecifier: 'vade@0.0.0',
332+
moduleSpecifier: undefined,
306333
});
307334
});
308335

‎packages/builders/src/module-specifier.ts‎

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -338,11 +338,16 @@ function isWorkspacePackage(filePath: string, projectRoot: string): boolean {
338338
* // => { moduleSpecifier: 'workflow/internal/builtins@4.0.0' }
339339
*
340340
* @example
341-
* // File in workspace package
342-
* resolveModuleSpecifier('/project/packages/shared/src/utils.ts', '/project')
341+
* // Exported root file in workspace package
342+
* resolveModuleSpecifier('/project/packages/shared/src/index.ts', '/project')
343343
* // => { moduleSpecifier: '@myorg/shared@0.0.0' }
344344
*
345345
* @example
346+
* // Non-exported / deep package file
347+
* resolveModuleSpecifier('/project/packages/shared/src/internal/foo.ts', '/project')
348+
* // => { moduleSpecifier: undefined }
349+
*
350+
* @example
346351
* // Local app file
347352
* resolveModuleSpecifier('/project/src/workflows/order.ts', '/project')
348353
* // => { moduleSpecifier: undefined }
@@ -373,16 +378,50 @@ export function resolveModuleSpecifier(
373378
allowSourceFallback: true,
374379
});
375380

376-
// Return the module specifier as "name/subpath@version" or "name@version"
377-
const specifier = subpath
378-
? `${pkg.name}${subpath}@${pkg.version}`
379-
: `${pkg.name}@${pkg.version}`;
381+
if (subpath) {
382+
return {
383+
moduleSpecifier: `${pkg.name}${subpath}@${pkg.version}`,
384+
};
385+
}
386+
387+
if (isRootEntrypointFile(filePath, pkg)) {
388+
return {
389+
moduleSpecifier: `${pkg.name}@${pkg.version}`,
390+
};
391+
}
380392

393+
// Non-exported package file (deep import that isn't reachable as a
394+
// package specifier). Returning undefined makes the SWC plugin fall back
395+
// to the relative file path, which keeps IDs unique per file. Previously
396+
// every non-exported file collapsed to "name@version", causing same-named
397+
// step/workflow functions in different files to silently overwrite each
398+
// other at runtime registration; the build-time duplicate-ID check now
399+
// also catches that class of collision.
381400
return {
382-
moduleSpecifier: specifier,
401+
moduleSpecifier: undefined,
383402
};
384403
}
385404

405+
/**
406+
* Strip the trailing "@<version>" suffix from a module specifier produced by
407+
* `resolveModuleSpecifier`, leaving the bare `name` or `name/subpath` form.
408+
*
409+
* Use this when you want to compare or dedupe specifiers across versions
410+
* (e.g. treating `@workflow/ai/agent@5.0.0-beta.4` and
411+
* `@workflow/ai/agent@5.0.0-beta.5` as the same logical module).
412+
*
413+
* Colocated with `resolveModuleSpecifier` so the construction and parsing
414+
* stay in sync — see the `${pkg.name}${subpath}@${pkg.version}` and
415+
* `${pkg.name}@${pkg.version}` paths above.
416+
*/
417+
export function stripPackageVersion(specifier: string): string {
418+
// The version is always the final segment after the last "@". Constrain
419+
// matching to characters that can't appear in a package name or subpath
420+
// (no "/", no nested "@") so scoped packages like `@workflow/ai@1.0.0`
421+
// keep their leading "@workflow/ai".
422+
return specifier.replace(/@[^/@]+$/, '');
423+
}
424+
386425
/**
387426
* Clear the package.json cache. Useful for testing or when package.json files may have changed.
388427
*/

‎packages/builders/src/swc-esbuild-plugin.test.ts‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,88 @@ describe('createSwcPlugin externalizeNonSteps', () => {
5151
rmSync(testRoot, { recursive: true, force: true });
5252
});
5353

54+
it('fails the build when two files emit the same step id', async () => {
55+
const srcDir = join(testRoot, 'src');
56+
const firstStepFile = join(srcDir, 'confirmation.ts');
57+
const secondStepFile = join(srcDir, 'reschedule.ts');
58+
59+
writeFile(firstStepFile, `export const first = true;`);
60+
writeFile(secondStepFile, `export const second = true;`);
61+
62+
applySwcTransformMock.mockImplementation(
63+
async (filename: string, source: string) => ({
64+
code: source,
65+
workflowManifest: {
66+
steps: {
67+
[filename]: {
68+
sendMessage: {
69+
stepId: 'step//shared-package@1.0.0//sendMessage',
70+
},
71+
},
72+
},
73+
},
74+
})
75+
);
76+
77+
await expect(
78+
esbuild.build({
79+
entryPoints: [firstStepFile, secondStepFile],
80+
absWorkingDir: testRoot,
81+
outdir: join(testRoot, 'out'),
82+
bundle: true,
83+
format: 'esm',
84+
platform: 'node',
85+
write: false,
86+
plugins: [
87+
createSwcPlugin({
88+
mode: 'step',
89+
}),
90+
],
91+
})
92+
).rejects.toThrow(/Duplicate workflow step ID/);
93+
});
94+
95+
it('fails the build when two files emit the same workflow id', async () => {
96+
const srcDir = join(testRoot, 'src');
97+
const firstWorkflowFile = join(srcDir, 'confirmation.ts');
98+
const secondWorkflowFile = join(srcDir, 'reschedule.ts');
99+
100+
writeFile(firstWorkflowFile, `export const first = true;`);
101+
writeFile(secondWorkflowFile, `export const second = true;`);
102+
103+
applySwcTransformMock.mockImplementation(
104+
async (filename: string, source: string) => ({
105+
code: source,
106+
workflowManifest: {
107+
workflows: {
108+
[filename]: {
109+
sendMessage: {
110+
workflowId: 'workflow//shared-package@1.0.0//sendMessage',
111+
},
112+
},
113+
},
114+
},
115+
})
116+
);
117+
118+
await expect(
119+
esbuild.build({
120+
entryPoints: [firstWorkflowFile, secondWorkflowFile],
121+
absWorkingDir: testRoot,
122+
outdir: join(testRoot, 'out'),
123+
bundle: true,
124+
format: 'esm',
125+
platform: 'node',
126+
write: false,
127+
plugins: [
128+
createSwcPlugin({
129+
mode: 'workflow',
130+
}),
131+
],
132+
})
133+
).rejects.toThrow(/Duplicate workflow ID/);
134+
});
135+
54136
it.each([
55137
{ inputExt: '.ts', outputExt: '.js' },
56138
{ inputExt: '.tsx', outputExt: '.js' },

0 commit comments

Comments
 (0)