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>
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>
- 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>
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>
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>
Server side of agent comment suggestions.
- CreateCommentDto gains optional suggestedText (<=2000). CommentService.create
accepts it ONLY for a top-level inline comment with a non-empty selection,
requires it be non-empty and differ from selection (else BadRequest), and
stores it.
- POST /comments/apply-suggestion (ApplySuggestionDto { commentId }): authorizes
with validateCanEdit (applying edits page text) BEFORE any structural check or
mutation, then CommentService.applySuggestion:
- runs the phase-3 collab event applyCommentSuggestion on `page.<pageId>` to
atomically check-and-replace the marked text, returning { applied, currentText };
- applied → stamp suggestion_applied_at/by, auto-resolve the thread, ws
commentUpdated, audit COMMENT_SUGGESTION_APPLIED;
- already-applied (DB) → idempotent success (no re-apply), self-healing the
resolve if it was missed — satisfies the issue's double-click / two-user
race requirement;
- collab verdict applied:false && currentText===suggestedText → idempotent
success (crash between doc mutation and DB write);
- text changed → 409 ConflictException carrying currentText;
- gateway undefined/throw → hard error, never a silent success.
- audit-events: COMMENT_SUGGESTION_APPLIED.
Tests: create validation (reply/no-selection/equal-to-selection rejected;
valid stored) + applySuggestion verdict branches incl. both idempotent paths.
jest src/core/comment: 33 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New custom collab event applyCommentSuggestion runs replaceYjsMarkedText inside
the document's Yjs transaction on the owning instance and returns the
{ applied, currentText } verdict to the API-server caller (cross-process via the
Redis bridge, whose customEventComplete/replyId already carries handler return
values).
- withYdocConnection is now generic and returns the callback's result (captured
in a closure, since hocuspocus connection.transact does not forward it). The
callback is typed synchronous-only: transact runs fn synchronously without
awaiting, so an async fn would mutate outside the transaction and lose
atomicity.
- collaboration.gateway.handleYjsEvent: when Redis is disabled
(COLLAB_DISABLE_REDIS), dispatch the handler locally against the single
hocuspocus instance and return its verdict instead of silently returning
undefined (which would make apply a no-op). Also fixes the pre-existing silent
no-op of setCommentMark/resolveCommentMark without Redis.
Tests: handler spec (applied mutates doc + returns verdict; changed-text returns
{applied:false} without mutating; args forwarded; withYdocConnection returns the
value) and gateway spec (no-Redis path dispatches locally, returns the verdict,
not undefined).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The primitive behind "Apply comment suggestion": walk the XmlFragment, collect
the delta segments carrying the `comment` mark for a commentId, and replace them
with new text ONLY if the run is intact (single Y.XmlText, contiguous, and the
joined text still equals the expected anchor). Otherwise return a verdict
{ applied:false, currentText } — null when the anchor is gone, else the current
text — so the caller can report "someone changed it". On apply it deletes the
run and re-inserts the new text re-attaching the same comment mark (thread stays
anchored). Mutates in place for the caller's connection.transact(); opens no
transaction of its own.
Non-string inserts (embeds) advance the offset by their 1-unit index length so a
marked segment after an embed gets the right position and an embed inside a run
is correctly rejected as a changed anchor.
Tests (yjs.util.spec.ts): happy path (mark preserved, surrounding text and no
mark-bleed), resolved-mark match, changed text, deleted anchor, paragraph split,
interleaved unmarked text, and embed before/inside the run. 17 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add suggested_text / suggestion_applied_at / suggestion_applied_by_id to the
comments table (migration) and mirror them in the hand-curated db.d.ts Comments
interface. suggested_text holds a proposed replacement for the comment's
anchored selection; the applied_* columns record who applied it and when.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In prod the AI provider resets the connection pre-response (ECONNRESET); the
#175 pre-response retry recovers it, but 2 of the 3 allowed attempts were burned
in a single turn — no headroom, and one more reset would surface an error to the
user. This is tuning for resilience (not a diagnosis of who resets):
- Retry budget 2 → 4 (total 5 attempts), env-configurable via
AI_STREAM_PRE_RESPONSE_RETRIES (0 = no retry; empty/invalid → default 4).
- Backoff: linear 150*(attempt+1) → capped exponential + full jitter
(preResponseBackoffMs, a pure injectable helper): base 150ms, ×2 per attempt,
capped 2000ms, delay = random in [0, capped]. Avoids a synchronized retry
storm and spreads reconnects across the reset window.
- Keep-alive default 10_000 → 4_000 ms so undici recycles idle sockets before a
~5s upstream/middlebox idle cutoff can poison them (a common pre-response
reset cause). Still env-overridable via AI_STREAM_KEEPALIVE_MS.
- .env.example documents both knobs.
Timeout (900s), RETRYABLE_CONNECT_CODES, and the instrumentation are unchanged.
refs #310
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
POST /api/ai-chat/bound-chat 500'd with Postgres 22P02 because the client
sends a page slugId (10-char nanoid) in the request `pageId` field, which the
server passed straight into the UUID `page_id` column. The chat-to-document
binding silently broke (client fail-softs to a new chat) and every slug-URL
page open logged a 500.
Fix: resolve the incoming id to a real page UUID on the server. PageRepo.findById
already accepts both a uuid and a slugId (isValidUUID→slugId fallback), so
boundChat now resolves the page first, guards it against a foreign/unknown
workspace (returns {chatId:null} before any chat lookup — no cross-workspace
probe), and looks up the latest chat by the resolved page.id (real uuid).
Client: renamed the local pageId→slugId for clarity (the value is a slugId);
the wire body key stays `pageId` so the DTO is unchanged. DTO left @IsString()
(a @IsUUID() would only turn the 500 into a 400 and still break binding).
Test: bound-chat spec asserts a slugId resolves and findLatestByPage is called
with the real uuid; a foreign-workspace page → {chatId:null} without a chat
lookup (no leak); an unknown id → {chatId:null}, no throw.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1: remove an accidentally-committed self-referential symlink
packages/mcp/node_modules/node_modules -> an absolute build-machine path (leaked a dev
home path, a pnpm artifact useless in the repo), and add a targeted ignore so it can't
recommit.
F2: the commentUpdated broadcast re-emitted the caller's pre-loaded comment mutated in
place, so the {agent,launcher} stack survived only because the controller happened to
load it with includeCreator:true — the fragile coupling that let the stack vanish on
edit once already. update() now RE-FETCHES the enriched comment before broadcasting,
symmetric with create()/resolveComment() (the row is already persisted), so all three
broadcasts carry the stack regardless of any caller's pre-load. Adds a caller-contract
test asserting all three broadcasts emit agent/launcher for an agent comment and neither
for a non-agent one, spotlighting the update path (non-vacuous vs the old re-emit).
F3: add a direct test of the page-history attachPageHistoryAgent mapping (its distinct
lastUpdatedSource/lastUpdatedAiChatId/lastUpdatedBy column set): role / no-role / MCP /
non-agent, and that the internal agentRole join column is stripped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
For AI-agent-authored content (comments + page history), replace the text AI-AGENT
badge with an avatar stack: the agent in front, the human who launched it smaller and
behind. This fixes the inverted hierarchy (the action was the agent's; the human just
launched it). closes#300.
Backend: a single server-authoritative resolver resolveAgentProvenance normalizes to
{ agent, launcher } from server columns only (createdSource/lastUpdatedSource, aiChatId,
creator, chat role) — nothing from request input, so agent identity can't be spoofed.
Internal chat -> agent = chat role (name/emoji), launcher = human; external MCP
(aiChatId null) -> agent = the agent account, launcher = null; non-agent -> neither.
The role join (aiChatId -> ai_chats.role_id -> ai_agent_roles) deliberately does NOT
filter enabled/deleted_at, so a later-disabled role still labels historical content
(mirrors findById, not findLiveEnabled). Enrichment is applied on BOTH findPageComments
(list) AND findById (the create/resolve/update broadcast path), so the stack shows on
live comment events and doesn't vanish on resolve/edit.
Frontend: new AgentAvatarStack + AgentGlyph (avatarUrl -> role emoji on violet ->
IconSparkles on violet), integrated into comment-list-item and history-item where the
badge was; the deep-link-to-chat click moved onto the stack. ai-agent-badge removed.
Tests: AgentAvatarStack (role/no-role/MCP/click/non-clickable), the provenance resolver
+ recorder tests proving the role join never filters enabled/deleted, and findById
enrichment (guards the live-broadcast regression).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Page/space export (Markdown & HTML, both via jsonToHtml -> generateHTML) crashed with
"Export failed:undefined" on any page carrying a `comment` mark. Root cause:
comment.renderHTML returned a LIVE DOM node (document.createElement + a click listener)
whenever a global `document` existed — and the in-process MCP module injects a jsdom
global.window+global.document into the Node server, defeating the old
`typeof document === "undefined"` guard. The server export runs happy-dom's
DOMSerializer, which crashes appending the foreign jsdom node
(NodeUtility.isInclusiveAncestor -> "Cannot read properties of undefined (reading
'length')"). comment is the only extension returning a live node.
Fix: widen the guard with an isNodeRuntime check (process.versions.node) so on any Node
runtime renderHTML returns the plain, serializable spec array — even when MCP injected
jsdom globals. The browser branch (createElement + click -> ACTIVE_COMMENT_EVENT) is
untouched, so in-editor comment interactivity is preserved (Vite defines only
process.env as a member-expression substitution, no `process` object in the browser
bundle, so isNodeRuntime is false there). The mcp schema mirror already returns a spec
array and is not on the export path (tiptapExtensions imports Comment from
@docmost/editor-ext), so no mirror change is needed.
Also: export-modal now reads the real error text from the response Blob
(responseType:'blob' made err.response.data.message always undefined) so a failed export
shows the server's message instead of "undefined".
Adds a regression test that runs the real jsonToHtml on a comment-marked doc with
jsdom globals injected (reproduces the crash on the unpatched code, passes after).
closes#298
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1: pc.title (untrusted cross-user page title) was interpolated raw into the
markdown export heading. Reusing escapeAttr alone (the prompt sink's XML-attribute
sanitizer, strips < > ") is insufficient here because the sink is MARKDOWN: link
/image syntax survives, so a title like  or [phish](http://evil)
injects a remote image / clickable link into the downloaded .md disguised as a
trusted system annotation. Add markdownHeadingSafe() = escapeAttr() + backslash-
escape [ and ] (disables both [text](url) and ; a bare (url) is inert).
F2: cover the title branch — a title that collapses to empty via escapeAttr falls
to the bare heading (no ("")), and a link/image-injection title is neutralized
(non-vacuous vs the escapeAttr-only version).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #274 page_changed marker lived only in the ephemeral system prompt, so the
diff the agent saw was invisible in the chat export/history, and the note was
too weak — the agent still overwrote the user's manual edits with a full-page
replace.
- Persist the diff the agent saw as metadata.pageChanged on the assistant row
(flushAssistant), threaded into all five flush call sites in stream(). Model
replay (rowToUiMessage/rowParts) reads only metadata.parts, so the sibling
never re-injects the note into the model context on later turns.
- Render the persisted diff as a labelled block (en/ru) before the message body
in the server-side Markdown export (chat-markdown.util.ts).
- Strengthen PAGE_CHANGED_NOTE: mandate a fresh getPage re-read and targeted
edits (editPageText/patchNode/insertNode/deleteNode) instead of a whole-page
replace, and never revert or overwrite the user's edits.
Tests: prompt, export and service specs updated; 114 pass, tsc clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
F1: escape the collaborative page title before interpolating into
<page_changed page="..."> (and the pre-existing openedPage attr) — strip
<>" and collapse whitespace, so a crafted title can't break out of the
attribute into the system prompt (cross-user injection).
F2: neutralize <page_changed>/</page_changed> occurrences inside the diff body
so a crafted line can't close the block early.
F3: remove the dead content_hash column (written every turn, never read) —
migration, repo, service hashing + crypto import, db.d.ts, spec asserts.
F4: test the best-effort catch branches (detectPageChange / snapshotOpenPage
swallow errors and don't break the turn).
F5: soften the overstated 'diff cannot smuggle instructions' comment to
defense-in-depth framing referencing the F1/F2 mitigations + safety sandwich.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The agent rebuilds context from DB each turn and didn't know the user manually
edited the open page since its last response, so it could overwrite those edits.
Add a per-turn ephemeral <page_changed> note in the system prompt (twin of
INTERRUPT_NOTE, self-clearing) carrying a unified Markdown diff of what changed
since the END of the agent's previous turn.
- New ai_chat_page_snapshots table (migration + hand-declared db.d.ts/entity
types) storing the page Markdown per (chat,page) at each turn's end.
- Pure computePageChange util (whitespace-normalized unified diff via the
existing jsdiff dep, 6KB cap + getPage hint).
- Turn start: if the open page's updatedAt moved past the snapshot, diff current
vs snapshot; non-empty -> PAGE_CHANGED_NOTE in the safety sandwich.
- Turn end: upsert the snapshot on EVERY terminal path (onFinish/onError/onAbort,
once) so the agent's own edits are excluded by construction even on aborted
turns.
All best-effort (never breaks/latency-regresses a turn); fast path when updatedAt
is unchanged. Server-only.
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>
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>
F17: the header's content-clause literal omitted the [[:space:]]* tolerance;
copy page.repo.ts's exact '"type"[[:space:]]*:[[:space:]]*"text"' (jsonb::text
renders a space after the colon, which is why the tolerance exists).
F16: remove expect(ttl).toBeGreaterThan(0) — the twin of the F15 removal;
expect(ttl).toBe(120) strictly subsumes it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F14: the lockstep int-spec header still described the pre-F6 two-clause set with
'iff' — add the content-JSON text-node clause so it matches embeddablePredicate.
F15: remove the redundant expect(ttl).toBeLessThanOrEqual(120) that followed
expect(ttl).toBe(120).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F11: PRE_SEED_TTL_SECONDS 45->120 (= client REINDEX_POLL_CAP_MS). At concurrency
1 a queued reindex can wait past the old 45s; if the pre-seed expired while
pending, getMasked fell back to the COUNT and reported done, so the client
stopped polling and missed the climb. Tie the pre-seed TTL to the client cap.
F12: extend the lockstep integration spec — insertPage takes content; a
text_content=null + text-node-content page is IN and a math-only page is OUT,
pinning the structural "type":"text" clause (and the jsonb space-after-colon).
F13: list all three embeddable clauses in the reindex JSDoc/inline comments.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F6: extend embeddablePredicate to pages with body content but null text_content,
keyed on the text-node marker "type":"text" (not a bare "text": key, which
also matched math nodes' attrs.text and would leave math-only pages stuck
below 100%). Numerator and denominator share the predicate; tests assert the
compiled WHERE is byte-identical and a math-only doc is excluded.
F7: correct the start() JSDoc (both totals are the real page count).
F8: nextReindexPollInterval reuses isReindexComplete.
F9: getMasked reads progress first and skips the two COUNTs while a reindex is active.
F10: pre-seed the progress entry with a short 45s TTL so a deduped enqueue's
phantom "0 of N" expires quickly instead of sticking for the 1h TTL.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
guardedFetch blocks loopback/private/link-local/metadata IPs and never calls
fetch; decryptHeaders fails open (returns undefined, warns once, no blob leak).
yjs.util setYjsMark/removeYjsMarkByAttribute/updateYjsMarkAttribute on real
Y.Docs. SHARED_TOOL_SPECS<->in-app parity (name/desc/input-schema; a dropped or
renamed wiring fails). Replace the tautological storage.service spec with
driver-delegation checks across every public method.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The pub/sub error handlers were `(err) => () => {}` — a noop returning an
inner arrow that never runs, so socket.io redis client errors were silently
swallowed. Log them via Nest Logger. Adjacent pre-existing bug surfaced in
review of #255.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The WsRedisIoAdapter creates two ioredis clients (pubClient/subClient) for
@socket.io/redis-adapter but never closed them, leaking their TCP handles on
application shutdown (#255). The redis-adapter does not own these clients'
lifecycle, and the adapter is instantiated from main.ts (not a DI provider),
so no Nest lifecycle hook applied to it.
Keep references to both clients and override dispose(), which Nest's
SocketModule.close() invokes exactly once during shutdown after all socket.io
servers are closed. Use disconnect(false) to mirror the sibling pub/sub pair
in collaboration/extensions/redis-sync (onDestroy): immediate close, no QUIT
round-trip, no auto-reconnect. Refs are nulled to guard against double-close.
Runtime behavior is unchanged; only the shutdown path is added.
Verified with a script that boots connectToRedis() against a real Redis:
2 sockets to :6379 open after connect, 0 remain after dispose().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The store-side empty-guard consumed the per-document intentional-clear flag
INSIDE the bounded retry loop. consumeIntentionalClear always deletes the
in-memory Map entry, but a tx rollback cannot un-delete it: attempt 1
consumed the flag then updatePage threw a transient error and rolled back;
attempt 2 re-read the page non-empty, saw the flag gone, and the empty-guard
silently BLOCKED the write — dropping the user's deliberate clear and
defeating the retry guarantee for clears.
Hoist the decision out of the loop (like consumeContributors /
consumeAgentTouched): consume once into `allowIntentionalClear` before the
`for`, and only read that boolean on the empty-over-non-empty branch. The
single hoisted consume still drops a pending flag for a non-empty store
(the "cleared then retyped" case), since every store consumes regardless of
incoming emptiness.
Add a regression test: arm via the real onStateless transport, updatePage
throws once then succeeds, assert it is called twice and the retry writes the
empty doc (the clear survives). It fails on the old consume-in-loop ordering
(updatePage called once) and passes after the hoist.
Document the known fail-safe limitation near the TTL constant: if document
ownership transfers / a node crashes between the stateless signal and the
debounced store, the in-memory flag is lost and the clear is silently not
applied (the doc reloads non-empty) — fail-safe, content is never destroyed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The full-AppModule e2e (apps/server/test/app.e2e-spec.ts) passed but jest
never exited, burning CI to its timeout. Diagnosis (process._getActiveHandles
after app.close()) showed exactly two ioredis sockets to :6379 still open after
shutdown; everything else (BullMQ queues/workers, @nestjs/schedule intervals,
nestjs-ioredis, nestjs-kysely pg pool, @nestjs/cache-manager Keyv store,
hocuspocus pub/sub) already closes on app.close().
The two leaks were owned-but-never-closed clients:
1. ThrottleModule passed a pre-built `new Redis(...)` instance to
ThrottlerStorageRedisService. With an instance, the lib sets
disconnectRequired=false, so its onModuleDestroy never disconnects.
Pass ioredis options instead so the service owns + disconnects the client.
2. CollaborationGateway created a source `new RedisClient(...)` that
RedisSyncExtension only duplicates into pub/sub; the extension's onDestroy
disconnects those duplicates but not the source. Keep a reference and
disconnect it after the hocuspocus onDestroy hook in destroy().
Both are real lifecycle fixes (production shutdown is now clean too), so no
--forceExit is needed. Verified against real Postgres+Redis:
- test:e2e (no forceExit, --runInBand) exits 0 in ~18s (was: hung forever)
- --detectOpenHandles exits 0 with no open-handle report
- active handles after app.close(): none
CI timeout-minutes safety nets left untouched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #248 store-side empty-guard (onStoreDocument) unconditionally refuses to
overwrite non-empty persisted content with an empty document, because a
momentarily-empty live Y.Doc is indistinguishable from a real clear at the
store layer. That correctly blocks glitches/bad-merges, but also blocks a user
who genuinely wants to empty a page. This re-introduces a WORKING, narrow,
non-spoofable exception (the dead context.intentionalClear hatch #248 removed
never had a real channel).
Definition of an intentional clear (client, IntentionalClear editor extension):
a LOCAL user transaction (docChanged, NOT a remote y-sync change — filtered via
isChangeOrigin) that reduces a non-empty doc to the empty single-paragraph
shape. This is exactly the select-all + Delete/Backspace keystroke path.
Transport (option b — hocuspocus stateless message): on that transition the
client sends a `{type:'intentional-clear'}` stateless message. The server
(PersistenceExtension.onStateless) records a short-lived (TTL 60s > 45s
maxDebounce), single-use "pending clear" flag keyed by the connection's
document. The next debounced onStoreDocument consumes it on the empty-guard
branch to let that one empty write through.
Why this is the right channel and non-spoofable:
- Yjs transaction origin/metadata does not survive to the server store; awareness
is per-connection and racy. A stateless message ties the signal to a specific
clear, survives the debounce, and rides the authenticated connection.
- The document is taken from the connection, never the payload, so a client
cannot target another page.
- The flag is read ONLY on the empty-over-non-empty branch, so the worst a forged
signal can do is clear a page the connection may already edit; it can never
force or alter a non-empty write. Read-only connections cannot arm it. Every
non-empty store drops a pending flag, so "cleared then retyped" leaves nothing
usable; the flag is single-use and TTL-bounded.
NOTE: #248 is not yet on develop, so the empty-guard block is included here as
the foundation this exception extends. If #248 lands first this rebases cleanly
(the guard logic is identical; the #251-unique additions are the exception,
onStateless, the pending-flag state, and the client extension).
Tests:
- Server (real transport path, not a hand-poke): onStateless sets the flag with
the exact client payload, then the debounced onStoreDocument persists the empty
doc; plus single-use consumption, read-only rejection, non-empty-store drops
the flag, and the unchanged #248 guard tests (empty-over-non-empty blocked,
empty-over-empty allowed).
- Client: a real Editor + the actual selectAll+deleteSelection command emits the
signal; typing / non-emptying edits / already-empty docs do not.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply review suggestions to the real-files block in
ai-agent-roles-catalog.provider.spec.ts (test-only):
1. Fix inaccurate comment: there are 5 content YAML files (index +
four per-bundle/lang files), not 6.
2. Improve isolation: read/parse the real index lazily inside tests
(via loadRealIndex) instead of in the describe body, so a broken
real file fails only these catalog tests, not collection of the
whole spec (incl. the unrelated mocked-remote provider tests).
3. Add the symmetric slug check: each language file's slug set must
equal the declared slug set (no undeclared/extra roles), matching
scripts/check.mjs's exact two-way correspondence.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The blob-sandbox feature (#243/#250) made AiChatToolsService.forUser()
eagerly call this.sandboxStore.asSink() while wiring the stash tool, but
the spec still passed an empty {} as the sandboxStore constructor arg.
That object has no asSink method, so all 19 tests in the suite failed in
CI with 'TypeError: this.sandboxStore.asSink is not a function'.
Replace the stale {} mock at all 4 constructor sites with a no-op sink
exposing asSink() -> { put, has, evict } (jest.fn()). These tests never
execute the stash tool, so a no-op sink is sufficient for forUser() to
wire successfully. Test-only change; production code is unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Provider tests only exercised synthetic stringifyYaml fixtures, so a
hand-conversion error in one of the 6 real catalog files (index.yaml,
bundles/{editorial,research}/{en,ru}.yaml) — a stray quote/colon in a
description, a broken emoji/arrow, a block-scalar indent slip that
silently changes or drops instructions — was caught by no automated
test. scripts/check.mjs is the only other guard and is wired into no
CI/turbo/husky step.
Add a real-files test block that reads each shipped file off disk,
parses it with the SAME options the provider uses
(strict: true, maxAliasCount: 100), and validates it through the
provider's own exported type guards (isCatalogIndex / isCatalogBundleFile
/ isCatalogRole). It is driven from the real index so new bundles/langs
are auto-covered, asserts the editorial bundle still ships fact-checker,
and requires every declared role to be present with non-empty
instructions/name in each language file.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1: clear the "Reindex now" spinner once the poll cap fires. Gate the
reindexing part of the button's loading state on the active poll window
(reindexDeadline !== null) so a run that outlives the 120s cap no longer
leaves the button stuck-disabled with a stale `reindexing: true`; the
admin can restart.
F2: rewrite reindexWorkspace JSDoc to describe the EMBEDDABLE page set
(text OR existing embeddings), matching getEmbeddablePageIds /
countEmbeddablePages instead of the old "every non-deleted page".
F3: extract the shared embeddable-content predicate into a private
PageRepo.embeddablePredicate helper, called by both countEmbeddablePages
and getEmbeddablePageIds, removing the verbatim duplication. Behavior is
identical (lockstep int-spec stays green).
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>
Must-fix:
- mcp.module: drop the now-dead EnvironmentModule import (and its stale
comment). McpService no longer injects EnvironmentService; EnvironmentModule
is @Global and imported at the app root, so DI still resolves.
Stability:
- environment.service: route getSandboxTtlMs + the three SANDBOX_MAX_*_BYTES
caps through a shared getPositiveIntEnv() helper that warns once per key and
falls back to the default on a non-integer or <= 0 value (previously the byte
caps did a bare parseInt, so SANDBOX_MAX_TOTAL_BYTES=0 made every stash_page
fail against a 0-byte cap). TTL behavior is unchanged.
Simplification:
- sandbox.controller: replace the homemade UUID_RE with the project's shared
`uuid` validator (import { validate as isValidUUID } from 'uuid'), matching
the attachment routes; update the spec fixtures to valid v4 UUIDs.
- mcp.service: inline the single-caller one-liner buildSandboxConfig() to
this.sandboxStore.asSink() at the wiring site.
Docs:
- CHANGELOG: add an [Unreleased] > Added entry for #243 (stash_page tool,
anonymous GET /api/sb/:id, five SANDBOX_* env vars).
- AGENTS.md: note that GET /api/sb/:id is in the workspace-gate preHandler's
excludedPaths and is fully tokenless, unlike /api/files/public/... which
still resolves a workspace and needs an attachment JWT.
Tests: cap-getter validation (0/-5/abc -> default, valid -> parsed), updated
UUID fixtures. apps/server jest sandbox/environment/mcp: 233 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>
claude code agent 227
agent_vscode
vvzvlad
agent_coder
