feat(workflow-executor): add TriggerActionStepExecutor with confirmation flow#1501
feat(workflow-executor): add TriggerActionStepExecutor with confirmation flow#1501Scra3 wants to merge 34 commits intofeat/prd-219-update-record-step-executorfrom
Conversation
…ion flow
Implements TriggerActionStepExecutor following the UpdateRecordStepExecutor
pattern (branches A/B/C, confirmation flow, automaticExecution).
- Add TriggerActionStepExecutionData type with executionParams (actionDisplayName
+ actionName), executionResult ({ success } | { skipped }), and pendingAction
- Add NoActionsError for collections with no actions
- Implement selectAction via AI tool with displayName enum and technical name hints
- resolveAndExecute stores the technical actionName in executionParams for
traceability; action result discarded per privacy constraint
- Fix buildStepSummary in BaseStepExecutor to include trigger-action pendingAction
in prior-step AI context (parity with update-record pendingUpdate)
- Export TriggerActionStepExecutor, TriggerActionStepExecutionData, NoActionsError
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ac706e8 to
c884d37
Compare
…t and pendingAction Resolve actionName once in handleFirstCall and store it in pendingAction, so resolveAndExecute receives it directly via ActionTarget without re-fetching the schema. Rename TriggerTarget → ActionTarget for consistency with English naming conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8 new issues
|
…f shapes to { name, displayName }
- Extract ActionRef { name, displayName } from inline types in TriggerActionStepExecutionData
- Extract FieldRef { name, displayName } replacing FieldReadBase in ReadRecord types
- UpdateRecordStepExecutionData executionParams/pendingUpdate now use FieldRef & { value }
- Rename actionDisplayName/actionName → displayName/name, fieldDisplayName/fieldName → displayName/name
- Move resolveFieldName to handleFirstCall (no re-resolution in resolveAndUpdate)
- Add missing tests: resolveActionName not-found path, saveStepExecution not-called assertions, trigger-action pendingAction in buildStepSummary
- Export ActionRef, FieldRef, NoActionsError from index; update CLAUDE.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…store display names in executionParams Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rams for consistency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-executor Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y" wording Use fieldNames instead of fieldDisplayNames and actionName instead of displayName in tool schemas exposed to the AI, to avoid confusion between property names and their semantics (values remain display names). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ize pending state - Make buildOutcomeResult abstract on BaseStepExecutor; each branch owns its outcome shape - Add RecordTaskStepExecutor intermediate class implementing buildOutcomeResult for 'record-task' - Add pendingData to BaseStepExecutionData, replacing pendingUpdate/pendingAction per-type fields; each executor sets it when saving, base class reads it directly with no type discrimination - Rename TriggerActionStepExecutor → TriggerRecordActionStepExecutor for naming consistency with ReadRecord/UpdateRecord Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…idate error handling - Introduce doExecute() as the abstract hook; execute() in the base class wraps it with the single try-catch that converts WorkflowExecutorErrors to error outcomes and rethrows infrastructure errors — removing all try-catch boilerplate from subclasses - Inline the former buildErrorOutcomeOrThrow helper directly into execute(), it no longer needs to be a named method - Add StepPersistenceError (extends WorkflowExecutorError) for post-side-effect persistence failures; these are now consistently converted to error outcomes - Extract handleConfirmationFlow into RecordTaskStepExecutor to DRY the confirmation pattern shared by update-record and trigger-record-action - Split the !execution?.pendingData guard into two distinct WorkflowExecutorErrors for clearer debug messages - Export BaseStepStatus from step-outcome; remove pendingData from BaseStepExecutionData (declared only on the concrete types that need it) - Fix extractToolCallArgs to throw MalformedToolCallError when args is null/undefined Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…error hierarchy, and AI-first Branch C - Add LoadRelatedRecordStepExecutor with Branch A/B/C confirmation flow - Replace candidates[] in LoadRelatedRecordPendingData with AI-selected suggestedRecordId/suggestedFields/relatedCollectionName - Extract selectBestFromRelatedData to share AI selection logic between Branch B and Branch C - Make WorkflowExecutorError abstract; introduce InvalidAIResponseError, RelationNotFoundError, FieldNotFoundError, ActionNotFoundError, StepStateError - Fix selectRelevantFields to use displayName in Zod enum and map back to fieldName - Add getRelatedData fields param forwarding in AgentClientAgentPort - Update CLAUDE.md with displayName-in-AI-tools and error hierarchy rules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ser facing messages Separates technical error messages (dev logs) from user-facing messages (Forest Admin UI). WorkflowExecutorError now carries a readonly `userMessage` field; base-step-executor uses it instead of `message` when building the step outcome error. Each subclass declares its own user-oriented message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts
Show resolved
Hide resolved
…nject logger into execution context Introduces a Logger interface (port) and ConsoleLogger (default implementation). Adds logger to ExecutionContext, masks raw error messages from the HTTP API (security), and logs unhandled HTTP errors with context instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eLogger output Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ated-record executor, rename ids→id
- Add StepExecutionFormatters (static per-type formatters) and StepSummaryBuilder
(orchestrates step summary for AI context); decouple formatting from BaseStepExecutor
- Load-related-record executionResult is now self-contained: { relation: RelationRef; record: RecordRef }
- Rename ids → id in AgentPort and all callers (id is a composite key of one record, not multiple records)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ve summary to sub-folder - Replace QueryBase intersections with named query types (GetRecordQuery, UpdateRecordQuery, GetRelatedDataQuery, ExecuteActionQuery) for better DX and readability - Save executeAction return value in executionResult.actionResult instead of discarding it - Move StepSummaryBuilder and StepExecutionFormatters to executors/summary/ sub-folder Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…edData signature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… into McpTaskStepExecutor remoteTools was a dependency of McpTaskStepExecutor only — 5 of 6 executors never used it. Inject it explicitly via the constructor so ExecutionContext stays focused on execution state, and remove the now-meaningless remoteTools: [] boilerplate from every other executor test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…utor Any WorkflowExecutorError with a cause is now logged automatically by the base executor using error.message as the log message. Removes the manual logger.error call from McpTaskStepExecutor and the StepPersistenceError- specific guard. Moves cause? up to the base class, removing duplicate declarations from StepPersistenceError and McpToolInvocationError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add AgentPortError and SafeAgentPort to centralise infra error handling for all agentPort operations (getRecord, updateRecord, getRelatedData, executeAction). - add AgentPortError extending WorkflowExecutorError with a user-friendly message and structured cause logging - add SafeAgentPort implementing AgentPort: wraps infra errors in AgentPortError, passes through WorkflowExecutorError subclasses unchanged - expose this.agentPort (SafeAgentPort) as a protected property in BaseStepExecutor; all executors use it instead of this.context.agentPort - export AgentPortError from index.ts - add unit tests for SafeAgentPort and one integration test per executor verifying userMessage and logger.error cause on infra failures Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ask dependencies Add McpToolRef, McpToolCall and McpTaskStepExecutionData to step-execution-data, required by mcp-task-step-executor. Update package.json to depend on @forestadmin/ai-proxy and align @langchain/core version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Export RemoteTool default, BaseChatModel, BaseMessage, HumanMessage, SystemMessage, StructuredToolInterface and DynamicStructuredTool so consumers of ai-proxy do not need a direct @langchain/core dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…restadmin/ai-proxy Replace all direct @langchain/core imports in src and tests with @forestadmin/ai-proxy re-exports. Add moduleNameMapper in jest.config.ts to resolve @anthropic-ai/sdk wildcard exports (Jest < 30 workaround, same pattern as ai-proxy). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@langchain/core is now consumed transitively via @forestadmin/ai-proxy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Implement polling loop with configurable interval and auto-reschedule - Add getExecutor() factory dispatching all StepTypes to their executor - Add triggerPoll() for webhook-driven execution - Add in-flight step deduplication via inFlightSteps Set - Add MCP lazy tool loading with once() thunk - Replace McpConfiguration placeholder type with @forestadmin/ai-proxy type - Fix missing stepIndex field in step-scoped error logs - Fix missing cause field in unexpected error logs in BaseStepExecutor - Add comprehensive test coverage for Runner Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| return this.config.aiClient.loadRemoteTools(mergedConfig); | ||
| } | ||
|
|
||
| private async executeStep( |
There was a problem hiding this comment.
🟡 Medium src/runner.ts:182
inFlightSteps.delete(key) runs in the finally block before updateStepExecution is awaited, so a concurrent poll cycle can re-execute the step before the backend marks it complete. Move the delete to after updateStepExecution succeeds, or await the update inside the try block before cleanup.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/runner.ts around line 182:
`inFlightSteps.delete(key)` runs in the `finally` block before `updateStepExecution` is awaited, so a concurrent poll cycle can re-execute the step before the backend marks it complete. Move the delete to after `updateStepExecution` succeeds, or await the update inside the try block before cleanup.
Evidence trail:
packages/workflow-executor/src/runner.ts lines 183-220 (REVIEWED_COMMIT): executeStep method shows `inFlightSteps.add(key)` at line 188, `finally { this.inFlightSteps.delete(key); }` at lines 210-212, and `await this.config.workflowPort.updateStepExecution(...)` at line 215 which is outside and after the try-catch-finally block.
packages/workflow-executor/src/runner.ts lines 153-166 (REVIEWED_COMMIT): runPollCycle method fetches pending steps from backend and filters using `!this.inFlightSteps.has(stepKey(s))`, showing the race window where a step could be re-fetched and re-executed.
After a MCP tool runs, trigger a second AI call that produces a concise human-readable summary stored as `formattedResponse` in `McpTaskStepExecutionData.executionResult`. The raw result is persisted first (safe state); the formatting step is non-blocking — failures are logged but never fail the step. Results longer than 20 000 chars are truncated before injection. `StepExecutionFormatters` now handles `mcp-task`: returns `Result: <summary>` when available, or a generic fallback line to avoid exposing raw `toolResult` in subsequent AI context messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…askStepExecutor Move handleConfirmationFlow() up to BaseStepExecutor so it is reusable by any executor. McpTaskStepExecutor now extends BaseStepExecutor directly instead of RecordTaskStepExecutor, and emits its own type 'mcp-task' outcome. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tic methods on Runner once, stepKey, stepOutcomeType, causeMessage are now private static methods on the Runner class instead of loose module-level functions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tory Move step dispatch logic from runner.ts into a dedicated StepExecutorFactory.create() static method in the executors/ folder, co-locating it with its peers. Runner is now solely responsible for orchestration; step type routing lives in the factory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pExecutor - Add IStepExecutor interface to execution.ts - Add stepTypeToOutcomeType() helper to step-outcome.ts - Add ErrorStepExecutor: catches construction errors, returns typed error outcome - StepExecutorFactory absorbs buildContext, takes StepContextConfig, never throws - Runner simplified: removes buildContext, stepOutcomeType, StepOutcome knowledge - Extract causeMessage() to errors.ts (shared utility, removes DRY violation) - BaseStepExecutor explicitly implements IStepExecutor - Add error-step-executor.test.ts with full coverage (contract, logging, type mapping) - Strengthen runner.test.ts: FATAL path, loadTools rejection, non-Error throwable, stepTypeToOutcomeType unit tests, deduplication assertion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| 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.
🟢 Low src/errors.ts:3
causeMessage throws a TypeError when error is null or undefined because the destructuring const { cause } = error as ... attempts to read from a non-object. Since this function handles errors from catch blocks where throw null or throw undefined is valid, consider adding a null check before the destructuring.
export function causeMessage(error: unknown): string | undefined {
+ if (error == null) return undefined;
const { cause } = error as { cause?: unknown };
return cause instanceof Error ? cause.message : undefined;🚀 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` throws a `TypeError` when `error` is `null` or `undefined` because the destructuring `const { cause } = error as ...` attempts to read from a non-object. Since this function handles errors from catch blocks where `throw null` or `throw undefined` is valid, consider adding a null check before the destructuring.
Evidence trail:
packages/workflow-executor/src/errors.ts lines 3-6: `causeMessage` function with destructuring `const { cause } = error as { cause?: unknown }`
packages/workflow-executor/src/runner.ts line 167: `causeMessage(error)` called in catch block
packages/workflow-executor/src/executors/error-step-executor.ts line 20: `causeMessage(this.error)` called with `unknown` typed error
…andling in factory ErrorStepExecutor had no meaningful state once logging was moved out of it. The factory catch block now logs directly and returns an inline IStepExecutor. stepTypeToOutcomeType unit tests moved to test/types/step-outcome.test.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…iggerPoll - Add RunNotFoundError (extends Error, not WorkflowExecutorError) - Add getPendingStepExecutionsForRun(runId) to WorkflowPort (returns PendingStepExecution | null) - Implement in ForestServerWorkflowPort with runId query param + URL encoding - triggerPoll uses the new method, throws RunNotFoundError if step is null - handleTrigger returns 404 on RunNotFoundError, rethrows otherwise Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…a — replace suggestedRecordId + selectedRecordId? with a single selectedRecordId The executor now writes the AI's pick directly into selectedRecordId. The future PATCH endpoint will overwrite it with the user's choice, removing the need to distinguish between "suggested" and "selected". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| // NOTE: The Zod schema's .min(0).max(maxIndex) shapes the tool prompt only — it is NOT | ||
| // validated against the AI response. This guard is the sole runtime enforcement. | ||
| if (recordIndex < 0 || recordIndex > maxIndex) { |
There was a problem hiding this comment.
🟢 Low executors/load-related-record-step-executor.ts:397
In selectBestRecordIndex, the guard at line 397 allows non-integer values like 1.5 to pass because it only checks range bounds, not whether the index is an integer. When recordIndex is 1.5, candidates[1.5] returns undefined, and toRecordRef(undefined) crashes when accessing data.collectionName. Add an integer check to the validation guard.
- if (recordIndex < 0 || recordIndex > maxIndex) {
+ if (!Number.isInteger(recordIndex) || recordIndex < 0 || recordIndex > maxIndex) {🚀 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 line 397:
In `selectBestRecordIndex`, the guard at line 397 allows non-integer values like `1.5` to pass because it only checks range bounds, not whether the index is an integer. When `recordIndex` is `1.5`, `candidates[1.5]` returns `undefined`, and `toRecordRef(undefined)` crashes when accessing `data.collectionName`. Add an integer check to the validation guard.
Evidence trail:
packages/workflow-executor/src/executors/load-related-record-step-executor.ts lines 395-401 (guard only checks range bounds with `recordIndex < 0 || recordIndex > maxIndex`), lines 182-186 and 193 (bestIndex returned from selectBestRecordIndex is used as array index: `relatedData[bestIndex]`), lines 405-410 (toRecordRef accesses `data.collectionName` which crashes when data is undefined)
…ontract doc Routes for the pending-data write endpoint are not yet defined (PRD-240). Replace the made-up PATCH route with explicit TODO markers throughout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Summary
TriggerActionStepExecutorfollowing theUpdateRecordStepExecutorpattern (branches A/B/C,automaticExecution, confirmation flow)select-actiontool (Zod enum of display names + technical names as hints);resolveActionNamemaps displayName → technical name with name fallbackTriggerActionStepExecutionDatawithexecutionParams: { actionDisplayName, actionName },executionResult: { success } | { skipped }, andpendingActionfor the awaiting-confirmation stateNoActionsErrorfor collections with no actionsbuildStepSummaryinBaseStepExecutorto includetrigger-actionpendingActionin prior-step AI context (parity withupdate-recordpendingUpdate)Test plan
automaticExecution: true) — triggers action, savesexecutionParamswith both display and technical name, returnssuccessautomaticExecution) — savespendingAction, returnsawaiting-inputpendingActionfor traceability{ skipped: true }NoActionsError— returnserroroutcomeWorkflowExecutorErrorfromexecuteAction→erroroutcome (branches A & B)resolveActionNamefailure (Branch A — action deleted between confirmation steps)erroroutcomegetCollectionSchemacall per collection)stepOutcomeshape🤖 Generated with Claude Code
Note
Add TriggerRecordActionStepExecutor with confirmation flow to workflow-executor
TriggerRecordActionStepExecutor,LoadRelatedRecordStepExecutor,McpTaskStepExecutor, andRecordTaskStepExecutor(abstract base), each supporting automatic execution and a user-confirmation flow viahandleConfirmationFlow.BaseStepExecutorto wrap all execution in a commonexecute()→doExecute()pattern that mapsWorkflowExecutorErrorsubclasses to structured error outcomes and logs unexpected errors instead of throwing.SafeAgentPortto normalize underlying agent-port errors toAgentPortError, and rewrites allAgentPortmethod signatures to accept query objects instead of positional parameters.errors.tswith ~15 new typed errors (e.g.StepPersistenceError,FieldNotFoundError,AgentPortError) each carrying a user-facinguserMessage.Runnerpolling loop: periodic step fetching, in-flight deduplication, lazy MCP tool loading, and outcome reporting viaworkflowPort.updateStepExecution.AgentPortmethods now require object-shaped query parameters; existing adapters and callers must be updated to the new signatures.Macroscope summarized a5d6249.