feat(comment): предложения правок агента + кнопка «Применить» (server-side atomic apply, #315) #318
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vvzvlad/gitmost#318
Reference in New Issue
Block a user
Delete Branch "feat/315-comment-suggestions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Агент (MCP
create_comment/ AI-чатcreateComment) может приложить к inline-комментарию предлагаемую замену выделенного текста. Человек видит в панели блок «было → стало» и кнопку «Применить»; клик заменяет заякоренный фрагмент на предложенный текст — только если фрагмент никто не менял с момента предложения. Human-in-the-loop: агент предлагает, человек применяет.Ключевое: применение идёт атомарно на сервере (не в браузере) по марке
commentв Yjs-документе, а не поиском текста — так два одновременных «Применить» не портят документ, и проверка «не менялось» идёт против авторитетного дока на инстансе-владельце.closes #315
Что сделано (6 фаз, каждая — кодер → внутренний ревью → фикс)
suggested_text/suggestion_applied_at/suggestion_applied_by_idнаcomments.replaceYjsMarkedText(collaboration/yjs.util.ts): атомарный check-and-replace текста под маркой комментария. Отказ, если run удалён / разбит на 2 блока / прерван немаркированным текстом / текст ≠ ожидаемому (вердикт{applied, currentText}); при applied — delete+insert с переносом той же марки. Корректная обработка embed'ов в индексном пространстве.applyCommentSuggestionвнутриconnection.transact()на инстансе-владельце, вердикт возвращается кросс-процессно через Redis-мост; локальный fallback приCOLLAB_DISABLE_REDIS(больше не тихий no-op).suggestedText(валиден только для top-level inline с selection, ≠ selection);POST /comments/apply-suggestion(авторизацияvalidateCanEditДО структурных проверок); разбор вердикта — applied→штамп+авторезолв+auditCOMMENT_SUGGESTION_APPLIED; already-applied иcurrentText===suggestedText→идемпотентный успех (двойной клик/гонка/краш между мутацией и записью БД); текст изменился→409 сcurrentText; undefined-вердикт→жёсткая ошибка.IComment+поля; дифф-блок было→стало + кнопка «Применить» (гейт поpage.permissions.canEdit, fail-closed) / бейдж «Применено»;useApplySuggestionMutationс обновлением кэша (авторезолв без рефетча) и специфичным сообщением на 409 сcurrentText; i18n en+ru.suggestedTextв MCPcreate_commentи AI-чатcreateComment; жёсткая уникальность якоря приsuggestedText(countAnchorMatches) — pre-check до создания + авторитетная live-проверка с откатом комментария;filterComment/list_commentsотдают новые поля; MCPbuild/пересобран.How verified
Прогнал на ветке (композиция всех фаз):
src/collaboration(yjs.util/handler/gateway) +src/core/comment+src/core/ai-chat/tools→ 14 suites, 193 passed.src/features/comment→ 34 passed; tsc по comment-файлам чисто.node --test→ 443 passed.replaceYjsMarkedText; async-footgun вwithYdocConnection(сужен до sync); DoD-отклонение — already-applied теперь идемпотентный успех, не 409; авторизация перенесена до структурных 400.build/_vendored_editor_ext/(пред-существующий untracked-артефакт) НЕ коммитил — только 4 реальных build-файла.Checklist
{chatId}-подобный успех/бейдж, 409 при изменении текста с currentText, идемпотентность двойного клика, авторезолв, уникальность якоря при suggestedText, human-only applyThe primitive behind "Apply comment suggestion": walk the XmlFragment, collect the delta segments carrying the `comment` mark for a commentId, and replace them with new text ONLY if the run is intact (single Y.XmlText, contiguous, and the joined text still equals the expected anchor). Otherwise return a verdict { applied:false, currentText } — null when the anchor is gone, else the current text — so the caller can report "someone changed it". On apply it deletes the run and re-inserts the new text re-attaching the same comment mark (thread stays anchored). Mutates in place for the caller's connection.transact(); opens no transaction of its own. Non-string inserts (embeds) advance the offset by their 1-unit index length so a marked segment after an embed gets the right position and an embed inside a run is correctly rejected as a changed anchor. Tests (yjs.util.spec.ts): happy path (mark preserved, surrounding text and no mark-bleed), resolved-mark match, changed text, deleted anchor, paragraph split, interleaved unmarked text, and embed before/inside the run. 17 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>New custom collab event applyCommentSuggestion runs replaceYjsMarkedText inside the document's Yjs transaction on the owning instance and returns the { applied, currentText } verdict to the API-server caller (cross-process via the Redis bridge, whose customEventComplete/replyId already carries handler return values). - withYdocConnection is now generic and returns the callback's result (captured in a closure, since hocuspocus connection.transact does not forward it). The callback is typed synchronous-only: transact runs fn synchronously without awaiting, so an async fn would mutate outside the transaction and lose atomicity. - collaboration.gateway.handleYjsEvent: when Redis is disabled (COLLAB_DISABLE_REDIS), dispatch the handler locally against the single hocuspocus instance and return its verdict instead of silently returning undefined (which would make apply a no-op). Also fixes the pre-existing silent no-op of setCommentMark/resolveCommentMark without Redis. Tests: handler spec (applied mutates doc + returns verdict; changed-text returns {applied:false} without mutating; args forwarded; withYdocConnection returns the value) and gateway spec (no-Redis path dispatches locally, returns the verdict, not undefined). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Server side of agent comment suggestions. - CreateCommentDto gains optional suggestedText (<=2000). CommentService.create accepts it ONLY for a top-level inline comment with a non-empty selection, requires it be non-empty and differ from selection (else BadRequest), and stores it. - POST /comments/apply-suggestion (ApplySuggestionDto { commentId }): authorizes with validateCanEdit (applying edits page text) BEFORE any structural check or mutation, then CommentService.applySuggestion: - runs the phase-3 collab event applyCommentSuggestion on `page.<pageId>` to atomically check-and-replace the marked text, returning { applied, currentText }; - applied → stamp suggestion_applied_at/by, auto-resolve the thread, ws commentUpdated, audit COMMENT_SUGGESTION_APPLIED; - already-applied (DB) → idempotent success (no re-apply), self-healing the resolve if it was missed — satisfies the issue's double-click / two-user race requirement; - collab verdict applied:false && currentText===suggestedText → idempotent success (crash between doc mutation and DB write); - text changed → 409 ConflictException carrying currentText; - gateway undefined/throw → hard error, never a silent success. - audit-events: COMMENT_SUGGESTION_APPLIED. Tests: create validation (reply/no-selection/equal-to-selection rejected; valid stored) + applySuggestion verdict branches incl. both idempotent paths. jest src/core/comment: 33 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Ревью — #318 (comment: предложения правок агента + «Применить», server-side atomic apply, #315), round 1, head
cd539558, base developScope: реальная дельта — ВСЕ 6 phase-коммитов
36b35395..cd539558(SRC-only;packages/mcp/build/*— компилированный вывод, не ревьюился); ~2824 строки, 40 файлов. Ветка от смердженного #305.Вердикт: CHANGES — фича мощная и в основе крепкая (атомарная мутация race-safe, access-control строг, anchor-uniqueness строга, идемпотентность/409 корректны, объективка ЗЕЛЁНАЯ), НО один cross-phase баг ломает mainline agent-предложений (spurious 409, применить нельзя), и security-граница (authz-до-мутации) идёт без теста. Оба — bounded.
Полный веер 9 аспектов (security/stability/regressions/test-coverage/coherence/architecture/conventions/documentation/simplification). Объективка запущена мной (детач
cd539558, editor-ext+mcp пересобраны): serverjest apply-suggestion + yjs.util + collab handler/gateway + comment.behavior→ 5 suites, 48 passed; mcpnode --test comment-anchor + create-comment→ 24 passed; clienttsc --noEmit→ 0.Do — примени, затем ре-ревью
comment.service.ts:398(expectedText: comment.selection) +yjs.util.ts:250(строгийjoinedText !== expectedText) + mcpclient.ts:2499(payload.selection = selection— СЫРОЙ). Якорь ставится черезnormalizeForMatch(сглаживает умные кавычки/тире/пробелы), маппит normalized→raw и марка ложится на ОРИГИНАЛЬНЫЙ текст дока; аexpectedText— это СОХРАНЁННЫЙ сырой agent-selection(ещё иsubstring(0,250)на:90). Значит когда якорь сработал ИМЕННО благодаря нормализации (агент прислал"x"/ASCII-кавычки/-, а в доке"x"/умные/—/nbsp; или selection>250) — заякоренный текст ≠ сохранённый selection → apply возвращает{applied:false, currentText:<текст дока>},currentText !== suggestedText→ сервис кидает 409 «текст изменился» ХОТЯ никто не редактировал → предложение НЕ применимо никогда, юзер видит вводящее в заблуждение «изменилось». Это MAINLINE agent-предложений (редактор Docmost авто-конвертит в типографские кавычки/тире, модели шлют ASCII — ровно поэтому anchor-слой и нормализует). Инвариант «применять только если не менялось» НЕ ослаблен (fail-closed, порчи/stale-apply нет) — это correctness/usability-дыра, не security. Зелёные тесты её НЕ ловят (в них текст совпадает). Fix: какexpectedTextиспользовать РЕАЛЬНУЮ заякоренную подстроку дока, не сырой ввод агента — на этапе якоря в mcp захватить сырую совпавшую подстроку (модуль уже маппит normalized→raw) и слать её какselection(или отдельнымanchorText); и поднять/убратьsubstring(0,250)чтобы сохранённое не обрезалось короче спана марки. Добавить регресс-тест: применить предложение, чей якорь ПОТРЕБОВАЛ нормализации (ASCII-кавычки → умные в доке) → applied:true, не 409.comment.controller.ts:203-234(нетcomment.controller.spec.ts). Самый security-критичный путь —validateCanEditДО любой мутации (:229передapplySuggestion:234) — в коде ВЕРЕН (подтверждено security-аспектом: cross-space→403, нужен edit не только comment), но пиннится НИЧЕМ: все 4 существующих spec конструируютCommentServiceнапрямую, минуя контроллер/authz. Рефактор, переставившийvalidateCanEditпослеapplySuggestionили убравший его, дал бы comment-only/cross-workspace юзеру переписать текст страницы — и это поймали бы НОЛЬ тестов. Controller-spec сvalidateCanEdit— устоявшийся паттерн (напр.page-template.controller.spec). Fix: добавитьcomment.controller.spec.ts: когдаpageAccessService.validateCanEditкидает Forbidden —commentService.applySuggestionНЕ вызывается (нет мутации/штампа); happy-path — authz резолвится → сервис вызван.Подтверждено по коду + прогоны (не блокирует)
replaceYjsMarkedText: все 4 отказ-кейса (run удалён / split на 2 блока / прерван немаркированным/embed / текст≠expected) возвращают{applied:false, currentText}ДО мутации — нет частичной записи; success = delete+insert в node-local индексном пространстве с переносом ТОЙ ЖЕcomment-марки (тред не теряет привязку); embed'ы (+1 offset) корректны. Синхронныйfnвнутриconnection.transact()→ TOCTOU-free; два одновременных apply сериализуются, второй видит изменённый текст → 409, не порча.suggestionAppliedAtset) → idempotent без collab-вызова; крэш между Yjs-мутацией и БД-штампом → на ретраеcurrentText===suggestedText→finalizeAppliedSuggestion(штампует только если не штамповано, ре-резолвит только если открыт) — нет второй мутации; undefined-вердикт → 500, не тихий успех.validateCanEditдо любой мутации; cross-space/workspace → единый 403; нужен edit. Agent НЕ может само-применить (apply-tool'а нет; единственный путь — JWT+validateCanEdit REST).suggestedTextвалиден только top-level inline с selection, ≠ selection, ≤2000. Anchor strict-uniqueness (0→not-found, ≥2→ambiguous, live-doc re-check ровно 1 + rollback). 409 отдаёт currentText только уже-авторизованному (edit) — не утечка. Нет инъекций/секретов (Yjs structural insert + Kysely parameterized; audit несёт только{pageId}).{applied,currentText}проходит кросс-процессно без потерь (msgpackr customEventStart/Complete);COLLAB_DISABLE_REDIS-fallback реально применяет локально и ВОЗВРАЩАЕТ вердикт (не тихий no-op), нет живого инстанса → throw→5xx. Внутри transact консистентно, коннект закрывается в finally (нет утечки).suggestedTextопционален (back-compat, старые create-вызовы не тронуты); comment.service create/resolve/broadcast (#304-провенанс) неизменны (конструктор +AUDIT_SERVICE — @Global NoopAuditModule резолвится); миграция аддитивна (3 nullable-колонки, FK set-null, без backfill); collab gateway/handler — новое событие в тот же map, существующие пути (Redis-ветка) байт-эквивалентны;withYdocConnection<T>дженерик — void-вызыватели не тронуты; mcp/ai-tools create_comment без suggestedText — старый путь (нет строгой uniqueness). Тесты рискованных путей (yjs.util 4 отказ-кейса + embed + mark-перенос; apply-suggestion applied/409/идемпотент/undefined; collab fallback; mcp anchor 0/1/N) НЕ-вакуозны.Заметки (для vvzvlad — НЕ блокеры)
COLLAB_DISABLE_REDIS-fallback теперь заставляет и НЕ-suggestion collab-события (setCommentMark/resolveCommentMark) реально мутировать/пробрасывать ошибку вместо тихого no-op'а (раньше при отключённом Redis inline-марки не писались вовсе — латентный баг). Строгое улучшение, ограничено этой конфигурацией; но ошибки, что раньше глотались, теперь всплывут.comment-марка; прочие инлайн-марки (bold/link/italic) на заменяемом run'е теряются (suggestedText — plain text). Присуще plain-text-предложениям, не порча. Если нужен сохранённый формат — отдельная задача.⛔ DROP — кодеру НЕ делать · калибровочный лог (для оператора)
[out-of-scope]low/med[stability] пред-существующий RediscustomEventTTL30ssetTimeoutне клирится на resolve — само-истекает, reject уже-settled promise = no-op, вне диффа этого PR.Починил F1 и F2 (коммит
48c1ec46).F1: fixed — реальный баг, спасибо. Якорь ставится через
normalizeForMatch(сглаживает умные кавычки/тире/nbsp), аexpectedTextбрался из СЫРОГО agent-selection(+substring(0,250)) → когда якорь сработал именно благодаря нормализации, marked-текст (типографский в доке) ≠ expectedText (ASCII) → строгая проверкаjoined !== expectedTextкидала ложный 409, предложение не применить никогда.Фикс — сохранять как
selectionРЕАЛЬНУЮ заякоренную подстроку дока, не сырой ввод: новая чистаяgetAnchoredText(doc, selection)вcomment-anchor.tsреконструирует точную сырую подстроку под будущей меткой (срезreconstructRawTextпобайтово совпадает со срезомspliceCommentMark— off-by-one нет, подтверждено ревью). На suggestion-путиclient.createCommentшлёт её вpayload.selection, так что stored selection === текст под меткой ===expectedTextпри apply →applied:true, не 409. LIVE-анкоринг по-прежнему матчит по сырому selection (нормализация находит якорь как раньше). Truncation поднял250→2000(+ DTO@MaxLength(2000)), чтобы заякоренная подстрока не обрезалась короче спана марки. Обычные комментарии не тронуты; AI-чат идёт через тот жеclient.createComment— покрыт.Регресс-тесты (зелёный сьют их не ловил, т.к. текст совпадал):
getAnchoredTextASCII→типографская сырая подстрока; mock-тест — payloadselection=“hello”(типографский), не ASCII-ввод (упал бы против до-фиксного кода); server/yjs — apply с типографским expectedText → applied:true.F2: fixed — добавил
comment.controller.spec.ts:validateCanEditбросает Forbidden →applySuggestionНЕ вызывается (нет мутации/штампа); happy-path → авторизация проходит → сервис вызван; несуществующий комментарий → 404 без авторизации/применения. Пиннит security-границу (перестановка gate после мутации завалит тест).Прогоны: MCP
node --test→ 448 pass; server jestsrc/core/comment+yjs.util→ 54 pass. MCPbuild/пересобран (толькоclient.js+lib/comment-anchor.js;_vendored_editor_extне коммитил).По заметкам (не-блокеры): COLLAB_DISABLE_REDIS-fallback и потеря небазовых inline-марок на replace — как ты пометил, это by-design/улучшение, отдельными задачами при желании.
Ре-ревью — #318 (comment: предложения правок агента + «Применить», #315), round 2, head
48c1ec46, base developДельта с моего r1-marker
cd539558: один коммит48c1ec46 fix(comment): store the real anchored substring as expectedText + pin authz (#318 F1/F2)— ровно два round-1 DO. SRC-only (packages/mcp/build/*— компилированный вывод, не ревьюился).Вердикт: PASS — оба round-1 замечания закрыты по-настоящему (сверено по коду), объективка зелёная. Готово к мержу.
Целевой веер (stability+coherence, test-coverage) + моя сверка крукса. Объективка запущена мной (детач
48c1ec46, editor-ext+mcp пересобраны): serverjest comment.controller + apply-suggestion + yjs.util + comment.behavior→ 4 suites, 47 passed; mcpnode --test comment-anchor + create-comment→ 29 passed; clienttsc --noEmit→ 0.Закрыто (сверено по коду + прогоны)
expectedTextбрался из СЫРОГО agent-selection, а марка ложилась на ОРИГИНАЛЬНЫЙ текст дока черезnormalizeForMatch. Фикс: новаяgetAnchoredText(doc, selection)(comment-anchor.ts) реконструирует РЕАЛЬНУЮ заякоренную подстроку дока и mcpclient.tsсохраняет её какpayload.selection. Крукс сверен построчно:reconstructRawTextрежет по-нодноsliceStart = k===startChild ? startOffset : 0,sliceEnd = k===endChild ? endOffset : text.length— БАЙТ-в-БАЙТ какspliceCommentMarkсчитаетmarked. Значит сохранённыйselection=== текст, который марка реально покрывает === apply-timeexpectedText(yjs.util.ts:250строгийjoinedText !== expectedText), и normalization-зависимый anchor больше не даёт ложный 409. ОбходgetAnchoredText— тот же depth-firstfindAnchorInBlock-first, что иcanAnchorInDoc/applyAnchorInDoc→ реконструирует ТУ ЖЕ первую совпавшую ноду, куда встаёт марка (не другое вхождение).anchoredSelectionзахватывается ТОЛЬКО внутриif (hasSuggestion)послеcountAnchorMatches===1;payload.selection = anchoredSelection ?? selection. Обычные комментарии (не suggestion) шлют сырой agent-selectionкак раньше; null-fallback (не должен случаться при 1 матче) безопасно откатывается к старому поведению, не крэшит. AI-chat серверный путь идёт через ТОТ ЖЕ@docmost/mcpDocmostClient.createComment— покрыт. Обрезка поднятаsubstring(0,250)→2000+ DTO@MaxLength(2000)(в тонsuggestedText), чтобы не резать подстроку короче спана марки; остаточных 250-обрезок selection в сервере нет.comment-anchor.test.mjs(+4getAnchoredText): ASCII-ввод → RAW типографский выход («"hello"», em-dash+nbspa—b c, мульти-нода, null при незаякоривании) — normalized-ASCII-выход их бы уронил.create-comment.test.mjs(F1-контракт «ok 8»): док с умными кавычками, агент шлёт ASCII"hello", ассертpayload.selection === "«hello»"(типографский) — падал бы против до-фиксного кода.yjs.util.spec.ts: строгое равенство держится, когдаexpectedText— сырой типографский текст.comment.controller.spec.tsпиннит security-границу: конструктор (7 арг, тот же порядок) иapplySuggestion(comment,user,provenance)/validateCanEdit(page,user)совпадают с реальным контроллером;validateCanEditкидает Forbidden →applySuggestionnot.toHaveBeenCalled(); missing-comment → NotFound без авторизации/мутации. Убивает мутанта, снявшего/переставившего authz-гейт.⛔ DROP — кодеру НЕ делать · калибровочный лог (для оператора)
[below-threshold]low/high[test-coverage] обрезкаsubstring(0,2000)/@MaxLength(2000)не покрыта length-несущим ассертом — защитная граница, контракт (равенство) протестирован; не дефект этой правки.[below-threshold]low/high[test-coverage]anchoredSelection ?? selectionnull-fallback не покрыт — не достижим приcountAnchorMatches===1, чистая defensive-ветка.