workflows: rollback types

[vaishnav-mk]

Adds Workers types for optional step.do(name, config?, callback, { rollback, rollbackConfig }) Workflow rollback options.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds TypeScript types for Workflow step.do() rollback options (WorkflowRollbackContext, WorkflowRollbackHandler, WorkflowStepRollbackOptions).

Issues

1. [MEDIUM] WorkflowStepRollbackOptions<T> generic not threaded through do() overloads — The WorkflowStepRollbackOptions type accepts a generic T that flows to WorkflowRollbackContext<T>.output, but both do() overloads use the bare WorkflowStepRollbackOptions (defaulting T to unknown). This means ctx.output in the rollback handler is always unknown, even though the step's return type T is known. Users will have to cast output manually, which defeats the purpose of the generic. See inline suggestion.

2. [LOW] Type tests assert output is unknown — masks issue #1 — The type tests currently assert ctx.output is unknown, which passes but only because the generic isn't wired through. If/when you fix issue #1, the tests for the first overload should assert the inferred type (e.g., string), which would catch any future regression.

---

These comments were generated by an AI code reviewer and may not be perfect. Please use your judgment when evaluating them.

[ask-bonk]

I've posted a review on PR #6766 with 2 findings:

1. [MEDIUM] Generic T not threaded through do() overloads — Both do() overloads use bare WorkflowStepRollbackOptions without passing T, so ctx.output in the rollback handler is always unknown even when the step's return type is known. Inline suggestions provided for both overloads in types/defines/rpc.d.ts.

2. [LOW] Type tests mask the issue — The tests assert ctx.output is unknown, which only passes because of issue Rename BUILD -> BUILD.bazel #1. Suggested updating the assertion to string to match the inferred callback return type once the generic is wired through.

github run

[codspeed-hq]

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 129 skipped benchmarks¹

---

_{Comparing vaish/rollbacks-types (b1e522d) with main (76ddb71)}

Footnotes

1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[petebacondarwin]

/bonk review

[ask-bonk]

Previous review findings have all been addressed — WorkflowStepRollbackOptions<T> is now correctly threaded through both do() overloads, and the type tests assert string | undefined instead of unknown.

Remaining findings:

1. [LOW] No backward-compatibility test for do() without rollback options — All three test cases pass a rollback options object (even {}). Consider adding a test that calls do() without the trailing parameter to verify the existing API shape still works.

2. [LOW] Separate import type statement — import type {WorkflowStep} on line 12 could be folded into the existing import block on lines 5-11 using inline type syntax (import { ..., type WorkflowStep }) to reduce import clutter.

---

These comments were generated by an AI code reviewer and may not be perfect. Please use your judgment when evaluating them.

[ask-bonk]

Consider adding tests without any rollback options to verify backward compatibility of the existing overloads:

Suggested change

	workflowStep.do('empty rollback options', async () => 'ok', {});
	workflowStep.do('empty rollback options', async () => 'ok', {});
	// Verify existing call signatures still work without rollback options
	workflowStep.do('no rollback 2-arg', async (): Promise<string> => 'ok');
	workflowStep.do('no rollback 3-arg', {retries: {limit: 1, delay: 0}}, async (): Promise<string> => 'ok');

[ask-bonk]

Nit: this import could be merged with the block above using inline type syntax:

Suggested change

	import type {WorkflowStep} from 'cloudflare:workers';
	import {
	cache,
	DurableObject,
	RpcStub,
	RpcTarget,
	WorkerEntrypoint,
	type WorkflowStep,
	} from 'cloudflare:workers';

[ask-bonk]

I've posted a review on PR #6766 with 2 findings:

1. [LOW] No backward-compatibility test for do() without rollback options — All test cases pass a rollback options object. Suggested adding tests that exercise the original do() signatures without the trailing parameter to verify backward compatibility.

2. [LOW] Separate import type statement — The standalone import type {WorkflowStep} could be merged into the existing import block using inline type syntax.

The previous review's MEDIUM finding (generic T not threaded through do() overloads) and LOW finding (tests asserting unknown instead of string) have both been addressed in the current revision. The type definitions, generated snapshots, and test coverage are all consistent.

github run