feat(node): contrib integrations + advanced workflows (conditions/gates/sub-workflows/saga)#271
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds advanced Node workflow capabilities (conditions, gates, sub-workflows, saga compensation) and a new Node contrib surface (Express, Fastify, NestJS, OpenTelemetry, Prometheus, Sentry), with matching Rust bindings, tests, packaging/build updates, pre-commit hooks, and expanded Node SDK/docs roadmap documentation. ChangesAdvanced workflow execution features
Node contrib integrations
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebAdapter
participant RestRoutes
participant Queue
participant Worker
Client->>WebAdapter: POST /tasks/enqueue (Express/Fastify/NestJS)
WebAdapter->>RestRoutes: route.handle(queue, request)
RestRoutes->>Queue: enqueue(task, args)
Queue-->>RestRoutes: jobId
RestRoutes-->>WebAdapter: { status: 201, body: {jobId} }
WebAdapter-->>Client: HTTP 201 with jobId
Worker->>Queue: runWorker polls and executes job
Client->>WebAdapter: GET /tasks/jobs/:id/result
WebAdapter->>RestRoutes: route.handle(queue, request)
RestRoutes->>Queue: result(jobId, timeoutMs)
Queue-->>RestRoutes: { status: 200, body: {jobId, status, result} }
RestRoutes-->>WebAdapter: HTTP 200
WebAdapter-->>Client: Job result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Shared flattenQueryParams uses a null-proto object and skips __proto__/constructor/prototype, so user-controlled query keys can't pollute. Resolves CodeQL remote-property-injection in express/fastify.
Collaborator
Author
|
CodeQL — remote property injection (express/fastify contrib query flattening): user-controlled query keys were assigned onto a plain object (prototype-pollution sink). Fixed by a shared |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
crates/taskito-node/src/queue/workflows.rs (1)
342-343: 💤 Low valueStale doc comment — sub-workflows are now supported.
The comment claims the Node SDK "does not yet create sub-workflows," but this PR adds sub-workflow support via
submitSubWorkflowin the tracker and theparent_run_id/parent_node_nameparameters added tosubmit_workflow.📝 Suggested fix
- /// Child (sub-workflow) runs spawned by a run. Always empty for runs - /// submitted by the Node SDK, which does not yet create sub-workflows. + /// Child (sub-workflow) runs spawned by a run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-node/src/queue/workflows.rs` around lines 342 - 343, The documentation comment describing the Child (sub-workflow) runs field contains outdated information claiming the Node SDK "does not yet create sub-workflows." Update this comment to reflect that sub-workflows are now supported in the Node SDK through the submitSubWorkflow function and the parent_run_id and parent_node_name parameters added to submit_workflow. Remove or revise the phrase that states sub-workflows are not yet supported.sdks/node/src/workflows/types.ts (1)
115-116: ⚡ Quick winUse
WorkflowConditionforStepMetadataJson.conditionfor stronger type safety.
condition?: stringallows invalid values in manually-authored specs; using the union keeps compile-time checks consistent withWorkflowStepOptions.♻️ Proposed change
export interface StepMetadataJson { @@ - condition?: string; + condition?: WorkflowCondition; @@ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/workflows/types.ts` around lines 115 - 116, In the StepMetadataJson type definition, the condition field is currently typed as string which allows invalid values. Change the type of the condition field from string to WorkflowCondition to enforce compile-time validation of valid condition values (on_success, on_failure, always) and maintain consistency with WorkflowStepOptions. This ensures only valid condition values can be specified in manually-authored workflow specs.NODE_SDK_TODO.md (1)
37-38: ⚡ Quick winAvoid hardcoded test counts in roadmap/status text.
These numeric counts are brittle and can drift quickly (and the PR context already references a different total). Prefer non-numeric wording or point to CI/test output instead of embedding fixed counts.
Also applies to: 240-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NODE_SDK_TODO.md` around lines 37 - 38, The roadmap document contains hardcoded test counts that are brittle and prone to drift as tests are added or modified. In the "Contrib integrations" row, replace the hardcoded "17 new vitest tests" with non-numeric language such as "comprehensive test coverage" or "extensive test suite". Similarly, in the "Workflows: conditions / gates / sub-workflows / saga" row, replace "12 new tests" with non-numeric wording. Also check line 240 and apply the same fix to any other hardcoded test counts in the document. This approach makes the document maintainable without requiring updates each time test counts change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/taskito-node/src/queue/workflows.rs`:
- Around line 682-721: The enqueue_compensation method enqueues a job but does
not cancel it if the subsequent set_workflow_node_compensation_job binding call
fails, leaving an untracked compensation job that will execute without saga
state machine tracking. Add a rollback mechanism similar to create_deferred_job:
after calling self.storage.enqueue, if set_workflow_node_compensation_job
returns an error, call a job cancellation method on the storage to remove the
already-enqueued job before returning the error. This ensures that failed
bindings do not leave orphaned compensation jobs in the queue.
In `@docs/content/docs/node/workflows/gates.mdx`:
- Around line 96-99: The documentation incorrectly states that all downstream
nodes are skipped when a gate is rejected. Clarify the rejection semantics by
specifying that only default on_success path downstream nodes are skipped, while
on_failure and always type successors can still execute after a gate rejection.
Revise the sentence in the gates.mdx file (around the section discussing
rejection behavior) to accurately reflect that only the default success paths
are blocked, not all downstream execution paths.
- Around line 114-118: The POST endpoint at /hooks/deploy-approval in the
example code handles privileged workflow state transitions through resolveGate()
but contains no authentication, authorization, or webhook signature validation.
Add documentation guidance explaining how to properly secure this endpoint by
implementing appropriate checks such as API key validation, webhook signature
verification, or authorization checks to ensure only trusted and authorized
requests can approve deployments. The hardened example should demonstrate one or
more verification mechanisms before the resolveGate() call executes.
In `@sdks/node/src/contrib/express.ts`:
- Around line 53-55: The catch block in the error handler is exposing raw
internal error messages to API callers by returning err.message directly in the
JSON response, which is a security risk. Instead, log the actual error details
server-side using a logger, and return a generic error message like "Internal
Server Error" to the client in the 500 response. This way, debugging information
remains on the server while protecting sensitive details from being leaked to
API callers.
In `@sdks/node/src/contrib/prometheus.ts`:
- Around line 158-162: The constructor accepts intervalMs without validation,
allowing invalid values like zero, negative numbers, or NaN to be assigned to
this.intervalMs. Add validation logic in the constructor after line 161 to
ensure intervalMs is a positive number, either by throwing an error for invalid
values or by using a minimum threshold to guarantee a reasonable polling
interval. Check both the provided options.intervalMs and the final assigned
value to this.intervalMs to catch invalid inputs early.
In `@sdks/node/src/contrib/rest.ts`:
- Around line 150-159: The buildRestRoutes function currently silently
prioritizes includeRoutes when both includeRoutes and excludeRoutes are
provided, but the documentation states they should be mutually exclusive. Add a
validation check at the beginning of the buildRestRoutes function that throws an
error if both options.includeRoutes and options.excludeRoutes are defined
simultaneously, making misconfiguration detectable rather than silently ignored.
In `@sdks/node/test/integrations/fastify.test.ts`:
- Around line 32-57: The Fastify app instance is closed at the end of the test
body, but if any assertion throws before reaching that line, the app will not be
closed and resources will leak. Wrap the test body logic (starting from queue
creation through the final assertion) in a try/finally block, moving the await
app.close() call to the finally block. This ensures the app is closed regardless
of whether assertions succeed or fail. Apply this pattern to all test functions
mentioned in the comment that create and register the Fastify app with
taskitoFastify.
In `@sdks/node/test/integrations/nest.test.ts`:
- Around line 35-50: The moduleRef.close() call is only executed if all
assertions pass, but if any expect() call fails earlier in the test, the module
context remains open and can destabilize subsequent tests. Wrap the test logic
in a try/finally block ensuring moduleRef.close() executes in the finally clause
regardless of assertion outcomes, or alternatively move the moduleRef creation
and closure logic to beforeEach/afterEach hooks to guarantee cleanup runs after
each test case.
---
Nitpick comments:
In `@crates/taskito-node/src/queue/workflows.rs`:
- Around line 342-343: The documentation comment describing the Child
(sub-workflow) runs field contains outdated information claiming the Node SDK
"does not yet create sub-workflows." Update this comment to reflect that
sub-workflows are now supported in the Node SDK through the submitSubWorkflow
function and the parent_run_id and parent_node_name parameters added to
submit_workflow. Remove or revise the phrase that states sub-workflows are not
yet supported.
In `@NODE_SDK_TODO.md`:
- Around line 37-38: The roadmap document contains hardcoded test counts that
are brittle and prone to drift as tests are added or modified. In the "Contrib
integrations" row, replace the hardcoded "17 new vitest tests" with non-numeric
language such as "comprehensive test coverage" or "extensive test suite".
Similarly, in the "Workflows: conditions / gates / sub-workflows / saga" row,
replace "12 new tests" with non-numeric wording. Also check line 240 and apply
the same fix to any other hardcoded test counts in the document. This approach
makes the document maintainable without requiring updates each time test counts
change.
In `@sdks/node/src/workflows/types.ts`:
- Around line 115-116: In the StepMetadataJson type definition, the condition
field is currently typed as string which allows invalid values. Change the type
of the condition field from string to WorkflowCondition to enforce compile-time
validation of valid condition values (on_success, on_failure, always) and
maintain consistency with WorkflowStepOptions. This ensures only valid condition
values can be specified in manually-authored workflow specs.
🪄 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: 827399bb-6eaa-4690-a679-261a6d1d8e63
⛔ Files ignored due to path filters (1)
sdks/node/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (66)
.pre-commit-config.yamlNODE_SDK_TODO.mdcrates/taskito-node/src/queue/workflows.rscrates/taskito-python/src/py_queue/workflow_ops/test_helpers.rscrates/taskito-python/src/py_workflow/mod.rscrates/taskito-workflows/src/definition.rscrates/taskito-workflows/src/tests.rscrates/taskito-workflows/tests/storage_contract.rsdocs/content/docs/node/contrib/index.mdxdocs/content/docs/node/contrib/meta.jsondocs/content/docs/node/contrib/observability.mdxdocs/content/docs/node/contrib/web-frameworks.mdxdocs/content/docs/node/index.mdxdocs/content/docs/node/meta.jsondocs/content/docs/node/workflows/conditions.mdxdocs/content/docs/node/workflows/fan-out.mdxdocs/content/docs/node/workflows/gates.mdxdocs/content/docs/node/workflows/index.mdxdocs/content/docs/node/workflows/meta.jsondocs/content/docs/node/workflows/saga.mdxdocs/content/docs/node/workflows/sub-workflows.mdxsdks/node/README.mdsdks/node/biome.jsonsdks/node/package.jsonsdks/node/src/contrib/express.tssdks/node/src/contrib/fastify.tssdks/node/src/contrib/nest.tssdks/node/src/contrib/otel.tssdks/node/src/contrib/prometheus.tssdks/node/src/contrib/rest.tssdks/node/src/contrib/sentry.tssdks/node/src/dashboard/index.tssdks/node/src/dashboard/server.tssdks/node/src/workflows/builder.tssdks/node/src/workflows/index.tssdks/node/src/workflows/manager.tssdks/node/src/workflows/tracker.tssdks/node/src/workflows/types.tssdks/node/test/core/cancel.test.tssdks/node/test/core/inspection.test.tssdks/node/test/core/locks.test.tssdks/node/test/core/logger.test.tssdks/node/test/core/options.test.tssdks/node/test/core/roundtrip.test.tssdks/node/test/core/serializer.test.tssdks/node/test/core/typed.test.tssdks/node/test/core/webhooks.test.tssdks/node/test/dashboard/server.test.tssdks/node/test/integrations/cli.test.tssdks/node/test/integrations/express.test.tssdks/node/test/integrations/fastify.test.tssdks/node/test/integrations/nest.test.tssdks/node/test/observability/otel.test.tssdks/node/test/observability/prometheus.test.tssdks/node/test/observability/sentry.test.tssdks/node/test/worker/events.test.tssdks/node/test/worker/mesh.test.tssdks/node/test/worker/periodic.test.tssdks/node/test/workflows/conditions.test.tssdks/node/test/workflows/dag.test.tssdks/node/test/workflows/fanout.test.tssdks/node/test/workflows/gates.test.tssdks/node/test/workflows/saga.test.tssdks/node/test/workflows/subworkflows.test.tssdks/node/tsconfig.jsonsdks/node/tsup.config.ts
💤 Files with no reviewable changes (1)
- docs/content/docs/node/workflows/fan-out.mdx
Collaborator
Author
|
Addressed CodeRabbit (8) + the earlier CodeQL prototype-pollution (×4) on this PR.
cargo + clippy (default & all features) + tsc + biome + 94 vitest green. |
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)
sdks/node/test/integrations/rest.test.ts (1)
18-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden the prototype-pollution regression input to exercise real forbidden-key paths.
This case can be a false negative because object-literal
__proto__is special syntax and may not become an enumerable own query key. Build the input with explicit own properties (including__proto__,constructor, andprototype) so the filter logic is actually tested.Suggested test adjustment
it("ignores prototype-polluting query keys", () => { - const out = flattenQueryParams({ __proto__: "x", constructor: "y", limit: "10" }); + const query = Object.create(null) as Record<string, unknown>; + query.__proto__ = "x"; + query.constructor = "y"; + query.prototype = "z"; + query.limit = "10"; + const out = flattenQueryParams(query); expect(out.limit).toBe("10"); // No pollution of Object.prototype, and the dangerous keys aren't carried over. expect(({} as Record<string, unknown>).x).toBeUndefined(); expect(Object.hasOwn(out, "__proto__")).toBe(false); + expect(Object.hasOwn(out, "constructor")).toBe(false); + expect(Object.hasOwn(out, "prototype")).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/test/integrations/rest.test.ts` around lines 18 - 24, The current test for prototype-pollution in the "ignores prototype-polluting query keys" test case uses object literal syntax for __proto__, which is special JavaScript syntax that doesn't create a regular enumerable own property. This means the test may not actually exercise the filtering logic properly. Instead, build the input object for flattenQueryParams using explicit property assignment (such as Object.assign or Object.defineProperty) to create real own properties including __proto__, constructor, and prototype keys. This ensures the flattenQueryParams function is genuinely tested against enumerable own properties that need to be filtered out.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sdks/node/test/integrations/rest.test.ts`:
- Around line 18-24: The current test for prototype-pollution in the "ignores
prototype-polluting query keys" test case uses object literal syntax for
__proto__, which is special JavaScript syntax that doesn't create a regular
enumerable own property. This means the test may not actually exercise the
filtering logic properly. Instead, build the input object for flattenQueryParams
using explicit property assignment (such as Object.assign or
Object.defineProperty) to create real own properties including __proto__,
constructor, and prototype keys. This ensures the flattenQueryParams function is
genuinely tested against enumerable own properties that need to be filtered out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee10e966-eef4-41dc-868a-575a848f1d6c
📒 Files selected for processing (8)
crates/taskito-node/src/queue/workflows.rsdocs/content/docs/node/workflows/gates.mdxsdks/node/src/contrib/express.tssdks/node/src/contrib/prometheus.tssdks/node/src/contrib/rest.tssdks/node/test/integrations/fastify.test.tssdks/node/test/integrations/nest.test.tssdks/node/test/integrations/rest.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/content/docs/node/workflows/gates.mdx
🚧 Files skipped from review as they are similar to previous changes (5)
- sdks/node/test/integrations/nest.test.ts
- sdks/node/test/integrations/fastify.test.ts
- sdks/node/src/contrib/rest.ts
- sdks/node/src/contrib/prometheus.ts
- crates/taskito-node/src/queue/workflows.rs
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.
What
createDashboardHandler), NestJS module. Each an optionaltaskito/contrib/*subpath export with an optional peer dep.WorkflowTrackerover the worker outcome stream:on_success/on_failure/always)resolveGate/approveGate/rejectGate, timeouts)Verification
cargo check/clippy -D warnings(default +postgres,redis,mesh,workflows) ✓napi build --release✓ ·tsc✓ · biome ✓ · tsup ✓vitest run— 90 tests pass ✓Notes
server.ts(kept feat(node): dashboard server, events/middleware, webhooks #269/feat(node): bind mesh + workflows, locks, periodic, fan-out #270 hardening + this branch'screateDashboardHandlerextraction + leveled logger),NODE_SDK_TODO.md(dropped hardcoded test counts), movedwebhooks.test.ts→test/core/keeping the validation test.Summary by CodeRabbit