From 746fdb3379339f1070b9354271d1b170d145e4a2 Mon Sep 17 00:00:00 2001 From: Kai-Chieh Yang Date: Mon, 7 Sep 2026 18:17:26 +0800 Subject: [PATCH 1/2] fix(workflow): keep cancelled rows as entities so their GraphQL getters resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cancelAdhocDirective` spread the loaded directive into a plain object before saving it. The saved value is returned straight to the resolver, and `AdhocDirective.targetValueJson` is a non-nullable field backed by a prototype getter, so the spread dropped the getter and the mutation answered `INTERNAL_SERVER_ERROR` — after the cancellation had already been committed. To the approver the withdraw looks like it failed while the row is in fact `CANCELLED`, so the natural reaction is to retry a directive that is already gone. Reproduced end to end against a deployed instance: the toast reads "Internal server error" and the GraphQL response carries `path: ["cancelAdhocDirective", "targetValueJson"]`, yet the directive reads back as `CANCELLED`. `cancelApprovalInstance` has the same shape and returns an entity with five getter-backed JSON fields, so any caller selecting one of them hits the same failure. Its client wrapper happens to select only `id` and `state`, which is why it has not surfaced yet; the API is reachable by other consumers, so it is fixed here too. Only the directive path carries a regression test. The instance repository stub re-wraps whatever it is handed via `Object.assign(createApprovalInstance(), …)`, so it restores the prototype the production code had dropped and a test written against it passes either way — verified by reverting the fix and watching it stay green. Closing that gap means changing the shared stub, which is left out of a bug fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018STtLMQTp1GVCYFp9GPSfg --- .../workflow-engine.service.spec.ts | 41 +++++++++++++++++++ .../workflow-engine.service.ts | 28 +++++++++---- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts b/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts index dd2aff6..6d74137 100644 --- a/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts +++ b/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts @@ -3189,6 +3189,47 @@ describe('WorkflowEngineService', () => { ); }); + it('returns a cancelled ad-hoc directive that still resolves its GraphQL getters', async (): Promise => { + const fixture = createServiceFixture({ + currentVersionId: 'template-version-1', + formVersionStatus: FormDefinitionVersionStatusEnum.PUBLISHED, + processAdhocDirectives: [ + createAdhocDirective({ + createdByMemberId: 'member-finance', + id: 'directive-cancel-1', + status: AdhocDirectiveStatusEnum.PENDING, + targetValue: { + kind: AdhocTargetKindEnum.MEMBER, + memberIds: ['member-x'], + }, + type: AdhocDirectiveTypeEnum.COUNTERSIGN, + }), + ], + processWorkflowSnapshot: createLinearUserTaskWorkflow(), + templateVersionStatus: ApprovalTemplateVersionStatusEnum.PUBLISHED, + }); + + const cancelled = await fixture.service.cancelAdhocDirective({ + cancelledByMemberId: 'member-finance', + directiveId: 'directive-cancel-1', + }); + + expect(cancelled.status).toBe(AdhocDirectiveStatusEnum.CANCELLED); + // `targetValueJson` is a non-nullable GraphQL field backed by a prototype + // getter, so the resolver only survives if the saved value is still an + // entity instance. Spreading the row into a plain object drops the getter + // and the mutation fails with an opaque INTERNAL_SERVER_ERROR *after* the + // cancellation has already been written, which reads to the approver as + // "the withdraw failed" even though it succeeded. + expect(cancelled).toBeInstanceOf(AdhocDirectiveEntity); + expect(cancelled.targetValueJson).toBe( + JSON.stringify({ + kind: AdhocTargetKindEnum.MEMBER, + memberIds: ['member-x'], + }), + ); + }); + it('dispatches ad-hoc completion notifications on reject and cancels pending flow directives', async (): Promise => { const fixture = createServiceFixture({ currentVersionId: 'template-version-1', diff --git a/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.ts b/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.ts index abad75e..e17a379 100644 --- a/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.ts +++ b/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.ts @@ -939,11 +939,15 @@ export class WorkflowEngineService { instanceId: instance.id, manager, }); - const cancelledInstance = await instanceRepository.save({ - ...instance, - completedAt: cancelledAt, - state: ApprovalInstanceStateEnum.CANCELLED, - }); + // Same prototype-preserving reason as `cancelAdhocDirective`: this + // entity is returned to the resolver and `ApprovalInstance` exposes + // five getter-backed JSON fields. + const cancelledInstance = await instanceRepository.save( + Object.assign(new ApprovalInstanceEntity(), instance, { + completedAt: cancelledAt, + state: ApprovalInstanceStateEnum.CANCELLED, + }), + ); await this.dispatchAdhocCompletionNotifications( manager, @@ -1317,10 +1321,16 @@ export class WorkflowEngineService { ); } - const cancelledDirective = await directiveRepository.save({ - ...directive, - status: AdhocDirectiveStatusEnum.CANCELLED, - }); + // Assigning onto a fresh entity keeps the prototype, so the row handed + // back to the resolver still carries the getters the GraphQL schema + // exposes (e.g. `targetValueJson`). Spreading it into a plain object + // drops them, and the mutation then fails with an opaque + // INTERNAL_SERVER_ERROR *after* the cancellation has been committed. + const cancelledDirective = await directiveRepository.save( + Object.assign(new AdhocDirectiveEntity(), directive, { + status: AdhocDirectiveStatusEnum.CANCELLED, + }), + ); await this.recordAdhocDirectiveActivity( manager, From fb37683c954235661d401fd11a47c9f4ca72b701 Mon Sep 17 00:00:00 2001 From: Chia Yu Pai Date: Mon, 7 Sep 2026 21:33:34 +0800 Subject: [PATCH 2/2] test(workflow): let the instance repository mock report what save was given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mock rebuilt every saved row as `Object.assign(createApprovalInstance(), entity)`, so it handed back an `ApprovalInstanceEntity` even when the caller had spread the row into a plain object — the exact regression the cancel paths guard against was invisible to the suite. `save` now keeps the prototype it was handed, and `findOne` rehydrates, which is what the database actually does: a row read back is always an entity, and only `save` sees the shape the caller passed. Adds the missing `cancelApprovalInstance` case. Both cancel tests were checked by reverting each fix in turn; each one fails alone. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflow-engine.service.spec.ts | 61 ++++++++++++++++--- 1 file changed, 51 insertions(+), 10 deletions(-) diff --git a/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts b/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts index 6d74137..093825c 100644 --- a/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts +++ b/libs/bpm-core/src/lib/workflow-engine/workflow-engine.service.spec.ts @@ -3230,6 +3230,31 @@ describe('WorkflowEngineService', () => { ); }); + it('returns a cancelled instance that still resolves its GraphQL getters', async (): Promise => { + const fixture = createServiceFixture({ + currentVersionId: 'template-version-1', + formVersionStatus: FormDefinitionVersionStatusEnum.PUBLISHED, + processFormData: { amount: 1200 }, + processWorkflowSnapshot: createLinearUserTaskWorkflow(), + templateVersionStatus: ApprovalTemplateVersionStatusEnum.PUBLISHED, + }); + + const cancelled = await fixture.service.cancelApprovalInstance({ + cancelledByMemberId: 'member-001', + comment: null, + instanceId: 'instance-1', + }); + + expect(cancelled.state).toBe(ApprovalInstanceStateEnum.CANCELLED); + // `ApprovalInstance` exposes five getter-backed JSON fields, all of them + // non-nullable. Spreading the row into a plain object drops every one, so + // the withdrawal is committed and the mutation still fails with an opaque + // INTERNAL_SERVER_ERROR — the initiator sees "cancel failed" on a case + // that is already cancelled. + expect(cancelled).toBeInstanceOf(ApprovalInstanceEntity); + expect(cancelled.formDataJson).toBe(JSON.stringify({ amount: 1200 })); + }); + it('dispatches ad-hoc completion notifications on reject and cancels pending flow directives', async (): Promise => { const fixture = createServiceFixture({ currentVersionId: 'template-version-1', @@ -3664,19 +3689,35 @@ function createServiceFixture({ ), findOne: jest.fn(() => Promise.resolve( - savedInstance ?? - createApprovalInstance({ - formData: processFormData, - formDataOptionSnapshot: processOptionSnapshot, - formDefinitionSnapshot: processFormDefinitionSnapshot, - state: instanceState, - updatedAt: transactionalInstanceUpdatedAt, - workflowSnapshot: processWorkflowSnapshot, - }), + // A row read back from the database is an entity whatever shape the + // caller last handed to `save`, so it is rehydrated here. Only + // `save` reports the shape it was actually given. + savedInstance + ? Object.assign(createApprovalInstance(), savedInstance) + : createApprovalInstance({ + formData: processFormData, + formDataOptionSnapshot: processOptionSnapshot, + formDefinitionSnapshot: processFormDefinitionSnapshot, + state: instanceState, + updatedAt: transactionalInstanceUpdatedAt, + workflowSnapshot: processWorkflowSnapshot, + }), ), ), save: jest.fn((entity: ApprovalInstanceEntity) => { - savedInstance = Object.assign(createApprovalInstance(), entity); + // The saved row keeps the prototype it arrived with. Rebuilding it as + // `Object.assign(createApprovalInstance(), entity)` handed back an + // entity even when the caller had spread the row into a plain object, + // which hid exactly the regression the cancel paths guard against: + // the getter-backed `*Json` fields the GraphQL schema exposes live on + // the prototype, so a plain object fails the mutation at resolve time. + savedInstance = Object.assign( + Object.create( + Object.getPrototypeOf(entity) as object, + ) as ApprovalInstanceEntity, + createApprovalInstance(), + entity, + ); return Promise.resolve(savedInstance); }),