diff --git a/apps/api/src/policies/dto/create-policy.dto.spec.ts b/apps/api/src/policies/dto/create-policy.dto.spec.ts index 588c0f51cd..c0259dfaf7 100644 --- a/apps/api/src/policies/dto/create-policy.dto.spec.ts +++ b/apps/api/src/policies/dto/create-policy.dto.spec.ts @@ -1,6 +1,7 @@ import { ValidationPipe } from '@nestjs/common'; import { CreatePolicyDto } from './create-policy.dto'; import { UpdatePolicyDto } from './update-policy.dto'; +import { UpdateVersionContentDto } from './version.dto'; /** * Regression test for the MCP/public-API policy content serialization bug. @@ -62,6 +63,20 @@ describe('Policy DTO content serialization (ValidationPipe)', () => { expect(result.content).not.toEqual([[], []]); }); + // PATCH /v1/policies/:id/versions/:versionId (MCP: update-policy-version-content). + // The controller reads req.body today, but this DTO is the published body + // schema — if it is ever wired to @Body() its transform must survive the pipe. + it('preserves structured TipTap content on UpdateVersionContentDto', async () => { + const result = await pipe.transform( + { content: TIPTAP_NODES }, + { type: 'body', metatype: UpdateVersionContentDto }, + ); + + expect(result.content).toEqual(TIPTAP_NODES); + expect(result.content[0]).toMatchObject({ type: 'heading' }); + expect(result.content).not.toEqual([[], []]); + }); + it('leaves content untouched when omitted on update', async () => { const result = await pipe.transform( { name: 'Renamed only' }, diff --git a/apps/api/src/policies/dto/version.dto.ts b/apps/api/src/policies/dto/version.dto.ts index 20f9eda52a..daaf3b5e94 100644 --- a/apps/api/src/policies/dto/version.dto.ts +++ b/apps/api/src/policies/dto/version.dto.ts @@ -42,9 +42,13 @@ export class UpdateVersionContentDto { type: 'array', items: { type: 'object', additionalProperties: true }, }) - @Transform(({ value }) => value) // Preserve raw JSON, don't let class-transformer mangle it @IsArray() - @Transform(({ value }) => value) + // Return the raw source value. Under the global ValidationPipe's implicit + // conversion, class-transformer coerces each TipTap node toward the reflected + // Array design-type of `content`, mangling `[{...}, {...}]` into `[[], []]`. + // The transform runs after that coercion, so `value` is already mangled — + // `obj.content` is the untouched original. Do not revert this to `value`. + @Transform(({ obj }) => obj.content) content: unknown[]; } diff --git a/apps/api/src/policies/policies.service.spec.ts b/apps/api/src/policies/policies.service.spec.ts index d4a7f8c537..6436bf4a2e 100644 --- a/apps/api/src/policies/policies.service.spec.ts +++ b/apps/api/src/policies/policies.service.spec.ts @@ -27,6 +27,7 @@ jest.mock('@db', () => ({ createMany: jest.fn(), }, $transaction: jest.fn(), + $executeRaw: jest.fn(), }, Frequency: { monthly: 'monthly', @@ -95,6 +96,7 @@ const { db } = require('@db') as { member: { findMany: jest.Mock; findFirst: jest.Mock }; auditLog: { createMany: jest.Mock }; $transaction: jest.Mock; + $executeRaw: jest.Mock; }; }; @@ -314,6 +316,119 @@ describe('PoliciesService', () => { expect(updateArg.data.signedBy).toEqual([]); }); + // CS-722: a content update on a DRAFT policy wrote `content` but left + // `draftContent` on the previous text. The stale draft then reported + // "unpublished changes" in the UI, and publishing without a versionId + // snapshots draftContent — silently reverting the rewrite that just landed. + it('syncs draftContent with content when updating a draft policy', async () => { + const orgId = 'org_abc'; + const newContent = [ + { type: 'paragraph', content: [{ type: 'text', text: 'rewritten body' }] }, + ]; + const existing = { + id: 'pol_1', + organizationId: orgId, + status: 'draft', + pendingVersionId: null, + approverId: null, + frequency: 'yearly', + pdfUrl: null, + }; + + db.$transaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + const tx = { + policy: { findFirst: db.policy.findFirst, update: db.policy.update }, + policyVersion: { update: db.policyVersion.update }, + }; + return callback(tx); + }); + db.policy.findFirst.mockResolvedValueOnce(existing); + db.policy.update.mockResolvedValueOnce({ + id: 'pol_1', + name: 'Test Policy', + status: 'draft', + currentVersionId: 'pv_1', + }); + + await service.updateById('pol_1', orgId, { content: newContent } as never); + + const updateArg = db.policy.update.mock.calls[0][0]; + expect(updateArg.data.content).toEqual(newContent); + expect(updateArg.data.draftContent).toEqual(newContent); + // Still a draft — draft content updates must not auto-publish. + expect(updateArg.data.status).toBeUndefined(); + expect(db.policyVersion.create).not.toHaveBeenCalled(); + // The current (draft) version snapshot stays in sync too. + expect(db.policyVersion.update).toHaveBeenCalledWith({ + where: { id: 'pv_1' }, + data: { content: newContent }, + }); + }); + + it('keeps the updated content when the caller then publishes without a versionId', async () => { + // The reported loss: update-policy followed by publish-policy-version with + // no versionId used to snapshot the stale draftContent, so the published + // version came back with the pre-update text. + const orgId = 'org_abc'; + const oldContent = [ + { type: 'paragraph', content: [{ type: 'text', text: 'old body' }] }, + ]; + const newContent = [ + { type: 'paragraph', content: [{ type: 'text', text: 'rewritten body' }] }, + ]; + + db.$transaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + const tx = { + policy: { findFirst: db.policy.findFirst, update: db.policy.update }, + policyVersion: { + findFirst: db.policyVersion.findFirst, + create: db.policyVersion.create, + update: db.policyVersion.update, + }, + }; + return callback(tx); + }); + db.policy.findFirst.mockResolvedValueOnce({ + id: 'pol_1', + organizationId: orgId, + status: 'draft', + pendingVersionId: null, + approverId: null, + frequency: 'yearly', + pdfUrl: null, + }); + db.policy.update.mockResolvedValueOnce({ + id: 'pol_1', + name: 'Test Policy', + status: 'draft', + currentVersionId: 'pv_1', + }); + + await service.updateById('pol_1', orgId, { content: newContent } as never); + + // Round-trip the row the update just wrote into the publish call. + const written = db.policy.update.mock.calls[0][0].data; + db.member.findFirst.mockResolvedValueOnce({ id: 'mem_caller' }); + db.policy.findUnique.mockResolvedValueOnce({ + id: 'pol_1', + organizationId: orgId, + content: written.content ?? oldContent, + draftContent: written.draftContent ?? oldContent, + pdfUrl: null, + frequency: 'yearly', + pendingVersionId: null, + approverId: null, + versions: [], + }); + db.policyVersion.findFirst.mockResolvedValueOnce({ version: 1 }); + db.policyVersion.create.mockResolvedValueOnce({ id: 'pv_2', version: 2 }); + + await service.publishVersion('pol_1', orgId, {}, 'usr_caller'); + + const createArg = db.policyVersion.create.mock.calls[0][0]; + expect(createArg.data.content).toEqual(newContent); + }); + // Auto-route: content update on a non-draft policy creates a new // PolicyVersion and publishes it, rather than mutating the published // version's content in place. This lets MCP/API consumers say @@ -605,6 +720,32 @@ describe('PoliciesService', () => { expect(policyUpdateArg.data.signedBy).toEqual([]); }); + // Same lock order as updateVersionContent / publishVersion: Policy row + // first, then PolicyVersion. Approving while someone edits a version is a + // realistic race, and the opposite order deadlocks (Postgres 40P01). + it('writes the policy row before the version row (single lock order)', async () => { + db.policy.findUnique.mockResolvedValueOnce(buildPendingPolicy()); + db.policyVersion.findUnique.mockResolvedValueOnce({ + id: 'ver_1', + version: 2, + content: [{ type: 'paragraph' }], + }); + db.member.findFirst.mockResolvedValueOnce({ id: 'mem_caller' }); + db.member.findMany.mockResolvedValueOnce([]); + mockTransactionTx(); + + await service.acceptChanges( + 'pol_1', + 'org_abc', + { approverId: 'mem_approver' }, + 'usr_caller', + ); + + expect(db.policy.update.mock.invocationCallOrder[0]).toBeLessThan( + db.policyVersion.update.mock.invocationCallOrder[0], + ); + }); + it('succeeds when called via session impersonation — caller userId differs from approverId', async () => { // Simulates an admin impersonating the assigned approver: // the impersonated session's userId belongs to the approver, but @@ -965,6 +1106,195 @@ describe('PoliciesService', () => { }); }); + // CS-722: editing a version wrote PolicyVersion.content only, leaving the + // Policy row on the previous text. get-policy then read back the stale + // content ("the write didn't land"), and publishing without a versionId + // snapshotted the stale draftContent — reverting the edit. + describe('updateVersionContent', () => { + const policyId = 'pol_1'; + const organizationId = 'org_abc'; + const oldContent = [ + { type: 'paragraph', content: [{ type: 'text', text: 'old body' }] }, + ]; + const newContent = [ + { type: 'paragraph', content: [{ type: 'text', text: 'MCP rewrite' }] }, + ]; + + const mockTransactionTx = ( + txFindUnique: jest.Mock = db.policyVersion.findUnique, + ) => { + db.$transaction.mockImplementation( + async (callback: (tx: unknown) => Promise) => { + const tx = { + $executeRaw: db.$executeRaw, + policyVersion: { + findUnique: txFindUnique, + findFirst: db.policyVersion.findFirst, + create: db.policyVersion.create, + update: db.policyVersion.update, + }, + policy: { update: db.policy.update }, + }; + return callback(tx); + }, + ); + }; + + const mockVersion = (policyOverrides: Record = {}) => + db.policyVersion.findUnique.mockResolvedValueOnce({ + id: 'pv_2', + policyId, + policy: { + id: policyId, + organizationId, + status: 'published', + currentVersionId: 'pv_1', + pendingVersionId: null, + ...policyOverrides, + }, + }); + + it('stages the edited content in Policy.draftContent without touching the live content', async () => { + mockVersion(); + mockTransactionTx(); + + await service.updateVersionContent(policyId, 'pv_2', organizationId, { + content: newContent, + }); + + expect(db.policyVersion.update).toHaveBeenCalledWith({ + where: { id: 'pv_2' }, + data: { content: newContent }, + }); + const policyUpdate = db.policy.update.mock.calls[0][0]; + expect(policyUpdate.where).toEqual({ id: policyId }); + expect(policyUpdate.data.draftContent).toEqual(newContent); + // The published body only moves when the version is published. + expect(policyUpdate.data.content).toBeUndefined(); + }); + + it('also syncs Policy.content when the edited version is the live draft version', async () => { + db.policyVersion.findUnique.mockResolvedValueOnce({ + id: 'pv_1', + policyId, + policy: { + id: policyId, + organizationId, + status: 'draft', + currentVersionId: 'pv_1', + pendingVersionId: null, + }, + }); + mockTransactionTx(); + + await service.updateVersionContent(policyId, 'pv_1', organizationId, { + content: newContent, + }); + + const policyUpdate = db.policy.update.mock.calls[0][0]; + expect(policyUpdate.data.content).toEqual(newContent); + expect(policyUpdate.data.draftContent).toEqual(newContent); + }); + + // The guards and the writes both key off status / currentVersionId, so they + // must run against state read inside the locked transaction. Validating a + // pre-transaction snapshot let a publish that committed in between pass the + // "still a draft" check, and the edit then overwrote the version — and the + // live Policy.content — that had just gone live. + it('rejects the edit when a concurrent publish promoted the version before the transaction', async () => { + const stalePolicy = { + id: policyId, + organizationId, + status: 'draft', + currentVersionId: 'pv_1', + pendingVersionId: null, + }; + // Read outside the transaction: pv_1 is still the current version of a + // draft policy. The locked read inside the transaction waits for the + // concurrent publish to commit and sees pv_1 already published. + db.policyVersion.findUnique.mockResolvedValue({ + id: 'pv_1', + policyId, + policy: stalePolicy, + }); + const txFindUnique = jest.fn().mockResolvedValue({ + id: 'pv_1', + policyId, + policy: { ...stalePolicy, status: 'published' }, + }); + mockTransactionTx(txFindUnique); + + await expect( + service.updateVersionContent(policyId, 'pv_1', organizationId, { + content: newContent, + }), + ).rejects.toThrow(/Cannot edit the published version/); + + expect(db.policyVersion.update).not.toHaveBeenCalled(); + expect(db.policy.update).not.toHaveBeenCalled(); + }); + + it('row-locks the policy row inside the transaction before validating', async () => { + mockVersion(); + mockTransactionTx(); + + await service.updateVersionContent(policyId, 'pv_2', organizationId, { + content: newContent, + }); + + expect(db.$executeRaw).toHaveBeenCalledTimes(1); + const [fragments, ...values] = db.$executeRaw.mock.calls[0]; + expect(fragments.join('?')).toContain('FOR UPDATE'); + // Org-scoped, so the lock can never be taken on another tenant's row. + expect(values).toEqual([policyId, organizationId]); + }); + + it('does not wipe the stored draft when the payload carries no content', async () => { + mockVersion(); + mockTransactionTx(); + + await service.updateVersionContent(policyId, 'pv_2', organizationId, { + content: [], + }); + + expect(db.policy.update).not.toHaveBeenCalled(); + }); + + it('keeps the edit when the caller then publishes without a versionId (MCP flow)', async () => { + // The tool sequence Claude is steered into: create-policy-version → + // update-policy-version-content → publish-policy-version. Without the + // draftContent sync, the publish snapshotted the pre-edit text. + mockVersion(); + mockTransactionTx(); + + await service.updateVersionContent(policyId, 'pv_2', organizationId, { + content: newContent, + }); + + // Round-trip the row the edit just wrote into the publish call. + const written = db.policy.update.mock.calls[0][0].data; + db.member.findFirst.mockResolvedValueOnce({ id: 'mem_caller' }); + db.policy.findUnique.mockResolvedValueOnce({ + id: policyId, + organizationId, + content: oldContent, + draftContent: written.draftContent ?? oldContent, + pdfUrl: null, + frequency: null, + pendingVersionId: null, + approverId: null, + versions: [], + }); + db.policyVersion.findFirst.mockResolvedValueOnce({ version: 2 }); + db.policyVersion.create.mockResolvedValueOnce({ id: 'pv_3', version: 3 }); + + await service.publishVersion(policyId, organizationId, {}, 'usr_caller'); + + const createArg = db.policyVersion.create.mock.calls[0][0]; + expect(createArg.data.content).toEqual(newContent); + }); + }); + describe('publishVersion', () => { const policyId = 'pol_1'; const organizationId = 'org_abc'; @@ -1044,6 +1374,34 @@ describe('PoliciesService', () => { }); }); + // updateVersionContent locks the Policy row (SELECT ... FOR UPDATE) before + // it writes the version, so publishing must take the same locks in the same + // order — Policy then PolicyVersion. The reverse order is an ABBA deadlock: + // Postgres aborts one of the two transactions with a 40P01. + it('writes the policy row before the version row (single lock order)', async () => { + db.member.findFirst.mockResolvedValueOnce({ id: 'mem_caller' }); + db.policy.findUnique.mockResolvedValueOnce(buildPolicy()); + db.policyVersion.findUnique.mockResolvedValueOnce({ + id: 'pv_target', + policyId, + content: versionContent, + version: 7, + pdfUrl: null, + }); + mockTransactionTx(); + + await service.publishVersion( + policyId, + organizationId, + { versionId: 'pv_target' }, + userId, + ); + + expect(db.policy.update.mock.invocationCallOrder[0]).toBeLessThan( + db.policyVersion.update.mock.invocationCallOrder[0], + ); + }); + it('sets displayFormat to EDITOR when the published version has no PDF', async () => { db.member.findFirst.mockResolvedValueOnce({ id: 'mem_caller' }); db.policy.findUnique.mockResolvedValueOnce(buildPolicy()); diff --git a/apps/api/src/policies/policies.service.ts b/apps/api/src/policies/policies.service.ts index eb0fdefdbe..ae998cc29a 100644 --- a/apps/api/src/policies/policies.service.ts +++ b/apps/api/src/policies/policies.service.ts @@ -410,6 +410,16 @@ export class PoliciesService { if (contentValue) { updatePayload.content = contentValue; + // Keep the working draft in lockstep with the live content. Callers of + // this endpoint (MCP, API consumers) send a single content payload — + // leaving draftContent on the previous text both shows a phantom + // "unpublished changes" banner and makes the next publish + // (POST :id/versions/publish with no versionId, which snapshots + // draftContent) revert the content that was just written. An empty + // array carries no text, so it must never wipe the stored draft. + if (contentValue.length > 0) { + updatePayload.draftContent = contentValue; + } } // All reads and writes in one transaction to prevent concurrent publish bypass @@ -849,53 +859,80 @@ export class PoliciesService { organizationId: string, dto: UpdateVersionContentDto, ) { - const version = await db.policyVersion.findUnique({ - where: { id: versionId }, - include: { - policy: { - select: { - id: true, - organizationId: true, - status: true, - currentVersionId: true, - pendingVersionId: true, + const processedContent = JSON.parse( + JSON.stringify(dto.content ?? []), + ) as Prisma.InputJsonValue[]; + + await db.$transaction(async (tx) => { + // Lock the policy row, then read its state inside the transaction. Both + // the guards and the writes below key off status / currentVersionId, so a + // publish or promotion committing in between would let this edit land on + // a version that has since become the live one. + await tx.$executeRaw`SELECT id FROM "Policy" WHERE id = ${policyId} AND "organizationId" = ${organizationId} FOR UPDATE`; + + const version = await tx.policyVersion.findUnique({ + where: { id: versionId }, + include: { + policy: { + select: { + id: true, + organizationId: true, + status: true, + currentVersionId: true, + pendingVersionId: true, + }, }, }, - }, - }); + }); - if ( - !version || - version.policy.id !== policyId || - version.policy.organizationId !== organizationId - ) { - throw new NotFoundException('Version not found'); - } + if ( + !version || + version.policy.id !== policyId || + version.policy.organizationId !== organizationId + ) { + throw new NotFoundException('Version not found'); + } - // Cannot edit the current version unless the policy is in draft status - // This covers both 'published' and 'needs_review' states - if ( - version.id === version.policy.currentVersionId && - version.policy.status !== 'draft' - ) { - throw new BadRequestException( - 'Cannot edit the published version. Create a new version to make changes.', - ); - } + // Cannot edit the current version unless the policy is in draft status + // This covers both 'published' and 'needs_review' states + if ( + version.id === version.policy.currentVersionId && + version.policy.status !== 'draft' + ) { + throw new BadRequestException( + 'Cannot edit the published version. Create a new version to make changes.', + ); + } - if (version.id === version.policy.pendingVersionId) { - throw new BadRequestException( - 'Cannot edit a version that is pending approval.', - ); - } + if (version.id === version.policy.pendingVersionId) { + throw new BadRequestException( + 'Cannot edit a version that is pending approval.', + ); + } - const processedContent = JSON.parse( - JSON.stringify(dto.content ?? []), - ) as Prisma.InputJsonValue[]; + await tx.policyVersion.update({ + where: { id: versionId }, + data: { content: processedContent }, + }); - await db.policyVersion.update({ - where: { id: versionId }, - data: { content: processedContent }, + // Mirror the edit onto the Policy row. draftContent is the working draft + // everywhere else — the "unpublished changes" banner compares it against + // content, and publishing without a versionId snapshots it — so leaving + // it on the previous text makes the next publish revert this edit. + // Policy.content only moves when the edited version IS the live one, + // which the guard above allows only while the policy is a draft. An empty + // payload carries no text, so it must never wipe the stored draft. + if (processedContent.length > 0) { + await tx.policy.update({ + where: { id: policyId }, + data: { + draftContent: processedContent, + ...(version.id === version.policy.currentVersionId && { + content: processedContent, + }), + }, + }); + } }); return { versionId }; @@ -1017,11 +1054,10 @@ export class PoliciesService { } await db.$transaction(async (tx) => { - await tx.policyVersion.update({ - where: { id: sourceVersion.id }, - data: { publishedById: memberId }, - }); - + // Policy row first, then the version row. Every path that writes both + // (updateById, updateVersionContent, acceptChanges) takes the locks in + // this order; taking them the other way round here deadlocks against a + // concurrent edit that already holds the policy lock. await tx.policy.update({ where: { id: policyId }, data: { @@ -1042,6 +1078,11 @@ export class PoliciesService { signedBy: [], }, }); + + await tx.policyVersion.update({ + where: { id: sourceVersion.id }, + data: { publishedById: memberId }, + }); }); checkAutoCompletePhases({ @@ -1316,13 +1357,9 @@ export class PoliciesService { const memberId = await this.getMemberId(organizationId, userId); await db.$transaction(async (tx) => { - // Update the version with the publisher - await tx.policyVersion.update({ - where: { id: version.id }, - data: { publishedById: memberId }, - }); - - // Publish the pending version + // Publish the pending version. Policy row first, then the version row — + // same lock order as every other path that writes both, so a concurrent + // version edit (which locks the policy first) cannot deadlock with this. await tx.policy.update({ where: { id: policyId }, data: { @@ -1338,6 +1375,12 @@ export class PoliciesService { signedBy: [], }, }); + + // Stamp the version with the publisher + await tx.policyVersion.update({ + where: { id: version.id }, + data: { publishedById: memberId }, + }); }); // Check timeline auto-completion after accepting changes (policy published)