From e8f778b252574664be478e217655339478cd8923 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 16:22:56 +0200 Subject: [PATCH 01/11] fix(workflow-executor): operation activity log targets the acted record (PRD-442 #1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operation audit-trail entry pointed at the run's trigger record, even when the step acted on a record loaded earlier in the run — possibly in another collection. Root cause: buildActivityLogArgs ran before doExecute (so the acted record wasn't resolved yet) and used context.collectionId + baseRecordRef. Record executors now emit the entry at the point of the agent call, via a new base helper withActivityLog(args, fn), targeting selectedRecordRef and its collection's numeric id (read from the live schema). CollectionSchema gains a required collectionId — the audit is not optional, so a missing id fails loud at the boundary (DomainValidationError); the orchestrator must include it in getCollectionSchema responses (coordinated deploy). Scope: bug #1 (wrong target) only. As a consequence of logging at the operation point, a confirmation step no longer logs before the write — the remaining #2 work (exactly-once / no-log-on-reject, MCP step) is a follow-up. The MCP executor still uses context.collectionId via the existing wrapper. fixes PRD-442 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/adapters/agent-client-agent-port.ts | 1 + .../src/executors/base-step-executor.ts | 19 ++++++ .../load-related-record-step-executor.ts | 65 +++++++++---------- .../executors/read-record-step-executor.ts | 27 +++----- .../src/executors/record-step-executor.ts | 16 +++++ .../trigger-record-action-step-executor.ts | 33 +++------- .../executors/update-record-step-executor.ts | 27 +++----- .../src/types/validated/collection.ts | 1 + .../adapters/agent-client-agent-port.test.ts | 6 ++ .../forest-server-workflow-port.test.ts | 13 ++++ .../load-related-record-step-executor.test.ts | 29 +++++++++ .../read-record-step-executor.test.ts | 23 ++++++- ...rigger-record-action-step-executor.test.ts | 28 ++++++++ .../update-record-step-executor.test.ts | 49 ++++++++++++++ .../integration/workflow-execution.test.ts | 5 ++ .../test/schema-cache.test.ts | 1 + 16 files changed, 250 insertions(+), 93 deletions(-) diff --git a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts index 9ca49d4500..26557cdd77 100644 --- a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts +++ b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts @@ -301,6 +301,7 @@ export default class AgentClientAgentPort implements AgentPort { return ( cached ?? { collectionName, + collectionId: collectionName, collectionDisplayName: collectionName, primaryKeyFields: ['id'], fields: [], diff --git a/packages/workflow-executor/src/executors/base-step-executor.ts b/packages/workflow-executor/src/executors/base-step-executor.ts index fb91e1549f..c1c46643e4 100644 --- a/packages/workflow-executor/src/executors/base-step-executor.ts +++ b/packages/workflow-executor/src/executors/base-step-executor.ts @@ -150,6 +150,25 @@ export default abstract class BaseStepExecutor( + args: CreateActivityLogArgs, + operation: () => Promise, + ): Promise { + const handle = await this.context.activityLogPort.createPending(args); + + try { + const result = await operation(); + void this.context.activityLogPort.markSucceeded(handle); + + return result; + } catch (err) { + const errorMessage = + err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; + void this.context.activityLogPort.markFailed(handle, errorMessage); + throw err; + } + } + // Promise.race doesn't abort the losing branch — it keeps running in the background. The .catch() // on execPromise must be attached BEFORE the race so a late rejection doesn't trigger // UnhandledPromiseRejection. Late resolutions are silently discarded. diff --git a/packages/workflow-executor/src/executors/load-related-record-step-executor.ts b/packages/workflow-executor/src/executors/load-related-record-step-executor.ts index 232e359f38..07eceeb00f 100644 --- a/packages/workflow-executor/src/executors/load-related-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/load-related-record-step-executor.ts @@ -1,4 +1,3 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; import type { StepExecutionResult } from '../types/execution-context'; import type { LoadRelatedRecordCandidate, @@ -56,15 +55,7 @@ interface RelationTarget extends RelationRef { } export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - protected override buildActivityLogArgs(): CreateActivityLogArgs | null { - return { - renderingId: this.context.user.renderingId, - action: 'listRelatedData', - type: 'read', - collectionId: this.context.collectionId, - recordId: this.context.baseRecordRef.recordId, - }; - } + protected readonly operation = { action: 'listRelatedData', type: 'read' } as const; protected async doExecute(): Promise { // Branch A -- Re-entry after pending execution found in RunStore @@ -197,35 +188,37 @@ export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - if (target.relationType === 'BelongsTo' || target.relationType === 'HasOne') { - const candidate = await this.fetchXToOneCandidate(target); + return this.logOperation(target.selectedRecordRef, async () => { + if (target.relationType === 'BelongsTo' || target.relationType === 'HasOne') { + const candidate = await this.fetchXToOneCandidate(target); - return candidate - ? { availableRecordIds: [candidate], suggestedRecord: candidate } - : { availableRecordIds: [] }; - } + return candidate + ? { availableRecordIds: [candidate], suggestedRecord: candidate } + : { availableRecordIds: [] }; + } - const { relatedData, bestIndex, relatedSchema } = await this.selectBestFromRelatedData( - target, - 50, - ); + const { relatedData, bestIndex, relatedSchema } = await this.selectBestFromRelatedData( + target, + 50, + ); - if (relatedData.length === 0) { - return { availableRecordIds: [] }; - } + if (relatedData.length === 0) { + return { availableRecordIds: [] }; + } - const referenceField = relatedSchema.referenceField ?? null; - const toCandidate = (r: RecordData): LoadRelatedRecordCandidate => ({ - recordId: r.recordId, - referenceFieldValue: referenceField - ? this.extractReferenceFieldValue(r.values, referenceField) - : null, - }); + const referenceField = relatedSchema.referenceField ?? null; + const toCandidate = (r: RecordData): LoadRelatedRecordCandidate => ({ + recordId: r.recordId, + referenceFieldValue: referenceField + ? this.extractReferenceFieldValue(r.values, referenceField) + : null, + }); - return { - availableRecordIds: relatedData.map(toCandidate), - suggestedRecord: toCandidate(relatedData[bestIndex]), - }; + return { + availableRecordIds: relatedData.map(toCandidate), + suggestedRecord: toCandidate(relatedData[bestIndex]), + }; + }); } private extractReferenceFieldValue( @@ -239,7 +232,9 @@ export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - const record = await this.fetchRecordForRelation(target); + const record = await this.logOperation(target.selectedRecordRef, () => + this.fetchRecordForRelation(target), + ); return this.persistAndReturn(record, target, undefined); } diff --git a/packages/workflow-executor/src/executors/read-record-step-executor.ts b/packages/workflow-executor/src/executors/read-record-step-executor.ts index b6c761632d..5bf4b798d7 100644 --- a/packages/workflow-executor/src/executors/read-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/read-record-step-executor.ts @@ -1,4 +1,3 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; import type { StepExecutionResult } from '../types/execution-context'; import type { FieldReadResult } from '../types/step-execution-data'; import type { CollectionSchema } from '../types/validated/collection'; @@ -19,15 +18,7 @@ Important rules: - Do not refer to yourself as "I" in the response, use a passive formulation instead.`; export default class ReadRecordStepExecutor extends RecordStepExecutor { - protected override buildActivityLogArgs(): CreateActivityLogArgs | null { - return { - renderingId: this.context.user.renderingId, - action: 'index', - type: 'read', - collectionId: this.context.collectionId, - recordId: this.context.baseRecordRef.recordId, - }; - } + protected readonly operation = { action: 'index', type: 'read' } as const; protected async doExecute(): Promise { const { stepDefinition: step } = this.context; @@ -50,13 +41,15 @@ export default class ReadRecordStepExecutor extends RecordStepExecutor + this.agentPort.getRecord( + { + collection: selectedRecordRef.collectionName, + id: selectedRecordRef.recordId, + fields: resolvedFieldNames, + }, + this.context.user, + ), ); const fieldResults = this.formatFieldResults(recordData.values, schema, selectedDisplayNames); diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index 4430dfacc4..32111472aa 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -12,6 +12,22 @@ import BaseStepExecutor from './base-step-executor'; export default abstract class RecordStepExecutor< TStep extends StepDefinition = StepDefinition, > extends BaseStepExecutor { + protected abstract readonly operation: { action: string; type: 'read' | 'write' }; + + protected async logOperation(record: RecordRef, run: () => Promise): Promise { + const { collectionId } = await this.getCollectionSchema(record.collectionName); + + return this.withActivityLog( + { + renderingId: this.context.user.renderingId, + ...this.operation, + collectionId, + recordId: record.recordId, + }, + run, + ); + } + protected buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; diff --git a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts index 3f8d09e504..7241e4e86b 100644 --- a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts +++ b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts @@ -1,4 +1,3 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; import type { StepExecutionResult } from '../types/execution-context'; import type { ActionRef, TriggerRecordActionStepExecutionData } from '../types/step-execution-data'; import type { CollectionSchema, RecordRef } from '../types/validated/collection'; @@ -29,21 +28,7 @@ interface ActionTarget extends ActionRef { } export default class TriggerRecordActionStepExecutor extends RecordStepExecutor { - protected override buildActivityLogArgs(): CreateActivityLogArgs | null { - // Skip when the frontend executes the action itself (non fully-automated mode). - // The front logs on its side via the standard agent activity flow. - if (this.context.stepDefinition.executionType !== StepExecutionMode.FullyAutomated) { - return null; - } - - return { - renderingId: this.context.user.renderingId, - action: 'action', - type: 'write', - collectionId: this.context.collectionId, - recordId: this.context.baseRecordRef.recordId, - }; - } + protected readonly operation = { action: 'action', type: 'write' } as const; protected override async checkIdempotency(): Promise { const existing = await this.findPendingExecution( @@ -153,13 +138,15 @@ export default class TriggerRecordActionStepExecutor extends RecordStepExecutor< idempotencyPhase: 'executing', }); - const actionResult = await this.agentPort.executeAction( - { - collection: selectedRecordRef.collectionName, - action: name, - id: selectedRecordRef.recordId, - }, - this.context.user, + const actionResult = await this.logOperation(selectedRecordRef, () => + this.agentPort.executeAction( + { + collection: selectedRecordRef.collectionName, + action: name, + id: selectedRecordRef.recordId, + }, + this.context.user, + ), ); await this.context.runStore.saveStepExecution(this.context.runId, { diff --git a/packages/workflow-executor/src/executors/update-record-step-executor.ts b/packages/workflow-executor/src/executors/update-record-step-executor.ts index f978e16d04..4f08f61f9b 100644 --- a/packages/workflow-executor/src/executors/update-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/update-record-step-executor.ts @@ -1,4 +1,3 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; import type { StepExecutionResult } from '../types/execution-context'; import type { FieldWithValue, UpdateRecordStepExecutionData } from '../types/step-execution-data'; import type { CollectionSchema, FieldSchema, RecordRef } from '../types/validated/collection'; @@ -127,15 +126,7 @@ interface UpdateTarget extends FieldWithValue { } export default class UpdateRecordStepExecutor extends RecordStepExecutor { - protected override buildActivityLogArgs(): CreateActivityLogArgs | null { - return { - renderingId: this.context.user.renderingId, - action: 'update', - type: 'write', - collectionId: this.context.collectionId, - recordId: this.context.baseRecordRef.recordId, - }; - } + protected readonly operation = { action: 'update', type: 'write' } as const; protected override async checkIdempotency(): Promise { const existing = await this.findPendingExecution( @@ -267,13 +258,15 @@ export default class UpdateRecordStepExecutor extends RecordStepExecutor + this.agentPort.updateRecord( + { + collection: selectedRecordRef.collectionName, + id: selectedRecordRef.recordId, + values: { [name]: value }, + }, + this.context.user, + ), ); await this.context.runStore.saveStepExecution(this.context.runId, { diff --git a/packages/workflow-executor/src/types/validated/collection.ts b/packages/workflow-executor/src/types/validated/collection.ts index 5ca727fc8a..27ddda751c 100644 --- a/packages/workflow-executor/src/types/validated/collection.ts +++ b/packages/workflow-executor/src/types/validated/collection.ts @@ -74,6 +74,7 @@ export type ActionSchema = z.infer; export const CollectionSchemaSchema = z .object({ collectionName: z.string().min(1), + collectionId: z.string().min(1), // null when the rendering has no explicit displayName configured — normalized to collectionName. collectionDisplayName: z.string().nullable(), primaryKeyFields: z.array(z.string().min(1)).min(1), diff --git a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts index cf03360cc1..fd2086abf3 100644 --- a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts +++ b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts @@ -49,6 +49,7 @@ describe('AgentClientAgentPort', () => { const schemaCache = new SchemaCache(); schemaCache.set('users', { collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [ @@ -62,6 +63,7 @@ describe('AgentClientAgentPort', () => { }); schemaCache.set('orders', { collectionName: 'orders', + collectionId: 'col-orders', collectionDisplayName: 'Orders', primaryKeyFields: ['tenantId', 'orderId'], fields: [ @@ -72,6 +74,7 @@ describe('AgentClientAgentPort', () => { }); schemaCache.set('posts', { collectionName: 'posts', + collectionId: 'col-posts', collectionDisplayName: 'Posts', primaryKeyFields: ['id'], fields: [ @@ -273,6 +276,7 @@ describe('AgentClientAgentPort', () => { describe('getRelatedData', () => { const postsSchema = { collectionName: 'posts', + collectionId: 'col-posts', collectionDisplayName: 'Posts', primaryKeyFields: ['id'], fields: [ @@ -480,6 +484,7 @@ describe('AgentClientAgentPort', () => { // that jsonapi-serializer emits as a nested object on the parent. const ordersSchema = { collectionName: 'orders', + collectionId: 'col-orders', collectionDisplayName: 'Orders', primaryKeyFields: ['id'], fields: [ @@ -758,6 +763,7 @@ describe('AgentClientAgentPort', () => { const schemaCache = new SchemaCache(); schemaCache.set('users', { collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [{ fieldName: 'id', displayName: 'id', isRelationship: false }], diff --git a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts index 30c8a4eaca..56cd44385f 100644 --- a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts +++ b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts @@ -591,6 +591,7 @@ describe('ForestServerWorkflowPort', () => { describe('getCollectionSchema', () => { const collectionSchema: CollectionSchema = { collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [], @@ -626,6 +627,7 @@ describe('ForestServerWorkflowPort', () => { // Shape invalide : fields[0] manque fieldName (violation FieldSchema.fieldName.min(1)). mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [{ displayName: 'Email', isRelationship: false }], @@ -638,6 +640,7 @@ describe('ForestServerWorkflowPort', () => { it('strips unknown extra fields on the wire (orchestrator drift tolerance)', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], referenceField: 'name', @@ -671,6 +674,7 @@ describe('ForestServerWorkflowPort', () => { it('defaults actions to [] when the orchestrator omits it', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [], @@ -684,6 +688,7 @@ describe('ForestServerWorkflowPort', () => { it('accepts a field without type (omitted by the orchestrator)', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [{ fieldName: 'email', displayName: 'Email', isRelationship: false }], @@ -700,6 +705,7 @@ describe('ForestServerWorkflowPort', () => { async displayName => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: displayName, primaryKeyFields: ['id'], fields: [], @@ -715,6 +721,7 @@ describe('ForestServerWorkflowPort', () => { it('accepts relationType BelongsToMany (many-to-many relation)', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [ @@ -738,6 +745,7 @@ describe('ForestServerWorkflowPort', () => { it("strips the target key from relatedCollectionName (Forest 'collection.key' reference)", async () => { mockQuery.mockResolvedValue({ collectionName: 'accounts', + collectionId: 'col-accounts', collectionDisplayName: 'Accounts', primaryKeyFields: ['id'], fields: [ @@ -761,6 +769,7 @@ describe('ForestServerWorkflowPort', () => { it('leaves relatedCollectionName unchanged when it carries no target key (no dot)', async () => { mockQuery.mockResolvedValue({ collectionName: 'accounts', + collectionId: 'col-accounts', collectionDisplayName: 'Accounts', primaryKeyFields: ['id'], fields: [ @@ -784,6 +793,7 @@ describe('ForestServerWorkflowPort', () => { it('accepts type File (Forest Admin extension)', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [ @@ -800,6 +810,7 @@ describe('ForestServerWorkflowPort', () => { it('accepts type [File] (array of files)', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [ @@ -821,6 +832,7 @@ describe('ForestServerWorkflowPort', () => { it('rejects enumValues: [] (empty enum is invalid)', async () => { mockQuery.mockResolvedValue({ collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [ @@ -1033,6 +1045,7 @@ describe('ForestServerWorkflowPort', () => { it('getCollectionSchema retries on HTTP 408 (timeout)', async () => { const validSchema: CollectionSchema = { collectionName: 'users', + collectionId: 'col-users', collectionDisplayName: 'Users', primaryKeyFields: ['id'], fields: [ diff --git a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts index 5ffe45226c..0b44f7671a 100644 --- a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts @@ -72,6 +72,7 @@ function makeMockAgentPort(relatedData: RecordData[] = [makeRelatedRecordData()] function makeCollectionSchema(overrides: Partial = {}): CollectionSchema { return { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -724,6 +725,34 @@ describe('LoadRelatedRecordStepExecutor', () => { }); }); + describe('operation activity log (PRD-442 #1)', () => { + it('logs listRelatedData against the source record and its collection, not the trigger', async () => { + const runStore = makeMockRunStore(); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const { model } = makeMockModel({ relationName: 'Order', reasoning: 'r' }); + const context = makeContext({ + model, + runStore, + activityLogPort, + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + await new LoadRelatedRecordStepExecutor(context).execute(); + + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'listRelatedData', + type: 'read', + collectionId: 'col-customers', + recordId: [42], + }); + }); + }); + describe('without executionType=FullyAutomated: awaiting-input (Branch C)', () => { it('saves AI suggestion in pendingData and returns awaiting-input (single record — no field/record AI calls)', async () => { const agentPort = makeMockAgentPort(); // returns 1 record: orders #99 diff --git a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts index d483ce2d37..160a0c6a99 100644 --- a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts @@ -48,6 +48,7 @@ function makeMockAgentPort( function makeCollectionSchema(overrides: Partial = {}): CollectionSchema { return { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -454,6 +455,7 @@ describe('ReadRecordStepExecutor', () => { const ordersSchema = makeCollectionSchema({ collectionName: 'orders', + collectionId: 'col-orders', collectionDisplayName: 'Orders', fields: [{ fieldName: 'total', displayName: 'Total', isRelationship: false }], }); @@ -501,7 +503,19 @@ describe('ReadRecordStepExecutor', () => { const agentPort = makeMockAgentPort({ orders: { values: { total: 150 } }, }); - const context = makeContext({ baseRecordRef, model, runStore, workflowPort, agentPort }); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + baseRecordRef, + model, + runStore, + workflowPort, + agentPort, + activityLogPort, + }); const executor = new ReadRecordStepExecutor(context); const result = await executor.execute(); @@ -519,6 +533,13 @@ describe('ReadRecordStepExecutor', () => { }), }), ); + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'index', + type: 'read', + collectionId: 'col-orders', + recordId: [99], + }); }); it('includes step index in select-record tool schema when records have stepIndex', async () => { diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index 59648d2751..65c441d0a8 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -44,6 +44,7 @@ function makeMockAgentPort(): AgentPort { function makeCollectionSchema(overrides: Partial = {}): CollectionSchema { return { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -188,6 +189,33 @@ describe('TriggerRecordActionStepExecutor', () => { }); }); + describe('operation activity log (PRD-442 #1)', () => { + it('logs the action against the acted record and its collection, not the trigger', async () => { + const agentPort = makeMockAgentPort(); + (agentPort.executeAction as jest.Mock).mockResolvedValue({ ok: true }); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + agentPort, + activityLogPort, + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + await new TriggerRecordActionStepExecutor(context).execute(); + + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'action', + type: 'write', + collectionId: 'col-customers', + recordId: [42], + }); + }); + }); + describe('without executionType=FullyAutomated: awaiting-input (Branch C)', () => { it('saves pendingAction and returns awaiting-input', async () => { const mockModel = makeMockModel({ diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index 48becbae9f..3780a320aa 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -47,6 +47,7 @@ function makeMockAgentPort( function makeCollectionSchema(overrides: Partial = {}): CollectionSchema { return { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -181,6 +182,54 @@ describe('UpdateRecordStepExecutor', () => { }); }); + describe('operation activity log (PRD-442 #1)', () => { + it('logs the update against the acted record and its collection, not the trigger', async () => { + const runStore = makeMockRunStore(); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + model: makeMockModel({ input: { fieldName: 'Status', value: 'active', reasoning: 'r' } }) + .model, + runStore, + activityLogPort, + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + await new UpdateRecordStepExecutor(context).execute(); + + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'update', + type: 'write', + collectionId: 'col-customers', + recordId: [42], + }); + }); + + it('does not log the update while only awaiting confirmation', async () => { + const runStore = makeMockRunStore(); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + model: makeMockModel({ input: { fieldName: 'Status', value: 'active', reasoning: 'r' } }) + .model, + runStore, + activityLogPort, + }); + + const result = await new UpdateRecordStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('awaiting-input'); + expect(activityLogPort.createPending).not.toHaveBeenCalled(); + }); + }); + describe('without executionType=FullyAutomated: awaiting-input (Branch C)', () => { it('saves execution and returns awaiting-input', async () => { const mockModel = makeMockModel({ diff --git a/packages/workflow-executor/test/integration/workflow-execution.test.ts b/packages/workflow-executor/test/integration/workflow-execution.test.ts index 2089b18bb4..08f8b99514 100644 --- a/packages/workflow-executor/test/integration/workflow-execution.test.ts +++ b/packages/workflow-executor/test/integration/workflow-execution.test.ts @@ -36,6 +36,7 @@ const STEP_USER: StepUser = { const COLLECTION_SCHEMA: CollectionSchema = { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -48,6 +49,7 @@ const COLLECTION_SCHEMA: CollectionSchema = { const COLLECTION_SCHEMA_WITH_STATUS: CollectionSchema = { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -59,6 +61,7 @@ const COLLECTION_SCHEMA_WITH_STATUS: CollectionSchema = { const COLLECTION_SCHEMA_WITH_ACTIONS: CollectionSchema = { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [{ fieldName: 'id', displayName: 'Id', isRelationship: false, type: 'Number' }], @@ -69,6 +72,7 @@ const COLLECTION_SCHEMA_WITH_ACTIONS: CollectionSchema = { const COLLECTION_SCHEMA_WITH_RELATION: CollectionSchema = { collectionName: 'customers', + collectionId: 'col-customers', collectionDisplayName: 'Customers', primaryKeyFields: ['id'], fields: [ @@ -86,6 +90,7 @@ const COLLECTION_SCHEMA_WITH_RELATION: CollectionSchema = { const ORDERS_SCHEMA: CollectionSchema = { collectionName: 'orders', + collectionId: 'col-orders', collectionDisplayName: 'Orders', primaryKeyFields: ['id'], fields: [ diff --git a/packages/workflow-executor/test/schema-cache.test.ts b/packages/workflow-executor/test/schema-cache.test.ts index 216721640d..a65c152176 100644 --- a/packages/workflow-executor/test/schema-cache.test.ts +++ b/packages/workflow-executor/test/schema-cache.test.ts @@ -5,6 +5,7 @@ import SchemaCache from '../src/schema-cache'; function makeSchema(collectionName: string): CollectionSchema { return { collectionName, + collectionId: `col-${collectionName}`, collectionDisplayName: collectionName, primaryKeyFields: ['id'], fields: [], From 58e5ec8bd639018e90e21ac011ee7b95edc16483 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 17:46:08 +0200 Subject: [PATCH 02/11] refactor(workflow): extract OperationStepExecutor and migrate MCP to logOperation Move the activity-log machinery out of BaseStepExecutor into a new OperationStepExecutor (Base -> Operation -> Record/Mcp). It owns the `operation` descriptor, `logOperation(record, fn)` and the now-private `withActivityLog` helper, so the base class no longer carries any audit concern. McpStepExecutor now extends OperationStepExecutor and logs its tool call against the run base record via context.collectionId. RecordStepExecutor overrides operationCollectionId to resolve the acted record's own collection id. Relocate the activity-log tests from base-step-executor.test.ts to a new operation-step-executor.test.ts. fixes PRD-442 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/executors/base-step-executor.ts | 62 +----- .../src/executors/mcp-step-executor.ts | 35 ++- .../src/executors/operation-step-executor.ts | 52 +++++ .../src/executors/record-step-executor.ts | 18 +- .../test/executors/base-step-executor.test.ts | 176 --------------- .../test/executors/mcp-step-executor.test.ts | 3 +- .../executors/operation-step-executor.test.ts | 200 ++++++++++++++++++ 7 files changed, 275 insertions(+), 271 deletions(-) create mode 100644 packages/workflow-executor/src/executors/operation-step-executor.ts create mode 100644 packages/workflow-executor/test/executors/operation-step-executor.test.ts diff --git a/packages/workflow-executor/src/executors/base-step-executor.ts b/packages/workflow-executor/src/executors/base-step-executor.ts index c1c46643e4..4dfa94b0b4 100644 --- a/packages/workflow-executor/src/executors/base-step-executor.ts +++ b/packages/workflow-executor/src/executors/base-step-executor.ts @@ -1,4 +1,3 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; import type { AgentPort } from '../ports/agent-port'; import type { ExecutionContext, @@ -50,7 +49,7 @@ export default abstract class BaseStepExecutor { - const args = this.buildActivityLogArgs(); - if (!args) return this.runWithTimeout(); - - const handle = await this.context.activityLogPort.createPending(args); - - let result: StepExecutionResult; - - try { - result = await this.runWithTimeout(); - } catch (err) { - // Use userMessage (not the technical message) — errorMessage is rendered to end-users - // in the Forest Admin UI. Privacy: no collection/field/AI internals in the audit trail. - const errorMessage = - err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; - void this.context.activityLogPort.markFailed(handle, errorMessage); - throw err; - } - - if (result.stepOutcome.status === 'error') { - void this.context.activityLogPort.markFailed( - handle, - result.stepOutcome.error ?? 'Step failed', - ); - } else { - void this.context.activityLogPort.markSucceeded(handle); - } - - return result; - } - - protected async withActivityLog( - args: CreateActivityLogArgs, - operation: () => Promise, - ): Promise { - const handle = await this.context.activityLogPort.createPending(args); - - try { - const result = await operation(); - void this.context.activityLogPort.markSucceeded(handle); - - return result; - } catch (err) { - const errorMessage = - err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; - void this.context.activityLogPort.markFailed(handle, errorMessage); - throw err; - } - } - // Promise.race doesn't abort the losing branch — it keeps running in the background. The .catch() // on execPromise must be attached BEFORE the race so a late rejection doesn't trigger // UnhandledPromiseRejection. Late resolutions are silently discarded. diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index 8d7c157b1a..dff12653f2 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -1,4 +1,3 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; import type { ExecutionContext, StepExecutionResult } from '../types/execution-context'; import type { McpStepExecutionData, McpToolCall } from '../types/step-execution-data'; import type { McpStepDefinition } from '../types/validated/step-definition'; @@ -14,7 +13,7 @@ import { NoMcpToolsError, StepStateError, } from '../errors'; -import BaseStepExecutor from './base-step-executor'; +import OperationStepExecutor from './operation-step-executor'; import { StepExecutionMode } from '../types/validated/step-definition'; const MCP_TASK_SYSTEM_PROMPT = `You are an AI agent selecting and executing a tool to fulfill a user request. @@ -24,7 +23,13 @@ Important rules: - Select only the tool directly relevant to the request. - Final answer is definitive, you won't receive any other input from the user.`; -export default class McpStepExecutor extends BaseStepExecutor { +export default class McpStepExecutor extends OperationStepExecutor { + protected readonly operation: { action: string; type: 'read' | 'write'; label?: string } = { + action: 'action', + type: 'write', + label: this.context.stepDefinition.mcpServerId, + }; + private readonly remoteTools: readonly RemoteTool[]; private readonly mcpServerName?: string; @@ -46,16 +51,6 @@ export default class McpStepExecutor extends BaseStepExecutor }; } - protected override buildActivityLogArgs(): CreateActivityLogArgs | null { - return { - renderingId: this.context.user.renderingId, - action: 'action', - type: 'write', - collectionId: this.context.collectionId, - label: this.context.stepDefinition.mcpServerId, - }; - } - protected buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; @@ -133,13 +128,13 @@ export default class McpStepExecutor extends BaseStepExecutor idempotencyPhase: 'executing', }); - let toolResult: unknown; - - try { - toolResult = await tool.base.invoke(target.input); - } catch (cause) { - throw new McpToolInvocationError(target.name, cause); - } + const toolResult = await this.logOperation(this.context.baseRecordRef, async () => { + try { + return await tool.base.invoke(target.input); + } catch (cause) { + throw new McpToolInvocationError(target.name, cause); + } + }); // 1. Persist raw result immediately — safe state before any further network calls const baseExecutionResult = { success: true as const, toolResult }; diff --git a/packages/workflow-executor/src/executors/operation-step-executor.ts b/packages/workflow-executor/src/executors/operation-step-executor.ts new file mode 100644 index 0000000000..09644b0833 --- /dev/null +++ b/packages/workflow-executor/src/executors/operation-step-executor.ts @@ -0,0 +1,52 @@ +import type { CreateActivityLogArgs } from '../ports/activity-log-port'; +import type { RecordRef } from '../types/validated/collection'; +import type { StepDefinition } from '../types/validated/step-definition'; + +import { WorkflowExecutorError } from '../errors'; +import BaseStepExecutor from './base-step-executor'; + +export default abstract class OperationStepExecutor< + TStep extends StepDefinition = StepDefinition, +> extends BaseStepExecutor { + protected abstract readonly operation: { action: string; type: 'read' | 'write'; label?: string }; + + // Defaults to the run's trigger collection; RecordStepExecutor overrides it to resolve the + // acted record's own collection (which may differ from the trigger). + // eslint-disable-next-line @typescript-eslint/no-unused-vars + protected operationCollectionId(record: RecordRef): Promise { + return Promise.resolve(this.context.collectionId); + } + + protected async logOperation(record: RecordRef, run: () => Promise): Promise { + const collectionId = await this.operationCollectionId(record); + + return this.withActivityLog( + { + renderingId: this.context.user.renderingId, + ...this.operation, + collectionId, + recordId: record.recordId, + }, + run, + ); + } + + private async withActivityLog( + args: CreateActivityLogArgs, + operation: () => Promise, + ): Promise { + const handle = await this.context.activityLogPort.createPending(args); + + try { + const result = await operation(); + void this.context.activityLogPort.markSucceeded(handle); + + return result; + } catch (err) { + const errorMessage = + err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; + void this.context.activityLogPort.markFailed(handle, errorMessage); + throw err; + } + } +} diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index 32111472aa..461fe61bb8 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -7,25 +7,15 @@ import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin import { z } from 'zod'; import { InvalidAIResponseError, InvalidPreRecordedArgsError, NoRecordsError } from '../errors'; -import BaseStepExecutor from './base-step-executor'; +import OperationStepExecutor from './operation-step-executor'; export default abstract class RecordStepExecutor< TStep extends StepDefinition = StepDefinition, -> extends BaseStepExecutor { - protected abstract readonly operation: { action: string; type: 'read' | 'write' }; - - protected async logOperation(record: RecordRef, run: () => Promise): Promise { +> extends OperationStepExecutor { + protected override async operationCollectionId(record: RecordRef): Promise { const { collectionId } = await this.getCollectionSchema(record.collectionName); - return this.withActivityLog( - { - renderingId: this.context.user.renderingId, - ...this.operation, - collectionId, - recordId: record.recordId, - }, - run, - ); + return collectionId; } protected buildOutcomeResult(outcome: { diff --git a/packages/workflow-executor/test/executors/base-step-executor.test.ts b/packages/workflow-executor/test/executors/base-step-executor.test.ts index bef89c9760..8f1479afe2 100644 --- a/packages/workflow-executor/test/executors/base-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/base-step-executor.test.ts @@ -18,7 +18,6 @@ import { NoRecordsError, RunStorePortError, StepStateError, - WorkflowExecutorError, } from '../../src/errors'; import BaseStepExecutor from '../../src/executors/base-step-executor'; import SchemaCache from '../../src/schema-cache'; @@ -530,181 +529,6 @@ describe('BaseStepExecutor', () => { }); }); - describe('activity log lifecycle', () => { - class LoggedExecutor extends BaseStepExecutor { - constructor(context: ExecutionContext, private readonly errorToThrow?: unknown) { - super(context); - } - - protected override buildActivityLogArgs() { - return { - renderingId: 1, - action: 'update', - type: 'write' as const, - collectionId: 'col-1', - recordId: [42], - }; - } - - protected async doExecute(): Promise { - if (this.errorToThrow !== undefined) throw this.errorToThrow; - - return this.buildOutcomeResult({ status: 'success' }); - } - - protected buildOutcomeResult(outcome: { - status: BaseStepStatus; - error?: string; - }): StepExecutionResult { - return { - stepOutcome: { - type: 'record', - stepId: this.context.stepId, - stepIndex: this.context.stepIndex, - status: outcome.status, - ...(outcome.error !== undefined && { error: outcome.error }), - }, - }; - } - } - - it('creates pending log, runs doExecute, then marks succeeded on success', async () => { - const context = makeContext(); - const executor = new LoggedExecutor(context); - - const result = await executor.execute(); - - expect(result.stepOutcome.status).toBe('success'); - expect(context.activityLogPort.createPending).toHaveBeenCalledWith( - expect.objectContaining({ - action: 'update', - type: 'write', - collectionId: 'col-1', - }), - ); - expect(context.activityLogPort.markSucceeded).toHaveBeenCalledWith({ - id: 'log-1', - index: '0', - }); - expect(context.activityLogPort.markFailed).not.toHaveBeenCalled(); - }); - - it('marks failed when doExecute throws a WorkflowExecutorError', async () => { - const context = makeContext(); - const executor = new LoggedExecutor(context, new NoRecordsError()); - - await executor.execute(); - - expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'No records available', - ); - expect(context.activityLogPort.markSucceeded).not.toHaveBeenCalled(); - }); - - it('fails the step and does NOT run doExecute when createPending throws ActivityLogCreationError', async () => { - // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require - const { ActivityLogCreationError } = require('../../src/errors'); - const context = makeContext(); - (context.activityLogPort.createPending as jest.Mock).mockRejectedValue( - new ActivityLogCreationError(new Error('net')), - ); - const doExecuteSpy = jest.fn().mockResolvedValue({ - stepOutcome: { type: 'record', stepId: 'x', stepIndex: 0, status: 'success' }, - }); - - class NeverRunExecutor extends LoggedExecutor { - protected override async doExecute(): Promise { - return doExecuteSpy(); - } - } - - const executor = new NeverRunExecutor(context); - const result = await executor.execute(); - - expect(result.stepOutcome.status).toBe('error'); - expect(result.stepOutcome.error).toBe( - 'Could not record this step in the audit log. Please try again, or contact your administrator if the problem persists.', - ); - expect(doExecuteSpy).not.toHaveBeenCalled(); - }); - - it('does NOT create pending log when buildActivityLogArgs returns null (default)', async () => { - const context = makeContext(); - const executor = new TestableExecutor(context); - - await executor.execute(); - - expect(context.activityLogPort.createPending).not.toHaveBeenCalled(); - expect(context.activityLogPort.markSucceeded).not.toHaveBeenCalled(); - }); - - it('calls markFailed with userMessage (not the technical message) on WorkflowExecutorError', async () => { - class DualMessageError extends WorkflowExecutorError { - constructor() { - super( - 'Internal: datasource "customers" returned no record for pk=42', - 'The record no longer exists.', - ); - } - } - const context = makeContext(); - const executor = new LoggedExecutor(context, new DualMessageError()); - - await executor.execute(); - - expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'The record no longer exists.', - ); - }); - - it('marks failed when doExecute returns an error outcome without throwing', async () => { - class ErrorOutcomeExecutor extends BaseStepExecutor { - protected override buildActivityLogArgs() { - return { - renderingId: 1, - action: 'update', - type: 'write' as const, - collectionName: 'customers', - recordId: [42], - }; - } - - protected async doExecute(): Promise { - return this.buildOutcomeResult({ status: 'error', error: 'soft failure' }); - } - - protected buildOutcomeResult(outcome: { - status: BaseStepStatus; - error?: string; - }): StepExecutionResult { - return { - stepOutcome: { - type: 'record', - stepId: this.context.stepId, - stepIndex: this.context.stepIndex, - status: outcome.status, - ...(outcome.error !== undefined && { error: outcome.error }), - }, - }; - } - } - - const context = makeContext(); - const executor = new ErrorOutcomeExecutor(context); - - const result = await executor.execute(); - - expect(result.stepOutcome.status).toBe('error'); - expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'soft failure', - ); - expect(context.activityLogPort.markSucceeded).not.toHaveBeenCalled(); - }); - }); - describe('invokeWithTool', () => { function makeMockModel(response: unknown) { const invoke = jest.fn().mockResolvedValue(response); diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index b057b6394f..61733e633a 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -951,7 +951,7 @@ describe('McpStepExecutor', () => { }); describe('activity log', () => { - it('creates activity log with collectionId, renderingId, action, type and mcpServerId as label', async () => { + it('logs against the run base record with collectionId, renderingId, action, type and mcpServerId as label', async () => { const tool = new MockRemoteTool({ name: 'send_notification', sourceId: 'mcp-server-1' }); const { model } = makeMockModel('send_notification', { message: 'Hello' }); const activityLogPort = { @@ -977,6 +977,7 @@ describe('McpStepExecutor', () => { action: 'action', type: 'write', collectionId: 'col-1', + recordId: [42], label: 'my-mcp-server', }); }); diff --git a/packages/workflow-executor/test/executors/operation-step-executor.test.ts b/packages/workflow-executor/test/executors/operation-step-executor.test.ts new file mode 100644 index 0000000000..2d459a4812 --- /dev/null +++ b/packages/workflow-executor/test/executors/operation-step-executor.test.ts @@ -0,0 +1,200 @@ +/* eslint-disable max-classes-per-file */ +import type { Logger } from '../../src/ports/logger-port'; +import type { RunStore } from '../../src/ports/run-store'; +import type { ExecutionContext, StepExecutionResult } from '../../src/types/execution-context'; +import type { RecordRef } from '../../src/types/validated/collection'; +import type { BaseStepStatus } from '../../src/types/validated/step-outcome'; + +import { ActivityLogCreationError, NoRecordsError, WorkflowExecutorError } from '../../src/errors'; +import OperationStepExecutor from '../../src/executors/operation-step-executor'; +import SchemaCache from '../../src/schema-cache'; +import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; + +class TestOperationExecutor extends OperationStepExecutor { + protected readonly operation = { action: 'update', type: 'write' as const }; + + constructor( + context: ExecutionContext, + private readonly run: () => Promise = () => Promise.resolve('ok'), + ) { + super(context); + } + + protected async doExecute(): Promise { + await this.logOperation(this.context.baseRecordRef, this.run); + + return this.buildOutcomeResult({ status: 'success' }); + } + + protected buildOutcomeResult(outcome: { + status: BaseStepStatus; + error?: string; + }): StepExecutionResult { + return { + stepOutcome: { + type: 'record', + stepId: this.context.stepId, + stepIndex: this.context.stepIndex, + status: outcome.status, + ...(outcome.error !== undefined && { error: outcome.error }), + }, + }; + } +} + +function makeMockRunStore(): RunStore { + return { + init: jest.fn().mockResolvedValue(undefined), + close: jest.fn().mockResolvedValue(undefined), + getStepExecutions: jest.fn().mockResolvedValue([]), + saveStepExecution: jest.fn().mockResolvedValue(undefined), + }; +} + +function makeMockLogger(): Logger { + return { info: jest.fn(), warn: jest.fn(), error: jest.fn() }; +} + +function makeMockActivityLogPort(): ExecutionContext['activityLogPort'] { + return { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; +} + +function makeContext(overrides: Partial = {}): ExecutionContext { + return { + runId: 'run-1', + stepId: 'step-0', + stepIndex: 0, + collectionId: 'col-1', + baseRecordRef: { + collectionName: 'customers', + recordId: [42], + stepIndex: 0, + } as RecordRef, + stepDefinition: { + type: StepType.UpdateRecord, + executionType: StepExecutionMode.FullyAutomated, + prompt: 'Update it', + }, + model: {} as ExecutionContext['model'], + agentPort: {} as ExecutionContext['agentPort'], + workflowPort: {} as ExecutionContext['workflowPort'], + runStore: makeMockRunStore(), + user: { + id: 1, + email: 'test@example.com', + firstName: 'Test', + lastName: 'User', + team: 'admin', + renderingId: 1, + role: 'admin', + permissionLevel: 'admin', + tags: {}, + }, + schemaCache: new SchemaCache(), + previousSteps: [], + logger: makeMockLogger(), + activityLogPort: makeMockActivityLogPort(), + ...overrides, + }; +} + +describe('OperationStepExecutor', () => { + describe('logOperation', () => { + it('logs the operation against the given record and trigger collection, then marks succeeded', async () => { + const context = makeContext(); + const executor = new TestOperationExecutor(context); + + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect(context.activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'update', + type: 'write', + collectionId: 'col-1', + recordId: [42], + }); + expect(context.activityLogPort.markSucceeded).toHaveBeenCalledWith({ + id: 'log-1', + index: '0', + }); + expect(context.activityLogPort.markFailed).not.toHaveBeenCalled(); + }); + + it('marks failed with the userMessage when the operation throws a WorkflowExecutorError', async () => { + const context = makeContext(); + const executor = new TestOperationExecutor(context, () => + Promise.reject(new NoRecordsError()), + ); + + await executor.execute(); + + expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( + { id: 'log-1', index: '0' }, + 'No records available', + ); + expect(context.activityLogPort.markSucceeded).not.toHaveBeenCalled(); + }); + + it('marks failed with the userMessage (not the technical message) on a dual-message error', async () => { + class DualMessageError extends WorkflowExecutorError { + constructor() { + super( + 'Internal: datasource "customers" returned no record for pk=42', + 'The record no longer exists.', + ); + } + } + const context = makeContext(); + const executor = new TestOperationExecutor(context, () => + Promise.reject(new DualMessageError()), + ); + + await executor.execute(); + + expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( + { id: 'log-1', index: '0' }, + 'The record no longer exists.', + ); + }); + + it('does NOT run the operation and propagates when createPending throws', async () => { + const context = makeContext(); + (context.activityLogPort.createPending as jest.Mock).mockRejectedValue( + new ActivityLogCreationError(new Error('net')), + ); + const operation = jest.fn().mockResolvedValue('ok'); + + const executor = new TestOperationExecutor(context, operation); + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toBe( + 'Could not record this step in the audit log. Please try again, or contact your administrator if the problem persists.', + ); + expect(operation).not.toHaveBeenCalled(); + }); + }); + + describe('operationCollectionId', () => { + it('targets the collection resolved by the override instead of the trigger collection', async () => { + class OverridingExecutor extends TestOperationExecutor { + protected override operationCollectionId(): Promise { + return Promise.resolve('col-orders'); + } + } + const context = makeContext(); + const executor = new OverridingExecutor(context); + + await executor.execute(); + + expect(context.activityLogPort.createPending).toHaveBeenCalledWith( + expect.objectContaining({ collectionId: 'col-orders', recordId: [42] }), + ); + }); + }); +}); From 77bc7f0467051813c748de789fe3cca09e399488 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 18:28:45 +0200 Subject: [PATCH 03/11] refactor(workflow): address review findings on activity-log operation - Tighten operation typing: CreateActivityLogArgs now uses the lib's ActivityLogAction/ActivityLogType (removes the `as ActivityLogAction` cast); extract OperationDescriptor (Pick) reused by the MCP executor. collectionId is now required on CreateActivityLogArgs. - Drop the dead `errorMessage` param from ActivityLogPort.markFailed: the server status endpoint only accepts { status }, so the message went nowhere. The failure cause is still logged by BaseStepExecutor. - Move the `executing` write-ahead marker inside the logOperation callback (after createPending, before the side effect) in update/trigger/mcp so an activity-log creation failure never leaves an orphan executing marker. - Fix stale comment in base-step-executor + CLAUDE.md idempotency section. - Reword the misleading MCP formatting-fallback log line. - Tests: cross-collection activity-log target for update + trigger, log on the load-related awaiting-input branch, no-orphan-marker on createPending failure, and markFailed single-arg assertions. fixes PRD-442 Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/workflow-executor/CLAUDE.md | 2 +- .../forestadmin-client-activity-log-port.ts | 10 +- .../src/executors/base-step-executor.ts | 5 +- .../src/executors/mcp-step-executor.ts | 33 +++--- .../src/executors/operation-step-executor.ts | 9 +- .../trigger-record-action-step-executor.ts | 22 ++-- .../executors/update-record-step-executor.ts | 22 ++-- .../src/ports/activity-log-port.ts | 12 +- ...n-client-activity-log-port-factory.test.ts | 7 +- ...restadmin-client-activity-log-port.test.ts | 67 ++++++++--- .../load-related-record-step-executor.test.ts | 24 ++++ .../test/executors/mcp-step-executor.test.ts | 2 +- .../executors/operation-step-executor.test.ts | 33 +----- ...rigger-record-action-step-executor.test.ts | 84 ++++++++++++++ .../update-record-step-executor.test.ts | 108 +++++++++++++++++- 15 files changed, 332 insertions(+), 108 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 9a3dabe201..00a373ea6a 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -88,7 +88,7 @@ src/ - **Boundary errors** (`extends Error`) — Thrown outside step execution, at the HTTP or Runner layer (e.g. `RunNotFoundError`, `PendingDataNotFoundError`, `ConfigurationError`). Caught by the HTTP server and translated into HTTP status codes (404, 400, etc.). These intentionally do NOT extend `WorkflowExecutorError` to prevent `base-step-executor` from catching them as step failures. - **Dual error messages** — `WorkflowExecutorError` carries two messages: `message` (technical, for dev logs) and `userMessage` (human-readable, surfaced to the Forest Admin UI via `stepOutcome.error`). The mapping happens in a single place: `base-step-executor.ts` uses `error.userMessage` when building the error outcome. When adding a new error subclass, always provide a distinct `userMessage` oriented toward end-users (no collection names, field names, or AI internals). If `userMessage` is omitted in the constructor call, it falls back to `message`. - **displayName in AI tools** — All `DynamicStructuredTool` schemas and system message prompts must use `displayName`, never `fieldName`. `displayName` is a Forest Admin frontend feature that replaces the technical field/relation/action name with a product-oriented label configured by the Forest Admin admin. End users write their workflow prompts using these display names, not the underlying technical names. After an AI tool call returns display names, map them back to `fieldName`/`name` before using them in datasource operations (e.g. filtering record values, calling `getRecord`). -- **Idempotency in mutating executors** — `update-record`, `trigger-action`, and `mcp` executors protect against duplicate side effects via a write-ahead log in the `RunStore`. Before the side effect fires, the executor saves `idempotencyPhase: 'executing'`. After, it saves `idempotencyPhase: 'done'` alongside the normal `executionResult`. On re-dispatch (same `runId + stepIndex`): `done` → reconstruct success outcome via `buildOutcomeResult` without re-executing or emitting an activity log; `executing` → throw `StepStateError` (user retries manually, also no activity log). The `checkIdempotency()` hook in `BaseStepExecutor` is called before `runWithActivityLog()` so neither cache hits nor uncertain-state errors emit activity log entries. Non-mutating executors (`condition`, `read-record`, `guidance`, `load-related-record`) do not override `checkIdempotency()` — replaying them is safe. +- **Idempotency in mutating executors** — `update-record`, `trigger-action`, and `mcp` executors protect against duplicate side effects via a write-ahead log in the `RunStore`. Before the side effect fires, the executor saves `idempotencyPhase: 'executing'`. After, it saves `idempotencyPhase: 'done'` alongside the normal `executionResult`. On re-dispatch (same `runId + stepIndex`): `done` → reconstruct success outcome via `buildOutcomeResult` without re-executing or emitting an activity log; `executing` → throw `StepStateError` (user retries manually, also no activity log). The `checkIdempotency()` hook in `BaseStepExecutor` runs before `doExecute()` so neither cache hits nor uncertain-state errors reach the activity log emitted inside `OperationStepExecutor.logOperation`. The `executing` write-ahead marker is saved inside the `logOperation` callback (after `createPending`, just before the side effect) so an activity-log creation failure never leaves an orphan `executing` marker. Non-mutating executors (`condition`, `read-record`, `guidance`, `load-related-record`) do not override `checkIdempotency()` — replaying them is safe. - **Fetched steps must be executed** — Any step retrieved from the orchestrator via `getAvailableRuns()` must be executed. Silently discarding a fetched step (e.g. filtering it out by `runId` after fetching) violates the executor contract: the orchestrator assumes execution is guaranteed once the step is dispatched. The only valid filter before executing is deduplication via `inFlightRuns` (keyed by `runId`, to avoid running the same run twice concurrently; the key is the run, not the step, because a chain advances the `stepId` between iterations). - **Auto-chain from `/update-step` response** — `WorkflowPort.updateStepExecution` returns `AvailableRunDispatch | null`: when non-null, the `Runner` executes the next step inline instead of waiting for the next poll. The chain exits on `null` (awaiting-input / finished / error), on a non-progressing `stepIndex` (server bug defense), at `maxChainDepth` (config, default 50), or when `stop()` is called. Each chained step uses the `forestServerToken` from its own dispatch — token freshness is preserved across the chain. The port retries `POST /update-step` on transient failures (network, 5xx) — this relies on server-side idempotency: the orchestrator MUST deduplicate identical outcomes for a given `(runId, stepIndex)` to prevent double side-effects on retry. - **Pre-recorded AI decisions** — Record step executors support `preRecordedArgs` in the step definition to bypass AI calls. When provided, executors use the pre-recorded values (display names) directly instead of invoking the AI. Each record step type has its own typed `preRecordedArgs` shape. Validation happens via schema resolution — invalid display names throw `InvalidPreRecordedArgsError`. Partial args are supported: only the provided fields skip AI, the rest still use AI. diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index f72ed8089e..feb270f96f 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -5,10 +5,7 @@ import type { CreateActivityLogArgs, } from '../ports/activity-log-port'; import type { Logger } from '../ports/logger-port'; -import type { - ActivityLogAction, - ActivityLogsServiceInterface, -} from '@forestadmin/forestadmin-client'; +import type { ActivityLogsServiceInterface } from '@forestadmin/forestadmin-client'; import { serializeRecordId } from './record-id-serializer'; import withRetry from './with-retry'; @@ -30,7 +27,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort this.service.createActivityLog({ forestServerToken: this.forestServerToken, renderingId: String(args.renderingId), - action: args.action as ActivityLogAction, + action: args.action, type: args.type, // The lib writes this value verbatim into relationships.collection.data.id // (JSON:API). The Forest server audit-trail API expects the numeric collectionId. @@ -76,7 +73,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort }); } - async markFailed(handle: ActivityLogHandle, errorMessage: string): Promise { + async markFailed(handle: ActivityLogHandle): Promise { return this.drainer.track(async () => { try { await withRetry( @@ -92,7 +89,6 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } catch (err) { this.logger.error('activity log mark-as-failed failed', { handleId: handle.id, - stepErrorMessage: errorMessage, error: extractErrorMessage(err), }); } diff --git a/packages/workflow-executor/src/executors/base-step-executor.ts b/packages/workflow-executor/src/executors/base-step-executor.ts index 4dfa94b0b4..329d2cc579 100644 --- a/packages/workflow-executor/src/executors/base-step-executor.ts +++ b/packages/workflow-executor/src/executors/base-step-executor.ts @@ -49,8 +49,9 @@ export default abstract class BaseStepExecutor { - protected readonly operation: { action: string; type: 'read' | 'write'; label?: string } = { + protected readonly operation: OperationDescriptor = { action: 'action', type: 'write', label: this.context.stepDefinition.mcpServerId, @@ -121,14 +121,14 @@ export default class McpStepExecutor extends OperationStepExecutor t.base.name === target.name && t.sourceId === target.sourceId); if (!tool) throw new McpToolNotFoundError(target.name); - await this.context.runStore.saveStepExecution(this.context.runId, { - ...existingExecution, - type: 'mcp', - stepIndex: this.context.stepIndex, - idempotencyPhase: 'executing', - }); - const toolResult = await this.logOperation(this.context.baseRecordRef, async () => { + await this.context.runStore.saveStepExecution(this.context.runId, { + ...existingExecution, + type: 'mcp', + stepIndex: this.context.stepIndex, + idempotencyPhase: 'executing', + }); + try { return await tool.base.invoke(target.input); } catch (cause) { @@ -155,12 +155,15 @@ export default class McpStepExecutor extends OperationStepExecutor; + export default abstract class OperationStepExecutor< TStep extends StepDefinition = StepDefinition, > extends BaseStepExecutor { - protected abstract readonly operation: { action: string; type: 'read' | 'write'; label?: string }; + protected abstract readonly operation: OperationDescriptor; // Defaults to the run's trigger collection; RecordStepExecutor overrides it to resolve the // acted record's own collection (which may differ from the trigger). @@ -43,9 +44,7 @@ export default abstract class OperationStepExecutor< return result; } catch (err) { - const errorMessage = - err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; - void this.context.activityLogPort.markFailed(handle, errorMessage); + void this.context.activityLogPort.markFailed(handle); throw err; } } diff --git a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts index 7241e4e86b..f85aad8df8 100644 --- a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts +++ b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts @@ -131,23 +131,23 @@ export default class TriggerRecordActionStepExecutor extends RecordStepExecutor< private async executeOnExecutor(target: ActionTarget): Promise { const { selectedRecordRef, displayName, name } = target; - await this.context.runStore.saveStepExecution(this.context.runId, { - type: 'trigger-action', - stepIndex: this.context.stepIndex, - selectedRecordRef, - idempotencyPhase: 'executing', - }); - - const actionResult = await this.logOperation(selectedRecordRef, () => - this.agentPort.executeAction( + const actionResult = await this.logOperation(selectedRecordRef, async () => { + await this.context.runStore.saveStepExecution(this.context.runId, { + type: 'trigger-action', + stepIndex: this.context.stepIndex, + selectedRecordRef, + idempotencyPhase: 'executing', + }); + + return this.agentPort.executeAction( { collection: selectedRecordRef.collectionName, action: name, id: selectedRecordRef.recordId, }, this.context.user, - ), - ); + ); + }); await this.context.runStore.saveStepExecution(this.context.runId, { type: 'trigger-action', diff --git a/packages/workflow-executor/src/executors/update-record-step-executor.ts b/packages/workflow-executor/src/executors/update-record-step-executor.ts index 4f08f61f9b..2256bbeb45 100644 --- a/packages/workflow-executor/src/executors/update-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/update-record-step-executor.ts @@ -250,24 +250,24 @@ export default class UpdateRecordStepExecutor extends RecordStepExecutor { const { selectedRecordRef, displayName, name, value } = target; - await this.context.runStore.saveStepExecution(this.context.runId, { - ...existingExecution, - type: 'update-record', - stepIndex: this.context.stepIndex, - selectedRecordRef, - idempotencyPhase: 'executing', - }); + const updated = await this.logOperation(selectedRecordRef, async () => { + await this.context.runStore.saveStepExecution(this.context.runId, { + ...existingExecution, + type: 'update-record', + stepIndex: this.context.stepIndex, + selectedRecordRef, + idempotencyPhase: 'executing', + }); - const updated = await this.logOperation(selectedRecordRef, () => - this.agentPort.updateRecord( + return this.agentPort.updateRecord( { collection: selectedRecordRef.collectionName, id: selectedRecordRef.recordId, values: { [name]: value }, }, this.context.user, - ), - ); + ); + }); await this.context.runStore.saveStepExecution(this.context.runId, { ...existingExecution, diff --git a/packages/workflow-executor/src/ports/activity-log-port.ts b/packages/workflow-executor/src/ports/activity-log-port.ts index d12ab9ce63..4116ecb84e 100644 --- a/packages/workflow-executor/src/ports/activity-log-port.ts +++ b/packages/workflow-executor/src/ports/activity-log-port.ts @@ -1,10 +1,12 @@ import type { RecordId } from '../types/validated/collection'; +import type { ActivityLogAction, ActivityLogType } from '@forestadmin/forestadmin-client'; export interface CreateActivityLogArgs { renderingId: number; - action: string; - type: 'read' | 'write'; - collectionId?: string; + action: ActivityLogAction; + type: ActivityLogType; + // Numeric Forest collection id; the adapter forwards it to the lib's `collectionName` param. + collectionId: string; recordId?: RecordId; label?: string; } @@ -15,11 +17,11 @@ export interface ActivityLogHandle { } // Per-run scoped port: token baked into the adapter's constructor. markSucceeded/markFailed -// retry transient failures internally and are invoked with `void` from base-step-executor. +// retry transient failures internally and are invoked with `void` from OperationStepExecutor. export interface ActivityLogPort { createPending(args: CreateActivityLogArgs): Promise; markSucceeded(handle: ActivityLogHandle): Promise; - markFailed(handle: ActivityLogHandle, errorMessage: string): Promise; + markFailed(handle: ActivityLogHandle): Promise; } // Produces per-run ActivityLogPort instances and exposes drain() at the process level so the diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port-factory.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port-factory.test.ts index fd5a89e465..3ad407023f 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port-factory.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port-factory.test.ts @@ -21,7 +21,12 @@ describe('ForestadminClientActivityLogPortFactory', () => { const factory = new ForestadminClientActivityLogPortFactory(service, makeLogger()); const port = factory.forRun('token-42'); - await port.createPending({ renderingId: 1, action: 'update', type: 'write' }); + await port.createPending({ + renderingId: 1, + action: 'update', + type: 'write', + collectionId: 'col-1', + }); expect(port).toBeInstanceOf(ForestadminClientActivityLogPort); expect(service.createActivityLog).toHaveBeenCalledWith( diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 428206a2d9..1b8287a992 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -54,7 +54,12 @@ describe('ForestadminClientActivityLogPort', () => { }); const port = makePort(service); - const handle = await port.createPending({ renderingId: 5, action: 'update', type: 'write' }); + const handle = await port.createPending({ + renderingId: 5, + action: 'update', + type: 'write', + collectionId: 'col-1', + }); expect(handle).toEqual({ id: 'log-1', index: '0' }); expect(service.createActivityLog).toHaveBeenCalledWith( @@ -89,7 +94,12 @@ describe('ForestadminClientActivityLogPort', () => { const logger = makeLogger(); const port = makePort(service, { logger }); - const promise = port.createPending({ renderingId: 5, action: 'update', type: 'write' }); + const promise = port.createPending({ + renderingId: 5, + action: 'update', + type: 'write', + collectionId: 'col-1', + }); await jest.advanceTimersByTimeAsync(100); const handle = await promise; @@ -106,7 +116,12 @@ describe('ForestadminClientActivityLogPort', () => { service.createActivityLog.mockRejectedValue(makeHttpError(502)); const port = makePort(service); - const promise = port.createPending({ renderingId: 5, action: 'update', type: 'write' }); + const promise = port.createPending({ + renderingId: 5, + action: 'update', + type: 'write', + collectionId: 'col-1', + }); const settled = promise.catch(err => err); await jest.advanceTimersByTimeAsync(2_600); const err = await settled; @@ -121,7 +136,12 @@ describe('ForestadminClientActivityLogPort', () => { const port = makePort(service); await expect( - port.createPending({ renderingId: 5, action: 'update', type: 'write' }), + port.createPending({ + renderingId: 5, + action: 'update', + type: 'write', + collectionId: 'col-1', + }), ).rejects.toBeInstanceOf(ActivityLogCreationError); expect(service.createActivityLog).toHaveBeenCalledTimes(1); }); @@ -132,7 +152,12 @@ describe('ForestadminClientActivityLogPort', () => { const port = makePort(service); await expect( - port.createPending({ renderingId: 5, action: 'update', type: 'write' }), + port.createPending({ + renderingId: 5, + action: 'update', + type: 'write', + collectionId: 'col-1', + }), ).rejects.toBeInstanceOf(ActivityLogCreationError); expect(service.createActivityLog).toHaveBeenCalledTimes(1); }); @@ -145,7 +170,12 @@ describe('ForestadminClientActivityLogPort', () => { .mockResolvedValueOnce({ id: 'log-3', attributes: { index: '2' } }); const port = makePort(service); - const promise = port.createPending({ renderingId: 5, action: 'update', type: 'write' }); + const promise = port.createPending({ + renderingId: 5, + action: 'update', + type: 'write', + collectionId: 'col-1', + }); await jest.advanceTimersByTimeAsync(100); await expect(promise).resolves.toEqual({ id: 'log-3', index: '2' }); }); @@ -158,7 +188,12 @@ describe('ForestadminClientActivityLogPort', () => { }); const port = makePort(service); - await port.createPending({ renderingId: 42, action: 'update', type: 'write' }); + await port.createPending({ + renderingId: 42, + action: 'update', + type: 'write', + collectionId: 'col-1', + }); expect(service.createActivityLog).toHaveBeenCalledWith( expect.objectContaining({ renderingId: '42' }), @@ -239,7 +274,7 @@ describe('ForestadminClientActivityLogPort', () => { .mockResolvedValueOnce(undefined); const port = makePort(service); - const promise = port.markFailed({ id: 'log-1', index: '0' }, 'boom'); + const promise = port.markFailed({ id: 'log-1', index: '0' }); await jest.advanceTimersByTimeAsync(100); await promise; @@ -249,26 +284,20 @@ describe('ForestadminClientActivityLogPort', () => { forestServerToken: 'tok', }), ); - expect(service.updateActivityLogStatus).toHaveBeenLastCalledWith( - expect.not.objectContaining({ errorMessage: expect.anything() }), - ); }); - it('swallows errors after retries are exhausted (fire-and-forget) and logs with stepErrorMessage', async () => { + it('swallows errors after retries are exhausted (fire-and-forget) and logs the handle', async () => { const service = makeService(); service.updateActivityLogStatus.mockRejectedValue(makeHttpError(503)); const logger = makeLogger(); const port = makePort(service, { logger }); - const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step-error-msg'); + const promise = port.markFailed({ id: 'log-1', index: '0' }); await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( 'activity log mark-as-failed failed', - expect.objectContaining({ - handleId: 'log-1', - stepErrorMessage: 'step-error-msg', - }), + expect.objectContaining({ handleId: 'log-1' }), ); }); @@ -279,7 +308,7 @@ describe('ForestadminClientActivityLogPort', () => { .mockResolvedValueOnce(undefined); const port = makePort(service); - const promise = port.markFailed({ id: 'log-1', index: '0' }, 'boom'); + const promise = port.markFailed({ id: 'log-1', index: '0' }); await jest.advanceTimersByTimeAsync(100); await expect(promise).resolves.toBeUndefined(); expect(service.updateActivityLogStatus).toHaveBeenCalledTimes(2); @@ -327,7 +356,7 @@ describe('ForestadminClientActivityLogPort', () => { const drainer = new ActivityLogDrainer(); const port = makePort(service, { drainer }); - const markPromise = port.markFailed({ id: 'log-1', index: '0' }, 'boom'); + const markPromise = port.markFailed({ id: 'log-1', index: '0' }); let drainResolved = false; const drainPromise = drainer.drain().then(() => { diff --git a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts index 0b44f7671a..66ed147ee8 100644 --- a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts @@ -751,6 +751,30 @@ describe('LoadRelatedRecordStepExecutor', () => { recordId: [42], }); }); + + it('logs the relation read once on the awaiting-input (Branch C) path', async () => { + const runStore = makeMockRunStore(); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const { model } = makeMockModel({ relationName: 'Order', reasoning: 'r' }); + const context = makeContext({ model, runStore, activityLogPort }); + + const result = await new LoadRelatedRecordStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('awaiting-input'); + expect(activityLogPort.createPending).toHaveBeenCalledTimes(1); + expect(activityLogPort.createPending).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'listRelatedData', + type: 'read', + collectionId: 'col-customers', + recordId: [42], + }), + ); + }); }); describe('without executionType=FullyAutomated: awaiting-input (Branch C)', () => { diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index 61733e633a..b684186621 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -267,7 +267,7 @@ describe('McpStepExecutor', () => { }), ); expect(logger.error).toHaveBeenCalledWith( - 'Failed to format MCP tool result, using generic fallback', + 'Failed to format MCP tool result, persisting raw result without summary', expect.objectContaining({ toolName: 'send_notification' }), ); }); diff --git a/packages/workflow-executor/test/executors/operation-step-executor.test.ts b/packages/workflow-executor/test/executors/operation-step-executor.test.ts index 2d459a4812..99cb4c4f1a 100644 --- a/packages/workflow-executor/test/executors/operation-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/operation-step-executor.test.ts @@ -5,13 +5,13 @@ import type { ExecutionContext, StepExecutionResult } from '../../src/types/exec import type { RecordRef } from '../../src/types/validated/collection'; import type { BaseStepStatus } from '../../src/types/validated/step-outcome'; -import { ActivityLogCreationError, NoRecordsError, WorkflowExecutorError } from '../../src/errors'; +import { ActivityLogCreationError, NoRecordsError } from '../../src/errors'; import OperationStepExecutor from '../../src/executors/operation-step-executor'; import SchemaCache from '../../src/schema-cache'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; class TestOperationExecutor extends OperationStepExecutor { - protected readonly operation = { action: 'update', type: 'write' as const }; + protected readonly operation = { action: 'update', type: 'write' } as const; constructor( context: ExecutionContext, @@ -125,7 +125,7 @@ describe('OperationStepExecutor', () => { expect(context.activityLogPort.markFailed).not.toHaveBeenCalled(); }); - it('marks failed with the userMessage when the operation throws a WorkflowExecutorError', async () => { + it('marks the log failed (by handle) when the operation throws', async () => { const context = makeContext(); const executor = new TestOperationExecutor(context, () => Promise.reject(new NoRecordsError()), @@ -133,35 +133,10 @@ describe('OperationStepExecutor', () => { await executor.execute(); - expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'No records available', - ); + expect(context.activityLogPort.markFailed).toHaveBeenCalledWith({ id: 'log-1', index: '0' }); expect(context.activityLogPort.markSucceeded).not.toHaveBeenCalled(); }); - it('marks failed with the userMessage (not the technical message) on a dual-message error', async () => { - class DualMessageError extends WorkflowExecutorError { - constructor() { - super( - 'Internal: datasource "customers" returned no record for pk=42', - 'The record no longer exists.', - ); - } - } - const context = makeContext(); - const executor = new TestOperationExecutor(context, () => - Promise.reject(new DualMessageError()), - ); - - await executor.execute(); - - expect(context.activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'The record no longer exists.', - ); - }); - it('does NOT run the operation and propagates when createPending throws', async () => { const context = makeContext(); (context.activityLogPort.createPending as jest.Mock).mockRejectedValue( diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index 65c441d0a8..436ba68a19 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -214,6 +214,90 @@ describe('TriggerRecordActionStepExecutor', () => { recordId: [42], }); }); + + it('logs against a related record in another collection (cross-collection)', async () => { + const baseRecordRef = makeRecordRef({ stepIndex: 1 }); + const relatedRecord = makeRecordRef({ + stepIndex: 2, + recordId: [99], + collectionName: 'orders', + }); + const ordersSchema = makeCollectionSchema({ + collectionName: 'orders', + collectionId: 'col-orders', + collectionDisplayName: 'Orders', + actions: [ + { + name: 'cancel-order', + displayName: 'Cancel Order', + endpoint: '/forest/actions/cancel-order', + }, + ], + }); + + const invoke = jest + .fn() + .mockResolvedValueOnce({ + tool_calls: [ + { name: 'select-record', args: { recordIdentifier: 'Step 2 - Orders #99' }, id: 'c1' }, + ], + }) + .mockResolvedValueOnce({ + tool_calls: [ + { + name: 'select-action', + args: { actionName: 'Cancel Order', reasoning: 'r' }, + id: 'c2', + }, + ], + }); + const model = { + bindTools: jest.fn().mockReturnValue({ invoke }), + } as unknown as ExecutionContext['model']; + + const runStore = makeMockRunStore({ + getStepExecutions: jest.fn().mockResolvedValue([ + { + type: 'load-related-record', + stepIndex: 2, + executionResult: { + relation: { name: 'order', displayName: 'Order' }, + record: relatedRecord, + }, + selectedRecordRef: makeRecordRef(), + }, + ]), + }); + const agentPort = makeMockAgentPort(); + (agentPort.executeAction as jest.Mock).mockResolvedValue({ ok: true }); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + baseRecordRef, + model, + runStore, + agentPort, + activityLogPort, + workflowPort: makeMockWorkflowPort({ + customers: makeCollectionSchema(), + orders: ordersSchema, + }), + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + await new TriggerRecordActionStepExecutor(context).execute(); + + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'action', + type: 'write', + collectionId: 'col-orders', + recordId: [99], + }); + }); }); describe('without executionType=FullyAutomated: awaiting-input (Branch C)', () => { diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index 3780a320aa..f15c5b1f5d 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -6,7 +6,12 @@ import type { UpdateRecordStepExecutionData } from '../../src/types/step-executi import type { CollectionSchema, RecordRef } from '../../src/types/validated/collection'; import type { UpdateRecordStepDefinition } from '../../src/types/validated/step-definition'; -import { AgentPortError, RunStorePortError, StepStateError } from '../../src/errors'; +import { + ActivityLogCreationError, + AgentPortError, + RunStorePortError, + StepStateError, +} from '../../src/errors'; import UpdateRecordStepExecutor from '../../src/executors/update-record-step-executor'; import SchemaCache from '../../src/schema-cache'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; @@ -228,6 +233,107 @@ describe('UpdateRecordStepExecutor', () => { expect(result.stepOutcome.status).toBe('awaiting-input'); expect(activityLogPort.createPending).not.toHaveBeenCalled(); }); + + it('logs against a related record in another collection (cross-collection)', async () => { + const baseRecordRef = makeRecordRef({ stepIndex: 1 }); + const relatedRecord = makeRecordRef({ + stepIndex: 2, + recordId: [99], + collectionName: 'orders', + }); + const ordersSchema = makeCollectionSchema({ + collectionName: 'orders', + collectionId: 'col-orders', + collectionDisplayName: 'Orders', + fields: [ + { fieldName: 'total', displayName: 'Total', isRelationship: false, type: 'Number' }, + ], + }); + + const invoke = jest + .fn() + .mockResolvedValueOnce({ + tool_calls: [ + { name: 'select-record', args: { recordIdentifier: 'Step 2 - Orders #99' }, id: 'c1' }, + ], + }) + .mockResolvedValueOnce({ + tool_calls: [ + { + name: 'update-record-field', + args: { input: { fieldName: 'Total', value: 200, reasoning: 'r' } }, + id: 'c2', + }, + ], + }); + const model = { + bindTools: jest.fn().mockReturnValue({ invoke }), + } as unknown as ExecutionContext['model']; + + const runStore = makeMockRunStore({ + getStepExecutions: jest.fn().mockResolvedValue([ + { + type: 'load-related-record', + stepIndex: 2, + executionResult: { + relation: { name: 'order', displayName: 'Order' }, + record: relatedRecord, + }, + selectedRecordRef: makeRecordRef(), + }, + ]), + }); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + baseRecordRef, + model, + runStore, + workflowPort: makeMockWorkflowPort({ + customers: makeCollectionSchema(), + orders: ordersSchema, + }), + activityLogPort, + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + await new UpdateRecordStepExecutor(context).execute(); + + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'update', + type: 'write', + collectionId: 'col-orders', + recordId: [99], + }); + }); + + it('does not persist the executing marker when the activity log cannot be created', async () => { + const runStore = makeMockRunStore(); + const activityLogPort = { + createPending: jest + .fn() + .mockRejectedValue(new ActivityLogCreationError(new Error('audit down'))), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ + runStore, + activityLogPort, + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + const result = await new UpdateRecordStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(runStore.saveStepExecution).not.toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ idempotencyPhase: 'executing' }), + ); + }); }); describe('without executionType=FullyAutomated: awaiting-input (Branch C)', () => { From b3ee99db9ca95dc78f4bbd77b5379629ecf9f843 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 20:16:43 +0200 Subject: [PATCH 04/11] refactor(workflow): extract activity-log audit into AgentWithLog Move the activity-log audit out of OperationStepExecutor into a dedicated AgentWithLog collaborator that wraps AgentPort. Each data-access call now emits one audit entry (pending -> success/failed); the action/type are intrinsic to the method and the target (collectionId, recordId) is derived from the call, so the per-executor `operation` descriptor and the `operationCollectionId` hook are gone. Idempotency stays in the executors. Write methods take a `beforeCall` thunk run between createPending and the side effect, so the executor persists its `executing` write-ahead marker in the correct order (createPending -> executing -> write) without AgentWithLog knowing anything about idempotency. MCP (whose side effect is a tool invocation, not an AgentPort call) uses the generic `logged()` method. Also restore the local-only `errorMessage` on markFailed: it never reaches the server (status endpoint accepts only { status }) but is kept for the adapter's diagnostic log line. - delete OperationStepExecutor; RecordStepExecutor + McpStepExecutor extend BaseStepExecutor directly - extract shared loadCollectionSchema helper (reused by getCollectionSchema and AgentWithLog collectionId resolution) - getActionFormInfo / probe stay on the raw agentPort (intentionally not audited) - new agent-with-log.test.ts (audit per method, beforeCall ordering, createPending-throws, userMessage vs 'Unexpected error'); relocate the audit suite out of the deleted operation-step-executor.test.ts fixes PRD-442 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../forestadmin-client-activity-log-port.ts | 3 +- .../src/executors/agent-with-log.ts | 144 +++++++++++ .../src/executors/base-step-executor.ts | 15 +- .../src/executors/load-collection-schema.ts | 18 ++ .../load-related-record-step-executor.ts | 90 +++---- .../src/executors/mcp-step-executor.ts | 49 ++-- .../src/executors/operation-step-executor.ts | 51 ---- .../executors/read-record-step-executor.ts | 17 +- .../src/executors/record-step-executor.ts | 25 +- .../trigger-record-action-step-executor.ts | 35 ++- .../executors/update-record-step-executor.ts | 37 ++- .../src/ports/activity-log-port.ts | 5 +- ...restadmin-client-activity-log-port.test.ts | 17 +- .../test/executors/agent-with-log.test.ts | 230 ++++++++++++++++++ .../executors/operation-step-executor.test.ts | 175 ------------- 15 files changed, 533 insertions(+), 378 deletions(-) create mode 100644 packages/workflow-executor/src/executors/agent-with-log.ts create mode 100644 packages/workflow-executor/src/executors/load-collection-schema.ts delete mode 100644 packages/workflow-executor/src/executors/operation-step-executor.ts create mode 100644 packages/workflow-executor/test/executors/agent-with-log.test.ts delete mode 100644 packages/workflow-executor/test/executors/operation-step-executor.test.ts diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index feb270f96f..f93be0a977 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -73,7 +73,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort }); } - async markFailed(handle: ActivityLogHandle): Promise { + async markFailed(handle: ActivityLogHandle, errorMessage: string): Promise { return this.drainer.track(async () => { try { await withRetry( @@ -89,6 +89,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } catch (err) { this.logger.error('activity log mark-as-failed failed', { handleId: handle.id, + stepErrorMessage: errorMessage, error: extractErrorMessage(err), }); } diff --git a/packages/workflow-executor/src/executors/agent-with-log.ts b/packages/workflow-executor/src/executors/agent-with-log.ts new file mode 100644 index 0000000000..2815b9b825 --- /dev/null +++ b/packages/workflow-executor/src/executors/agent-with-log.ts @@ -0,0 +1,144 @@ +import type { ActivityLogPort, CreateActivityLogArgs } from '../ports/activity-log-port'; +import type { + AgentPort, + ExecuteActionQuery, + GetRecordQuery, + GetRelatedDataQuery, + GetSingleRelatedDataQuery, + UpdateRecordQuery, +} from '../ports/agent-port'; +import type { WorkflowPort } from '../ports/workflow-port'; +import type SchemaCache from '../schema-cache'; +import type { StepUser } from '../types/execution-context'; +import type { RecordData, RecordId } from '../types/validated/collection'; + +import { WorkflowExecutorError } from '../errors'; +import loadCollectionSchema from './load-collection-schema'; + +export type LoggedOperation = Pick; + +type WriteOptions = { beforeCall: () => Promise }; + +export interface AgentWithLogDeps { + agentPort: AgentPort; + activityLogPort: ActivityLogPort; + workflowPort: WorkflowPort; + schemaCache: SchemaCache; + user: StepUser; + runId: string; +} + +// Wraps AgentPort and emits an activity-log entry around each data-access call +// (pending → success/failed). The audit target is derived from the call: the numeric +// collectionId is resolved from the call's collection name, the recordId from its id. +// Idempotency stays in the executors: write methods run a `beforeCall` thunk between +// createPending and the side effect (the executor persists its write-ahead marker there), +// so AgentWithLog never reaches into run state. +export default class AgentWithLog { + private readonly agentPort: AgentPort; + private readonly activityLogPort: ActivityLogPort; + private readonly workflowPort: WorkflowPort; + private readonly schemaCache: SchemaCache; + private readonly user: StepUser; + private readonly runId: string; + + constructor(deps: AgentWithLogDeps) { + this.agentPort = deps.agentPort; + this.activityLogPort = deps.activityLogPort; + this.workflowPort = deps.workflowPort; + this.schemaCache = deps.schemaCache; + this.user = deps.user; + this.runId = deps.runId; + } + + async getRecord(query: GetRecordQuery): Promise { + const collectionId = await this.resolveCollectionId(query.collection); + + return this.audit({ action: 'index', type: 'read', collectionId, recordId: query.id }, () => + this.agentPort.getRecord(query, this.user), + ); + } + + async getRelatedData(query: GetRelatedDataQuery): Promise { + const collectionId = await this.resolveCollectionId(query.collection); + + return this.audit( + { action: 'listRelatedData', type: 'read', collectionId, recordId: query.id }, + () => this.agentPort.getRelatedData(query, this.user), + ); + } + + async getSingleRelatedData(query: GetSingleRelatedDataQuery): Promise { + const collectionId = await this.resolveCollectionId(query.collection); + + return this.audit( + { action: 'listRelatedData', type: 'read', collectionId, recordId: query.id }, + () => this.agentPort.getSingleRelatedData(query, this.user), + ); + } + + async updateRecord(query: UpdateRecordQuery, opts: WriteOptions): Promise { + const collectionId = await this.resolveCollectionId(query.collection); + + return this.audit( + { action: 'update', type: 'write', collectionId, recordId: query.id }, + () => this.agentPort.updateRecord(query, this.user), + opts.beforeCall, + ); + } + + async executeAction(query: ExecuteActionQuery, opts: WriteOptions): Promise { + const collectionId = await this.resolveCollectionId(query.collection); + + return this.audit( + { action: 'action', type: 'write', collectionId, recordId: query.id }, + () => this.agentPort.executeAction(query, this.user), + opts.beforeCall, + ); + } + + // For operations that are not AgentPort calls (e.g. MCP tool invocation): the caller + // supplies the full audit target since there is no collection name to resolve. + logged( + op: LoggedOperation & { collectionId: string; recordId?: RecordId }, + run: () => Promise, + opts?: { beforeCall?: () => Promise }, + ): Promise { + return this.audit(op, run, opts?.beforeCall); + } + + private async audit( + args: Omit, + run: () => Promise, + beforeCall?: () => Promise, + ): Promise { + const handle = await this.activityLogPort.createPending({ + renderingId: this.user.renderingId, + ...args, + }); + + try { + if (beforeCall) await beforeCall(); + const result = await run(); + void this.activityLogPort.markSucceeded(handle); + + return result; + } catch (err) { + const errorMessage = + err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; + void this.activityLogPort.markFailed(handle, errorMessage); + throw err; + } + } + + private async resolveCollectionId(collectionName: string): Promise { + const schema = await loadCollectionSchema( + this.schemaCache, + this.workflowPort, + this.runId, + collectionName, + ); + + return schema.collectionId; + } +} diff --git a/packages/workflow-executor/src/executors/base-step-executor.ts b/packages/workflow-executor/src/executors/base-step-executor.ts index 329d2cc579..3cc0f521e8 100644 --- a/packages/workflow-executor/src/executors/base-step-executor.ts +++ b/packages/workflow-executor/src/executors/base-step-executor.ts @@ -25,6 +25,7 @@ import { WorkflowExecutorError, extractErrorMessage, } from '../errors'; +import AgentWithLog from './agent-with-log'; import patchBodySchemas from '../http/pending-data-validators'; import StepSummaryBuilder from './summary/step-summary-builder'; @@ -33,11 +34,23 @@ export default abstract class BaseStepExecutor; + // Raw port — kept only for getActionFormInfo, which is intentionally not audited. protected readonly agentPort: AgentPort; + // Audited data access — every call emits an activity-log entry. + protected readonly agent: AgentWithLog; + constructor(context: ExecutionContext) { this.context = context; this.agentPort = context.agentPort; + this.agent = new AgentWithLog({ + agentPort: context.agentPort, + activityLogPort: context.activityLogPort, + workflowPort: context.workflowPort, + schemaCache: context.schemaCache, + user: context.user, + runId: context.runId, + }); } async execute(): Promise { @@ -51,7 +64,7 @@ export default abstract class BaseStepExecutor { + const cached = schemaCache.get(collectionName); + if (cached) return cached; + + const schema = await workflowPort.getCollectionSchema(collectionName, runId); + schemaCache.set(collectionName, schema); + + return schema; +} diff --git a/packages/workflow-executor/src/executors/load-related-record-step-executor.ts b/packages/workflow-executor/src/executors/load-related-record-step-executor.ts index 07eceeb00f..9fce505981 100644 --- a/packages/workflow-executor/src/executors/load-related-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/load-related-record-step-executor.ts @@ -55,8 +55,6 @@ interface RelationTarget extends RelationRef { } export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - protected readonly operation = { action: 'listRelatedData', type: 'read' } as const; - protected async doExecute(): Promise { // Branch A -- Re-entry after pending execution found in RunStore const pending = await this.patchAndReloadPendingData( @@ -188,37 +186,35 @@ export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - return this.logOperation(target.selectedRecordRef, async () => { - if (target.relationType === 'BelongsTo' || target.relationType === 'HasOne') { - const candidate = await this.fetchXToOneCandidate(target); - - return candidate - ? { availableRecordIds: [candidate], suggestedRecord: candidate } - : { availableRecordIds: [] }; - } + if (target.relationType === 'BelongsTo' || target.relationType === 'HasOne') { + const candidate = await this.fetchXToOneCandidate(target); - const { relatedData, bestIndex, relatedSchema } = await this.selectBestFromRelatedData( - target, - 50, - ); + return candidate + ? { availableRecordIds: [candidate], suggestedRecord: candidate } + : { availableRecordIds: [] }; + } - if (relatedData.length === 0) { - return { availableRecordIds: [] }; - } + const { relatedData, bestIndex, relatedSchema } = await this.selectBestFromRelatedData( + target, + 50, + ); - const referenceField = relatedSchema.referenceField ?? null; - const toCandidate = (r: RecordData): LoadRelatedRecordCandidate => ({ - recordId: r.recordId, - referenceFieldValue: referenceField - ? this.extractReferenceFieldValue(r.values, referenceField) - : null, - }); + if (relatedData.length === 0) { + return { availableRecordIds: [] }; + } - return { - availableRecordIds: relatedData.map(toCandidate), - suggestedRecord: toCandidate(relatedData[bestIndex]), - }; + const referenceField = relatedSchema.referenceField ?? null; + const toCandidate = (r: RecordData): LoadRelatedRecordCandidate => ({ + recordId: r.recordId, + referenceFieldValue: referenceField + ? this.extractReferenceFieldValue(r.values, referenceField) + : null, }); + + return { + availableRecordIds: relatedData.map(toCandidate), + suggestedRecord: toCandidate(relatedData[bestIndex]), + }; } private extractReferenceFieldValue( @@ -232,9 +228,7 @@ export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - const record = await this.logOperation(target.selectedRecordRef, () => - this.fetchRecordForRelation(target), - ); + const record = await this.fetchRecordForRelation(target); return this.persistAndReturn(record, target, undefined); } @@ -271,16 +265,13 @@ export default class LoadRelatedRecordStepExecutor extends RecordStepExecutor { - return this.agentPort.getRelatedData( - { - collection: target.selectedRecordRef.collectionName, - id: target.selectedRecordRef.recordId, - relation: target.name, - relatedSchema, - limit, - }, - this.context.user, - ); + return this.agent.getRelatedData({ + collection: target.selectedRecordRef.collectionName, + id: target.selectedRecordRef.recordId, + relation: target.name, + relatedSchema, + limit, + }); } /** Persists the loaded record ref and returns a success outcome. */ diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index 90a3de0e4d..92c5d3ff04 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -13,7 +13,7 @@ import { NoMcpToolsError, StepStateError, } from '../errors'; -import OperationStepExecutor, { type OperationDescriptor } from './operation-step-executor'; +import BaseStepExecutor from './base-step-executor'; import { StepExecutionMode } from '../types/validated/step-definition'; const MCP_TASK_SYSTEM_PROMPT = `You are an AI agent selecting and executing a tool to fulfill a user request. @@ -23,13 +23,7 @@ Important rules: - Select only the tool directly relevant to the request. - Final answer is definitive, you won't receive any other input from the user.`; -export default class McpStepExecutor extends OperationStepExecutor { - protected readonly operation: OperationDescriptor = { - action: 'action', - type: 'write', - label: this.context.stepDefinition.mcpServerId, - }; - +export default class McpStepExecutor extends BaseStepExecutor { private readonly remoteTools: readonly RemoteTool[]; private readonly mcpServerName?: string; @@ -121,20 +115,31 @@ export default class McpStepExecutor extends OperationStepExecutor t.base.name === target.name && t.sourceId === target.sourceId); if (!tool) throw new McpToolNotFoundError(target.name); - const toolResult = await this.logOperation(this.context.baseRecordRef, async () => { - await this.context.runStore.saveStepExecution(this.context.runId, { - ...existingExecution, - type: 'mcp', - stepIndex: this.context.stepIndex, - idempotencyPhase: 'executing', - }); - - try { - return await tool.base.invoke(target.input); - } catch (cause) { - throw new McpToolInvocationError(target.name, cause); - } - }); + const toolResult = await this.agent.logged( + { + action: 'action', + type: 'write', + label: this.context.stepDefinition.mcpServerId, + collectionId: this.context.collectionId, + recordId: this.context.baseRecordRef.recordId, + }, + async () => { + try { + return await tool.base.invoke(target.input); + } catch (cause) { + throw new McpToolInvocationError(target.name, cause); + } + }, + { + beforeCall: () => + this.context.runStore.saveStepExecution(this.context.runId, { + ...existingExecution, + type: 'mcp', + stepIndex: this.context.stepIndex, + idempotencyPhase: 'executing', + }), + }, + ); // 1. Persist raw result immediately — safe state before any further network calls const baseExecutionResult = { success: true as const, toolResult }; diff --git a/packages/workflow-executor/src/executors/operation-step-executor.ts b/packages/workflow-executor/src/executors/operation-step-executor.ts deleted file mode 100644 index 6363e657f2..0000000000 --- a/packages/workflow-executor/src/executors/operation-step-executor.ts +++ /dev/null @@ -1,51 +0,0 @@ -import type { CreateActivityLogArgs } from '../ports/activity-log-port'; -import type { RecordRef } from '../types/validated/collection'; -import type { StepDefinition } from '../types/validated/step-definition'; - -import BaseStepExecutor from './base-step-executor'; - -export type OperationDescriptor = Pick; - -export default abstract class OperationStepExecutor< - TStep extends StepDefinition = StepDefinition, -> extends BaseStepExecutor { - protected abstract readonly operation: OperationDescriptor; - - // Defaults to the run's trigger collection; RecordStepExecutor overrides it to resolve the - // acted record's own collection (which may differ from the trigger). - // eslint-disable-next-line @typescript-eslint/no-unused-vars - protected operationCollectionId(record: RecordRef): Promise { - return Promise.resolve(this.context.collectionId); - } - - protected async logOperation(record: RecordRef, run: () => Promise): Promise { - const collectionId = await this.operationCollectionId(record); - - return this.withActivityLog( - { - renderingId: this.context.user.renderingId, - ...this.operation, - collectionId, - recordId: record.recordId, - }, - run, - ); - } - - private async withActivityLog( - args: CreateActivityLogArgs, - operation: () => Promise, - ): Promise { - const handle = await this.context.activityLogPort.createPending(args); - - try { - const result = await operation(); - void this.context.activityLogPort.markSucceeded(handle); - - return result; - } catch (err) { - void this.context.activityLogPort.markFailed(handle); - throw err; - } - } -} diff --git a/packages/workflow-executor/src/executors/read-record-step-executor.ts b/packages/workflow-executor/src/executors/read-record-step-executor.ts index 5bf4b798d7..2c9c495ee3 100644 --- a/packages/workflow-executor/src/executors/read-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/read-record-step-executor.ts @@ -18,8 +18,6 @@ Important rules: - Do not refer to yourself as "I" in the response, use a passive formulation instead.`; export default class ReadRecordStepExecutor extends RecordStepExecutor { - protected readonly operation = { action: 'index', type: 'read' } as const; - protected async doExecute(): Promise { const { stepDefinition: step } = this.context; const { preRecordedArgs } = step; @@ -41,16 +39,11 @@ export default class ReadRecordStepExecutor extends RecordStepExecutor - this.agentPort.getRecord( - { - collection: selectedRecordRef.collectionName, - id: selectedRecordRef.recordId, - fields: resolvedFieldNames, - }, - this.context.user, - ), - ); + const recordData = await this.agent.getRecord({ + collection: selectedRecordRef.collectionName, + id: selectedRecordRef.recordId, + fields: resolvedFieldNames, + }); const fieldResults = this.formatFieldResults(recordData.values, schema, selectedDisplayNames); await this.context.runStore.saveStepExecution(this.context.runId, { diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index 461fe61bb8..4cbac52c0c 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -7,17 +7,12 @@ import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin import { z } from 'zod'; import { InvalidAIResponseError, InvalidPreRecordedArgsError, NoRecordsError } from '../errors'; -import OperationStepExecutor from './operation-step-executor'; +import BaseStepExecutor from './base-step-executor'; +import loadCollectionSchema from './load-collection-schema'; export default abstract class RecordStepExecutor< TStep extends StepDefinition = StepDefinition, -> extends OperationStepExecutor { - protected override async operationCollectionId(record: RecordRef): Promise { - const { collectionId } = await this.getCollectionSchema(record.collectionName); - - return collectionId; - } - +> extends BaseStepExecutor { protected buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; @@ -69,17 +64,13 @@ export default abstract class RecordStepExecutor< return [this.context.baseRecordRef, ...relatedRecords]; } - protected async getCollectionSchema(collectionName: string): Promise { - const cached = this.context.schemaCache.get(collectionName); - if (cached) return cached; - - const schema = await this.context.workflowPort.getCollectionSchema( - collectionName, + protected getCollectionSchema(collectionName: string): Promise { + return loadCollectionSchema( + this.context.schemaCache, + this.context.workflowPort, this.context.runId, + collectionName, ); - this.context.schemaCache.set(collectionName, schema); - - return schema; } protected findField(schema: CollectionSchema, name: string): FieldSchema | undefined { diff --git a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts index f85aad8df8..b65d1a678e 100644 --- a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts +++ b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts @@ -28,8 +28,6 @@ interface ActionTarget extends ActionRef { } export default class TriggerRecordActionStepExecutor extends RecordStepExecutor { - protected readonly operation = { action: 'action', type: 'write' } as const; - protected override async checkIdempotency(): Promise { const existing = await this.findPendingExecution( 'trigger-action', @@ -131,23 +129,22 @@ export default class TriggerRecordActionStepExecutor extends RecordStepExecutor< private async executeOnExecutor(target: ActionTarget): Promise { const { selectedRecordRef, displayName, name } = target; - const actionResult = await this.logOperation(selectedRecordRef, async () => { - await this.context.runStore.saveStepExecution(this.context.runId, { - type: 'trigger-action', - stepIndex: this.context.stepIndex, - selectedRecordRef, - idempotencyPhase: 'executing', - }); - - return this.agentPort.executeAction( - { - collection: selectedRecordRef.collectionName, - action: name, - id: selectedRecordRef.recordId, - }, - this.context.user, - ); - }); + const actionResult = await this.agent.executeAction( + { + collection: selectedRecordRef.collectionName, + action: name, + id: selectedRecordRef.recordId, + }, + { + beforeCall: () => + this.context.runStore.saveStepExecution(this.context.runId, { + type: 'trigger-action', + stepIndex: this.context.stepIndex, + selectedRecordRef, + idempotencyPhase: 'executing', + }), + }, + ); await this.context.runStore.saveStepExecution(this.context.runId, { type: 'trigger-action', diff --git a/packages/workflow-executor/src/executors/update-record-step-executor.ts b/packages/workflow-executor/src/executors/update-record-step-executor.ts index 2256bbeb45..fa3fa5d540 100644 --- a/packages/workflow-executor/src/executors/update-record-step-executor.ts +++ b/packages/workflow-executor/src/executors/update-record-step-executor.ts @@ -126,8 +126,6 @@ interface UpdateTarget extends FieldWithValue { } export default class UpdateRecordStepExecutor extends RecordStepExecutor { - protected readonly operation = { action: 'update', type: 'write' } as const; - protected override async checkIdempotency(): Promise { const existing = await this.findPendingExecution( 'update-record', @@ -250,24 +248,23 @@ export default class UpdateRecordStepExecutor extends RecordStepExecutor { const { selectedRecordRef, displayName, name, value } = target; - const updated = await this.logOperation(selectedRecordRef, async () => { - await this.context.runStore.saveStepExecution(this.context.runId, { - ...existingExecution, - type: 'update-record', - stepIndex: this.context.stepIndex, - selectedRecordRef, - idempotencyPhase: 'executing', - }); - - return this.agentPort.updateRecord( - { - collection: selectedRecordRef.collectionName, - id: selectedRecordRef.recordId, - values: { [name]: value }, - }, - this.context.user, - ); - }); + const updated = await this.agent.updateRecord( + { + collection: selectedRecordRef.collectionName, + id: selectedRecordRef.recordId, + values: { [name]: value }, + }, + { + beforeCall: () => + this.context.runStore.saveStepExecution(this.context.runId, { + ...existingExecution, + type: 'update-record', + stepIndex: this.context.stepIndex, + selectedRecordRef, + idempotencyPhase: 'executing', + }), + }, + ); await this.context.runStore.saveStepExecution(this.context.runId, { ...existingExecution, diff --git a/packages/workflow-executor/src/ports/activity-log-port.ts b/packages/workflow-executor/src/ports/activity-log-port.ts index 4116ecb84e..c1c1c86428 100644 --- a/packages/workflow-executor/src/ports/activity-log-port.ts +++ b/packages/workflow-executor/src/ports/activity-log-port.ts @@ -17,11 +17,12 @@ export interface ActivityLogHandle { } // Per-run scoped port: token baked into the adapter's constructor. markSucceeded/markFailed -// retry transient failures internally and are invoked with `void` from OperationStepExecutor. +// retry transient failures internally and are invoked with `void` from AgentWithLog. export interface ActivityLogPort { createPending(args: CreateActivityLogArgs): Promise; markSucceeded(handle: ActivityLogHandle): Promise; - markFailed(handle: ActivityLogHandle): Promise; + // errorMessage is for local diagnostics only — the server status endpoint accepts just { status }. + markFailed(handle: ActivityLogHandle, errorMessage: string): Promise; } // Produces per-run ActivityLogPort instances and exposes drain() at the process level so the diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 1b8287a992..97a1af1b20 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -267,14 +267,14 @@ describe('ForestadminClientActivityLogPort', () => { }); describe('markFailed', () => { - it('sends status: failed (no errorMessage — server schema rejects unknown fields) and retries on 503', async () => { + it('sends status: failed to the server without the local errorMessage, and retries on 503', async () => { const service = makeService(); service.updateActivityLogStatus .mockRejectedValueOnce(makeHttpError(503)) .mockResolvedValueOnce(undefined); const port = makePort(service); - const promise = port.markFailed({ id: 'log-1', index: '0' }); + const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step failed'); await jest.advanceTimersByTimeAsync(100); await promise; @@ -284,20 +284,23 @@ describe('ForestadminClientActivityLogPort', () => { forestServerToken: 'tok', }), ); + expect(service.updateActivityLogStatus).toHaveBeenLastCalledWith( + expect.not.objectContaining({ errorMessage: expect.anything() }), + ); }); - it('swallows errors after retries are exhausted (fire-and-forget) and logs the handle', async () => { + it('swallows errors after retries are exhausted (fire-and-forget) and logs the step error message', async () => { const service = makeService(); service.updateActivityLogStatus.mockRejectedValue(makeHttpError(503)); const logger = makeLogger(); const port = makePort(service, { logger }); - const promise = port.markFailed({ id: 'log-1', index: '0' }); + const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step-error-msg'); await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( 'activity log mark-as-failed failed', - expect.objectContaining({ handleId: 'log-1' }), + expect.objectContaining({ handleId: 'log-1', stepErrorMessage: 'step-error-msg' }), ); }); @@ -308,7 +311,7 @@ describe('ForestadminClientActivityLogPort', () => { .mockResolvedValueOnce(undefined); const port = makePort(service); - const promise = port.markFailed({ id: 'log-1', index: '0' }); + const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step failed'); await jest.advanceTimersByTimeAsync(100); await expect(promise).resolves.toBeUndefined(); expect(service.updateActivityLogStatus).toHaveBeenCalledTimes(2); @@ -356,7 +359,7 @@ describe('ForestadminClientActivityLogPort', () => { const drainer = new ActivityLogDrainer(); const port = makePort(service, { drainer }); - const markPromise = port.markFailed({ id: 'log-1', index: '0' }); + const markPromise = port.markFailed({ id: 'log-1', index: '0' }, 'step failed'); let drainResolved = false; const drainPromise = drainer.drain().then(() => { diff --git a/packages/workflow-executor/test/executors/agent-with-log.test.ts b/packages/workflow-executor/test/executors/agent-with-log.test.ts new file mode 100644 index 0000000000..bca29927b0 --- /dev/null +++ b/packages/workflow-executor/test/executors/agent-with-log.test.ts @@ -0,0 +1,230 @@ +import type { AgentWithLogDeps } from '../../src/executors/agent-with-log'; +import type { AgentPort } from '../../src/ports/agent-port'; +import type { WorkflowPort } from '../../src/ports/workflow-port'; +import type SchemaCache from '../../src/schema-cache'; +import type { StepUser } from '../../src/types/execution-context'; +import type { CollectionSchema } from '../../src/types/validated/collection'; + +import { NoRecordsError } from '../../src/errors'; +import AgentWithLog from '../../src/executors/agent-with-log'; + +function makeUser(): StepUser { + return { + id: 1, + email: 'test@example.com', + firstName: 'Test', + lastName: 'User', + team: 'admin', + renderingId: 1, + role: 'admin', + permissionLevel: 'admin', + tags: {}, + } as StepUser; +} + +function makeSchema(collectionId = 'col-customers'): CollectionSchema { + return { + collectionName: 'customers', + collectionId, + collectionDisplayName: 'Customers', + primaryKeyFields: ['id'], + referenceField: null, + fields: [], + actions: [], + }; +} + +function makeActivityLogPort() { + return { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; +} + +function makeDeps(overrides: Partial = {}) { + const activityLogPort = makeActivityLogPort(); + const agentPort = { + getRecord: jest + .fn() + .mockResolvedValue({ collectionName: 'customers', recordId: [42], values: {} }), + updateRecord: jest + .fn() + .mockResolvedValue({ collectionName: 'customers', recordId: [42], values: {} }), + getRelatedData: jest.fn().mockResolvedValue([]), + getSingleRelatedData: jest.fn().mockResolvedValue(null), + executeAction: jest.fn().mockResolvedValue({ ok: true }), + } as unknown as AgentPort; + const schemaCache = { + get: jest.fn().mockReturnValue(makeSchema()), + set: jest.fn(), + } as unknown as SchemaCache; + const workflowPort = { + getCollectionSchema: jest.fn().mockResolvedValue(makeSchema()), + } as unknown as WorkflowPort; + + const deps = { + agentPort, + activityLogPort, + workflowPort, + schemaCache, + user: makeUser(), + runId: 'run-1', + ...overrides, + }; + + return { deps, agentPort, activityLogPort, schemaCache, workflowPort }; +} + +describe('AgentWithLog', () => { + describe('read methods', () => { + it('logs getRecord as index/read against the call target and returns the data', async () => { + const { deps, agentPort, activityLogPort } = makeDeps(); + const agent = new AgentWithLog(deps); + + const result = await agent.getRecord({ collection: 'customers', id: [42], fields: ['name'] }); + + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'index', + type: 'read', + collectionId: 'col-customers', + recordId: [42], + }); + expect(activityLogPort.markSucceeded).toHaveBeenCalledWith({ id: 'log-1', index: '0' }); + expect(agentPort.getRecord).toHaveBeenCalledWith( + { collection: 'customers', id: [42], fields: ['name'] }, + expect.objectContaining({ id: 1 }), + ); + expect(result).toEqual({ collectionName: 'customers', recordId: [42], values: {} }); + }); + + it('logs getRelatedData as listRelatedData/read', async () => { + const { deps, activityLogPort } = makeDeps(); + const agent = new AgentWithLog(deps); + + await agent.getRelatedData({ + collection: 'customers', + id: [42], + relation: 'orders', + relatedSchema: makeSchema('col-orders'), + limit: 50, + }); + + expect(activityLogPort.createPending).toHaveBeenCalledWith( + expect.objectContaining({ action: 'listRelatedData', type: 'read', recordId: [42] }), + ); + }); + }); + + describe('write methods', () => { + it('runs beforeCall between createPending and the agent call (audit precedes the side effect)', async () => { + const order: string[] = []; + const { deps, agentPort, activityLogPort } = makeDeps(); + (activityLogPort.createPending as jest.Mock).mockImplementation(async () => { + order.push('createPending'); + + return { id: 'log-1', index: '0' }; + }); + (agentPort.updateRecord as jest.Mock).mockImplementation(async () => { + order.push('updateRecord'); + + return { collectionName: 'customers', recordId: [42], values: {} }; + }); + const agent = new AgentWithLog(deps); + + await agent.updateRecord( + { collection: 'customers', id: [42], values: { name: 'X' } }, + { + beforeCall: async () => { + order.push('beforeCall'); + }, + }, + ); + + expect(order).toEqual(['createPending', 'beforeCall', 'updateRecord']); + expect(activityLogPort.createPending).toHaveBeenCalledWith( + expect.objectContaining({ action: 'update', type: 'write', recordId: [42] }), + ); + }); + + it('does NOT run beforeCall or the agent call when createPending throws', async () => { + const { deps, agentPort, activityLogPort } = makeDeps(); + (activityLogPort.createPending as jest.Mock).mockRejectedValue(new Error('audit down')); + const beforeCall = jest.fn().mockResolvedValue(undefined); + const agent = new AgentWithLog(deps); + + await expect( + agent.updateRecord( + { collection: 'customers', id: [42], values: { name: 'X' } }, + { beforeCall }, + ), + ).rejects.toThrow('audit down'); + expect(beforeCall).not.toHaveBeenCalled(); + expect(agentPort.updateRecord).not.toHaveBeenCalled(); + }); + }); + + describe('failure marking', () => { + it('marks failed with the userMessage when the operation throws a WorkflowExecutorError', async () => { + const { deps, agentPort, activityLogPort } = makeDeps(); + (agentPort.updateRecord as jest.Mock).mockRejectedValue(new NoRecordsError()); + const agent = new AgentWithLog(deps); + + await expect( + agent.updateRecord( + { collection: 'customers', id: [42], values: { name: 'X' } }, + { beforeCall: async () => undefined }, + ), + ).rejects.toBeInstanceOf(NoRecordsError); + + expect(activityLogPort.markFailed).toHaveBeenCalledWith( + { id: 'log-1', index: '0' }, + 'No records available', + ); + expect(activityLogPort.markSucceeded).not.toHaveBeenCalled(); + }); + + it("marks failed with 'Unexpected error' for a non-WorkflowExecutorError", async () => { + const { deps, agentPort, activityLogPort } = makeDeps(); + (agentPort.getRecord as jest.Mock).mockRejectedValue(new Error('boom')); + const agent = new AgentWithLog(deps); + + await expect(agent.getRecord({ collection: 'customers', id: [42] })).rejects.toThrow('boom'); + + expect(activityLogPort.markFailed).toHaveBeenCalledWith( + { id: 'log-1', index: '0' }, + 'Unexpected error', + ); + }); + }); + + describe('logged (generic, non-AgentPort operations)', () => { + it('audits an arbitrary operation against the provided target with a label', async () => { + const { deps, activityLogPort } = makeDeps(); + const agent = new AgentWithLog(deps); + + const result = await agent.logged( + { + action: 'action', + type: 'write', + label: 'my-mcp-server', + collectionId: 'col-1', + recordId: [7], + }, + async () => 'done', + { beforeCall: async () => undefined }, + ); + + expect(result).toBe('done'); + expect(activityLogPort.createPending).toHaveBeenCalledWith({ + renderingId: 1, + action: 'action', + type: 'write', + label: 'my-mcp-server', + collectionId: 'col-1', + recordId: [7], + }); + }); + }); +}); diff --git a/packages/workflow-executor/test/executors/operation-step-executor.test.ts b/packages/workflow-executor/test/executors/operation-step-executor.test.ts deleted file mode 100644 index 99cb4c4f1a..0000000000 --- a/packages/workflow-executor/test/executors/operation-step-executor.test.ts +++ /dev/null @@ -1,175 +0,0 @@ -/* eslint-disable max-classes-per-file */ -import type { Logger } from '../../src/ports/logger-port'; -import type { RunStore } from '../../src/ports/run-store'; -import type { ExecutionContext, StepExecutionResult } from '../../src/types/execution-context'; -import type { RecordRef } from '../../src/types/validated/collection'; -import type { BaseStepStatus } from '../../src/types/validated/step-outcome'; - -import { ActivityLogCreationError, NoRecordsError } from '../../src/errors'; -import OperationStepExecutor from '../../src/executors/operation-step-executor'; -import SchemaCache from '../../src/schema-cache'; -import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; - -class TestOperationExecutor extends OperationStepExecutor { - protected readonly operation = { action: 'update', type: 'write' } as const; - - constructor( - context: ExecutionContext, - private readonly run: () => Promise = () => Promise.resolve('ok'), - ) { - super(context); - } - - protected async doExecute(): Promise { - await this.logOperation(this.context.baseRecordRef, this.run); - - return this.buildOutcomeResult({ status: 'success' }); - } - - protected buildOutcomeResult(outcome: { - status: BaseStepStatus; - error?: string; - }): StepExecutionResult { - return { - stepOutcome: { - type: 'record', - stepId: this.context.stepId, - stepIndex: this.context.stepIndex, - status: outcome.status, - ...(outcome.error !== undefined && { error: outcome.error }), - }, - }; - } -} - -function makeMockRunStore(): RunStore { - return { - init: jest.fn().mockResolvedValue(undefined), - close: jest.fn().mockResolvedValue(undefined), - getStepExecutions: jest.fn().mockResolvedValue([]), - saveStepExecution: jest.fn().mockResolvedValue(undefined), - }; -} - -function makeMockLogger(): Logger { - return { info: jest.fn(), warn: jest.fn(), error: jest.fn() }; -} - -function makeMockActivityLogPort(): ExecutionContext['activityLogPort'] { - return { - createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), - markSucceeded: jest.fn().mockResolvedValue(undefined), - markFailed: jest.fn().mockResolvedValue(undefined), - }; -} - -function makeContext(overrides: Partial = {}): ExecutionContext { - return { - runId: 'run-1', - stepId: 'step-0', - stepIndex: 0, - collectionId: 'col-1', - baseRecordRef: { - collectionName: 'customers', - recordId: [42], - stepIndex: 0, - } as RecordRef, - stepDefinition: { - type: StepType.UpdateRecord, - executionType: StepExecutionMode.FullyAutomated, - prompt: 'Update it', - }, - model: {} as ExecutionContext['model'], - agentPort: {} as ExecutionContext['agentPort'], - workflowPort: {} as ExecutionContext['workflowPort'], - runStore: makeMockRunStore(), - user: { - id: 1, - email: 'test@example.com', - firstName: 'Test', - lastName: 'User', - team: 'admin', - renderingId: 1, - role: 'admin', - permissionLevel: 'admin', - tags: {}, - }, - schemaCache: new SchemaCache(), - previousSteps: [], - logger: makeMockLogger(), - activityLogPort: makeMockActivityLogPort(), - ...overrides, - }; -} - -describe('OperationStepExecutor', () => { - describe('logOperation', () => { - it('logs the operation against the given record and trigger collection, then marks succeeded', async () => { - const context = makeContext(); - const executor = new TestOperationExecutor(context); - - const result = await executor.execute(); - - expect(result.stepOutcome.status).toBe('success'); - expect(context.activityLogPort.createPending).toHaveBeenCalledWith({ - renderingId: 1, - action: 'update', - type: 'write', - collectionId: 'col-1', - recordId: [42], - }); - expect(context.activityLogPort.markSucceeded).toHaveBeenCalledWith({ - id: 'log-1', - index: '0', - }); - expect(context.activityLogPort.markFailed).not.toHaveBeenCalled(); - }); - - it('marks the log failed (by handle) when the operation throws', async () => { - const context = makeContext(); - const executor = new TestOperationExecutor(context, () => - Promise.reject(new NoRecordsError()), - ); - - await executor.execute(); - - expect(context.activityLogPort.markFailed).toHaveBeenCalledWith({ id: 'log-1', index: '0' }); - expect(context.activityLogPort.markSucceeded).not.toHaveBeenCalled(); - }); - - it('does NOT run the operation and propagates when createPending throws', async () => { - const context = makeContext(); - (context.activityLogPort.createPending as jest.Mock).mockRejectedValue( - new ActivityLogCreationError(new Error('net')), - ); - const operation = jest.fn().mockResolvedValue('ok'); - - const executor = new TestOperationExecutor(context, operation); - const result = await executor.execute(); - - expect(result.stepOutcome.status).toBe('error'); - expect(result.stepOutcome.error).toBe( - 'Could not record this step in the audit log. Please try again, or contact your administrator if the problem persists.', - ); - expect(operation).not.toHaveBeenCalled(); - }); - }); - - describe('operationCollectionId', () => { - it('targets the collection resolved by the override instead of the trigger collection', async () => { - class OverridingExecutor extends TestOperationExecutor { - protected override operationCollectionId(): Promise { - return Promise.resolve('col-orders'); - } - } - const context = makeContext(); - const executor = new OverridingExecutor(context); - - await executor.execute(); - - expect(context.activityLogPort.createPending).toHaveBeenCalledWith( - expect.objectContaining({ collectionId: 'col-orders', recordId: [42] }), - ); - }); - }); -}); From 2bf18bc88d2e2e6693c41968c6933ffc4975aa75 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 20:33:16 +0200 Subject: [PATCH 05/11] chore(workflow): address AgentWithLog review nits - AgentWithLog: replace LoggedOperation with AuditTarget (Omit), reused by logged() and audit() so the audit-arg shape stays anchored to the port type. - CLAUDE.md: update the idempotency section to reference AgentWithLog and the beforeCall thunk (the deleted OperationStepExecutor.logOperation). - tests: assert stepOutcome.error (audit-creation failure message) on the no-orphan-marker case; pin getSingleRelatedData (xToOne) audit action. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/workflow-executor/CLAUDE.md | 2 +- .../src/executors/agent-with-log.ts | 11 ++++++----- .../test/executors/agent-with-log.test.ts | 16 ++++++++++++++++ .../update-record-step-executor.test.ts | 3 +++ 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 00a373ea6a..001eacf29f 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -88,7 +88,7 @@ src/ - **Boundary errors** (`extends Error`) — Thrown outside step execution, at the HTTP or Runner layer (e.g. `RunNotFoundError`, `PendingDataNotFoundError`, `ConfigurationError`). Caught by the HTTP server and translated into HTTP status codes (404, 400, etc.). These intentionally do NOT extend `WorkflowExecutorError` to prevent `base-step-executor` from catching them as step failures. - **Dual error messages** — `WorkflowExecutorError` carries two messages: `message` (technical, for dev logs) and `userMessage` (human-readable, surfaced to the Forest Admin UI via `stepOutcome.error`). The mapping happens in a single place: `base-step-executor.ts` uses `error.userMessage` when building the error outcome. When adding a new error subclass, always provide a distinct `userMessage` oriented toward end-users (no collection names, field names, or AI internals). If `userMessage` is omitted in the constructor call, it falls back to `message`. - **displayName in AI tools** — All `DynamicStructuredTool` schemas and system message prompts must use `displayName`, never `fieldName`. `displayName` is a Forest Admin frontend feature that replaces the technical field/relation/action name with a product-oriented label configured by the Forest Admin admin. End users write their workflow prompts using these display names, not the underlying technical names. After an AI tool call returns display names, map them back to `fieldName`/`name` before using them in datasource operations (e.g. filtering record values, calling `getRecord`). -- **Idempotency in mutating executors** — `update-record`, `trigger-action`, and `mcp` executors protect against duplicate side effects via a write-ahead log in the `RunStore`. Before the side effect fires, the executor saves `idempotencyPhase: 'executing'`. After, it saves `idempotencyPhase: 'done'` alongside the normal `executionResult`. On re-dispatch (same `runId + stepIndex`): `done` → reconstruct success outcome via `buildOutcomeResult` without re-executing or emitting an activity log; `executing` → throw `StepStateError` (user retries manually, also no activity log). The `checkIdempotency()` hook in `BaseStepExecutor` runs before `doExecute()` so neither cache hits nor uncertain-state errors reach the activity log emitted inside `OperationStepExecutor.logOperation`. The `executing` write-ahead marker is saved inside the `logOperation` callback (after `createPending`, just before the side effect) so an activity-log creation failure never leaves an orphan `executing` marker. Non-mutating executors (`condition`, `read-record`, `guidance`, `load-related-record`) do not override `checkIdempotency()` — replaying them is safe. +- **Idempotency in mutating executors** — `update-record`, `trigger-action`, and `mcp` executors protect against duplicate side effects via a write-ahead log in the `RunStore`. Before the side effect fires, the executor saves `idempotencyPhase: 'executing'`. After, it saves `idempotencyPhase: 'done'` alongside the normal `executionResult`. On re-dispatch (same `runId + stepIndex`): `done` → reconstruct success outcome via `buildOutcomeResult` without re-executing or emitting an activity log; `executing` → throw `StepStateError` (user retries manually, also no activity log). The `checkIdempotency()` hook in `BaseStepExecutor` runs before `doExecute()` so neither cache hits nor uncertain-state errors reach the activity log emitted by `AgentWithLog`. The `executing` write-ahead marker is saved in the `beforeCall` thunk the executor passes to `AgentWithLog`'s write methods (run after `createPending`, just before the side effect) so an activity-log creation failure never leaves an orphan `executing` marker. Non-mutating executors (`condition`, `read-record`, `guidance`, `load-related-record`) do not override `checkIdempotency()` — replaying them is safe. - **Fetched steps must be executed** — Any step retrieved from the orchestrator via `getAvailableRuns()` must be executed. Silently discarding a fetched step (e.g. filtering it out by `runId` after fetching) violates the executor contract: the orchestrator assumes execution is guaranteed once the step is dispatched. The only valid filter before executing is deduplication via `inFlightRuns` (keyed by `runId`, to avoid running the same run twice concurrently; the key is the run, not the step, because a chain advances the `stepId` between iterations). - **Auto-chain from `/update-step` response** — `WorkflowPort.updateStepExecution` returns `AvailableRunDispatch | null`: when non-null, the `Runner` executes the next step inline instead of waiting for the next poll. The chain exits on `null` (awaiting-input / finished / error), on a non-progressing `stepIndex` (server bug defense), at `maxChainDepth` (config, default 50), or when `stop()` is called. Each chained step uses the `forestServerToken` from its own dispatch — token freshness is preserved across the chain. The port retries `POST /update-step` on transient failures (network, 5xx) — this relies on server-side idempotency: the orchestrator MUST deduplicate identical outcomes for a given `(runId, stepIndex)` to prevent double side-effects on retry. - **Pre-recorded AI decisions** — Record step executors support `preRecordedArgs` in the step definition to bypass AI calls. When provided, executors use the pre-recorded values (display names) directly instead of invoking the AI. Each record step type has its own typed `preRecordedArgs` shape. Validation happens via schema resolution — invalid display names throw `InvalidPreRecordedArgsError`. Partial args are supported: only the provided fields skip AI, the rest still use AI. diff --git a/packages/workflow-executor/src/executors/agent-with-log.ts b/packages/workflow-executor/src/executors/agent-with-log.ts index 2815b9b825..bd0a2153a8 100644 --- a/packages/workflow-executor/src/executors/agent-with-log.ts +++ b/packages/workflow-executor/src/executors/agent-with-log.ts @@ -10,12 +10,13 @@ import type { import type { WorkflowPort } from '../ports/workflow-port'; import type SchemaCache from '../schema-cache'; import type { StepUser } from '../types/execution-context'; -import type { RecordData, RecordId } from '../types/validated/collection'; +import type { RecordData } from '../types/validated/collection'; import { WorkflowExecutorError } from '../errors'; import loadCollectionSchema from './load-collection-schema'; -export type LoggedOperation = Pick; +// The audit-log target minus renderingId, which audit() stamps centrally. +export type AuditTarget = Omit; type WriteOptions = { beforeCall: () => Promise }; @@ -100,15 +101,15 @@ export default class AgentWithLog { // For operations that are not AgentPort calls (e.g. MCP tool invocation): the caller // supplies the full audit target since there is no collection name to resolve. logged( - op: LoggedOperation & { collectionId: string; recordId?: RecordId }, + target: AuditTarget, run: () => Promise, opts?: { beforeCall?: () => Promise }, ): Promise { - return this.audit(op, run, opts?.beforeCall); + return this.audit(target, run, opts?.beforeCall); } private async audit( - args: Omit, + args: AuditTarget, run: () => Promise, beforeCall?: () => Promise, ): Promise { diff --git a/packages/workflow-executor/test/executors/agent-with-log.test.ts b/packages/workflow-executor/test/executors/agent-with-log.test.ts index bca29927b0..279eeb4cb5 100644 --- a/packages/workflow-executor/test/executors/agent-with-log.test.ts +++ b/packages/workflow-executor/test/executors/agent-with-log.test.ts @@ -115,6 +115,22 @@ describe('AgentWithLog', () => { expect.objectContaining({ action: 'listRelatedData', type: 'read', recordId: [42] }), ); }); + + it('logs getSingleRelatedData as listRelatedData/read (xToOne)', async () => { + const { deps, activityLogPort } = makeDeps(); + const agent = new AgentWithLog(deps); + + await agent.getSingleRelatedData({ + collection: 'customers', + id: [42], + relation: 'order', + relatedSchema: makeSchema('col-orders'), + }); + + expect(activityLogPort.createPending).toHaveBeenCalledWith( + expect.objectContaining({ action: 'listRelatedData', type: 'read', recordId: [42] }), + ); + }); }); describe('write methods', () => { diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index f15c5b1f5d..08b392839c 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -329,6 +329,9 @@ describe('UpdateRecordStepExecutor', () => { const result = await new UpdateRecordStepExecutor(context).execute(); expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toBe( + 'Could not record this step in the audit log. Please try again, or contact your administrator if the problem persists.', + ); expect(runStore.saveStepExecution).not.toHaveBeenCalledWith( 'run-1', expect.objectContaining({ idempotencyPhase: 'executing' }), From 0399c11bc9c00773559ebaa17aadedd693ed3975 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 20:43:26 +0200 Subject: [PATCH 06/11] fix(workflow): fail loud when a collection schema is not cached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveSchema fabricated a CollectionSchema on cache miss, including a buggy `collectionId: cached.collectionId` (cached is undefined in that branch). The fallback is unreachable in the normal flow — AgentWithLog resolves (and caches, via the shared SchemaCache) the schema right before delegating to the agent port — so throw a SchemaNotCachedError instead of returning a fabricated schema with a wrong primaryKeyFields/collectionId. - new SchemaNotCachedError (domain error) - agent-client-agent-port test: fallback case becomes a throw assertion Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/adapters/agent-client-agent-port.ts | 18 +++--------------- packages/workflow-executor/src/errors.ts | 11 +++++++++++ .../adapters/agent-client-agent-port.test.ts | 16 +++++----------- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts index 26557cdd77..8ece667f43 100644 --- a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts +++ b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts @@ -19,6 +19,7 @@ import { AgentPortError, AgentProbeError, RecordNotFoundError, + SchemaNotCachedError, WorkflowExecutorError, extractErrorMessage, } from '../errors'; @@ -291,22 +292,9 @@ export default class AgentClientAgentPort implements AgentPort { const cached = this.schemaCache.get(collectionName); if (!cached) { - // eslint-disable-next-line no-console - console.warn( - `[workflow-executor] Schema not found in cache for collection "${collectionName}". ` + - 'Falling back to primaryKeyFields: ["id"]. Call getCollectionSchema first.', - ); + throw new SchemaNotCachedError(collectionName); } - return ( - cached ?? { - collectionName, - collectionId: collectionName, - collectionDisplayName: collectionName, - primaryKeyFields: ['id'], - fields: [], - actions: [], - } - ); + return cached; } } diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 70af3e8abe..3dea5306f6 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -259,6 +259,17 @@ export class AgentPortError extends WorkflowExecutorError { } } +// Invariant guard: the agent port reads a collection's schema (for its primary keys) from the +// cache, which the executor must populate via getCollectionSchema before any record access. +export class SchemaNotCachedError extends WorkflowExecutorError { + constructor(collectionName: string) { + super( + `Collection schema for "${collectionName}" was not loaded before access — call getCollectionSchema first`, + 'An error occurred while accessing your data. Please try again.', + ); + } +} + export class WorkflowPortError extends WorkflowExecutorError { constructor(operation: string, cause: unknown) { super( diff --git a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts index fd2086abf3..219fe3fa4d 100644 --- a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts +++ b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts @@ -4,7 +4,7 @@ import { createRemoteAgentClient } from '@forestadmin/agent-client'; import jsonwebtoken from 'jsonwebtoken'; import AgentClientAgentPort from '../../src/adapters/agent-client-agent-port'; -import { AgentProbeError, RecordNotFoundError } from '../../src/errors'; +import { AgentProbeError, RecordNotFoundError, SchemaNotCachedError } from '../../src/errors'; import SchemaCache from '../../src/schema-cache'; jest.mock('@forestadmin/agent-client', () => ({ @@ -192,17 +192,11 @@ describe('AgentClientAgentPort', () => { }); }); - it('should fallback to pk field "id" when collection is unknown', async () => { - mockCollection.list.mockResolvedValue([{ id: 1 }]); - - const result = await port.getRecord({ collection: 'unknown', id: [1] }, user); - - expect(mockCollection.list).toHaveBeenCalledWith( - expect.objectContaining({ - filters: { field: 'id', operator: 'Equal', value: 1 }, - }), + it('throws SchemaNotCachedError when the collection schema was not loaded first', async () => { + await expect(port.getRecord({ collection: 'unknown', id: [1] }, user)).rejects.toBeInstanceOf( + SchemaNotCachedError, ); - expect(result.collectionName).toBe('unknown'); + expect(mockCollection.list).not.toHaveBeenCalled(); }); }); From 4e12bb8a841456f5a7644021da5f7c52345468dd Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Thu, 4 Jun 2026 21:05:04 +0200 Subject: [PATCH 07/11] refactor(workflow): fold schema cache-or-fetch into getOrLoad Move the cache-or-fetch logic onto SchemaCache as getOrLoad(name, load): the cache owns "check cache, else load via the injected thunk, then store", while staying decoupled from WorkflowPort (the loader closure captures both the port and the per-step runId). Removes the free loadCollectionSchema helper; RecordStepExecutor.getCollectionSchema and AgentWithLog.resolveCollectionId both go through getOrLoad. Also pin the confirmation-flow audit behavior (PRD-442 #2 premature/duplicate symptom, fixed as a consequence of logging at the operation point): a rejected step emits no activity-log entry. Plus direct getOrLoad unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/executors/agent-with-log.ts | 8 ++--- .../src/executors/load-collection-schema.ts | 18 ------------ .../src/executors/record-step-executor.ts | 8 ++--- .../workflow-executor/src/schema-cache.ts | 15 ++++++++++ .../test/executors/agent-with-log.test.ts | 1 + .../update-record-step-executor.test.ts | 9 +++++- .../test/schema-cache.test.ts | 29 +++++++++++++++++++ 7 files changed, 57 insertions(+), 31 deletions(-) delete mode 100644 packages/workflow-executor/src/executors/load-collection-schema.ts diff --git a/packages/workflow-executor/src/executors/agent-with-log.ts b/packages/workflow-executor/src/executors/agent-with-log.ts index bd0a2153a8..6a252d6d28 100644 --- a/packages/workflow-executor/src/executors/agent-with-log.ts +++ b/packages/workflow-executor/src/executors/agent-with-log.ts @@ -13,7 +13,6 @@ import type { StepUser } from '../types/execution-context'; import type { RecordData } from '../types/validated/collection'; import { WorkflowExecutorError } from '../errors'; -import loadCollectionSchema from './load-collection-schema'; // The audit-log target minus renderingId, which audit() stamps centrally. export type AuditTarget = Omit; @@ -133,11 +132,8 @@ export default class AgentWithLog { } private async resolveCollectionId(collectionName: string): Promise { - const schema = await loadCollectionSchema( - this.schemaCache, - this.workflowPort, - this.runId, - collectionName, + const schema = await this.schemaCache.getOrLoad(collectionName, () => + this.workflowPort.getCollectionSchema(collectionName, this.runId), ); return schema.collectionId; diff --git a/packages/workflow-executor/src/executors/load-collection-schema.ts b/packages/workflow-executor/src/executors/load-collection-schema.ts deleted file mode 100644 index 690d7ad131..0000000000 --- a/packages/workflow-executor/src/executors/load-collection-schema.ts +++ /dev/null @@ -1,18 +0,0 @@ -import type { WorkflowPort } from '../ports/workflow-port'; -import type SchemaCache from '../schema-cache'; -import type { CollectionSchema } from '../types/validated/collection'; - -export default async function loadCollectionSchema( - schemaCache: SchemaCache, - workflowPort: WorkflowPort, - runId: string, - collectionName: string, -): Promise { - const cached = schemaCache.get(collectionName); - if (cached) return cached; - - const schema = await workflowPort.getCollectionSchema(collectionName, runId); - schemaCache.set(collectionName, schema); - - return schema; -} diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index 4cbac52c0c..da9b7e0dd2 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -8,7 +8,6 @@ import { z } from 'zod'; import { InvalidAIResponseError, InvalidPreRecordedArgsError, NoRecordsError } from '../errors'; import BaseStepExecutor from './base-step-executor'; -import loadCollectionSchema from './load-collection-schema'; export default abstract class RecordStepExecutor< TStep extends StepDefinition = StepDefinition, @@ -65,11 +64,8 @@ export default abstract class RecordStepExecutor< } protected getCollectionSchema(collectionName: string): Promise { - return loadCollectionSchema( - this.context.schemaCache, - this.context.workflowPort, - this.context.runId, - collectionName, + return this.context.schemaCache.getOrLoad(collectionName, () => + this.context.workflowPort.getCollectionSchema(collectionName, this.context.runId), ); } diff --git a/packages/workflow-executor/src/schema-cache.ts b/packages/workflow-executor/src/schema-cache.ts index d6f04056e7..fa51269622 100644 --- a/packages/workflow-executor/src/schema-cache.ts +++ b/packages/workflow-executor/src/schema-cache.ts @@ -30,6 +30,21 @@ export default class SchemaCache { this.store.set(collectionName, { schema, fetchedAt: this.now() }); } + // Returns the cached schema, or loads it via `load`, caches it, and returns it. + // `load` is injected so the cache stays decoupled from the orchestrator port. + async getOrLoad( + collectionName: string, + load: () => Promise, + ): Promise { + const cached = this.get(collectionName); + if (cached) return cached; + + const schema = await load(); + this.set(collectionName, schema); + + return schema; + } + // Yields non-expired entries; deletes stale ones along the way. *[Symbol.iterator](): IterableIterator<[string, CollectionSchema]> { const now = this.now(); diff --git a/packages/workflow-executor/test/executors/agent-with-log.test.ts b/packages/workflow-executor/test/executors/agent-with-log.test.ts index 279eeb4cb5..044dab76fb 100644 --- a/packages/workflow-executor/test/executors/agent-with-log.test.ts +++ b/packages/workflow-executor/test/executors/agent-with-log.test.ts @@ -58,6 +58,7 @@ function makeDeps(overrides: Partial = {}) { const schemaCache = { get: jest.fn().mockReturnValue(makeSchema()), set: jest.fn(), + getOrLoad: jest.fn().mockResolvedValue(makeSchema()), } as unknown as SchemaCache; const workflowPort = { getCollectionSchema: jest.fn().mockResolvedValue(makeSchema()), diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index 08b392839c..11b9d23257 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -553,13 +553,20 @@ describe('UpdateRecordStepExecutor', () => { const runStore = makeMockRunStore({ getStepExecutions: jest.fn().mockResolvedValue([execution]), }); - const context = makeContext({ agentPort, runStore }); + const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), + }; + const context = makeContext({ agentPort, runStore, activityLogPort }); const executor = new UpdateRecordStepExecutor(context); const result = await executor.execute(); expect(result.stepOutcome.status).toBe('success'); expect(agentPort.updateRecord).not.toHaveBeenCalled(); + // No side effect happened → no audit-log entry (PRD-442 #2: no premature/duplicate log). + expect(activityLogPort.createPending).not.toHaveBeenCalled(); expect(runStore.saveStepExecution).toHaveBeenCalledWith( 'run-1', expect.objectContaining({ diff --git a/packages/workflow-executor/test/schema-cache.test.ts b/packages/workflow-executor/test/schema-cache.test.ts index a65c152176..a8cf726d20 100644 --- a/packages/workflow-executor/test/schema-cache.test.ts +++ b/packages/workflow-executor/test/schema-cache.test.ts @@ -129,4 +129,33 @@ describe('SchemaCache', () => { expect([...cache]).toHaveLength(0); }); }); + + describe('getOrLoad', () => { + it('returns the cached schema without invoking the loader on a hit', async () => { + const cache = new SchemaCache(); + const schema = makeSchema('customers'); + cache.set('customers', schema); + const load = jest.fn(); + + const result = await cache.getOrLoad('customers', load); + + expect(result).toBe(schema); + expect(load).not.toHaveBeenCalled(); + }); + + it('loads, caches, and returns the schema on a miss', async () => { + const cache = new SchemaCache(); + const schema = makeSchema('orders'); + const load = jest.fn().mockResolvedValue(schema); + + const result = await cache.getOrLoad('orders', load); + + expect(result).toBe(schema); + expect(load).toHaveBeenCalledTimes(1); + // subsequent call hits the cache — loader not invoked again + await cache.getOrLoad('orders', load); + expect(load).toHaveBeenCalledTimes(1); + expect(cache.get('orders')).toBe(schema); + }); + }); }); From 453f88a07de02373df79917c38d6d611473669bc Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Fri, 5 Jun 2026 15:46:49 +0200 Subject: [PATCH 08/11] refactor(workflow): replace SchemaCache.getOrLoad with per-run SchemaResolver Both getOrLoad callers rewrote the same loader closure, only because the process-wide SchemaCache singleton can't hold the per-run runId that getCollectionSchema needs. Bind (schemaCache, workflowPort, runId) once in a SchemaResolver instead, and make SchemaCache a pure store again. The resolver feeds the same shared cache AgentClientAgentPort reads from, preserving the SchemaNotCachedError invariant. AgentWithLogDeps drops from 6 to 4 fields. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/executors/agent-with-log.ts | 19 ++---- .../src/executors/base-step-executor.ts | 4 +- .../src/executors/record-step-executor.ts | 4 +- .../src/executors/step-executor-factory.ts | 2 + packages/workflow-executor/src/index.ts | 1 + .../workflow-executor/src/schema-cache.ts | 15 ----- .../workflow-executor/src/schema-resolver.ts | 29 +++++++++ .../src/types/execution-context.ts | 2 + .../test/executors/agent-with-log.test.ts | 20 ++---- .../test/executors/base-step-executor.test.ts | 12 +++- .../executors/condition-step-executor.test.ts | 12 +++- .../executors/guidance-step-executor.test.ts | 12 +++- .../load-related-record-step-executor.test.ts | 12 +++- .../test/executors/mcp-step-executor.test.ts | 12 +++- .../read-record-step-executor.test.ts | 12 +++- ...rigger-record-action-step-executor.test.ts | 12 +++- .../update-record-step-executor.test.ts | 12 +++- .../test/schema-cache.test.ts | 29 --------- .../test/schema-resolver.test.ts | 65 +++++++++++++++++++ 19 files changed, 184 insertions(+), 102 deletions(-) create mode 100644 packages/workflow-executor/src/schema-resolver.ts create mode 100644 packages/workflow-executor/test/schema-resolver.test.ts diff --git a/packages/workflow-executor/src/executors/agent-with-log.ts b/packages/workflow-executor/src/executors/agent-with-log.ts index 6a252d6d28..384cc55820 100644 --- a/packages/workflow-executor/src/executors/agent-with-log.ts +++ b/packages/workflow-executor/src/executors/agent-with-log.ts @@ -7,8 +7,7 @@ import type { GetSingleRelatedDataQuery, UpdateRecordQuery, } from '../ports/agent-port'; -import type { WorkflowPort } from '../ports/workflow-port'; -import type SchemaCache from '../schema-cache'; +import type SchemaResolver from '../schema-resolver'; import type { StepUser } from '../types/execution-context'; import type { RecordData } from '../types/validated/collection'; @@ -22,10 +21,8 @@ type WriteOptions = { beforeCall: () => Promise }; export interface AgentWithLogDeps { agentPort: AgentPort; activityLogPort: ActivityLogPort; - workflowPort: WorkflowPort; - schemaCache: SchemaCache; + schemaResolver: SchemaResolver; user: StepUser; - runId: string; } // Wraps AgentPort and emits an activity-log entry around each data-access call @@ -37,18 +34,14 @@ export interface AgentWithLogDeps { export default class AgentWithLog { private readonly agentPort: AgentPort; private readonly activityLogPort: ActivityLogPort; - private readonly workflowPort: WorkflowPort; - private readonly schemaCache: SchemaCache; + private readonly schemaResolver: SchemaResolver; private readonly user: StepUser; - private readonly runId: string; constructor(deps: AgentWithLogDeps) { this.agentPort = deps.agentPort; this.activityLogPort = deps.activityLogPort; - this.workflowPort = deps.workflowPort; - this.schemaCache = deps.schemaCache; + this.schemaResolver = deps.schemaResolver; this.user = deps.user; - this.runId = deps.runId; } async getRecord(query: GetRecordQuery): Promise { @@ -132,9 +125,7 @@ export default class AgentWithLog { } private async resolveCollectionId(collectionName: string): Promise { - const schema = await this.schemaCache.getOrLoad(collectionName, () => - this.workflowPort.getCollectionSchema(collectionName, this.runId), - ); + const schema = await this.schemaResolver.resolve(collectionName); return schema.collectionId; } diff --git a/packages/workflow-executor/src/executors/base-step-executor.ts b/packages/workflow-executor/src/executors/base-step-executor.ts index 3cc0f521e8..d4e6cec1a5 100644 --- a/packages/workflow-executor/src/executors/base-step-executor.ts +++ b/packages/workflow-executor/src/executors/base-step-executor.ts @@ -46,10 +46,8 @@ export default abstract class BaseStepExecutor { - return this.context.schemaCache.getOrLoad(collectionName, () => - this.context.workflowPort.getCollectionSchema(collectionName, this.context.runId), - ); + return this.context.schemaResolver.resolve(collectionName); } protected findField(schema: CollectionSchema, name: string): FieldSchema | undefined { diff --git a/packages/workflow-executor/src/executors/step-executor-factory.ts b/packages/workflow-executor/src/executors/step-executor-factory.ts index 3fd1533df9..9cc5fd06ca 100644 --- a/packages/workflow-executor/src/executors/step-executor-factory.ts +++ b/packages/workflow-executor/src/executors/step-executor-factory.ts @@ -23,6 +23,7 @@ import type { } from '../types/validated/step-definition'; import { StepStateError, causeMessage, extractErrorMessage } from '../errors'; +import SchemaResolver from '../schema-resolver'; import ConditionStepExecutor from './condition-step-executor'; import GuidanceStepExecutor from './guidance-step-executor'; import LoadRelatedRecordStepExecutor from './load-related-record-step-executor'; @@ -138,6 +139,7 @@ export default class StepExecutorFactory { workflowPort: cfg.workflowPort, runStore: cfg.runStore, schemaCache: cfg.schemaCache, + schemaResolver: new SchemaResolver(cfg.schemaCache, cfg.workflowPort, step.runId), logger: cfg.logger, incomingPendingData, stepTimeoutMs: cfg.stepTimeoutMs, diff --git a/packages/workflow-executor/src/index.ts b/packages/workflow-executor/src/index.ts index 14435b4850..7fa349a083 100644 --- a/packages/workflow-executor/src/index.ts +++ b/packages/workflow-executor/src/index.ts @@ -122,6 +122,7 @@ export { default as Runner } from './runner'; export type { RunnerConfig, RunnerState } from './runner'; export { default as validateSecrets } from './validate-secrets'; export { default as SchemaCache } from './schema-cache'; +export { default as SchemaResolver } from './schema-resolver'; export { default as InMemoryStore } from './stores/in-memory-store'; export { default as DatabaseStore } from './stores/database-store'; export type { DatabaseStoreOptions } from './stores/database-store'; diff --git a/packages/workflow-executor/src/schema-cache.ts b/packages/workflow-executor/src/schema-cache.ts index fa51269622..d6f04056e7 100644 --- a/packages/workflow-executor/src/schema-cache.ts +++ b/packages/workflow-executor/src/schema-cache.ts @@ -30,21 +30,6 @@ export default class SchemaCache { this.store.set(collectionName, { schema, fetchedAt: this.now() }); } - // Returns the cached schema, or loads it via `load`, caches it, and returns it. - // `load` is injected so the cache stays decoupled from the orchestrator port. - async getOrLoad( - collectionName: string, - load: () => Promise, - ): Promise { - const cached = this.get(collectionName); - if (cached) return cached; - - const schema = await load(); - this.set(collectionName, schema); - - return schema; - } - // Yields non-expired entries; deletes stale ones along the way. *[Symbol.iterator](): IterableIterator<[string, CollectionSchema]> { const now = this.now(); diff --git a/packages/workflow-executor/src/schema-resolver.ts b/packages/workflow-executor/src/schema-resolver.ts new file mode 100644 index 0000000000..11dc9391d0 --- /dev/null +++ b/packages/workflow-executor/src/schema-resolver.ts @@ -0,0 +1,29 @@ +import type { WorkflowPort } from './ports/workflow-port'; +import type SchemaCache from './schema-cache'; +import type { CollectionSchema } from './types/validated/collection'; + +// Per-run schema resolution: binds the shared SchemaCache, the orchestrator port and the +// current runId once, so callers never thread a loader. Writes into the SAME SchemaCache +// instance AgentClientAgentPort reads from (get/iterate) — that shared instance is the +// invariant that keeps the agent-port's SchemaNotCachedError guard unreachable in normal flow. +export default class SchemaResolver { + private readonly cache: SchemaCache; + private readonly workflowPort: WorkflowPort; + private readonly runId: string; + + constructor(cache: SchemaCache, workflowPort: WorkflowPort, runId: string) { + this.cache = cache; + this.workflowPort = workflowPort; + this.runId = runId; + } + + async resolve(collectionName: string): Promise { + const cached = this.cache.get(collectionName); + if (cached) return cached; + + const schema = await this.workflowPort.getCollectionSchema(collectionName, this.runId); + this.cache.set(collectionName, schema); + + return schema; + } +} diff --git a/packages/workflow-executor/src/types/execution-context.ts b/packages/workflow-executor/src/types/execution-context.ts index 4a1ca06128..8169fface7 100644 --- a/packages/workflow-executor/src/types/execution-context.ts +++ b/packages/workflow-executor/src/types/execution-context.ts @@ -6,6 +6,7 @@ import type { Logger } from '../ports/logger-port'; import type { RunStore } from '../ports/run-store'; import type { WorkflowPort } from '../ports/workflow-port'; import type SchemaCache from '../schema-cache'; +import type SchemaResolver from '../schema-resolver'; import type { RecordRef } from './validated/collection'; import type { AvailableStepExecution, Step, StepUser } from './validated/execution'; import type { StepDefinition } from './validated/step-definition'; @@ -37,6 +38,7 @@ export interface ExecutionContext readonly runStore: RunStore; readonly user: StepUser; readonly schemaCache: SchemaCache; + readonly schemaResolver: SchemaResolver; readonly previousSteps: ReadonlyArray>; readonly logger: Logger; readonly incomingPendingData?: unknown; diff --git a/packages/workflow-executor/test/executors/agent-with-log.test.ts b/packages/workflow-executor/test/executors/agent-with-log.test.ts index 044dab76fb..a3f63920bf 100644 --- a/packages/workflow-executor/test/executors/agent-with-log.test.ts +++ b/packages/workflow-executor/test/executors/agent-with-log.test.ts @@ -1,7 +1,6 @@ import type { AgentWithLogDeps } from '../../src/executors/agent-with-log'; import type { AgentPort } from '../../src/ports/agent-port'; -import type { WorkflowPort } from '../../src/ports/workflow-port'; -import type SchemaCache from '../../src/schema-cache'; +import type SchemaResolver from '../../src/schema-resolver'; import type { StepUser } from '../../src/types/execution-context'; import type { CollectionSchema } from '../../src/types/validated/collection'; @@ -55,26 +54,19 @@ function makeDeps(overrides: Partial = {}) { getSingleRelatedData: jest.fn().mockResolvedValue(null), executeAction: jest.fn().mockResolvedValue({ ok: true }), } as unknown as AgentPort; - const schemaCache = { - get: jest.fn().mockReturnValue(makeSchema()), - set: jest.fn(), - getOrLoad: jest.fn().mockResolvedValue(makeSchema()), - } as unknown as SchemaCache; - const workflowPort = { - getCollectionSchema: jest.fn().mockResolvedValue(makeSchema()), - } as unknown as WorkflowPort; + const schemaResolver = { + resolve: jest.fn().mockResolvedValue(makeSchema()), + } as unknown as SchemaResolver; const deps = { agentPort, activityLogPort, - workflowPort, - schemaCache, + schemaResolver, user: makeUser(), - runId: 'run-1', ...overrides, }; - return { deps, agentPort, activityLogPort, schemaCache, workflowPort }; + return { deps, agentPort, activityLogPort, schemaResolver }; } describe('AgentWithLog', () => { diff --git a/packages/workflow-executor/test/executors/base-step-executor.test.ts b/packages/workflow-executor/test/executors/base-step-executor.test.ts index 8f1479afe2..fd7f17e352 100644 --- a/packages/workflow-executor/test/executors/base-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/base-step-executor.test.ts @@ -21,6 +21,7 @@ import { } from '../../src/errors'; import BaseStepExecutor from '../../src/executors/base-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; /** Concrete subclass that exposes protected methods for testing. */ @@ -103,8 +104,12 @@ function makeMockActivityLogPort(): ExecutionContext['activityLogPort'] { } function makeContext(overrides: Partial = {}): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? ({} as ExecutionContext['workflowPort']); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'step-0', stepIndex: 0, collectionId: 'col-1', @@ -121,7 +126,7 @@ function makeContext(overrides: Partial = {}): ExecutionContex }, model: {} as ExecutionContext['model'], agentPort: {} as ExecutionContext['agentPort'], - workflowPort: {} as ExecutionContext['workflowPort'], + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -134,7 +139,8 @@ function makeContext(overrides: Partial = {}): ExecutionContex permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: makeMockLogger(), diff --git a/packages/workflow-executor/test/executors/condition-step-executor.test.ts b/packages/workflow-executor/test/executors/condition-step-executor.test.ts index 83135f3378..aad401e164 100644 --- a/packages/workflow-executor/test/executors/condition-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/condition-step-executor.test.ts @@ -7,6 +7,7 @@ import type { ConditionStepOutcome } from '../../src/types/validated/step-outcom import { RunStorePortError } from '../../src/errors'; import ConditionStepExecutor from '../../src/executors/condition-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; function makeStep(overrides: Partial = {}): ConditionStepDefinition { @@ -44,8 +45,12 @@ function makeMockModel(toolCallArgs?: Record) { function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? ({} as ExecutionContext['workflowPort']); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'cond-1', stepIndex: 0, collectionId: 'col-1', @@ -57,7 +62,7 @@ function makeContext( stepDefinition: makeStep(), model: makeMockModel().model, agentPort: {} as ExecutionContext['agentPort'], - workflowPort: {} as ExecutionContext['workflowPort'], + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -70,7 +75,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/guidance-step-executor.test.ts b/packages/workflow-executor/test/executors/guidance-step-executor.test.ts index aa5d0b09ee..a07979318c 100644 --- a/packages/workflow-executor/test/executors/guidance-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/guidance-step-executor.test.ts @@ -6,6 +6,7 @@ import type { GuidanceStepOutcome } from '../../src/types/validated/step-outcome import GuidanceStepExecutor from '../../src/executors/guidance-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; function makeMockRunStore(overrides: Partial = {}): RunStore { @@ -21,8 +22,12 @@ function makeMockRunStore(overrides: Partial = {}): RunStore { function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? ({} as ExecutionContext['workflowPort']); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'guidance-1', stepIndex: 0, collectionId: 'col-1', @@ -34,7 +39,7 @@ function makeContext( stepDefinition: { type: StepType.Guidance, executionType: StepExecutionMode.Manual }, model: {} as ExecutionContext['model'], agentPort: {} as ExecutionContext['agentPort'], - workflowPort: {} as ExecutionContext['workflowPort'], + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -47,7 +52,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts index 66ed147ee8..6268511415 100644 --- a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts @@ -9,6 +9,7 @@ import type { LoadRelatedRecordStepDefinition } from '../../src/types/validated/ import { AgentPortError, RunStorePortError } from '../../src/errors'; import LoadRelatedRecordStepExecutor from '../../src/executors/load-related-record-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; function makeStep( @@ -141,8 +142,12 @@ function makeMockModel(toolCallArgs?: Record, toolName = 'selec function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'load-1', stepIndex: 0, collectionId: 'col-1', @@ -150,7 +155,7 @@ function makeContext( stepDefinition: makeStep(), model: makeMockModel({ relationName: 'Order', reasoning: 'User requested order' }).model, agentPort: makeMockAgentPort(), - workflowPort: makeMockWorkflowPort(), + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -163,7 +168,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index b684186621..913a301e86 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -9,6 +9,7 @@ import RemoteTool from '@forestadmin/ai-proxy/src/remote-tool'; import { RunStorePortError, StepStateError } from '../../src/errors'; import McpStepExecutor from '../../src/executors/mcp-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; // --------------------------------------------------------------------------- @@ -87,8 +88,12 @@ function makeMockModel(toolName: string, toolArgs: Record) { function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'mcp-1', stepIndex: 0, collectionId: 'col-1', @@ -101,7 +106,7 @@ function makeContext( getRelatedData: jest.fn(), executeAction: jest.fn(), } as unknown as ExecutionContext['agentPort'], - workflowPort: makeMockWorkflowPort(), + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -114,7 +119,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts index 160a0c6a99..a62019f29e 100644 --- a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts @@ -8,6 +8,7 @@ import type { ReadRecordStepDefinition } from '../../src/types/validated/step-de import { AgentPortError, NoRecordsError, RecordNotFoundError } from '../../src/errors'; import ReadRecordStepExecutor from '../../src/executors/read-record-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; function makeStep(overrides: Partial = {}): ReadRecordStepDefinition { @@ -108,8 +109,12 @@ function makeMockModel( function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'read-1', stepIndex: 0, collectionId: 'col-1', @@ -117,7 +122,7 @@ function makeContext( stepDefinition: makeStep(), model: makeMockModel({ fieldNames: ['email'] }).model, agentPort: makeMockAgentPort(), - workflowPort: makeMockWorkflowPort(), + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -130,7 +135,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index 436ba68a19..18475ceb6c 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -9,6 +9,7 @@ import type { TriggerActionStepDefinition } from '../../src/types/validated/step import { AgentPortError, RunStorePortError, StepStateError } from '../../src/errors'; import TriggerRecordActionStepExecutor from '../../src/executors/trigger-record-action-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; function makeStep( @@ -107,8 +108,12 @@ function makeMockModel(toolCallArgs?: Record, toolName = 'selec function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'trigger-1', stepIndex: 0, collectionId: 'col-1', @@ -119,7 +124,7 @@ function makeContext( reasoning: 'User requested welcome email', }).model, agentPort: makeMockAgentPort(), - workflowPort: makeMockWorkflowPort(), + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -132,7 +137,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index 11b9d23257..717ff760a3 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -14,6 +14,7 @@ import { } from '../../src/errors'; import UpdateRecordStepExecutor from '../../src/executors/update-record-step-executor'; import SchemaCache from '../../src/schema-cache'; +import SchemaResolver from '../../src/schema-resolver'; import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; function makeStep(overrides: Partial = {}): UpdateRecordStepDefinition { @@ -110,8 +111,12 @@ function makeMockModel(toolCallArgs?: Record, toolName = 'updat function makeContext( overrides: Partial> = {}, ): ExecutionContext { + const runId = overrides.runId ?? 'run-1'; + const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); + const schemaCache = overrides.schemaCache ?? new SchemaCache(); + return { - runId: 'run-1', + runId, stepId: 'update-1', stepIndex: 0, collectionId: 'col-1', @@ -121,7 +126,7 @@ function makeContext( input: { fieldName: 'Status', value: 'active', reasoning: 'User requested status change' }, }).model, agentPort: makeMockAgentPort(), - workflowPort: makeMockWorkflowPort(), + workflowPort, runStore: makeMockRunStore(), user: { id: 1, @@ -134,7 +139,8 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache: new SchemaCache(), + schemaCache, + schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/schema-cache.test.ts b/packages/workflow-executor/test/schema-cache.test.ts index a8cf726d20..a65c152176 100644 --- a/packages/workflow-executor/test/schema-cache.test.ts +++ b/packages/workflow-executor/test/schema-cache.test.ts @@ -129,33 +129,4 @@ describe('SchemaCache', () => { expect([...cache]).toHaveLength(0); }); }); - - describe('getOrLoad', () => { - it('returns the cached schema without invoking the loader on a hit', async () => { - const cache = new SchemaCache(); - const schema = makeSchema('customers'); - cache.set('customers', schema); - const load = jest.fn(); - - const result = await cache.getOrLoad('customers', load); - - expect(result).toBe(schema); - expect(load).not.toHaveBeenCalled(); - }); - - it('loads, caches, and returns the schema on a miss', async () => { - const cache = new SchemaCache(); - const schema = makeSchema('orders'); - const load = jest.fn().mockResolvedValue(schema); - - const result = await cache.getOrLoad('orders', load); - - expect(result).toBe(schema); - expect(load).toHaveBeenCalledTimes(1); - // subsequent call hits the cache — loader not invoked again - await cache.getOrLoad('orders', load); - expect(load).toHaveBeenCalledTimes(1); - expect(cache.get('orders')).toBe(schema); - }); - }); }); diff --git a/packages/workflow-executor/test/schema-resolver.test.ts b/packages/workflow-executor/test/schema-resolver.test.ts new file mode 100644 index 0000000000..dd338d766a --- /dev/null +++ b/packages/workflow-executor/test/schema-resolver.test.ts @@ -0,0 +1,65 @@ +import type { WorkflowPort } from '../src/ports/workflow-port'; +import type { CollectionSchema } from '../src/types/validated/collection'; + +import SchemaCache from '../src/schema-cache'; +import SchemaResolver from '../src/schema-resolver'; + +function makeSchema(collectionName: string): CollectionSchema { + return { + collectionName, + collectionId: `col-${collectionName}`, + collectionDisplayName: collectionName, + primaryKeyFields: ['id'], + fields: [], + actions: [], + }; +} + +function makeWorkflowPort(schema: CollectionSchema) { + return { + getCollectionSchema: jest.fn().mockResolvedValue(schema), + } as unknown as WorkflowPort & { getCollectionSchema: jest.Mock }; +} + +describe('SchemaResolver', () => { + it('returns the cached schema without calling the orchestrator on a hit', async () => { + const cache = new SchemaCache(); + const schema = makeSchema('customers'); + cache.set('customers', schema); + const workflowPort = makeWorkflowPort(makeSchema('other')); + const resolver = new SchemaResolver(cache, workflowPort, 'run-1'); + + const result = await resolver.resolve('customers'); + + expect(result).toBe(schema); + expect(workflowPort.getCollectionSchema).not.toHaveBeenCalled(); + }); + + it('fetches with the bound runId, caches, and skips the fetch on a subsequent hit', async () => { + const cache = new SchemaCache(); + const schema = makeSchema('orders'); + const workflowPort = makeWorkflowPort(schema); + const resolver = new SchemaResolver(cache, workflowPort, 'run-42'); + + const result = await resolver.resolve('orders'); + + expect(result).toBe(schema); + expect(workflowPort.getCollectionSchema).toHaveBeenCalledTimes(1); + expect(workflowPort.getCollectionSchema).toHaveBeenCalledWith('orders', 'run-42'); + + // second call hits the cache — orchestrator not queried again + await resolver.resolve('orders'); + expect(workflowPort.getCollectionSchema).toHaveBeenCalledTimes(1); + }); + + it('writes the fetched schema into the shared cache (read back by other consumers)', async () => { + const cache = new SchemaCache(); + const schema = makeSchema('products'); + const resolver = new SchemaResolver(cache, makeWorkflowPort(schema), 'run-1'); + + await resolver.resolve('products'); + + // The same shared SchemaCache instance is what AgentClientAgentPort reads via .get(). + expect(cache.get('products')).toBe(schema); + }); +}); From 73dd0152e7413895be34af8b6f90eff272bf1974 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Fri, 5 Jun 2026 15:53:35 +0200 Subject: [PATCH 09/11] refactor(workflow): drop errorMessage from ActivityLogPort.markFailed The step error is already logged/surfaced by base-step-executor when rethrown, so threading it into markFailed only enriched a rare double-fault diagnostic. Drop the param: markFailed(handle) is now symmetric with markSucceeded, and AgentWithLog no longer computes a userMessage/'Unexpected error' string. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../forestadmin-client-activity-log-port.ts | 3 +-- .../src/executors/agent-with-log.ts | 8 +++----- .../src/ports/activity-log-port.ts | 5 +++-- .../forestadmin-client-activity-log-port.test.ts | 12 ++++++------ .../test/executors/agent-with-log.test.ts | 14 ++++---------- 5 files changed, 17 insertions(+), 25 deletions(-) diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index f93be0a977..feb270f96f 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -73,7 +73,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort }); } - async markFailed(handle: ActivityLogHandle, errorMessage: string): Promise { + async markFailed(handle: ActivityLogHandle): Promise { return this.drainer.track(async () => { try { await withRetry( @@ -89,7 +89,6 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } catch (err) { this.logger.error('activity log mark-as-failed failed', { handleId: handle.id, - stepErrorMessage: errorMessage, error: extractErrorMessage(err), }); } diff --git a/packages/workflow-executor/src/executors/agent-with-log.ts b/packages/workflow-executor/src/executors/agent-with-log.ts index 384cc55820..e50ffa4c5f 100644 --- a/packages/workflow-executor/src/executors/agent-with-log.ts +++ b/packages/workflow-executor/src/executors/agent-with-log.ts @@ -11,8 +11,6 @@ import type SchemaResolver from '../schema-resolver'; import type { StepUser } from '../types/execution-context'; import type { RecordData } from '../types/validated/collection'; -import { WorkflowExecutorError } from '../errors'; - // The audit-log target minus renderingId, which audit() stamps centrally. export type AuditTarget = Omit; @@ -117,9 +115,9 @@ export default class AgentWithLog { return result; } catch (err) { - const errorMessage = - err instanceof WorkflowExecutorError ? err.userMessage : 'Unexpected error'; - void this.activityLogPort.markFailed(handle, errorMessage); + // The step error is logged/surfaced by base-step-executor when rethrown, so the audit + // transition only needs the handle. + void this.activityLogPort.markFailed(handle); throw err; } } diff --git a/packages/workflow-executor/src/ports/activity-log-port.ts b/packages/workflow-executor/src/ports/activity-log-port.ts index c1c1c86428..439489c831 100644 --- a/packages/workflow-executor/src/ports/activity-log-port.ts +++ b/packages/workflow-executor/src/ports/activity-log-port.ts @@ -21,8 +21,9 @@ export interface ActivityLogHandle { export interface ActivityLogPort { createPending(args: CreateActivityLogArgs): Promise; markSucceeded(handle: ActivityLogHandle): Promise; - // errorMessage is for local diagnostics only — the server status endpoint accepts just { status }. - markFailed(handle: ActivityLogHandle, errorMessage: string): Promise; + // The server status endpoint accepts just { status }; the step error itself is logged by + // base-step-executor when the failure is rethrown, so no error message is threaded here. + markFailed(handle: ActivityLogHandle): Promise; } // Produces per-run ActivityLogPort instances and exposes drain() at the process level so the diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 97a1af1b20..dd11300c76 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -274,7 +274,7 @@ describe('ForestadminClientActivityLogPort', () => { .mockResolvedValueOnce(undefined); const port = makePort(service); - const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step failed'); + const promise = port.markFailed({ id: 'log-1', index: '0' }); await jest.advanceTimersByTimeAsync(100); await promise; @@ -289,18 +289,18 @@ describe('ForestadminClientActivityLogPort', () => { ); }); - it('swallows errors after retries are exhausted (fire-and-forget) and logs the step error message', async () => { + it('swallows errors after retries are exhausted (fire-and-forget) and logs the failure', async () => { const service = makeService(); service.updateActivityLogStatus.mockRejectedValue(makeHttpError(503)); const logger = makeLogger(); const port = makePort(service, { logger }); - const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step-error-msg'); + const promise = port.markFailed({ id: 'log-1', index: '0' }); await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( 'activity log mark-as-failed failed', - expect.objectContaining({ handleId: 'log-1', stepErrorMessage: 'step-error-msg' }), + expect.objectContaining({ handleId: 'log-1' }), ); }); @@ -311,7 +311,7 @@ describe('ForestadminClientActivityLogPort', () => { .mockResolvedValueOnce(undefined); const port = makePort(service); - const promise = port.markFailed({ id: 'log-1', index: '0' }, 'step failed'); + const promise = port.markFailed({ id: 'log-1', index: '0' }); await jest.advanceTimersByTimeAsync(100); await expect(promise).resolves.toBeUndefined(); expect(service.updateActivityLogStatus).toHaveBeenCalledTimes(2); @@ -359,7 +359,7 @@ describe('ForestadminClientActivityLogPort', () => { const drainer = new ActivityLogDrainer(); const port = makePort(service, { drainer }); - const markPromise = port.markFailed({ id: 'log-1', index: '0' }, 'step failed'); + const markPromise = port.markFailed({ id: 'log-1', index: '0' }); let drainResolved = false; const drainPromise = drainer.drain().then(() => { diff --git a/packages/workflow-executor/test/executors/agent-with-log.test.ts b/packages/workflow-executor/test/executors/agent-with-log.test.ts index a3f63920bf..5f3919da40 100644 --- a/packages/workflow-executor/test/executors/agent-with-log.test.ts +++ b/packages/workflow-executor/test/executors/agent-with-log.test.ts @@ -175,7 +175,7 @@ describe('AgentWithLog', () => { }); describe('failure marking', () => { - it('marks failed with the userMessage when the operation throws a WorkflowExecutorError', async () => { + it('marks failed (not succeeded) and rethrows the original WorkflowExecutorError', async () => { const { deps, agentPort, activityLogPort } = makeDeps(); (agentPort.updateRecord as jest.Mock).mockRejectedValue(new NoRecordsError()); const agent = new AgentWithLog(deps); @@ -187,24 +187,18 @@ describe('AgentWithLog', () => { ), ).rejects.toBeInstanceOf(NoRecordsError); - expect(activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'No records available', - ); + expect(activityLogPort.markFailed).toHaveBeenCalledWith({ id: 'log-1', index: '0' }); expect(activityLogPort.markSucceeded).not.toHaveBeenCalled(); }); - it("marks failed with 'Unexpected error' for a non-WorkflowExecutorError", async () => { + it('marks failed and rethrows a non-WorkflowExecutorError unchanged', async () => { const { deps, agentPort, activityLogPort } = makeDeps(); (agentPort.getRecord as jest.Mock).mockRejectedValue(new Error('boom')); const agent = new AgentWithLog(deps); await expect(agent.getRecord({ collection: 'customers', id: [42] })).rejects.toThrow('boom'); - expect(activityLogPort.markFailed).toHaveBeenCalledWith( - { id: 'log-1', index: '0' }, - 'Unexpected error', - ); + expect(activityLogPort.markFailed).toHaveBeenCalledWith({ id: 'log-1', index: '0' }); }); }); From 36295706a80886b53540f5d5fa03b6e37ad49e41 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Fri, 5 Jun 2026 15:56:05 +0200 Subject: [PATCH 10/11] test(workflow): cover beforeCall-throws in AgentWithLog audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When beforeCall (the write-ahead marker save) throws, the datasource write must not run and the pending entry is resolved to failed — never left orphan-pending. Documents the intended behavior raised in review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/executors/agent-with-log.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/workflow-executor/test/executors/agent-with-log.test.ts b/packages/workflow-executor/test/executors/agent-with-log.test.ts index 5f3919da40..f42e193a10 100644 --- a/packages/workflow-executor/test/executors/agent-with-log.test.ts +++ b/packages/workflow-executor/test/executors/agent-with-log.test.ts @@ -172,6 +172,29 @@ describe('AgentWithLog', () => { expect(beforeCall).not.toHaveBeenCalled(); expect(agentPort.updateRecord).not.toHaveBeenCalled(); }); + + it('marks failed and rethrows when beforeCall throws — the side effect never runs', async () => { + // beforeCall persists the write-ahead "executing" marker. If that save fails, the datasource + // write must not run, and the already-created pending entry is resolved to failed (never left + // orphan-pending). The record is genuinely untouched, which "failed" correctly conveys. + const { deps, agentPort, activityLogPort } = makeDeps(); + const agent = new AgentWithLog(deps); + + await expect( + agent.updateRecord( + { collection: 'customers', id: [42], values: { name: 'X' } }, + { + beforeCall: async () => { + throw new Error('marker save failed'); + }, + }, + ), + ).rejects.toThrow('marker save failed'); + + expect(agentPort.updateRecord).not.toHaveBeenCalled(); + expect(activityLogPort.markFailed).toHaveBeenCalledWith({ id: 'log-1', index: '0' }); + expect(activityLogPort.markSucceeded).not.toHaveBeenCalled(); + }); }); describe('failure marking', () => { From 8bb1ba6a485521c109aa6f728116b5308e144b64 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Fri, 5 Jun 2026 16:11:00 +0200 Subject: [PATCH 11/11] refactor(workflow): drop dead schemaCache field from ExecutionContext MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the SchemaResolver extraction no executor reads context.schemaCache — all schema access goes through context.schemaResolver. Exposing the raw cache next to the resolver advertised a second, unsafe path (a direct .get() bypasses the resolver's read-through fetch, re-opening the SchemaNotCachedError footgun). The factory keeps cfg.schemaCache to wire the resolver and the agent-port. Also soften the SchemaResolver header comment (the guard is unreachable under normal TTLs, not absolutely) and note markFailed must be called on a rethrow path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflow-executor/src/executors/step-executor-factory.ts | 1 - packages/workflow-executor/src/ports/activity-log-port.ts | 3 ++- packages/workflow-executor/src/schema-resolver.ts | 5 +++-- packages/workflow-executor/src/types/execution-context.ts | 2 -- .../test/executors/base-step-executor.test.ts | 3 +-- .../test/executors/condition-step-executor.test.ts | 3 +-- .../test/executors/guidance-step-executor.test.ts | 3 +-- .../test/executors/load-related-record-step-executor.test.ts | 3 +-- .../test/executors/mcp-step-executor.test.ts | 3 +-- .../test/executors/read-record-step-executor.test.ts | 3 +-- .../executors/trigger-record-action-step-executor.test.ts | 3 +-- .../test/executors/update-record-step-executor.test.ts | 3 +-- 12 files changed, 13 insertions(+), 22 deletions(-) diff --git a/packages/workflow-executor/src/executors/step-executor-factory.ts b/packages/workflow-executor/src/executors/step-executor-factory.ts index 9cc5fd06ca..75e360d55b 100644 --- a/packages/workflow-executor/src/executors/step-executor-factory.ts +++ b/packages/workflow-executor/src/executors/step-executor-factory.ts @@ -138,7 +138,6 @@ export default class StepExecutorFactory { agentPort: cfg.agentPort, workflowPort: cfg.workflowPort, runStore: cfg.runStore, - schemaCache: cfg.schemaCache, schemaResolver: new SchemaResolver(cfg.schemaCache, cfg.workflowPort, step.runId), logger: cfg.logger, incomingPendingData, diff --git a/packages/workflow-executor/src/ports/activity-log-port.ts b/packages/workflow-executor/src/ports/activity-log-port.ts index 439489c831..51320571e6 100644 --- a/packages/workflow-executor/src/ports/activity-log-port.ts +++ b/packages/workflow-executor/src/ports/activity-log-port.ts @@ -22,7 +22,8 @@ export interface ActivityLogPort { createPending(args: CreateActivityLogArgs): Promise; markSucceeded(handle: ActivityLogHandle): Promise; // The server status endpoint accepts just { status }; the step error itself is logged by - // base-step-executor when the failure is rethrown, so no error message is threaded here. + // base-step-executor when the failure is rethrown (as the sole caller, AgentWithLog.audit, + // does), so no error message is threaded here. Only call this on a rethrow path. markFailed(handle: ActivityLogHandle): Promise; } diff --git a/packages/workflow-executor/src/schema-resolver.ts b/packages/workflow-executor/src/schema-resolver.ts index 11dc9391d0..61212bd383 100644 --- a/packages/workflow-executor/src/schema-resolver.ts +++ b/packages/workflow-executor/src/schema-resolver.ts @@ -4,8 +4,9 @@ import type { CollectionSchema } from './types/validated/collection'; // Per-run schema resolution: binds the shared SchemaCache, the orchestrator port and the // current runId once, so callers never thread a loader. Writes into the SAME SchemaCache -// instance AgentClientAgentPort reads from (get/iterate) — that shared instance is the -// invariant that keeps the agent-port's SchemaNotCachedError guard unreachable in normal flow. +// instance AgentClientAgentPort reads from (get/iterate): the resolver always populates an +// entry before the agent-port reads it, so the agent-port's SchemaNotCachedError guard does +// not fire under normal TTLs (a TTL shorter than a single step's round-trip could still evict). export default class SchemaResolver { private readonly cache: SchemaCache; private readonly workflowPort: WorkflowPort; diff --git a/packages/workflow-executor/src/types/execution-context.ts b/packages/workflow-executor/src/types/execution-context.ts index 8169fface7..fcf2f2c02c 100644 --- a/packages/workflow-executor/src/types/execution-context.ts +++ b/packages/workflow-executor/src/types/execution-context.ts @@ -5,7 +5,6 @@ import type { AgentPort } from '../ports/agent-port'; import type { Logger } from '../ports/logger-port'; import type { RunStore } from '../ports/run-store'; import type { WorkflowPort } from '../ports/workflow-port'; -import type SchemaCache from '../schema-cache'; import type SchemaResolver from '../schema-resolver'; import type { RecordRef } from './validated/collection'; import type { AvailableStepExecution, Step, StepUser } from './validated/execution'; @@ -37,7 +36,6 @@ export interface ExecutionContext readonly workflowPort: WorkflowPort; readonly runStore: RunStore; readonly user: StepUser; - readonly schemaCache: SchemaCache; readonly schemaResolver: SchemaResolver; readonly previousSteps: ReadonlyArray>; readonly logger: Logger; diff --git a/packages/workflow-executor/test/executors/base-step-executor.test.ts b/packages/workflow-executor/test/executors/base-step-executor.test.ts index fd7f17e352..c31dd1f71d 100644 --- a/packages/workflow-executor/test/executors/base-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/base-step-executor.test.ts @@ -106,7 +106,7 @@ function makeMockActivityLogPort(): ExecutionContext['activityLogPort'] { function makeContext(overrides: Partial = {}): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? ({} as ExecutionContext['workflowPort']); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -139,7 +139,6 @@ function makeContext(overrides: Partial = {}): ExecutionContex permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: makeMockLogger(), diff --git a/packages/workflow-executor/test/executors/condition-step-executor.test.ts b/packages/workflow-executor/test/executors/condition-step-executor.test.ts index aad401e164..12aa578102 100644 --- a/packages/workflow-executor/test/executors/condition-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/condition-step-executor.test.ts @@ -47,7 +47,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? ({} as ExecutionContext['workflowPort']); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -75,7 +75,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/guidance-step-executor.test.ts b/packages/workflow-executor/test/executors/guidance-step-executor.test.ts index a07979318c..ec7ca80f43 100644 --- a/packages/workflow-executor/test/executors/guidance-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/guidance-step-executor.test.ts @@ -24,7 +24,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? ({} as ExecutionContext['workflowPort']); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -52,7 +52,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts index 6268511415..dab0dc43ad 100644 --- a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts @@ -144,7 +144,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -168,7 +168,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index 913a301e86..238731250b 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -90,7 +90,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -119,7 +119,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts index a62019f29e..789f178d9e 100644 --- a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts @@ -111,7 +111,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -135,7 +135,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index 18475ceb6c..7375e12f30 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -110,7 +110,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -137,7 +137,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index 717ff760a3..f9ad2ac4bc 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -113,7 +113,7 @@ function makeContext( ): ExecutionContext { const runId = overrides.runId ?? 'run-1'; const workflowPort = overrides.workflowPort ?? makeMockWorkflowPort(); - const schemaCache = overrides.schemaCache ?? new SchemaCache(); + const schemaCache = new SchemaCache(); return { runId, @@ -139,7 +139,6 @@ function makeContext( permissionLevel: 'admin', tags: {}, }, - schemaCache, schemaResolver: new SchemaResolver(schemaCache, workflowPort, runId), previousSteps: [], logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() },