From 59db81b06bd0de9a0c64b1966ff300d659fdb286 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 18:09:10 +0300 Subject: [PATCH] =?UTF-8?q?fix(comments):=20suggestion=20=D1=81=20=D0=B2?= =?UTF-8?q?=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.