-
Notifications
You must be signed in to change notification settings - Fork 178
Ensure class serialization / deserialization only happens in the proper global context #873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 01-27-fix_class_id_generation_when_class_is_bound_to_a_variable
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 55dc421 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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🌍 Community Worlds (162 failed)mongodb (40 failed):
redis (40 failed):
starter (41 failed):
turso (41 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
80a9195 to
b0a69ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR tightens class serialization/deserialization so that it is always scoped to the appropriate global object, preventing leakage of class registrations between the host environment and workflow VM contexts.
Changes:
- Updated
getCommonReviversto pass the activeglobalobject intogetSerializationClass, ensuring deserialization uses the correct registry for the current VM/host context. - Simplified
getSerializationClassto look up classes only on the provided global’s registry (removing theglobalThisfallback) and expanded custom class serialization tests to more accurately simulate VM vs host behavior. - Added a changeset entry for
@workflow/coredocumenting the behavior change as a patch.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/core/src/serialization.ts |
Ensures class and instance revivers use the provided global when resolving class constructors, aligning deserialization with the current VM/host context. |
packages/core/src/class-serialization.ts |
Removes cross-context fallback in getSerializationClass and requires an explicit global, so class lookup is strictly scoped to a given global registry. |
packages/core/src/serialization.test.ts |
Adjusts custom class serialization tests to define/register classes separately in host and VM contexts and to assert across-context instance behavior using constructor names. |
.changeset/olive-cars-run.md |
Records a patch release note describing the tightened global-context scoping for class serialization/deserialization. |
Comments suppressed due to low confidence (1)
packages/core/src/serialization.test.ts:1100
- These updated custom class serialization tests exercise the positive path for VM/host isolation, but there is no test that verifies the new behavior where a class registered only on the host is not deserializable inside the VM (i.e., that the old
globalThisfallback is truly gone). To fully cover the new semantics described in the PR (that classes registered in one context are not accessible from another), consider adding a test in this block that registers a class only on the host, hydrates usingvmGlobalThis, and asserts that deserialization fails with the expected "Class "..." not found" error.
101,
97,
109,
34,
44,
49,
51,
93,
44,
123,
34,
110,
97,
109,
101,
34,
58,
49,
52,
44,
34,
116,
121,
112,
101,
34,
58,
49,
53,
125,
44,
34,
115,
116,
114,
109,
95,
48,
49,
65,
82,
90,
51,
78,
68,
69,
75,
84,
83,
86,
52,
82,
82,
70,
70,
81,
54,
57,
71,
53,
70,
65,
49,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getSerializationClass( | ||
| classId: string, | ||
| global: Record<string, any> = globalThis | ||
| global: Record<string, any> |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing getSerializationClass to require a non-optional global parameter removes the previous default of globalThis and is a breaking API change for any external callers that relied on calling this exported helper with just classId. To preserve backward compatibility (especially since this is released as a patch), consider keeping global optional with a default of globalThis while still dropping the cross-context fallback so that existing call sites outside this file continue to compile and behave as before in the host context.
| global: Record<string, any> | |
| global: Record<string, any> = globalThis |
…er global context
80bfb95 to
1973bae
Compare
b0a69ea to
55dc421
Compare

Fixed class serialization/deserialization to properly respect VM context boundaries, ensuring workflow code only accesses classes registered in its own global context.
What changed?
getSerializationClass()to no longer fall back to checkingglobalThiswhen a class isn't found in the provided global contextHow to test?
Why make this change?
This change ensures proper isolation between the host environment and workflow VM contexts, matching production serverless behavior where workflow code runs in isolation. Previously, the code could leak class registrations between contexts, potentially causing unexpected behavior or security issues. This fix maintains proper boundaries between execution contexts.