[types] add restart-from-step support to WorkflowInstance.restart()#6701
[types] add restart-from-step support to WorkflowInstance.restart()#6701avenceslau wants to merge 3 commits intomainfrom
Conversation
…from: { name, count?, type? } }
Fix the request body sent by WorkflowInstance.restart() to use 'from' instead of 'step', matching the binding shim's expected field name. Add tests for restart with no options, with step name only, and with all options (name, count, type).
|
UnknownError: ProviderInitError |
|
@avenceslau Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
The generated output of Full Type Diffdiff -r types/generated-snapshot/latest/index.d.ts bazel-bin/types/definitions/latest/index.d.ts
15225a15226,15239
> interface WorkflowInstanceRestartOptions {
> /**
> * Restart from a specific step. If omitted, the instance restarts from the beginning.
> * The step must exist in the instance's execution history.
> */
> from?: {
> /** The step name as defined in your workflow code. */
> name: string;
> /** 1-indexed occurrence of this step name. Defaults to 1. Use when the same step name appears multiple times (e.g. in a loop). */
> count?: number;
> /** Step type filter. Use when different step types share the same name. */
> type?: "do" | "sleep" | "waitForEvent";
> };
> }
15241c15255,15257
< * Restart the instance.
---
> * Restart the instance. Optionally restart from a specific step, preserving
> * cached results for all steps before it.
> * @param options Options for the restart, including an optional step to restart from.
15243c15259
< public restart(): Promise<void>;
---
> public restart(options?: WorkflowInstanceRestartOptions): Promise<void>;
diff -r types/generated-snapshot/latest/index.ts bazel-bin/types/definitions/latest/index.ts
15177a15178,15191
> export interface WorkflowInstanceRestartOptions {
> /**
> * Restart from a specific step. If omitted, the instance restarts from the beginning.
> * The step must exist in the instance's execution history.
> */
> from?: {
> /** The step name as defined in your workflow code. */
> name: string;
> /** 1-indexed occurrence of this step name. Defaults to 1. Use when the same step name appears multiple times (e.g. in a loop). */
> count?: number;
> /** Step type filter. Use when different step types share the same name. */
> type?: "do" | "sleep" | "waitForEvent";
> };
> }
15193c15207,15209
< * Restart the instance.
---
> * Restart the instance. Optionally restart from a specific step, preserving
> * cached results for all steps before it.
> * @param options Options for the restart, including an optional step to restart from.
15195c15211
< public restart(): Promise<void>;
---
> public restart(options?: WorkflowInstanceRestartOptions): Promise<void>; |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR extends WorkflowInstance.restart() to accept an optional from parameter for restarting from a specific step. Types and implementation look correct, and backward compatibility is preserved (JSON.stringify drops undefined values so the no-argument case is unchanged).
Issues found (ranked by severity):
- [MEDIUM] Mock doesn't validate
frompayload, so tests don't actually verify the parameter is passed through. The/restartmock returns 200 unconditionally regardless of body content.testRestartFromStepNameOnlyandtestRestartFromStepAllOptionsonly verify no error is thrown -- a regression that silently dropsfromwould pass all tests. - [LOW]
check-snapshotCI is failing. The generated types snapshot needs to be regenerated withjust generate-types. (Already flagged by the CI bot.)
This review was generated by an AI assistant. Findings may contain errors -- please verify before acting on them.
| } | ||
|
|
||
| if (reqUrl.pathname === '/restart' && request.method === 'POST') { | ||
| return Response.json({}, { status: 200 }); |
There was a problem hiding this comment.
The mock returns 200 unconditionally without inspecting the request body. This means the testRestartFromStepNameOnly and testRestartFromStepAllOptions tests only verify the call doesn't throw -- they don't assert that from is actually included in the request payload. A regression that silently drops the from field would pass all tests.
Consider validating the body and returning the from data so the tests can assert on it. For example:
| return Response.json({}, { status: 200 }); | |
| if (reqUrl.pathname === '/restart' && request.method === 'POST') { | |
| return Response.json({ result: { from: data.from } }, { status: 200 }); | |
| } |
Then in the test assertions, verify the response carries the expected from value back (or at minimum, assert data.from is defined when it should be).
|
Review posted on PR #6701. Two findings:
|
2 participants
Extends WorkflowInstance.restart() to accept an optional from parameter, allowing users to restart a workflow instance from a specific step instead of from the beginning.