refactor: replace workflow db queries with wrapper functions #1111
Conversation
Sithaarth24
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
Summary of ChangesHello @Sithaarth24, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring effort to improve the architecture of database interactions related to workflows and workflow tools. It moves direct Drizzle ORM queries from the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces direct DB queries in workflow APIs with centralized service/DB helpers, adds permission-checked public fetchers, zod-based result normalization, batch create/update helpers for executions/steps/tools, agent-wiring for AI tools, and moves execution start to async/non-blocking flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as server/api/workflow.ts
participant DBH as Service/DB helpers
participant DB
rect rgba(240,248,255,0.9)
note right of Client: List templates
Client->>API: GET /workflows/templates
API->>DBH: getAccessibleWorkflowTemplates(workspaceId, userId)
DBH->>DB: query templates
DB-->>DBH: templates
DBH-->>API: normalized templates
API-->>Client: 200 OK
end
rect rgba(245,255,245,0.9)
note right of Client: Get template (public check)
Client->>API: GET /workflows/templates/:id
API->>DBH: getWorkflowTemplateByIdWithPublicCheck(id, workspaceId, userId)
DBH->>DB: fetch template, steps, tools, access
DB-->>DBH: entities or null
alt found
DBH-->>API: populated template
API-->>Client: 200 OK
else not found
API-->>Client: 404 Not Found
end
end
rect rgba(255,250,240,0.9)
note right of Client: Execute workflow (async)
Client->>API: POST /workflows/:id/execute
API->>DBH: getWorkflowTemplateByIdWithPublicCheck(id, workspaceId, userId)
DBH->>DB: fetch template, steps, tools
DB-->>DBH: entities
API->>DBH: createWorkflowExecution(status="queued")
DBH->>DB: insert execution
DB-->>DBH: execution
DBH-->>API: execution
API-->>Client: 202 Accepted
Note over API,DBH: Background: create step executions, tool executions, agent creation
API->>DBH: createWorkflowStepExecutionsFromSteps(...) (async)
DBH->>DB: insert step executions & tool executions
DB-->>DBH: results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring effort to abstract database queries into wrapper functions, which significantly improves code structure and maintainability. However, I've identified a critical syntax error that would cause a crash, a couple of performance regressions where bulk database operations were replaced with multiple individual calls, and a data loss issue where timestamps are no longer being saved. I've also pointed out some minor areas for code cleanup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/workflow.ts (1)
640-644: Correct mixed status enums
- In the API response (
responseData.rootStepExecution, line ~642), use WorkflowStatus.COMPLETED instead of ToolExecutionStatus.COMPLETED.- In every
.set({ status: … })call that updates a workflow step (where you currently use raw"failed"orToolExecutionStatus.*), replace with the corresponding WorkflowStatus.* enum.- Keep all
ToolExecutionStatus.*usages in tool-execution payloads (e.g. increateToolExecution) unchanged.
🧹 Nitpick comments (9)
server/db/workflowTool.ts (1)
71-86: Consider Zod-parse for runtime validation consistency.Elsewhere (e.g., workflow templates) results are parsed via schema. For parity and safer shape guarantees, parse with
selectWorkflowToolSchema(if available) instead of raw casts.server/db/workflow.ts (2)
192-218: Guard optionalstatusto preserve DB defaults.If
data.statusisundefined, avoid sending it to ensure the column default applies.- const [execution] = await trx - .insert(workflowExecution) - .values({ - workflowTemplateId: data.workflowTemplateId, - userId: data.userId, - workspaceId: data.workspaceId, - name: data.name, - description: data.description, - metadata: data.metadata || {}, - status: data.status, - }) + const values: any = { + workflowTemplateId: data.workflowTemplateId, + userId: data.userId, + workspaceId: data.workspaceId, + name: data.name, + description: data.description, + metadata: data.metadata || {}, + } + if (data.status !== undefined) values.status = data.status + const [execution] = await trx.insert(workflowExecution).values(values) .returning()
240-253: Tiny cleanup: redundantand(...)wrapper.Single predicate doesn’t need
and(). Optional.- const [execution] = await trx - .select() - .from(workflowExecution) - .where(and( - eq(workflowExecution.id, id), - )) + const [execution] = await trx + .select() + .from(workflowExecution) + .where(eq(workflowExecution.id, id)) .limit(1)server/api/workflow.ts (6)
1081-1110: UsecreateToolExecutionwrapper instead of raw inserts.Keeps defaults, types, and shaping consistent; removes duplicate insert error handling paths.
- let toolExecutionRecord - try { - const [execution] = await db - .insert(toolExecution) - .values({ - workflowToolId: tool.id, - workflowExecutionId: executionId, - status: "failed", - result: toolResult.result, - startedAt: new Date(), - completedAt: new Date(), - }) - .returning() - toolExecutionRecord = execution - } catch (dbError) { - ... - const [execution] = await db - .insert(toolExecution) - .values({ ... }) - .returning() - toolExecutionRecord = execution - } + const toolExecutionRecord = await createToolExecution(db, { + workflowToolId: tool.id, + workflowExecutionId: executionId, + status: ToolExecutionStatus.FAILED, + result: toolResult.result, + }) @@ - let toolExecutionRecord - try { - const [execution] = await db - .insert(toolExecution) - .values({ - workflowToolId: tool.id, - workflowExecutionId: executionId, - status: ToolExecutionStatus.COMPLETED, - result: toolResult.result, - startedAt: new Date(), - completedAt: new Date(), - }) - .returning() - toolExecutionRecord = execution - } catch (dbError) { - ... - try { - const [execution] = await db - .insert(toolExecution) - .values({ ... }) - .returning() - toolExecutionRecord = execution - } catch (secondError) { - const [execution] = await db - .insert(toolExecution) - .values({ ... }) - .returning() - toolExecutionRecord = execution - } - } + const toolExecutionRecord = await createToolExecution(db, { + workflowToolId: tool.id, + workflowExecutionId: executionId, + status: ToolExecutionStatus.COMPLETED, + result: toolResult.result, + })Also applies to: 1142-1153, 1165-1180, 1187-1204
2841-2850: Prefer wrappers for tool/step creation.Use
createWorkflowToolandcreateWorkflowStepTemplateto keep behavior consistent with the refactor goal and centralize defaults.- const [newTool] = await db - .insert(workflowTool) - .values({ - type: requestData.tool.type, - workspaceId: user.workspaceId, - userId: user.id, - value: requestData.tool.value, - config: requestData.tool.config || {}, - }) - .returning() + const newTool = await createWorkflowTool(db, { + type: requestData.tool.type, + workspaceId: user.workspaceId, + userId: user.id, + value: requestData.tool.value, + config: requestData.tool.config || {}, + }) @@ - const [newStep] = await db - .insert(workflowStepTemplate) - .values({ - workflowTemplateId: templateId, - name: requestData.stepName, - description: requestData.stepDescription || `Step ${stepOrder}`, - type: requestData.stepType || "automated", - parentStepId: null, - prevStepIds: isFirstStep ? [] : [], - nextStepIds: [], - toolIds: [newTool.id], - timeEstimate: requestData.timeEstimate || 300, - metadata: { - icon: getStepIcon(requestData.tool.type), - step_order: stepOrder, - ...requestData.metadata, - }, - }) - .returning() + const newStep = await createWorkflowStepTemplate(db, { + workflowTemplateId: templateId, + name: requestData.stepName, + description: requestData.stepDescription || `Step ${stepOrder}`, + type: requestData.stepType || StepType.AUTOMATED, + parentStepId: undefined, + prevStepIds: [], + nextStepIds: [], + toolIds: [newTool.id], + timeEstimate: requestData.timeEstimate || 300, + metadata: { + icon: getStepIcon(requestData.tool.type), + step_order: stepOrder, + ...requestData.metadata, + }, + })Also applies to: 2863-2881
2307-2316: Also refactor complex template tool/step inserts to wrappers.Aligns with the PR objective and reduces duplication.
- const [createdTool] = await db - .insert(workflowTool) - .values({ - type: tool.type, - workspaceId: user.workspaceId, - userId: user.id, - value: processedValue, - config: processedConfig, - }) - .returning() + const createdTool = await createWorkflowTool(db, { + type: tool.type, + workspaceId: user.workspaceId, + userId: user.id, + value: processedValue, + config: processedConfig, + }) @@ - const [createdStep] = await db - .insert(workflowStepTemplate) - .values({ - workflowTemplateId: templateId, - name: stepData.name, - description: stepData.description || "", - type: stepData.type === "form_submission" || stepData.type === "manual" ? "manual" : "automated", - timeEstimate: 180, - metadata: { ... }, - prevStepIds: [], - nextStepIds: [], - toolIds: [], - }) - .returning() + const createdStep = await createWorkflowStepTemplate(db, { + workflowTemplateId: templateId, + name: stepData.name, + description: stepData.description || "", + type: (stepData.type === "form_submission" || stepData.type === "manual") ? StepType.MANUAL : StepType.AUTOMATED, + timeEstimate: 180, + metadata: { + icon: stepData.metadata?.icon, + step_order: stepData.metadata?.step_order, + schema_version: stepData.metadata?.schema_version, + user_instructions: stepData.metadata?.user_instructions, + ai_model: stepData.metadata?.ai_model, + automated_description: stepData.metadata?.automated_description, + position: node.position, + ...stepData.config, + }, + prevStepIds: [], + nextStepIds: [], + toolIds: [], + })Also applies to: 2343-2364
2673-2678: Nice: access-check before update. Consider wrapper for the update itself.
updateWorkflowTool(trx, id, data)would keep updatedAt handling and return typing consistent inside the transaction.
2889-2895: Optional: no need to passupdatedAtto wrapper.
updateWorkflowTemplatealready setsupdatedAt. Passing it is redundant.Also applies to: 3041-3047
599-606: Safer metadata merge.Avoid
as Objectin spreads. Prefer nullish-coalescing; keeps typing cleaner.- ...(rootStepExecution.metadata as Object || {}), + ...(rootStepExecution.metadata ?? {}), @@ - metadata: { - ...(step.metadata as Object || {}), + metadata: { + ...(step.metadata ?? {}),Also applies to: 3105-3107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/workflow.ts(49 hunks)server/db/workflow.ts(7 hunks)server/db/workflowTool.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/db/workflowTool.ts (2)
server/types.ts (1)
TxnOrClient(319-319)server/db/schema/workflows.ts (2)
SelectWorkflowTool(242-242)workflowTool(115-133)
server/api/workflow.ts (2)
server/db/workflow.ts (11)
getAccessibleWorkflowTemplates(94-112)getWorkflowStepTemplatesByTemplateId(179-190)getWorkflowTemplateByIdWithPublicCheck(58-78)getWorkflowStepTemplateById(166-177)createWorkflowExecution(193-219)createWorkflowStepExecution(290-322)getWorkflowExecutionById(240-253)getWorkflowExecutionByIdWithChecks(221-238)createWorkflowTemplate(27-55)updateWorkflowTemplate(114-129)getWorkflowTemplateById(81-92)server/db/workflowTool.ts (6)
getWorkflowToolsByIds(100-113)getWorkflowToolById(39-50)createToolExecution(147-167)getAccessibleWorkflowTools(71-86)createWorkflowTool(15-37)getWorkflowToolByIdWithChecks(52-69)
server/db/workflow.ts (2)
server/types.ts (1)
TxnOrClient(319-319)server/db/schema/workflows.ts (6)
workflowTemplate(63-87)selectWorkflowTemplateSchema(213-213)SelectWorkflowStepTemplate(239-241)workflowStepTemplate(90-112)SelectWorkflowExecution(243-245)workflowExecution(136-161)
🔇 Additional comments (4)
server/db/workflowTool.ts (1)
52-69: Access-checked fetch looks correct.Query is scoped by id, workspaceId, and userId; returns normalized shape. LGTM.
server/api/workflow.ts (3)
77-89: Good move: centralized DB access via wrappers.Imports align API with the new service-layer helpers. Keeps authorization and shaping consistent.
Also applies to: 90-97
153-157: LGTM: using access-checked/list helpers.These calls correctly scope by workspace/user and simplify the API logic.
Also applies to: 162-166, 178-182, 226-231, 237-241, 243-247
1298-1305: LGTM: secure execution fetches.Using
getWorkflowExecutionByIdWithChecksfor status/details is correct.Also applies to: 1329-1335
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/workflow.ts (1)
639-641: Use WorkflowStatus for workflow/step statuses; avoid raw strings.Several places set workflow/step status with ToolExecutionStatus or "failed". Use WorkflowStatus for these fields to keep types and enum domains consistent.
- status: ToolExecutionStatus.COMPLETED, + status: WorkflowStatus.COMPLETED, @@ - status: "failed", + status: WorkflowStatus.FAILED, @@ - status: ToolExecutionStatus.COMPLETED, + status: WorkflowStatus.COMPLETED, @@ - status: "failed", + status: WorkflowStatus.FAILED, @@ - status: "failed", + status: WorkflowStatus.FAILED, @@ - status: ToolExecutionStatus.COMPLETED, + status: WorkflowStatus.COMPLETED,Lines affected:
- Response shaping: Line 640 (rootStepExecution status)
- Completion checks/updates: Lines 858-862, 903-907, 976-983
- Failure paths: Lines 1112-1116, 1123-1127
- Final completion: Lines 1271-1277
Also applies to: 858-862, 903-907, 976-983, 1112-1116, 1123-1127, 1271-1277
♻️ Duplicate comments (2)
server/api/workflow.ts (1)
572-585: Tool execution wrapper drops startedAt/completedAt; extend API and pass timestamps.Refactor replaced inline inserts that set timestamps with createToolExecution, but wrapper doesn’t accept or set startedAt/completedAt, losing data.
Proposed wrapper change (server/db/workflowTool.ts):
-export const createToolExecution = async ( +export const createToolExecution = async ( trx: TxnOrClient, data: { workflowToolId: string workflowExecutionId: string status?: ToolExecutionStatus result?: any + startedAt?: Date + completedAt?: Date }, ): Promise<SelectToolExecution> => { const [execution] = await trx .insert(toolExecution) .values({ workflowToolId: data.workflowToolId, workflowExecutionId: data.workflowExecutionId, status: data.status || ToolExecutionStatus.PENDING, result: data.result, + startedAt: data.startedAt ?? new Date(), + completedAt: data.completedAt ?? (data.status === ToolExecutionStatus.COMPLETED ? new Date() : null), }) .returning() return execution as SelectToolExecution }Then pass timestamps at call sites:
- toolExecutionRecord = await createToolExecution( + toolExecutionRecord = await createToolExecution( db, { workflowToolId: rootStepTool.id, workflowExecutionId: execution.id, status: ToolExecutionStatus.COMPLETED, result: { formData: processedFormData, submittedAt: new Date().toISOString(), submittedBy: "api", autoCompleted: true, }, + startedAt: new Date(), + completedAt: new Date(), } )- const toolExecutionRecord = await createToolExecution( + const toolExecutionRecord = await createToolExecution( db, { workflowToolId: formTool.id, workflowExecutionId: stepExecution.workflowExecutionId, status: ToolExecutionStatus.COMPLETED, result: { formData: formData, submittedAt: new Date().toISOString(), submittedBy: "demo", } + ,startedAt: new Date() + ,completedAt: new Date() } )Also applies to: 1598-1610
server/db/workflow.ts (1)
159-170: Make PK fetch nullable and limit(1); drop meaningless orderBy.Current code can return undefined while the type is non-null and orders by createdAt on an equality. Return null and cap to 1 row.
Apply:
-export const getWorkflowStepTemplateById = async( - trx: TxnOrClient, - id: string, -): Promise<SelectWorkflowStepTemplate> => { +export const getWorkflowStepTemplateById = async ( + trx: TxnOrClient, + id: string, +): Promise<SelectWorkflowStepTemplate | null> => { const [step] = await trx .select() .from(workflowStepTemplate) - .where(eq(workflowStepTemplate.id, id)) - .orderBy(workflowStepTemplate.createdAt) + .where(eq(workflowStepTemplate.id, id)) + .limit(1) - return step as SelectWorkflowStepTemplate + return step ? (step as SelectWorkflowStepTemplate) : null }
🧹 Nitpick comments (3)
server/db/workflow.ts (1)
233-245: Avoid and() with a single predicate.Minor clarity: use eq(...) directly.
- const [execution] = await trx - .select() - .from(workflowExecution) - .where(and( - eq(workflowExecution.id, id), - )) + const [execution] = await trx + .select() + .from(workflowExecution) + .where(eq(workflowExecution.id, id)) .limit(1)server/api/workflow.ts (2)
1077-1106: Prefer createToolExecution wrapper over direct inserts for consistency.These direct inserts bypass the wrapper, duplicating logic and risking divergence (timestamps, defaults, result sanitation).
- Replace the three db.insert(toolExecution) blocks with createToolExecution(trx/db, {...}) calls.
- If wrapper needs to handle sanitization, add an optional sanitizeResult flag or pre-sanitize the payload before passing.
Also applies to: 1138-1150, 1161-1200
2303-2313: Use wrappers (or add bulk variants) for tool/step template creates.Direct inserts here can be replaced with createWorkflowTool/createWorkflowStepTemplate, or introduce bulk wrappers for performance and consistency.
- For many tools/steps, add createWorkflowTools/createWorkflowStepTemplates(trx, data[]) to batch.
- Keeps updatedAt/default handling centralized and consistent.
Also applies to: 2338-2361
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/api/workflow.ts(50 hunks)server/db/workflow.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/db/workflow.ts (2)
server/types.ts (1)
TxnOrClient(319-319)server/db/schema/workflows.ts (9)
SelectWorkflowTemplate(236-238)workflowTemplate(63-87)selectWorkflowTemplateSchema(213-213)SelectWorkflowStepTemplate(239-241)workflowStepTemplate(90-112)SelectWorkflowExecution(243-245)workflowExecution(136-161)SelectWorkflowStepExecution(246-248)workflowStepExecution(164-190)
server/api/workflow.ts (2)
server/db/workflow.ts (11)
getAccessibleWorkflowTemplates(87-105)getWorkflowStepTemplatesByTemplateId(172-183)getWorkflowTemplateByIdWithPublicCheck(51-71)getWorkflowStepTemplateById(159-170)createWorkflowExecution(186-212)createWorkflowStepExecutions(317-353)getWorkflowExecutionById(233-246)getWorkflowExecutionByIdWithChecks(214-231)createWorkflowTemplate(20-48)updateWorkflowTemplate(107-122)getWorkflowTemplateById(74-85)server/db/workflowTool.ts (6)
getWorkflowToolsByIds(100-113)getWorkflowToolById(39-50)createToolExecution(147-167)getAccessibleWorkflowTools(71-86)createWorkflowTool(15-37)getWorkflowToolByIdWithChecks(52-69)
🔇 Additional comments (4)
server/db/workflow.ts (2)
73-85: Public getter LGTM.Null-safe and schema-validated return looks good.
317-353: Great: bulk step-execution insert wrapper.Eliminates N inserts and aligns with PR goals. Thanks for adding the empty-array fast path too.
server/api/workflow.ts (2)
153-157: Nice adoption of accessors and bulk insert.Swapping to getAccessibleWorkflowTemplates/getWorkflowStepTemplatesByTemplateId and createWorkflowStepExecutions removes N queries and aligns with the PR’s goals.
Also applies to: 439-454, 726-742
2921-2929: Handle potential nulls from getWorkflowTemplateById().These calls can return null; add a null-check (404 or fallback) before using in response.
- Add guard after fetching updatedTemplate; return 404 if missing (deleted concurrently).
Also applies to: 3107-3115
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/db/workflow.ts (1)
159-170: Fix the missing null check (critical issue from past review still unresolved).The function returns
step as SelectWorkflowStepTemplatewithout checking ifstepexists. When no row is found,stepisundefined, but the return type promisesSelectWorkflowStepTemplate | null. This violates the type contract and can cause runtime errors in calling code.Apply this diff to add the null check:
export const getWorkflowStepTemplateById = async( trx: TxnOrClient, id: string, ): Promise<SelectWorkflowStepTemplate | null> => { const [step] = await trx .select() .from(workflowStepTemplate) .where(eq(workflowStepTemplate.id, id)) .limit(1) - return step as SelectWorkflowStepTemplate + return step ? (step as SelectWorkflowStepTemplate) : null }
🧹 Nitpick comments (1)
server/db/workflow.ts (1)
233-246: Simplify: Remove unnecessaryand()wrapper.The
and()wraps a single condition. You can passeq()directly to.where()for cleaner code.Apply this diff:
export const getWorkflowExecutionById = async ( trx: TxnOrClient, id: string, ): Promise<SelectWorkflowExecution | null> => { const [execution] = await trx .select() .from(workflowExecution) - .where(and( - eq(workflowExecution.id, id), - )) + .where(eq(workflowExecution.id, id)) .limit(1) return execution ? (execution as SelectWorkflowExecution) : null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/db/workflow.ts(8 hunks)server/db/workflowTool.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/db/workflowTool.ts (2)
server/types.ts (1)
TxnOrClient(319-319)server/db/schema/workflows.ts (2)
SelectWorkflowTool(242-242)workflowTool(115-133)
server/db/workflow.ts (2)
server/types.ts (1)
TxnOrClient(319-319)server/db/schema/workflows.ts (9)
SelectWorkflowTemplate(236-238)workflowTemplate(63-87)selectWorkflowTemplateSchema(213-213)SelectWorkflowStepTemplate(239-241)workflowStepTemplate(90-112)SelectWorkflowExecution(243-245)workflowExecution(136-161)SelectWorkflowStepExecution(246-248)workflowStepExecution(164-190)
🔇 Additional comments (5)
server/db/workflowTool.ts (2)
51-68: LGTM!The permission-checked helper correctly validates workspace and user ownership before returning the tool. The null handling and type casting are consistent with the existing codebase pattern.
70-85: LGTM!The helper correctly fetches tools accessible to the user within the workspace. The ordering and type casting are consistent with the codebase pattern.
server/db/workflow.ts (3)
74-85: LGTM!The non-checked variant correctly fetches templates by ID without permission validation. The null check and schema parsing are properly implemented.
195-207: LGTM!The optional
statusparameter is correctly added. When undefined, the database default (WorkflowStatus.DRAFT) will be used automatically.
317-353: LGTM!The batch insert helper is correctly implemented. The empty array guard, field mapping with defaults, and efficient batch operation are all properly handled.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/db/workflowTool.ts (2)
42-53: Add null check before parsing to prevent runtime crash.If no row is found,
toolwill beundefined, andselectWorkflowToolSchema.parse(undefined)will throw an error. Since the return type isPromise<SelectWorkflowTool | null>, you should returnnullwhen the row doesn't exist.Apply this diff to fix the issue:
export const getWorkflowToolById = async ( trx: TxnOrClient, id: string, ): Promise<SelectWorkflowTool | null> => { const [tool] = await trx .select() .from(workflowTool) .where(eq(workflowTool.id, id)) .limit(1) - return selectWorkflowToolSchema.parse(tool) + return tool ? selectWorkflowToolSchema.parse(tool) : null }
172-183: Add null check before parsing to prevent runtime crash.Same pattern –
executioncan beundefinedif no row is found, causingparse(undefined)to throw.Apply this diff:
export const getToolExecutionById = async ( trx: TxnOrClient, id: string, ): Promise<SelectToolExecution | null> => { const [execution] = await trx .select() .from(toolExecution) .where(eq(toolExecution.id, id)) .limit(1) - return selectToolExecutionSchema.parse(execution) + return execution ? selectToolExecutionSchema.parse(execution) : null }server/db/workflow.ts (1)
118-150: Fix return type and schema mismatch.The function returns
Promise<InsertWorkflowStepTemplate>and parses withinsertWorkflowStepTemplateSchema, but.insert(...).returning()returns a full database row, which should matchSelectWorkflowStepTemplate. The Insert schema is for input data, not database output.Apply this diff:
export const createWorkflowStepTemplate = async ( trx: TxnOrClient, data: { workflowTemplateId: string name: string description?: string type: StepType parentStepId?: string prevStepIds?: string[] nextStepIds?: string[] toolIds?: string[] timeEstimate?: number metadata?: any }, -): Promise<InsertWorkflowStepTemplate> => { +): Promise<SelectWorkflowStepTemplate> => { const [step] = await trx .insert(workflowStepTemplate) .values({ workflowTemplateId: data.workflowTemplateId, name: data.name, description: data.description, type: data.type, parentStepId: data.parentStepId, prevStepIds: data.prevStepIds || [], nextStepIds: data.nextStepIds || [], toolIds: data.toolIds || [], timeEstimate: data.timeEstimate || 0, metadata: data.metadata || {}, }) .returning() - return insertWorkflowStepTemplateSchema.parse(step) + return selectWorkflowStepTemplateSchema.parse(step) }
♻️ Duplicate comments (1)
server/db/workflow.ts (1)
152-163: [UNRESOLVED] Add null check before parsing to prevent runtime crash.This issue was previously flagged and marked as "resolved in recent commit", but the fix is not present in the current code. If no row is found,
stepwill beundefinedandparse(undefined)will throw.Apply this diff (as originally suggested):
export const getWorkflowStepTemplateById = async ( trx: TxnOrClient, id: string, ): Promise<SelectWorkflowStepTemplate | null> => { const [step] = await trx .select() .from(workflowStepTemplate) .where(eq(workflowStepTemplate.id, id)) .limit(1) - return selectWorkflowStepTemplateSchema.parse(step) + return step ? selectWorkflowStepTemplateSchema.parse(step) : null }
🧹 Nitpick comments (1)
server/api/workflow.ts (1)
596-596: Consider simplifying metadata spread pattern.The
...(rootStepExecution.metadata as Object || {})pattern works, butas Objectis unnecessary since the|| {}fallback handles null/undefined. This pattern appears in several places (lines 836, 3098).For clarity, consider:
- ...(rootStepExecution.metadata as Object || {}), + ...(rootStepExecution.metadata || {}),The
|| {}already provides type safety by ensuring a spreadable object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/workflow.ts(50 hunks)server/db/workflow.ts(16 hunks)server/db/workflowTool.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/api/workflow.ts (2)
server/db/workflow.ts (10)
getAccessibleWorkflowTemplates(80-98)getWorkflowStepTemplatesByTemplateId(165-176)getWorkflowTemplateByIdWithPublicCheck(58-78)getWorkflowStepTemplateById(152-163)createWorkflowExecution(179-205)createWorkflowStepExecutions(310-346)getWorkflowExecutionById(226-239)getWorkflowExecutionByIdWithChecks(207-224)createWorkflowTemplate(27-55)updateWorkflowTemplate(100-115)server/db/workflowTool.ts (6)
getWorkflowToolsByIds(103-116)getWorkflowToolById(42-53)createToolExecution(150-170)getAccessibleWorkflowTools(74-89)createWorkflowTool(18-40)getWorkflowToolByIdWithChecks(55-72)
server/db/workflowTool.ts (2)
server/db/schema/workflows.ts (4)
selectWorkflowToolSchema(216-216)SelectWorkflowTool(242-242)workflowTool(115-133)selectToolExecutionSchema(222-222)server/types.ts (1)
TxnOrClient(319-319)
server/db/workflow.ts (2)
server/db/schema/workflows.ts (10)
selectWorkflowTemplateSchema(213-213)insertWorkflowStepTemplateSchema(225-226)workflowStepTemplate(90-112)selectWorkflowStepTemplateSchema(214-215)selectWorkflowExecutionSchema(217-218)SelectWorkflowExecution(243-245)workflowExecution(136-161)selectWorkflowStepExecutionSchema(219-221)SelectWorkflowStepExecution(246-248)workflowStepExecution(164-190)server/types.ts (1)
TxnOrClient(319-319)
🔇 Additional comments (2)
server/api/workflow.ts (1)
77-96: LGTM – clean service-layer integration.The import reorganization and adoption of centralized DB wrapper functions (
createWorkflowTemplate,getAccessibleWorkflowTemplates,createWorkflowExecution, etc.) significantly improves maintainability and consistency.server/db/workflow.ts (1)
310-346: LGTM – efficient bulk insert with proper validation.The new
createWorkflowStepExecutionsfunction correctly handles batch inserts and validates the array result viaz.array(selectWorkflowStepExecutionSchema).parse(stepExecutions). This addresses the performance concern raised in past reviews about N individual inserts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/db/workflowTool.ts (1)
55-72: LGTM! Past null check issue resolved.The function now correctly checks for null before parsing and returns null when no matching record exists, as suggested in previous reviews.
server/db/workflow.ts (1)
152-163: LGTM! Past issues fully resolved.The function now correctly:
- Uses
.limit(1)for single-result queries- Returns
Promise<SelectWorkflowStepTemplate | null>- Checks for null before parsing
All concerns from previous reviews have been addressed.
🧹 Nitpick comments (1)
server/db/workflow.ts (1)
226-239: Consider simplifying single-condition query.The
and()wrapper with a single condition is unnecessary.Apply this diff:
export const getWorkflowExecutionById = async ( trx: TxnOrClient, id: string, ): Promise<SelectWorkflowExecution | null> => { const [execution] = await trx .select() .from(workflowExecution) - .where(and( - eq(workflowExecution.id, id), - )) + .where(eq(workflowExecution.id, id)) .limit(1) return execution ? selectWorkflowExecutionSchema.parse(execution) : null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/db/workflow.ts(16 hunks)server/db/workflowTool.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/db/workflowTool.ts (2)
server/db/schema/workflows.ts (4)
selectWorkflowToolSchema(216-216)SelectWorkflowTool(242-242)workflowTool(115-133)selectToolExecutionSchema(222-222)server/types.ts (1)
TxnOrClient(319-319)
server/db/workflow.ts (2)
server/db/schema/workflows.ts (11)
selectWorkflowTemplateSchema(213-213)insertWorkflowStepTemplateSchema(225-226)SelectWorkflowStepTemplate(239-241)workflowStepTemplate(90-112)selectWorkflowStepTemplateSchema(214-215)selectWorkflowExecutionSchema(217-218)SelectWorkflowExecution(243-245)workflowExecution(136-161)selectWorkflowStepExecutionSchema(219-221)SelectWorkflowStepExecution(246-248)workflowStepExecution(164-190)server/types.ts (1)
TxnOrClient(319-319)
🔇 Additional comments (5)
server/db/workflowTool.ts (2)
74-89: LGTM!The function correctly implements access control by filtering on both
workspaceIdanduserId, and consistently parses results using the zod schema array validator.
39-39: LGTM! Consistent zod parsing patterns.All functions consistently apply zod schema validation:
- Single-result functions check for null before parsing
- Array-result functions use
z.array(schema).parse()- Return types correctly reflect nullable results
This aligns with the PR's objective to normalize database query results through schema validation.
Also applies to: 52-52, 100-100, 115-115, 132-132, 169-169, 182-182, 195-195, 208-208, 225-225
server/db/workflow.ts (3)
132-132: Verify: Insert schema used for parsing DB result.The function parses the returned database row with
insertWorkflowStepTemplateSchemarather thanselectWorkflowStepTemplateSchema. Insert schemas validate input data before insertion, while Select schemas parse complete database rows (including auto-generated fields likeid,createdAt, etc.).This pattern differs from other create functions in this file (e.g.,
createWorkflowTemplate,createWorkflowExecution) which use Select schemas for parsing returned rows.Please verify this is intentional. If not, apply this diff:
-): Promise<InsertWorkflowStepTemplate> => { +): Promise<SelectWorkflowStepTemplate> => { const [step] = await trx .insert(workflowStepTemplate) .values({ workflowTemplateId: data.workflowTemplateId, name: data.name, description: data.description, type: data.type, parentStepId: data.parentStepId, prevStepIds: data.prevStepIds || [], nextStepIds: data.nextStepIds || [], toolIds: data.toolIds || [], timeEstimate: data.timeEstimate || 0, metadata: data.metadata || {}, }) .returning() - return insertWorkflowStepTemplateSchema.parse(step) + return selectWorkflowStepTemplateSchema.parse(step) }Also applies to: 149-149
310-346: LGTM! Well-implemented batch operation.The function correctly:
- Handles empty input arrays with early return
- Maps input data to insert values with appropriate defaults
- Uses batch insert for efficiency
- Consistently validates results with zod array schema
This aligns with the PR's goal of adding batch create helpers for workflow executions/steps.
54-54: LGTM! Consistent zod validation throughout.All database operations consistently apply schema validation:
- Create operations parse returned rows with Select schemas
- Update operations check for null before parsing
- Array queries use
z.array(schema).parse()- Access-controlled functions filter by workspace/user IDs
- New
statusparameter increateWorkflowExecutionextends functionality appropriatelyThis thorough schema validation aligns with the PR's goal of centralizing and standardizing database query patterns.
Also applies to: 57-78, 97-97, 114-114, 175-175, 188-188, 204-204, 223-223, 255-255, 272-272, 307-307, 358-358, 371-371, 401-401
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
server/api/workflow.ts (4)
625-629: Fix enum/status mismatches (workflow vs tool).You’re setting workflow execution status using ToolExecutionStatus and raw strings. Use WorkflowStatus for workflow_execution rows and ToolExecutionStatus only for tool_execution.
Apply these targeted fixes:
- Line 627: rootStepExecution status should be WorkflowStatus.COMPLETED (not ToolExecutionStatus).
- Lines 831-835, 979-983: use WorkflowStatus.FAILED (not "failed").
- Lines 876-881, 949-957, 1246-1251: use WorkflowStatus.COMPLETED (not ToolExecutionStatus.COMPLETED).
Example patch for one block (replicate similarly in other spots):
- status: ToolExecutionStatus.COMPLETED, + status: WorkflowStatus.COMPLETED,- status: "failed", + status: WorkflowStatus.FAILED,Also applies to: 831-835, 876-881, 949-957, 1246-1251
1380-1392: Add authorization checks for step-execution reads/updates.These endpoints fetch/update workflowStepExecution by id without verifying workspace/user ownership via workflowExecution. A user guessing a stepId could read/mutate another user’s execution.
- SubmitWorkflowFormApi: Lines 1380-1392 and 1501-1513
- GetFormDefinitionApi: Lines 3183-3191
- UpdateWorkflowStepExecutionApi: Lines 3125-3135
- CompleteWorkflowStepExecutionApi: Lines 3154-3161
Recommended: introduce a DB helper and use it here:
// server/db/workflow.ts export const getWorkflowStepExecutionByIdWithChecks = async ( trx: TxnOrClient, id: string, workspaceId: number, userId: number ) => { const [row] = await trx.select({ se: workflowStepExecution }) .from(workflowStepExecution) .innerJoin( workflowExecution, eq(workflowExecution.id, workflowStepExecution.workflowExecutionId) ) .where(and( eq(workflowExecution.workspaceId, workspaceId), eq(workflowExecution.userId, userId), eq(workflowStepExecution.id, id), )) .limit(1) return row ? selectWorkflowStepExecutionSchema.parse(row.se) : null }Then replace direct stepExecution selects/updates to first fetch via this helper and bail with 404 if null; only then perform the update.
Also applies to: 1501-1513, 3183-3191, 3125-3135, 3154-3161
123-137: Avoid logging attachment IDs at info level.Attachment IDs may be sensitive. Downgrade to debug and/or redact.
- Logger.info(`Processing field ${key} with attachment ID: ${file.attachmentId}`) + Logger.debug(`Processing field ${key} with attachment ID: ****${String(file.attachmentId).slice(-4)}`)
1049-1079: Refactor raw database inserts to use wrapper functions for consistency and to prevent logic drift.The review comment is accurate. Raw
db.insert(toolExecution)anddb.insert(workflowTool)calls exist at all identified locations:
toolExecutioninserts: lines 1051, 1065, 1112, 1135, 1157workflowToolinserts: lines 2279, 2810The proposed wrapper functions (
createToolExecution,markToolExecutionFailed,markToolExecutionCompleted,createWorkflowTool) do not yet exist in the file, confirming they need to be created before refactoring these calls.Consolidate these duplicated insert patterns into dedicated wrapper functions to ensure Zod parsing and validation are applied consistently across all tool execution and creation flows.
server/db/workflow.ts (1)
339-347: Add with-checks helpers to support API auth fixes.Consider adding:
- getWorkflowStepExecutionByIdWithChecks (used by API mutations)
- updateWorkflowStepExecutionWithChecks (enforce workspace/user in WHERE)
I can draft these if you want them in this PR.
Also applies to: 349-360
🧹 Nitpick comments (2)
server/api/workflow.ts (2)
1840-1841: Hard-coded timezone in emails.Use user/workspace timezone or UTC to avoid incorrect timestamps.
- ${new Date().toLocaleString("en-US", {timeZone: "Asia/Kolkata"})} + ${new Date().toISOString()}Or inject workspace/user tz from context.
584-591: Defensive spread for metadata.Spreading metadata with “as Object” can misbehave for non-objects. Guard first.
- ...(rootStepExecution.metadata as Object || {}), + ...(typeof rootStepExecution.metadata === 'object' && rootStepExecution.metadata ? rootStepExecution.metadata : {}),Apply similarly at lines 808-820 and 3073-3076.
Also applies to: 808-820, 3073-3076
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/workflow.ts(50 hunks)server/db/workflow.ts(15 hunks)server/db/workflowTool.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/db/workflow.ts (2)
server/db/schema/workflows.ts (11)
selectWorkflowTemplateSchema(213-213)InsertWorkflowStepTemplate(254-256)insertWorkflowStepTemplateSchema(225-226)workflowStepTemplate(90-112)selectWorkflowStepTemplateSchema(214-215)selectWorkflowExecutionSchema(217-218)SelectWorkflowExecution(243-245)workflowExecution(136-161)selectWorkflowStepExecutionSchema(219-221)SelectWorkflowStepExecution(246-248)workflowStepExecution(164-190)server/types.ts (1)
TxnOrClient(319-319)
server/db/workflowTool.ts (2)
server/db/schema/workflows.ts (4)
selectWorkflowToolSchema(216-216)SelectWorkflowTool(242-242)workflowTool(115-133)selectToolExecutionSchema(222-222)server/types.ts (1)
TxnOrClient(319-319)
server/api/workflow.ts (2)
server/db/workflow.ts (10)
getAccessibleWorkflowTemplates(80-97)getWorkflowStepTemplatesByTemplateId(164-174)getWorkflowTemplateByIdWithPublicCheck(58-78)getWorkflowStepTemplateById(151-162)createWorkflowExecution(177-203)createWorkflowStepExecutionsFromSteps(307-335)getWorkflowExecutionById(224-237)getWorkflowExecutionByIdWithChecks(205-222)createWorkflowTemplate(27-55)updateWorkflowTemplate(99-114)server/db/workflowTool.ts (6)
getWorkflowToolsByIds(100-112)getWorkflowToolById(41-52)createToolExecution(146-170)getAccessibleWorkflowTools(73-87)createWorkflowTool(17-39)getWorkflowToolByIdWithChecks(54-71)
🔇 Additional comments (3)
server/db/workflowTool.ts (1)
145-170: LGTM: tool execution wrapper supports timestamps.createToolExecution now accepts startedAt/completedAt; aligns with API usage and prior review.
server/db/workflow.ts (2)
151-162: LGTM: getWorkflowStepTemplateById now limits(1) and returns null.Correct fix; removes undefined risk and aligns with parsed return.
307-335: Nice: bulk create of step executions.createWorkflowStepExecutionsFromSteps prevents N inserts and returns parsed rows.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/db/workflow.ts (1)
117-149: Return type mismatch: should return SelectWorkflowStepTemplate.The function uses
.returning()which provides the full inserted row including generated fields (id,createdAt,updatedAt), but the return type isPromise<InsertWorkflowStepTemplate>. Insert types represent input data, while Select types represent complete rows from the database.Apply this diff:
export const createWorkflowStepTemplate = async ( trx: TxnOrClient, data: { workflowTemplateId: string name: string description?: string type: StepType parentStepId?: string prevStepIds?: string[] nextStepIds?: string[] toolIds?: string[] timeEstimate?: number metadata?: any }, -): Promise<InsertWorkflowStepTemplate> => { +): Promise<SelectWorkflowStepTemplate> => { const [step] = await trx .insert(workflowStepTemplate) .values({ workflowTemplateId: data.workflowTemplateId, name: data.name, description: data.description, type: data.type, parentStepId: data.parentStepId, prevStepIds: data.prevStepIds || [], nextStepIds: data.nextStepIds || [], toolIds: data.toolIds || [], timeEstimate: data.timeEstimate || 0, metadata: data.metadata || {}, }) .returning() - return insertWorkflowStepTemplateSchema.parse(step) + return selectWorkflowStepTemplateSchema.parse(step) }
🧹 Nitpick comments (2)
server/db/workflow.ts (2)
224-237: Remove redundantand()wrapper.The
and()combinator is meant for multiple conditions; with a single condition, passeq()directly towhere().Apply this diff:
export const getWorkflowExecutionById = async ( trx: TxnOrClient, id: string, ): Promise<SelectWorkflowExecution | null> => { const [execution] = await trx .select() .from(workflowExecution) - .where(and( - eq(workflowExecution.id, id), - )) + .where(eq(workflowExecution.id, id)) .limit(1) return execution ? selectWorkflowExecutionSchema.parse(execution) : null }
307-335: Minor: clarify comment about status default.Line 319 explicitly sets
WorkflowStatus.DRAFTrather than relying on the table's default. The comment "Default status from table schema" is slightly misleading.Consider updating the comment to be more precise:
name: step.name, type: step.type, - status: WorkflowStatus.DRAFT, // Default status from table schema + status: WorkflowStatus.DRAFT, // Set initial status parentStepId: step.parentStepId,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/db/workflow.ts(15 hunks)server/db/workflowTool.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/db/workflowTool.ts (2)
server/db/schema/workflows.ts (5)
selectWorkflowToolSchema(216-216)SelectWorkflowTool(242-242)workflowTool(115-133)toolExecution(193-210)selectToolExecutionSchema(222-222)server/types.ts (1)
TxnOrClient(319-319)
server/db/workflow.ts (2)
server/db/schema/workflows.ts (10)
selectWorkflowTemplateSchema(213-213)insertWorkflowStepTemplateSchema(225-226)workflowStepTemplate(90-112)selectWorkflowStepTemplateSchema(214-215)selectWorkflowExecutionSchema(217-218)SelectWorkflowExecution(243-245)workflowExecution(136-161)selectWorkflowStepExecutionSchema(219-221)SelectWorkflowStepExecution(246-248)workflowStepExecution(164-190)server/types.ts (1)
TxnOrClient(319-319)
🔇 Additional comments (3)
server/db/workflowTool.ts (1)
1-259: LGTM! Clean implementation of DB wrappers with proper validation.All previously flagged issues have been resolved:
- Correct Zod import form (
import { z } from "zod")- Null checks before parsing in all getters
- Consistent schema validation throughout
The new permission-checked helpers (
getWorkflowToolByIdWithChecks,getAccessibleWorkflowTools) properly filter by workspace and user, and the parsing ensures type safety across all operations.server/db/workflow.ts (2)
151-162: LGTM! Previous issues resolved.The function now correctly:
- Uses
.limit(1)for single-row fetch- Removes unnecessary
.orderBy()on primary key lookup- Returns
nullwhen no row found- Includes null check before parsing
58-78: LGTM! Consistent implementation across all workflow operations.The new and updated functions properly implement:
- Permission-checked access via
getWorkflowTemplateByIdWithPublicCheck- Consistent zod parsing with null safety
- Array normalization in
updateWorkflowStepExecutionusingArray.from()- Proper handling of optional parameters (status in
createWorkflowExecution)Also applies to: 80-97, 177-203, 239-253, 273-305, 362-390
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/workflow.ts (1)
3193-3247: CRITICAL: Implement authentication and workspace validation to prevent IDOR vulnerability.This endpoint has no user context extraction and performs no workspace validation, allowing any authenticated user to access step definitions, form configurations, and tool details from any workspace by simply providing a
stepIdUUID.Attack vector:
// Attacker from Workspace A obtains stepId from Workspace B GET /api/v1/workflow/form/{stepId_from_workspace_B} // Returns form definition and tool config without authorization checkImpact:
- Cross-workspace data leakage
- Exposure of proprietary form definitions and workflow logic
- Potential exposure of sensitive configuration data in tool configs
Required fixes:
- Extract user context:
export const GetFormDefinitionApi = async (c: Context) => { try { + const user = await getUserFromJWT(db, c.get(JwtPayloadKey)) const stepId = c.req.param("stepId")
- Validate workspace ownership through the execution chain:
const stepExecution = stepExecutions[0] + + // Validate workspace access through workflowExecution + const execution = await getWorkflowExecutionByIdWithChecks( + db, + stepExecution.workflowExecutionId, + user.workspaceId, + user.id + ) + + if (!execution) { + throw new HTTPException(403, { message: "Access denied" }) + } + const stepTemplate = await getWorkflowStepTemplateById(
- Use workspace-scoped tool fetcher:
- const formTool = await getWorkflowToolById( + const formTool = await getWorkflowToolByIdWithChecks( db, toolIds[0], + user.workspaceId, + user.id )
🧹 Nitpick comments (1)
server/api/workflow.ts (1)
343-350: Consider workspace-scoped tool fetcher for defense in depth.While the template is validated with
getWorkflowTemplateByIdWithPublicCheckat line 311, usinggetWorkflowToolByIdwithout workspace checks creates a theoretical vulnerability ifstepTemplate.toolIdscould reference cross-workspace tools (via DB manipulation or bugs elsewhere).Recommendation:
- const toolResult = await getWorkflowToolById( + const toolResult = await getWorkflowToolByIdWithChecks( db, rootStep.toolIds[0], + user.workspaceId, + user.id )This provides defense in depth and aligns with the principle of validating resources at point of use, not just relying on upstream validation.
Note: This same pattern appears at lines 1407-1410, 1527-1530, 1547-1550 in
SubmitWorkflowFormApi. Consider applying this refactor consistently across all tool fetch locations where user context is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api/workflow.ts(50 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/api/workflow.ts (2)
server/db/workflow.ts (10)
getAccessibleWorkflowTemplates(80-97)getWorkflowStepTemplatesByTemplateId(164-174)getWorkflowTemplateByIdWithPublicCheck(58-78)getWorkflowStepTemplateById(151-162)createWorkflowExecution(177-203)createWorkflowStepExecutionsFromSteps(307-335)getWorkflowExecutionById(224-237)getWorkflowExecutionByIdWithChecks(205-222)createWorkflowTemplate(27-55)updateWorkflowTemplate(99-114)server/db/workflowTool.ts (6)
getWorkflowToolsByIds(101-113)getWorkflowToolById(42-53)createToolExecution(147-171)getAccessibleWorkflowTools(74-88)createWorkflowTool(17-39)getWorkflowToolByIdWithChecks(55-72)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor