Validate unique workflow step IDs at build time#2018
Conversation
🦋 Changeset detectedLatest commit: 8784a0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (2 failed)nuxt (1 failed):
sveltekit (1 failed):
📋 Other (1 failed)e2e-vercel-prod-tanstack-start (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
pranaygp
force-pushed
the
pranaygp/codex/validate-step-id-duplicates-build
branch
from
a862049 to
f7f3f45
Compare
pranaygp
force-pushed
the
pranaygp/codex/validate-step-id-duplicates-build
branch
from
be57b18 to
cc23370
Compare
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Comment-only review
The core diagnosis is right and the two-part fix is well-targeted:
- Build-time duplicate-ID detection in
assertUniqueManifestIds— turns a silent last-write-wins runtime bug into a loud build-timeWorkflowBuildErrorwith a helpful hint. This is excellent regardless of how the specifier scheme evolves. Per-build state viabuild.onStartcorrectly handles esbuild rebuilds, the early-return for same-filePath+name avoids false positives on same-file-twice, and the error message reports both conflicting locations. base-builder.tscarve-outs for the workflow builtins (pseudo-use stepruntime intrinsics) and src-backed-by-dist deduplication. Both make sense.
Where I'd push back is on the synthetic specifier scheme for private workspace files (see inline). The src/→dist/ rewrite assumes every workspace package uses dist/ as its compiled output dir, which holds for this repo's internal packages and the motivating case (@internal/agent), but not universally. Packages using lib/, build/, out/, or no separate output dir at all would get the wrong dev↔prod ID mapping under this PR. The build-time check would catch the symptom (duplicate IDs across two file paths emitting the same name), but the cause (the synthetic specifier not matching across dev/prod) is silent.
I think there's a real choice to make here about how the specifier should behave for non-exported package files:
- Synthetic
name/dist/path@version(this PR): stable dev↔prod IDs fordist/-using packages, broken for everything else - Filepath-based
./packages/foo/src/path(themoduleSpecifier: undefinedfallback the SWC plugin already supports vianaming.rs::get_module_path): never stable across dev/prod for any package, but always at least internally consistent - Inferred output dir (read from
pkg.exports/pkg.main/pkg.module): stable for any package, more code
Worth thinking through which trade-off matches the actual use cases. Inline comment has more detail.
What I verified
- 167 builder tests pass (+6 new) including the duplicate-ID detection for both step and workflow modes, the builtins exclusion, and the src↔dist dedup
- Build + workspace typecheck clean
- CI: 80 success, 0 failures, 15 in progress
- Module-specifier semantics traced end-to-end:
apply-swc-transform.tspassesmoduleSpecifierto the SWC plugin, which uses it as the middle segment of the step ID vianaming.rs::format_name({prefix}//{module_path}//{identifier}). WhenmoduleSpecifierisNone, falls back to./{filepath_without_ext}— so themoduleSpecifier: undefinedfallback path IS reachable in practice when needed. - Per-build state isolation:
stepIdsForCurrentBuildandworkflowIdsForCurrentBuildare reset onbuild.onStart, so the duplicate-ID check correctly handles incremental builds and watch mode.
Two inline comments
- Hardcoded
dist/convention in the specifier scheme — the main design concern - The
isWorkflowInternalBuiltinsFileregex is brittle to a futureworkflowpackage rename — non-blocking but worth noting
The duplicate-ID check is a strong improvement on its own, and the specifier-scheme question is more of a design question than a correctness one — happy to approve once we've talked through the dev↔prod stability story for non-dist/ packages.
|
Took over this PR to address the dist/ convention concern. Going with option 2 from that comment: drop the synthetic What changed in e86a96d
This is consistent with how the codebase already handles duplicate workflow function names across local files — see Dropped from the PR
Kept from the PR
TradeoffThe synthetic-specifier approach would have kept a non-exported file's ID stable across directory renames (within the same package + version). The file-path fallback embeds the relative path, so moving Validation
|
There was a problem hiding this comment.
Pull request overview
This PR improves build-time correctness for workflow/step ID generation in @workflow/builders (preventing silent runtime overwrites) and hardens @workflow/world-postgres startup by serializing Graphile Worker initialization to avoid concurrent migration races.
Changes:
- Generate file-scoped IDs for non-exported workspace package files by returning
moduleSpecifier: undefinedfor deep/non-exported package paths. - Validate that workflow/step IDs are unique across the build when merging per-file manifests in the SWC esbuild plugin, failing early with a
WorkflowBuildError. - Serialize Graphile Worker initialization in world-postgres using a Postgres advisory lock; add targeted tests for both the lock and the new ID behaviors.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/world-postgres/src/reenqueue.test.ts | Updates pool mocks to support advisory-lock startup path (pool.connect()). |
| packages/world-postgres/src/queue.ts | Adds startup advisory lock around Graphile Worker init/migrate to reduce migration race conditions. |
| packages/world-postgres/src/queue.test.ts | Adds advisory-lock ordering test and updates pool mocks to include a connected client. |
| packages/builders/src/swc-esbuild-plugin.ts | Adds manifest ID uniqueness validation and switches manifest merging to a checked merge. |
| packages/builders/src/swc-esbuild-plugin.test.ts | Adds tests asserting the build fails on duplicate step/workflow IDs across files. |
| packages/builders/src/module-specifier.ts | Changes workspace deep/non-exported files to return moduleSpecifier: undefined to keep IDs file-scoped. |
| packages/builders/src/module-specifier.test.ts | Adds coverage for workspace package root entrypoints vs non-exported deep files. |
| .changeset/validate-step-id-duplicates.md | Changeset for builders behavior change + build-time duplicate ID failure. |
| .changeset/fix-postgres-graphile-startup-lock.md | Changeset for world-postgres advisory-lock startup serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
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.
|
Update: walking back the previous comment — option 2 (relative-path fallback) turned out to be worse than I claimed, not equivalent. Here's the final state. Where this PR landedRestored the synthetic
Why option 2 was wrongI had assumed the relative-path fallback (
→ The build-time duplicate-ID check doesn't catch this — both bundles see the proxy/registration as the only entry for their (divergent) ID, so neither bundle's check fires. Current commit list
The diff vs. TradeoffThe Validation
|
|
Update — fixed at the right layer this time. You were right that this is a Next.js builder problem, not a Root cause
That alone wouldn't break anything, but base-builder's esbuild bundle (which produces the workflow VM and step registrations) doesn't go through this rewrite — it resolves
Different IDs → Fix in be40f72Dropped Final stateThe PR is now four commits. None of them ship src/dist hardcoding:
Diff vs
Validation
Apologies for the runaround on the previous iterations — your push to look at the resolution layer instead of working around it was right. |
…very 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.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
… hint, note new build-time check in changeset
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
|
||
| function assertUniqueManifestIds( | ||
| entriesByFile: WorkflowManifest['steps'] | WorkflowManifest['workflows'], | ||
| ids: Map<string, IdLocation>, |
There was a problem hiding this comment.
SWC plugin's assertUniqueManifestIds falsely flags src/dist copies of the same package export as duplicate step IDs, causing the Next.js turbopack workbench build to fail.
Summary
package@versionRoot Cause
Workspace package files that did not match an export subpath still received a package-level
moduleSpecifierlikevade@0.0.0. That made same-named step functions in different package files emit the same ID, for examplestep//vade@0.0.0//sendMessage, and runtime registration used last-write-wins semantics.Validation
fnm exec --using 24 pnpm --filter @workflow/builders exec vitest run src/module-specifier.test.ts src/swc-esbuild-plugin.test.tsfnm exec --using 24 pnpm --filter @workflow/builders typecheckfnm exec --using 24 pnpm --filter @workflow/builders testgit diff --checkfnm exec --using 24 pnpm changeset status --since=origin/main