Conversation
📝 WalkthroughWalkthroughThis PR introduces a centralized ExecutionRegister system for managing node executors. It adds type definitions (ExecutionContext, ExecutionResult, NodeExecutor), creates an ExecutionRegistry class to register and execute node handlers (Gmail, Google Sheets), integrates it into the worker and HTTP backend, and adds per-node execution tracking with timestamps and status management. Changes
Sequence DiagramsequenceDiagram
actor Client
participant HttpBackend as HTTP Backend
participant Auth as Auth Middleware
participant Validator as Request Validator
participant DB as Database
participant ER as ExecutionRegister
participant Executor as Node Executor
Client->>HttpBackend: POST /execute/node {nodeId, config?}
HttpBackend->>Auth: Validate user token
Auth-->>HttpBackend: User authenticated
HttpBackend->>Validator: Validate ExecuteNode schema
Validator-->>HttpBackend: Input valid
HttpBackend->>DB: Find node by nodeId
DB-->>HttpBackend: Node + AvailableNode
HttpBackend->>HttpBackend: Derive config (request or DB)
HttpBackend->>ER: ExecutionRegister.execute(type, context)
ER->>Executor: Get executor for node type
Executor->>Executor: execute(context)
Executor-->>ER: ExecutionResult {success, output?, error?}
ER-->>HttpBackend: ExecutionResult
HttpBackend-->>Client: 202 Accepted {results}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/worker/src/engine/executor.ts (1)
64-71:⚠️ Potential issue | 🟠 MajorField name mismatch:
credIdvscredentialsId.The context object uses
credId(line 67), butExecutionContextinExecution.config.types.tsdefinescredentialsId. This mismatch means the executor won't receive the credentials ID correctly, potentially causing authorization failures.🐛 Proposed fix - align field name with type definition
const context = { // nodeId: node.id, userId: data.workflow.userId, - credId: node.CredentialsID, + credentialsId: node.CredentialsID, // config: node.config as Record<string, any>, config: nodeConfig, inputData: currentInputData, };Note: This requires updating the executor implementations (
GmailExecutor,GoogleSheetsNodeExecutor) to read fromcredentialsIdinstead ofcredId, or updating theExecutionContexttype to usecredId.
🤖 Fix all issues with AI agents
In `@apps/http-backend/src/routes/userRoutes/executionRoutes.ts`:
- Around line 41-49: The current truthy check on executionResult causes the
ACCEPTED branch to always run; change the conditional to inspect the actual
success flag returned by ExecutionRegister.execute() by replacing
if(executionResult) with if (executionResult && executionResult.success ===
true) (or simply if (executionResult?.success)), so the
res.status(statusCodes.ACCEPTED) path runs only on success and the
res.status(statusCodes.FORBIDDEN) path is reachable when executionResult.success
is falsy; keep the existing response bodies that reference nodeData.name and
executionResult.data.
- Around line 33-36: The execution context object currently created in
executionRoutes (variable context with userId: req.user.sub and config) must
also include credentialsId so executors like GmailExecutor and
GoogleSheetsNodeExecutor can authenticate; update the context to add
credentialsId sourced from the node data (e.g., nodeData.CredentialsID or
nodeData.credentials.id) and ensure the Prisma query that fetches nodeData
includes the credentials relation so CredentialsID is available to use in
context.
- Line 34: The current fallback to an empty string for userId (userId:
req.user.sub || "") is unsafe; update the route handler in executionRoutes to
validate req.user?.sub and return a 400/401 error if missing instead of using ""
so downstream credential lookups won't receive an invalid id. Locate the handler
constructing the payload with userId and add a guard that checks req.user and
req.user.sub, sending an explicit error response (with a clear message) when
absent; only proceed to call the downstream logic using the validated userId
value.
In `@packages/nodes/src/registry/Execution.config.types.ts`:
- Around line 1-7: The ExecutionContext interface uses the wrong field name for
credentials: change the property name from credentialsId to credId so it matches
usage across the codebase (e.g., GmailExecutor, GoogleSheetsNodeExecutor) and
the worker engine that sets context.credId (see executor implementations and
apps/worker/src/engine/executor.ts), ensuring all references are consistent and
types updated accordingly.
In `@packages/nodes/src/registry/execution.registory.ts`:
- Around line 34-44: The code registers GmailExecutor and
GoogleSheetsNodeExecutor using unsafe casts (as unknown as NodeExecutor), hiding
real interface mismatches; replace these casts by adapting the executors to the
NodeExecutor contract: implement an adapter or wrapper class/function (e.g.,
GmailNodeExecutorAdapter, GoogleSheetsNodeExecutorAdapter) that implements
NodeExecutor and internally translates the executor methods/fields (map credId
-> credentialsId, convert their ExecutionContext shape to the NodeExecutor
ExecutionContext, and delegate calls to GmailExecutor/GoogleSheetsNodeExecutor),
then call this.register("gmail", new GmailNodeExecutorAdapter(new
GmailExecutor())) and this.register("google_sheet", new
GoogleSheetsNodeExecutorAdapter(new GoogleSheetsNodeExecutor())); ensure the
adapter types satisfy NodeExecutor and remove the casts so the TypeScript
compiler enforces correctness.
- Around line 27-32: The catch block returns the raw caught value into
ExecutionResult.error which is typed as string; change the catch handling in the
catch of the function in execution.registory.ts to normalize the error to a
string (e.g. use error instanceof Error ? error.message : typeof error ===
'string' ? error : JSON.stringify(error)) and return that string in the error
field so ExecutionResult.error remains a string; ensure you handle potential
JSON.stringify failures by falling back to String(error).
🧹 Nitpick comments (7)
packages/nodes/src/index.ts (1)
14-14: Typo in filename: "registory" should be "registry".The export path contains a typo (
execution.registory.js). Consider renaming the file toexecution.registry.tsfor correctness and consistency.packages/nodes/src/registry/Execution.config.types.ts (1)
12-12: Consider usingRecord<string, any>instead ofRecord<any, any>.
Record<any, any>is unusual since object keys in JavaScript are coerced to strings (or symbols). UsingRecord<string, any>would be more idiomatic.💡 Suggested fix
- metadata?: Record<any, any>; + metadata?: Record<string, any>;packages/nodes/src/registry/execution.registory.ts (1)
24-24: Replace debugconsole.logwith structured logging or remove.These debug statements will pollute production logs. Consider using a proper logger with configurable log levels.
Also applies to: 43-43
apps/worker/src/index.ts (1)
3-4: Remove commented-out import.The old import should be removed rather than left commented out.
🧹 Cleanup
import { ExecutionRegister } from "@repo/nodes"; -// import { register } from "./engine/registory.js";apps/worker/src/engine/executor.ts (1)
2-3: Remove commented-out import.Clean up the commented code.
🧹 Cleanup
-// import { register } from "./registory.js"; import { ExecutionRegister } from "@repo/nodes";apps/http-backend/src/routes/userRoutes/executionRoutes.ts (1)
39-39: Object logged incorrectly - useJSON.stringify.
console.log(\Execution result: ${executionResult}`)will output[object Object]. UseJSON.stringify(executionResult)` for meaningful output.💡 Fix logging
- console.log(`Execution result: ${executionResult}`) + console.log(`Execution result: ${JSON.stringify(executionResult)}`)apps/http-backend/src/index.ts (1)
42-46: Fail fast on registry initialization errors.Consider wrapping startup in a try/catch to avoid partial startup if registry initialization fails (and to future‑proof if init becomes async).
🧯 Suggested startup guard
async function startServer() { - await NodeRegistry.registerAll() - tokenScheduler.start(); - ExecutionRegister.initialize() - app.listen(PORT, () => { - console.log(`Server running on port ${PORT}`); - }) - } + try { + await NodeRegistry.registerAll() + tokenScheduler.start(); + ExecutionRegister.initialize() + app.listen(PORT, () => { + console.log(`Server running on port ${PORT}`); + }) + } catch (err) { + console.error("Failed to start server:", err); + process.exit(1); + } +}
| const context = { | ||
| userId: req.user.sub || "", | ||
| config: config | ||
| } |
There was a problem hiding this comment.
Missing credentialsId in execution context.
The context object doesn't include credentialsId, which is needed by executors like GmailExecutor and GoogleSheetsNodeExecutor for authentication. Without it, node execution will fail with authorization errors.
🐛 Proposed fix - include credentials
const context = {
userId: req.user.sub || "",
- config: config
+ config: config,
+ credentialsId: nodeData.CredentialsID
}Note: Verify that nodeData includes CredentialsID by updating the Prisma query to include the credentials relation if needed.
📝 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.
| const context = { | |
| userId: req.user.sub || "", | |
| config: config | |
| } | |
| const context = { | |
| userId: req.user.sub || "", | |
| config: config, | |
| credentialsId: nodeData.CredentialsID | |
| } |
🤖 Prompt for AI Agents
In `@apps/http-backend/src/routes/userRoutes/executionRoutes.ts` around lines 33 -
36, The execution context object currently created in executionRoutes (variable
context with userId: req.user.sub and config) must also include credentialsId so
executors like GmailExecutor and GoogleSheetsNodeExecutor can authenticate;
update the context to add credentialsId sourced from the node data (e.g.,
nodeData.CredentialsID or nodeData.credentials.id) and ensure the Prisma query
that fetches nodeData includes the credentials relation so CredentialsID is
available to use in context.
| const config = dataSafe.data.Config ? dataSafe.data.Config : nodeData.config // for test api data prefered fist then config in db | ||
| console.log(`config and type: ${JSON.stringify(config)} & ${type}`) | ||
| const context = { | ||
| userId: req.user.sub || "", |
There was a problem hiding this comment.
Empty string fallback for userId may cause issues.
If req.user.sub is undefined, using an empty string as userId could cause downstream failures (e.g., credential lookups). Consider returning an error instead.
🛡️ Suggested defensive handling
+ if (!req.user.sub) {
+ return res.status(statusCodes.BAD_REQUEST).json({
+ message: "User ID not found in token"
+ });
+ }
const context = {
- userId: req.user.sub || "",
+ userId: req.user.sub,
config: config
}🤖 Prompt for AI Agents
In `@apps/http-backend/src/routes/userRoutes/executionRoutes.ts` at line 34, The
current fallback to an empty string for userId (userId: req.user.sub || "") is
unsafe; update the route handler in executionRoutes to validate req.user?.sub
and return a 400/401 error if missing instead of using "" so downstream
credential lookups won't receive an invalid id. Locate the handler constructing
the payload with userId and add a guard that checks req.user and req.user.sub,
sending an explicit error response (with a clear message) when absent; only
proceed to call the downstream logic using the validated userId value.
| if(executionResult) | ||
| return res.status(statusCodes.ACCEPTED).json({ | ||
| message: `${nodeData.name} node execution done` , | ||
| data: executionResult | ||
| }) | ||
|
|
||
| return res.status(statusCodes.FORBIDDEN).json({ | ||
| message: `${nodeData.name} node execution failed` | ||
| }) |
There was a problem hiding this comment.
Logic error: Response always returns 202 regardless of execution success.
The condition if(executionResult) checks for truthiness of the object, which is always true since ExecutionRegister.execute() returns an object. The subsequent FORBIDDEN response (lines 47-49) is unreachable. You should check executionResult.success instead.
🐛 Proposed fix
- if(executionResult)
+ if(executionResult.success)
return res.status(statusCodes.ACCEPTED).json({
message: `${nodeData.name} node execution done` ,
data: executionResult
})
return res.status(statusCodes.FORBIDDEN).json({
- message: `${nodeData.name} node execution failed`
+ message: `${nodeData.name} node execution failed`,
+ error: executionResult.error
})🤖 Prompt for AI Agents
In `@apps/http-backend/src/routes/userRoutes/executionRoutes.ts` around lines 41 -
49, The current truthy check on executionResult causes the ACCEPTED branch to
always run; change the conditional to inspect the actual success flag returned
by ExecutionRegister.execute() by replacing if(executionResult) with if
(executionResult && executionResult.success === true) (or simply if
(executionResult?.success)), so the res.status(statusCodes.ACCEPTED) path runs
only on success and the res.status(statusCodes.FORBIDDEN) path is reachable when
executionResult.success is falsy; keep the existing response bodies that
reference nodeData.name and executionResult.data.
| export interface ExecutionContext { | ||
| nodeId?: string; | ||
| userId: string; | ||
| credentialsId?: string; | ||
| config: Record<string, any>; | ||
| inputData?: any; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for credId and credentialsId usage patterns in executor files
echo "=== Checking credId usage ==="
rg -n "credId" --type ts
echo ""
echo "=== Checking credentialsId usage ==="
rg -n "credentialsId" --type tsRepository: Dev-Pross/BuildFlow
Length of output: 3481
Fix field naming mismatch between ExecutionContext interface and executor implementations.
ExecutionContext defines credentialsId, but GmailExecutor and GoogleSheetsNodeExecutor access context.credId. The worker engine (apps/worker/src/engine/executor.ts:67) also sets the context field as credId, not credentialsId. This mismatch will cause credentials to be undefined at runtime. Rename credentialsId to credId in the interface to match the actual usage.
🤖 Prompt for AI Agents
In `@packages/nodes/src/registry/Execution.config.types.ts` around lines 1 - 7,
The ExecutionContext interface uses the wrong field name for credentials: change
the property name from credentialsId to credId so it matches usage across the
codebase (e.g., GmailExecutor, GoogleSheetsNodeExecutor) and the worker engine
that sets context.credId (see executor implementations and
apps/worker/src/engine/executor.ts), ensuring all references are consistent and
types updated accordingly.
| } catch (error: any) { | ||
| return { | ||
| success: false, | ||
| error: error, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Error object assigned to string field.
ExecutionResult.error is typed as string, but the caught error object is assigned directly. This can result in [object Object] or unexpected serialization.
🐛 Proposed fix
} catch (error: any) {
return {
success: false,
- error: error,
+ error: error instanceof Error ? error.message : String(error),
};
}📝 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.
| } catch (error: any) { | |
| return { | |
| success: false, | |
| error: error, | |
| }; | |
| } | |
| } catch (error: any) { | |
| return { | |
| success: false, | |
| error: error instanceof Error ? error.message : String(error), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@packages/nodes/src/registry/execution.registory.ts` around lines 27 - 32, The
catch block returns the raw caught value into ExecutionResult.error which is
typed as string; change the catch handling in the catch of the function in
execution.registory.ts to normalize the error to a string (e.g. use error
instanceof Error ? error.message : typeof error === 'string' ? error :
JSON.stringify(error)) and return that string in the error field so
ExecutionResult.error remains a string; ensure you handle potential
JSON.stringify failures by falling back to String(error).
| initialize() { | ||
| // TODO: Ensure GmailExecutor implements NodeExecutor and is compatible with ExecutionContext | ||
| // If needed, adapt/extract a compatible Executor for registration. | ||
| // For now, this cast suppresses the type error. Refactor as soon as possible! | ||
|
|
||
|
|
||
| //wehen visits this next time make sure chang gmail executor implements NodeExecutor | ||
| this.register("gmail", new GmailExecutor() as unknown as NodeExecutor); | ||
| this.register("google_sheet", new GoogleSheetsNodeExecutor() as unknown as NodeExecutor) | ||
| console.log(`The current Executors are ${this.executors.size}`); | ||
| } |
There was a problem hiding this comment.
Address the type incompatibility TODOs before merging.
The as unknown as NodeExecutor casts suppress type errors but hide real interface mismatches (e.g., credId vs credentialsId, different ExecutionContext types). This can cause runtime failures. Consider creating adapter functions or updating the executor interfaces to properly implement NodeExecutor.
Would you like me to help design an adapter pattern to properly bridge the executor implementations with the NodeExecutor interface?
🤖 Prompt for AI Agents
In `@packages/nodes/src/registry/execution.registory.ts` around lines 34 - 44, The
code registers GmailExecutor and GoogleSheetsNodeExecutor using unsafe casts (as
unknown as NodeExecutor), hiding real interface mismatches; replace these casts
by adapting the executors to the NodeExecutor contract: implement an adapter or
wrapper class/function (e.g., GmailNodeExecutorAdapter,
GoogleSheetsNodeExecutorAdapter) that implements NodeExecutor and internally
translates the executor methods/fields (map credId -> credentialsId, convert
their ExecutionContext shape to the NodeExecutor ExecutionContext, and delegate
calls to GmailExecutor/GoogleSheetsNodeExecutor), then call
this.register("gmail", new GmailNodeExecutorAdapter(new GmailExecutor())) and
this.register("google_sheet", new GoogleSheetsNodeExecutorAdapter(new
GoogleSheetsNodeExecutor())); ensure the adapter types satisfy NodeExecutor and
remove the casts so the TypeScript compiler enforces correctness.
3 participants
execution rregistory has been copied to repo/nodes for sharable access need to review and remove existing registory from worker
Summary by CodeRabbit
Release Notes
/executeendpoint for running nodes with user authentication✏️ Tip: You can customize this high-level summary in your review settings.