Resolve the code-review findings from comment #1571 on PR #119. Engine (packages/git-sync): - Idempotent CREATE on retry: before createPage, look the page up in the live Docmost tree by (parentPageId, title) and ADOPT it instead of duplicating when a prior cycle created it but failed to persist the pageId back to disk. Only trust a COMPLETE tree for the lookup; fall back to createPage otherwise. Covered by new tests incl. a complete=false regression-lock. - Route applyPullActions diagnostics through an injected logger instead of bare console (thread log from the cycle). - Add a timeout to the git execFile chokepoint (runRaw) so a hung git subprocess cannot wedge a sync cycle. - Translate remaining Russian code comments to English. - Remove dead standalone-CLI code (parseArgs/PushParsedArgs, parseSettings/envSchema, loadSettingsOrExit + config-errors.ts) and the matching index exports/specs; keep the Settings type. - Fix the dangling docs link in package.json. - Add a schema-surface snapshot guard so any drift in the vendored document schema is a loud, must-review CI failure (+ provenance header). Server (apps/server): - Add a configurable watchdog timeout to the spawned git http-backend so a stalled push cannot hold the per-space lock forever (GIT_SYNC_BACKEND_TIMEOUT_MS). - Close the in-process TOCTOU window in SpaceLockService.withSpaceLock by reserving the slot synchronously before acquire. - Add tests: removePage git-sync provenance (both branches), ensureServable force-push-protection git configs, and the phase-B+ datasource methods. Docs / build: - AGENTS.md: list git-sync as the fifth workspace package and note the three schema mirrors; fix the dangling git-sync-plan.md backlog link. - pnpm-lock.yaml: add the missing @docmost/git-sync workspace link so pnpm install --frozen-lockfile (CI default) succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
11 KiB
Дублирование определений инструментов: in-app агент vs standalone MCP-пакет
Статус: частично закрыто. Квирк «node как объект ИЛИ JSON-строка» вынесен
в общий хелпер parseNodeArg (см. «Прогресс» ниже); остальной долг (единый
реестр спеков + унификация конвертера) всё ещё открыт. Это forward-looking
стоимость поддержки, НЕ баг — код корректен сегодня. Держим запись открытой,
чтобы при росте набора инструментов долг не разъезжался молча.
Прогресс
- ✅ Квирк node-arg вынесен в хелпер (
refactor/ai-chat-tool-spec-registry, PR #114). Шесть рукописных копий нормализации «node как объект ИЛИ JSON-строка» свёрнуты вparseNodeArg: по одному источнику на пакет —packages/mcp/src/lib/parse-node-arg.ts(standalone) иapps/server/src/core/ai-chat/tools/parse-node-arg.ts(in-app). Две копии намеренны (ESM/CJS-граница), поведение тождественно. - ⏳ Единый реестр спеков (схема + описание на инструмент) и вывод
DocmostClientLikeиз реального типа — отложены (см. «Фикс»): требуют пересечения ESM/CJS-границы для данных+zod и ломают тест-стабы in-app инструментов при точных типах. Делать инкрементально. - ⏳ Унификация конвертера ProseMirror ↔ Markdown — открыта (см. раздел
«Расширение …» ниже); на неё опирается план git-синка
(
docs/backlog/git-sync-thin-meta.md).
Суть
Один и тот же набор инструментов поверх одного DocmostClient описан
тремя независимыми рукописными слоями. Каждое добавление инструмента или
правка его model-facing описания требует синхронной правки в 2–3 местах, а
parity-баги (расхождение копий) приходится чинить/переоткрывать дважды.
Где дублируется (три слоя)
- Standalone MCP-сервер —
packages/mcp/src/index.ts(~38registerTool). Для внешних MCP-клиентов (stdio/http). На каждый инструмент: zod-схема + длинное model-facing описание + тонкийexecute, вызывающийDocmostClient. - Встроенный AI-чат —
apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts(~39tool({...})черезai-SDK). Своя zod-схема + своё описание + свойexecuteповерх ТОГО ЖЕ клиента (@docmost/mcpгрузится вtools/docmost-client.loader.ts:188через динамическийimport()). - Ручная копия сигнатур — интерфейс
DocmostClientLikeвapps/server/src/core/ai-chat/tools/docmost-client.loader.ts:9(в комментарии прямо: «Signatures here mirror that file exactly»), скопирован руками изpackages/mcp/src/client.ts.
Что именно продублировано (с подтверждением по коду)
- zod-схема + описание каждого инструмента — в слоях 1 и 2 целиком.
Квирк «node как объект ИЛИ JSON-строка» реализован дважды (НЕ в общем клиенте)— закрыто (PR #114): вынесен вparseNodeArg(по хелперу на пакет), 6 inline-копий устранены:- in-app:
patchNode,insertNode,updatePageJson→apps/server/src/core/ai-chat/tools/parse-node-arg.ts; - standalone:
patch_node,insert_node,update_page_json→packages/mcp/src/lib/parse-node-arg.ts.
- in-app:
- Guardrail/семантика
transformPage(dryRun) описана в обоих:ai-chat-tools.service.ts:~935иindex.ts:~1006.
Почему разделение слоёв 1 и 2 само по себе оправдано
У путей разный транспорт и auth-контекст, и это правильно держать раздельно:
in-app путь чеканит per-user JWT + provenance collab-токен (подписанная
agent-claim, docmost-client.loader.ts:159 — getCollabToken; см. план §6.5),
а standalone обслуживает внешних клиентов по stdio/http. Но это оправдывает
два тонких адаптера (execute + auth-обвязка), а НЕ две рукописные копии
МЕТАДАННЫХ (схема + описание + квирки). Метаданные можно объявить один раз и
переиспользовать обоими транспортами.
Доказательство стоимости (наблюдалось при фиксе edit_page_text)
При исправлении ложного «успеха» edit_page_text (refuse форматных правок +
verify-отчёт):
- Поведение легло в общий
DocmostClient→ автоматически дошло до обоих агентов ОДНОЙ правкой. Это «хороший» случай — логика в едином источнике. - Описание инструмента пришлось править ДВАЖДЫ: в
index.ts(кодером) и отдельно вai-chat-tools.service.ts:617, где описание продолжало рекламировать «Markdown wrappers tolerated via strip-and-retry» — ровно ту формулировку, что ввела исходного агента в заблуждение. Копия молча разъехалась и какое-то время встроенный агент получал устаревшую подсказку. Это и есть материализованный parity-баг.
Расширение: дублируется не только описания инструментов — ещё и конвертер (PM ↔ Markdown)
Зафиксировано при планировании встраивания git-синка (docmost-sync → gitmost,
нативная in-process интеграция). Та же болезнь «несколько рукописных копий одного
кода» теперь касается слоя конвертации ProseMirror ↔ Markdown и его lib, а не
только метаданных инструментов.
- Копия в gitmost —
packages/mcp/src/lib/:markdown-converter.ts(~885 строк),markdown-document.ts(~136),node-ops.ts,diff.ts,docmost-schema.ts. Канонизатора (canonicalize.ts) здесь НЕТ. - Копия в docmost-sync —
packages/docmost-client/src/lib/: тот же набор +canonicalize.ts(~11 КБ, держит идемпотентность round-trip, SPEC §11) +markdown-document.tsс режимом «тело + якоря, без тредов комментов» (includeCommentThreads:false, на ~20 строк больше). - Третья копия (планируется) — план git-синка вендорит чистую часть
конвертера в новый
packages/git-sync(collab-файл не нужен: запись идёт нативно черезopenDirectConnection+@docmost/editor-ext).
Копии уже молча разъехались (docmost-sync vs packages/mcp): collaboration.ts
~329 изменённых строк, node-ops.ts ~53, markdown-converter.ts ~24,
markdown-document.ts ~20. Отдельно: docmost-schema.ts в lib дублирует
реальную схему сервера @docmost/editor-ext (её использует collab/persistence)
— расхождение схем = риск битой конвертации нод.
Вывод: тот же фикс-вектор (единый источник правды), что и для инструментов, стоит
распространить на конвертер — общий пакет конвертации, потребляемый mcp,
git-sync и (в идеале) сервером. До конвергенции git-sync держит вендоренную
копию валидированного конвертера с гейтом round-trip против схемы editor-ext
(осознанный долг «третья копия сейчас, объединяем позже»).
Фикс
Единый реестр спеков (полное устранение дублирования).** Вынести в
packages/mcp один источник на инструмент: name + zod-схема + model-facing
описание + общий хелпер нормализации node-строки (для patch/insert/update).
И index.ts, и ai-chat-tools.service.ts импортируют спеки и добавляют только
свой execute/auth. DocmostClientLike — выводить из типа реального клиента
(type-only import / генерация), а не копировать руками.
- Ограничение:
@docmost/mcp— ESM-only, сервер грузит его через трюкnew Function('import(specifier)')(docmost-client.loader.ts:174), потому чтоmodule:commonjsдаунлевелитimport()вrequire(). Реестр спеков (данные + zod) должен пересекать ту же ESM/CJS-границу — выполнимо тем же динамическим импортом;ai-SDKtool()и MCPregisterTool()имеют разную форму, поэтому реестр экспортирует транспорт-агностичные{name, schema, description}, а каждая сторона оборачивает их сама.zod— общая зависимость обоих пакетов, типы переносятся.