From 782f55f315b38eaf4072916c61da198fe0b228ab Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Wed, 29 Jul 2026 05:43:55 -0400 Subject: [PATCH 1/3] fix(in-progress): [bug] mcp issue - serialization bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem CloudconPTY Ltd Client is attempting to update policy via MCP, however claude ran into an issue and points to a serialization bug in Comp AI: "Retrying now with the same content.The call went through this time — but something's wrong. Look at what came back: the `content` field saved as **20 empty arrays** (`[[],[],...]`) instead of my 20 content nodes. The write landed (the `updatedAt` timestamp changed to just now), but the content didn't serialize correctly — the policy body may now be blank rather than holding the new text. Let me verify exactly what's stored before doing anything else.This didn't work either, and I need to be upfront: the retry returned successfully but **corrupted the content instead of saving it.** Here's the actual state I just confirmed by re-reading the policy: * The policy's `content` (its live body) is now **20 empty blocks** — effectively blank. * The `currentVersion` content is also blank. * The good news: your **original text is still intact** in the policy's `draftContent` field, untouched. What's happening is a serialization bug in the Comp AI MCP write path: when I send the structured policy body, each content block arrives as an empty array `[]` rather than the actual heading/paragraph. So the write "succeeds" but lands empty. This is why I don't want to keep firing it — every attempt through this tool will keep blanking the `content` field the same way, and it won't ever land the real text. It's not something retrying fixes. Because the policy was never published and is still a draft, the original wording surviving in `draftContent` means you likely haven't lost anything the editor shows — but I won't guarantee that, and importantly: **don't publish this policy until the body is restored**, or you'd publish a blank version. Given the write path is broken, the reliable way to get the rewrite in is to paste it into the Comp AI policy editor directly, bypassing the integration entirely. The finished text is in my earlier message — I can re-send it as clean copy-pasteable text if that's easier. How would you like to proceed? 1. I re-send the rewritten policy as plain text for you to paste into Comp AI's editor (surest fix — gets the correct new content in and overwrites the blank body). 2. I attempt to write the *original* text back into `content` so it's not blank — but I expect this to hit the exact same bug and stay empty, so I'd only do it if you want to confirm the behavior. 3. You raise the serialization issue with Comp AI support, since their MCP write endpoint is mangling structured content. I'd recommend option 1. Want me to re-post the text?" --- ticket comments (read the WHOLE thread and use the LATEST reconciled understanding — customers/CX often correct themselves, e.g. "my mistake, it works in chrome"; scope the bug to what is actually CONFIRMED now, not an earlier claim that was later retracted) --- [redacted-email]: https://linear.app/compai/profiles/paul hey so i re-checked the serialization bug you mention and it's actually already fixed in the current code we added a Transform decorator on the content field in the DTO a while back that prevents the empty array issue. the pipe that was causing `[[],[],...]` doesn't touch it anymore. that said the customer now says claude doesn't see trycomp tools at all and can't connect to the MCP server, which sounds different from the content blank thing. i can't tell from the code if that's a real issue or maybe they just need to restart the claude app or reconnect. can you ask them: do they see the trycomp tools listed when they first open claude, or is it blank from the start? and when they try to use it what error do they get exactly something in the claude app or does it just hang? that'll help me figure out if it's mcp connection or something else Unthread: Brendan Leung: I tried generating a new API key to use already as well​‌​ Unthread: Brendan Leung: I was using the Claude desktop app and tried starting a new chat and asking something like “list compliance tasks” which worked initially but encountered the first issue with the serialisation. Now, it doesn’t recognise anything from trycomp.​‌​ Unthread: Paul: The error here is often erroneous - are you using Claude Code? Can you go into the session and try to begin working?​‌​ Unthread: Brendan Leung: Hi <@U0936FZ6TFB|Paul>, any update on this? We're trying to update these policies ASAP​‌​ Unthread: Paul: Hi <@U07ET6LPSHK|Brendan Leung> just checking this​‌​ Unthread: Brendan Leung: Hey <@U0A64NBGY3S|Mark>, trying again by reconnecting to MCP server but seems like there's an issue connecting now - is this a known issue at the moment?​‌​ [redacted-email]: https://linear.app/compai/profiles/paul Fix merged. Covers exactly what Brendan described. **What was broken:** The API's validation pipeline was silently coercing each TipTap content node (heading, paragraph, etc.) into an empty array `[]` before saving. So a 20-node policy body became 20 empty arrays write "succeeded" but content was blank. The MCP path hit this the same as any other API caller. **What the fix does:** Reads the raw content from the original request before that coercion runs. Content nodes now survive the pipeline intact. **Does it cover their report?** Yes, exactly. The symptom they described `[[],[],...]` instead of actual content nodes is precisely what this fix corrects. --- **What to tell Brendan:** The fix is shipped. He can retry updating his policy via MCP now content should save correctly. He'll want to verify the current policy body first (it may still have the 20 blank blocks from the earlier failed write) and re-send the correct content. His `draftContent` was untouched, so he can use that as the source if he needs the original text. Unthread: Mark: Hey <@U07ET6LPSHK|Brendan Leung>, i have noticed that there is a comment from Engineering stating they have fixed the serialisation issue, are you able to try again and let me know?​‌​ Unthread: Mark: Hey <@U07ET6LPSHK|Brendan Leung>, this is currently in review with Engineering, so has made progress. I will follow up now with them to see if we can get this turned around today.​‌​ Unthread: Brendan Leung: Hey <@U0936FZ6TFB|Paul> any update on this issue?​‌​ [redacted-email]: fixed the serialization issue in the mcp write path for policy updates content was arriving empty because the structured blocks werent being serialized correctly before saving we patched it and tested it working now customer can use the integration again to update their policy https://github.com/trycompai/comp/pull/3388 Unthread: Linked to Unthread ticket: > [AI assistant edits resulted in incomplete sentences and incorrect formatting, unable to restore previous version #11108](https://compai.unthread.io/dashboard/inbox/all/conversations/f6dffd3d-3864-4027-bb97-10074eb8ea1c) [redacted-email]: This comment thread is synced to a corresponding [thread in Slack](https://comp-ai-workspace.slack.com/archives/C09CV8JCGKD/p1783667450211219). All replies are displayed in both locations. --- PREVIOUS FIX ATTEMPTS (this ticket was reopened 2x) --- attempt 1: merged (PR https://github.com/trycompai/comp/pull/3388) came back because: Customer reports the serialization bug is still occurring when using Claude desktop app to ask for compliance tasks, and now the app doesn't recognize trycomp at all. attempt 2: reopened but could not confirm: The serialization defect is real and precisely located in our code: main.ts:127-134 runs the global ValidationPipe with enableImplicitConversion, so class-transformer reads the reflected Array design-type of `content: unknown[]` and instantiates each TipTap node as `new Array()` with string keys, which Prisma/JSON serializes as `[]` — the customer's `[[],[],...]`. Current main already defeats it at create-policy.dto.ts:98 (`@Transform(({ obj }) => obj.content)`, inherited by UpdatePolicyDto via PartialType; I ran the existing spec against the identical pipe options and it passes), and I swept mechanism-first for residual paths: the only other policy-content write bypasses the pipe via `@Req().body` (policies.controller.ts:1268), org-chart takes `@Body() Record` (native metatype → ValidationPipe skips), and every other object-array DTO field uses `@Type()` or a union type that reflects as Object — so no live path can still blank persisted JSON. The symptom actually live per the LATEST thread message ("reconnecting to MCP server but seems like there's an issue connecting now" / Claude sees no trycomp tools) is a different, runtime connect/tool-discovery claim I cannot prove from static code, and its only levers are the generated+published apps/mcp-server (never-touch) or the MCP OAuth/mcpOrgBinding path in hybrid-auth.guard.ts (§4 auth) — needs real run/log evidence, not a code guess. came back because: The customer reports the MCP server connection is still not working after the fix. --- A previous fix for this ALREADY SHIPPED (merged to main) and the ticket came back, so the shipped fix is INSUFFICIENT. Do NOT conclude "already fixed" just because the code already contains that change — it is present and still not enough. Re-investigate the root cause, find the remaining gap the earlier attempt missed, and fix THAT. --- ## Fix [Bug] MCP issue - serialization bug ## Verification ✅ added a regression test; suite green locally --- .../policies/dto/create-policy.dto.spec.ts | 15 ++ apps/api/src/policies/dto/version.dto.ts | 8 +- .../api/src/policies/policies.service.spec.ts | 245 ++++++++++++++++++ apps/api/src/policies/policies.service.ts | 37 ++- 4 files changed, 300 insertions(+), 5 deletions(-) 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..f1d825584d 100644 --- a/apps/api/src/policies/policies.service.spec.ts +++ b/apps/api/src/policies/policies.service.spec.ts @@ -314,6 +314,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 @@ -965,6 +1078,138 @@ 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 = () => { + db.$transaction.mockImplementation( + async (callback: (tx: unknown) => Promise) => { + const tx = { + policyVersion: { + 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); + }); + + 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'; diff --git a/apps/api/src/policies/policies.service.ts b/apps/api/src/policies/policies.service.ts index eb0fdefdbe..02624cda34 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 @@ -893,9 +903,30 @@ export class PoliciesService { JSON.stringify(dto.content ?? []), ) as Prisma.InputJsonValue[]; - await db.policyVersion.update({ - where: { id: versionId }, - data: { content: processedContent }, + await db.$transaction(async (tx) => { + await tx.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 }; From c09a3c40bfd5d7722c974763fc44ecc810485592 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Wed, 29 Jul 2026 06:00:52 -0400 Subject: [PATCH 2/3] fix(in-progress): address cubic review Address review findings. --- .../api/src/policies/policies.service.spec.ts | 61 +++++++++++++- apps/api/src/policies/policies.service.ts | 84 ++++++++++--------- 2 files changed, 105 insertions(+), 40 deletions(-) diff --git a/apps/api/src/policies/policies.service.spec.ts b/apps/api/src/policies/policies.service.spec.ts index f1d825584d..2b9aaf0d35 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; }; }; @@ -1092,11 +1094,15 @@ describe('PoliciesService', () => { { type: 'paragraph', content: [{ type: 'text', text: 'MCP rewrite' }] }, ]; - const mockTransactionTx = () => { + 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, @@ -1164,6 +1170,59 @@ describe('PoliciesService', () => { 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(); diff --git a/apps/api/src/policies/policies.service.ts b/apps/api/src/policies/policies.service.ts index 02624cda34..591f0537c6 100644 --- a/apps/api/src/policies/policies.service.ts +++ b/apps/api/src/policies/policies.service.ts @@ -859,51 +859,57 @@ 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'); - } + }); - // 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 || + version.policy.id !== policyId || + version.policy.organizationId !== organizationId + ) { + throw new NotFoundException('Version not found'); + } - if (version.id === version.policy.pendingVersionId) { - throw new BadRequestException( - 'Cannot edit a version that is pending approval.', - ); - } + // 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.', + ); + } - const processedContent = JSON.parse( - JSON.stringify(dto.content ?? []), - ) as Prisma.InputJsonValue[]; + if (version.id === version.policy.pendingVersionId) { + throw new BadRequestException( + 'Cannot edit a version that is pending approval.', + ); + } - await db.$transaction(async (tx) => { await tx.policyVersion.update({ where: { id: versionId }, data: { content: processedContent }, From 0491519b9e174368a55340521ff60bbc7c63b488 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Wed, 29 Jul 2026 06:15:29 -0400 Subject: [PATCH 3/3] fix(in-progress): address cubic review Address review findings. --- .../api/src/policies/policies.service.spec.ts | 54 +++++++++++++++++++ apps/api/src/policies/policies.service.ts | 30 ++++++----- 2 files changed, 72 insertions(+), 12 deletions(-) diff --git a/apps/api/src/policies/policies.service.spec.ts b/apps/api/src/policies/policies.service.spec.ts index 2b9aaf0d35..6436bf4a2e 100644 --- a/apps/api/src/policies/policies.service.spec.ts +++ b/apps/api/src/policies/policies.service.spec.ts @@ -720,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 @@ -1348,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 591f0537c6..ae998cc29a 100644 --- a/apps/api/src/policies/policies.service.ts +++ b/apps/api/src/policies/policies.service.ts @@ -1054,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: { @@ -1079,6 +1078,11 @@ export class PoliciesService { signedBy: [], }, }); + + await tx.policyVersion.update({ + where: { id: sourceVersion.id }, + data: { publishedById: memberId }, + }); }); checkAutoCompletePhases({ @@ -1353,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: { @@ -1375,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)