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>
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>
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>
Approve-with-comments follow-ups:
- breadcrumb: fix the reverse regression where navigating A->B to a page absent
from the lazily-built tree (before its ancestors load) left the previous
page's clickable chain on screen. New pure computeBreadcrumbState clears a
stale chain that doesn't end at the current page, while keeping one that does
(no blank flash for an already-resolved page); unit-tested for the
navigated-to-absent-page case.
- share.service: getShareAncestorPage no longer swallows DB errors silently —
now a live public-share path (isPageReachableThroughShare), so a transient
error is logged with ancestor/child ids and still fails closed (caller 404s)
instead of becoming a traceless misleading "not found".
- i18n: register the new "Connecting… (read-only)" key (U+2026 ellipsis) in
en-US (source of truth) and ru-RU (Подключение… (только чтение)).
- share.service: correct the FUTURE note — 3 callers pass no shareId
(share-alias.controller/.service, share-seo.controller); the two ai-chat
callers already pass a real shareId.
- CHANGELOG: add Unreleased Changed/Fixed/Security entries for #216 opt-in
sub-pages default, #218 trimmed page-info payload + forged-shareId 404, #204
export internal-link name, #206/#218 breadcrumb, #192 callout paste, #218
editor pre-sync read-only gate.
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>
Review follow-ups for the combined QA-UI fixes (#216/#206/#204/#218/#192):
- export/utils: correct the misleading getInternalLinkPageName comment — a
bare `v1.2` loses its last dot-segment (`v1`); dots survive only in
multi-segment names like `v1.2.md` -> `v1.2`.
- share: extract toPublicSharePayload(page, share): PublicSharePayload, an
explicit allowlist type+mapper replacing the inline literal in the
/shares/page-info anonymous path (#218). Add share.controller.spec.ts that
stubs getSharedPage returning internal fields and asserts the response key
set EXACTLY equals the whitelist (page + share), so any `...shareData`
regression or new leaking field fails. Also key-tests the extracted mapper.
- breadcrumb: extract pure resolveBreadcrumbNodes(treeData, ancestors, pageId)
(tree-hit -> tree; tree-miss -> map ancestors via canonical pageToTreeNode,
dropping the as-any casts; else null) and unit-test all three branches.
- share-modal: RTL test asserting enabling a share calls mutateAsync with
includeSubPages: false (#216 security default).
- share.service: one-line note at getSharedPage on the deferred consolidation
of the ancestor-aware match into resolveReadableSharePage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The footnote canonicalizer was wired into the MCP and editor-ext write paths
but NOT into the server's user-facing markdown/HTML import paths, so importing
or pasting markdown with out-of-order, reused, or orphan footnotes did not
canonicalize -- the exact trigger bug #228 fixes was still reproduced on
import. markdownToHtml -> htmlToJson builds ProseMirror JSON directly and never
runs the editor's footnoteSyncPlugin, and that plugin does not reorder an
existing list, so the stored footnotes kept the source's physical definition
order, retained orphans, and did not collapse reused references.
Wire canonicalizeFootnotes (already exported from @docmost/editor-ext) into
every server markdown/HTML -> page-JSON seam, before persisting:
- ImportService.importPage (REST single-file .md/.html import)
- FileImportTaskService (zip import worker)
- PageService.parseProsemirrorContent (API createPage / updatePageContent)
Also hook the client markdown paste: handlePaste applies a manual transaction
(returns true), bypassing transformPasted/footnoteSyncPlugin, so a pasted
out-of-order markdown footnote block would persist out of order.
canonicalizePastedFootnotes reorders a self-contained pasted block (one that
carries its own footnotesList) to reference order, deduped and orphan-free; it
is deliberately scoped to whole-block pastes so a reference-only paste that
reuses a footnote already defined in the target doc is left untouched.
canonicalizeFootnotes is pure, idempotent and shape-safe (a doc with no
footnotes is unchanged), so it is safe on every write path.
Residual: when a pasted block merges into a doc that already has footnotes,
ordering relative to the pre-existing footnotes is still governed by the live
sync plugin (which does not reorder across the boundary).
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>
The agent-roles catalog source is no longer hardcoded in app code and no longer
supports a local filesystem directory. The provider fetches only from an
http(s):// base URL read at runtime from AI_AGENT_ROLES_CATALOG_URL; an empty or
non-http value yields a 502 (catalog unavailable). The image ships a per-branch
default for that URL (set in CI), still overridable at runtime via the env var.
- provider: drop readLocal + node:fs/node:path; readRelative requires http(s)
and 502s otherwise; remote fetch/streaming-cap/SSRF guards unchanged.
- environment.service: keep AI_AGENT_ROLES_CATALOG_URL (default ''); comment
reflects the per-branch build-time default that is runtime-overridable.
- Dockerfile: add ARG+ENV AI_AGENT_ROLES_CATALOG_URL in the installer stage as
the image default.
- CI: develop.yml builds with the develop raw URL; release.yml defines the main
raw URL once in workflow env and references it from both build steps.
- tests: replace local-fixture tests with remote-mock happy/malformed bundle
tests and a non-http => 502 case; path-traversal block uses an https source.
- docs: update .env.example, CHANGELOG (#222), agent-roles-catalog/README.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR #227 re-review (comment 2193).
- Stability: `updatePageId`/`updateAlias` now `executeTakeFirstOrThrow`, so a row
reaped by a concurrent `removeAlias` between the read and the UPDATE (READ
COMMITTED) raises `NoResultError` instead of returning `undefined`. The service
maps that to a retryable `ConflictException` (`ALIAS_PAGE_RACE`) rather than a
200-without-alias (swap) or a generic 400 from `undefined.id` (rename). Tests
cover both branches.
- Simplification: drop the redundant secondary "unexpected unique index" warn and
the now-unused `UNIQUE_ALIAS_INDEX` const (the constraint name is already logged
unconditionally; both index branches still distinguish "Alias already taken" vs
ALIAS_PAGE_RACE).
- Architecture: extract `isUniqueViolation`/`violatedConstraint` into
database/utils.ts; adopt them in the share-alias service and favorite.repo
(the bare `23505` check). ai-agent-roles (#222) is on a separate unmerged branch
and should adopt them after #227 merges (noted at the helpers). Helper unit test
added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The agent-roles catalog source is no longer hardcoded in app code and no
longer supports a local filesystem directory. The provider now fetches only
from an http(s):// base URL read from AI_AGENT_ROLES_CATALOG_URL; an empty or
non-http value yields a 502 (catalog unavailable). The default URL is baked
into the Docker image at build time and set per branch in CI.
- provider: drop readLocal + node:fs/node:path; readRelative requires http(s)
and 502s otherwise; remote fetch/streaming-cap/SSRF guards unchanged.
- environment.service: keep AI_AGENT_ROLES_CATALOG_URL (default ''); comment
updated to reflect build-time injection, remote-only.
- Dockerfile: add ARG+ENV AI_AGENT_ROLES_CATALOG_URL in the installer stage.
- CI: develop.yml builds with the develop raw URL; release.yml (both build
steps) with the main raw URL.
- tests: replace local-fixture tests with remote-mock happy/malformed bundle
tests and a non-http => 502 case; path-traversal block uses an https source.
- docs: update .env.example, CHANGELOG (#222), agent-roles-catalog/README.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review on PR #227.
- setAlias confirmed-reassign branch: DELETE the target page's existing
alias row(s) BEFORE retargeting `byName` onto the page, instead of after.
The new partial unique index `(workspace_id, page_id)` is non-deferrable
and checked at each statement, so retargeting first momentarily left two
rows for the page -> immediate 23505 -> rolled-back tx surfaced as a
misleading "Alias already taken" (regressing a previously-working swap onto
a page that already had its own alias). The reordered branch needs no
trailing self-heal. JSDoc updated to describe the real ordering.
- catch block: the postgres@3.x driver exposes the violated index as
`err.constraint_name` (with `.constraint` as a fallback). Map
`share_aliases_workspace_id_alias_unique` -> "Alias already taken" and the
new `share_aliases_workspace_id_page_id_unique` -> a distinct ALIAS_PAGE_RACE
outcome (a concurrent same-page write, not a name clash). Always log the
constraint name on any 23505 so the race is diagnosable.
- migration 20260627T120000: document that the dedup DELETE is intended,
irreversible data loss (old duplicate `/l/<old>` links start 404ing after
upgrade; `down()` cannot restore the rows). Same note added to CHANGELOG
[Unreleased] Fixed.
Tests:
- integration: confirmed reassign onto a page that ALREADY has its own alias
(RED before the reorder); migration up() dedup scoping across pages and a
second workspace; mid-transaction error -> BadRequest with clean rollback.
- unit: constraint_name distinguishing (alias index, page_id index, fallback
`.constraint`, no-info default) and non-unique error -> BadRequest; retarget
test now asserts delete-before-update order.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Editing an existing share alias (e.g. slug `te` -> `ted`) failed to update
the displayed `/l/<alias>` link: `setAlias()` looked the requested slug up by
name and, if free, INSERTed a brand-new row, leaving the page with multiple
alias rows. The modal then read via `findByPageId().executeTakeFirst()` with no
`ORDER BY`, so Postgres returned an arbitrary (in practice the oldest, stale)
row. Every edit also spawned an orphan row that kept a live `/l/<old>` link
forever. Regression of #205.
Enforce the invariant "a page has EXACTLY ONE custom address":
- `setAlias()` now resolves the page's current alias row and RENAMES it in
place when the requested name is free (insert only when the page has none),
keeps the same-name no-op and the cross-page 409 `ALIAS_REASSIGN_REQUIRED`
+ confirmed-retarget flow, and after any successful write DELETEs all other
alias rows for the page (self-heal). Runs in one transaction so the page is
never transiently empty or duplicated.
- repo: add `updateAlias` (rename) and `deleteOthersForPage`; make
`findByPageId` deterministic with `ORDER BY created_at DESC, id DESC`.
- migration: dedup existing rows (keep newest per page) + a PARTIAL unique
index `(workspace_id, page_id) WHERE page_id IS NOT NULL` so dangling
aliases still coexist while live ones are one-per-page.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ITEM 1: cover useImportAiRolesFromCatalogMutation onSuccess notifications.
Add import-from-catalog-message.test.tsx (twin of update-from-catalog-message)
asserting the always-shown summary (errors:[]) and the additional red
"Failed to import N role(s)" notification when result.errors is non-empty.
ITEM 2: pass redirect:'error' to the remote catalog fetch in fetchRemote so a
compromised-but-trusted upstream cannot 3xx the fetch into the internal network
(redirect-SSRF). Add provider specs asserting the option is passed and that a
redirect rejection maps to BadGatewayException.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MUST-FIX
- isSourceUniqueViolation read the wrong error field: kysely-postgres-js
(postgres@3.4.8) puts the violated constraint on `constraint_name`, not
node-postgres' `.constraint`, so a concurrent same-slug+language import's
23505 was never recognized as a source-collision and surfaced a false
"name already exists" error. Now read `constraint_name` (with `.constraint`
as a fallback for other drivers). Fix the faked test fixture (it built the
error with the same wrong `.constraint` field, masking the bug): it now
uses `constraint_name`, so the test genuinely exercises the skip path and
FAILS against the unfixed code.
- Extract the catalog modal's role-state computation into a pure
`catalogRoleInstallState(role, workspaceRoles, language)` helper (mirrors
role-launch.ts) and cover it with vitest: import / installed / update /
same-slug-different-language.
SUGGESTIONS
- Restore IAiRoleUpdateFromCatalogResult as a discriminated union mirroring
the server; narrow the consumer via `"reason" in result` (the boolean
discriminant does not narrow under strictNullChecks:false).
- README: add a "How it's served" section documenting AI_AGENT_ROLES_CATALOG_URL
(remote http(s) base / local path / empty => in-repo folder).
- check.mjs: drop the redundant `const key = slug` alias.
- Cover the reason->message mapping in useUpdateAiRoleFromCatalogMutation
(4 branches) via renderHook with a mocked service.
- Cover importFromCatalog "bundle not in index" => BadGateway.
- Cover updateFromCatalog "slug in index but missing in bundle file" =>
not-in-catalog.
ARCHITECTURE
- Extract the shared catalog read prefix: a private `loadBundleById`
(fetchIndex -> meta -> fetchBundle -> versionMap) reused by getCatalogBundle
and importFromCatalog, and a `catalogRoleContentFields` mapper shared by the
import insert and update patch. The three orchestrations and their distinct
write paths stay separate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review comment 2159 on the temporary-notes UI work.
Tests:
- tree-model: cover handleCreate's race-guard temporaryExpiresAt patch — (a)
server node inserted WITHOUT a deadline + create response carries one => node
gains the deadline; (b) node already has a deadline => not overwritten, prev
returned by reference.
- ws-tree.service.spec: broadcastPageCreated now asserts the deadline is carried
when present and pinned to null (`?? null`) when absent.
- page-embed-query (new spec): syncTemporaryExpiresInCache patches the in-tree
node's temporaryExpiresAt, and leaves the atom value at the same reference when
the id is absent from the loaded tree (no write).
Refactor (closes the drift bug-class at the root):
- Client: extract one canonical pageToTreeNode(page, overrides) mapper in
tree/utils and route buildTree, handleCreate's optimistic insert, the restore
mutation and the duplicate handler through it. Restore stays permanent (server
nulls temporaryExpiresAt) and duplicate stays permanent (server arms no timer)
— both now reflect the server without a reload, where before they dropped the
field entirely.
- Server: extract one toTreeNodeSnapshot(page) helper called by both the
PAGE_CREATED event enrichment (page.repo) and the addTreeNode broadcast
(ws-tree.service), so the optional temporaryExpiresAt can't drift between the
two literals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Issue 1 — the sidebar tree's temporary-note clock marker did not appear/
disappear until a page reload when a note's temporary state changed.
- Make/unmake permanent from the page header menu and the in-page banner went
through syncTemporaryExpiresInCache(), which patched the page query cache but
never touched treeDataAtom, so the sidebar node kept its stale
temporaryExpiresAt. Patch the tree node there too (via jotai's default store),
so the marker updates without a reload.
- Creating a note as temporary showed no marker until reload: the create flow's
cache write (invalidateOnCreatePage) omitted temporaryExpiresAt, so the tree
rebuild (buildTree -> mergeRootTrees) overwrote the optimistic/socket node's
marker with undefined. Carry temporaryExpiresAt in that cached entry.
- Thread temporaryExpiresAt through the server addTreeNode broadcast (PAGE_CREATED
snapshot -> TreeNodeSnapshot -> broadcastPageCreated) so OTHER clients watching
the space also render the marker immediately, and harden handleCreate's
idempotency guard to patch the deadline if the broadcast won the insert race.
Issue 2 — the home and space-overview "New note" / "New temporary note" buttons
sat side-by-side and the temporary label clipped on narrow mobile widths. Lay
them out full-width, stacked vertically, and tint the temporary button orange
(matching the clock marker + banner) while the regular one stays neutral gray.
Tests: extend tree-socket-reducers.test.ts (addTreeNode carries
temporaryExpiresAt). Verified live with Playwright: marker appears on create and
toggles both ways with no reload; mobile buttons are stacked, full-width,
unclipped, and differently colored.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Item 1 (concurrency-safe import): add a partial UNIQUE index on
(workspace_id, source->>'slug', source->>'language') WHERE source IS NOT NULL
AND deleted_at IS NULL, so two concurrent imports of the same bundle can no
longer create duplicate roles for one catalog slug+language. The in-memory
installedKeys snapshot cannot see a sibling request's writes; the index is the
backstop. importFromCatalog now catches the 23505 from THIS index (keyed off
the constraint name) and treats it as "already installed" -> skip, batch
continues. A 23505 from the name-uniqueness index keeps its existing friendly
per-role error behavior (distinguished by constraint name; an indeterminate
23505 falls back to that path, so no regression).
Item 2 (single source validator): strengthen parseSource into THE single form
validator for the source jsonb column -> returns a fully-valid RoleSource | null
(slug/language non-empty strings, version a number). The service's weaker
roleSource is removed and both layers share the RoleSource type (defined in the
db entity.types module both already import AiAgentRole from, so no import
cycle). normalizeRow / the read path now only ever yield a valid RoleSource or
null; a malformed stored source normalizes to null (tolerated by the service).
Tests: parseSource null for {} / {slug:123} / {slug:'a'} / empty-string keys /
string version, typed value for a full valid shape; service test that a
source-uniqueness 23505 is skipped (not errored) and the batch continues.
Verified the partial index rejects a duplicate source-not-null row but allows
two source-NULL rows, and the migration up/down run cleanly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CHANGELOG: document the importable multilingual agent-roles catalog under
[Unreleased] (browse/import/update, 4 new endpoints, source column, the new
AI_AGENT_ROLES_CATALOG_URL env var) (#222).
- Fix importFromCatalog docstring: a role is skipped only on source.slug AND
source.language; another language of the same slug still imports.
- Provider: map a timeout/abort (or any failure) during the response-BODY read
to a logged BadGatewayException, so a slow/dripping source yields a 502, not a
generic 500. Existing too-large BadGateway cases are rethrown as-is.
- Service: inject a Nest Logger and log the root cause (with workspaceId/
bundleId/slug) on a non-23505 insert error during import.
- Modal: hoist the duplicated i18n base-subtag into a single baseLang const.
- Tests: AbortError body-read -> BadGateway; null-body text() fallback (under
and over cap); invalid-JSON and malformed-index BadGateway; non-23505 import
error -> generic message + logged root cause.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename catalog-source migration 20260626T120000 -> T150000 so it sorts
after develop's latest migration (T140000-page-temporary-notes); the old
timestamp predated ai-chat-message-status/share-aliases and tripped
Kysely's #ensureMigrationsInOrder, aborting server boot.
- Provider: inject a Nest Logger and log the real cause (incl. response
status) in the parseJson / readLocal / fetchRemote catch blocks, and
propagate a useful cause into the BadGatewayException message; add a
shortError helper (robust to jest's realm-shifted Error-likes).
- Provider: replace the manual Uint8Array assembly with
Buffer.concat(chunks).toString('utf8'); keep the streaming size cap.
- Controller spec: add admin-gate coverage for the 4 catalog routes
(catalog/catalogBundle/import/updateFromCatalog) - non-admin Forbidden +
service untouched, admin delegates with the right args.
- Service spec: add getCatalog/getCatalogBundle tests covering the
localized() three-tier fallback, the sorted language union, the
missing-bundle BadGateway, and the role-version default.
- Provider spec: add remote fetch-rejects and non-ok (503) error branches.
- Service: drop the dead Date.now() tail in freeName (now an explicit
unreachable throw) and extract a shared isUniqueViolation() predicate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Admins can browse a curated catalog of agent roles, import roles/bundles
into a workspace, and update an imported role when the catalog ships a
newer version.
Catalog: a set of JSON files (index.json manifest + bundles/<id>/<lang>.json)
served from a local folder (dev) or a remote http(s) base URL via
AI_AGENT_ROLES_CATALOG_URL. Seeded with the existing 7 RU roles (editorial +
research bundles) plus EN translations.
Server:
- migration: nullable jsonb `source` column on ai_agent_roles
({ slug, language, version }; null => manually created)
- catalog provider: remote fetch with timeout + streaming size cap, or local
read; ^[a-z0-9-]+$ segment guard against path-traversal/SSRF
- admin endpoints: catalog, catalog/bundle, import, update-from-catalog
- import/update match by slug+language; update preserves `enabled`
Client:
- catalog modal with language selector and Import/Installed/Update states
- "Import from catalog" button + empty-state CTA in the roles settings panel
- en-US/ru-RU strings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the separate, un-toggleable `settings.ai.generative` workspace flag
(and its write-side alias `generativeAi`) along with the dead "Ask AI"
generative editor menu, and re-gate the AI page-title generation on the
general AI chat flag (`settings.ai.chat`) — the same toggle that enables
the chat agent and the chat stream endpoint.
Why: the `generative` flag had no UI toggle (its switch was already removed,
leaving orphaned i18n strings), so the title-generation button was
unreachable on self-hosted. The "Ask AI" menu was dead — its atom was never
rendered. Consolidating onto the AI chat flag makes the title button follow
the one AI switch users actually have.
Changes:
- server: title-gen endpoint gate generative -> chat (ai-chat.controller.ts);
remove generativeAi from update DTO and workspace service (update block,
delete line, cloud default now { ai: { chat: true } }); fix repo comment;
migrate generate-page-title spec assertions generative -> chat.
- client: title-gen gate -> settings.ai.chat (full-editor.tsx); remove the
dead Ask AI button + showAiMenu wiring from bubble-menu; remove AskAiGroup
usage/import and commented block from fixed-toolbar; delete ask-ai-group.tsx;
remove showAiMenuAtom; drop generative/generativeAi from workspace types.
- i18n: remove 3 orphaned generative-AI keys from all 12 locales.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On opening the floating AI-chat window from the header on a document page,
auto-open the LAST chat bound to that document. Binding reuses the existing
ai_chats.page_id (no migration): the bound chat is the requesting user's
most-recent non-deleted chat created on that page, so a new chat on the page
becomes the bound one for free. Resolution happens only on a genuine
closed -> open transition; the provenance badge deep-link is untouched.
Server: AiChatRepo.findLatestByPage + POST /ai-chat/bound-chat (BoundChatDto),
both read-only and owner/workspace-scoped.
Client: getBoundChat service + useOpenAiChatForCurrentPage hook wired into the
app-header entry point (fail-soft to a fresh chat; draft/role cleared only on a
real switch).
Tests: repo scoping/ordering, controller wiring, and hook behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
develop merged 20260626T130000-share-aliases; rename this PR's migration to
20260626T140000 so the two no longer share a timestamp prefix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Register the new AI-chat keys "Send now" and "Interrupt and send now" in
both en-US and ru-RU catalogs so the UI never renders mixed-language
tooltip/aria-label (i18n policy).
- Make INTERRUPT_NOTE module-private (drop the unused re-export), matching the
module's private DEFAULT_PROMPT/SAFETY_FRAMEWORK siblings.
- Reset interruptNextSendRef in the flush-on-abort branch when nothing is
actually sent, so a stuck one-shot interrupt flag cannot tag the next
unrelated send; flushNext now reports whether it sent.
- Add a CHANGELOG [Unreleased]/Added entry for #198.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The deletion guard skips a note when its re-read deadline is still in the
future (user disarmed-then-re-armed in the race window between the batch
SELECT and the per-row re-read). The default stub returns an epoch deadline
(always < now), so the existing race tests never exercised the
`new Date(temporaryExpiresAt) >= now` branch; a regression dropping it or
inverting the comparison would pass unnoticed. Add a test that re-reads a
fresh future deadline and asserts removePage is not called.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- CHANGELOG: add [Unreleased]/Added entry for temporary notes (#201).
- temporary-note-cleanup: re-check temporary_expires_at at deletion time so a
concurrent "Make permanent" (sets it NULL) between the batch SELECT and the
per-row removePage wins the race and the note is not trashed. Add unit tests
for the make-permanent and already-trashed race windows.
Non-blocking review items:
- temporary-note-cleanup: cap the sweep batch (LIMIT 500) so a large backlog is
not loaded into memory; remainder drains on the next hourly run.
- client: extract duplicated post-toggle cache sync into
syncTemporaryExpiresInCache() shared by the header menu and the banner.
- Remove the tautological migration spec that mocked the whole Kysely builder.
- Tests: cover create() frozen temporaryExpiresAt (workspace override + NULL
default fallback + non-temporary skips lookup) and restorePage disarming the
timer (temporaryExpiresAt: null).
Deferred (forward-looking, non-blocking): extract
PageService.computeTemporaryExpiresAt() to dedupe the deadline formula and drop
the @InjectKysely from PageTemplateController; replace migration unit test with
a real Postgres up/down integration test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the PR #202 review (approve-with-comments). The only actionable
non-blocking item was the test-coverage suggestion: the source switch in
AiChatService.handle from findRecent(chatId, ws, 50) to findAllByChat(chatId,
ws) was not pinned by a test. handle() is a streaming method the project marks
as not unit-testable, so cover the behavioral guarantee it now relies on at the
repo/integration level — seed a chat of 60 messages and assert the default
findAllByChat (exactly how handle calls it) returns the FULL transcript in
chronological order, including the first turn the old 50-window would have
dropped.
Also document the behavior change under CHANGELOG [Unreleased] -> Changed.
The two stability items (token-budget trim before streamText; O(N) history
rebuild per turn) are deferred: the reviewer flagged both as non-blocking
conscious trade-offs aligned with the PR's stated goal, and the trim is a
larger architecture change out of scope for this follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-turn model conversation was rebuilt via findRecent(chatId, ws, 50),
a sliding window that dropped the beginning of any chat longer than ~50 stored
rows. Switch streamChat to the existing findAllByChat, which loads the full
non-deleted transcript chronologically with a 5000-row memory-safety backstop
(keeps the newest rows + logs a warning on overflow) — a safety net, not a
conversational limit. Remove the now-unused findRecent method and update the
comments/log text that referenced it (findAllByChat now feeds both the Markdown
export and the model history).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add an AI button in the page byline that generates a note's title from the
live editor content (including unsaved edits) and applies it immediately.
Server: one-shot, non-streaming POST /ai-chat/generate-page-title mirroring
the chat generateTitle path — gated by settings.ai.generative, throttled via
AI_CHAT_THROTTLER, resolves the workspace chat model and returns { title }.
The endpoint never touches the page; the client applies the title through the
existing /pages/update route (which enforces edit permission).
Client: ai-chat-service.generatePageTitle, a useGeneratePageTitle hook that
converts the editor HTML to markdown, calls the endpoint, applies the title
via updateTitle + updatePageData, reflects it in the unfocused title editor,
and broadcasts the UpdateEvent (mirroring TitleEditor.saveTitle). A sparkles
button (GenerateTitleGroup) renders next to dictation, edit-mode + flag gated.
Tests: pure cleanGeneratedTitle helper + controller gate/delegation/error-map.
i18n: en-US + ru-RU strings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
"Temporary notes" with a death timer: created via a dedicated hourglass button
in the space-tree header, a note auto-moves to Trash after a configurable X
hours (default 24) unless explicitly made permanent ("structure or die").
Reuses existing mechanisms, mirroring is_template and the trash-cleanup job:
- New nullable column pages.temporary_expires_at (NULL = permanent; non-NULL =
frozen deadline) + partial index for the sweep; workspace column
temporary_note_hours (default via DEFAULT_TEMPORARY_NOTE_HOURS = 24).
- create-page DTO `temporary` flag; the deadline is frozen at creation so later
setting changes never reschedule existing notes.
- POST /pages/toggle-temporary (mirror of toggle-template): arm/clear the timer,
CASL-guarded via validateCanEdit, cross-workspace NotFound defense-in-depth.
- TemporaryNoteCleanupService: hourly @Interval sweep that soft-deletes expired
notes through the exact PageRepo.removePage path (recursive over children,
emits PAGE_SOFT_DELETED), attributed to the creator; idempotent via
deletedAt IS NULL filters.
- restorePage clears temporary_expires_at so a restored note can't be re-trashed.
- Workspace setting temporary_note_hours (audit-tracked) + a hours editor in
workspace General settings.
- Client: second create button, orange tree icon, tree + page-header menu toggle
("Make temporary"/"Make permanent"), an open-note banner with a rescue action,
and en/ru i18n.
Tests (unit): toggle-temporary controller (toggle/explicit/permission/cross-ws +
DTO validation), cleanup-job sweep (selection filters, per-note removePage,
error isolation), and a migration up/down sanity. Server tsc, client tsc -b,
and the page+workspace jest suites are green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a "send now" button to queued AI-chat messages: it interrupts the
running agent and immediately sends that message, while the agent's
partial output at interruption is kept in history and the next turn is
marked as a user interrupt.
Client:
- queue-helpers: pure `promoteToHead` to move a queued message to the head.
- chat-thread: `sendNow` (promote head + abort + flush-on-abort), one-shot
`flushOnAbortRef`/`interruptNextSendRef`, `interrupted` flag in the
request body, and the "send now" ActionIcon in the queued list.
Server:
- `interrupted` on AiChatStreamBody; pure `isInterruptResume` confirms the
client hint against persisted history (prev assistant turn aborted/
streaming) before honouring it.
- prompt: INTERRUPT_NOTE injected in the context section only on a
confirmed interrupt-resume turn so the model treats the partial answer
above as incomplete.
Tests: promoteToHead, chat-thread send-now (abort + resend + one-shot
interrupt flag + non-streaming immediate send), isInterruptResume, and
the prompt interrupt-note injection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #205 share-aliases feature placed share-aliases.migration.spec.ts
inside src/database/migrations/. Kysely's FileMigrationProvider loads
EVERY file in that folder as a migration, so `migration:latest` imported
the test file and crashed with "ReferenceError: describe is not defined"
(no Jest globals under tsx). That broke the migration step shared by the
e2e-server, e2e-mcp and integration-test (test/test) jobs.
Move the spec one level up to src/database/ (matching the existing
src/database/jsonb-bind.spec.ts convention) so the migration runner no
longer sees it, and fix its relative imports
(./migrations/... and ./types/...). Jest still picks it up via the
src/**/*.spec.ts test glob. Verified locally: 3 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The e2e transform matches .js (required so ESM-only node_modules like
nanoid/@sindresorhus get transpiled), which also sweeps in editor-ext's
prebuilt CommonJS dist/*.js. ts-jest then warns "Got a .js file to
compile while allowJs is not set to true" for each footnote file. The
.js match cannot be dropped without reintroducing the ESM load errors, so
enable allowJs for ts-jest via an inline tsconfig override (merged with
apps/server/tsconfig.json — decorators/paths/module stay intact).
Verified locally: 0 allowJs warnings, app still compiles and boots to the
Redis connection (no DI/metadata regressions).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous jest-config fix let the module graph load further and exposed
two more reasons the server e2e never passed since it was added:
1. ESM transform chain: AppModule pulls in editor-ext -> @tiptap ->
@sindresorhus/slugify -> @sindresorhus/transliterate / escape-string-regexp,
plus p-limit -> yocto-queue — all ESM-only. Extend the e2e
transformIgnorePatterns whitelist to transform them (scoped packages need
both the pnpm `@scope+name` and nested `@scope/name` path forms, hence
`@sindresorhus[+/][a-z0-9-]+`). Verified locally: the graph now fully
transforms and resolves.
2. Wrong HTTP adapter: Docmost runs on Fastify (main.ts uses FastifyAdapter)
and does not depend on @nestjs/platform-express, but the scaffold test used
the default createNestApplication() (Express) and died with
"@nestjs/platform-express package is missing". Switch the test to
FastifyAdapter + getInstance().ready(), close in afterEach. Verified locally:
createNestApplication + app.init() now proceed to the live Redis/Postgres
connection (the infra CI provides via services + migrations).
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>
