allow passing { unstableArgs } to step.run*#225
Merged
ianmacartney merged 5 commits intomainfrom Apr 15, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ianmacartney
changed the title
ianmacartney
changed the title
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/workflowContext.ts (1)
163-177: 🛠️ Refactor suggestion | 🟠 MajorDestructure
unstableArgsfrom opts to avoid leaking it intoschedulerOptions.Unlike
runFunction(line 222) which correctly destructuresunstableArgsout before spreading toschedulerOptions, this function only extractsname. This meansunstableArgswill be included inschedulerOptions, which is inconsistent and could cause issues ifschedulerOptionsis validated downstream.♻️ Proposed fix
runWorkflow: async (workflow, args, opts?) => { - const { name, ...schedulerOptions } = opts ?? {}; + const { name, unstableArgs, ...schedulerOptions } = opts ?? {}; return run(sender, { name: name ?? safeFunctionName(workflow), target: { kind: "workflow", function: workflow, args, }, retry: undefined, inline: false, - unstableArgs: opts?.unstableArgs ?? false, + unstableArgs: unstableArgs ?? false, schedulerOptions, }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/workflowContext.ts` around lines 163 - 177, The runWorkflow implementation leaks unstableArgs into schedulerOptions because it only destructures name from opts; update runWorkflow to also destructure unstableArgs from opts (e.g., const { name, unstableArgs, ...schedulerOptions } = opts ?? {}) before passing schedulerOptions to run so unstableArgs is not spread into schedulerOptions and instead use unstableArgs in the unstableArgs field (matching runFunction behavior); edit the runWorkflow function where run is called (symbols: runWorkflow, opts, unstableArgs, schedulerOptions, run, safeFunctionName, sender) to apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/workflowContext.ts`:
- Around line 163-177: The runWorkflow implementation leaks unstableArgs into
schedulerOptions because it only destructures name from opts; update runWorkflow
to also destructure unstableArgs from opts (e.g., const { name, unstableArgs,
...schedulerOptions } = opts ?? {}) before passing schedulerOptions to run so
unstableArgs is not spread into schedulerOptions and instead use unstableArgs in
the unstableArgs field (matching runFunction behavior); edit the runWorkflow
function where run is called (symbols: runWorkflow, opts, unstableArgs,
schedulerOptions, run, safeFunctionName, sender) to apply this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 642092c8-79c5-4c3f-99c9-9f0ef093562c
📒 Files selected for processing (2)
src/client/step.tssrc/client/workflowContext.ts
ianmacartney
force-pushed
the
ian/unstable-args-option
branch
from
a731cc5 to
9f25441
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/workflowContext.ts (1)
163-175: HardenrunWorkflowagainst unsupportedinlineoption.At Line 164,
opts.inlinecan still flow intoschedulerOptionsforrunWorkflow. Since inline execution is not supported for workflows, explicitly reject or strip it to avoid silent misuse.Proposed fix
- runWorkflow: async (workflow, args, opts?) => { - const { name, unstableArgs, ...schedulerOptions } = opts ?? {}; + runWorkflow: async (workflow, args, opts?) => { + const { name, unstableArgs, inline, ...schedulerOptions } = opts ?? {}; + if (inline !== undefined) { + throw new Error("`inline` is only supported for runQuery/runMutation."); + } return run(sender, { name: name ?? safeFunctionName(workflow), target: { kind: "workflow", function: workflow, args, }, retry: undefined, inline: false, unstableArgs: unstableArgs ?? false, schedulerOptions, }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/workflowContext.ts` around lines 163 - 175, The runWorkflow implementation allows opts.inline to leak into schedulerOptions which is unsupported; in the runWorkflow function detect if opts?.inline is set and either throw a clear error (e.g., "inline execution not supported for workflows") or explicitly remove/ignore the inline property before building schedulerOptions—update the destructuring around const { name, unstableArgs, ...schedulerOptions } = opts ?? {} to grab inline (const { name, unstableArgs, inline, ...schedulerOptions } = opts ?? {}) and then either if (inline) throw new Error(...) or ensure schedulerOptions does not contain inline before calling run so workflows cannot be started with inline execution.src/client/stepContext.test.ts (1)
460-470: Make the drain loop less brittle.The fixed loop bound (
8) can become a hidden hang point when this test is extended. Prefer a named expected count and assert it explicitly.Proposed fix
test("unstableArgs passes through and defaults correctly", async () => { const channel = new BaseChannel<StepRequest>(0); const ctx = createWorkflowCtx("wf-test" as any, channel); const calls: StepRequest[] = []; + const expectedCalls = 8; const drain = async () => { - for (let i = 0; i < 8; i++) { + for (let i = 0; i < expectedCalls; i++) { const msg = await channel.get(); calls.push(msg); msg.resolve({ kind: "success", returnValue: null }); } }; @@ await Promise.all([ @@ drain(), ]); + expect(calls).toHaveLength(expectedCalls); expect(calls[0].unstableArgs).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/stepContext.test.ts` around lines 460 - 470, The drain loop in the "unstableArgs passes through and defaults correctly" test is brittle because it uses a hardcoded 8; replace the magic number with a named constant (e.g., const expectedCount = X) and use that constant in the drain loop (for or while based on calls.length < expectedCount) to drive reads from BaseChannel<StepRequest>, then assert at the end that calls.length === expectedCount to make the test explicit and self-documenting; update references in the test to use expectedCount instead of 8 and keep the msg.resolve logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/stepContext.test.ts`:
- Around line 460-470: The drain loop in the "unstableArgs passes through and
defaults correctly" test is brittle because it uses a hardcoded 8; replace the
magic number with a named constant (e.g., const expectedCount = X) and use that
constant in the drain loop (for or while based on calls.length < expectedCount)
to drive reads from BaseChannel<StepRequest>, then assert at the end that
calls.length === expectedCount to make the test explicit and self-documenting;
update references in the test to use expectedCount instead of 8 and keep the
msg.resolve logic unchanged.
In `@src/client/workflowContext.ts`:
- Around line 163-175: The runWorkflow implementation allows opts.inline to leak
into schedulerOptions which is unsupported; in the runWorkflow function detect
if opts?.inline is set and either throw a clear error (e.g., "inline execution
not supported for workflows") or explicitly remove/ignore the inline property
before building schedulerOptions—update the destructuring around const { name,
unstableArgs, ...schedulerOptions } = opts ?? {} to grab inline (const { name,
unstableArgs, inline, ...schedulerOptions } = opts ?? {}) and then either if
(inline) throw new Error(...) or ensure schedulerOptions does not contain inline
before calling run so workflows cannot be started with inline execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82225fd9-117a-4803-8302-83f83fe210b8
📒 Files selected for processing (3)
src/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/step.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/workflowContext.ts (1)
44-60: Type system allowsinline+ scheduler combinations that should be caught at compile time.The intersection
RunOptions & { inline?: boolean }preserves the union structure ofSchedulerOptions, meaning TypeScript permits{ inline: true, runAt: 123 }or{ inline: true, runAfter: undefined }at compile time. The runtime guard using theinoperator catches these at execution, but compile-time validation would be clearer and safer.To improve this, consider:
- Restructure the type to use a discriminated union that makes
inlineand scheduler options mutually exclusive at the type level, or- Add explicit type tests validating the compile-time contract, and enable the skipped tests in
example/convex/inlineTest.test.tsto ensure runtime behavior is covered in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/workflowContext.ts` around lines 44 - 60, The current signatures for runQuery and runMutation use OptionalRestArgs<RunOptions & { inline?: boolean }, ...> which allows invalid combinations like { inline: true, runAt: 123 } at compile time; change the types so RunOptions is a discriminated union (e.g. RunOptions = { inline: true } | ({ inline?: false } & SchedulerOptions)) and update the OptionalRestArgs usage in runQuery and runMutation to use this new union (referencing RunOptions, SchedulerOptions, OptionalRestArgs, runQuery, runMutation) so inline and scheduling options are mutually exclusive at the type level; also add/enable the compile-time tests in example/convex/inlineTest.test.ts to validate the contract in CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/workflowContext.ts`:
- Around line 23-30: The unstableArgs flag currently weakens replay identity to
only name/kind in step comparison (see unstableArgs and the comparison logic in
src/client/step.ts), which lets a refactor that preserves a custom name
accidentally reuse a journal entry for the wrong target; update the
replay/keying logic to include the operation's specific target identity (e.g.,
include functionType for runQuery/runMutation/runAction and the workflow
identifier for runWorkflow) when unstableArgs is true so the stored key remains
unique, or alternatively short-term add a validation that rejects unstableArgs
when a custom name is explicitly provided (check callers runQuery, runMutation,
runAction, runWorkflow and the comparison code in step.ts) and surface a clear
error instructing the user to remove the name or disable unstableArgs.
---
Nitpick comments:
In `@src/client/workflowContext.ts`:
- Around line 44-60: The current signatures for runQuery and runMutation use
OptionalRestArgs<RunOptions & { inline?: boolean }, ...> which allows invalid
combinations like { inline: true, runAt: 123 } at compile time; change the types
so RunOptions is a discriminated union (e.g. RunOptions = { inline: true } | ({
inline?: false } & SchedulerOptions)) and update the OptionalRestArgs usage in
runQuery and runMutation to use this new union (referencing RunOptions,
SchedulerOptions, OptionalRestArgs, runQuery, runMutation) so inline and
scheduling options are mutually exclusive at the type level; also add/enable the
compile-time tests in example/convex/inlineTest.test.ts to validate the contract
in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 089f2d3a-f071-404b-b0f5-bd4115ab8285
📒 Files selected for processing (1)
src/client/workflowContext.ts
ianmacartney
force-pushed
the
ian/unstable-args-option
branch
2 times, most recently
from
2bbf4ae to
6a680b6
Compare
Member
Author
|
@cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings. |
ianmacartney
force-pushed
the
ian/unstable-args-option
branch
from
6a680b6 to
6ecd002
Compare
ianmacartney
force-pushed
the
ian/unstable-args-option
branch
from
6ecd002 to
5037738
Compare
reeceyang
reviewed
Apr 15, 2026
| * work pool. Cannot be combined with `runAfter` or `runAt`. | ||
| */ | ||
| inline?: boolean; | ||
| inline: true; |
There was a problem hiding this comment.
are the changes to inline related to unstableArgs?
Member
Author
There was a problem hiding this comment.
Only that by adding another arg I realized that "inline" was allowed to be passed to actions and things that didn't actually support it, and you could pass runAfter and inline at the same time which doesn't make sense. So I updated the types to add unstableArgs & fix inline handling
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Added
unstableArgsoption to workflow step execution that allows journal validation to skip argument comparison during replay.Why make this change?
This enables workflows to handle scenarios where step arguments are non-deterministic and may change between executions, such as when arguments are derived from stack traces or other runtime-dependent data. Without this option, such workflows would fail during replay due to argument mismatches in the journal validation.