[Workflows] Add sensitive option to Workflows steps#14742
Conversation
🦋 Changeset detectedLatest commit: 1acfba1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
UnknownError: ProviderInitError |
|
@pombosilva Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
✅ All changesets look good |
|
UnknownError: ProviderInitError |
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
/bonk |
There was a problem hiding this comment.
Review — [Workflows] Add sensitive option to Workflows steps
Reviewed the redaction logic, validator changes, and tests. The implementation is correct and well-scoped. 👍
What works well
- Redaction happens at the right seam. Applying redaction only when writing
events.success(context.ts:1263-1276) means every downstream consumer —readLogs,waitForStepResult(engine.ts:710-714,746-748), and the binding output (binding.ts:434) — reads the already-redacted value from the same persisted log metadata. No consumer sees the raw value. - The real value still flows downstream.
persistStepResultstores/returns the unmodifiedresult, soredactOutputonly affects the log payload. The testshould redact ... while passing the real value downstreamcovers this precisely. - Errors are never redacted. Redaction is confined to the success path, so
ATTEMPT_FAILURE/STEP_FAILUREerror messages are untouched — verified by theshould not redact a sensitive step's errortest. - Streams handled. When
redactOutputis set,streamOutputmeta is omitted (context.ts:1271-1274) andresultbecomes[REDACTED], so a sensitive stream can't be replayed viawaitForStepResult. Good. sensitivepropagates via config.toEngineStepConfigspreads...config, soctx.config.sensitiveis exposed to callbacks, matching prod.- Validator is appropriately strict —
z.literal("output")rejectssensitive: "all"/sensitive: true, both covered invalidators.test.ts.
Minor observations (non-blocking)
- Changeset targeting —
@cloudflare/workflows-sharedisprivate, so targetingwrangler/miniflare(minor) in the changeset is the correct user-facing choice. Just confirming this is intentional and not an oversight. REDACTED_STEP_OUTPUTexport surface — it's exported fromsrc/context.tspurely for the test. That's fine, but if you'd prefer to keep it internal you could colocate the constant in a test helper instead. Not important.
Suggestion (optional)
Consider a test asserting ctx.config.sensitive === "output" is observable inside the step callback, since that's now part of the public-facing EngineStepConfig shape and prod parity. Currently only the redaction side-effect is tested.
Overall LGTM.
|
Review posted successfully to PR #14742. SummaryI reviewed PR #14742 ( Key findings (all positive):
Minor, non-blocking notes:
Overall LGTM. |
a021392 to
35d8f07
Compare
35d8f07 to
1acfba1
Compare
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Fixes WOR-1485.
Adds
sensitive: outputto step outputs, matching prod behavior.A picture of a cute animal (not mandatory, but encouraged)