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>
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>
stashPage is declared in the server's DocmostClientLike interface and
shipped as the stash_page MCP tool (client.ts, tool-specs.ts, index.ts),
but the hand-maintained HOST_CONTRACT_METHODS mirror in the contract test
was never updated — so the drift-guard test failed and broke CI's
unit-test job. Add the missing name; both directions now agree.
Merging the image-captions (#221) and lossless-export branches each added
its own escapeHtmlAttr in turndown.utils.ts, producing two implementations
of the same function and breaking `tsc --build` (TS2393) — which failed the
Build editor-ext step across all CI jobs.
Drop the lighter image-captions duplicate (escapes & and ") and keep the
fuller version (escapes & " < >). It is a strict superset: both call sites
(serializeAttrs, the image rule) place the value inside a double-quoted HTML
attribute, where extra < > escaping is harmless and idempotent on re-import.
Verified: editor-ext builds; turndown.dataloss + image-markdown tests pass.
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>
The special-chars test only checked substrings (data-caption=/Tom/Jerry) that
survive even if escapeHtmlAttr stopped escaping " or double-encoded &. Assert
the exact escaped attribute in the intermediate Markdown
(data-caption="Tom & "Jerry"") and re-parse the rendered HTML to
confirm the recovered caption is exactly Tom & "Jerry".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an inline spoiler (Telegram/Discord-style hidden text): a TipTap mark
`spoiler` rendered as <span data-spoiler="true" class="spoiler">, blurred via
CSS and revealed on click (UI-only is-revealed class, never persisted).
- packages/editor-ext: the Spoiler mark (inclusive:false, set/toggle/unset
commands, ||text|| input rule), exported; a lossless turndown rule emitting
raw inline HTML; round-trip test.
- apps/client: SpoilerView mark-view (ReactMarkViewRenderer, Link pattern),
registration in extensions, bubble-menu toggle button (editable only), CSS
(blur + @media print reveal), en/ru i18n.
- apps/server: register Spoiler in collaboration.util tiptapExtensions so the
mark survives HTML<->JSON export/index/import/Yjs; a test proving the public
share keeps the spoiler (it isn't stripped with comments).
No keyboard shortcut: the proposed Mod-Shift-s collides with Strike (and
Mod-Shift-h with Highlight); the ||text|| input rule + the bubble-menu button
cover ergonomics.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment claimed vitest skips the file because it has no test cases; vitest
collects by filename glob, so the real reason is the name not matching
*.{test,spec}.ts. Reword to cite the glob and warn that adding test cases here
would not run them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The schema + cell/row/table/doc builders + grid/stateFor/trFor were copied
verbatim into the 3 new table-utils test files (and the pre-existing
table-utils.test.ts) — a schema change would have to be synced across all four.
Move them into a shared table-test-helpers.ts (test-only, excluded from the
build like footnote-corpus.ts) and import it everywhere; cell uses the
(txt, attrs?) superset (a drop-in for the bare (txt) copies). No assertion
changes — test counts unchanged (223 passed + 3 expected-fail).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
uploadImage is internal to client.ts (called by insertImage/replaceImage);
the MCP transport (index.ts) does not call it directly. Remove it from the
comment's list of transport-called methods.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
The DocmostClientLike mirror covers only methods the in-app adapter consumes;
the standalone MCP transport calls additional client methods not tracked here
(covered by its own typecheck). Fixes the misleading 'superset' wording (F2).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A captioned image in a column is emitted via the imageToHtml helper, a
separate path from the top-level image case whose data-caption branch was
untested. Add a round-trip test with special chars (Tom & "Jerry") that
fails if the imageToHtml caption branch breaks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
The setImageCaption command and its Commands<> declaration were dead:
captions are written via the generic updateAttributes in
useImageTextFieldControl, and a repo-wide grep finds zero callers.
Remove the speculative implementation (image.ts) and its type
declaration.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The escaping round-trip test's data (A & "B") only contained & and ",
so the <,> branches of escapeHtmlAttr (&,",<,>) and escapeHtmlText (&,<,>)
were never exercised; a regression dropping <,> escaping would still pass.
Extend the data to A & <B> "C" in both the data-label attribute and the
visible text so both functions' <,> branches are genuinely covered. Assert
the well-formed escaped tag (attr: A & <B> "C", text:
A & <B> "C"), explicitly reject the raw tag-corrupting forms,
and confirm markdownToHtml restores the originals. Comment updated to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Architect-review hardening of the bidirectional DocmostClientLike <->
HOST_CONTRACT_METHODS guard (test-only, no production change):
- Interface method-name regex now accepts full TS identifiers
(digits/_/$) and generic signatures (method<T>(), avoiding a future
benign false-FAIL.
- Skip /* ... */ block comments in the interface body so a `name(` line
inside one is not falsely parsed as a method.
- Wrap the cross-package readFileSync with a clear "expected monorepo
layout" error instead of a bare ENOENT when run outside the monorepo.
- Narrow the guard's comments/error to state plainly it checks the
method-NAME set only; signature parity remains the deferred staged-plan
item.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review F2. The escapeHtmlAttr (&,",<,>) and escapeHtmlText (&,<,>)
helpers in turndown.utils were untested — every existing round-trip case used
alphanumeric values, so no escape branch ran. A mention/status carrying HTML
special chars would re-emit malformed HTML that import's parseHTML can't
restore → the same data loss this PR fixes, uncaught.
Add a round-trip case to turndown.dataloss.test.ts: a mention with `&` and `"`
in both data-label and visible text. Assert (a) the exported Markdown carries
the correctly-escaped, well-formed tag (data-label="A & "B"",
text escapes &), not the raw malformed form; and (b) markdownToHtml restores
the original unescaped values (attribute `A & "B"`, text `@A & "B"`).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add PM -> markdown -> PM round-trip assertions for image caption
(plain and special-char), which fail without F1 and pass with it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
The contract test only checked one direction (each name in
HOST_CONTRACT_METHODS exists on the real DocmostClient). But
HOST_CONTRACT_METHODS is itself a hand-copy of the server's
DocmostClientLike interface (docmost-client.loader.ts), and that
list<->interface link was untested: a method added to the interface +
consumed by the adapter but forgotten in the list (or removed from the
interface but left in the list) would escape both the server typecheck
(the pkg emits no .d.ts) and the existing test (name not in the list) ->
a runtime "x is not a function" in a tool call.
Parse the method names from the DocmostClientLike interface body (read
the .ts source via import.meta.url, scan member-signature lines) and
assert.deepEqual them against HOST_CONTRACT_METHODS BOTH ways. Lists are
currently identical (39=39), so this is a coverage hole closed, not a
live bug.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
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>
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>
Issue #193's tool-half has two open items. The shared, zod-agnostic tool-spec
registry (SHARED_TOOL_SPECS) for the identical tools is already merged
(f3fa15e7) and consumed by both layers, so that subset is done. The remaining
items are: (a) deriving the layer-3 hand-mirror `DocmostClientLike` from the
real client type, and (b) folding more tools into the registry. Both were
deferred as risky, and that deferral still holds (verified, see below) — so
this change ships the safest concrete increment instead of forcing the risk.
What this adds (behaviour-neutral, test-only + a doc comment):
- packages/mcp/test/unit/client-host-contract.test.mjs: pins the layer-3
contract from the ESM side, where the real DocmostClient is importable. It
asserts every method the in-app `DocmostClientLike` mirror declares exists as
a function on a real DocmostClient instance (constructor is side-effect-free).
A rename/removal in client.ts now fails this test instead of silently shipping
a runtime "x is not a function" into an agent tool call. Negative-case
verified (a bogus method name is detected).
- docmost-client.loader.ts: replaces the vague mirror comment with a pointer to
the guard test and a concrete, empirically-grounded staged plan for the full
type-derivation. Verified blockers kept it deferred: @docmost/mcp emits no
.d.ts (no `declaration`, no `types` export) and the server has no path mapping
for it, so there is no type to import today; and the real methods' inferred
CONCRETE return types conflict with the in-app adapter's loose
Record<string,unknown> + `as`-cast result handling (deriving the exact type
breaks the build / forces pervasive double-casts and full-surface test stubs).
Out of scope (noted in the issue): the PM<->Markdown converter unification.
Verified: server tsc clean; mcp tsc clean; mcp tests 369 pass (367 + 2 new);
ai-chat tools specs 51 pass. No behaviour change; committed mcp build untouched
(no mcp src changed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mdrt-2 (markdown export): add lossless turndown rules for the custom nodes
that had no rule — transclusionReference, pageBreak, mention, status. Each
re-emits the node as inert raw HTML carrying every data-* attribute instead
of being silently dropped (childless atom divs) or collapsed to bare text
(mention/status losing data-id/data-color). Empty atom blocks are made
non-blank before turndown's blank-rule strips them (mirrors the footnote-ref
fix). markdownToHtml passes the raw HTML through and each node's parseHTML
rebuilds it, so the form round-trips. Flips the it.fails cases to passing and
adds export + import round-trip coverage.
persist-6 (collab store): add a store-side empty-guard in onStoreDocument.
Before updatePage, if the serialized live doc is an empty paragraph doc AND
the persisted page is non-empty, skip the write and log — unless an explicit
context.intentionalClear signal is present (deliberate select-all+delete).
New/empty pages and unchanged docs are unaffected. Flips the it.failing case
to passing and adds escape-hatch + empty-over-empty coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- docs: add CHANGELOG Unreleased/Added entry for editable image captions
- test: export sanitizeCaption and add vitest unit coverage
(whitespace collapse, trim, 500-char boundary)
- refactor: drop duplicate .imageCaption CSS module class, keep the
global .image-caption as the single source
- docs: fix turndown image-caption comment (video rule emits a markdown
link, not a <div>)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
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>
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>
Approve-with-comments follow-ups (no blockers):
- callout: unify the GitHub-callout feature ticket on #192 (the callout-paste
feature the CHANGELOG already tracks); #218 is the public-share security work.
Fixed the code comment and test reference.
- export/utils.spec: pin current behavior of a leading-dot name (".gitignore" ->
"") — same bug class as #204 but unreachable via the sole caller, so document
not change.
- share.types: narrow ISharedPage to the actual /shares/page-info allowlist
(page -> Pick of id/slugId/title/icon/content; trimmed share; dropped the
spurious `extends IShare`). Verified all three consumers (shared-page,
link-view, mention-view) read only allowlist fields.
- editor-ext: extract shared CALLOUT_TYPES / normalizeCalloutType /
renderCalloutHtml into callout-common.marked.ts; both tokenizers
(`:::type` and `> [!type]`) now share the renderer + type dict while staying
separate. Eliminates the byte-identical renderer + duplicated type list.
- share.service: extract named predicate shareIdGrantsAccess(requestedShareId,
resolvedShare) for the id-or-key fast path (naming only, no control-flow
change); kept narrower than resolveReadableSharePage's id-only gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
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>
Additive test coverage across server, editor-ext, client and mcp.
#192 — AiChatService.stream integration (Section 3, against real Postgres):
- new apps/server/test/integration/ai-chat-stream.int-spec.ts drives the real
streamText through a seeded ai/test MockLanguageModelV3 and a real Node
ServerResponse, covering: onError persists an assistant error record
(status 'error' + partial answer + provider cause in metadata); external MCP
client closed exactly once on BOTH onFinish and onError; anti-tamper —
history is rebuilt from the DB transcript, not from body.messages.
#206 — red-team findings (most already fixed+tested in #212):
- mdrt-2 (UNFIXED, data loss): turndown.dataloss.test.ts documents that
pageBreak / transclusionReference / mention are silently dropped on Markdown
export (characterization + it.fails for the desired survive-export contract).
- persist-6 (UNFIXED, data loss): persistence-store.spec.ts adds an it.failing
documenting that a momentarily-empty live doc overwrites non-empty content
(left unfixed — a store-side empty-guard is a behaviour change).
#204 — test-strategy plan, highest-priority subset:
- Phase 1: mcp-clients.lease.spec.ts covers the external MCP client
lease/refcount/eviction lifecycle (leak / premature-close / double-close).
- Phase 2 data-integrity pure functions: editor-ext table-utils
(transpose/moveRow/convert round-trip) and math tokenizer false-positive
guard; client emoji-menu (+ it.fails for the unguarded localStorage
JSON.parse bug), sort-cells, normalizeTableColumnWidths; mcp htmlEmbed/
pageBreak markdown data-loss + footnote-diff; server export
getInternalLinkPageName extensionless-path bug — FIXED (small/clear) + tested.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Public sharing (#218):
- Bind public-share content to the requested shareId. getSharedPage now
enforces dto.shareId (forwarded from /share/:shareId/p/:slug): the page must
be reachable THROUGH that exact share (its own share, or an includeSubPages
ancestor that contains it). A forged/mismatched shareId 404s instead of
rendering off the slug alone and no longer leaks the real canonical key via
redirect. A request with no shareId keeps the legacy slug-capability path.
- Trim /shares/page-info: drop internal metadata (creatorId, spaceId,
workspaceId, contributorIds, lastUpdated*, parent/position, lock/template
flags, timestamps) from the anonymous payload.
- Default share-to-web includeSubPages to false (opt-in), so enabling a share
no longer silently exposes the whole sub-tree (#216).
Editor (#218):
- Harden the new-page pre-sync window: the body editor is kept read-only until
the collab provider is Connected and synced, so early keystrokes can't land
only in local ProseMirror and then be clobbered by the server's empty doc.
- Surface a "Connecting… (read-only)" affordance during the static phase so
input isn't silently swallowed.
Other:
- Breadcrumb: resolve from the page's own ancestor data (/pages/breadcrumbs)
instead of waiting for the lazily-built sidebar tree, so deep pages don't
render a blank breadcrumb for seconds.
- Pasting GitHub `> [!type]` callouts now converts to a callout node instead of
a literal blockquote (new marked extension wired into markdownToHtml).
Tests: editor-sync-state gate (client), getSharedPage share-binding (server),
github-callout markdown conversion (editor-ext).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With the image fix in place, the mcp e2e ran through every section and
failed only at the last one (comments): create_comment was hardened to
require an inline "selection" (exact text to anchor on) for a top-level
comment, but the test created one without a selection ("an inline
'selection' ... is required for a top-level comment").
Pass an inline selection ("Добавленный абзац.", a plain paragraph
re-imported in section 5 and still present at the comments stage). The
reply is unchanged: it carries a parentCommentId, so it is a reply and
needs no selection.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to the first e2e fix: with nanoid/editRes.edits resolved, the
suites failed one layer deeper. Both layers were never green since the
e2e jobs were added (non-blocking in CI), so the failures had stacked up.
server e2e (jest-e2e.json) — align module resolution/transform with the
working unit/integration jest configs so AppModule's full import graph
loads:
- moduleFileExtensions: add "tsx" (React-Email .tsx templates are pulled
in via the auth controller chain).
- transform: ^.+\.(t|j)s$ -> ^.+\.(t|j)sx?$ so .tsx is transformed.
- moduleNameMapper: add ^src/(.*)$ -> <rootDir>/../src/$1 (code imports
via the absolute 'src/...' alias). Verified locally: the module graph
now fully resolves (only env vars, supplied by CI, remain).
mcp e2e (test-e2e.mjs) — insert_image/replace_image accept only http(s)
URLs the server fetches; the test passed local file paths and died with
"Invalid image URL". Serve the PNG bytes over a throwaway 127.0.0.1 HTTP
server (the Docmost server runs on the same CI host) and pass URLs. The
featPng negative test is untouched: replaceImage checks the attachmentId
and throws before fetching, so its local path is never validated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two unrelated CI failures on the 0.94.0 release PR:
- server e2e: jest-e2e.json lacked transformIgnorePatterns, so the
ESM-only nanoid@5 package was loaded as CommonJS and crashed with
"Cannot use import statement outside a module". Add the same
node_modules whitelist already present in the unit and integration
jest configs (nanoid|uuid|image-dimensions|marked|happy-dom|lib0).
- mcp e2e: test-e2e.mjs read editRes.edits, but editPageText() returns
the per-edit results under `applied` (not `edits`), so editRes.edits
was undefined and .every() threw. Read editRes.applied instead.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A colliding transclusionSource id is deliberately NOT reassigned (its id is a
cross-reference key), while a missing id is still filled. Add coverage for both:
two sources sharing an id keep it (red if the NO_REASSIGN guard is removed), and
a source with no id gets a fresh one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
claude code agent 227
vvzvlad
a
