diff --git a/apps/api/src/knowledge-base/knowledge-base.controller.spec.ts b/apps/api/src/knowledge-base/knowledge-base.controller.spec.ts index 7e0b499b1f..12a461346b 100644 --- a/apps/api/src/knowledge-base/knowledge-base.controller.spec.ts +++ b/apps/api/src/knowledge-base/knowledge-base.controller.spec.ts @@ -8,6 +8,17 @@ jest.mock('../auth/auth.server', () => ({ auth: { api: { getSession: jest.fn() } }, })); +// The controller pulls in the auth guard (and therefore the Prisma client and +// the @trycompai/auth permission definitions) at import time. The service and +// guards are fully mocked/overridden below, so none of this is exercised — stub +// them out to keep this a hermetic unit test (matches the convention used by +// the other controller specs in this app). +jest.mock('@db', () => ({ db: {} })); +jest.mock('@trycompai/auth', () => ({ + statement: {}, + BUILT_IN_ROLE_PERMISSIONS: {}, +})); + describe('KnowledgeBaseController', () => { let controller: KnowledgeBaseController; let service: jest.Mocked; @@ -89,10 +100,16 @@ describe('KnowledgeBaseController', () => { }); }); + // These handlers scope to the caller's active organization from the auth + // context. Each test passes one org in the body and a different authenticated + // org, and asserts the authenticated org is what reaches the service. + const AUTH_ORG = 'org_authenticated'; + const OTHER_ORG = 'org_supplied_in_body'; + describe('uploadDocument', () => { - it('should delegate to service', async () => { + it('uses the authenticated organization, not the body organizationId', async () => { const dto = { - organizationId: 'org_1', + organizationId: OTHER_ORG, fileName: 'doc.pdf', fileType: 'application/pdf', fileData: 'base64', @@ -103,50 +120,73 @@ describe('KnowledgeBaseController', () => { s3Key: 'key', }); - const result = await controller.uploadDocument(dto as any); + const result = await controller.uploadDocument(AUTH_ORG, dto as any); expect(result.id).toBe('d1'); - expect(service.uploadDocument).toHaveBeenCalledWith(dto); + expect(service.uploadDocument).toHaveBeenCalledWith({ + ...dto, + organizationId: AUTH_ORG, + }); }); }); describe('getDownloadUrl', () => { - it('should merge documentId param with dto', async () => { - const dto = { organizationId: 'org_1' }; + it('scopes to the authenticated organization and merges documentId', async () => { + const dto = { organizationId: OTHER_ORG }; mockService.getDownloadUrl.mockResolvedValue({ signedUrl: 'https://example.com/signed', fileName: 'doc.pdf', }); - const result = await controller.getDownloadUrl('d1', dto as any); + const result = await controller.getDownloadUrl('d1', AUTH_ORG, dto as any); expect(result.signedUrl).toBe('https://example.com/signed'); expect(service.getDownloadUrl).toHaveBeenCalledWith({ - ...dto, documentId: 'd1', + organizationId: AUTH_ORG, + }); + }); + }); + + describe('getViewUrl', () => { + it('scopes to the authenticated organization and merges documentId', async () => { + const dto = { organizationId: OTHER_ORG }; + mockService.getViewUrl.mockResolvedValue({ + signedUrl: 'https://example.com/view', + fileName: 'doc.pdf', + fileType: 'application/pdf', + viewableInBrowser: true, + }); + + const result = await controller.getViewUrl('d1', AUTH_ORG, dto as any); + + expect(result.signedUrl).toBe('https://example.com/view'); + expect(service.getViewUrl).toHaveBeenCalledWith({ + documentId: 'd1', + organizationId: AUTH_ORG, }); }); }); describe('deleteDocument', () => { - it('should merge documentId param with dto', async () => { - const dto = { organizationId: 'org_1' }; + it('scopes to the authenticated organization and merges documentId', async () => { + const dto = { organizationId: OTHER_ORG }; mockService.deleteDocument.mockResolvedValue({ success: true }); - const result = await controller.deleteDocument('d1', dto as any); + const result = await controller.deleteDocument('d1', AUTH_ORG, dto as any); expect(result).toEqual({ success: true }); expect(service.deleteDocument).toHaveBeenCalledWith({ - ...dto, documentId: 'd1', + organizationId: AUTH_ORG, }); }); }); describe('processDocuments', () => { - it('should delegate to service', async () => { + it('uses the authenticated organization, not the body organizationId', async () => { const dto = { - organizationId: 'org_1', + organizationId: OTHER_ORG, documentIds: ['d1', 'd2'], }; mockService.processDocuments.mockResolvedValue({ @@ -155,56 +195,49 @@ describe('KnowledgeBaseController', () => { message: 'Processing 2 documents in parallel...', }); - const result = await controller.processDocuments(dto as any); + const result = await controller.processDocuments(AUTH_ORG, dto as any); expect(result.success).toBe(true); - expect(service.processDocuments).toHaveBeenCalledWith(dto); - }); - }); - - describe('createRunToken', () => { - it('should return token when created', async () => { - mockService.createRunReadToken.mockResolvedValue('token_123'); - - const result = await controller.createRunToken('run_1'); - - expect(result).toEqual({ success: true, token: 'token_123' }); - expect(service.createRunReadToken).toHaveBeenCalledWith('run_1'); - }); - - it('should return success false when token creation fails', async () => { - mockService.createRunReadToken.mockResolvedValue(undefined); - - const result = await controller.createRunToken('run_1'); - - expect(result).toEqual({ success: false, token: undefined }); + expect(service.processDocuments).toHaveBeenCalledWith({ + ...dto, + organizationId: AUTH_ORG, + }); }); }); describe('deleteManualAnswer', () => { - it('should merge manualAnswerId param with dto', async () => { - const dto = { organizationId: 'org_1' }; + it('scopes to the authenticated organization and merges manualAnswerId', async () => { + const dto = { organizationId: OTHER_ORG }; mockService.deleteManualAnswer.mockResolvedValue({ success: true }); - const result = await controller.deleteManualAnswer('ma1', dto as any); + const result = await controller.deleteManualAnswer( + 'ma1', + AUTH_ORG, + dto as any, + ); expect(result).toEqual({ success: true }); expect(service.deleteManualAnswer).toHaveBeenCalledWith({ - ...dto, manualAnswerId: 'ma1', + organizationId: AUTH_ORG, }); }); }); describe('deleteAllManualAnswers', () => { - it('should delegate to service', async () => { - const dto = { organizationId: 'org_1' }; + it('uses the authenticated organization, not the body organizationId', async () => { + const dto = { organizationId: OTHER_ORG }; mockService.deleteAllManualAnswers.mockResolvedValue({ success: true }); - const result = await controller.deleteAllManualAnswers(dto as any); + const result = await controller.deleteAllManualAnswers( + AUTH_ORG, + dto as any, + ); expect(result).toEqual({ success: true }); - expect(service.deleteAllManualAnswers).toHaveBeenCalledWith(dto); + expect(service.deleteAllManualAnswers).toHaveBeenCalledWith({ + organizationId: AUTH_ORG, + }); }); }); }); diff --git a/apps/api/src/knowledge-base/knowledge-base.controller.ts b/apps/api/src/knowledge-base/knowledge-base.controller.ts index a1f56c85fa..16bebbe4b6 100644 --- a/apps/api/src/knowledge-base/knowledge-base.controller.ts +++ b/apps/api/src/knowledge-base/knowledge-base.controller.ts @@ -70,13 +70,19 @@ export class KnowledgeBaseController { }); } + // All handlers scope operations to the caller's active organization, resolved + // from the authenticated request context via @OrganizationId. + @Post('documents/upload') @RequirePermission('questionnaire', 'create') @ApiOperation({ summary: 'Upload a knowledge base document' }) @ApiConsumes('application/json') @ApiOkResponse({ description: 'Document uploaded successfully' }) - async uploadDocument(@Body() dto: UploadDocumentDto) { - return this.knowledgeBaseService.uploadDocument(dto); + async uploadDocument( + @OrganizationId() organizationId: string, + @Body() dto: UploadDocumentDto, + ) { + return this.knowledgeBaseService.uploadDocument({ ...dto, organizationId }); } @Post('documents/:documentId/download') @@ -87,11 +93,13 @@ export class KnowledgeBaseController { @ApiOkResponse({ description: 'Signed download URL generated' }) async getDownloadUrl( @Param('documentId') documentId: string, + @OrganizationId() organizationId: string, @Body() dto: Omit, ) { return this.knowledgeBaseService.getDownloadUrl({ ...dto, documentId, + organizationId, }); } @@ -103,11 +111,13 @@ export class KnowledgeBaseController { @ApiOkResponse({ description: 'Signed view URL generated' }) async getViewUrl( @Param('documentId') documentId: string, + @OrganizationId() organizationId: string, @Body() dto: Omit, ) { return this.knowledgeBaseService.getViewUrl({ ...dto, documentId, + organizationId, }); } @@ -119,11 +129,13 @@ export class KnowledgeBaseController { @ApiOkResponse({ description: 'Document deleted successfully' }) async deleteDocument( @Param('documentId') documentId: string, + @OrganizationId() organizationId: string, @Body() dto: Omit, ) { return this.knowledgeBaseService.deleteDocument({ ...dto, documentId, + organizationId, }); } @@ -132,17 +144,14 @@ export class KnowledgeBaseController { @ApiOperation({ summary: 'Trigger processing of knowledge base documents' }) @ApiConsumes('application/json') @ApiOkResponse({ description: 'Document processing triggered' }) - async processDocuments(@Body() dto: ProcessDocumentsDto) { - return this.knowledgeBaseService.processDocuments(dto); - } - - @Post('runs/:runId/token') - @RequirePermission('questionnaire', 'read') - @ApiOperation({ summary: 'Create a public access token for a run' }) - @ApiOkResponse({ description: 'Public access token created' }) - async createRunToken(@Param('runId') runId: string) { - const token = await this.knowledgeBaseService.createRunReadToken(runId); - return { success: !!token, token }; + async processDocuments( + @OrganizationId() organizationId: string, + @Body() dto: ProcessDocumentsDto, + ) { + return this.knowledgeBaseService.processDocuments({ + ...dto, + organizationId, + }); } @Post('manual-answers/:manualAnswerId/delete') @@ -153,11 +162,13 @@ export class KnowledgeBaseController { @ApiOkResponse({ description: 'Manual answer deleted' }) async deleteManualAnswer( @Param('manualAnswerId') manualAnswerId: string, + @OrganizationId() organizationId: string, @Body() dto: DeleteManualAnswerDto, ) { return this.knowledgeBaseService.deleteManualAnswer({ ...dto, manualAnswerId, + organizationId, }); } @@ -167,7 +178,13 @@ export class KnowledgeBaseController { @ApiOperation({ summary: 'Delete all manual answers for an organization' }) @ApiConsumes('application/json') @ApiOkResponse({ description: 'All manual answers deleted' }) - async deleteAllManualAnswers(@Body() dto: DeleteAllManualAnswersDto) { - return this.knowledgeBaseService.deleteAllManualAnswers(dto); + async deleteAllManualAnswers( + @OrganizationId() organizationId: string, + @Body() dto: DeleteAllManualAnswersDto, + ) { + return this.knowledgeBaseService.deleteAllManualAnswers({ + ...dto, + organizationId, + }); } } 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)