The setImageCaption command and its Commands<> declaration were dead:
captions are written via the generic updateAttributes in
useImageTextFieldControl, and a repo-wide grep finds zero callers.
Remove the speculative implementation (image.ts) and its type
declaration.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract the ~110 duplicated lines into one parameterized
useImageTextFieldControl and make useAltTextControl/useCaptionControl
thin wrappers. Behavior identical; t("...") literals stay in the
wrappers so i18n extraction keeps working. sanitizeCaption still
exported for its unit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment claimed 250 groups -> 499 chars -> slice past 500; the
input is 120 "a b " groups collapsing to 479 chars, under the cap
with no slice. Correct the comment and assert the 479 length.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add PM -> markdown -> PM round-trip assertions for image caption
(plain and special-char), which fail without F1 and pass with it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stock @tiptap/extension-image carries no caption attribute, so
markdownToProseMirror through docmostExtensions dropped the
data-caption the client emits, breaking the lossless claim. Extend the
Image node (mirroring editor-ext image.ts and the nearby Highlight
extend) to parse/render data-caption. Rebuilt build/.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- docs: add CHANGELOG Unreleased/Added entry for editable image captions
- test: export sanitizeCaption and add vitest unit coverage
(whitespace collapse, trim, 500-char boundary)
- refactor: drop duplicate .imageCaption CSS module class, keep the
global .image-caption as the single source
- docs: fix turndown image-caption comment (video rule emits a markdown
link, not a <div>)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a visible caption (<figcaption>) under images, editable from the
image bubble-menu and persisted across all formats: native Yjs/JSON,
HTML export, and Markdown.
- image node: new plain-text `caption` attribute (parse/render
`data-caption` on <img>, emitted only when set) + `setImageCaption`
command. The node stays an atom; the schema shape is unchanged, so the
server's generateHTML/generateJSON path round-trips it for free.
- resize node-view: re-parent the resizable wrapper into a <figure> and
render the caption in a <figcaption> BELOW it, outside nodeView.wrapper
(so onCommit's offsetHeight measurement and the left/right resize
handles still cover the image only). This path also drives read-only /
share rendering. React placeholder view renders the caption too.
- bubble-menu: new useCaptionControl panel modeled on useAltTextControl
(own icon, Caption strings, softer sanitizer, ~500 char limit).
- markdown lossless round-trip: a captioned image is emitted as a raw
<img data-caption> wrapped in a block <div> (same trick as <video>) in
both the editor-ext turndown rule and the MCP converter; caption-less
images stay clean . Import restores the caption via the
shared markdownToHtml + parseHTML.
- styles + i18n keys; tests for the schema attr round-trip, markdown
round-trip (editor-ext) and the MCP converter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rework the fact-checker editorial role prompt so it stops commenting on
correct facts and only flags problems (errors, doubtful, unverifiable).
- Add the directive "don't write/comment that a fact is right or confirmed:
your job is to find errors, not confirm facts" to both RU and EN bundles.
- Remove the [Подтверждено]/[Verified] verdict; reframe the verdict list as
"for problem claims only".
- Reword the role description (no longer "confirms") and the
comment-on-every-claim rule to "problem claims only".
- Bump fact-checker role version 2 -> 3 and refresh the content-hash lock.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review #6 (approve-with-comments) follow-ups:
1. canonicalize step 7 now strips bare footnoteDefinitions at ANY depth
(stripFootnoteDefinitionsDeep), not just footnotesList, in BOTH copies. A
definition hand-authored outside a list (e.g. nested in a callout via a
raw-JSON write path) was left in place while a copy was also added to the
rebuilt list -> duplicate, idempotent, self-perpetuating. Runs only in the
rebuild path (after the lists are stripped); the fast-path / placement-keep
branch is untouched. Added a shared-corpus case (bare def nested in a callout)
to pin it in both mirrors.
2. markdown-clipboard: removed the dead top-level footnoteReference check in
canonicalizePastedFootnotes (an inline atom is never a top-level slice child;
only the descendants scan can find it).
Test coverage:
4. New MCP binding tests (full-doc-write-canonicalize.test.mjs): update_page_json
and copy_page_content canonicalize the persisted full doc, asserted via a new
`replacePage` seam (symmetric to the existing `mutatePage` seam) so no live
collab socket is needed. Routed both writers through the seam.
5. New server spec (file-import-task.service.footnote-canonicalize.spec.ts): the
zip-import path (processGenericImport) canonicalizes footnotes — real
markdown->HTML->JSON via a real ImportService over a temp-dir .md file, DB trx
stubbed to capture the persisted page content. FileImportTaskService had no
spec before.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- insertInlineFootnote could glue a footnoteReference inside an EXISTING
definition (nested footnotesList, or a bare footnoteDefinition with no list
wrapper), which canonicalize then dropped as an orphan — silently losing the
definition's prose. Now: (a) the body/notes boundary is computed from the first
top-level block that IS or CONTAINS (recursively) a footnotesList/
footnoteDefinition, not just a top-level list; and (b) the insertNodesAfterAnchor
core skips footnotesList/footnoteDefinition subtrees entirely (skipSubtreeTypes),
so an anchor whose only match is inside a definition -> inserted:false (clean
abort, no write). Added tests: nested-definition, bare-definition, and
body-before-nested-list-still-inserts.
- editor-ext footnote-canonicalize header listed `markdownToProseMirror` among the
canonicalizing MCP paths; it is the NON-canonicalizing primitive. Replaced with
`markdownToProseMirrorCanonical` (+ note that the plain primitive is for comment
bodies) and added copy_page_content.
- Client paste: canonicalizePastedFootnotes now skips a definitions-ONLY paste
(no footnoteReference anywhere) — canonicalizing it would strip the
reference-less list and yield an EMPTY paste. Added a test.
Suggestions:
- docmost_transform now runs validateDocStructure/validateDocUrls on the RAW
transform output BEFORE canonicalizeFootnotes (mirrors updatePageJson), so a
too-deep doc gives the intended max-depth error instead of a stack overflow.
- docmost_transform tool description now states the RESULT is footnote-canonical
(dryRun diff may show tidy-ups; idempotent after first run).
- insertFootnote: dropped the dead `result ? … : undefined` ternaries and the
`as any` casts (result is always set by the time we return; the not-found path
throws and aborts mutatePage). `const r = result!;`.
Tests / architecture:
- Added a LIVE-plugin golden case: the real footnoteSyncPlugin leaves a list with
non-empty content after it in place, and canonicalize agrees (placement parity
is now a driven property, not a hand-set expected).
- Added generateFootnoteId uuidv7 shape + uniqueness test.
- Item 9: added the ENFORCEMENT-RULE comments at the server parseProsemirrorContent
and the MCP canonicalizer header (any NEW full-doc persist path MUST canonicalize;
fragments/append/prepend and comment bodies MUST NOT). Kept per-call-site over a
brittle grep CI test (the replace-vs-fragment + comment-vs-page nuance makes a
single wrapper unsafe).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix (REAL DATA LOSS):
- markdownToProseMirror is reused for COMMENT bodies (createComment/updateComment).
It unconditionally canonicalized, so a comment carrying a standalone footnote
definition ([^1]: text with no matching reference) had its whole footnotesList
stripped (referenceIds.length===0 -> stripFootnotesListsDeep) — the text
vanished. Fix: markdownToProseMirror no longer canonicalizes (content-preserving
primitive); a new markdownToProseMirrorCanonical wraps it for the PAGE write
paths (markdown import via importPageMarkdown, update_page markdown via
updatePageContentRealtime). Comment callers keep the non-canonicalizing
primitive. Updated the now-false header comment and added create/update-comment
inline notes. Added collaboration tests: comment path PRESERVES a reference-less
definition; page path still drops it AND still reorders real footnotes. Updated
the page-import canonicalization test to use the canonical variant.
Suggestions / architecture:
- #2: collapsed transforms.footnoteDefinition onto the shared
makeFootnoteDefinition factory (adds only the inner paragraph block id); kept
the dependency direction transforms -> footnote-authoring (no circular import,
mirror stays pure).
- #3: confirmed docmost_transform auto-canonicalization is documented (inline
comment, tool description, CHANGELOG) — no code change.
- #4: copyPageContent is a FULL-document write (replacePageContent of a
type:"doc"); added a defensive canonicalizeFootnotes pass (no-op on
already-canonical source).
- CHANGELOG entry refined to list the FULL-document write paths (incl.
copy_page_content) and to state canonicalization is NOT applied to comment
bodies.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Approve-with-comments follow-ups (no blockers):
- callout: unify the GitHub-callout feature ticket on #192 (the callout-paste
feature the CHANGELOG already tracks); #218 is the public-share security work.
Fixed the code comment and test reference.
- export/utils.spec: pin current behavior of a leading-dot name (".gitignore" ->
"") — same bug class as #204 but unreachable via the sole caller, so document
not change.
- share.types: narrow ISharedPage to the actual /shares/page-info allowlist
(page -> Pick of id/slugId/title/icon/content; trimmed share; dropped the
spurious `extends IShare`). Verified all three consumers (shared-page,
link-view, mention-view) read only allowlist fields.
- editor-ext: extract shared CALLOUT_TYPES / normalizeCalloutType /
renderCalloutHtml into callout-common.marked.ts; both tokenizers
(`:::type` and `> [!type]`) now share the renderer + type dict while staying
separate. Eliminates the byte-identical renderer + duplicated type list.
- share.service: extract named predicate shareIdGrantsAccess(requestedShareId,
resolvedShare) for the id-or-key fast path (naming only, no control-flow
change); kept narrower than resolveReadableSharePage's id-only gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- REAL BUG: insertInlineFootnote could splice a footnoteReference (inline atom)
into a codeBlock or an existing footnoteDefinition, persisting a schema-invalid
doc (insert_footnote skips validateDocStructure). Now the search is bounded to
the BODY (before the first footnotesList) and the insertNodesAfterAnchor core
refuses textblocks that can't hold the atom (codeBlock); when the only match is
in such a place the insert returns inserted:false and the write aborts cleanly.
Reachable via docmost_transform too. Added codeBlock / definition / fall-through
tests.
- Fixed the deepEqualJson doc comment in both copies: arrays are order-SENSITIVE
(correctness depends on it), only object keys are order-insensitive.
- README.ru.md MCP tool count 38 -> 39 (lines 36/47/63), matching README.md/AGENTS.
- CHANGELOG [Unreleased] Added entry for insert_footnote + server-side footnote
canonicalization on non-editor write paths (#228).
Suggestions:
- canonicalize step 5/7 now strips footnotesList at ANY depth (both copies), so a
schema-valid list nested in a callout/blockquote can't leave duplicate defs.
- Exclude the test-only footnote-corpus.ts fixture from the editor-ext build
(tsconfig), so it no longer ships in dist/.
- Removed the duplicate manual canonicalize cases from the MCP unit test (the
shared corpus covers them via full deepEqual); kept idempotence + immutability.
- insertInlineFootnote dedup key now keys off the inline array directly
(footnoteContentKey({ content: inline })) instead of a throwaway node.
Tests / architecture:
- New client-wrapper test (#9): overrides a small mutatePage seam to assert the
not-found path throws and persists NOTHING, and the success path shapes
footnoteId/reused/message/verify and writes the right content. Fixed the
misleading comment in footnote-write.test.mjs.
- B: cross-copy corpus parity guard test (loads both corpora, asserts deep-equal)
so a typo in one copy can't pass both suites green.
- A: declined — the full-vs-fragment decision lives at the call site, so a
prepareDocForPersist wrapper would be a bare alias for canonicalizeFootnotes;
kept the existing per-call-site comments instead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Make footnotes author-inline: the agent/tool inserts a footnote at its point
of use (anchor + text) and the numbering plus the bottom list are DERIVED
deterministically server-side. The agent has no access to footnotesList and
cannot desync — out-of-order lists, orphan definitions, and raw trailing
[^id] blocks become structurally impossible.
editor-ext:
- canonicalizeFootnotes(docJSON) -> docJSON: a pure, EditorView-free port of
footnoteSyncPlugin's end-state. Distinct reference ids in document order are
the source of truth; exactly one trailing footnotesList holds one definition
per referenced id in reference order (reusing the existing node or
synthesizing an empty one); orphans dropped; duplicate definitions resolved
deterministically (first wins, never lost); idempotent.
- Unit tests + a golden parity suite: on every editor-reachable steady state
the live footnoteSyncPlugin's JSON is a canonicalize no-op (byte-for-byte
parity), and the canonicalizer additionally repairs the out-of-order list a
non-editor write produces.
mcp:
- footnote-canonicalize.ts: behavioural mirror of the editor-ext canonicalizer
(the MCP package is intentionally decoupled from the editor barrel, like
footnote-lex/docmost-schema), plus footnoteContentKey for content dedup.
- Auto-canonicalize on EVERY write path: markdownToProseMirror (fixes import
ordering), update_page_json, and after every docmost_transform. Idempotent,
so it is a no-op when footnotes are already canonical.
- insert_footnote tool + insertInlineFootnote: anchor + markdown text -> a
mark-safe footnoteReference and a content-dedup'd definition; the list and
numbering are derived. Same-content footnotes reuse one number/definition.
- canonicalizeFootnotes + insertInlineFootnote exposed as docmost_transform
sandbox helpers.
Tests: editor-ext 157 green; MCP 325 green; server + client tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Additive test coverage across server, editor-ext, client and mcp.
#192 — AiChatService.stream integration (Section 3, against real Postgres):
- new apps/server/test/integration/ai-chat-stream.int-spec.ts drives the real
streamText through a seeded ai/test MockLanguageModelV3 and a real Node
ServerResponse, covering: onError persists an assistant error record
(status 'error' + partial answer + provider cause in metadata); external MCP
client closed exactly once on BOTH onFinish and onError; anti-tamper —
history is rebuilt from the DB transcript, not from body.messages.
#206 — red-team findings (most already fixed+tested in #212):
- mdrt-2 (UNFIXED, data loss): turndown.dataloss.test.ts documents that
pageBreak / transclusionReference / mention are silently dropped on Markdown
export (characterization + it.fails for the desired survive-export contract).
- persist-6 (UNFIXED, data loss): persistence-store.spec.ts adds an it.failing
documenting that a momentarily-empty live doc overwrites non-empty content
(left unfixed — a store-side empty-guard is a behaviour change).
#204 — test-strategy plan, highest-priority subset:
- Phase 1: mcp-clients.lease.spec.ts covers the external MCP client
lease/refcount/eviction lifecycle (leak / premature-close / double-close).
- Phase 2 data-integrity pure functions: editor-ext table-utils
(transpose/moveRow/convert round-trip) and math tokenizer false-positive
guard; client emoji-menu (+ it.fails for the unguarded localStorage
JSON.parse bug), sort-cells, normalizeTableColumnWidths; mcp htmlEmbed/
pageBreak markdown data-loss + footnote-diff; server export
getInternalLinkPageName extensionless-path bug — FIXED (small/clear) + tested.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Public sharing (#218):
- Bind public-share content to the requested shareId. getSharedPage now
enforces dto.shareId (forwarded from /share/:shareId/p/:slug): the page must
be reachable THROUGH that exact share (its own share, or an includeSubPages
ancestor that contains it). A forged/mismatched shareId 404s instead of
rendering off the slug alone and no longer leaks the real canonical key via
redirect. A request with no shareId keeps the legacy slug-capability path.
- Trim /shares/page-info: drop internal metadata (creatorId, spaceId,
workspaceId, contributorIds, lastUpdated*, parent/position, lock/template
flags, timestamps) from the anonymous payload.
- Default share-to-web includeSubPages to false (opt-in), so enabling a share
no longer silently exposes the whole sub-tree (#216).
Editor (#218):
- Harden the new-page pre-sync window: the body editor is kept read-only until
the collab provider is Connected and synced, so early keystrokes can't land
only in local ProseMirror and then be clobbered by the server's empty doc.
- Surface a "Connecting… (read-only)" affordance during the static phase so
input isn't silently swallowed.
Other:
- Breadcrumb: resolve from the page's own ancestor data (/pages/breadcrumbs)
instead of waiting for the lazily-built sidebar tree, so deep pages don't
render a blank breadcrumb for seconds.
- Pasting GitHub `> [!type]` callouts now converts to a callout node instead of
a literal blockquote (new marked extension wired into markdownToHtml).
Tests: editor-sync-state gate (client), getSharedPage share-binding (server),
github-callout markdown conversion (editor-ext).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The proofreader role content was changed (STYLE SHEET block removed) without
bumping its catalog version, so clients never saw an update. Bump proofreader
2 -> 3, and add a content-hash guard so this can't happen silently again.
- index.json: proofreader version 2 -> 3
- scripts/check.mjs: new content-hash guard. A scripts/content-hashes.json lock
maps slug -> { version, hash } (sha256 over emoji/autoStart/name/description/
instructions/launchMessage across all languages). check.mjs now fails when a
role's content changed without bumping its version; the new --update-hashes
(alias --fix) refreshes the lock but refuses to write when a bump is missing.
- check.mjs: also require every index.json role to carry a finite numeric
version (matches the server's catalog validation), with defense-in-depth so a
missing version can't bypass the bump guard.
- scripts/content-hashes.json: new lock artifact (not part of the served catalog).
- README.md: document the guard, the lockfile, --update-hashes, and the
prune-then-readd limitation.
Co-Authored-By: Claude Opus 4.8 <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>
Remove the STYLE SHEET / СТАЙЛ-ШИТ section from the copyeditor
(proofreader) role and clean up all dangling references to it in both
the ru and en editorial bundles:
- description: drop "maintains a style sheet" / "ведёт стайл-шит"
- instructions: remove the STYLE SHEET block
- instructions: drop "record it in the style sheet" mentions in the
WHAT YOU DO and WHEN UNSURE sections
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>
The share modal flagged a custom address already owned by another page with a
red "This address is already in use" error driven by the availability probe.
That reads as terminal even though Save actually triggers the server's
409 `ALIAS_REASSIGN_REQUIRED` and opens the "Move custom address?" confirm
modal that retargets the address to the current page — so the reassign path was
hidden behind what looked like a hard stop.
Replace the red error with an informational description hint ("This address is
in use. Saving will move it to this page.") and keep Save enabled, so the
existing confirm-reassign flow is discoverable. Renaming to a FREE name was
already correct (the probe returns available -> no error -> server renames the
single row in place); this only changes the taken-name presentation.
Verified end-to-end in a real browser against a live stand on this branch:
- A (free rename `test`->`test2`): 200, same alias row renamed in place, link
becomes `/l/test2`, no error, exactly one DB row for the page.
- B (`test2` owned by another page): hint shown (no dead-end error), Save ->
409 ALIAS_REASSIGN_REQUIRED -> "Move custom address?" modal -> confirm -> 200,
the single row retargets, one row each.
- C (same-name re-save): Save disabled (no-op); first-time set inserts.
Add a client component test covering both branches (taken name -> hint not
error + Save enabled; 409 -> reassign modal -> confirm sends confirmReassign).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merge the copy-editor (📐) and proofreader (🧹 "Корректор") editorial roles
into a single role. Keep slug `proofreader`, drop slug `copy-editor`, and set
the merged role's emoji to 📐.
- index.json: remove copy-editor; bump structural-editor, line-editor,
fact-checker, proofreader to version 2 (narrator unchanged); update editorial
bundle description (ru/en).
- bundles/editorial/{ru,en}.json: delete copy-editor; refresh emoji/name/
description/instructions of structural-editor, line-editor, fact-checker and
the merged proofreader verbatim from gitmost-agenty-ru.md / gitmost-agents-en.md;
preserve autoStart and launchMessage; leave narrator untouched.
- README.md: drop copy-editor from the editorial role list.
Validated with scripts/check.mjs (OK).
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>
The right border on the address prefix read as a stray vertical line
between the domain and the slug. Remove it and rely on the subtle
prefix background alone to separate the two parts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR #224 fixed an AI-chat streaming-render regression by moving the React.memo
content signature into the parent: MessageList now snapshots
messageSignature(message) per render and passes it to MessageItem as the
immutable `signature` prop. The existing memo tests only SIMULATED that
parent half by hardcoding `signature={messageSignature(message)}` in their
harness; the real MessageList was never exercised (chat-thread.test.tsx mocks
it out, and there was no message-list.test).
Add message-list.test.tsx that mounts the REAL MessageList (without mocking
MessageItem or messageSignature) and asserts that an in-place mutation of a
reused message object surfaces on re-render. This guards the parent-side
contract: re-caching the signature on message identity (stable across deltas
while parts mutate) would refreeze the row, and this test would fail.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add fast note-creation entry points alongside the existing space-sidebar
actions.
- Home: refactor new-note-button.tsx into a reusable inner CreateNoteButton
(parametrized by `temporary`/label/icon, keeps the 0/1/many writable-space
resolution and space-picker dropdown) and render two equal-width buttons via
`Group grow` — a regular note and a temporary note (IconHourglass).
- Space overview: new SpaceCreateNoteButtons component with two buttons that
create a regular/temporary note directly in the current space and open it,
reusing useTreeMutation.handleCreate (optimistic sidebar-tree insert +
navigation). Permission-gated to members who can manage pages; a local
pending state shows a per-button spinner and disables both to prevent a
double-create. Wired into space-home.tsx above the tabs.
- Reuse existing i18n keys (no new strings): "New note", "New temporary note",
"Create in space".
- Docs: add a CHANGELOG [Unreleased] entry and a "Temporary notes" roadmap
bullet to README.md and README.ru.md.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
