fix(workflow): persist execution metadata and fix SQL metadata filters#1085
fix(workflow): persist execution metadata and fix SQL metadata filters#1085
Conversation
🦋 Changeset detectedLatest commit: ea3e904 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis PR adds workflow execution metadata filtering support by persisting options.metadata in workflow execution states, accepting metadata in server schemas, and fixing JSON metadata query comparisons for LibSQL and Cloudflare D1 adapters to enable tenant/user-aware filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Server<br/>(Schema)
participant Workflow as Workflow Core<br/>(core.ts)
participant Memory as Memory Adapter<br/>(queryWorkflowRuns)
participant DB as Database
Client->>Server: POST /workflows/executions<br/>{options: {metadata: {...}}}
Server->>Server: Validate with<br/>WorkflowExecutionRequestSchema
Server->>Workflow: Execute workflow<br/>with metadata in options
Workflow->>Workflow: Extract options.metadata<br/>into workflow state
Workflow->>Memory: Update state with<br/>persisted metadata
Memory->>DB: Write workflow state<br/>with metadata field
Client->>Server: GET /workflows/executions<br/>?metadata.tenant=x
Server->>Memory: Query with<br/>metadata filter
Memory->>DB: Execute query with<br/>json_extract(metadata, path)<br/>= json_extract(json(value), '$')
DB->>Memory: Return filtered<br/>results
Memory->>Server: Decode JSON fields<br/>& return executions
Server->>Client: Filtered workflow executions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Deploying voltagent with
|
| Latest commit: |
ea3e904
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4615fbbc.voltagent.pages.dev |
| Branch Preview URL: | https://fix-workflow-execution-metad.voltagent.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cloudflare-d1/src/memory-adapter.spec.ts (1)
57-60: Decodedmetadatafield not asserted.The mock row has
metadata: '{"tenantId":"acme"}', but the decoded result is never checked. The three other JSON-decoded fields (input,context,workflowState) are verified; skippingmetadataleaves its decode path untested.🔍 Suggested additional assertion
expect(result[0]?.workflowState).toEqual({ phase: "done" }); +expect(result[0]?.metadata).toEqual({ tenantId: "acme" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cloudflare-d1/src/memory-adapter.spec.ts` around lines 57 - 60, Add an assertion that verifies the decoded metadata field on the returned row: after the existing checks on result, assert that result[0]?.metadata equals the parsed object { tenantId: "acme" } (the mock row used has metadata: '{"tenantId":"acme"}'), so the decode path for metadata is covered alongside input, context, and workflowState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cloudflare-d1/src/memory-adapter.spec.ts`:
- Around line 65-70: The test currently asserts the lower-bound "created_at >="
but never checks the upper-bound; update the spec to also assert that the
generated SQL string contains the upper-bound condition "created_at <=" (e.g.,
add expect(sql).toContain("created_at <=")) and optionally verify the
corresponding args entry in the args array matches the supplied `to` value (use
the existing `sql` and `args` variables in memory-adapter.spec.ts to locate
where to add these assertions).
---
Nitpick comments:
In `@packages/cloudflare-d1/src/memory-adapter.spec.ts`:
- Around line 57-60: Add an assertion that verifies the decoded metadata field
on the returned row: after the existing checks on result, assert that
result[0]?.metadata equals the parsed object { tenantId: "acme" } (the mock row
used has metadata: '{"tenantId":"acme"}'), so the decode path for metadata is
covered alongside input, context, and workflowState.
| expect(sql).toContain("workflow_id = ?"); | ||
| expect(sql).toContain("status = ?"); | ||
| expect(sql).toContain("created_at >="); | ||
| expect(sql).toContain("user_id = ?"); | ||
| expect(sql).toContain("json_extract(metadata, ?) = json_extract(json(?), '$')"); | ||
| expect(sql).toContain("ORDER BY created_at DESC"); |
There was a problem hiding this comment.
to-filter SQL condition is not asserted.
The test verifies created_at >= (for from) and includes the to-date value in the args array, but never checks the upper-bound SQL condition (e.g., created_at <=). A bug that emits the wrong operator or omits the clause entirely would pass this test.
🔍 Suggested additional assertion
expect(sql).toContain("created_at >=");
+expect(sql).toContain("created_at <=");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(sql).toContain("workflow_id = ?"); | |
| expect(sql).toContain("status = ?"); | |
| expect(sql).toContain("created_at >="); | |
| expect(sql).toContain("user_id = ?"); | |
| expect(sql).toContain("json_extract(metadata, ?) = json_extract(json(?), '$')"); | |
| expect(sql).toContain("ORDER BY created_at DESC"); | |
| expect(sql).toContain("workflow_id = ?"); | |
| expect(sql).toContain("status = ?"); | |
| expect(sql).toContain("created_at >="); | |
| expect(sql).toContain("created_at <="); | |
| expect(sql).toContain("user_id = ?"); | |
| expect(sql).toContain("json_extract(metadata, ?) = json_extract(json(?), '$')"); | |
| expect(sql).toContain("ORDER BY created_at DESC"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cloudflare-d1/src/memory-adapter.spec.ts` around lines 65 - 70, The
test currently asserts the lower-bound "created_at >=" but never checks the
upper-bound; update the spec to also assert that the generated SQL string
contains the upper-bound condition "created_at <=" (e.g., add
expect(sql).toContain("created_at <=")) and optionally verify the corresponding
args entry in the args array matches the supplied `to` value (use the existing
`sql` and `args` variables in memory-adapter.spec.ts to locate where to add
these assertions).
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
/workflows/executionsmetadata filters (metadataJSON andmetadata.<key>) can fail even when execution metadata is provided.json_extract(metadata, ?) = json(?), which does not match text values like"acme".What is the new behavior?
options.metadatais persisted on workflow state creation.options.metadata.options.metadatafixes (issue)
N/A
Notes for reviewers
.changeset/workflow-execution-metadata-filters.mdSummary by cubic
Persisted workflow execution metadata and fixed metadata filters so /workflows/executions reliably matches JSON values across adapters. Server now accepts options.metadata, and metadata is preserved on cancel/error updates.
New Features
Bug Fixes
Written for commit ea3e904. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes