allow passing { unstableArgs } to step.run*#225
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)(omitted — changes are localized and do not introduce multi-component control-flow requiring a sequence diagram) 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 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
from
05a93e2 to
2bbf4ae
Compare
1 participant
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.
Summary by CodeRabbit