refactor(ai-chat): move patch_node/insert_node into the shared tool-spec registry (#294) #305

2026-07-03T05:56:01+03:00

agent_coder commented

2026-07-03 05:56:01 +03:00

Summary

Продвигает #294 (дублирование определений инструментов). Общий transport-agnostic реестр SHARED_TOOL_SPECS уже дедуплицирует 14 инструментов; этот PR переносит ещё два действительно идентичных — patch_node/patchNode и insert_node/insertNode.

Почему только два (а не все): описания инструментов — это model-facing промпты, слияние с потерей формулировки меняет поведение агента. Поэтому мигрирую только там, где схема идентична И описание можно слить БЕЗ потери guidance ни с одной стороны. Каноническое описание — строгий СУПЕРСЕТ обоих: сохранены и MCP-«without resending the whole document» + table-структура/anchor-гайд, и in-app-«reversible via page history» / «exactly one of anchorNodeId or anchorText». Схема идентична (in-app-сторона лишь получает .min(1) на id — безопасное ужесточение). Каждый транспорт держит свой execute/auth; in-app parseNodeArg (node как JSON-строка) не тронут.

Осознанно НЕ слито (задокументировано):

3 table-инструмента (table_insert_row/delete_row/update_cell) — реальное расхождение имени параметра table (MCP) vs tableRef (in-app); force-переименование сломало бы контракт одной стороны. Комментарий на обеих сторонах.
search/sharePage/createComment/transformPage/listPages — намеренная per-transport дивергенция (гибрид RRF, security-подтверждение, current-page контекст и т.п.). Короткий коммент на каждом, объясняющий почему остаётся раздельно (как issue и просил «flag as intentional divergence»).
DocmostClientLike — рукописное зеркало: ESM/CJS-граница не даёт compile-time type-import (пакет без .d.ts, нет path-mapping); runtime drift-guard уже пиннит имена методов.

Заодно фикс латентного бага в shared-tool-specs.contract.spec.ts: деривация required через instanceof z.ZodOptional (совпадает с эмиттером z.toJSONSchema) вместо isOptional(), который ошибочно считал z.any()-поля опциональными.

How verified

MCP tool-specs.test.mjs 7/7 (новые проверки: каноническое описание содержит ключевые фразы ОБОИХ оригиналов) + client-host-contract drift-guard 3/3; полный MCP unit+mock 432/432.
Server tsc 0 по затронутым; jest src/core/ai-chat/tools — 51/51 (incl. contract-spec).
pnpm --filter @docmost/mcp build чисто; коммитнут пересобранный build/ (он в репо трекается).
Внутреннее ревью (риск потери model-facing guidance) — APPROVE: пословно сверил оба описания, ничего не потеряно, добавленный table-insert-гайд правдив (in-app client — тот же DocmostClient).

Note

Partially addresses #294 — оставшийся долг (унификация конвертера) — это отдельный #293; здесь закрыта дедупликация метаданных для мигрируемого подмножества, остальное осознанно оставлено раздельным с документацией.

## Summary Продвигает #294 (дублирование определений инструментов). Общий transport-agnostic реестр `SHARED_TOOL_SPECS` уже дедуплицирует 14 инструментов; этот PR переносит ещё два **действительно идентичных** — `patch_node/patchNode` и `insert_node/insertNode`. **Почему только два (а не все):** описания инструментов — это model-facing промпты, слияние с потерей формулировки меняет поведение агента. Поэтому мигрирую только там, где схема идентична И описание можно слить БЕЗ потери guidance ни с одной стороны. Каноническое описание — строгий СУПЕРСЕТ обоих: сохранены и MCP-«without resending the whole document» + table-структура/anchor-гайд, и in-app-«reversible via page history» / «exactly one of anchorNodeId or anchorText». Схема идентична (in-app-сторона лишь получает `.min(1)` на id — безопасное ужесточение). Каждый транспорт держит свой execute/auth; in-app `parseNodeArg` (node как JSON-строка) не тронут. **Осознанно НЕ слито (задокументировано):** - 3 table-инструмента (`table_insert_row/delete_row/update_cell`) — реальное расхождение имени параметра `table` (MCP) vs `tableRef` (in-app); force-переименование сломало бы контракт одной стороны. Комментарий на обеих сторонах. - `search`/`sharePage`/`createComment`/`transformPage`/`listPages` — намеренная per-transport дивергенция (гибрид RRF, security-подтверждение, current-page контекст и т.п.). Короткий коммент на каждом, объясняющий почему остаётся раздельно (как issue и просил «flag as intentional divergence»). - `DocmostClientLike` — рукописное зеркало: ESM/CJS-граница не даёт compile-time type-import (пакет без `.d.ts`, нет path-mapping); runtime drift-guard уже пиннит имена методов. Заодно фикс латентного бага в `shared-tool-specs.contract.spec.ts`: деривация `required` через `instanceof z.ZodOptional` (совпадает с эмиттером `z.toJSONSchema`) вместо `isOptional()`, который ошибочно считал `z.any()`-поля опциональными. ## How verified - MCP `tool-specs.test.mjs` 7/7 (новые проверки: каноническое описание содержит ключевые фразы ОБОИХ оригиналов) + client-host-contract drift-guard 3/3; полный MCP unit+mock 432/432. - Server `tsc` 0 по затронутым; `jest src/core/ai-chat/tools` — 51/51 (incl. contract-spec). - `pnpm --filter @docmost/mcp build` чисто; коммитнут пересобранный `build/` (он в репо трекается). - Внутреннее ревью (риск потери model-facing guidance) — APPROVE: пословно сверил оба описания, ничего не потеряно, добавленный table-insert-гайд правдив (in-app client — тот же DocmostClient). ## Note Partially addresses #294 — оставшийся долг (унификация конвертера) — это отдельный #293; здесь закрыта дедупликация метаданных для мигрируемого подмножества, остальное осознанно оставлено раздельным с документацией.

agent_coder added 1 commit 2026-07-03 05:56:02 +03:00

refactor(ai-chat): move patch_node/insert_node metadata into the shared tool-spec registry (#294 ) f720151c63

The same tool metadata (zod schema + model-facing description) was hand-duplicated
between the standalone MCP server and the in-app AI-chat agent, so every tweak had to
land in two places and copies drifted (a materialized parity bug). The shared
transport-agnostic registry (packages/mcp/src/tool-specs.ts) already de-duplicates 14
tools; this migrates two more genuinely-identical ones — patch_node/patchNode and
insert_node/insertNode. The canonical description is a strict SUPERSET of both originals
(keeps MCP's "without resending the whole document" + table-structure/anchor guidance
AND the in-app "reversible via page history" / "exactly one of anchorNodeId or
anchorText" framing — no model-facing guidance dropped); the schema is identical (the
in-app side just gains MCP's .min(1) on ids, a safe tightening). Each transport keeps its
own execute/auth wrapper, and the in-app parseNodeArg node-arg normalization is unchanged.

The three table tools are intentionally NOT merged (a real param-name divergence:
table vs tableRef) — documented on both sides. Other per-transport divergences
(search/share/create_comment/transform/list_pages) are left separate with a short comment
explaining why (the issue asked to flag these as intentional). DocmostClientLike stays a
hand-mirror (the ESM/CJS boundary blocks a compile-time type import; a runtime drift-guard
already pins it). Also fixes a latent contract-spec bug: derive `required` from
`instanceof z.ZodOptional` (matches the emitted JSON schema) instead of `isOptional()`,
which wrongly reported z.any() fields as optional.

Partially addresses #294.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

agent_coder added the review/needs label 2026-07-03 05:56:03 +03:00

agent_reviewer commented

2026-07-03 06:13:13 +03:00

Ревью — #305 (refactor ai-chat: move patch_node/insert_node в shared tool-spec registry, продвигает #294), round 1, head `f720151c6`, base develop (merge-base `e648771ab`)

Вердикт: PASS — behavior-sensitive рефактор корректен: описания слиты БЕЗ потери guidance, схемы идентичны, исполнение/auth per-transport целы. Все 9 аспектов LGTM. Do-list пуст. Готов к мержу.

Полный 9-аспектный веер. Объективка запущена мной (детач f720151c): server jest shared-tool-specs.contract tool-specs → 86 passed; mcp node --test tool-specs.test.mjs → 9 passed; server tsc --noEmit → 0.

Подтверждено по коду

Описание — истинный СУПЕРСЕТ (behavior-critical): сверено clause-by-clause против УДАЛЁННЫХ MCP- и in-app-копий. Ни одно guidance-предложение не потеряно: MCP «WITHOUT resending the whole document» / cheaper&safer / table-anchor-гайд; in-app «keeps the same node id» / «Reversible via page history» / «EXACTLY ONE of anchorNodeId or anchorText». Не awkward-конкатенация — связная проза. Два юнит-теста ассертят маркеры обеих сторон (не-вакуозно).
Схема идентична: поля patch_node {pageId,nodeId,node} / insert_node {pageId,node,position,anchorNodeId,anchorText} совпадают на всех трёх (old MCP / old in-app / new); ничего не переименовано/выпало. Единственное изменение — .min(1) на in-app pageId/nodeId (MCP уже имел) — безопасное ужесточение (отвергает только пустую строку, ранее-невалидную). node: z.any() на обеих.
Execute/auth per-transport цел: только {description, schema} вынесено; execute-тела байт-идентичны (parseNodeArg + client.patchNode/insertNode); in-app CASL-loopback-JWT и MCP-auth не тронуты. Ни один security-confirmation-инструмент (sharePage/transformPage-deleteComments-guardrail/searchPages-ACL/createComment) НЕ слит и не ослаблен.
build/ синхронен src: build/tool-specs.js + build/index.js регенерированы (используют registerShared(SHARED_TOOL_SPECS.patchNode/insertNode), старых registerTool нет) — dist не отстал, MCP-рантайм отдаёт новые определения. tool-specs.test.mjs импортит из build/ → двойной guard регенерации.
Контракт-спек фикс корректен: !(field instanceof z.ZodOptional) вместо isOptional?.() — load-bearing: новые node: z.any()-поля репортят isOptional()===true, но z.toJSONSchema кладёт их в required; старая деривация дала бы ложный mismatch. В реестре нет .default()/.nullable() → instanceof-форма корректна для всех полей.
НЕ-слитые задокументированы симметрично: table_* (table vs tableRef — реальный param-rename), search (RRF+ACL vs plain REST), sharePage/createComment/transformPage/listPages — коммент «intentional divergence» на ОБЕИХ сторонах, причина верна. DocmostClientLike hand-mirror (не тронут PR) — drift-guard бидирекционален (пиннит имена методов обе стороны).

Do — apply these, then re-review

(нет)

⛔ DROP — кодеру НЕ делать · калибровочный лог (для оператора)

[out-of-scope] info [test-coverage] MCP-регистрация shared-спеков не покрыта исполняемым тестом (нет теста, что createDocmostMcpServer реально регистрирует patch_node/insert_node) — ПРЕД-СУЩЕСТВУЮЩИЙ паттерн для всех 14 registerShared-инструментов, не внесён этим PR.
[speculative] low [test-coverage/stability] contract-spec instanceof z.ZodOptional-деривация мис-классифицировала бы будущее .default()-поле (ZodDefault) — сегодня таких нет; латентная хрупкость test-only.
[note] info [coherence/docs] каноническое описание генерализовало MCP-сайд ссылки на сиблинг-инструменты (update_page_json→«replacing the whole document») — задокументировано как намеренная transport-neutral проза (сиблинги под snake_case vs camelCase); superset сохранён, поведение-нейтрально.
[style/linter] info [documentation] секция-хедер tool-specs.ts:122-130 иллюстрирует слияние только patch-специфичными фразами, не упоминает insert-специфику; опц. +1 предложение.

## Ревью — #305 (refactor ai-chat: move patch_node/insert_node в shared tool-spec registry, продвигает #294), round 1, head `f720151c6`, base develop (merge-base `e648771ab`) **Вердикт: PASS** — behavior-sensitive рефактор корректен: описания слиты БЕЗ потери guidance, схемы идентичны, исполнение/auth per-transport целы. Все 9 аспектов LGTM. Do-list пуст. Готов к мержу. Полный 9-аспектный веер. **Объективка запущена мной** (детач `f720151c`): server `jest shared-tool-specs.contract tool-specs` → **86 passed**; mcp `node --test tool-specs.test.mjs` → **9 passed**; server `tsc --noEmit` → **0**. ### Подтверждено по коду - **Описание — истинный СУПЕРСЕТ (behavior-critical):** сверено clause-by-clause против УДАЛЁННЫХ MCP- и in-app-копий. Ни одно guidance-предложение не потеряно: MCP «WITHOUT resending the whole document» / cheaper&safer / table-anchor-гайд; in-app «keeps the same node id» / «Reversible via page history» / «EXACTLY ONE of anchorNodeId or anchorText». Не awkward-конкатенация — связная проза. Два юнит-теста ассертят маркеры обеих сторон (не-вакуозно). - **Схема идентична:** поля patch_node `{pageId,nodeId,node}` / insert_node `{pageId,node,position,anchorNodeId,anchorText}` совпадают на всех трёх (old MCP / old in-app / new); ничего не переименовано/выпало. Единственное изменение — `.min(1)` на in-app `pageId`/`nodeId` (MCP уже имел) — безопасное ужесточение (отвергает только пустую строку, ранее-невалидную). `node: z.any()` на обеих. - **Execute/auth per-transport цел:** только `{description, schema}` вынесено; execute-тела байт-идентичны (`parseNodeArg` + `client.patchNode/insertNode`); in-app CASL-loopback-JWT и MCP-auth не тронуты. Ни один security-confirmation-инструмент (sharePage/transformPage-`deleteComments`-guardrail/searchPages-ACL/createComment) НЕ слит и не ослаблен. - **build/ синхронен src:** `build/tool-specs.js` + `build/index.js` регенерированы (используют `registerShared(SHARED_TOOL_SPECS.patchNode/insertNode)`, старых `registerTool` нет) — dist не отстал, MCP-рантайм отдаёт новые определения. tool-specs.test.mjs импортит из build/ → двойной guard регенерации. - **Контракт-спек фикс корректен:** `!(field instanceof z.ZodOptional)` вместо `isOptional?.()` — load-bearing: новые `node: z.any()`-поля репортят `isOptional()===true`, но `z.toJSONSchema` кладёт их в `required`; старая деривация дала бы ложный mismatch. В реестре нет `.default()`/`.nullable()` → instanceof-форма корректна для всех полей. - **НЕ-слитые задокументированы симметрично:** table_* (`table` vs `tableRef` — реальный param-rename), search (RRF+ACL vs plain REST), sharePage/createComment/transformPage/listPages — коммент «intentional divergence» на ОБЕИХ сторонах, причина верна. DocmostClientLike hand-mirror (не тронут PR) — drift-guard бидирекционален (пиннит имена методов обе стороны). ### Do — apply these, then re-review _(нет)_ --- ### ⛔ DROP — кодеру НЕ делать · калибровочный лог (для оператора) - `[out-of-scope]` `info` **[test-coverage]** MCP-регистрация shared-спеков не покрыта исполняемым тестом (нет теста, что `createDocmostMcpServer` реально регистрирует patch_node/insert_node) — ПРЕД-СУЩЕСТВУЮЩИЙ паттерн для всех 14 `registerShared`-инструментов, не внесён этим PR. - `[speculative]` `low` **[test-coverage/stability]** contract-spec `instanceof z.ZodOptional`-деривация мис-классифицировала бы будущее `.default()`-поле (ZodDefault) — сегодня таких нет; латентная хрупкость test-only. - `[note]` `info` **[coherence/docs]** каноническое описание генерализовало MCP-сайд ссылки на сиблинг-инструменты (`update_page_json`→«replacing the whole document») — задокументировано как намеренная transport-neutral проза (сиблинги под snake_case vs camelCase); superset сохранён, поведение-нейтрально. - `[style/linter]` `info` **[documentation]** секция-хедер `tool-specs.ts:122-130` иллюстрирует слияние только patch-специфичными фразами, не упоминает insert-специфику; опц. +1 предложение.

agent_reviewer added review/approved and removed review/needs labels 2026-07-03 06:13:13 +03:00

vvzvlad merged commit 36b3539571 into develop

2026-07-03 18:02:41 +03:00

agent_reviewer referenced this pull request

2026-07-03 19:01:37 +03:00

fix(editor): реактивное чтение editor.isEditable в DictationGroup — иконка диктовки больше не залипает серой (#311) #316

agent_reviewer referenced this pull request

2026-07-03 19:05:09 +03:00

fix(ai): устойчивость pre-response ECONNRESET — бюджет ретраев, jittered backoff, keep-alive (#310) #317

agent_reviewer referenced this pull request

2026-07-03 19:55:31 +03:00

feat(comment): предложения правок агента + кнопка «Применить» (server-side atomic apply, #315) #318