First step of docs/git-sync-plan.md. New workspace package @docmost/git-sync
vendoring the PURE parts from docmost-sync (HEAD b03eb35):
- lib: markdown-converter, markdown-document, canonicalize, docmost-schema,
node-ops, diff, and an extracted markdown-to-prosemirror (only the pure
marked->HTML->generateJSON path from upstream collaboration.ts; no websocket).
- engine (pure, no IO): reconcile, layout, sanitize, stabilize, loop-guard.
Ported the upstream pure-module + round-trip corpus tests (vitest): 314 pass,
3 expected upstream known-limitation fails. tsc clean. No server wiring yet.
docmost-schema inlines getStyleProperty (as packages/mcp does — @tiptap/core
3.20.4 doesn't export it). IO engine (pull/push/git/settings) deferred to later
Phase A/B steps; the editor-ext idempotency gate (plan §13.1) is the next step.
Co-Authored-By: Claude Opus 4.8 <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>
Update the release documentation to emphasize tagging on develop before merging to main, detail steps for pushing tags to both gitea and github, and explain the back‑merge and remote tag considerations.
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>
Remove top margin from the first paragraph in footnote definitions so the
marker aligns with the first line of text. Adjust the task‑list label to use
the editor line‑height variable for its height and center the checkbox
vertically, keeping it in line with the item’s first text line.
With the image fix in place, the mcp e2e ran through every section and
failed only at the last one (comments): create_comment was hardened to
require an inline "selection" (exact text to anchor on) for a top-level
comment, but the test created one without a selection ("an inline
'selection' ... is required for a top-level comment").
Pass an inline selection ("Добавленный абзац.", a plain paragraph
re-imported in section 5 and still present at the comments stage). The
reply is unchanged: it carries a parentCommentId, so it is a reply and
needs no selection.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
The controller's buildPageSlug truncates the page title via
`title?.substring(0, 70)` before slugifying, but no test drove that
branch (the only titled case was 16 chars). Add a resolvable-alias
case with a 119-char title whose 70-char boundary falls mid-word and
assert the 302 target's slug reflects only the first 70 characters.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-row MCP Test button derived its presentation solely from the test
mutation's data ({ ok, tools } | { ok, error }). When the request itself
rejected (401/403/500/network) there is no payload, so the row silently spun
back to the idle "Test" instead of reporting the failure.
Feed the mutation error into mcpTestButtonView so a reject also renders a red
"Failed", with the tooltip taken from the server message
(error.response.data.message) or a generic i18n fallback. Enable the tooltip
for any non-idle state. Cover the reject branch (with and without a server
message) in the helper unit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code-review follow-ups (Approve-with-comments) for batch #197
(context badge #189 / e2e in CI #187 / inline MCP test #170):
- server: extract the duplicated chatContextWindow ::text->positive-int
coercion (resolve() + getMasked()) into an exported parsePositiveInt
helper and unit-test its branches (200000/1.9/0/-5/""/abc/undefined),
closing the untested read-path gap.
- client: merge the two backward scans over messageRows into one pure,
exported selectContextBadge helper (numerator and denominator still
taken from the most recent row carrying EACH value) and unit-test the
different-rows and fresh-zero-doesn't-shadow cases.
- client: extract the MCP "Test" button tristate presentation into a pure
mcpTestButtonView helper (collapses the two parallel if/else chains) and
unit-test idle/ok-with-tools/ok-no-tools/failed label+tooltip branches.
- ci: redirect the backgrounded prod server's stdout/stderr to a log file
in e2e-mcp and cat it on failure, so a start-up crash is diagnosable
instead of surfacing only as the generic health timeout.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the two blocking test-coverage specs requested in the PR #214 review and
clear the cheap non-blocking items.
Must-fix:
- share-alias-redirect.controller.spec.ts: routing/leak guard for the public
GET /l/:alias resolver (modeled on share-seo.controller.routing.spec). Pins
302-to-canonical on a hit; SPA index without a 302 for unknown/dangling/
unreadable aliases and a null workspace (no name-existence leak); defensive
percent-decoding treated as unknown; self-hosted findFirst vs subdomain
findByHostname workspace resolution; 404 when no built client index exists.
- share-alias.controller.spec.ts: authz gates with mocked PageRepo/ShareService/
ShareAliasService/PageAccessService. Covers cross-workspace/nonexistent page
-> NotFoundException, validateCanEdit, resolveReadableSharePage null ->
BadRequestException, isSharingAllowed false -> ForbiddenException, set happy
path delegation, remove() of a dangling alias (pageId null) skipping
validateCanEdit but still deleting, and for-page validateCanView.
Cheap review items:
- Remove dead Logger import/field from ShareAliasRedirectController.
- Remove dead PagePermissionRepo import/dependency from ShareAliasController.
- Register the new share-alias UI strings in en-US and ru-RU catalogs.
- Add an [Unreleased]/Added CHANGELOG entry for /l/:alias (#205).
- Drop the tautological boilerplate assertions from the migration spec
(exports up/down; runtime checks of typed entity literals).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
onFinish always receives a totalUsage object, so the `?? {}` guard and
optional chaining were dead. Extract the field-level extraction into a
recordTurnUsage method (totalTokens, else input+output) and unit-test that
recordShareTokens receives the summed value when totalTokens is absent and the
authoritative total when present.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A colliding transclusionSource id is deliberately NOT reassigned (its id is a
cross-reference key), while a missing id is still filled. Add coverage for both:
two sources sharing an id keep it (red if the NO_REASSIGN guard is removed), and
a source with no id gets a fresh one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the new per-workspace rolling-day token-budget env var in
.env.example alongside the existing share-assistant cost knobs, and add
[Unreleased] Fixed entries for #161/#190/#207/#206 plus a Security entry
for the #159 token budget.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address non-blocking review items on the AI-chat stream-perf PR:
- Drop the unused `metadata` param from the `msg` test factory in
message-item.test.ts; no caller passed it.
- Add a per-part-kind coupling guard to message-signature.test.ts that, for
each part kind rendered today (text, reasoning, tool-*) plus the metadata
banners, asserts that mutating a field the MessageItem render body DRAWS
flips messageSignature — an executable lock for the load-bearing memo
invariant documented in message-signature.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a retargetable, human-readable vanity link namespace /l/<alias> that
sits alongside the untouched /share/... routes.
- New share_aliases table (workspace-scoped, UNIQUE(workspace_id, alias),
page_id nullable ON DELETE SET NULL so the address outlives its target).
- ShareAliasRepo + ShareAliasService (create / no-op / 409 reassign guard /
availability / request-time readable-target resolution through the single
existing share boundary).
- Public ShareAliasRedirectController (GET /l/:alias) issues a 302 (never 301,
the target is mutable) to the canonical /share/:key/p/:slug page; unknown /
dangling / no-longer-readable aliases serve the SPA index with no leak.
'l/:alias' excluded from the global /api prefix.
- Authenticated ShareAliasController (set/remove/availability/for-page).
- Shared ASCII-only normalize/validate util (server + client copies).
- Client: Custom address block in the share modal (live normalize + debounced
availability + copy + reassign confirmation dialog).
- Unit tests: util, repo SQL-shape, service semantics, migration/entity sanity
(server jest) + client alias util (vitest).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The anonymous public-share assistant only capped the COUNT of requests
(100/hour/workspace), not their cost. One accepted turn runs the agent loop
up to stepCountIs(5), re-sending the whole client-held transcript as input on
every step, while maxOutputTokens caps only the output; the request window is
hourly with no daily ceiling, so a steady stream at the cap sustains ~24x its
count per day. Counting requests therefore does not bound the owner's LLM bill
(red-team finding #5).
Add a second cost contour: a cluster-wide, sliding-window per-workspace TOKEN
budget over a rolling day. It is checked read-only BEFORE a turn streams (429,
no request slot consumed, nothing spent) and the turn's real usage
(totalUsage: input re-sent per step + output, summed across all steps) is
recorded once it finishes via streamText onFinish. Fails closed on the check
(deny when Redis can't prove we're under budget); best-effort on the record.
Env-overridable via SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY (default 1M/day).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
editor-pm-7: addUniqueIdsToDoc only FILLED missing ids and never deduplicated
existing ones, so a copy/paste or bulk-JSON duplicate that kept its attrs.id
produced two nodes sharing an id. MCP addressed edits (patch_node /
delete_node "before/after id") then hit the wrong node or both.
Walk the configured-type nodes in document order: the first occurrence of an
id keeps it (stable anchor), later duplicates are reassigned a fresh id.
transclusionSource ids are cross-reference keys (references resolve a source by
this id), so they are only filled-when-missing, never reassigned, to avoid
orphaning their references.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
persist-1: onStoreDocument wrapped the page write in a try/catch that only
logged and swallowed the error, then resolved "successfully". hocuspocus
destroys/unloads the in-memory Y.Doc right after the hook resolves (the only
copy of the latest edit), so a transient DB error (deadlock, serialization
failure, dropped connection) silently lost the edit. Worse, the post-store
branch ran on the partially-assigned `page`, broadcasting a phantom
"page.updated" and enqueueing a history snapshot for content never written.
Wrap the write in a small bounded retry (3 attempts) so the save is
re-attempted while we still hold the doc, and clear `page` on failure so the
success-only side effects never report a save that didn't happen.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ui-state-races-1: the server-authoritative move path (placeByPosition, via
applyMoveTreeNode) lacked the isDescendant cycle guard that drag-drop `move`
has. When move events arrive out of order so the destination parent is still
nested inside the moved node's own subtree, remove(source) dropped the whole
subtree (incl. the future parent) and insertByPosition could not re-place it —
the node and all descendants silently vanished with no error/refetch.
Add the isDescendant guard to placeByPosition (returns same ref, like its other
no-op cases) and short-circuit applyMoveTreeNode on the same condition BEFORE
the placed===prev remove-fallback (which would otherwise still drop the
subtree). Leave the tree untouched so a later corrective event / reconnect
reconcile fixes it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
attach-1: when the same attachmentId was referenced by more than one page
in a duplicated subtree, the per-attachmentId map held only a single copy
entry, so the last page processed clobbered the others. The downstream
ownership guard (`attachment.pageId !== oldPageId`) then matched at most one
page and skipped the lone DB row entirely: no blob copied, no new row, every
copy's image 404'd. Key the map to a list of entries and copy one blob/row
per referencing page; drop the now-incorrect ownership guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getPageBreadCrumbs (ancestor CTE) and forceDelete (descendant CTE) used
withRecursive + unionAll with no CYCLE clause or depth cap. If a parent/child
cycle already exists in the data (e.g. one slipped in via the #7 TOCTOU race),
both queries loop forever — hang / statement timeout. Worse, the move guard
itself runs the ancestor CTE, so a cycle would disable the very guard meant to
prevent it (#207#8).
Add a depth counter bounded by MAX_PAGE_TREE_DEPTH to both recursive CTEs; the
walk stops at the cap, so a cycle yields a bounded result instead of hanging.
Real page trees are only a few levels deep, so the cap never truncates a
legitimate result. getPageBreadCrumbs selects an explicit column list (not
selectAll) so the internal depth counter never leaks into the breadcrumb shape.
Adds an integration test that seeds an A<->B cycle directly and asserts both
getPageBreadCrumbs and forceDelete return bounded / complete under a short
connection-level statement_timeout instead of hanging.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The server-side move cycle guard (getPageBreadCrumbs) and the move UPDATE ran
as two separate, unlocked statements. Two concurrent moves ("A under B" and
"B under A") could each read the same pre-write acyclic snapshot, both pass the
guard, then persist A.parentPageId=B AND B.parentPageId=A — a parent/child
cycle (TOCTOU, #207#7).
Run the cycle check and the UPDATE inside one transaction (executeTx) guarded
by a per-space advisory lock (pg_advisory_xact_lock, held until COMMIT) so all
moves within a space serialize: the second mover blocks until the first commits
and then sees the freshly written parent, so its guard rejects the cycle.
getPageBreadCrumbs gains an optional trx so the check runs on the locked snapshot.
Adds an integration test driving two opposing concurrent movePage calls and
asserting no cycle ever persists and exactly one move is rejected. Updates the
movePage unit-test stubs for the new transactional path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In-app AI-chat tools used bare zod schemas, so when the model dropped a
required arg (typically pageId) in a parallel/batch tool call, the AI SDK
forwarded zod's raw "expected string, received undefined" text to the model
— not actionable. Add a centralized modelFriendlyInput(shape) wrapper that
keeps the exact JSON Schema contract (required/description/constraints via
z.toJSONSchema draft-7) but replaces the raw zod text with a message naming
each missing/invalid parameter and reminding the model not to drop ids like
pageId in parallel batches. No value is guessed/backfilled (cf. #159).
Applied to every in-app tool: the sharedTool() builder and all inline
inputSchema in ai-chat-tools.service.ts, plus public-share-chat-tools.service.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #182 review (post-fix pass) surfaced two latent correctness risks in the
new MessageItem memo: the per-message signature tracks only [type, text length,
state, error/output presence] + metadata, so a part kind whose VISIBLE content
can change WITHOUT changing those fields would silently freeze a stale row.
Neither is reachable with the current toolset (tool output is set once;
streaming is append-only with a fixed id), so the correct fix is to harden the
documented invariant rather than hash output content on every delta (getPage
returns full page content — hashing it per-delta would tax the hot path this
PR optimizes).
Add a WARNING in messageSignature naming the two future triggers (a tool that
streams `preliminary` output; a client-side regenerate/edit that mutates a
finalized row in place) and the required action (extend the signature).
No behavior change (comment only). vitest src/features/ai-chat 189/189 pass,
tsc clean for the touched files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removed six outdated markdown files from the `docs/backlog` and other docs directories that were no longer relevant to the project. This cleans up the repository and reduces clutter.
The floating chat window's header badge flipped meaning — a live per-turn token
counter while streaming, the persisted context size at rest — so it "reset to 1"
on each prompt and conflated two different numbers. Replace it with a stable
"current / max" context badge (e.g. `572 / 200k`). The live "Thinking · N tokens"
inside the chat body stays; only the duplicate live counter is removed from the
header.
Max comes from a new admin setting "Context window (tokens)". The server resolves
it and attaches `maxContextTokens` to the completed assistant turn's metadata
(next to contextTokens), so the badge needs no client-side model resolution and
this survives public shares / per-role models.
Server:
- ai.types: chatContextWindow on AiProviderSettings + PROVIDER_SETTINGS_KEYS +
ResolvedAiConfig + MaskedAiSettings.
- workspace.repo: chatContextWindow in AI_PROVIDER_SETTINGS_ALLOWED (parity).
- update-ai-settings.dto: @IsInt @Min(0) chatContextWindow.
- ai-settings.service: coerce the ::text-stored value to a positive int in
resolve()/getMasked().
- ai-chat.service: flushAssistant writes metadata.maxContextTokens (>0); the
completed turn passes resolved.chatContextWindow.
Client:
- ai-chat.types: maxContextTokens on the message-row metadata.
- ai-chat-window: read maxContextTokens; render "current [/ max]"; drop the
liveTurnTokens state/branch and the onLiveTurnTokens prop; new tooltip.
- chat-thread: remove the live-turn-token throttle effect and plumbing.
- count-stream-tokens: drop the now-dead liveTurnTokens()/types; keep
estimateTokens.
- settings: chatContextWindow on IAiSettings(+Update) + a NumberInput in the AI
provider settings form.
i18n: add the badge/settings keys (en, ru); remove the two now-unused keys.
Tests: flushAssistant maxContextTokens, DTO validation, trim token tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the PR #182 code-review (Request changes) on top of the already-merged
develop (the merge commit preserves both the markdown useMemo and the
collapseBlankLines fix in reasoning-block.tsx).
- Extract messageSignature from message-item.tsx into utils/message-signature.ts
(matches the feature's "pure UIMessage helper + colocated test" convention) and
export arePropsEqual so the memo seam is unit-testable. No logic change.
- Add utils/message-signature.test.ts covering every change signal (text grows,
part appended, state flip, output appears, errorText appears, usage.reasoningTokens
arriving on finish-step, metadata error/finishReason) plus the negative
content-identical-clone case.
- Add components/message-item.test.ts for arePropsEqual (each prop diff -> false,
identity fast-path -> true, same-content-different-object -> true, changed -> false).
- Add components/message-item-memo.test.tsx: render-level proof that finalized text
parts are not re-parsed when only a tail part grows (MarkdownPart memo).
- CHANGELOG: add the user-facing 100% CPU freeze fix under [Unreleased] / Fixed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a per-row Test button to the external MCP servers list that shows the
connection result inline (no toasts). Extract the row into AiMcpServerRow so
each row owns its own useTestAiMcpServerMutation instance — independent loading
and result, no cross-row flicker.
States: idle (Test), pending (loading), success (green, "OK · N" with the tool
count), failure (red, "Failed"); a tooltip shows the tool list or the error.
The result resets when url/transport/headers change (the row is keyed by id, so
it does not remount). Backend, service and mutation are unchanged.
- ai-mcp-servers.tsx: AiMcpServerRow + Test button + reset effect + tooltip.
- i18n: add Failed / "OK · {{n}}" (en, ru) and ru Test / tool-list keys.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add two independent jobs to develop.yml — e2e-server and e2e-mcp — that run on
each push to develop alongside test/build. `build` stays `needs: test` only, so
a failing e2e never blocks the :develop image build/publish; the red run plus
GitHub's email to the pusher is the notification.
- e2e-server: pgvector + redis services, migrations, apps/server test:e2e.
- e2e-mcp: build editor-ext/server/mcp, migrate, start the prod server
(REST + /collab in one process), wait for /api/health, seed the admin via
/api/auth/setup, then run @docmost/mcp test:e2e.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Approve-with-comments auto-review (8 axes); no blockers. Closes the two flagged
test gaps; the two forward-looking dedup suggestions (reconcileHasChildren helper;
unifying reconcileChildren/mergeRootTrees) are non-blocking architecture notes and
left for a follow-up (as with #186's forward-looking point).
1. Ambiguous-id refusal end-to-end (#159): the patch_node/delete_node guard
`if (replaced/deleted !== 1) return null` was only covered in pieces — the
replaceNodeById/deleteNodeById counts and assertUnambiguousMatch in isolation —
so loosening the guard would not have failed a test. New mock test stands up a
REAL Hocuspocus collab server seeded (via buildYDoc, same docmost extensions)
with a two-blocks-one-id document and drives the real client methods: both must
reject with /ambiguous/ AND never write to collab. Tracked via Hocuspocus
onChange (fires synchronously per update, unlike the debounced onStoreDocument)
so a clobbering write is actually observed — verified the test FAILS when the
guard is loosened to `< 1`.
2. scrollToReference zero-match bail: the branch "non-empty id but querySelectorAll
returns 0 -> matches[index] ?? matches[0] is undefined -> return false" (the real
desync: definition present, inline ref removed from the DOM) was uncovered. Added
a footnote.test.ts case: a definition for 'ghost' with no rendered ref -> false,
no scroll.
Verified: 313 mcp tests + 24 editor-ext footnote tests; prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Approve-with-comments re-review; no blockers. All 7 actionable points (8 is a
forward-looking architecture note — recommendation A, keep as-is):
1. chat-markdown.util spec: restore parity coverage of the removed client spec —
tool error state (+ errorText), unknown-tool fallback (`Ran tool <name>` en /
`Выполнил инструмент <name>` ru), and the circular-output stringify catch.
2. findAllByChat row cap is now testable (injectable limit) + an int-spec proves
truncation on a modest volume.
3. Stability: the per-step durability updates are SERIALIZED via a promise chain
(stepUpdateChain) so they commit in step order — onlyIfStreaming already
closed the finalize race, this closes inter-step ordering.
4. findAllByChat keeps the NEWEST messages on truncation (order DESC + reverse,
like findRecent) and logs a warning with chatId, instead of silently dropping
the newest tail.
5. The LABELS parity comment already references the real path (tool-parts.tsx /
toolLabelKey) — confirmed accurate.
6. Removed the redundant 'off-by-one boundary' test (strict subset of the two
adjacent prepareAgentStep cases).
7. Extracted the terminal-finalize dispatch into a shared `applyFinalize`, used
by BOTH the service's finalizeAssistant and its test — the test now exercises
the real path, not a copy, so a production drift fails it.
Verified: server build + 325 ai-chat unit + 6 integration; prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The re-review's blocking/structural points (lease leak, dup-id guard test,
body-before-title test, CHANGELOG, pg18, shared jsonb decoder) were already
addressed in commit 24264ef; this adds the 4 genuinely-new coverage requests:
- pt 6: `scrollToReference(id, index?)` exercised against a live editor DOM —
selects the index-th `sup[data-footnote-ref][data-id]` occurrence, falls back
to the first for out-of-range, returns false for an empty id (scrollIntoView
stubbed). (#168)
- pt 7: export `backlinkLabel` and pin the base-26 carry boundary
(25->z, 26->aa, 27->ab, 51->az, 52->ba). (#168)
- pt 8: integration fail-open — a PRESENT-but-corrupt tool_allowlist (jsonb
string scalar holding non-array JSON) reads back as null ("no restriction"),
covering normalizeRow's degrade branch. (#159 #172/#173)
- pt 9: getFootnoteRefCount cache invalidation — adding a `[^a]` reference bumps
the cached count 2 -> 3. (#168)
Verified: editor-ext footnote 23; client structure 7 + tsc; server int 8.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
