-
Notifications
You must be signed in to change notification settings - Fork 188
Add Encryptor interface and thread through serialization layer #979
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: nate/async-serde
Are you sure you want to change the base?
Conversation
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (42 failed)mongodb (1 failed):
turso (41 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
🦋 Changeset detectedLatest commit: fc25b9c The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
|
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. |
pranaygp
left a comment
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.
Review: PR #979 - Add Encryptor interface and thread through serialization layer
Summary: Adds the Encryptor, EncryptionContext, and KeyMaterial interfaces to @workflow/world, makes World extend Encryptor, and threads the encryptor parameter through all serialization functions. This is a no-op refactor -- the encryptor parameter is unused (_encryptor) throughout.
Strengths:
- Clean interface design:
Encryptorhas all-optional methods, so existingWorldimplementations don't break EncryptionContextis minimal (justrunId) -- good for forward compatibilityKeyMaterialinterface for o11y tooling is a thoughtful addition- The
getEncryptorForRun()method onWorldis a well-designed escape hatch for cross-deployment encryption (e.g.,resumeHook()from newer deployment) getHookByTokenWithEncryptor()resolves the encryptor once and reuses it -- avoids redundant key resolution
Concerns:
-
resolveEncryptorForRuntype safety: Inresume-hook.tsline 29-31,getEncryptorForRunis accessed via(world as any).getEncryptorForRun. SinceWorldalready extendsEncryptorandgetEncryptorForRunis defined onWorld, you should be able to use optional chaining directly:world.getEncryptorForRun?.(runId). The'getEncryptorForRun' in world+as anypattern bypasses type checking unnecessarily. -
Serialization parameter ordering: The PR reorders parameters in the dehydrate/hydrate functions. For example,
dehydrateWorkflowArgumentsgoes from(value, ops, runId, ...)to(value, runId, encryptor, ops, ...). This is a breaking change to the internal API. While these aren't public, any external code calling these directly would break. The reorder makes sense semantically (runId + encryptor are conceptually paired), but consider documenting this in the changeset. -
_encryptorunused parameter pattern: All 8 functions have_encryptor: Encryptorthat is unused. This is expected since the actual wiring happens in #957. However, this means if #979 lands but #957 doesn't (or is delayed), there's dead parameter threading throughout the codebase. A minor code smell but acceptable for a PR stack. -
hydrateResourceIOnow requiresencryptor: Inobservability.ts,hydrateResourceIOnow takes anEncryptorparameter, and all callers passworld. This means the observability layer now has a dependency on the World instance. Previously it was a pure data transformation. This is a reasonable tradeoff for encryption support, but worth noting the coupling increase.
Overall, well-structured interface design. The cross-deployment encryption support via getEncryptorForRun shows good foresight for production scenarios.
30fc9e5 to
fc25b9c
Compare

Summary
Adds the
Encryptorinterface and threads it through the serialization layer, preparing the codebase for E2E encryption without yet wiring in any actual encryption logic.New types (
@workflow/world)Encryptor— optionalencrypt(),decrypt(), andgetKeyMaterial()methodsEncryptionContext— containsrunIdfor per-run key derivationKeyMaterial— key bytes + derivation metadata for external tooling (o11y)Worldnow extendsEncryptor— all methods optional, so existing implementations are unaffectedWorld.getEncryptorForRun?(runId)— resolves anEncryptorfor a specific run's deployment context, enabling cross-deployment encryption (e.g.,resumeHook()from a newer deployment)Serialization signature changes
All 8 dehydrate/hydrate functions gain
encryptor: Encryptoras a new parameter (prefixed with_since it's unused in this PR):dehydrateWorkflowArguments(value, ops, runId, ...)dehydrateWorkflowArguments(value, runId, encryptor, ops, ...)hydrateWorkflowArguments(value, global, ...)hydrateWorkflowArguments(value, runId, encryptor, global, ...)dehydrateStepReturnValue(value, ops, runId, ...)dehydrateStepReturnValue(value, runId, encryptor, ops, ...)Runtime changes
WorkflowOrchestratorContext— addsrunId: stringandencryptor: EncryptorrunWorkflow()— acceptsencryptoras 4th parameterresumeHook()— restructured withgetHookByTokenWithEncryptor()to resolve the encryptor once and reuse for both metadata decryption and payload encryption (zero redundant key resolutions)hydrateResourceIO()— acceptsencryptorparameter, threaded from CLI/web callersCross-deployment design
When
resumeHook()is called from a different deployment than the target workflow run, the encryption keys differ. The newWorld.getEncryptorForRun(runId)method allows the World implementation to resolve the correct key (e.g., via an authenticated API endpoint). TheresumeHookflow resolves the encryptor once viagetHookByTokenWithEncryptor()and reuses it for both operations.Test plan
All 305 core tests pass. Build succeeds. The
encryptorparameter is unused (_encryptor) in this PR — actual encryption is wired in PR #957.