ccee32cb0b1a0feb697b94a74eeb284ad5b491fa
68 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
36b940fdb8 |
fix(#294 review F1-F2): test the changed execute wirings + transport-neutral descriptions
- F1: added in-app execute tests for the two wirings that ACTUALLY changed in the migration (the contract-parity test only checks advertised schema keys, not execute bodies): movePage forwards the newly-added optional `position` to client.movePage (and passes undefined position + null parent when omitted); the table trio (insert/delete/updateCell) forwards the unified `table` param positionally. A field destructured under the wrong name would have silently passed undefined to the client (execute is any-cast, tsc won't catch it). - F2: rewrote the three migrated descriptions that hardcoded snake_case sibling tool names (which the in-app camelCase layer exposes under different ids, violating the registry's own transport-neutral-prose convention) into neutral prose: 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" (the last was a direct regress — the in-app base said the camelCase importPageMarkdown). (stashPage's pre-existing get_page_json mention is out of scope, per the reviewer.) Gate: mcp build 0; ai-chat-tools.service + tool-tiers (catalog-partition) pass, incl. the 5 new execute-wiring tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
ce70fab1df |
refactor(ai-chat): unify share_page into SHARED_TOOL_SPECS (#294, misc family)
Migrates share_page / sharePage into the transport-agnostic spec registry (schema + description declared once; each transport keeps only its execute/auth): - sharePage (deferred) -> SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS. Drift reconciled (documented inline): both inline copies already carried the "only share when the user explicitly asked" security framing, so the old "per-transport divergence" note in BOTH layers was STALE — there was no real behavioral divergence, only wording drift. The canonical description merges the MCP copy's URL-format + idempotency detail with the in-app copy's reversibility note and keeps the shared security framing. pageId keeps the MCP copy's stricter .min(1). The MCP execute keeps its own `searchIndexing ?? true` default (per-layer, not part of the shared schema). Intentionally NOT migrated (kept inline — genuinely divergent, as their existing notes state): - search / searchPages: the in-app tool is a semantic+keyword hybrid (RRF) with in-process access control and a tuned schema (limit 1-20); the MCP `search` is a plain REST full-text search (limit up to 100). Different behavior AND schema. - docmost_transform / transformPage: the in-app tool deliberately omits the `deleteComments` schema field (a comment-deletion guardrail) and carries a shorter description. Different schema. 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 775 incl. tool-tiers catalog-partition + shared-spec contract parity, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
b51dae16a6 |
docs(mcp): mark media tools MCP-only in index.ts (#294, media family)
The media tools — insert_image, replace_image, insert_footnote — are MCP-only by design: the in-app AI-chat agent exposes no image or footnote tools, so there is no second layer to unify into SHARED_TOOL_SPECS. A registry spec's tier/catalogLine are in-app metadata and the catalog-partition test forbids a spec without a live in-app tool, so forcing them into the registry would break the invariant. They stay per-transport (inline in index.ts). No behavior change — documentation only (adds the rationale above each tool so a future migrator does not re-investigate why these are not shared). Gate: mcp tsc 0 (comment-only change). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
39735afd73 |
refactor(ai-chat): unify page tools into SHARED_TOOL_SPECS (#294, pages family)
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>
|
||
|
|
eebbe6717c |
refactor(ai-chat): unify table row/cell tools into SHARED_TOOL_SPECS (#294, tables family)
Migrates the three-layer table WRITE tools into the transport-agnostic spec registry (schema + description declared once; each transport keeps only its execute/auth): - tableInsertRow, tableDeleteRow, tableUpdateCell -> SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS (all three are deferred; not in CORE_TOOL_KEYS). Drift reconciled (documented inline): the four table tools previously carried a "NOT shared" note in both layers over a single parameter-NAME drift — the MCP layer named the table reference `table`, the in-app layer `tableRef`. Unified on the MCP name `table` (renaming the public MCP parameter would break external MCP clients; the in-app parameter is model-facing/prompt-only and safe to rename). The in-app execute bodies now destructure `table`. Descriptions took the MCP copy's richer wording (documents `#<index>`, padding, header-row behavior) plus the in-app copy's "Reversible via page history" note; both fields keep the MCP copy's stricter .min(1) (in-app left them unbounded); sibling tool references phrased transport-neutrally. Intentionally NOT migrated (kept inline): table_get / getTable. Its MCP tool name is noun-first (`table_get`) while the in-app key is verb-first (`getTable`), which breaks the snake_case(inAppKey) naming convention the registry enforces (shared-tool-specs.contract.spec.ts). Renaming the public MCP tool would break external clients, so it stays per-transport — but its in-app reference param was still aligned to `table` (was `tableRef`) for consistency with the migrated trio. Gate: mcp tsc 0 + node --test 458/458 (page-search excluded — hangs only under the local re2->RegExp type-shim, its source is untouched), server jest 730 incl. tool-tiers catalog-partition + shared-spec contract parity, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
e348433a39 |
refactor(ai-chat): unify comment tools into SHARED_TOOL_SPECS (#294, comments family)
Migrates the three-layer comment tools into the single transport-agnostic spec registry (schema + model-facing description declared once; each transport keeps only its execute/auth): - createComment, listComments, resolveComment, checkNewComments — moved to SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS (tier/catalogLine now on the spec). Tiers preserved from CORE_TOOL_KEYS (create/list/resolve = core, check = deferred). Intentionally NOT migrated (kept MCP-inline): update_comment / delete_comment — they are MCP-only by design; the in-app AI-chat layer deliberately has no updateComment/deleteComment (comment edits are irreversible / not version-tracked), asserted by ai-chat-tools.service.spec.ts. A registry spec's tier/catalogLine are in-app metadata and the catalog-partition test forbids a deferred spec without a live in-app tool, so these stay per-transport. Drift reconciled (documented inline): createComment/listComments/checkNewComments took the more-maintained/superset description + stricter .min(1) guards. resolveComment: `resolved` drifted (MCP optional+default(true) vs in-app required) — kept the MCP superset, so in-app resolveComment now accepts an omitted `resolved` (defaults to resolve) — a deliberate, backward-compatible unification (never rejects a previously-valid input). Gate: mcp build 0 + node --test 480/480, ai-chat 654, tool-tiers (incl. F3 catalog-partition) 16/16, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
e431b33bb1 |
feat(ai-chat): deferred tool loading (tiers + loadTools meta-tool) (#332)
The in-app AI agent shipped all ~41 tool schemas on every model step. This adds a two-tier catalog: core tools (frequent or one-line) stay always-active; the rest are advertised as a compact catalog and their full schema is fetched on demand via the loadTools meta-tool, wired through ai@6 prepareStep's per-step activeTools. - tools/tool-tiers.ts: CORE_TOOL_KEYS, INLINE_TOOL_TIERS, applyLoadTools, catalog builders (+ tool-tiers.spec.ts, 13 cases). - ai-chat.service.ts prepareAgentStep: returns activeTools = [...CORE_TOOL_KEYS, loadTools, ...activatedTools]; per-turn activated Set. - ai-chat.prompt.ts: buildToolCatalogBlock renders the deferred catalog. - mcp/tool-specs.ts: tier + catalogLine metadata (external snake_case /mcp transport unchanged). - EnvironmentService.isAiChatDeferredToolsEnabled(): AI_CHAT_DEFERRED_TOOLS, default ON per issue intent (kill-switch =false restores old behavior). Gate: server ai-chat 631/631, tool-tiers 13/13, mcp 472/472, tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
eacc1c4811 |
Merge branch 'develop' of https://gitea.vvzvlad.xyz/vvzvlad/gitmost into feat/293-B-prosemirror-markdown-pkg
# Conflicts: # packages/mcp/build/client.js # packages/mcp/build/index.js # packages/mcp/build/tool-specs.js |
||
|
|
086bc1bf8b |
docs(mcp): search_in_page regex desc names RE2, not JS regex (#330 review F5)
The RE2 swap narrowed the contract: regex:true rejects lookaround ((?=…)/(?<=…)) and backreferences (\1). The internal JSDoc was updated, but the AGENT-VISIBLE tool-spec (the only text the agent reads at call time, single-sourced to both transports) still said 'a JS regular expression' — so an agent would write a lookahead/backref and hit an error. Updated the .description and the regex flag .describe() to name RE2 (linear-time, ReDoS-safe), list that char classes / word boundaries / anchors / quantifiers work while lookaround and backreferences do NOT, and keep the 'invalid/unsupported regex -> clear error' note. mcp: tsc clean; tool-specs / server-instructions / contract tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
77b245461f |
fix(mcp): search_in_page regex via re2 (ReDoS-safe) + review DO F1-F4 (#330 review)
Maintainer decision on the escalated ReDoS fork: use re2. The regex path compiled agent-supplied patterns with `new RegExp` and ran them synchronously in the shared event-loop; a catastrophic-backtracking pattern (e.g. `(a+)+$`) hung the whole Node backend for all users (the tool is in both transports incl. the in-app apps/server agent), and size caps do NOT bound backtracking. Switch the regex engine to re2 (Google RE2, linear-time, no backtracking): - `new RE2(query, caseSensitive?'g':'gi')`. RE2 extends RegExp, so eachMatch and the zero-length-match lastIndex guard are unchanged. - Unsupported patterns are now a CLEAN error, not a hang: RE2 throws on invalid syntax AND on the backtracking-only features it can't do (lookaround (?=…)/(?<=…), backreferences \1) — caught at compile and returned as a clear tool error telling the agent to rewrite without them. - Removed MAX_CONTAINER_TEXT + the per-container slice (re2 is linear, so it's no longer a ReDoS defense, and truncating risked silently dropping real matches in a long container); kept MAX_PATTERN_LENGTH as a cheap query sanity cap. - Verified: `(a+)+$` over 50k `a` completes in ~4ms; lookaround/backref throw. - Added re2 (^1.21.0) to packages/mcp; lockfile updated. Reviewer DO items: - F1 [doc]: removed the false "pass nodeId as a comment anchor" claim (create_comment has no nodeId param — it needs a text `selection`). Fixed in tool-specs.ts + page-search.ts (module + SearchMatch JSDoc) + client.ts; the ref is for get_node/patch_node, and for a comment you build a unique text selection from before+match+after. - F2 [doc]: clarified `#<index>` refs (id-less table/cell) are accepted by get_node but NOT patch_node (id-only). - F3 [test]: round-trip — each match's nodeId fed to the real getNodeByRef (attrs.id node + `#<index>` table-cell) to prove the ref format is consumable. - F4 [test]: before/after edge-pinning (match in first 40 chars of a long container; index 0 → before==""; container end → after==""). - New re2 tests: catastrophic patterns complete fast; lookaround/backref → error. mcp: tsc clean; node --test 472 passed (+5). apps/server: tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
40d42d61e6 |
feat(mcp): search_in_page tool — in-page substring/regex search for the agent (#330)
Editorial roles (Corrector/Factchecker) brute-forced `get_node` block-by-block to
find occurrences (unquoted «ё», straight quotes, «т.е.»), burning tokens. New
`search_in_page(pageId, query, {regex?, caseSensitive?, limit?})` reads the page's
ProseMirror JSON via the existing getPageRaw and searches it IN MEMORY — no server
endpoint, no DB/schema change, no touch to the packages/mcp/src/lib schema mirror.
New pure `searchInDoc(doc, query, opts)` (packages/mcp/src/lib/page-search.ts):
recursive descent to each TEXT CONTAINER (paragraph/heading/table-cell paragraph),
glues its inline text via `blockPlainText` (a match survives inline-mark
boundaries — e.g. «т.е.» split across bold/italic), searches literal (indexOf) or
regex, and returns `{ total, truncated, matches:[{ nodeId, blockIndex, type,
before, match, after }] }`. `nodeId` is the container's attrs.id or the
`#<topLevelIndex>` of the enclosing top-level block — the SAME ref format
get_node/patch_node/comment-anchoring accept (verified identical to getNodeByRef),
so the agent goes straight from a hit to a targeted comment; `before`/`after` are
~40-char windows for a unique selection. `total`/`truncated` always reported (never
silent truncation). Lives in the SHARED_TOOL_SPECS registry → exposed in BOTH
transports (external /mcp + in-app AI-chat), with a SERVER_INSTRUCTIONS line and a
DocmostClientLike signature + contract-test entry. Corrector/Factchecker prompts
get a one-line "use search_in_page first" hint (versions bumped, catalog hash lock
refreshed).
Guards: empty/whitespace query → clear error; invalid regex → clear error (not a
generic 500); zero-length regex matches (`\b`, `a*`) skipped with lastIndex
advanced (no loop/flood); MAX_PATTERN_LENGTH=1000, MAX_CONTAINER_TEXT=100k bound
each exec; limit clamped [1,200] (default 50).
Tests: new page-search.test.mjs (17) — literal+regex, case-sensitivity,
mark-boundary glue, nodeId for paragraph/heading (attrs.id) and table-cell
(#<index> fallback), context bounds, limit/total/truncated + clamp, invalid
regex/empty/over-long errors, zero-length skip, empty-doc null-safety.
mcp: tsc clean; node --test 467 passed (+17). apps/server: tsc --noEmit clean
(DocmostClientLike + wiring). catalog check.mjs OK.
Known limitations (from internal review, non-blocking):
- Residual ReDoS: a crafted catastrophic-backtracking pattern (e.g. `(a+)+$`)
against a large single container can hang the event loop — JS regex is not
interruptible, so the length caps bound the base but not the backtracking.
Realistic exposure is low (containers are small; the pattern is supplied by the
authenticated model). Candidate for a follow-up hardening (safe-regex validation
or a worker+timeout) if it matters.
- Case-insensitive LITERAL search folds via toLowerCase; a char whose lowercase
differs in length (e.g. Turkish İ) BEFORE a match could shift the context
window — negligible for the RU/EN editorial scenario.
- On a `#<index>` table-cell fallback, `type` is the inline container ("paragraph")
while nodeId addresses the top-level block — addressing is correct; the field is
documented as the container's type.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
bcd194ee5d |
feat(mcp): hide resolved-comment anchors + feed from the agent (#328)
The AI agent (MCP + in-app chat) saw ALL comments incl. resolved via two channels, cluttering its context and breaking fragment search. Default now: the agent sees only ACTIVE discussions; resolved is opt-in. Active anchors and threads are always kept. Channel 1 — resolved comment anchors on agent reads (converter option): `convertProseMirrorToMarkdown(content, options?)` gains `options.dropResolvedCommentAnchors` (default false — zero change for every existing caller incl. git-sync). Both `case "comment"` emitters (top-level and the raw-HTML inlineToHtml path) emit BARE text (no `<span data-comment-id>`) when `resolved && the flag`; active anchors keep their wrapper. mcp `getPage` passes the flag; `export_page_markdown` does NOT (lossless export must preserve resolved anchors — that is why it is an opt-in option, not unconditional); `get_page_json` is untouched (lossless PM JSON). Built on the #293 package converter. Channel 2 — `list_comments` default active-only: `listComments(pageId, includeResolved=false)` now returns `{ items, resolvedThreadsHidden }` (was a bare array). By default a RESOLVED top-level thread is hidden wholesale — the root AND every reply anchored to it (a thread is gated only by its root's resolvedAt; a resolved reply under an ACTIVE root stays). `resolvedThreadsHidden` counts hidden threads so the agent knows to re-query. `includeResolved:true` returns everything. The `includeResolved` param is added to both tool registrations (MCP index.ts + in-app ai-chat-tools.service.ts); `DocmostClientLike` signature updated. Server `findPageComments` is NOT touched — the web UI's tabs depend on the full feed; filtering is only at the mcp-client level. All internal call sites (export_page_markdown / checkNewComments / transformPage) updated to `.items` with `includeResolved:true` to keep their full-feed behavior. The comment model is assumed FLAT (a reply's parentCommentId points at the thread root) — documented in the filter; a future reply-of-reply model would need a root-walk there. Tests: resolved-comment-anchors.test.ts (6 — anchor dropped with flag / kept without, for BOTH emitters; active always kept); list-comments-resolved.test.mjs (4 — resolved thread+reply hidden + counter; includeResolved:true returns all; an ACTIVE thread with a RESOLVED reply is NOT hidden). package vitest: 664 passed; tsc clean. mcp: node --test 458 passed; tsc clean. apps/server + git-sync: tsc clean (converter option default-off). NOTE: based on feat/293-B (#293/#326 STEP 5) — the converter lives in the package; this PR is stacked on #333 and its base retargets to develop once #333 merges. mcp/build is gitignored (not committed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
1a7b817250 |
fix(prosemirror-markdown): escape image alt + consolidate schema sanitizers + tidy (#333 review F1-F4)
F1 [critical, data-loss] — escape the image alt in ``. Canon #4 moved the top-level image off the lossless <img> form onto markdown ``, but the alt was inserted raw; the importer re-parses the `![alt]` label as CommonMark inline, so a markdown-active char in a realistic description ("Figure [1]", "the *new* logo", "a]b[c") broke the round-trip — the image node vanished or emphasis collapsed. Now `escapeLinkText(imgAttrs.alt ?? "")`, exactly as the link-form media (attachment/pdf/embed) already escape their visible text. Regression test added: six active-punctuation alts round-trip byte-stable with the node intact. F2 [drift] — re-export `clampCalloutType` / `sanitizeCssColor` from the package barrel and drop the verbatim copies in the mcp schema shim. The copies had already drifted (the mcp `clampCalloutType` lost the callout-type alias mapping the package applies), which is exactly the schema drift #293 exists to kill. The sanitizers now live only in the package; mcp `schema.test.mjs` exercises the single alias-aware implementation. F3 [docs] — AGENTS.md:296 said `packages/mcp/build/` is committed; this branch gitignored it (git-sync/prosemirror-markdown convention). Updated the line to say it is gitignored and rebuilt in CI/Docker via `pnpm build`. F4 [cleanup] — removed the dead `test.typecheck` block from the package vitest.config.ts and deleted tsconfig.vitest.json. Both were copied verbatim from git-sync; this package has zero `*.test-d.ts` files, and the ported comments referenced git-sync-only entities. Kept the `docmost-client` resolve alias (22 tests use it) and the runtime include/environment. package vitest: 658 passed (+1 F1 regression); tsc clean. git-sync: 268 passed. mcp: node --test 454 passed; tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
124f5a45a2 |
refactor(mcp): consume @docmost/prosemirror-markdown, drop the drifted converter copy (#293/#326 step 5)
mcp had its OWN drifted copy of the converter (markdown-converter.ts ~900 lines, docmost-schema.ts ~1270 lines, markdown-document.ts) — older than the shared package, missing the git-sync fixes AND the #293 canon. This switches mcp's converter CORE to @docmost/prosemirror-markdown, so mcp jumps straight to the canonical format and the drift-generating second copy is gone. - markdown-converter.ts / markdown-document.ts / docmost-schema.ts become thin re-export shims of the package (convertProseMirrorToMarkdown, the docmost:meta envelope, docmostExtensions + docmostSchema=getSchema(docmostExtensions)). The mcp-only helpers clampCalloutType/sanitizeCssColor are preserved verbatim in the schema shim (the package doesn't expose them via its barrel). ~2170 lines of the drifted converter/schema bodies deleted. - collaboration.ts drops its own ~360-line marked pipeline (preprocessCallouts, bridgeTaskLists, extractFootnotes, the footnoteRef extension) and re-points to the package's markdownToProseMirror, keeping markdownToProseMirrorCanonical and all the yjs/collab write glue. footnote-lex/analyze doc comments updated (they now describe advisory legacy-syntax diagnostics, not an importer). Schema parity verified: the package schema is a strict SUPERSET of mcp's old schema — every node and attr mcp declared is present (the package only adds status/pageEmbed/transclusion*/subpages.recursive/etc.), so nothing is silently dropped on the switch. The switch actually FIXES two pre-existing mcp data-loss bugs its own tests documented: htmlEmbed and pageBreak now round-trip (were dropped by the old mcp converter). Footnotes: the package assembles inline ^[body] footnotes on import (sequential fn-N ids, identical bodies merged), so mcp's canonicalizeFootnotes is now an idempotent no-op after it (verified). Legacy reference footnotes [^id]/[^id]: are inert literal text (canon #2 no-backward-compat) — lossless, the text survives verbatim. Build hygiene: packages/mcp/build/ is now gitignored and untracked, matching the git-sync/prosemirror-markdown convention (private package, rebuilt in CI/Docker, so src and prod can never silently diverge). This also removes a dead untracked build/_vendored_editor_ext/ artifact that a broad `git add` would otherwise commit. Dependency: packages/mcp/package.json gains @docmost/prosemirror-markdown (workspace:*); pnpm-lock.yaml gets the matching link importer (mirrors git-sync). mcp tests updated deliberately to the canonical forms (highlight ==, math $…$, image <!--img-->, drawio/media discriminators, subpages/pageBreak comments, textAlign, inline ^[…] footnotes) with strict assertions; 4 structural safety-net round-trip tests added. mcp: node --test 454 passed; tsc clean. package: 657 passed. git-sync: 268 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
351615e5bc |
prompt(mcp): fix inaccurate and misleading tool descriptions
Audit of all 41 tool descriptions against the actual implementation found factually wrong or misleading texts: - list_comments claimed '(paginated)' — it takes only pageId and returns ALL comments in one call (internal pagination); now also states that RESOLVED threads are included and how to filter them. In-app twin synced. - search claimed the limit default is 'applied by the client' — the client deliberately omits it so the SERVER applies its default. - create_page's '(automatically moves it to the correct hierarchy)' said nothing useful — now documents parentPageId nesting semantics; move_page drops the stale 'essential for organizing pages created via create_page'. - share_page now warns the page becomes accessible to ANYONE with the URL. - get_page (both transports) now explains inline <span data-comment-id> tags are comment anchors (incl. resolved) — markup, not page text. - patch_node/delete_node/insert_node pointed only at the expensive page-JSON view for block ids — now route through the cheap page outline first. - docmost_transform marks 'Примечания переводчика' as the DEFAULT notesHeading, overridable for non-Russian pages. Checks: @docmost/mcp tests 450/450 (incl. the server-instructions guard); server ai-chat-tools spec 20/20; mcp build/ artifacts rebuilt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
1fda0ec8b0 |
prompt(mcp): rewrite SERVER_INSTRUCTIONS to cover all tools + guard test
The intent-routing guide had rotted: 17 of 41 registered tools were absent (get_outline, get_node, the whole table_* family, search, stash_page, sharing, page lifecycle), and two tips were actively harmful — 'read block ids via get_page_json' told agents to pull the whole ~100KB document when get_outline exists precisely to grab ids cheaply, and 'table cell -> patch_node by attrs.id' dead-ends because table nodes carry no attrs.id. - Rewrite SERVER_INSTRUCTIONS as intent clusters (READ / EDIT / PAGES / COMMENTS / HISTORY) covering every tool except get_workspace; add safety notes (share_page = PUBLIC, delete_page = soft) and a comment-anchor markup warning for get_page. - delete_page tool description: state SOFT delete / restorable explicitly. - MAINTENANCE RULE comments at both registration sites (index.ts, tool-specs.ts) + an AGENTS.md convention bullet: adding/renaming/removing a tool REQUIRES updating the guide. - New guard test (test/unit/server-instructions.test.mjs): extracts every registered tool name from source and fails when one is not mentioned in the shipped SERVER_INSTRUCTIONS (word-boundary match, so get_page can't hide behind get_page_json); EXCEPTIONS list is itself validated against the registry. SERVER_INSTRUCTIONS exported for the test. Tests: @docmost/mcp 450/450 (448 + 2 new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
588596fb2f |
prompt(agents): teach agent prompts to use comment suggestedText fixes (#315)
- editorial roles (ru/en): proofreader and line editor attach suggestedText replacements to targeted fixes; fact-checker ALWAYS attaches the ready correction for [Incorrect] verdicts; structural editor and narrator get a light-touch rule for in-place rewordings; role versions bumped and the content-hash lock refreshed - MCP SERVER_INSTRUCTIONS: route 'propose a concrete text fix for one-click human approval' to create_comment with suggestedText (unique-selection reminder); build/ artifacts rebuilt - AI-chat SAFETY_FRAMEWORK: mention the comment-suggestion capability so the default assistant offers ready fixes instead of only describing changes Checks: catalog check.mjs OK; @docmost/mcp tests 448/448; server ai-chat.prompt spec 28/28. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
48c1ec46f7 |
fix(comment): store the real anchored substring as expectedText + pin authz (#318 F1/F2)
F1 [blocking]: a suggestion whose anchor matched via normalization could never be applied (spurious 409). The comment mark lands on the doc's ACTUAL text (Docmost auto-converts to typographic quotes/dashes/nbsp), but the stored selection — used as expectedText at apply — was the raw ASCII agent input (+substring(0,250)). So replaceYjsMarkedText's strict joined!==expectedText always failed and threw "text changed" though nobody edited. Fix: new pure getAnchoredText(doc, selection) reconstructs the exact raw doc substring the mark covers (slicing identical to spliceCommentMark); on the suggestion path client.createComment stores THAT as selection, so expectedText equals the marked text and apply returns applied:true. Live anchoring still uses the raw agent selection (normalization still finds the anchor). Truncation raised 250->2000 (+ DTO @MaxLength(2000)) so the anchored substring is never cut below the mark span. Ordinary comments unchanged. AI-chat shares client.createComment, so covered. Regression tests: getAnchoredText raw-vs-ASCII; create payload selection is the typographic substring; apply with typographic expectedText -> applied. F2 [blocking]: added comment.controller.spec.ts pinning that validateCanEdit runs before applySuggestion (Forbidden -> applySuggestion never called; happy path -> called; missing comment -> 404 without authorizing). MCP 448 pass; server comment+yjs 54 pass. MCP build/ rebuilt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
cd539558ed |
feat(agent-tools): suggestedText on create_comment with strict anchor uniqueness (#315 phase 6)
Agents can attach a suggested replacement when creating an inline comment, via both the MCP create_comment tool and the AI-chat createComment tool. Because applying a suggestion edits the EXACT anchored text, an ambiguous anchor would let Apply corrupt the wrong occurrence. So when suggestedText is set the selection must occur EXACTLY ONCE: - new countAnchorMatches(doc, selection) counts occurrences across all blocks (same normalization/traversal as canAnchorInDoc), counting occurrences (2 in one block => 2) — stricter than block-count, never under-counting distinct occurrences (false-unique is the dangerous direction). - client.createComment gains suggestedText: a pre-check (getPageJson + countAnchorMatches: 0 => not-found, >=2 => ambiguity error) before create, and an AUTHORITATIVE live check inside the anchoring mutation that recomputes on the live doc and, if != 1, aborts and rolls back the just-created comment (reusing the existing safeDeleteComment "anchor not found" path). Ordinary comments keep first-occurrence behavior unchanged. - suggestedText is rejected on a reply or without selection in all three layers (MCP handler, MCP client, AI-chat tool), mirroring the server DTO/service. - filterComment surfaces suggestedText/suggestionAppliedAt/suggestionAppliedById. - DocmostClientLike.createComment signature updated. MCP build/ rebuilt. Tests: countAnchorMatches (0/1/N, within/across/nested block, span nodes, quote normalization); createComment (ambiguous refused pre-create, reply and no-selection rejected, unique succeeds and forwards suggestedText, filterComment surfaces it); ai-chat schema accepts suggestedText. MCP 443 pass; ai-chat 601 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
f720151c63 |
refactor(ai-chat): move patch_node/insert_node metadata into the shared tool-spec registry (#294)
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> |
||
|
|
e04afee629 |
test(#260): cover replaceImage's UUID lock-key invariant; drop dead cache line
Reviewer round 1 on the #260 collab-doc-name fix: - F1: replaceImage is the one path where the resolved UUID gates BOTH the collab-doc open AND the per-page mutex key (withPageLock(pageUuid)). Add a deterministic test to resolve-page-id-collab-doc-name.test.mjs: it gates /files/upload so replaceImage parks mid-upload holding its lock, asserts the doc opened as page.<uuid> (never page.<slug>), and probes the SHARED page-lock chain — a withPageLock(UUID) probe must stay blocked while replaceImage holds it (with a free-key probe as a non-vacuity guard). The test fails if the lock key is reverted to the slugId (verified). - F2: drop the dead `pageIdCache.set(uuid, uuid)` — resolvePageId returns on the isUuid() short-circuit before the cache is ever read with a uuid key, so only slugId->uuid entries are stored/read. Comment corrected to match. MCP suite 430/430, tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
3b80285d57 |
fix(#260): open MCP collab docs by canonical UUID (slugId doc-name split)
Real root cause of the silent MCP edit loss: the web editor always opens the
collaboration document by the page UUID (`page.${page.id}`), but the MCP
opened it by the agent-supplied id — usually a slugId — so `page.${pageId}`
became `page.<slugId>`. For one DB page that is TWO independent Yjs documents;
both persist to the same `pages` row (findById/updatePage resolve id or
slugId), so the human tab's debounced store overwrites the agent edit
(last-store-wins) — gone after reload, never shown live. The slugId doc also
made the server's transclusion sync + embedding reindex throw Postgres 22P02.
Fix:
- MCP (primary): resolvePageId(pageId) returns the canonical UUID — a UUID
short-circuits with no network call, a slugId resolves once via getPageRaw
and is cached both ways. Every collab-write path (mutatePageContent /
updatePageContentRealtime / replacePageContent and the mutate/replace/
unlocked seams) now opens by the resolved UUID, so the MCP and the editor
share ONE Yjs doc. replaceImage's whole-operation page lock also keys on the
UUID so it serializes against the other (now-UUID-keyed) writes.
- Server (defense + kills the 22P02 noise): onStoreDocument passes the resolved
page.id — not the raw doc-name id — to syncTransclusion, the embedding queue,
the mention-notification job, addContributors, and the in-tx history read.
Content store and the empty-guard are untouched.
Tests: a new MCP test stands up a real Hocuspocus server and asserts a slugId
input opens `page.<uuid>` (never `page.<slugId>`), with UUID short-circuit and
single-resolve caching; the server spec asserts the side-effects receive the
UUID for a `page.<slugId>` doc. closes #260
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
22ea387495 |
Merge pull request 'feat(#246): inline spoiler mark (blur + click-reveal, lossless Markdown)' (#259) from feat/246-spoiler into develop
Reviewed-on: #259 |
||
|
|
b56a1629d2 |
Merge pull request 'feat(editor): image captions (figcaption) with lossless markdown round-trip (#221)' (#233) from feat/221-image-captions into develop
Reviewed-on: #233 |
||
|
|
a8a7fad850 |
Merge pull request 'test(#244): Part B backlog — editor-ext/mcp/client/server unit+contract tests + findBreadcrumbPath mutation fix' (#257) from test/244-part-b into develop
Reviewed-on: #257 |
||
|
|
f9d8a6ede1 |
fix(mcp): mirror the spoiler mark in the vendored MCP schema; changelog (F1,F2)
F1 (data loss): packages/mcp keeps its own copy of the document schema (AGENTS.md), and the spoiler mark was only added to editor-ext + the server tiptapExtensions, so a doc with a spoiler silently lost the mark through /mcp. Add a local Spoiler mark to docmostExtensions (span[data-spoiler] parse, data-spoiler="true"+class render) and a case "spoiler" in markdown-converter emitting the same <span data-spoiler="true">…</span> as the editor-ext turndown rule; add an MCP json->md->json round-trip test. Regenerated build/lib output. F2: add the #259 inline-spoiler entry to CHANGELOG [Unreleased] Added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
0724d8d362 |
feat(mcp): expose resolve_comment tool to resolve/reopen comment threads
The Docmost backend (POST /comments/resolve) and the MCP client method resolveComment() already supported resolving/reopening comment threads, but no MCP tool surfaced it — so agents could only close threads destructively via delete_comment. Register a resolve_comment tool wrapping the existing client method. - packages/mcp/src/index.ts: register resolve_comment (commentId + optional resolved, default true → close; false → reopen); extend SERVER_INSTRUCTIONS - packages/mcp/build/index.js: regenerated via tsc - packages/mcp/README.md / README.ru.md: document resolve_comment; bump tool count 40 → 41 - packages/mcp/test-e2e.mjs: add resolve → verify resolvedAt → reopen coverage Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
e2b7ff10d9 |
test(mcp): media round-trip attrs, cookie parsing, anchor apply, recreate drift
Extract pure extractAuthTokenFromSetCookie from performLogin (behavior-identical) so cookie parsing is unit-testable without a network login. Add round-trip coverage for media attrs (width/height/align/drawio/escaping) the existing suite omitted; applyAnchorInDoc selection/ambiguity/atom-break cases; and a cross-copy drift guard proving the vendored editor-ext recreate-transform and the @fellow npm copy used by diff.ts emit identical steps (apply(diff)==target). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
b02101b58a |
docs(mcp): correct captioned-image import comment (F6)
The comment referenced markdownToHtml, which does not exist in the mcp package; the import path is marked.parse + generateJSON (which runs the image extension's parseHTML). Describe the actual step and regenerate the build artifact in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
607aed5997 |
fix(mcp): restore image caption on markdown round-trip (F1)
Stock @tiptap/extension-image carries no caption attribute, so markdownToProseMirror through docmostExtensions dropped the data-caption the client emits, breaking the lossless claim. Extend the Image node (mirroring editor-ext image.ts and the nearby Highlight extend) to parse/render data-caption. Rebuilt build/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
204cf9dfe7 |
test(sandbox): address PR #250 round-4 review — SSRF accept-path tests, MCP structuredContent (#243)
Mandatory (test-coverage): - internal-file-urls.test: pin the SSRF/traversal ACCEPT path of resolveInternalFilePath (the sole guard for content-controlled `src`): an absolute/protocol-relative URL has its foreign host dropped and only an /api/files/ pathname survives (http://evil.com/api/files/x/y.png -> /files/x/y.png), while a host-dropped path that escapes /api/files/ (https://evil.com/api/auth/whoami) or a backslash-traversal (/api/files\..\auth\whoami) is rejected. Locks the behavior so a future prefix-only refactor cannot silently open a bypass. Suggestions: - index.ts: the stash_page MCP tool now returns structuredContent { uri, sha256, size, images } alongside the resource_link, so the MCP output matches the documented shape (clients get the blob's sha256/ETag and the mirror counts, not just the link). No outputSchema registered. Rebuilt build/. - new stash-page-mcp-result.test: server round-trip via InMemoryTransport asserts both the resource_link and the structuredContent mirror. - internal-file-urls.test: cover the new URL parse-failure catch branch (http://[ -> "Invalid internal file src"). - environment.service.spec: assert getPositiveIntEnv warns once per key and independently across keys (the invalidPositiveIntWarned dedup). Tests: packages/mcp 383 pass; apps/server sandbox/environment/mcp 235 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
8842bc8bf3 |
fix(sandbox): address PR #250 follow-up review — XSS hardening, eviction reconcile, doc sync (#243)
Security (must-fix):
- sandbox.controller: the anonymous GET /api/sb/:id response now sets
X-Content-Type-Options: nosniff, a restrictive CSP, and Content-Disposition=
attachment for any mime outside a raster-image allowlist (png/jpeg/gif/webp/
avif). entry.mime is attacker-controlled, so an evil.svg/evil.html could
otherwise execute script inline on the Docmost origin (stored XSS). Mirrors
the public attachment route's hardening.
Stability:
- client.stashPage: reconcile mirrors AFTER the final document put, not only
before it. The doc blob is the newest entry and FIFO eviction drops the
oldest = this stash's own images, so the stored doc could reference an
evicted blob (consumer 404) and over-report images.mirrored. A bounded loop
now reverts doc-put-evicted mirrors, drops the stale doc blob, and re-puts
until stable. Regenerated packages/mcp/build/.
- sandbox.controller: emit Cache-Control on the 304 branch too (ttlSeconds is
computed before the conditional check).
Docs:
- Bump the MCP tool count 39 -> 40 across all READMEs and AGENTS.md (the
registry now exposes exactly 40 tools).
Refactor:
- SandboxStore.asSink() centralizes the {put,has,evict} sink + uri<->id
mapping; the embedded-MCP and in-app agent-tools wiring sites share it.
Tests:
- security headers (inline vs attachment, nosniff, CSP), 304 Cache-Control,
putAndLink URL form, has()/remove(), asSink() round-trip, getSandboxPublicUrl
(trailing-slash trim + APP_URL fallback), and a stash test where the doc put
itself evicts a mirrored image.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
|
6eb335d5e3 |
fix(sandbox): address PR #250 review — SSRF guard, eviction safety, cleanup (#243)
Security: - stash_page: reject path-traversal / percent-encoded srcs before the authed loopback fetch (resolveInternalFilePath), closing an SSRF/exfiltration hole where a crafted node.attrs.src could read an arbitrary internal GET endpoint into the anonymous sandbox. Stability: - stash_page: revert + recount mirrors FIFO-evicted by a later put in the same stash (no dangling sandbox refs, honest images.mirrored/failed); free image blobs if the final document put throws. - Reject/clamp non-positive SANDBOX_TTL_MS to the 1h default (warn once). - Log mirror failures unconditionally (console.warn, no blob bodies). Cleanup / architecture: - Remove dead expiresAt from SandboxPutResult. - Centralize the /api/sb route in SANDBOX_ROUTE_SEGMENT/SANDBOX_API_PATH and move URL composition into SandboxStore.putAndLink; drop the duplicated sink closures and the now-unused EnvironmentService injection from McpService and AiChatToolsService. - Un-export isInternalFileUrl; document the process-local (instance-bound) sandbox limitation in the tool description and .env.example. Docs/tests: - README/README.ru: 38 -> 39 tools + stash_page entry. - Add traversal/normalize/recursion unit tests, stash self-eviction + doc-put-throw + empty/octet-stream mock tests, controller If-None-Match (wildcard/weak/list) + Cache-Control tests, and SANDBOX_TTL_MS validation tests. Regenerate packages/mcp/build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
2fe4ca8537 |
feat(sandbox): in-RAM blob sandbox for out-of-band page transfer (#243)
Add an ephemeral, process-local blob store so the in-app agent (and the
embedded MCP) can hand a large page document and its images to an external
consumer WITHOUT routing the bytes through the model context or Docmost auth.
- SandboxStore (@Injectable singleton): Map<uuid,{buf,mime,sha256,expiresAt}>
in RAM only. put() picks a per-blob cap by mime (image vs doc), enforces a
total-bytes RAM guard with oldest-first eviction, and stamps a TTL; get()
lazily expires. sha256 computed at put() doubles as the strong ETag. An
unref'd sweep interval clears expired entries and is cleared on destroy.
- GET /api/sb/:uuid anonymous controller: serves raw bytes with Content-Type,
Content-Length and ETag=sha256; 404 on missing/expired/non-UUID (anti-
traversal), 304 on a matching If-None-Match. No tokens, no 401 — the
capability is the unguessable UUID + short TTL + TLS. Auth-exempt the same
way as /api/files/public (no JwtAuthGuard) plus an /api/sb entry in main.ts's
workspace-resolution preHandler so a remote consumer with no workspace host
is not rejected.
- stash_page tool in both layers (MCP resource_link + in-app {uri,size,sha256,
images}). client.stashPage serializes the get_page_json shape, mirrors every
INTERNAL file/image src (type-agnostic, covers drawio/excalidraw/video/file)
into the sandbox under Docmost auth and rewrites src to the sandbox URL;
external http(s) srcs are left untouched; dedup by src; a failed image fetch
is counted, never aborts the doc.
- SANDBOX_PUBLIC_URL / SANDBOX_TTL_MS / SANDBOX_MAX_BYTES /
SANDBOX_MAX_IMAGE_BYTES / SANDBOX_MAX_TOTAL_BYTES wired through the
environment service + validation + .env.example.
- SandboxModule (@Global) provides the shared store to the controller,
McpService and AiChatToolsService (same instance for put and get).
Tests: SandboxStore (round-trip, sha256, TTL lazy + sweep, caps, eviction),
SandboxController (200+ETag+CT+CL, 404 missing/expired/non-UUID, 304), and a
mock-HTTP stashPage test (mirror+rewrite internal, keep external, dedup, failed
image counted, returns only a link). Interoperates with the vvzvlad/habr-mcp
consumer's anonymous-GET + sha256-ETag + resource_link contract.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
2aa482f62d |
feat(editor): add editable image captions (#221)
Add a visible caption (<figcaption>) under images, editable from the image bubble-menu and persisted across all formats: native Yjs/JSON, HTML export, and Markdown. - image node: new plain-text `caption` attribute (parse/render `data-caption` on <img>, emitted only when set) + `setImageCaption` command. The node stays an atom; the schema shape is unchanged, so the server's generateHTML/generateJSON path round-trips it for free. - resize node-view: re-parent the resizable wrapper into a <figure> and render the caption in a <figcaption> BELOW it, outside nodeView.wrapper (so onCommit's offsetHeight measurement and the left/right resize handles still cover the image only). This path also drives read-only / share rendering. React placeholder view renders the caption too. - bubble-menu: new useCaptionControl panel modeled on useAltTextControl (own icon, Caption strings, softer sanitizer, ~500 char limit). - markdown lossless round-trip: a captioned image is emitted as a raw <img data-caption> wrapped in a block <div> (same trick as <video>) in both the editor-ext turndown rule and the MCP converter; caption-less images stay clean . Import restores the caption via the shared markdownToHtml + parseHTML. - styles + i18n keys; tests for the schema attr round-trip, markdown round-trip (editor-ext) and the MCP converter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
c4ed4a4855 |
fix(footnotes): strip bare definitions on rebuild; MCP full-doc + zip-import canonicalize tests (#228)
Review #6 (approve-with-comments) follow-ups: 1. canonicalize step 7 now strips bare footnoteDefinitions at ANY depth (stripFootnoteDefinitionsDeep), not just footnotesList, in BOTH copies. A definition hand-authored outside a list (e.g. nested in a callout via a raw-JSON write path) was left in place while a copy was also added to the rebuilt list -> duplicate, idempotent, self-perpetuating. Runs only in the rebuild path (after the lists are stripped); the fast-path / placement-keep branch is untouched. Added a shared-corpus case (bare def nested in a callout) to pin it in both mirrors. 2. markdown-clipboard: removed the dead top-level footnoteReference check in canonicalizePastedFootnotes (an inline atom is never a top-level slice child; only the descendants scan can find it). Test coverage: 4. New MCP binding tests (full-doc-write-canonicalize.test.mjs): update_page_json and copy_page_content canonicalize the persisted full doc, asserted via a new `replacePage` seam (symmetric to the existing `mutatePage` seam) so no live collab socket is needed. Routed both writers through the seam. 5. New server spec (file-import-task.service.footnote-canonicalize.spec.ts): the zip-import path (processGenericImport) canonicalizes footnotes — real markdown->HTML->JSON via a real ImportService over a temp-dir .md file, DB trx stubbed to capture the persisted page content. FileImportTaskService had no spec before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
9c1f952b2f |
fix(footnotes): guard insert against nested/bare definitions, skip definitions-only paste, doc + reorder fixes (#228)
Must-fix: - insertInlineFootnote could glue a footnoteReference inside an EXISTING definition (nested footnotesList, or a bare footnoteDefinition with no list wrapper), which canonicalize then dropped as an orphan — silently losing the definition's prose. Now: (a) the body/notes boundary is computed from the first top-level block that IS or CONTAINS (recursively) a footnotesList/ footnoteDefinition, not just a top-level list; and (b) the insertNodesAfterAnchor core skips footnotesList/footnoteDefinition subtrees entirely (skipSubtreeTypes), so an anchor whose only match is inside a definition -> inserted:false (clean abort, no write). Added tests: nested-definition, bare-definition, and body-before-nested-list-still-inserts. - editor-ext footnote-canonicalize header listed `markdownToProseMirror` among the canonicalizing MCP paths; it is the NON-canonicalizing primitive. Replaced with `markdownToProseMirrorCanonical` (+ note that the plain primitive is for comment bodies) and added copy_page_content. - Client paste: canonicalizePastedFootnotes now skips a definitions-ONLY paste (no footnoteReference anywhere) — canonicalizing it would strip the reference-less list and yield an EMPTY paste. Added a test. Suggestions: - docmost_transform now runs validateDocStructure/validateDocUrls on the RAW transform output BEFORE canonicalizeFootnotes (mirrors updatePageJson), so a too-deep doc gives the intended max-depth error instead of a stack overflow. - docmost_transform tool description now states the RESULT is footnote-canonical (dryRun diff may show tidy-ups; idempotent after first run). - insertFootnote: dropped the dead `result ? … : undefined` ternaries and the `as any` casts (result is always set by the time we return; the not-found path throws and aborts mutatePage). `const r = result!;`. Tests / architecture: - Added a LIVE-plugin golden case: the real footnoteSyncPlugin leaves a list with non-empty content after it in place, and canonicalize agrees (placement parity is now a driven property, not a hand-set expected). - Added generateFootnoteId uuidv7 shape + uniqueness test. - Item 9: added the ENFORCEMENT-RULE comments at the server parseProsemirrorContent and the MCP canonicalizer header (any NEW full-doc persist path MUST canonicalize; fragments/append/prepend and comment bodies MUST NOT). Kept per-call-site over a brittle grep CI test (the replace-vs-fragment + comment-vs-page nuance makes a single wrapper unsafe). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
3fd66b4245 |
fix(footnotes): don't canonicalize comment bodies (data loss); canonicalize only page write paths (#228)
Must-fix (REAL DATA LOSS): - markdownToProseMirror is reused for COMMENT bodies (createComment/updateComment). It unconditionally canonicalized, so a comment carrying a standalone footnote definition ([^1]: text with no matching reference) had its whole footnotesList stripped (referenceIds.length===0 -> stripFootnotesListsDeep) — the text vanished. Fix: markdownToProseMirror no longer canonicalizes (content-preserving primitive); a new markdownToProseMirrorCanonical wraps it for the PAGE write paths (markdown import via importPageMarkdown, update_page markdown via updatePageContentRealtime). Comment callers keep the non-canonicalizing primitive. Updated the now-false header comment and added create/update-comment inline notes. Added collaboration tests: comment path PRESERVES a reference-less definition; page path still drops it AND still reorders real footnotes. Updated the page-import canonicalization test to use the canonical variant. Suggestions / architecture: - #2: collapsed transforms.footnoteDefinition onto the shared makeFootnoteDefinition factory (adds only the inner paragraph block id); kept the dependency direction transforms -> footnote-authoring (no circular import, mirror stays pure). - #3: confirmed docmost_transform auto-canonicalization is documented (inline comment, tool description, CHANGELOG) — no code change. - #4: copyPageContent is a FULL-document write (replacePageContent of a type:"doc"); added a defensive canonicalizeFootnotes pass (no-op on already-canonical source). - CHANGELOG entry refined to list the FULL-document write paths (incl. copy_page_content) and to state canonicalization is NOT applied to comment bodies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
a77a0bc92b |
fix(footnotes): re-review #232 — refuse footnoteRef into codeBlock/definition, deep-strip nested lists, docs + cross-copy guard (#228)
Must-fix: - REAL BUG: insertInlineFootnote could splice a footnoteReference (inline atom) into a codeBlock or an existing footnoteDefinition, persisting a schema-invalid doc (insert_footnote skips validateDocStructure). Now the search is bounded to the BODY (before the first footnotesList) and the insertNodesAfterAnchor core refuses textblocks that can't hold the atom (codeBlock); when the only match is in such a place the insert returns inserted:false and the write aborts cleanly. Reachable via docmost_transform too. Added codeBlock / definition / fall-through tests. - Fixed the deepEqualJson doc comment in both copies: arrays are order-SENSITIVE (correctness depends on it), only object keys are order-insensitive. - README.ru.md MCP tool count 38 -> 39 (lines 36/47/63), matching README.md/AGENTS. - CHANGELOG [Unreleased] Added entry for insert_footnote + server-side footnote canonicalization on non-editor write paths (#228). Suggestions: - canonicalize step 5/7 now strips footnotesList at ANY depth (both copies), so a schema-valid list nested in a callout/blockquote can't leave duplicate defs. - Exclude the test-only footnote-corpus.ts fixture from the editor-ext build (tsconfig), so it no longer ships in dist/. - Removed the duplicate manual canonicalize cases from the MCP unit test (the shared corpus covers them via full deepEqual); kept idempotence + immutability. - insertInlineFootnote dedup key now keys off the inline array directly (footnoteContentKey({ content: inline })) instead of a throwaway node. Tests / architecture: - New client-wrapper test (#9): overrides a small mutatePage seam to assert the not-found path throws and persists NOTHING, and the success path shapes footnoteId/reused/message/verify and writes the right content. Fixed the misleading comment in footnote-write.test.mjs. - B: cross-copy corpus parity guard test (loads both corpora, asserts deep-equal) so a typo in one copy can't pass both suites green. - A: declined — the full-vs-fragment decision lives at the call site, so a prepareDocForPersist wrapper would be a bare alias for canonicalizeFootnotes; kept the existing per-call-site comments instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
07ebd8c63e |
fix(footnotes): address PR #232 review — fragment-safe canonicalization, plugin placement parity, dead-code removal (#228)
Must-fix: - Move canonicalizeFootnotes OUT of parseProsemirrorContent. It now runs only on FULL writes (createPage, updatePageContent operation==='replace'), never on an append/prepend fragment (a fragment would lose definition-only footnotes or synthesize a bogus empty list). Add a server binding spec. - Match the live plugin's list PLACEMENT: a single already-canonical footnotesList is left exactly where it sits (the plugin never repositions a sole correct list), so the first write no longer reorders content that follows the list. Applied to BOTH the editor-ext copy and the MCP mirror; pinned by a shared golden corpus case with content after the list. - Fix MCP tool count 38 -> 39 (README x3, AGENTS.md) and the transformJs param help (add canonicalizeFootnotes/insertInlineFootnote). Simplifications: - Remove the dead duplicate re-id mechanism (deriveFootnoteId/suffix/occurrence) from the PURE canonicalizer in both copies — references are never renamed, so the derived ids were never requested; first-wins-drop is the real behaviour. This also makes the editor-ext footnote-util note about "no cross-package copy" true again. - Remove the sentinel round-trip in insertInlineFootnote: a generalized insertNodesAfterAnchor core inserts the footnoteReference node directly. - Drop the redundant per-definition deep clone in step 4 (shallow id-normalizing copy; out is already deep-cloned). Docs / architecture: - Correct the editor-ext copy's "It exists because…" header to its real consumers (server import, page.service create/update, client paste). - Note markdownToProseMirror reuse for create/update comment in collaboration.ts. - A: shared golden JSON corpus exercised by BOTH the editor-ext copy and the MCP mirror (footnote-corpus.ts / .mjs) so "the two copies behave identically" is checkable. - C: split the MCP canonicalizer into a pure mirror + footnote-authoring.ts. - B: import services persist via a different path, so left one-line consolidation comments at the call sites rather than folding (does not fall out cleanly). Tests: insertFootnote wrapper guards + docmost_transform dryRun auto-canonicalize (MCP mock), page.service create/update + append/prepend binding (server jest), shared corpus incl. nested-container reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
30cb9d293c |
feat(footnotes): inline authoring + deterministic server-side canonicalization
Make footnotes author-inline: the agent/tool inserts a footnote at its point of use (anchor + text) and the numbering plus the bottom list are DERIVED deterministically server-side. The agent has no access to footnotesList and cannot desync — out-of-order lists, orphan definitions, and raw trailing [^id] blocks become structurally impossible. editor-ext: - canonicalizeFootnotes(docJSON) -> docJSON: a pure, EditorView-free port of footnoteSyncPlugin's end-state. Distinct reference ids in document order are the source of truth; exactly one trailing footnotesList holds one definition per referenced id in reference order (reusing the existing node or synthesizing an empty one); orphans dropped; duplicate definitions resolved deterministically (first wins, never lost); idempotent. - Unit tests + a golden parity suite: on every editor-reachable steady state the live footnoteSyncPlugin's JSON is a canonicalize no-op (byte-for-byte parity), and the canonicalizer additionally repairs the out-of-order list a non-editor write produces. mcp: - footnote-canonicalize.ts: behavioural mirror of the editor-ext canonicalizer (the MCP package is intentionally decoupled from the editor barrel, like footnote-lex/docmost-schema), plus footnoteContentKey for content dedup. - Auto-canonicalize on EVERY write path: markdownToProseMirror (fixes import ordering), update_page_json, and after every docmost_transform. Idempotent, so it is a no-op when footnotes are already canonical. - insert_footnote tool + insertInlineFootnote: anchor + markdown text -> a mark-safe footnoteReference and a content-dedup'd definition; the list and numbering are derived. Same-content footnotes reuse one number/definition. - canonicalizeFootnotes + insertInlineFootnote exposed as docmost_transform sandbox helpers. Tests: editor-ext 157 green; MCP 325 green; server + client tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
f80276d41a |
refactor(review): address PR #185 review (lease leak, tests, changelog, jsonb seam)
8-point multi-aspect review of the batch PR; security/regressions were clean. 1. Lease leak: the #180 reorder moved `toolsFor` (which leases external MCP clients, refCount+1) ahead of buildSystemPrompt + forUser, but the only release (closeExternalClients) was bound to the streamText callbacks. A throw in between leaked the lease (refCount stuck, undici sockets held until restart). Define closeExternalClients right after the lease and wrap buildSystemPrompt+forUser in try/catch that closes-then-rethrows. 2. Cover the patch_node/delete_node dup-id refusal (#159 #6): extract the guard into a pure `assertUnambiguousMatch` (node-ops) and unit-test 0/1/>1. 3. Regress the body-before-title order (#159 #10): mock-HTTP test (collab fails fast against a server with no WS upgrade) asserts /pages/update (title) is NEVER posted when the body write fails — for updatePage AND updatePageJson. 4. CHANGELOG [Unreleased]: #180, #168 (Added); #163 (Fixed). 5. Add the missing en-US i18n keys (Back to references / {{label}}). 6. Drop the duplicate content/empty/blank cases in ai-chat.prompt.spec.ts (they repeat the buildMcpToolingBlock unit tests); keep only sandwich placement + both-safety-copies. 7. CI Postgres pg16 -> pg18 (match docker-compose). 8. jsonb decode seam: shared `parseJsonbValue(value, guard)` in database/utils.ts holds the legacy double-encoding self-heal in one place; parseToolAllowlist / parseModelConfig keep only a type-guard. Verified: server build + 124 unit + 15 integration; mcp 311; prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
8f1af676ba |
fix(mcp): write page body before title to avoid split-brain on failure (#159)
updatePage (markdown) and updatePageJson wrote the title via REST FIRST, then the body via collab. If the body write failed (e.g. a collab persist timeout), the page was left with the NEW title over its OLD body — a split-brain the tool reported as an error but never repaired (red-team finding #10). Reorder both: write the body first, and only set the title after the body has persisted. Now a body-write failure leaves the title untouched (no split-brain). A title write failing after a successful body is rarer (REST is fast) and leaves correct content under a stale title — the strictly lesser inconsistency — which is the same trade-off the issue's "atomic, or roll back the title" intends, without the fragility of a rollback write that could itself fail. No unit test: both paths require a live collab provider and the suite has no provider mock; the change is a pure reordering. All 306 mcp tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
fdaf20ca7b |
fix(mcp): refuse ambiguous patch_node/delete_node on duplicated ids (#159)
Docmost duplicates block ids on copy/paste, and copyPageContent writes the source document verbatim with the same ids. `patchNode`/`deleteNode` address a block by `attrs.id` via replaceNodeById/deleteNodeById, which act on EVERY node sharing the id — so a single patch_node/delete_node could silently replace/remove multiple unrelated blocks with no signal to the model (red-team finding #6). Guard both write paths: when more than one node matches the id, skip the write entirely (the transform returns null -> no mutation) and throw a clear "ambiguous id — N nodes share it" error so the model re-targets with a more specific anchor. Only an unambiguous single match is written; the 0-match and 1-match behavior is unchanged. The duplicate-count basis is covered by node-ops.test.mjs (replaceNodeById / deleteNodeById report count===2 for a 2-duplicate doc). The end-to-end guard is not unit-tested because patchNode/deleteNode require a live collab provider and the test suite has no provider mock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
a766672574 |
fix(mcp): replaceImage no longer yanks the cursor (#164)
`mutateLiveContentUnlocked` — the write path used by `replaceImage` — still did the pre-#152 destructive write (delete the whole fragment + applyUpdate a fresh Y.Doc), discarding every Yjs node id. y-prosemirror anchors the editor selection to those ids, so an open editor's cursor snapped to the document end on every image swap, exactly the #152 jump that the main write path no longer causes. Switch it to the same `applyDocToFragment(ydoc, newDoc)` structural diff (updateYFragment) as the main path, so unchanged nodes keep their ids and the live cursor stays put. It runs its own atomic transact, so the old explicit transact/delete is gone; the now-unused docmostExtensions import is dropped. Regression tests (cursor-stability suite): a sibling paragraph's RelativePosition survives a top-level image src/attachmentId swap, and an image nested in a callout, matching the shapes replaceImage produces. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
0e8af13122 |
test(footnotes): cover footnoteWarnings import plumbing + doc fixes (#169 second review)
Follow-up to the merged #166/#169. Addresses the second review pass (comment 1227): - footnoteWarnings plumbing: extract a single `footnoteWarningsField(markdown)` helper (footnote-analyze) and use it at all three call sites (create_page, update_page, import_page_markdown) so the field is attached identically. - New unit test footnote-warnings-import.test.mjs pins the contract that was uncovered: the field is present on problems / omitted on clean input, and the IMPORT path analyzes the BODY after the docmost:meta / docmost:comments blocks (a footnote-like token inside those JSON blocks must NOT warn; a real body marker must). Tested via the same pure composition the importer uses (footnoteWarningsField(parseDocmostMarkdown(full).body)) — no collab socket needed; a regression that analyzed fullMarkdown or skipped the body split would now go red. - footnote.marked.ts: correct the stale module header — it claimed "only definitions that have a matching reference are emitted", which was never true (orphan defs are emitted; the editor sync plugin reconciles). Now describes first-wins + reuse + sync reconciliation. - derive-id golden test: rename the describe from "(cross-package drift guard)" to "(deterministic-scheme pin)" — there is no second package to drift against. editor-ext 129, MCP 304 (+3), client+server tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
a0cc625dfe |
refactor(footnotes): address PR #169 review
- footnote-sync: remove the now-dead `refReids` (CollisionPlan field, local, return, the 6a consumer loop) — references are never re-id'd under reuse, so it was dead structure on the hot reconciliation path. Rewrite the stale comments (plugin header, step 0, refOccurrences field) that still described the old "duplicates re-id'd so both survive" model to the reuse model. - Shared footnote lexer: new packages/mcp/src/lib/footnote-lex.ts (lexFootnoteLines + forEachFootnoteReference). extractFootnotes (collaboration) and analyzeFootnotes now consume the SAME fence-aware lexer, so "the analyzer sees exactly what the importer keeps/strips" is structural, not comment-kept. Removed the duplicated DEF_RE/fence machine from both consumers. - Tests: new mock test for the footnoteWarnings plumbing on createPage (problems -> field present; clean -> omitted); new paste-reuse case for TWO colliding pasted definitions (reservation -> distinct ids). Updated the derive-id golden test header (no MCP copy / parity test anymore). - CHANGELOG: [Unreleased] entries for footnote reuse (Changed, supersedes 0.93.0) and footnoteWarnings (Added). editor-ext 129, MCP 301, server roundtrip 2; client+server tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
17e683a311 |
feat(footnotes): reuse semantics + import diagnostics (#166)
Footnotes were strict 1:1: a repeated `[^a]` reference was treated as a collision and re-id'd to `a__2`, and a reference with no definition synthesized its own empty one — so an agent-authored article with reused labels produced dozens of empty `kowiki__N` footnotes. Move to Pandoc REUSE semantics and add non-fatal import diagnostics. Reuse (core): - resolveCollisions (footnote-sync): repeated references sharing an id are REUSE (recorded once in document order, never re-id'd) — one number, one shared definition. Only a duplicate DEFINITION is re-id'd deterministically and, with no matching reference, dropped by the existing orphan policy (first-wins). CollisionPlan.refReids is now always empty (harmless no-op downstream). - extractFootnoteDefinitions (marked) and extractFootnotes (MCP): duplicate definition ids are FIRST-WINS (keep first, drop rest); reference markers are never rewritten. Removed the marker-rewriting and the now-dead deriveFootnoteId mirror + helpers from the MCP path. Import diagnostics: - New analyzeFootnotes() (MCP): fence-aware pure scan reporting dangling references, empty/duplicate definitions and `[^id]` markers inside table rows. - createPage / updatePage / importPageMarkdown now attach `footnoteWarnings` (only when non-empty) so an agent can fix its markup; the page is still created. Paste-reuse: - footnotePastePlugin remaps only ids the pasted slice DEFINES (a colliding definition); a pasted lone reference to an existing id keeps it (reuse). Tests: reuse/first-wins rewrites of footnote.test, footnote-markdown.test, footnote.marked.orphan.test and the MCP footnotes.test; new footnote-paste.test (editor-ext) and footnote-analyze.test (MCP). Deleted derive-id-parity.test.mjs (the MCP no longer derives ids; editor-ext's deriveFootnoteId keeps its own golden test). editor-ext 128, MCP 299, server roundtrip 2, client views 3, client+server tsc clean. Two review suggestions applied: corrected a stale "duplicated in MCP" comment and the dangling-reference warning wording. Note: the multi-backlink editor UI (a reused definition linking back to each of its references) is deferred to a follow-up — this PR delivers the data-integrity core (reuse + warnings + paste-reuse). Forward links and numbering already reuse correctly; the backlink currently targets the first reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
aca075108c |
refactor(mcp): accurate encode-failure labels + diff edge-case tests (#154 review)
Addresses the approve-with-comments review on PR #154: - applyDocToFragment: hydrate PMNode.fromJSON in its OWN try so a hydration failure (e.g. an unknown node type) is labelled "fromJSON" — the stage that actually threw — instead of the misleading "updateYFragment". The diagnostic comment on unstorableYjsError ("label names the stage that failed") is now truthful. - assertYjsEncodable: also rehearse PMNode.fromJSON(docmostSchema, …) so a doc that would only fail in apply's hydration step is rejected at preview time too, narrowing the preview/apply gap (review suggestion B). Still cheap — no live fragment, no updateYFragment. - Tests: relabel the diagnostic test to (fromJSON); add structural-diff edge cases — neighbour deletion keeps the unchanged node's cursor anchor, doc->empty clears the fragment without throwing, top-level node-type change diffs in place — plus a preview-gate test for the new fromJSON rehearsal. 297/297 green. build/ rebuilt for the changed lib module only (build/client.js left untouched to avoid pulling in pre-existing unrelated src/build drift). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
c7c0c28e38 |
refactor(mcp): single docmostSchema + shared encode-error helper + catch test (#152 review)
Review of #154 (Request changes) — all clean follow-ups, no defect in the fix: 1. Single source of the ProseMirror schema: export `docmostSchema` from docmost-schema.ts (next to docmostExtensions); diff.ts and collaboration.ts import it instead of each calling getSchema(docmostExtensions) — the schema can no longer drift between call sites. Removed both local builds + the now unused getSchema imports. 2. Doc fix: assertYjsEncodable's docstring and the client.ts comment no longer claim "the same encoder as apply" — apply uses updateYFragment, the dry-run uses toYdoc; both reject the same unstorable attrs but are NOT byte-identical. Reworded to "independent encodability gate". 3+4+5. Extracted `unstorableYjsError(safe, label, e)` — buildYDoc and applyDocToFragment now share one message template (label kept for diagnostics: toYdoc vs updateYFragment), so the wording can't drift between dry-run/apply. 6. Test for applyDocToFragment's catch branch: an unknown node type makes the schema-validated PMNode.fromJSON throw, and the function must re-throw it wrapped with the (updateYFragment) diagnostic. build/ rebuilt for the three changed lib modules; 293 package tests green. (Left build/client.js untouched: rebuilding it would pull in a pre-existing, unrelated src/build drift — a listSidebarPages slugId fix never rebuilt on develop — and my client.ts change there is comment-only.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |