From 11b2c55485ec2a75d10408d0882d939e59fb7535 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 17:48:08 +0300 Subject: [PATCH 1/6] =?UTF-8?q?feat(audit):=20DB-backed=20audit=20trail=20?= =?UTF-8?q?=D0=B2=D0=BC=D0=B5=D1=81=D1=82=D0=BE=20Noop=20(#496)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Событие comment.suggestion_applied/dismissed раньше улетало в NoopAuditService и молча терялось; при childless-ветке apply/dismiss комментарий hard-delete'ится, поэтому восстановить, кто и что решил, было невозможно. - DatabaseAuditService пишет в уже существующую таблицу `audit` (миграция 20260228T223532); actor/workspace/ip берутся из CLS AuditContext, вне HTTP — из явного контекста (logWithContext). Аудит — побочная запись: сбой записи не ломает исходный запрос, EXCLUDED_AUDIT_EVENTS отбрасываются. - Биндинг AUDIT_SERVICE переключён с Noop на DatabaseAuditService. - payload apply/dismiss дополнен suggestedText/selection/commentAuthor/ decidedBy — на childless-ветке это единственная уцелевшая запись. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/server/src/app.module.ts | 4 +- .../comment.service.apply-suggestion.spec.ts | 8 + ...comment.service.dismiss-suggestion.spec.ts | 10 + .../src/core/comment/comment.service.ts | 29 ++- .../src/integrations/audit/audit.module.ts | 11 +- .../audit/database-audit.service.spec.ts | 171 ++++++++++++++++++ .../audit/database-audit.service.ts | 160 ++++++++++++++++ 7 files changed, 384 insertions(+), 9 deletions(-) create mode 100644 apps/server/src/integrations/audit/database-audit.service.spec.ts create mode 100644 apps/server/src/integrations/audit/database-audit.service.ts diff --git a/apps/server/src/app.module.ts b/apps/server/src/app.module.ts index 527035b9..c46310f6 100644 --- a/apps/server/src/app.module.ts +++ b/apps/server/src/app.module.ts @@ -25,7 +25,7 @@ import { CacheModule } from '@nestjs/cache-manager'; import KeyvRedis from '@keyv/redis'; import { LoggerModule } from './common/logger/logger.module'; import { ClsModule } from 'nestjs-cls'; -import { NoopAuditModule } from './integrations/audit/audit.module'; +import { AuditModule } from './integrations/audit/audit.module'; import { ThrottleModule } from './integrations/throttle/throttle.module'; import { McpModule } from './integrations/mcp/mcp.module'; import { SandboxModule } from './integrations/sandbox/sandbox.module'; @@ -55,7 +55,7 @@ try { middleware: { mount: true }, }), LoggerModule, - NoopAuditModule, + AuditModule, CoreModule, DatabaseModule, EnvironmentModule, diff --git a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts index 486f89bb..bcfc2f2c 100644 --- a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts @@ -146,11 +146,19 @@ describe('CommentService — applySuggestion', () => { 'page-1', expect.objectContaining({ operation: 'commentDeleted', commentId: 'c-1' }), ); + // #496: hard-deleted row → the audit payload is the only surviving record. expect(auditService.log).toHaveBeenCalledWith( expect.objectContaining({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED, resourceType: AuditResource.COMMENT, resourceId: 'c-1', + metadata: expect.objectContaining({ + pageId: 'page-1', + suggestedText: 'new text', + selection: 'old text', + commentAuthor: 'user-1', + decidedBy: 'user-1', + }), }), ); expect(result.outcome).toBe('deleted'); diff --git a/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts index 819071ff..1899377a 100644 --- a/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts @@ -107,11 +107,21 @@ describe('CommentService — dismissSuggestion', () => { 'page-1', expect.objectContaining({ operation: 'commentDeleted', commentId: 'c-1' }), ); + // #496: the row is hard-deleted, so the audit payload must carry the + // decision's substance (what was suggested, the anchored text, who authored + // it, who decided) — it is the only surviving record. expect(auditService.log).toHaveBeenCalledWith( expect.objectContaining({ event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, resourceType: AuditResource.COMMENT, resourceId: 'c-1', + metadata: expect.objectContaining({ + pageId: 'page-1', + suggestedText: 'new text', + selection: 'old text', + commentAuthor: 'user-1', + decidedBy: 'user-1', + }), }), ); expect(result.outcome).toBe('deleted'); diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index ef97c108..c053ccf8 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -524,7 +524,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return { ...updatedComment, outcome: 'resolved' }; } @@ -538,7 +538,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return settled; } @@ -597,7 +597,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return { ...updatedComment, outcome: 'resolved' }; @@ -616,7 +616,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return settled; @@ -732,6 +732,27 @@ export class CommentService { return this.generalQueue.add(QueueJob.COMMENT_MARK_UPDATE, jobData); } + /** + * Build the audit metadata for a suggestion apply/dismiss decision (#496). + * The subject comment is HARD-DELETED on the childless path, so the audit row + * is the only surviving record — capture the decision's substance (what was + * suggested, the anchored text it replaced, who authored it, who decided) + * before the row can vanish. `decidedBy` is the acting user; `commentAuthor` + * is the suggestion's creator. + */ + private suggestionAuditMetadata( + comment: Comment, + user: User, + ): Record { + return { + pageId: comment.pageId, + suggestedText: comment.suggestedText ?? null, + selection: comment.selection ?? null, + commentAuthor: comment.creatorId ?? null, + decidedBy: user.id, + }; + } + private async queueCommentNotification( content: any, oldMentionIds: string[], diff --git a/apps/server/src/integrations/audit/audit.module.ts b/apps/server/src/integrations/audit/audit.module.ts index eafffdd7..07ef04d3 100644 --- a/apps/server/src/integrations/audit/audit.module.ts +++ b/apps/server/src/integrations/audit/audit.module.ts @@ -1,14 +1,19 @@ import { Global, Module } from '@nestjs/common'; -import { AUDIT_SERVICE, NoopAuditService } from './audit.service'; +import { AUDIT_SERVICE } from './audit.service'; +import { DatabaseAuditService } from './database-audit.service'; +// #496: bind the audit token to a real DB-backed trail (was NoopAuditService, +// which silently dropped every event). Kysely (@Global DatabaseModule) and +// ClsService (@Global ClsModule) are both globally available, so this module +// needs no extra imports. @Global() @Module({ providers: [ { provide: AUDIT_SERVICE, - useClass: NoopAuditService, + useClass: DatabaseAuditService, }, ], exports: [AUDIT_SERVICE], }) -export class NoopAuditModule {} +export class AuditModule {} diff --git a/apps/server/src/integrations/audit/database-audit.service.spec.ts b/apps/server/src/integrations/audit/database-audit.service.spec.ts new file mode 100644 index 00000000..90456c72 --- /dev/null +++ b/apps/server/src/integrations/audit/database-audit.service.spec.ts @@ -0,0 +1,171 @@ +import { DatabaseAuditService } from './database-audit.service'; +import { AUDIT_CONTEXT_KEY } from '../../common/middlewares/audit-context.middleware'; +import { AuditEvent, AuditResource } from '../../common/events/audit-events'; + +/** + * Observable-property coverage for the DB-backed audit trail (#496): every + * assertion pins what actually reaches the `audit` table (or that nothing does), + * driven through a chainable Kysely mock that captures the inserted rows. + */ +describe('DatabaseAuditService', () => { + function makeService(clsContext: any) { + const inserted: any[] = []; + const updated: any[] = []; + let failNextInsert = false; + + const db: any = { + insertInto: jest.fn(() => ({ + values: jest.fn((rows: any) => ({ + execute: jest.fn(async () => { + if (failNextInsert) { + failNextInsert = false; + throw new Error('boom'); + } + inserted.push(rows); + }), + })), + })), + updateTable: jest.fn(() => ({ + set: jest.fn((patch: any) => ({ + where: jest.fn(() => ({ + execute: jest.fn(async () => { + updated.push(patch); + }), + })), + })), + })), + }; + + const store: Record = { [AUDIT_CONTEXT_KEY]: clsContext }; + const cls: any = { + get: jest.fn((key: string) => store[key]), + set: jest.fn((key: string, val: any) => { + store[key] = val; + }), + }; + + const service = new DatabaseAuditService(db, cls); + return { + service, + inserted, + updated, + cls, + store, + failInsert: () => { + failNextInsert = true; + }, + }; + } + + const applyPayload = () => ({ + event: AuditEvent.COMMENT_SUGGESTION_APPLIED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + spaceId: 'space-1', + metadata: { pageId: 'p-1', suggestedText: 'new', decidedBy: 'u-2' }, + }); + + const ctx = (over?: any) => ({ + workspaceId: 'ws-1', + actorId: 'u-2', + actorType: 'user', + ipAddress: '10.0.0.1', + userAgent: 'jest', + ...over, + }); + + it('log() persists a row with the CLS context merged onto the payload', async () => { + const { service, inserted } = makeService(ctx()); + service.log(applyPayload()); + // log() is fire-and-forget; flush the microtask queue. + await Promise.resolve(); + await Promise.resolve(); + + expect(inserted).toHaveLength(1); + expect(inserted[0]).toMatchObject({ + workspaceId: 'ws-1', + actorId: 'u-2', + actorType: 'user', + event: AuditEvent.COMMENT_SUGGESTION_APPLIED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + spaceId: 'space-1', + ipAddress: '10.0.0.1', + metadata: { pageId: 'p-1', suggestedText: 'new', decidedBy: 'u-2' }, + }); + }); + + it('log() is a no-op when there is no workspace in scope', async () => { + const { service, inserted } = makeService(ctx({ workspaceId: null })); + service.log(applyPayload()); + await Promise.resolve(); + expect(inserted).toHaveLength(0); + }); + + it('log() drops EXCLUDED_AUDIT_EVENTS (e.g. comment.created)', async () => { + const { service, inserted } = makeService(ctx()); + service.log({ + event: AuditEvent.COMMENT_CREATED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + spaceId: 'space-1', + }); + await Promise.resolve(); + await Promise.resolve(); + expect(inserted).toHaveLength(0); + }); + + it('a failed insert never throws to the caller (audit is a side-record)', async () => { + const { service, failInsert } = makeService(ctx()); + failInsert(); + // Must not reject / throw. + expect(() => service.log(applyPayload())).not.toThrow(); + await Promise.resolve(); + await Promise.resolve(); + }); + + it('logWithContext() persists with an explicit (non-request) context', async () => { + const { service, inserted } = makeService(undefined); + service.logWithContext(applyPayload(), ctx({ actorType: 'system' }) as any); + await Promise.resolve(); + await Promise.resolve(); + expect(inserted).toHaveLength(1); + expect(inserted[0].actorType).toBe('system'); + expect(inserted[0].workspaceId).toBe('ws-1'); + }); + + it('logBatchWithContext() inserts only non-excluded events', async () => { + const { service, inserted } = makeService(undefined); + service.logBatchWithContext( + [ + applyPayload(), + { + event: AuditEvent.COMMENT_CREATED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-2', + }, + ], + ctx() as any, + ); + await Promise.resolve(); + await Promise.resolve(); + // Batch is a single insert call carrying only the applied event. + expect(inserted).toHaveLength(1); + expect(inserted[0]).toHaveLength(1); + expect(inserted[0][0].event).toBe(AuditEvent.COMMENT_SUGGESTION_APPLIED); + }); + + it('setActorId / setActorType mutate the ambient CLS context', () => { + const { service, store } = makeService(ctx({ actorId: null })); + service.setActorId('u-9'); + service.setActorType('api_key'); + expect(store[AUDIT_CONTEXT_KEY].actorId).toBe('u-9'); + expect(store[AUDIT_CONTEXT_KEY].actorType).toBe('api_key'); + }); + + it('updateRetention() writes the workspace retention window', async () => { + const { service, updated } = makeService(ctx()); + await service.updateRetention('ws-1', 30); + expect(updated).toEqual([{ auditRetentionDays: 30 }]); + }); +}); diff --git a/apps/server/src/integrations/audit/database-audit.service.ts b/apps/server/src/integrations/audit/database-audit.service.ts new file mode 100644 index 00000000..cfa3f321 --- /dev/null +++ b/apps/server/src/integrations/audit/database-audit.service.ts @@ -0,0 +1,160 @@ +import { Injectable, Logger } from '@nestjs/common'; +import { InjectKysely } from 'nestjs-kysely'; +import { ClsService } from 'nestjs-cls'; +import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { + AuditContext, + AUDIT_CONTEXT_KEY, +} from '../../common/middlewares/audit-context.middleware'; +import { + AuditLogPayload, + ActorType, + EXCLUDED_AUDIT_EVENTS, +} from '../../common/events/audit-events'; +import { AuditLogContext, IAuditService } from './audit.service'; + +/** + * Minimal DB-backed audit trail (#496). Replaces NoopAuditService so that + * decision-bearing events — notably comment.suggestion_applied / + * comment.suggestion_dismissed, whose subject comment is HARD-DELETED on the + * childless path — leave a durable record of who decided what. Without this the + * events were emitted (comment.service / *.controller) but swallowed, so an + * applied/dismissed suggestion was unrecoverable once the row was gone. + * + * Rows land in the pre-existing `audit` table (migration 20260228T223532). The + * per-request actor/workspace/ip come from the CLS AuditContext populated by + * AuditContextMiddleware + AuditActorInterceptor; callers that run OUTSIDE a + * request (queue workers, imports) pass an explicit context via + * logWithContext / logBatchWithContext. + * + * Audit is a side-record: a write failure MUST NOT break the originating + * request, so every persistence path swallows its error with a warn. Events in + * EXCLUDED_AUDIT_EVENTS (high-volume/low-signal) are dropped. + */ +@Injectable() +export class DatabaseAuditService implements IAuditService { + private readonly logger = new Logger(DatabaseAuditService.name); + + constructor( + @InjectKysely() private readonly db: KyselyDB, + private readonly cls: ClsService, + ) {} + + /** + * Persist a single event using the ambient request-scoped AuditContext. A + * no-op when there is no workspace in scope (the table's workspace_id is NOT + * NULL) or the event is excluded. Fire-and-forget: the returned promise is not + * awaited by hot callers, and its rejection is swallowed here. + */ + log(payload: AuditLogPayload): void { + const context = this.cls?.get(AUDIT_CONTEXT_KEY); + if (!context?.workspaceId) { + // No workspace in scope — nothing we can attribute the row to. This is + // expected for events emitted outside an HTTP request; those callers must + // use logWithContext instead. + return; + } + void this.persist(payload, { + workspaceId: context.workspaceId, + actorId: context.actorId ?? undefined, + actorType: context.actorType, + ipAddress: context.ipAddress ?? undefined, + userAgent: context.userAgent ?? undefined, + }); + } + + /** Persist a single event with an explicit (non-request) context. */ + logWithContext(payload: AuditLogPayload, context: AuditLogContext): void { + if (!context?.workspaceId) return; + void this.persist(payload, context); + } + + /** Persist a batch of events sharing one explicit context (imports). */ + logBatchWithContext( + payloads: AuditLogPayload[], + context: AuditLogContext, + ): void { + if (!context?.workspaceId || payloads.length === 0) return; + const rows = payloads + .filter((p) => !EXCLUDED_AUDIT_EVENTS.has(p.event)) + .map((p) => this.toRow(p, context)); + if (rows.length === 0) return; + this.db + .insertInto('audit') + .values(rows) + .execute() + .catch((err: any) => + this.logger.warn(`Failed to persist ${rows.length} audit events: ${err?.message}`), + ); + } + + /** Update the ambient request actor (e.g. after login resolves the user). */ + setActorId(actorId: string): void { + const context = this.cls?.get(AUDIT_CONTEXT_KEY); + if (context) { + context.actorId = actorId; + this.cls.set(AUDIT_CONTEXT_KEY, context); + } + } + + /** Update the ambient request actor type (user | system | api_key). */ + setActorType(actorType: ActorType): void { + const context = this.cls?.get(AUDIT_CONTEXT_KEY); + if (context) { + context.actorType = actorType; + this.cls.set(AUDIT_CONTEXT_KEY, context); + } + } + + /** Persist a workspace's audit-log retention window (days). */ + async updateRetention( + workspaceId: string, + retentionDays: number, + ): Promise { + try { + await this.db + .updateTable('workspaces') + .set({ auditRetentionDays: retentionDays }) + .where('id', '=', workspaceId) + .execute(); + } catch (err: any) { + this.logger.warn( + `Failed to update audit retention for workspace ${workspaceId}: ${err?.message}`, + ); + } + } + + private async persist( + payload: AuditLogPayload, + context: AuditLogContext, + ): Promise { + if (EXCLUDED_AUDIT_EVENTS.has(payload.event)) return; + try { + await this.db + .insertInto('audit') + .values(this.toRow(payload, context)) + .execute(); + } catch (err: any) { + // Audit is a side-record; never let a failed write surface to the caller. + this.logger.warn( + `Failed to persist audit event ${payload.event}: ${err?.message}`, + ); + } + } + + private toRow(payload: AuditLogPayload, context: AuditLogContext) { + return { + workspaceId: context.workspaceId, + actorId: context.actorId ?? null, + actorType: context.actorType ?? 'user', + event: payload.event, + resourceType: payload.resourceType, + resourceId: payload.resourceId ?? null, + spaceId: payload.spaceId ?? null, + // jsonb columns: node-postgres serializes plain objects to JSON. + changes: payload.changes ? (payload.changes as any) : null, + metadata: payload.metadata ? (payload.metadata as any) : null, + ipAddress: context.ipAddress ?? null, + }; + } +} From c7073b62d147d7a801a0569252994dfad09dfbd4 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 17:50:17 +0300 Subject: [PATCH 2/6] =?UTF-8?q?fix(comments):=20apply=20=D0=BD=D0=B5=20?= =?UTF-8?q?=D1=81=D1=82=D0=B8=D1=80=D0=B0=D0=B5=D1=82=20=D1=84=D0=BE=D1=80?= =?UTF-8?q?=D0=BC=D0=B0=D1=82=D0=B8=D1=80=D0=BE=D0=B2=D0=B0=D0=BD=D0=B8?= =?UTF-8?q?=D0=B5=20(#496)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit replaceYjsMarkedText вставлял замену только с меткой `{comment}`, молча теряя bold/italic/code/link исходного run'а. Теперь захватываем полный набор атрибутов доминирующего (самого длинного) сегмента заменяемого диапазона и применяем его к вставке: однородное форматирование сохраняется точно, для смешанного run'а берётся преобладающий стиль вместо полной потери. Метка комментария при этом гарантированно сохраняется (переутверждается явно). Клиентское предупреждение в превью диффа не добавлялось: у клиента нет марок из документа (только plain selection/suggestedText), а фикс на сервере уже сохраняет форматирование, так что баннер был бы неточным. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../server/src/collaboration/yjs.util.spec.ts | 75 +++++++++++++++++++ apps/server/src/collaboration/yjs.util.ts | 26 +++++-- 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/apps/server/src/collaboration/yjs.util.spec.ts b/apps/server/src/collaboration/yjs.util.spec.ts index 8f6fc2e4..93f4ea5a 100644 --- a/apps/server/src/collaboration/yjs.util.spec.ts +++ b/apps/server/src/collaboration/yjs.util.spec.ts @@ -529,4 +529,79 @@ describe('replaceYjsMarkedText', () => { expect(result).toEqual({ applied: false, currentText: 'abcdef' }); expect(text.toDelta()).toEqual(before); }); + + // #496: apply must NOT silently strip the replaced run's inline formatting. + // Build a paragraph and format the marked range with extra marks, then assert + // the replacement carries them. + function buildFormatted( + runs: Array<{ text: string; attrs?: Record }>, + ): { fragment: Y.XmlFragment; text: Y.XmlText } { + const ydoc = new Y.Doc(); + const fragment = ydoc.getXmlFragment('default'); + const para = new Y.XmlElement('paragraph'); + fragment.insert(0, [para]); + const text = new Y.XmlText(); + para.insert(0, [text]); + text.insert(0, runs.map((r) => r.text).join('')); + let offset = 0; + for (const run of runs) { + if (run.attrs) text.format(offset, run.text.length, run.attrs); + offset += run.text.length; + } + return { fragment, text }; + } + + it('preserves the original run formatting (bold + link) on the replacement', () => { + const { fragment, text } = buildFormatted([ + { text: 'see ' }, + { + text: 'old', + attrs: { + comment: { commentId: 'c1', resolved: false }, + bold: true, + link: { href: 'https://x.test' }, + }, + }, + { text: ' end' }, + ]); + + const result = replaceYjsMarkedText(fragment, 'c1', 'old', 'new'); + + expect(result).toEqual({ applied: true, currentText: 'new' }); + // The comment anchor AND the bold/link marks survive the delete+insert. + expect(text.toDelta()).toEqual([ + { insert: 'see ' }, + { + insert: 'new', + attributes: { + comment: { commentId: 'c1', resolved: false }, + bold: true, + link: { href: 'https://x.test' }, + }, + }, + { insert: ' end' }, + ]); + }); + + it('mixed formatting under the mark: replacement takes the DOMINANT run marks', () => { + // Marked run = "bold" (bold, 4 chars) + "x" (plain, 1 char), same commentId. + // The dominant (longer) segment is bold, so the flat replacement is bold. + const { fragment, text } = buildFormatted([ + { + text: 'bold', + attrs: { comment: { commentId: 'c1', resolved: false }, bold: true }, + }, + { text: 'x', attrs: { comment: { commentId: 'c1', resolved: false } } }, + ]); + + const result = replaceYjsMarkedText(fragment, 'c1', 'boldx', 'Z'); + + expect(result).toEqual({ applied: true, currentText: 'Z' }); + expect(text.toDelta()).toEqual([ + { + insert: 'Z', + attributes: { comment: { commentId: 'c1', resolved: false }, bold: true }, + }, + ]); + }); }); diff --git a/apps/server/src/collaboration/yjs.util.ts b/apps/server/src/collaboration/yjs.util.ts index bea28dfc..25c6af27 100644 --- a/apps/server/src/collaboration/yjs.util.ts +++ b/apps/server/src/collaboration/yjs.util.ts @@ -145,6 +145,10 @@ type MarkedSegment = { length: number; text: string; markAttrs: Record; + // The FULL attribute set of this delta run — the `comment` mark plus any + // inline formatting (bold/italic/code/link/…). Captured so apply can carry the + // original run's formatting onto the replacement instead of dropping it. + attributes: Record; }; /** @@ -202,6 +206,7 @@ export function replaceYjsMarkedText( length, text: insert, markAttrs: markAttr, + attributes, }); } offset += length; @@ -251,15 +256,26 @@ export function replaceYjsMarkedText( return { applied: false, currentText: joinedText }; } - // 3. All guards passed: delete the marked run and re-insert newText with the - // same comment attributes at the same offset. Atomic within the caller's - // transaction. + // 3. All guards passed: delete the marked run and re-insert newText at the + // same offset. Atomic within the caller's transaction. const start = segments[0].offset; const len = segments.reduce((sum, s) => sum + s.length, 0); - const markAttrs = segments[0].markAttrs; + + // Carry the ORIGINAL run's formatting onto the replacement (#496): inserting + // with only the `comment` mark silently dropped bold/italic/code/link of the + // replaced text. Yjs applies one flat attribute set to the whole insert, so + // when the marked run mixes formatting we pick the DOMINANT segment (the one + // covering the most characters) and apply its attributes — a v1 that preserves + // the common single-format case exactly and, for a mixed run, keeps the + // prevailing style rather than losing all of it. The dominant segment already + // carries the `comment` mark (every collected segment does), so the anchor is + // preserved; we defensively re-assert it in case a future attribute shape + // omits it. + const dominant = segments.reduce((a, b) => (b.length > a.length ? b : a)); + const insertAttrs = { ...dominant.attributes, comment: dominant.markAttrs }; node.delete(start, len); - node.insert(start, newText, { comment: markAttrs }); + node.insert(start, newText, insertAttrs); return { applied: true, currentText: newText }; } From 7be49c1280402314e62c063ca677fe497ea86674 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 17:53:41 +0300 Subject: [PATCH 3/6] =?UTF-8?q?fix(comments):=20=D0=B4=D0=B5=D0=B4=D1=83?= =?UTF-8?q?=D0=BF=20=D0=B4=D0=B2=D0=BE=D0=B9=D0=BD=D0=BE=D0=B3=D0=BE=20WS-?= =?UTF-8?q?broadcast=20=D0=BF=D1=80=D0=B8=20apply=20=D1=82=D1=80=D0=B5?= =?UTF-8?q?=D0=B4=D0=B0=20=D1=81=20=D0=BE=D1=82=D0=B2=D0=B5=D1=82=D0=B0?= =?UTF-8?q?=D0=BC=D0=B8=20(#496)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finalizeAppliedSuggestion на ветке «есть ответы» вызывал resolveComment (бродкаст commentResolved с обогащённой строкой), а затем сам слал ещё и commentUpdated — клиент получал два события на один apply. Теперь commentUpdated шлётся только когда resolveComment НЕ вызывался (редкий повторный вход по уже разрешённому треду), иначе один бродкаст. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comment.service.apply-suggestion.spec.ts | 42 ++++++++++++++++++- .../src/core/comment/comment.service.ts | 21 +++++++--- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts index bcfc2f2c..100dae67 100644 --- a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts @@ -197,17 +197,25 @@ describe('CommentService — applySuggestion', () => { expect(resolvePatch.resolvedAt).toBeInstanceOf(Date); expect(resolvePatch.resolvedById).toBe('user-1'); - // NOT deleted; broadcast an update, not a deletion. + // NOT deleted. expect(commentRepo.deleteComment).not.toHaveBeenCalled(); expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith( 'deleteCommentMark', expect.anything(), expect.anything(), ); + // #496 dedup: resolveComment broadcasts `commentResolved` with the enriched + // row; finalize must NOT ALSO emit a redundant `commentUpdated`. So the + // thread receives exactly ONE resolve broadcast and no update broadcast. expect(wsService.emitCommentEvent).toHaveBeenCalledWith( 'space-1', 'page-1', - expect.objectContaining({ operation: 'commentUpdated', comment: UPDATED }), + expect.objectContaining({ operation: 'commentResolved', comment: UPDATED }), + ); + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentUpdated' }), ); expect(auditService.log).toHaveBeenCalledWith( @@ -219,6 +227,36 @@ describe('CommentService — applySuggestion', () => { expect(result.outcome).toBe('resolved'); }); + it('re-entry: already applied+resolved WITH replies → emits commentUpdated (dedup does not over-suppress)', async () => { + // suggestionAppliedAt set → idempotent finalize; resolvedAt set → resolveComment + // is skipped, so there is NO commentResolved broadcast. The applied-stamp state + // must still reach clients via a single commentUpdated. + const { service, wsService } = makeService( + { applied: false, currentText: 'new text' }, + true, + ); + + await service.applySuggestion( + suggestionComment({ + suggestionAppliedAt: new Date(), + resolvedAt: new Date(), + }), + user(), + ); + + expect(wsService.emitCommentEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentUpdated', comment: UPDATED }), + ); + // Nothing resolved this time (already resolved) → no resolve broadcast. + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentResolved' }), + ); + }); + // --- error / rejection branches ----------------------------------------- it('applied=false and currentText differs → ConflictException with currentText in payload', async () => { diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index c053ccf8..88122e96 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -577,8 +577,10 @@ export class CommentService { // Auto-resolve the thread. resolveComment handles the resolve mark, its ws // broadcast and the resolve notification. Stay defensive on re-entry. + let didResolveBroadcast = false; if (!comment.resolvedAt) { await this.resolveComment(comment, true, user, provenance); + didResolveBroadcast = true; } const updatedComment = await this.commentRepo.findById(comment.id, { @@ -586,11 +588,20 @@ export class CommentService { includeResolvedBy: true, }); - this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { - operation: 'commentUpdated', - pageId: comment.pageId, - comment: updatedComment, - }); + // #496 dedup: resolveComment already broadcast `commentResolved` carrying + // the fully-enriched row (the applied stamps were persisted above, before + // that call, so its re-read reflects them). Emitting `commentUpdated` here + // too made the client receive TWO events for one apply. Broadcast the + // update ONLY when we did NOT resolve — i.e. the rare re-entry on an + // already-resolved thread, where the applied-stamp change still needs a + // broadcast and resolveComment did not run. + if (!didResolveBroadcast) { + this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { + operation: 'commentUpdated', + pageId: comment.pageId, + comment: updatedComment, + }); + } this.auditService.log({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED, From 826bb491ca0304224ddbdcca79b43cf8a56fd471 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 17:56:30 +0300 Subject: [PATCH 4/6] fix(comments): orphan-anchor reconcile + docstring (#496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deleteEphemeralSuggestion docstring обещал инвариант «метка снимается FIRST and FATALLY» синхронно — после #399 это уже не так: fatal только ENQUEUE снятия метки, сама операция идёт в воркере с ретраями. Docstring переписан под фактическое поведение. Reconcile: воркер COMMENT_MARK_UPDATE на resolve/unresolve, обнаружив что строки комментария больше нет (hard-delete гонкой с ephemeral apply/ dismiss), теперь СНИМАЕТ осиротевшую метку вместо тихого return. Это самозаживляет тихую дивергенцию и закрывает fire-and-forget resolve/ unresolve enqueue из resolveComment. Операция идемпотентна. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/core/comment/comment.service.ts | 19 +++++++++++-------- ...neral-queue.processor.comment-mark.spec.ts | 10 ++++++++-- .../processors/general-queue.processor.ts | 16 ++++++++++++++-- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index 88122e96..85af14e0 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -638,14 +638,17 @@ export class CommentService { * inline `comment` anchor mark, then ATOMICALLY hard-delete the row only if it * is still childless. Shared by the apply/dismiss no-replies branches (#329). * - * ORDER MATTERS: the anchor mark is removed FIRST and FATALLY (mirrors - * applySuggestion, which mutates the doc before writing the DB). The row - * delete is irreversible, so if the mark removal fails — including the - * COLLAB_DISABLE_REDIS "no live instance" hard-error — we must NOT delete the - * row and report success, or the document is left with a permanent orphan - * anchor pointing at a comment that no longer exists (the exact data-integrity - * bug #329 targets). Let the exception propagate (→ 5xx); the operation is - * then repeatable with row + mark still consistent. + * ORDER MATTERS (updated #399 → #496): what runs FIRST and FATALLY here is the + * mark-removal ENQUEUE (a fast, durable Redis add), NOT the mark op itself. + * deleteCommentMark awaits only the enqueue, so a failed add throws BEFORE the + * irreversible row delete — the row + mark stay consistent and the operation is + * repeatable. The actual anchor strip then runs off the HTTP path in the worker + * (idempotent, 3 retries). Only an EXHAUSTED-retries job could leave the doc + * with an orphan anchor pointing at a hard-deleted comment (the data-integrity + * bug #329 targets); that residual divergence is now self-healed by the + * resolve/unresolve mark worker, which strips an orphan mark whenever its + * comment row is gone (#496), and it is meanwhile VISIBLE via BullMQ failed-job + * metrics rather than a silently-swallowed warn. * * RACE (#338 F4): the caller read `hasChildren` BEFORE the (slow) mark * removal, so a reply can land in that window. `comments.parent_comment_id` is diff --git a/apps/server/src/integrations/queue/processors/general-queue.processor.comment-mark.spec.ts b/apps/server/src/integrations/queue/processors/general-queue.processor.comment-mark.spec.ts index 7adf30ed..29c958c7 100644 --- a/apps/server/src/integrations/queue/processors/general-queue.processor.comment-mark.spec.ts +++ b/apps/server/src/integrations/queue/processors/general-queue.processor.comment-mark.spec.ts @@ -139,13 +139,19 @@ describe('GeneralQueueProcessor — COMMENT_MARK_UPDATE (#399)', () => { ); }); - it('skips (no throw) when the comment row has vanished', async () => { + it('reconcile (#496): comment row vanished → strips the orphan anchor mark', async () => { const { proc, collaborationGateway, commentRepo } = makeProc(); commentRepo.findById.mockResolvedValue(undefined); await expect( proc.process(job({ ...base, action: 'resolve', ts: 1000 })), ).resolves.toBeUndefined(); - expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled(); + // A resolve/unresolve mark job whose comment row is gone leaves a silent + // orphan; the worker self-heals by stripping the anchor instead of returning. + expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( + 'deleteCommentMark', + 'page.page-1', + { commentId: 'c-1', user: { id: 'user-1' } }, + ); }); }); diff --git a/apps/server/src/integrations/queue/processors/general-queue.processor.ts b/apps/server/src/integrations/queue/processors/general-queue.processor.ts index e75cd8c7..d2dee503 100644 --- a/apps/server/src/integrations/queue/processors/general-queue.processor.ts +++ b/apps/server/src/integrations/queue/processors/general-queue.processor.ts @@ -95,7 +95,8 @@ export class GeneralQueueProcessor * #399: apply a comment's inline-mark mirror in the collab Y.Doc, off the HTTP * critical path. Runs the SAME gateway path the synchronous comment.service * code used (byte-identical mark op): - * - resolve / unresolve → resolveCommentMark (flip the `resolved` attribute); + * - resolve / unresolve → resolveCommentMark (flip the `resolved` attribute), + * OR strip an orphan anchor when the comment row has vanished (#496); * - delete → deleteCommentMark (strip the ephemeral-suggestion anchor #329). * The op is idempotent, so a BullMQ retry is safe. Throwing propagates to * WorkerHost → the job is retried and, on exhaustion, surfaces in failed-job @@ -133,7 +134,18 @@ export class GeneralQueueProcessor // of this resolve), skip it rather than flip the mark to a stale state. const comment = await this.commentRepo.findById(commentId); if (!comment) { - // The comment vanished (e.g. hard-deleted) → nothing left to mirror. + // #496 reconcile: the comment row is GONE (e.g. an ephemeral apply/dismiss + // hard-deleted it while this resolve/unresolve mark job sat in the queue), + // but its inline anchor may still live in the doc — a silent orphan mark + // pointing at a comment that no longer exists. Self-heal by stripping it + // instead of just returning: this closes the divergence the fire-and-forget + // resolve/unresolve enqueue (comment.service resolveComment) could leave. + // Idempotent — deleteCommentMark on an already-absent mark is a no-op. + await this.getCollaborationGateway().handleYjsEvent( + 'deleteCommentMark', + documentName, + { commentId, user }, + ); return; } const wantResolved = action === 'resolve'; From 0099ba272d8c27aed00bc0116a9938c61541be78 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 18:09:10 +0300 Subject: [PATCH 5/6] =?UTF-8?q?fix(comments):=20suggestion=20=D1=81=20?= =?UTF-8?q?=D0=B2=D0=B5=D1=87=D0=BD=D1=8B=D0=BC=20409=20(#496)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit expectedText брался из debounced REST-снапшота (getAnchoredText по pages/info), а метка ставилась в live-доке — при расхождении (док ушёл вперёд за окно дебаунса) apply строго сравнивал текст под меткой с устаревшим stored selection и давал 409 на каждый вызов. MCP-клиент теперь в transform-фазе (та же версия live-дока, где ставится метка) перечитывает фактическую подстроку под меткой и, если она отличается от сохранённого selection, синкает её через новый эндпоинт POST /comments/resync-suggestion-anchor. Best-effort: сбой синка не откатывает уже заякоренный комментарий, а лишь выдаёт мягкое предупреждение. Совпадающий снапшот не делает лишнего round-trip. Сервер: resyncSuggestionAnchor правит только stored selection незаселённой suggestion своего автора (guards: top-level, есть suggestedText, не applied/resolved, отличается от suggestedText), идемпотентно, без ws-бродкаста. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/core/comment/comment.controller.ts | 34 ++++ .../comment.service.resync-anchor.spec.ts | 126 ++++++++++++ .../src/core/comment/comment.service.ts | 70 +++++++ .../dto/resync-suggestion-anchor.dto.ts | 20 ++ packages/mcp/src/client/comments.ts | 42 ++++ .../mcp/test/mock/create-comment.test.mjs | 183 ++++++++++++++++++ 6 files changed, 475 insertions(+) create mode 100644 apps/server/src/core/comment/comment.service.resync-anchor.spec.ts create mode 100644 apps/server/src/core/comment/dto/resync-suggestion-anchor.dto.ts diff --git a/apps/server/src/core/comment/comment.controller.ts b/apps/server/src/core/comment/comment.controller.ts index fbd2c190..c04ed622 100644 --- a/apps/server/src/core/comment/comment.controller.ts +++ b/apps/server/src/core/comment/comment.controller.ts @@ -16,6 +16,7 @@ import { UpdateCommentDto } from './dto/update-comment.dto'; import { ResolveCommentDto } from './dto/resolve-comment.dto'; import { ApplySuggestionDto } from './dto/apply-suggestion.dto'; import { DismissSuggestionDto } from './dto/dismiss-suggestion.dto'; +import { ResyncSuggestionAnchorDto } from './dto/resync-suggestion-anchor.dto'; import { PageIdDto, CommentIdDto } from './dto/comments.input'; import { AuthUser } from '../../common/decorators/auth-user.decorator'; import { AuthWorkspace } from '../../common/decorators/auth-workspace.decorator'; @@ -235,6 +236,39 @@ export class CommentController { return this.commentService.applySuggestion(comment, user, provenance); } + @HttpCode(HttpStatus.OK) + @Post('resync-suggestion-anchor') + async resyncSuggestionAnchor( + @Body() dto: ResyncSuggestionAnchorDto, + @AuthUser() user: User, + @AuthWorkspace() workspace: Workspace, + ) { + const comment = await this.commentRepo.findById(dto.commentId, { + includeCreator: true, + includeResolvedBy: true, + }); + if (!comment) { + throw new NotFoundException('Comment not found'); + } + + const page = await this.pageRepo.findById(comment.pageId); + if (!page || page.deletedAt) { + throw new NotFoundException('Page not found'); + } + + // Authorize BEFORE revealing structural detail (mirrors apply/dismiss). + // Re-anchoring does NOT change the page text — it only corrects the stored + // selection metadata — so the page-level gate is comment access. The service + // further restricts it to the suggestion's own author. + await this.pageAccessService.validateCanComment(page, user, workspace.id); + + return this.commentService.resyncSuggestionAnchor( + comment, + dto.selection, + user, + ); + } + @HttpCode(HttpStatus.OK) @Post('dismiss-suggestion') async dismissSuggestion( diff --git a/apps/server/src/core/comment/comment.service.resync-anchor.spec.ts b/apps/server/src/core/comment/comment.service.resync-anchor.spec.ts new file mode 100644 index 00000000..3602a53f --- /dev/null +++ b/apps/server/src/core/comment/comment.service.resync-anchor.spec.ts @@ -0,0 +1,126 @@ +import { BadRequestException, ForbiddenException } from '@nestjs/common'; +import { CommentService } from './comment.service'; + +/** + * Coverage for CommentService.resyncSuggestionAnchor (#496): re-anchoring a + * suggestion's stored selection (== apply-time expectedText) to the live-doc + * substring. The service is built directly with jest-mocked deps (the + * @InjectQueue tokens can't be resolved by Test.createTestingModule — see the + * sibling specs). + */ +describe('CommentService — resyncSuggestionAnchor', () => { + const UPDATED = { id: 'c-1', selection: 'new anchor', __updated: true } as any; + + function makeService() { + const commentRepo: any = { + updateComment: jest.fn(async () => undefined), + findById: jest.fn(async () => UPDATED), + }; + const service = new CommentService( + commentRepo, + {} as any, + { emitCommentEvent: jest.fn() } as any, + {} as any, + { add: jest.fn() } as any, + { add: jest.fn() } as any, + { log: jest.fn() } as any, + ); + return { service, commentRepo }; + } + + const suggestion = (over?: Partial): any => ({ + id: 'c-1', + creatorId: 'user-1', + parentCommentId: null, + selection: 'old anchor', + suggestedText: 'new text', + suggestionAppliedAt: null, + resolvedAt: null, + ...over, + }); + const user = (over?: Partial): any => ({ id: 'user-1', ...over }); + + it('persists the new selection and returns the enriched comment', async () => { + const { service, commentRepo } = makeService(); + + const out = await service.resyncSuggestionAnchor( + suggestion(), + 'new anchor', + user(), + ); + + expect(commentRepo.updateComment).toHaveBeenCalledWith( + { selection: 'new anchor' }, + 'c-1', + ); + expect(out).toBe(UPDATED); + }); + + it('is idempotent: no write when the anchor already matches', async () => { + const { service, commentRepo } = makeService(); + + const out = await service.resyncSuggestionAnchor( + suggestion({ selection: 'same' }), + 'same', + user(), + ); + + expect(commentRepo.updateComment).not.toHaveBeenCalled(); + expect(out).toEqual(suggestion({ selection: 'same' })); + }); + + it('rejects a non-author (only the suggestion owner may re-anchor)', async () => { + const { service, commentRepo } = makeService(); + await expect( + service.resyncSuggestionAnchor(suggestion(), 'new anchor', user({ id: 'other' })), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(commentRepo.updateComment).not.toHaveBeenCalled(); + }); + + it('rejects a reply / a comment with no suggestion', async () => { + const { service } = makeService(); + await expect( + service.resyncSuggestionAnchor( + suggestion({ parentCommentId: 'p-1' }), + 'x', + user(), + ), + ).rejects.toBeInstanceOf(BadRequestException); + await expect( + service.resyncSuggestionAnchor( + suggestion({ suggestedText: null }), + 'x', + user(), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('rejects re-anchoring an already applied or resolved suggestion', async () => { + const { service } = makeService(); + await expect( + service.resyncSuggestionAnchor( + suggestion({ suggestionAppliedAt: new Date() }), + 'x', + user(), + ), + ).rejects.toBeInstanceOf(BadRequestException); + await expect( + service.resyncSuggestionAnchor( + suggestion({ resolvedAt: new Date() }), + 'x', + user(), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('rejects a no-op selection equal to the suggested text', async () => { + const { service } = makeService(); + await expect( + service.resyncSuggestionAnchor( + suggestion({ suggestedText: 'new text' }), + 'new text', + user(), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); +}); diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index 85af14e0..a5911047 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -370,6 +370,76 @@ export class CommentService { return updatedComment; } + /** + * Re-sync a suggestion's stored `selection` (== apply-time expectedText) to the + * RAW substring the inline mark actually covers in the LIVE document (#496). + * + * The MCP client creates the comment from a DEBOUNCED REST snapshot, then + * anchors the mark in the live collab doc. When the two disagree (the doc moved + * on in the debounce window) the stored selection no longer equals the marked + * text, so EVERY apply 409s ("the commented text changed"). After anchoring the + * client re-reads the exact marked substring and calls this to store it, making + * apply's strict equality hold. + * + * Only meaningful for an un-settled top-level suggestion authored by the + * caller: applying/resolving freezes the anchor, and a reply-carrying thread is + * preserved rather than mutated. The new text must still differ from the + * suggestion (else "apply" would be a no-op), preserving create()'s invariant. + */ + async resyncSuggestionAnchor( + comment: Comment, + selection: string, + user: User, + ): Promise { + if (comment.creatorId !== user.id) { + throw new ForbiddenException( + 'You can only re-anchor your own suggestion', + ); + } + if (comment.parentCommentId) { + throw new BadRequestException( + 'Only a top-level comment can carry a suggested edit', + ); + } + if (!comment.suggestedText) { + throw new BadRequestException('This comment has no suggested edit'); + } + // A settled suggestion's anchor is frozen: re-anchoring an applied/resolved + // thread is meaningless and could resurrect a stale expectedText. + if (comment.suggestionAppliedAt || comment.resolvedAt) { + throw new BadRequestException( + 'Cannot re-anchor a suggestion that was already applied or resolved', + ); + } + const trimmed = selection.trim(); + if (trimmed.length === 0) { + throw new BadRequestException('The re-anchored selection cannot be empty'); + } + // Same no-op guard as create(): the suggestion must differ from the text it + // replaces, or apply becomes indistinguishable from already-applied. + if (trimmed === comment.suggestedText.trim()) { + throw new BadRequestException( + 'A suggested edit must differ from the selected text', + ); + } + + // Idempotent: nothing to persist when the anchor already matches. + if (comment.selection === selection) { + return comment; + } + + await this.commentRepo.updateComment({ selection }, comment.id); + + const updatedComment = await this.commentRepo.findById(comment.id, { + includeCreator: true, + includeResolvedBy: true, + }); + + // Re-anchoring only corrects stored metadata; it does not change the page + // text or the comment body, so no ws broadcast / notification is warranted. + return updatedComment; + } + /** * Apply the suggested edit carried by a top-level inline comment: atomically * replace the text under the comment mark in the collaborative document with diff --git a/apps/server/src/core/comment/dto/resync-suggestion-anchor.dto.ts b/apps/server/src/core/comment/dto/resync-suggestion-anchor.dto.ts new file mode 100644 index 00000000..003edc9d --- /dev/null +++ b/apps/server/src/core/comment/dto/resync-suggestion-anchor.dto.ts @@ -0,0 +1,20 @@ +import { IsString, IsUUID, MaxLength, MinLength } from 'class-validator'; + +/** + * #496: after the MCP client anchors a suggestion in the LIVE collab doc, it + * re-reads the exact substring under the new mark and syncs it here as the + * comment's stored `selection` (== apply-time expectedText). Fixes the perpetual + * 409 where expectedText came from a debounced REST snapshot while the mark sat + * in the live doc. + */ +export class ResyncSuggestionAnchorDto { + @IsUUID() + commentId: string; + + // The raw substring the mark now covers in the live document. Bounded like the + // create-time selection (2000) so a legitimate anchored span is never cut. + @IsString() + @MinLength(1) + @MaxLength(2000) + selection: string; +} diff --git a/packages/mcp/src/client/comments.ts b/packages/mcp/src/client/comments.ts index c1fd557d..9ba3dc17 100644 --- a/packages/mcp/src/client/comments.ts +++ b/packages/mcp/src/client/comments.ts @@ -450,6 +450,12 @@ export function CommentsMixin>( // can surface the closest-block / spans-multiple-blocks hint built from the // LIVE document (the pre-check page is not in scope there). let liveNotFoundError: Error | null = null; + // #496: the RAW substring the mark actually covers in the LIVE doc. The + // stored selection (payload.selection) came from a DEBOUNCED REST snapshot, + // which can differ from the live doc — and apply compares the marked live + // text to the stored selection strictly, so a stale snapshot 409s on EVERY + // apply. Captured here (same doc version the mark is set in) and synced below. + let liveAnchoredSelection: string | null = null; try { const collabToken = await this.getCollabTokenWithReauth(); // Open the collab doc by the canonical UUID, never the slugId (#260). The @@ -489,6 +495,12 @@ export function CommentsMixin>( } if (applyAnchorInDoc(doc, selection as string, newCommentId)) { anchored = true; + // For a suggestion, re-read the exact substring now under the mark + // (the mark is an attribute, so it does not change the raw text) to + // sync as the stored expectedText after the mutation resolves. + if (hasSuggestion) { + liveAnchoredSelection = getAnchoredText(doc, selection as string); + } return doc; } // Selection text not found in the LIVE document: abort the write. The @@ -527,6 +539,36 @@ export function CommentsMixin>( ); } + // #496: sync the stored selection (== apply-time expectedText) to the RAW + // substring the mark actually covers in the LIVE doc when it diverged from + // the debounced REST snapshot we stored at create time. Without this, apply + // strictly compares the marked live text to a stale stored selection and + // 409s every time. Best-effort: the comment is already correctly anchored, so + // a resync failure must NOT roll it back — it only risks a later apply 409, + // which we surface as a soft warning. + if ( + hasSuggestion && + liveAnchoredSelection != null && + liveAnchoredSelection !== payload.selection + ) { + try { + await this.client.post("/comments/resync-suggestion-anchor", { + commentId: newCommentId, + selection: liveAnchoredSelection, + }); + // Reflect the corrected anchor in the returned comment. + if (result.data) result.data.selection = liveAnchoredSelection; + } catch (e) { + if (process.env.DEBUG) { + console.error("Failed to resync suggestion anchor:", e); + } + result.warning = + "The suggestion was anchored, but its stored selection could not be " + + "synced to the live document; applying it may report a conflict if the " + + "text changed. Re-create the suggestion if Apply fails."; + } + } + // Soft warning (like editPageText): the selection only matched after // stripping markdown, so the caller likely quoted a styled fragment. if (anchorNormalized) { diff --git a/packages/mcp/test/mock/create-comment.test.mjs b/packages/mcp/test/mock/create-comment.test.mjs index 4936ccb4..d4ec8809 100644 --- a/packages/mcp/test/mock/create-comment.test.mjs +++ b/packages/mcp/test/mock/create-comment.test.mjs @@ -553,6 +553,189 @@ test("suggestedText: the stored selection is the doc's RAW typographic substring assert.equal(createPayload.suggestedText, "goodbye"); }); +// ----------------------------------------------------------------------------- +// 8b) #496: the DEBOUNCED REST snapshot (pages/info) DIFFERS from the LIVE collab +// doc (mutatePage) — the doc moved on in the debounce window. The stored +// selection is captured from the snapshot at create time, but the mark is set +// in the live doc, so apply would 409 forever. After anchoring, the client +// re-reads the RAW substring under the mark from the LIVE doc and POSTs it to +// /comments/resync-suggestion-anchor so the stored expectedText matches. +// ----------------------------------------------------------------------------- +test("suggestion: re-syncs the stored selection to the LIVE doc substring when the REST snapshot lagged", async () => { + let createPayload = null; + let resyncPayload = null; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (req.url === "/api/auth/login") { + sendJson(res, 200, { success: true }, { + "Set-Cookie": "authToken=t; Path=/; HttpOnly", + }); + return; + } + if (req.url === "/api/pages/info") { + // DEBOUNCED snapshot: ASCII quotes (stale — the live doc has since been + // typographically corrected). + sendJson(res, 200, { + data: { + id: "33333333-3333-3333-3333-333333333333", + content: { + type: "doc", + content: [ + { + type: "paragraph", + content: [{ type: "text", text: 'he said "hello" loudly' }], + }, + ], + }, + }, + }); + return; + } + if (req.url === "/api/comments/create") { + createPayload = JSON.parse(raw); + sendJson(res, 200, { + data: { + id: "cmt-resync-1", + content: createPayload.content, + selection: createPayload.selection, + suggestedText: createPayload.suggestedText, + type: createPayload.type, + }, + }); + return; + } + if (req.url === "/api/comments/resync-suggestion-anchor") { + resyncPayload = JSON.parse(raw); + sendJson(res, 200, { data: { id: "cmt-resync-1" } }); + return; + } + sendJson(res, 404, { message: "not found" }); + }); + + class TestClient extends DocmostClient { + async getCollabTokenWithReauth() { + return "collab-token"; + } + async resolvePageId() { + return "33333333-3333-3333-3333-333333333333"; + } + async mutatePage(pageId, collabToken, apiUrl, transform) { + // LIVE doc: SMART quotes (what the mark is actually set over). + const doc = { + type: "doc", + content: [ + { + type: "paragraph", + content: [{ type: "text", text: "he said “hello” loudly" }], + }, + ], + }; + const out = transform(doc); + return { doc: out, verify: { ok: true } }; + } + } + + const client = new TestClient(baseURL, "user@example.com", "pw"); + + const result = await client.createComment( + "33333333-3333-3333-3333-333333333333", + "please change", + "inline", + '"hello"', // ASCII quotes + undefined, + "goodbye", + ); + + assert.equal(result.success, true); + assert.equal(result.anchored, true); + // Create stored the STALE snapshot substring (ASCII quotes). + assert.equal(createPayload.selection, '"hello"'); + // …then the client re-synced to the LIVE marked substring (smart quotes). + assert.ok(resyncPayload, "/comments/resync-suggestion-anchor must be called"); + assert.equal(resyncPayload.commentId, "cmt-resync-1"); + assert.equal(resyncPayload.selection, "“hello”"); + // The returned comment reflects the corrected anchor. + assert.equal(result.data.selection, "“hello”"); +}); + +// ----------------------------------------------------------------------------- +// 8c) #496: when the REST snapshot and the LIVE doc AGREE, no resync round-trip +// is made (the common case must stay a single write). +// ----------------------------------------------------------------------------- +test("suggestion: no resync call when the snapshot already matches the live doc", async () => { + let resyncCalls = 0; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (req.url === "/api/auth/login") { + sendJson(res, 200, { success: true }, { + "Set-Cookie": "authToken=t; Path=/; HttpOnly", + }); + return; + } + if (req.url === "/api/pages/info") { + sendJson(res, 200, { + data: { + id: "44444444-4444-4444-4444-444444444444", + content: { + type: "doc", + content: [ + { type: "paragraph", content: [{ type: "text", text: "Hello brave world" }] }, + ], + }, + }, + }); + return; + } + if (req.url === "/api/comments/create") { + const p = JSON.parse(raw); + sendJson(res, 200, { + data: { id: "cmt-nosync-1", content: p.content, selection: p.selection, suggestedText: p.suggestedText, type: p.type }, + }); + return; + } + if (req.url === "/api/comments/resync-suggestion-anchor") { + resyncCalls++; + sendJson(res, 200, { data: {} }); + return; + } + sendJson(res, 404, { message: "not found" }); + }); + + class TestClient extends DocmostClient { + async getCollabTokenWithReauth() { + return "collab-token"; + } + async resolvePageId() { + return "44444444-4444-4444-4444-444444444444"; + } + async mutatePage(pageId, collabToken, apiUrl, transform) { + const doc = { + type: "doc", + content: [ + { type: "paragraph", content: [{ type: "text", text: "Hello brave world" }] }, + ], + }; + const out = transform(doc); + return { doc: out, verify: { ok: true } }; + } + } + + const client = new TestClient(baseURL, "user@example.com", "pw"); + const result = await client.createComment( + "44444444-4444-4444-4444-444444444444", + "rename", + "inline", + "brave", + undefined, + "bold", + ); + + assert.equal(result.anchored, true); + assert.equal(resyncCalls, 0, "matching snapshot must NOT trigger a resync round-trip"); +}); + // ----------------------------------------------------------------------------- // 8) #408: a not-found selection error QUOTES the closest block text so the // model can self-correct instead of blind-retrying. From eae7640f30b3ba85c376e9fd399f87cb43996807 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 23:43:26 +0300 Subject: [PATCH 6/6] =?UTF-8?q?test(comments):=20=D0=BD=D0=B5-=D0=B2=D0=B0?= =?UTF-8?q?=D0=BA=D1=83=D1=83=D0=BC=D0=BD=D1=8B=D0=B5=20=D1=82=D0=B5=D1=81?= =?UTF-8?q?=D1=82=D1=8B=20audit-swallow=20+=20dominant-run;=20CHANGELOG=20?= =?UTF-8?q?(#512=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DO1: тест audit-swallow был вакуумен (log() — fire-and-forget void persist(), .not.toThrow() зелен независимо от try/catch). Теперь: после failInsert() шпионим Logger.warn + слушаем unhandledRejection, флашим микро/макротаски, ассертим warn=1 и 0 floating-rejection. Mutation: убрать try/catch у persist → красный. DO2: dominant-run тест не отличал longest от first (самый длинный ран был и первым). Перестроено: короткий plain ран впереди, длинный bold — следом → ассерт bold:true держится только при genuine longest; +тест tie→first. Mutation: reduce→segments[0] → красный. DO3: CHANGELOG [Unreleased] — 3 записи по #496. DO4: убран no-op override insertAttrs.comment (=dominant.markAttrs, а он и есть attributes['comment'] — сегменты фильтруются по нему; {...attributes,comment: attributes.comment}={...attributes}); коммент про 'defensive re-assert' исправлен. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 22 +++++++++- .../server/src/collaboration/yjs.util.spec.ts | 40 +++++++++++++++--- apps/server/src/collaboration/yjs.util.ts | 9 ++-- .../audit/database-audit.service.spec.ts | 41 +++++++++++++++---- 4 files changed, 93 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adecf01f..da8e1690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -129,6 +129,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **A drifted comment suggestion can be re-synced instead of failing forever + with a 409.** A suggestion whose stored anchor no longer matched the live + document used to reject every apply attempt with an unrecoverable conflict; a + new resync path re-reads the live anchor so the suggestion applies against the + current text, and orphaned anchors (whose marked run was deleted) are + reconciled rather than left blocking. (#496) + - **Place several images side by side in a row.** A new "Inline (side by side)" alignment mode in the image bubble menu renders consecutive inline images as a row that wraps onto the next line on narrow screens. The row is @@ -352,7 +359,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 body-timeout so a legitimate >1-min idle between the model's tool calls no longer breaks a long-lived SSE socket (new `AI_MCP_SSE_BODY_TIMEOUT_MS`, default 10 min; see `.env.example`). (#489) - +- **Decisions on comment suggestions now leave a durable audit record.** + Applying or dismissing a comment suggestion hard-deletes the (childless) + subject comment, so the only surviving trace of who decided what is the audit + event — but the audit trail was wired to a Noop service that silently + swallowed every event. The trail is now DB-backed, so + `comment.suggestion_applied` / `comment.suggestion_dismissed` (and the other + comment-decision events) persist to the `audit` table and can be reviewed + after the comment is gone. A persistence failure is still swallowed with a + warning so it never breaks the originating request. (#496) +- **Applying a comment suggestion no longer strips the replaced run's inline + formatting.** The suggested text was re-inserted carrying only the comment + anchor mark, silently dropping bold/italic/code/link on the affected run; the + prevailing formatting of the replaced run is now carried onto the applied + text. (#496) - **The server no longer runs out of heap during long autonomous agent runs.** A new pnpm patch on `ai@6.0.134` stops the SDK from building a cumulative snapshot of the ENTIRE turn text on every streamed text-delta when no output diff --git a/apps/server/src/collaboration/yjs.util.spec.ts b/apps/server/src/collaboration/yjs.util.spec.ts index 93f4ea5a..f35e44f4 100644 --- a/apps/server/src/collaboration/yjs.util.spec.ts +++ b/apps/server/src/collaboration/yjs.util.spec.ts @@ -583,18 +583,20 @@ describe('replaceYjsMarkedText', () => { ]); }); - it('mixed formatting under the mark: replacement takes the DOMINANT run marks', () => { - // Marked run = "bold" (bold, 4 chars) + "x" (plain, 1 char), same commentId. - // The dominant (longer) segment is bold, so the flat replacement is bold. + it('mixed formatting under the mark: replacement takes the DOMINANT (longest) run, NOT the leading one', () => { + // Leading run is SHORT + plain ("x", 1 char); the following run is LONGER + + // bold ("bolded", 6 chars), same commentId. The longest run is deliberately + // NOT first: a "first-wins" pick would carry plain (no bold), so asserting + // bold on the result only holds if the code genuinely selects the LONGEST run. const { fragment, text } = buildFormatted([ + { text: 'x', attrs: { comment: { commentId: 'c1', resolved: false } } }, { - text: 'bold', + text: 'bolded', attrs: { comment: { commentId: 'c1', resolved: false }, bold: true }, }, - { text: 'x', attrs: { comment: { commentId: 'c1', resolved: false } } }, ]); - const result = replaceYjsMarkedText(fragment, 'c1', 'boldx', 'Z'); + const result = replaceYjsMarkedText(fragment, 'c1', 'xbolded', 'Z'); expect(result).toEqual({ applied: true, currentText: 'Z' }); expect(text.toDelta()).toEqual([ @@ -604,4 +606,30 @@ describe('replaceYjsMarkedText', () => { }, ]); }); + + it('mixed formatting under the mark: on a length tie the FIRST run wins', () => { + // Two equal-length runs (2 chars each) with different formatting, same + // commentId. The reduce keeps the accumulator on a tie, so the FIRST run + // (italic) prevails over the later bold one. + const { fragment, text } = buildFormatted([ + { + text: 'AA', + attrs: { comment: { commentId: 'c1', resolved: false }, italic: true }, + }, + { + text: 'BB', + attrs: { comment: { commentId: 'c1', resolved: false }, bold: true }, + }, + ]); + + const result = replaceYjsMarkedText(fragment, 'c1', 'AABB', 'Z'); + + expect(result).toEqual({ applied: true, currentText: 'Z' }); + expect(text.toDelta()).toEqual([ + { + insert: 'Z', + attributes: { comment: { commentId: 'c1', resolved: false }, italic: true }, + }, + ]); + }); }); diff --git a/apps/server/src/collaboration/yjs.util.ts b/apps/server/src/collaboration/yjs.util.ts index 25c6af27..03e9d687 100644 --- a/apps/server/src/collaboration/yjs.util.ts +++ b/apps/server/src/collaboration/yjs.util.ts @@ -267,12 +267,11 @@ export function replaceYjsMarkedText( // when the marked run mixes formatting we pick the DOMINANT segment (the one // covering the most characters) and apply its attributes — a v1 that preserves // the common single-format case exactly and, for a mixed run, keeps the - // prevailing style rather than losing all of it. The dominant segment already - // carries the `comment` mark (every collected segment does), so the anchor is - // preserved; we defensively re-assert it in case a future attribute shape - // omits it. + // prevailing style rather than losing all of it. `attributes` already carries + // the `comment` mark (every collected segment is filtered on it above), so the + // anchor is preserved by copying the run's attribute set verbatim. const dominant = segments.reduce((a, b) => (b.length > a.length ? b : a)); - const insertAttrs = { ...dominant.attributes, comment: dominant.markAttrs }; + const insertAttrs = { ...dominant.attributes }; node.delete(start, len); node.insert(start, newText, insertAttrs); diff --git a/apps/server/src/integrations/audit/database-audit.service.spec.ts b/apps/server/src/integrations/audit/database-audit.service.spec.ts index 90456c72..ba586166 100644 --- a/apps/server/src/integrations/audit/database-audit.service.spec.ts +++ b/apps/server/src/integrations/audit/database-audit.service.spec.ts @@ -1,3 +1,4 @@ +import { Logger } from '@nestjs/common'; import { DatabaseAuditService } from './database-audit.service'; import { AUDIT_CONTEXT_KEY } from '../../common/middlewares/audit-context.middleware'; import { AuditEvent, AuditResource } from '../../common/events/audit-events'; @@ -115,13 +116,39 @@ describe('DatabaseAuditService', () => { expect(inserted).toHaveLength(0); }); - it('a failed insert never throws to the caller (audit is a side-record)', async () => { - const { service, failInsert } = makeService(ctx()); - failInsert(); - // Must not reject / throw. - expect(() => service.log(applyPayload())).not.toThrow(); - await Promise.resolve(); - await Promise.resolve(); + it('a failed insert is swallowed with a warn and floats no rejection (audit is a side-record)', async () => { + // This pins the load-bearing swallow inside persist(). Because log() is + // fire-and-forget (`void this.persist(...)`), it always returns synchronously + // without throwing — so `not.toThrow()` alone would stay green even if the + // try/catch were removed. We instead observe the two effects the catch is + // responsible for: a warn IS emitted, and NO unhandled rejection floats. + // Removing persist()'s try/catch reddens both assertions (warn count 0 + a + // captured rejection). + const warnSpy = jest + .spyOn(Logger.prototype, 'warn') + .mockImplementation(() => undefined as any); + const rejections: unknown[] = []; + const onRejection = (err: unknown) => rejections.push(err); + process.on('unhandledRejection', onRejection); + try { + const { service, failInsert } = makeService(ctx()); + failInsert(); + expect(() => service.log(applyPayload())).not.toThrow(); + // Flush microtasks so the rejected insert settles, then give any floated + // rejection a macrotask tick to be reported by the runtime. + await Promise.resolve(); + await Promise.resolve(); + await new Promise((r) => setImmediate(r)); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(String(warnSpy.mock.calls[0][0])).toContain( + 'Failed to persist audit event', + ); + expect(rejections).toHaveLength(0); + } finally { + process.off('unhandledRejection', onRejection); + warnSpy.mockRestore(); + } }); it('logWithContext() persists with an explicit (non-request) context', async () => {