feat(workflow-executor): scaffold @forestadmin/workflow-executor package#1493
feat(workflow-executor): scaffold @forestadmin/workflow-executor package#1493
Conversation
…premature deps, add smoke test - Rewrite CLAUDE.md with project overview and architecture principles, remove changelog - Remove unused dependencies (ai-proxy, sequelize, zod) per YAGNI - Add smoke test so CI passes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Scra3
force-pushed
the
feat/prd-214-setup-workflow-executor-package
branch
from
083894b to
4510b7b
Compare
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (12) 🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
… document system architecture
- Lint now covers src and test directories
- Replace require() with import, use stronger assertion (toHaveLength)
- Add System Architecture section describing Front/Orchestrator/Executor/Agent
- Mark Architecture Principles as planned (not yet implemented)
- Remove redundant test/.gitkeep
- Make index.ts a valid module with export {}
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| export type McpConfiguration = unknown; | ||
|
|
||
| export interface WorkflowPort { | ||
| getPendingStepExecutions(): Promise<PendingStepExecution[]>; |
There was a problem hiding this comment.
this method will retrieve the workflowRun and workflowSteps of one pending run. It will take an optional runId, and return an object with complete workflowRun, workflowSteps, workflowRecords
| interface BaseStepHistory { | ||
| stepId: string; | ||
| stepIndex: number; | ||
| status: StepStatus; | ||
| /** Present when status is 'error'. */ | ||
| error?: string; | ||
| } | ||
|
|
||
| export interface ConditionStepHistory extends BaseStepHistory { | ||
| type: 'condition'; | ||
| /** Present when status is 'success'. */ | ||
| selectedOption?: string; | ||
| } | ||
|
|
||
| export interface AiTaskStepHistory extends BaseStepHistory { | ||
| type: 'ai-task'; | ||
| } |
There was a problem hiding this comment.
what are those types ? this does not correspond to anything
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| return { ...ref, recordId, values: updatedRecord }; | ||
| } | ||
|
|
||
| async getRelatedData( |
There was a problem hiding this comment.
🟡 Medium adapters/agent-client-agent-port.ts:76
getRelatedData passes relationName (the field name, e.g., 'author') to getCollectionRef, which looks up collection metadata by collection name. This returns a fallback CollectionRef with primaryKeyFields: ['id'] instead of the actual related collection's metadata. Consequently, extractRecordId extracts the wrong fields from related records, returning incorrect or undefined values for the primary key. Consider obtaining the target collection name from the relation's metadata rather than using the field name directly.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/agent-client-agent-port.ts around line 76:
`getRelatedData` passes `relationName` (the field name, e.g., `'author'`) to `getCollectionRef`, which looks up collection metadata by collection name. This returns a fallback `CollectionRef` with `primaryKeyFields: ['id']` instead of the actual related collection's metadata. Consequently, `extractRecordId` extracts the wrong fields from related records, returning incorrect or `undefined` values for the primary key. Consider obtaining the target collection name from the relation's metadata rather than using the field name directly.
Evidence trail:
packages/workflow-executor/src/adapters/agent-client-agent-port.ts lines 72-87 (getRelatedData method passes relationName to getCollectionRef at line 76), lines 100-112 (getCollectionRef looks up by collectionName and returns fallback with primaryKeyFields: ['id']), packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts lines 152-174 (test only works because relation name 'posts' matches collection name 'posts'; fallback test confirms behavior when they don't match)
16 new issues
|
| function extractRecordId( | ||
| primaryKeyFields: string[], | ||
| record: Record<string, unknown>, | ||
| ): Array<string | number> { | ||
| return primaryKeyFields.map(field => record[field] as string | number); | ||
| } |
There was a problem hiding this comment.
🟢 Low adapters/agent-client-agent-port.ts:30
extractRecordId returns undefined for missing primary key fields, but the string | number type assertion hides this. When passed to encodePk, undefined becomes the literal string "undefined", causing lookups to silently fail with wrong record IDs. Consider adding a runtime check for missing fields and throwing an error, or return the actual values and let encodePk validate them.
function extractRecordId(
primaryKeyFields: string[],
record: Record<string, unknown>,
): Array<string | number> {
- return primaryKeyFields.map(field => record[field] as string | number);
+ return primaryKeyFields.map(field => {
+ const value = record[field];
+ if (value === undefined || value === null) {
+ throw new Error(`Missing primary key field: ${field}`);
+ }
+ return value as string | number;
+ });
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/agent-client-agent-port.ts around lines 30-35:
`extractRecordId` returns `undefined` for missing primary key fields, but the `string | number` type assertion hides this. When passed to `encodePk`, `undefined` becomes the literal string `"undefined"`, causing lookups to silently fail with wrong record IDs. Consider adding a runtime check for missing fields and throwing an error, or return the actual values and let `encodePk` validate them.
Evidence trail:
packages/workflow-executor/src/adapters/agent-client-agent-port.ts lines 25-33 (REVIEWED_COMMIT): `encodePk` uses `String(v)` which converts undefined to "undefined"; `extractRecordId` uses type assertion `as string | number` on `record[field]` which can be undefined at runtime. Line 83 shows `extractRecordId` is called in `getRelatedData` to create record IDs that are returned and could be used in subsequent operations.
| // TODO: finalize route paths with the team — these are placeholders | ||
| const ROUTES = { | ||
| pendingStepExecutions: '/liana/v1/workflow-step-executions/pending', | ||
| updateStepExecution: (runId: string) => `/liana/v1/workflow-step-executions/${runId}/complete`, | ||
| collectionRef: (collectionName: string) => `/liana/v1/collections/${collectionName}`, | ||
| mcpServerConfigs: '/liana/mcp-server-configs-with-details', | ||
| }; |
There was a problem hiding this comment.
🟢 Low adapters/forest-server-workflow-port.ts:9
ROUTES.updateStepExecution(runId) and ROUTES.collectionRef(collectionName) interpolate raw values into URL paths without encoding, so special characters like /, ?, or % in runId or collectionName produce malformed URLs (e.g., collectionName="a/b" becomes /liana/v1/collections/a/b with three path segments). Consider wrapping the parameters with encodeURIComponent() to ensure they are safely encoded.
-// TODO: finalize route paths with the team — these are placeholders
const ROUTES = {
pendingStepExecutions: '/liana/v1/workflow-step-executions/pending',
- updateStepExecution: (runId: string) => `/liana/v1/workflow-step-executions/${runId}/complete`,
- collectionRef: (collectionName: string) => `/liana/v1/collections/${collectionName}`,
+ updateStepExecution: (runId: string) => `/liana/v1/workflow-step-executions/${encodeURIComponent(runId)}/complete`,
+ collectionRef: (collectionName: string) => `/liana/v1/collections/${encodeURIComponent(collectionName)}`,
mcpServerConfigs: '/liana/mcp-server-configs-with-details',
};🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/forest-server-workflow-port.ts around lines 9-15:
`ROUTES.updateStepExecution(runId)` and `ROUTES.collectionRef(collectionName)` interpolate raw values into URL paths without encoding, so special characters like `/`, `?`, or `%` in `runId` or `collectionName` produce malformed URLs (e.g., `collectionName="a/b"` becomes `/liana/v1/collections/a/b` with three path segments). Consider wrapping the parameters with `encodeURIComponent()` to ensure they are safely encoded.
Evidence trail:
packages/workflow-executor/src/adapters/forest-server-workflow-port.ts lines 11-13 (ROUTES definitions with template literals), packages/forestadmin-client/src/utils/server.ts lines 63-70 (queryWithHeaders passes path directly to new URL() without encoding)
| export function causeMessage(error: unknown): string | undefined { | ||
| const { cause } = error as { cause?: unknown }; | ||
|
|
||
| return cause instanceof Error ? cause.message : undefined; | ||
| } |
There was a problem hiding this comment.
🟠 High src/errors.ts:3
causeMessage(null) and causeMessage(undefined) throw TypeError: Cannot destructure property 'cause' of 'error' as it is null/undefined. Since the parameter is unknown and this is called in catch blocks where any thrown value is possible, consider adding a guard before destructuring.
-export function causeMessage(error: unknown): string | undefined {
- const { cause } = error as { cause?: unknown };
-
- return cause instanceof Error ? cause.message : undefined;
-}Also found in 3 other location(s)
packages/workflow-executor/src/executors/base-step-executor.ts:163
In
invokeWithTools,toolCall.nameis used directly without a null/undefined check on line 163, but the fallback patterntoolCall.name ?? 'unknown'on line 166 indicates thattoolCall.namecan be undefined. IftoolCall.argsis present buttoolCall.nameis undefined, the method returns{ toolName: undefined, ... }which violates the declared return type{ toolName: string; args: T }.
packages/workflow-executor/src/executors/mcp-task-step-executor.ts:148
If
toolResultis a value that causesJSON.stringifyto returnundefined(such as a bare function or symbol),resultStrwill beundefinedand line 148 will throwTypeError: Cannot read properties of undefined (reading 'length'). While MCP tools typically return JSON-serializable data, defensive code could check forresultStr === undefinedafter the stringify.
packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts:42
In
handleConfirmation, the code spreadspendingDatawith a cast...(pendingData as ActionRef)on line 42, butpendingDatais typed as optional (ActionRef | undefined) inTriggerRecordActionStepExecutionData. IfpendingDataisundefined(e.g., due to data corruption or a race condition during persistence), spreading it produces an empty object, leavingtarget.nameandtarget.displayNameasundefined. This then passesundefinedtoexecuteActionat line 89 and storesundefinedvalues inexecutionParamsat line 98. Adding a guard to verifypendingDataexists before proceeding would prevent silent failures.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/errors.ts around lines 3-7:
`causeMessage(null)` and `causeMessage(undefined)` throw `TypeError: Cannot destructure property 'cause' of 'error' as it is null/undefined`. Since the parameter is `unknown` and this is called in catch blocks where any thrown value is possible, consider adding a guard before destructuring.
Evidence trail:
packages/workflow-executor/src/errors.ts lines 3-6 (function definition showing direct destructuring without null/undefined guard), packages/workflow-executor/src/executors/step-executor-factory.ts line 78 (usage in catch block), packages/workflow-executor/src/runner.ts line 169 (usage in catch block). JavaScript specification: destructuring from null/undefined throws TypeError.
Also found in 3 other location(s):
- packages/workflow-executor/src/executors/base-step-executor.ts:163 -- In `invokeWithTools`, `toolCall.name` is used directly without a null/undefined check on line 163, but the fallback pattern `toolCall.name ?? 'unknown'` on line 166 indicates that `toolCall.name` can be undefined. If `toolCall.args` is present but `toolCall.name` is undefined, the method returns `{ toolName: undefined, ... }` which violates the declared return type `{ toolName: string; args: T }`.
- packages/workflow-executor/src/executors/mcp-task-step-executor.ts:148 -- If `toolResult` is a value that causes `JSON.stringify` to return `undefined` (such as a bare function or symbol), `resultStr` will be `undefined` and line 148 will throw `TypeError: Cannot read properties of undefined (reading 'length')`. While MCP tools typically return JSON-serializable data, defensive code could check for `resultStr === undefined` after the stringify.
- packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts:42 -- In `handleConfirmation`, the code spreads `pendingData` with a cast `...(pendingData as ActionRef)` on line 42, but `pendingData` is typed as optional (`ActionRef | undefined`) in `TriggerRecordActionStepExecutionData`. If `pendingData` is `undefined` (e.g., due to data corruption or a race condition during persistence), spreading it produces an empty object, leaving `target.name` and `target.displayName` as `undefined`. This then passes `undefined` to `executeAction` at line 89 and stores `undefined` values in `executionParams` at line 98. Adding a guard to verify `pendingData` exists before proceeding would prevent silent failures.
| expect(selectRecordTool.schema.shape.recordIdentifier.options).not.toContain( | ||
| expect.stringContaining('stepIndex: 3'), | ||
| ); |
There was a problem hiding this comment.
🟡 Medium executors/load-related-record-step-executor.test.ts:1492
The assertion at lines 1492-1494 checks that recordIdentifier.options does not contain 'stepIndex: 3', but the actual format used in select-record tool identifiers is 'Step N - ...' (as shown at line 1447). Since 'stepIndex: 3' never appears in the options, this assertion always passes regardless of whether the pending execution is correctly excluded from the pool.
- expect(selectRecordTool.schema.shape.recordIdentifier.options).not.toContain(
- expect.stringContaining('stepIndex: 3'),
- );🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts around lines 1492-1494:
The assertion at lines 1492-1494 checks that `recordIdentifier.options` does not contain `'stepIndex: 3'`, but the actual format used in `select-record` tool identifiers is `'Step N - ...'` (as shown at line 1447). Since `'stepIndex: 3'` never appears in the options, this assertion always passes regardless of whether the pending execution is correctly excluded from the pool.
Evidence trail:
packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts lines 1492-1494 (assertion checking 'stepIndex: 3'), line 1446 (mock args using format 'Step 2 - Orders #99'), packages/workflow-executor/src/executors/base-step-executor.ts lines 263-267 (toRecordIdentifier method defining format as `Step ${record.stepIndex} - ${schema.collectionDisplayName} #${record.recordId}`), packages/workflow-executor/test/executors/read-record-step-executor.test.ts lines 565-567 (expected options format showing 'Step 3 - Customers #42')
| return selectedDisplayNames.map( | ||
| dn => nonRelationFields.find(f => f.displayName === dn)?.fieldName ?? dn, | ||
| ); |
There was a problem hiding this comment.
🟢 Low executors/load-related-record-step-executor.ts:345
In selectRelevantFields, when the AI returns a displayName that doesn't exist in nonRelationFields, the find returns undefined and the fallback ?? dn preserves the raw display name. This invalid field name then fails to match any key in c.values during selectBestRecordIndex, causing the filtered candidates to have empty values objects and making the AI's record selection arbitrary. Consider throwing InvalidAIResponseError when find returns undefined instead of falling back to the raw display name.
+ return selectedDisplayNames.map(dn => {
+ const field = nonRelationFields.find(f => f.displayName === dn);
+ if (!field) {
+ throw new InvalidAIResponseError(
+ `AI returned unknown field name "${dn}" for collection "${schema.collectionName}"`,
+ );
+ }
+ return field.fieldName;
+ });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/executors/load-related-record-step-executor.ts around lines 345-347:
In `selectRelevantFields`, when the AI returns a `displayName` that doesn't exist in `nonRelationFields`, the `find` returns `undefined` and the fallback `?? dn` preserves the raw display name. This invalid field name then fails to match any key in `c.values` during `selectBestRecordIndex`, causing the filtered candidates to have empty `values` objects and making the AI's record selection arbitrary. Consider throwing `InvalidAIResponseError` when `find` returns `undefined` instead of falling back to the raw display name.
Evidence trail:
1. packages/workflow-executor/src/executors/load-related-record-step-executor.ts lines 337-347: Comment states 'Zod's .min(1) shapes the prompt but is NOT validated against the AI response' and the mapping logic `nonRelationFields.find(f => f.displayName === dn)?.fieldName ?? dn`.
2. packages/workflow-executor/src/executors/base-step-executor.ts lines 149-163: `invokeWithTools` returns `toolCall.args as T` without runtime Zod validation.
3. packages/workflow-executor/src/executors/load-related-record-step-executor.ts lines 357-365: `filteredCandidates` filters `c.values` entries by checking `fieldNames.includes(k)`.
4. packages/workflow-executor/src/types/record.ts line 37: `RecordData` definition shows `values: Record<string, unknown>` keyed by technical field names.
|
|
||
| export default class ConsoleLogger implements Logger { | ||
| error(message: string, context: Record<string, unknown>): void { | ||
| console.error(JSON.stringify({ message, timestamp: new Date().toISOString(), ...context })); |
There was a problem hiding this comment.
🟡 Medium adapters/console-logger.ts:5
The spread ...context comes after message and timestamp, so any message or timestamp keys in context overwrite the explicit parameters. Callers logging { message: "other" } will see their context value instead of the actual error message. Consider moving the spread first so explicit parameters take precedence.
| console.error(JSON.stringify({ message, timestamp: new Date().toISOString(), ...context })); | |
| console.error(JSON.stringify({ ...context, message, timestamp: new Date().toISOString() })); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/console-logger.ts around line 5:
The spread `...context` comes after `message` and `timestamp`, so any `message` or `timestamp` keys in `context` overwrite the explicit parameters. Callers logging `{ message: "other" }` will see their context value instead of the actual error message. Consider moving the spread first so explicit parameters take precedence.
Evidence trail:
packages/workflow-executor/src/adapters/console-logger.ts:5 - The code `{ message, timestamp: new Date().toISOString(), ...context }` shows explicit properties defined first and spread operator last. In JavaScript, object spread puts later properties after earlier ones, so `context.message` would overwrite the `message` parameter if present. This is standard JavaScript object spread behavior documented at MDN and in the ECMAScript specification.
…erver (#1504) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: alban bertolini <albanb@forestadmin.com>
| const baseData: McpTaskStepExecutionData = { | ||
| ...existingExecution, | ||
| type: 'mcp-task', | ||
| stepIndex: this.context.stepIndex, | ||
| executionParams: { name: target.name, input: target.input }, | ||
| executionResult: baseExecutionResult, | ||
| }; |
There was a problem hiding this comment.
🟢 Low executors/mcp-task-step-executor.ts:106
When executeToolAndPersist is called with existingExecution (re-entry from handleConfirmationFlow), the spread ...existingExecution at line 107 copies pendingData into baseData. After successful execution, the saved record still contains pendingData, so findPendingExecution will incorrectly match it as pending on subsequent re-entries and trigger duplicate execution. Consider explicitly clearing pendingData: undefined when constructing baseData to mark the step as completed.
- const baseData: McpTaskStepExecutionData = {
- ...existingExecution,
+ const baseData: McpTaskStepExecutionData = {
+ ...existingExecution,
+ pendingData: undefined,
type: 'mcp-task',
stepIndex: this.context.stepIndex,
executionParams: { name: target.name, input: target.input },
executionResult: baseExecutionResult,
};🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/executors/mcp-task-step-executor.ts around lines 106-112:
When `executeToolAndPersist` is called with `existingExecution` (re-entry from `handleConfirmationFlow`), the spread `...existingExecution` at line 107 copies `pendingData` into `baseData`. After successful execution, the saved record still contains `pendingData`, so `findPendingExecution` will incorrectly match it as pending on subsequent re-entries and trigger duplicate execution. Consider explicitly clearing `pendingData: undefined` when constructing `baseData` to mark the step as completed.
Evidence trail:
packages/workflow-executor/src/executors/mcp-task-step-executor.ts lines 106-114 (baseData construction with spread of existingExecution), packages/workflow-executor/src/executors/base-step-executor.ts lines 94-102 (findPendingExecution only checks type and stepIndex, not executionResult), packages/workflow-executor/src/executors/base-step-executor.ts lines 111-136 (handleConfirmationFlow checks pendingData.userConfirmed but not executionResult existence), packages/workflow-executor/src/types/step-execution-data.ts lines 88-96 (McpTaskStepExecutionData type showing optional pendingData field), git_grep for 'pendingData: undefined' returned no matches confirming pendingData is never explicitly cleared.
…+ DatabaseStore) (#1506)
| async loadRemoteTools(mcpConfig: McpConfiguration): Promise<McpClient['tools']> { | ||
| await this.closeMcpClient('Error closing previous MCP connection'); | ||
|
|
||
| const newClient = new McpClient(mcpConfig, this.logger); | ||
| const tools = await newClient.loadTools(); | ||
| this.mcpClient = newClient; | ||
|
|
||
| return tools; | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/ai-client.ts:39
In loadRemoteTools, if newClient.loadTools() throws, the McpClient instance is orphaned and never closed. Since this.mcpClient is only assigned after success, closeConnections() cannot reach the failed client, leaving resources leaked. Consider wrapping loadTools() in a try/finally to ensure cleanup on failure.
async loadRemoteTools(mcpConfig: McpConfiguration): Promise<McpClient['tools']> {
await this.closeMcpClient('Error closing previous MCP connection');
const newClient = new McpClient(mcpConfig, this.logger);
- const tools = await newClient.loadTools();
- this.mcpClient = newClient;
+ try {
+ const tools = await newClient.loadTools();
+ this.mcpClient = newClient;
- return tools;
+ return tools;
+ } catch (error) {
+ await newClient.closeConnections();
+ throw error;
+ }
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/ai-proxy/src/ai-client.ts around lines 39-47:
In `loadRemoteTools`, if `newClient.loadTools()` throws, the `McpClient` instance is orphaned and never closed. Since `this.mcpClient` is only assigned after success, `closeConnections()` cannot reach the failed client, leaving resources leaked. Consider wrapping `loadTools()` in a try/finally to ensure cleanup on failure.
Evidence trail:
packages/ai-proxy/src/ai-client.ts lines 39-45 (loadRemoteTools function) and lines 52-60 (closeMcpClient function) at REVIEWED_COMMIT. The code shows newClient is instantiated at line 42, loadTools() is called at line 43, and this.mcpClient is assigned at line 44. If line 43 throws, line 44 never executes, leaving newClient orphaned with no cleanup path.
|
|
||
| import { RecordNotFoundError } from '../errors'; | ||
|
|
||
| function buildPkFilter( |
There was a problem hiding this comment.
🟢 Low adapters/agent-client-agent-port.ts:13
In buildPkFilter, when id.length < primaryKeyFields.length, id[i] evaluates to undefined for missing indices, producing filter conditions with value: undefined. This creates an And aggregator with Equal conditions comparing against undefined, which likely matches zero records or causes unexpected query behavior instead of the intended record lookup.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/agent-client-agent-port.ts around line 13:
In `buildPkFilter`, when `id.length < primaryKeyFields.length`, `id[i]` evaluates to `undefined` for missing indices, producing filter conditions with `value: undefined`. This creates an `And` aggregator with `Equal` conditions comparing against `undefined`, which likely matches zero records or causes unexpected query behavior instead of the intended record lookup.
Evidence trail:
packages/workflow-executor/src/adapters/agent-client-agent-port.ts lines 13-27 at REVIEWED_COMMIT - the `buildPkFilter` function uses `primaryKeyFields.map((field, i) => ({ field, operator: 'Equal', value: id[i] }))` which iterates over `primaryKeyFields` length and accesses `id[i]` without checking if `id` has enough elements. JavaScript array access beyond bounds returns `undefined`.
|
|
||
| throw new MalformedToolCallError(toolCall.name ?? 'unknown', 'args field is missing or null'); | ||
| } | ||
|
|
||
| const invalidCall = response.invalid_tool_calls?.[0]; | ||
|
|
||
| if (invalidCall) { |
There was a problem hiding this comment.
🟢 Low executors/base-step-executor.ts:172
invokeWithTools returns toolCall.name directly at line 174 without validating it exists. When the AI returns a tool call with defined args but undefined name, the function returns { toolName: undefined, args }, violating the declared return type { toolName: string; args: T }. This can cause downstream failures when code expects a valid string tool name. Consider checking toolCall.name before returning and throwing MalformedToolCallError if it's missing, consistent with how args is validated.
if (toolCall !== undefined) {
- if (toolCall.args !== undefined && toolCall.args !== null) {
- return { toolName: toolCall.name, args: toolCall.args as T };
+ if (toolCall.name !== undefined && toolCall.name !== null && toolCall.args !== undefined && toolCall.args !== null) {
+ return { toolName: toolCall.name, args: toolCall.args as T };
}
- throw new MalformedToolCallError(toolCall.name ?? 'unknown', 'args field is missing or null');
+ throw new MalformedToolCallError(toolCall.name ?? 'unknown', 'name or args field is missing or null');
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/executors/base-step-executor.ts around lines 172-178:
`invokeWithTools` returns `toolCall.name` directly at line 174 without validating it exists. When the AI returns a tool call with defined `args` but undefined `name`, the function returns `{ toolName: undefined, args }`, violating the declared return type `{ toolName: string; args: T }`. This can cause downstream failures when code expects a valid string tool name. Consider checking `toolCall.name` before returning and throwing `MalformedToolCallError` if it's missing, consistent with how `args` is validated.
Evidence trail:
packages/workflow-executor/src/executors/base-step-executor.ts lines 160-175 (REVIEWED_COMMIT) - shows the `invokeWithTools` function with return type `Promise<{ toolName: string; args: T }>` returning `toolCall.name` without validation. packages/ai-proxy/package.json shows `@langchain/core: 1.1.15`. @langchain/core ToolCall type definition (https://github.com/langchain-ai/langchainjs/blob/main/libs/langchain-core/src/messages/tool.ts) shows `name?: string` is optional.
…xecutor factories (#1510)
| describe('no records available', () => { | ||
| it('returns error when no records are available', () => { | ||
| const error = new NoRecordsError(); | ||
|
|
||
| expect(error).toBeInstanceOf(NoRecordsError); | ||
| expect(error.message).toBe('No records available'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟢 Low executors/read-record-step-executor.test.ts:317
The test 'returns error when no records are available' only instantiates NoRecordsError and checks its properties — it never creates a ReadRecordStepExecutor or calls execute(). This gives false confidence that the executor handles the "no records" scenario when it actually tests nothing about executor behavior. Consider removing this test or implementing it to verify the executor throws when no records are available.
- describe('no records available', () => {
- it('returns error when no records are available', () => {
- const error = new NoRecordsError();
-
- expect(error).toBeInstanceOf(NoRecordsError);
- expect(error.message).toBe('No records available');
- });
- });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/test/executors/read-record-step-executor.test.ts around lines 317-324:
The test `'returns error when no records are available'` only instantiates `NoRecordsError` and checks its properties — it never creates a `ReadRecordStepExecutor` or calls `execute()`. This gives false confidence that the executor handles the "no records" scenario when it actually tests nothing about executor behavior. Consider removing this test or implementing it to verify the executor throws when no records are available.
Evidence trail:
packages/workflow-executor/test/executors/read-record-step-executor.test.ts lines 317-323 (the test in question - only creates NoRecordsError and checks properties)
packages/workflow-executor/test/executors/read-record-step-executor.test.ts lines 326-340 (adjacent test showing proper executor test pattern with ReadRecordStepExecutor instantiation and execute() call)
packages/workflow-executor/test/executors/read-record-step-executor.test.ts lines 1-12 (imports showing both NoRecordsError and ReadRecordStepExecutor are available)
…ain (#1512) Co-authored-by: alban bertolini <albanb@forestadmin.com>
| return promise; | ||
| } | ||
|
|
||
| private async doExecuteStep(step: PendingStepExecution, key: string): Promise<void> { |
There was a problem hiding this comment.
🟡 Medium src/runner.ts:236
In doExecuteStep, the step is deleted from inFlightSteps in the finally block (line 253) before updateStepExecution completes (lines 256-267). If runPollCycle executes between the delete and updateStepExecution completing, the same step is refetched from the server, passes the !this.inFlightSteps.has(...) filter on line 204, and gets executed a second time concurrently. Move the delete to after updateStepExecution succeeds or fails, or wrap both in a single try/finally.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/runner.ts around line 236:
In `doExecuteStep`, the step is deleted from `inFlightSteps` in the `finally` block (line 253) before `updateStepExecution` completes (lines 256-267). If `runPollCycle` executes between the delete and `updateStepExecution` completing, the same step is refetched from the server, passes the `!this.inFlightSteps.has(...)` filter on line 204, and gets executed a second time concurrently. Move the `delete` to after `updateStepExecution` succeeds or fails, or wrap both in a single try/finally.
Evidence trail:
packages/workflow-executor/src/runner.ts lines 235-268 (REVIEWED_COMMIT): The `doExecuteStep` method shows the `finally` block with `this.inFlightSteps.delete(key)` at line 253, followed by the `updateStepExecution` call in a separate try block at lines 256-267. The `runPollCycle` method at lines 200-212 shows the filter `!this.inFlightSteps.has(Runner.stepKey(s))` at line 203.
3 participants
Summary
@forestadmin/workflow-executorpackage (tsconfig, jest, eslint, CI matrix)fixes PRD-214
Test plan
yarn workspace @forestadmin/workflow-executor buildpassesyarn workspace @forestadmin/workflow-executor lintpassesyarn workspace @forestadmin/workflow-executor testpasses🤖 Generated with Claude Code
Note
[!NOTE]
Scaffold
@forestadmin/workflow-executorpackage in the monorepoAdds the initial package structure for
@forestadmin/workflow-executorwith an emptysrc/index.ts, TypeScript and ESLint configs, Jest setup, and apackage.jsonconfigured for the workspace. The package is also added to the CI matrix in build.yml so it participates in build and test jobs.Changes since #1493 opened
BaseStepExecutorabstract class andConditionStepExecutorconcrete implementation [127b579]RunStoreinterface to scope all methods to the current run by removingrunIdparameter [127b579]ConditionStepDefinition.optionstype constraint to require at least one option [127b579]@langchain/coreversion 1.1.33 andzodversion 4.3.6 as runtime dependencies to@forestadmin/workflow-executorpackage [127b579]@forestadmin/workflow-executorpackage public API [127b579]BaseStepExecutorandConditionStepExecutor[127b579]CLAUDE.md[127b579]RecordRefwithCollectionRefand support composite primary keys as arrays [cb8036b]AgentClientAgentPortadapter class with methods for record operations, related data access, and action execution [cb8036b]RecordNotFoundErrorclass, updated package exports, added@forestadmin/agent-clientdependency, and created test suite forAgentClientAgentPort[cb8036b]AiClientclass in@forestadmin/ai-proxypackage to manage AI model instances and MCP tool lifecycle [0ebae51]Routerclass in@forestadmin/ai-proxyto use extracted validation and configuration utilities [0ebae51]createBaseChatModelfunction andAiClientclass from@forestadmin/ai-proxypackage public API [0ebae51]AiClientclass,createBaseChatModelfunction, andgetAiConfigurationfunction [0ebae51]ForestServerWorkflowPortadapter class that implementsWorkflowPortinterface using@forestadmin/forestadmin-clientServerUtils.queryto communicate with Forest server endpoints [c25a953]WorkflowPortinterface method fromcompleteStepExecutiontoupdateStepExecution[c25a953]@forestadmin/forestadmin-clientversion1.37.17as dependency to@forestadmin/workflow-executorpackage and exportedForestServerWorkflowPortfrom package index [c25a953]ServerUtilsas named export from@forestadmin/forestadmin-clientpackage [c25a953]BaseStepExecutorto provide unified execution wrapper with error handling, confirmation support, and shared utilities [39de72a]StepExecutorFactoryto instantiate executors by step type with fallback error handling [39de72a]Runnerclass with polling loop, HTTP server, and step execution orchestration [39de72a]ExecutorHttpServerto return generic error messages and handleRunNotFoundErrorwith 404 response [39de72a]WorkflowExecutorErrorbase class and added thirteen new error subclasses with user-facing messages [39de72a]AgentPortinterface andAgentClientAgentPortadapter to use query objects with optional fields and pagination [39de72a]StepSummaryBuilderandStepExecutionFormattersfor generating multi-line step summaries with custom formatting [39de72a]getPendingStepExecutionsForRun()method toWorkflowPortandForestServerWorkflowPortadapter [39de72a]ExecutionContextinterface to replacehistorywithpreviousSteps, addlogger, and includeuserConfirmedflag [39de72a]ConsoleLoggeradapter implementingLoggerport for structured error logging [39de72a]SafeAgentPortwrapper to convert infrastructure errors toAgentPortError[39de72a]@forestadmin/ai-proxyexports and replaced@langchain/coredependency in@forestadmin/workflow-executor[39de72a]jest.config.tsmoduleNameMapper to resolve@anthropic-ai/sdksubpath imports [39de72a]isExecutedStepOnExecutor[39de72a]/runs/:runIdendpoints inExecutorHttpServer[de39c30]Runner.start[de39c30]hasRunAccessmethod toWorkflowPortinterface and implemented it inForestServerWorkflowPort[de39c30]ConfigurationErrorandvalidateSecretsfrom the package entry point [de39c30]jsonwebtoken,koa-jwt, and@types/jsonwebtokento the package [de39c30]WorkflowPortinterface [de39c30]pendingDatainRunStoreinstead ofExecutionContext[9399bb7]McpTaskStepExecutorto persist formatted responses with best-effort error handling [9399bb7]@koa/bodyparserdependency version^6.1.0to@forestadmin/workflow-executorpackage [9399bb7]initandcloselifecycle methods to theRunStoreinterface with optionalLoggerparameters [50583e8]DatabaseStoreclass as a Sequelize-backedRunStorewith migration support [50583e8]InMemoryStoreclass as a Map-backedRunStore[50583e8]buildDatabaseRunStoreandbuildInMemoryRunStoreto construct initializedRunStoreinstances [50583e8]RunStorelifecycle methods intoRunner.startandRunner.stop[50583e8]InMemoryStore,DatabaseStore,DatabaseStoreOptions,buildDatabaseRunStore, andbuildInMemoryRunStorefrom the package index [50583e8]RunStorelifecycle methods [50583e8]sequelizeandumzug, and dev dependencies@types/sequelizeandsqlite3[50583e8]DatabaseStoreandInMemoryStore[50583e8]/runs/:runId/steps/:stepIndex/pending-dataendpoint [6e7858a]LoadRelatedRecordStepExecutorto deriverelatedCollectionNamefrom schema at resolution time instead of storing it inpendingData[6e7858a]relatedCollectionNameoptional property toFieldSchemainterface [6e7858a]relatedCollectionNamefromLoadRelatedRecordPendingData[6e7858a]Runner.patchPendingDataandRunner.getRunStepExecutionsmethods to centralize pending-data validation and persistence logic [a52c00a]PendingDataNotFoundErrorandInvalidPendingDataErrorclasses with distinction between boundary and domain error families [a52c00a]RunStoredependency fromhttp.ExecutorHttpServerand delegated run retrieval and pending-data patching toRunner[a52c00a]pending-data-validatorsmodule fromsrc/http/tosrc/directory [a52c00a]Runnermethods instead ofRunStoreand verify new error handling behavior [a52c00a]PATCH /runs/:runId/steps/:stepIndex/pending-dataendpoint with Zod-validated request bodies, strict rejection of unknown fields, and response status codes (204/400/404) for updating pending data [cf7d069]userConfirmedfield inpendingDataduring step execution, returningawaiting-inputstatus when undefined, executing when true, and skipping when false [cf7d069]update-record,trigger-action,mcp-task, andload-related-recordstep pending data contracts to includeuserConfirmedfield and defined step-type-specific PATCH request body schemas [cf7d069]userConfirmedfield fromPendingStepExecutioninterface, updated last-updated date, reworded note aboutPOST /completebody, and removed TODOs about unimplemented pending-data endpoint [cf7d069]user: StepUserparameter to allAgentPortinterface methods (getRecord,updateRecord,getRelatedData,executeAction) and propagated user context fromExecutionContextthrough all step executors (ReadRecordStepExecutor,UpdateRecordStepExecutor,LoadRelatedRecordStepExecutor,TriggerRecordActionStepExecutor) to their respectiveAgentPortcalls, and updatedSafeAgentPortwrapper to forward the user parameter [cf8e699]AgentClientAgentPortto construct per-callRemoteAgentClientinstances authenticated with short-lived JWTs signed usingauthSecretand scoped to 'step-execution', replacing the previous constructor parameters (client,collectionSchemas) withagentUrl,authSecret, andschemaCache, and building action endpoints dynamically from cached schemas [cf8e699]PATCH /runs/:runId/steps/:stepIndex/pending-dataendpoint fromExecutorHttpServerand extendedPOST /runs/:runId/triggerto accept optionalpendingDatain the request body, validatebearerUserIdfrom the decoded JWT token, and handleUserMismatchError,PendingDataNotFoundError, andInvalidPendingDataErrorwith specific HTTP status codes (403, 404, 400 respectively) [cf8e699]Runner.triggerPollsignature to accept an options object containing optionalpendingDataandbearerUserId, added user authorization check that throwsUserMismatchErrorwhenbearerUserIddoes not match the pending step'suser.id, madepatchPendingDataprivate, and invoked it withintriggerPollwhenpendingDatais provided [cf8e699]SchemaCacheclass providing TTL-based in-memory caching ofCollectionSchemaobjects with configurable expiration (default 10 minutes), an iterator for non-expired entries, and integrated it intoExecutionContext,StepContextConfig,BaseStepExecutor.getCollectionSchema,AgentClientAgentPort, andRunnerviabuildCommonDependencies[cf8e699]ConditionStepExecutor.doExecuteto return an error outcome withstatus: 'error'and a user-facing message instructing to rephrase the prompt when the AI does not select an option, replacing the previous 'manual-decision' status, and removed theConditionStepStatustype fromstep-outcome.ts[cf8e699]buildInMemoryExecutorandbuildDatabaseExecutorfactory functions that construct aRunnerinstance with wired dependencies includingInMemoryStore/DatabaseStore,ForestServerWorkflowPort,AiClient,SchemaCache, andAgentClientAgentPortusing providedExecutorOptionsorDatabaseExecutorOptions[cf8e699]endpoint: stringproperty to theActionSchemainterface intypes/record.ts[cf8e699]once<T>memoization utility fromRunnerand updatedexecuteStepandrunPollCycleto callfetchRemoteToolsdirectly for each step execution instead of using a per-cycle memoized loader [cf8e699]WorkflowPort.hasRunAccesssignature to acceptuser: StepUserinstead ofuserToken: string, changedExecutorHttpServerto use a newhasRunAccessMiddlewarethat callshasRunAccesswith the decoded user object fromctx.state.userfor theGET /runs/:runIdroute, and removed the previous router-level access middleware that validated raw tokens [cf8e699]Runnerclass andWorkflowExecutorfacade [7142e6c]RunnerintoExecutorHttpServercomposition at facade level [7142e6c]GET /healthendpoint toExecutorHttpServerreturning runner state [7142e6c]Loggerinterface with optionalinfo()method and implemented inConsoleLogger[7142e6c]Macroscope summarized 17f26ca.