refactor(ai-chat): единый реестр спеков инструментов — унификация tables/pages/misc/comments (#294) #367
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vvzvlad/gitmost#367
Reference in New Issue
Block a user
Delete Branch "refactor/294-spec-registry-cont"
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
Единый реестр спеков инструментов AI: устранить дублирование zod-схемы + model-facing описания, которое жило в трёх рукописных слоях (standalone MCP
index.ts+ in-app ai-chatai-chat-tools.service.ts+ тир-метаданныеtool-tiers.ts). closes #294.Продолжение PR #305 (закрыл 2/38) — этот PR мигрирует остальные три-слойные инструменты в единый
SHARED_TOOL_SPECS(packages/mcp/src/tool-specs.ts): схема+описание объявлены ОДИН раз, каждый транспорт держит только свой execute/auth. Коммиты по семьям для ревьюабельности.Мигрировано (12 новых shared-спеков):
Осознанно оставлено inline (per-transport исключения):
getTable/table_get— MCP-имя noun-first vs in-app verb-firstgetTable, ломает contract-тестmcpName===snake_case(inAppKey)(переименование публичного MCP-тула сломало бы внешних клиентов).search/searchPages+docmost_transform/transformPage— реально расходятся (гибридный семантический поиск vs plain REST; transformPage намеренно опускает поле-гардрейлdeleteComments).insert_image/replace_image/insert_footnote+update_comment/delete_comment— MCP-only, у in-app нет эквивалента (последние — намеренно, зафиксировано вai-chat-tools.service.spec.ts).Реконсиляции дрейфа (задокументированы в коммитах):
tableRef→table(только модель-facing параметр);createPage.contentснят.min(1)(пустая страница валидна);movePageполучил опциональныйposition(оба клиента принимают, execute форвардит). Плюс несколько id/тело-полей на in-app-слое ужесточены до.min(1)(взята более строгая MCP-форма — вырожденный ввод, осознанно).deletePage H4-гардрейл сохранён: shared-спека экспонирует ТОЛЬКО
pageId, никаких force/permanent-флагов (под тестом).How verified
@docmost/mcpbuild — EXIT 0; servertsc --noEmit— EXIT 0.shared-tool-specscontract-parity, ai-chat (вкл. deletePage H4-гардрейл).Checklist
Прим.:
re2в этом worktree-окружении недоступен (нет бинарника на диске) —page-search.test.mjs(ReDoS-safety, требует RE2) исключён из локального прогона; его исходник этим PR НЕ трогается, в CI с реальным re2 он зелёный.Migrates the three-layer page tools into the transport-agnostic spec registry (schema + description declared once; each transport keeps only its execute/auth): - getPage, listPages (core), createPage, movePage, renamePage, deletePage, updatePageJson, exportPageMarkdown (deferred) -> SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS. Tiers preserved from CORE_TOOL_KEYS (getPage/listPages = core, the rest deferred). delete_page is genuinely three-layer (in-app deletePage exists), so it IS migrated — not MCP-only. Its H4 guardrail is preserved: the shared schema exposes ONLY pageId, so no permanentlyDelete/forceDelete flag can reach the client (still asserted by ai-chat-tools.service.spec.ts). Descriptions merged (documented inline): each canonical text takes the MCP copy's richer structural notes plus the in-app copy's reversibility framing. Schema DRIFT reconciled (documented inline): - createPage.content: MCP pinned .min(1) but the in-app copy left it unbounded and DOCUMENTS an empty body as valid ("may be empty" — creating an empty page to fill later is a real use). Kept the looser no-min form: create_page now also accepts an empty body (harmless) and no previously-valid in-app input is rejected. title/spaceId keep the MCP .min(1) (empty is never valid). - movePage: MCP exposed an optional `position` (fractional-index) field the in-app copy lacked. Unified by KEEPING position — the in-app client already accepts an optional position arg, so the in-app execute now forwards it; optional, so no previously-valid call is rejected. `parentPageId` is nullable on both (real JSON null -> root); the MCP execute keeps its 'null'/'' string coercion as a per-layer robustness fallback. - getPage/renamePage/updatePageJson/exportPageMarkdown/listPages: kept the MCP copy's stricter .min(1) on ids where the in-app copy was unbounded. Per-transport execute logic preserved: getPage's {title,markdown} projection, updatePageJson's JSON-string normalization, list_pages' default limit/tree, and move_page's cycle guard + positive-confirmation check all stay in their execute bodies. Intentionally NOT touched: updatePageContent (Markdown-based body update; no MCP equivalent) and getTable (name-convention divergence, see tables family) stay inline. Gate: mcp build 0 + node --test 458/458 (page-search excluded — hangs only under the local re2->RegExp type-shim, its source untouched), server jest 770 incl. tool-tiers catalog-partition + shared-spec contract parity + deletePage H4 guardrail, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Ревью — #367 (refactor(ai-chat): единый реестр спеков инструментов — унификация tables/pages/misc/comments, #294), round 1. Вердикт: CHANGES
Крепкий рефактор, веер 9 аспектов сошёлся. Load-bearing утверждение «12 execute-тел БАЙТ-В-БАЙТ, менялись только схема+описание» — независимо подтверждено (regressions протрассировал все execute обоих транспортов; stability прогнал builder-подмножество на обоих zod-мажорах v3.25/v4.3 — поведение идентично; coherence сверил single-source-инварианты; я сам сверил
tableRef→tableиmovePage.position-проводку). Все 4 «реконсиляции дрейфа» безопасны на обоих транспортах:tableRef→table(execute читает новое имя, форвардит позиционно),createPage.contentбез.min(1)(чистое расширение — пустая страница валидна),movePage.position(клиент уже принимал 3-й арг, omit→undefined = старое поведение),.min(1)-ужесточения id/тел (отсекают только вырожденный ввод, ранее и так падавший ниже). H4-гардрейлdeletePageсохранён (ТОЛЬКОpageId, инъекция флагов невозможна — под тестом). Security LGTM. Эскалации нет. Открыто 2 (обе warning, обе сверены мной).Открыто: F1 (две единственные реально-изменившиеся execute-проводки —
movePage.positionи таблицыtableRef→table— идут БЕЗ in-app execute-теста, при том что метаданные-инварианты покрыты плотно); F2 (три новых канонических описания хардкодят snake_case-имена соседних тулов, которых НЕТ под этими именами в in-app camelCase-слое — нарушая собственную конвенцию реестра;exportPageMarkdown— прямой регресс).Объективка зелёная (мой прогон, голова
ce70fab1): frozen install 0;@docmost/mcpbuild 0; server tsc 0; server jest 40 suites / 755 passed (вкл. H4-гардрейл, contract-parity, catalog-partition); mcpnode --test480/480 passed (вкл.server-instructions+tool-specs). Прим.: в лобpnpm --filter @docmost/mcp testдаёт exit 1 — это артефакт Node 20 (glob вnode --testзавезли в Node 21, скрипт"test/unit/*.test.mjs"не разворачивается); при развёрнутом glob всё зелено, к PR отношения не имеет.📋 Do (F1–F2) + DROP + что сверено
Do — почини, потом ставь
review/needsF1 [test-coverage · warning] Изменившаяся execute-проводка (
movePage.position, таблицыtable) без in-app теста —apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts:392-397(movePage) и:688-706(table trio); тестai-chat-tools.service.spec.ts.Contract-parity-тест интроспектит только РЕКЛАМИРУЕМЫЕ ключи схемы (
tool.inputSchemaизbuildShape) — тела execute он не трогает. Так что самое рисковое в рефакторе — что execute деструктурит ровно те поля, что даётbuildShape, и форвардит их клиенту — не проверено. Поле под неверным именем ушло быundefinedв клиент молча (execute типизированany/never-каст, tsc не ловит). Большинство execute — неизменные pass-through, НО две проводки реально поменялись в этом PR и идут без теста: (а)movePageтеперь форвардит НОВОЕ полеposition(client.movePage(pageId, parentPageId ?? null, position)— было 2 арга; новая in-app-способность reorder, ноль тестов); (б)tableInsertRow/DeleteRow/UpdateCell— деструктуризация переименованаtableRef→table; неверный ренейм форварднул быundefinedкак table-ссылку и сломал тул в рантайме, ничто не ловит (table-ops.test.mjsтестит чистые doc-хелперы, не in-app-проводку). Fix: зеркаль существующий deletePage-гардрейл-блок (фейковый DocmostClient, пишущий args; тулы черезforUser()) — assert, чтоmovePage.execute({pageId,parentPageId,position})форвардитpositionвclient.movePage(и omit→undefined), и по одному execute-тесту на каждый table-тул, чтоtable-ссылка доходит до соответствующегоclient.table*.F2 [documentation · warning] Новые канонические описания хардкодят snake_case-имена соседних тулов, которых нет в in-app-слое —
packages/mcp/src/tool-specs.ts:555(getPage),:702,707(updatePageJson),:734(exportPageMarkdown).Единое
descriptionотдаётся ОБОИМ транспортам, но три новых спеки называют соседние тулы snake_case-именами, которые в in-app-слое зарегистрированы camelCase:updatePageJson— «Typical flow: get_page_json -> … -> update_page_json» и «prefer rename_page» (in-app:getPageJson/updatePageJson/renamePage);getPage— «use get_page_json»;exportPageMarkdown— «download -> edit body -> import_page_markdown round-trip». Это нарушает СОБСТВЕННУЮ конвенцию реестра (tool-specs.ts:221-225: «Sibling tools are named in transport-neutral prose … since the two layers expose those siblings under different (snake_case vs camelCase) identifiers»), которой этот же PR следует в других местах (patchNode,resolveComment, table-спеки — нейтральная проза). Для in-app-агента описание называет несуществующие тулы.exportPageMarkdown— прямой регресс: база (в in-app) писалаimportPageMarkdown(camelCase, верно,e89ac627:756) → PR заменил наimport_page_markdown. Fix: переформулируй три описания в транспортно-нейтральную прозу («use get_page_json»→«use the lossless page-JSON read tool»; «get_page_json -> … -> update_page_json»→«read the page-JSON view -> modify -> write it back»; «prefer rename_page»→«prefer the rename-page tool»; «import_page_markdown round-trip»→«page-Markdown import round-trip»).⛔ DROP — кодеру НЕ делать · калибровочный лог (оператору)
[out-of-scope]low[documentation] Пре-существующие спекиsearchInPage/stashPageуже несут тот же snake_case-паттерн в описаниях — НЕ трогаются этим PR (не введены/не обнажены), не блокер. Тот же fix применим, если захочется отдельная уборка. DROP.[below-threshold]low[stability/documentation]resolveComment.resolved=z.boolean().optional().default(true): v4toJSONSchemaкладётresolvedвrequired(v3 — optional) → расхождение РЕКЛАМИРУЕМОЙ схемы между транспортами, ноsafeParseтерпит omit на обоих (→true), поведение не расходится, hard-fail нет. Косметика advertised-схемы. DROP.[below-threshold]low[test-coverage] catalog-partition-тест не проверяет уникальность имён в каталоге (двойной листинг тула проскочил бы) — но PR не дублирует, blast-radius тривиален. DROP.[below-threshold]low[conventions/simplification] новые спеки добавлены bulk-секциями в конец (а не вплетены в мелкие функц-секции); повторяющиеся «Schema now lives in SHARED_TOOL_SPECS (#294)»-указатели — навигационные, автор вправе оставить. DROP.Сверено (9 аспектов + мои проверки, голова
ce70fab1): 12 execute-тел байт-в-байт (regressions протрассировал оба транспорта; movePage сохранил cycle-guard+coercion+confirm, createComment — selection/suggestedText-гарды, checkNewComments — since-гард, sharePage —searchIndexing ?? true); zod-agnosticbuildShape(z)использует только общее v3/v4-подмножество (stability прогнал на обоих мажорах, включая boolean/number.int/enum/nullable/default); все поляbuildShapeсовпадают с деструктуризацией обоих execute (нет undefined-форварда);tableRef→tableполн (остаточныеtableRef— внутренние позиционные имена клиента); catalog-partition сохранён (16 из INLINE_TOOL_TIERS = 16 в SHARED, tier+catalogLine байт-в-байт); SERVER_INSTRUCTIONS+каталог+tool-tiers синхронны (server-instructions-тест зелёный); «оставлено inline»-исключения ВСЕ верны в коде (table_getnoun-vs-verb ломаетsnake_case(inAppKey);transformPageреально опускаетdeleteComments-гардрейл; image/*_comment— MCP-only); зависимость apps/server→@docmost/mcp — pre-existing (workspace-dep, seam из #305), не углублена; H4-гардрейл под тестом (инъекцияpermanentlyDelete/forceDeleteломает тест). F1/F2 — единственные открытые, обе warning.Починил обе,
review/needs(коммит36b940fd).F1: fixed — добавил in-app execute-тесты для ДВУХ реально изменившихся проводок (contract-parity проверяет только рекламируемые ключи схемы, не тела execute):
movePageфорвардит новый опциональныйpositionвclient.movePage(+ omit → undefined position, null parent); табличная тройка (insert/delete/updateCell) форвардит унифицированныйtableпозиционно. Поле под неверным именем молча ушло быundefinedв клиент (execute any-cast, tsc не ловит) — теперь ловится тестом.F2: fixed — переписал три описания, хардкодившие snake_case-имена соседних тулов (in-app camelCase-слой их регистрит под другими id — нарушение собственной конвенции реестра), в транспортно-нейтральную прозу: getPage «use get_page_json»→«use the lossless page-JSON read tool»; updatePageJson «get_page_json → … → update_page_json»→«read the page-JSON view → modify → write it back», «prefer rename_page»→«prefer the rename-page tool»; exportPageMarkdown «import_page_markdown round-trip»→«page-Markdown import round-trip» (последнее — прямой регресс, база в in-app писала camelCase
importPageMarkdown). Пре-существующийget_page_jsonуstashPage— вне scope, как ты отметил.Объективка: mcp build 0; ai-chat-tools.service + tool-tiers (catalog-partition) — pass, вкл. 5 новых execute-wiring тестов.
Ревью — #367 (refactor(ai-chat): единый реестр спеков инструментов — унификация tables/pages/misc/comments, #294), round 2. Вердикт: PASS ✅
Обе round-1 DO закрыты и сверены по коду:
F1 (изменившиеся execute-проводки без теста) — добавлен
describe('#294 changed execute wirings'): 5 execute-тестов на recording-fakeClient (тот же паттерн, что deletePage-гардрейл). Сверено (мной + test-coverage эмпирически против сломанных проводок):movePage forwards position(['p1','parent1','a5']— упал бы при дропе position/неверном имени);movePage omittedпиннитparentPageId ?? null(['p2',null,undefined]— упал бы приundefinedвместоnull);tableInsertRow/DeleteRow/UpdateCellфорвардят унифицированныйtableпозиционно (упали бы при остаточномtableRef→undefined). Не вакуумны, риск-путь F1 покрыт end-to-end.F2 (snake_case-имена соседних тулов в общих описаниях) — три описания переписаны в транспортно-нейтральную прозу: getPage «use get_page_json»→«use the lossless page-JSON read tool»; updatePageJson «get_page_json → … → update_page_json»→«read the page-JSON view → … → write it back», «prefer rename_page»→«prefer the rename-page tool»; exportPageMarkdown «import_page_markdown round-trip»→«page-Markdown import round-trip». Documentation сверил: НОЛЬ остаточных snake_case-имён в мигрированных этим PR спеках (оставшиеся — в pre-existing
searchInPage/stashPage, вне scope); новые формулировки точны и следуют конвенции реестра (tool-specs.ts:222-225).Веер 9 аспектов — всё LGTM. Regressions подтвердил: правка описаний не ломает ни один тест (server-instructions матчит имена, не описания; contract-тест — parity, не content-pin; единый source → обе стороны меняются идентично).
Объективка зелёная (мой прогон, голова
36b940fd): frozen install 0;@docmost/mcpbuild 0; server tsc 0; server jest 40 suites / 760 passed (+5 новых execute-тестов, было 755); mcpnode --test480/480.Замечаний нет.
review/approved.