Схлопнутая дельта feat/git-sync-2 поверх актуального develop (3085ec1b).
Почему squash, а не буквальный rebase: в ветке 113 коммитов, из которых
develop уже впитал ~80 в курированном виде при унификации конвертера
(#326/#293) — но не patch-identical, поэтому они не отваливаются сами; а
коммит, УДАЛЯЮЩИЙ вендоренную копию конвертера, апстримный и в replay-набор
не входит, так что наивный replay заново добавил бы вендоренные копии.
Поэтому корректный итог — «develop + чистая net-дельта ветки», сведённая
3-way мержом (merge-base 5336f06d).
Net-дельта: серверный git-sync модуль (GitSyncModule/orchestrator/HTTP),
движок git-sync (layout/reconcile/pull/push/stabilize + QA), два фикса
round-trip/data-loss конвертера поверх УНИФИЦИРОВАННОГО
@docmost/prosemirror-markdown (вендоренной копии больше нет — git-sync
целиком на унифицированном конвертере, поведение унифицированного принято
за эталон), e2e-скрипты, доки.
Конфликты (4) слиты объединением: main.ts (метрики + GitHttpService),
apps/server/package.json (pretest-суперсет; moduleNameMapper: git-sync→src
и .js-strip оставлены, а bare-specifier @docmost/prosemirror-markdown→src
УБРАН — серверный jest выровнен на develop-подход #345 со сборённым
пакетом), .env.example (метрики + GIT_SYNC блоки), AGENTS.md (строки про
пакеты; устаревшая заметка про «три hand-synced копии схемы» в git-sync
поправлена — схема теперь только в @docmost/prosemirror-markdown).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Once the ESM transform fix (8e125799) let the four previously-unparseable
int-specs run, the server integration suite stopped exiting: all 66 tests pass,
then jest prints "Jest did not exit one second after the test run has
completed." and idles until the 20-minute job timeout kills it. Separately,
ai-chat-stream.int-spec.ts failed because its afterAll (destroyTestDb) hit the
60s hook timeout — postgres.js .end() waits for in-flight queries forever, so a
leaked/stuck pooled connection hung teardown.
Pragmatic unblock (the underlying open-handle leak is tracked in #382):
- jest-integration.json: add forceExit so jest always exits after the run even
if a suite leaves an open handle.
- db.ts: capture the singleton's raw postgres sql instance and bound the pool
shutdown with sql.end({ timeout: 5 }) (the same bounded-end pattern already
used in global-setup.ts) instead of Kysely.destroy(), so destroyTestDb can no
longer hang the afterAll hook.
forceExit masks residual handle leaks rather than fixing them; the proper
investigation (run with --detectOpenHandles on a pg+redis stand, close the
leaking timers/sockets, then drop forceExit) is filed as #382.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The develop build failed in two jobs, both rooted in the ESM-only workspace
package @docmost/prosemirror-markdown (type: module, built to build/index.js
with `export * from`), which the server imports at runtime (collaboration.util,
page.service). Refactor #345 taught only the unit jest config (package.json
"jest" key) to consume it, leaving the integration and e2e configs — and the
e2e-server CI job — broken:
- `pnpm --filter server test:int` -> SyntaxError: Unexpected token 'export'
(jest did not transform prosemirror-markdown/build/*.js).
- e2e-server job -> TS2307 Cannot find module '@docmost/prosemirror-markdown'
(the package was never built in that job).
Mirror the proven unit config into the two failing jest configs and add the
missing build step:
- jest-integration.json / jest-e2e.json: add a babel-jest transform rule for
`prosemirror-markdown/build/.+\.js$` (before the ts-jest rule so it wins) and
add @docmost/prosemirror-markdown to the transformIgnorePatterns allowlist so
the pnpm-symlinked package is transformed instead of ignored.
- develop.yml: build @docmost/prosemirror-markdown in the e2e-server job (after
editor-ext, before migrations), like the test.yml job already does.
Verified locally: an isolated spec importing the package fails with the exact
SyntaxError under the old config and passes under the new one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 нейтрализация ловила только spaced-разделители (`- - -`),
но пропускала сплошные `---`/`***`/`___`: на git-sync round-trip такая
строка становится horizontalRule, а он не несёт текста — строка терялась
целиком (хуже list/quote-порчи). Символ `_` вообще отсутствовал в regexе.
GITMOST_MD_BLOCK_TRIGGER_RE дополнен альтернативой на целую строку-
thematic-break `([-*_])(?:\s*\1){2,}\s*$` (3+ одинаковых `-`/`*`/`_`,
solid или через пробел); прежние группы переведены в non-capturing,
чтобы `\1` ссылался на единственную capture-группу. Обе копии regexа
(bridge и pm-markdown тест) синхронны.
Тесты: bare `---`/`***`/`___` документированы как text-losing
horizontalRule; ZWSP-нейтрализованная форма round-trip'ится параграфом
с сохранённым текстом. Кейсы падают на старом regexе.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review found two issues:
- [regressions] Inserting a transcript line VERBATIM as a paragraph is unsafe on the
git-sync doc->markdown->doc round-trip: the paragraph serializer emits text with no
block-escape, so a line starting at col 0 with `- `/`> `/`# `/`1. `/```` ``` ````/`|`/
`> [!info]` silently re-parses into a list/quote/heading/code/table/callout (the last
hits the #359 callout machinery). Safe under the host contract (every line prefixed
`You:`/`Speaker N:`, which starts with a letter) but the helper didn't enforce it, and
leading-whitespace / stray lines exist. Fix: trim each kept line (drops the indent leak),
and if it STILL begins with a col-0 markdown block trigger, prepend an invisible
zero-width space (U+200B) so the trigger isn't at col 0 — the round-trip keeps it a
paragraph. (Backslash-escape was rejected: `marked` consumes the `\` on re-import, so it
would render visibly then vanish. ZWSP is invisible and never markdown-escaped.) The
serializer's missing block-escape is the pre-existing root cause; this is the boundary
defense. Prefixed transcript lines never match the trigger regex → left byte-exact.
- [test-coverage] The "inserted as TEXT, not HTML/markdown" contract wasn't locked (all
test strings were plain alphabet). Added a test that inserts `<b>bold</b>
<script>alert(1)</script> and *stars* and [link](x)` (with Bold/Italic/Link marks
registered so an insertContent(html) regression would parse them) and asserts one
verbatim text node, no marks, and getHTML() has no live tags.
Verified: apps/client tsc --noEmit 0 errors; gitmost bridge tests 4 passed; a NEW
prosemirror-markdown round-trip test (real convertProseMirrorToMarkdown ->
markdownToProseMirror) proves bare triggers corrupt while the ZWSP-neutralized form stays
a single byte-preserved paragraph and normal You:/Speaker N: lines round-trip byte-exact —
3 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The native gitmost.app (stage 2) now sends a `transcript` field to
window.gitmost.createPageWithRecording, but the web bridge ignored it — the page
was created with audio only. Insert it.
- gitmost-recording.ts: add `transcript?: string` to GitmostCreatePagePayload
(plain text, \n-separated `You:` / `Speaker N:` lines, ready to insert). Add a
gitmostInsertTranscriptIntoEditor(editor, transcript) helper: a "Transcript"
heading + one paragraph per non-empty line, each line inserted VERBATIM as a
text node (never HTML → no injection), appended at doc end (below the audio).
No-op when transcript is undefined/empty/whitespace-only/non-string.
- gitmost-global-bridge.tsx (createPageWithRecording): after the audio insert
succeeds, call the helper inside a try/catch — best-effort, so a transcript
failure can never turn the already-successful recording into an error (logs +
still returns ok). Absent transcript → audio-only page, exactly as today.
DoD: recording with speech → page has audio AND a labeled transcript block;
without transcript → unchanged. Closes the web-side dependency of gitmost-app
stage 2. Verified: apps/client tsc --noEmit 0 errors; 2 unit tests
(transcript present → heading+paragraphs; undefined/""/whitespace/number/object
/null → no-op, doc unchanged).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The {search-queue} BullMQ queue had producers on every page/space/workspace change
but NO consumer/@Processor — it lives in Docmost's EE edition (external search
drivers), never in this fork. Search works independently via the pages tsvector DB
trigger; the queue was pure dead weight, growing forever (1902 stuck jobs in Redis,
the first real hit of the #355 queue-growing alert — which fired correctly).
Remove the plumbing:
- the 3 producers: drop @InjectQueue(SEARCH_QUEUE) + every searchQueue.add(...) from
page/space/workspace.listener.ts (and the isTypesense() gate that only wrapped the
search enqueue). page.listener.handlePageUpdated did ONLY the search enqueue, so its
@OnEvent(PAGE_UPDATED) handler is removed; the other handlers keep their aiQueue.add.
- the registration (queue.module.ts) and the metrics injection + 'search' depth-metric
entry (metrics-bull.service.ts).
- the constants: QueueName.SEARCH_QUEUE + the 6 unused SEARCH_INDEX_* QueueJob members
(grep-confirmed unreferenced). Shared QueueJob members (PAGE_*, SPACE_DELETED,
WORKSPACE_DELETED — used by the AI queue / embedding/history/notification processors)
are kept.
Search (tsvector trigger, search.service) and the PAGE_UPDATED websocket listener are
untouched. Verified: apps/server tsc --noEmit 0 errors; 6 spec suites / 52 tests green.
Ops (maintainer, out of code scope): one-time Redis cleanup
`redis-cli --scan --pattern "bull:{search-queue}:*" | xargs -r redis-cli del`, then
confirm bullmq_queue_depth{queue="search"} stays 0 (the metric no longer injects it).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The patch forks createOutputTransformStream: output==null skips partialOutput
(the OOM fix, already tested), output!=null preserves the original cumulative
accumulation. Only the skip branch was tested; the preserve branch — on which the
patch's "byte-identical when an output strategy is set" safety claim rests — had no
coverage, so a future re-port (patches are re-created via `pnpm patch` on every ai
bump) could silently route output-set calls into the skip branch and leave
partialOutput empty for object/text-output consumers, uncaught.
Add a 4th test: streamText({ ..., experimental_output: Output.text() }), drain
textStream, collect experimental_partialOutputStream, and assert it is non-empty and
cumulative (last partial == full text "Hello, world!"). Reuses the existing
makeModel() harness. Verified on the patched dist: partials are
["Hello","Hello, ","Hello, world!"]. `npx jest ai-sdk-partial-output.patch.spec.ts`
→ 4 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1 [WARNING] bundlePhase returned 'allInstalled' when a bundle's only
non-installed role was skipped (0 installed for it), so the collapsed green 'All
installed · up to date' header contradicted the open 'Installed 0 · 1 skipped'
plaque. It now returns 'mixed' whenever a skipped role is present. Fixed the test
that encoded the wrong behavior.
F2 [WARNING] The reason->action branch (name-conflict -> transient overlay +
'Rename & install'; already-installed -> informational, no button) lived only in
the component, untested. Extracted the two decisions into pure, unit-tested
helpers nameConflictSlugs() and partialOffersRename() and wired them into the
modal; both reason values are now covered.
F3 [low] Removed the unused useRef import (client eslint no-unused-vars is off, so
it shipped silently).
F4 [low] Extracted bundleCounts() as the single tally pass; bundlePhase and the
panel both derive from it instead of rescanning the roles array ~5x per render
(the same model<->component consolidation this PR is about).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F9 [WARNING] The line-anchored front-matter regex from round 2 requires a bare
LF after the opening `---`, so a Windows/CRLF foreign file (`---\r\n...`) slips
past the strip and leaks its front-matter into the body (where `title: Foo`
renders as a setext heading that title extraction hijacks). The canonical parser
whose regex shape this copied (page-file.ts) normalizes CRLF -> LF BEFORE its
FRONTMATTER_RE; the import path copied the regex but missed the normalization.
normalizeForeignMarkdown now replaces CRLF with LF first (which also makes
convertReferenceFootnotes' split('\n') consistent). Adds a CRLF fixture.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Integrates the designer-handoff Roles Catalog modal, wired to the real API; the
parent ai-agent-roles.tsx and the { opened, onClose, roles } contract are
unchanged.
- Server importFromCatalog now returns per-role lists (createdRoles /
skippedRoles with a reason) alongside the existing counters (compat-preserving),
so the UI can name the conflicting/installed roles.
- New pure view-model (catalog-bundle-model.ts): bundlePhase (empty | allNew |
allInstalled | updates | mixed, ignoring the transient 'skipped'),
installedLangForRole (same-slug-different-language hint), mapCatalogRoleToView —
all unit-tested without mounting.
- Bundle cards with a summary status in the collapsed header (eager useQueries
fan-out over all bundles, sharing the existing per-bundle cache keys), a single
primary action per bundle, checkboxes + select/deselect-all, an inline result
plaque that keeps the modal open, per-bundle and global 'Update all' request
series with progress, and the other-language hint.
- The partial-result plaque distinguishes the skip reason: only a name-conflict
offers 'Rename & install'; an already-installed race is informational (a rename
re-import would just skip again and self-heal into a false success).
- All strings i18n'd (en/ru); mock handoff code (SEED/mockImport/delay) removed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F7 [CRITICAL] The round-1 F2(a) fix built ONE alternation regex over all
definition ids (`(id1|id2|...)`). On prefix-chain ids (a, aa, aaa, ...) V8's
regex compiler blows its stack with a fatal, UNCATCHABLE 'RegExpCompiler
Allocation failed' that kills the whole process — strictly worse than the
original per-def thread-hang, and its match cost was still O(text x defs).
Replaced with a single FIXED generic scanner `/\[\^([^\]]+)\]/g` plus a map
lookup in the replacer: genuinely O(total text), no per-document regex
compilation, cannot blow up. Output is identical (only real def ids are inlined).
F8 [WARNING] The frontmatter strip regex was not line-anchored: it closed on the
FIRST `---` anywhere, so a value containing a triple-dash (e.g.
'title: Q1 --- Q2') truncated the frontmatter and leaked the rest into the body.
Replaced with the line-anchored shape the canonical parser already uses
(page-file.ts): open on `---\n`, close on a `\n---` line.
Adds tests: 4000 prefix-chain ids do not crash and stay fast; a frontmatter
value containing '---' is stripped whole.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the round-1 review of #369:
F1 [CRITICAL] Restore prom-client. The prior commit removed it as a 'stray dep',
but metrics.registry.ts imports it unconditionally at startup (main.ts boot), so
a clean frozen install had no prom-client -> server tsc TS2307 + boot crash. It
was surviving only via hoisting from a warm store. Restored to apps/server
dependencies + regenerated the lock (prom-client/tdigest/bintrees return),
keeping the @docmost/prosemirror-markdown dep. Verified: clean frozen install ->
require.resolve('prom-client') ok, server tsc EXIT 0.
F2 [HIGH] Two quadratic ReDoS vectors in foreign-markdown.ts on untrusted import
(runs synchronously on the request thread, 30MB cap):
(a) pass-2 was O(lines x defs) — a per-def RegExp rebuilt and run over every
line. Replaced with ONE precompiled alternation regex over all def ids,
built once per document, with an id->body lookup in the replacer: O(text).
(b) the inline-code split alternation backtracks quadratically on a long
UNCLOSED backtick run. Lines over 8KB now skip the split (left untouched) —
a real footnote line is never that long.
F3 [WARNING] Restore the leading YAML front-matter strip that the retired
markdownToHtml layer did. Without it, Obsidian/Hugo/Jekyll/git-sync files leak
their front-matter into the body (and 'title:' renders as a setext heading that
title extraction can hijack).
F4 [WARNING] Extend the zip-import spec with an image (width+align) + callout
fidelity assertion through the PM->HTML->PM hop (the one hop the package suite
does not cover).
F5/F6 Update AGENTS.md (apps/server is now a prosemirror-markdown consumer) and
make the server pretest build prosemirror-markdown too.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The foreign-markdown import normalizer rewrote GFM reference footnotes
(`[^id]` + `[^id]: def`) into canonical inline `^[def]` footnotes, but two
edge cases corrupted content:
1. A `[^id]` inside an inline-code span (backticks) was rewritten like prose
text — only fenced code blocks were protected. Now the rewrite pass splits
each line on inline-code spans and only touches the text outside them.
2. An unbalanced `]` in a definition body truncated the resulting `^[...]`
footnote at the canonical tokenizer, leaking the tail as literal text. The
body's square brackets are now backslash-escaped before wrapping.
Adds golden cases for both.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The step-1 package.json declared the new @docmost/prosemirror-markdown workspace
dep but the lock was not regenerated (CI frozen install would fail), and it also
added a stray prom-client dep (a coder env-workaround for a pre-existing hoisted
import, unrelated to #345 — removed). Regenerated the lock with only the
prosemirror-markdown dep; faithful frozen install now passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move every SERVER Markdown->ProseMirror path off the editor-ext markdown layer
(`markdownToHtml`, a second marked-based parser) onto the canonical
`@docmost/prosemirror-markdown` package, and add a foreign-markdown normalizer at
the import boundary.
Code:
- `ImportService.processMarkdown` (single `.md` upload) now parses
`markdownToProseMirror(normalizeForeignMarkdown(md))` directly — no HTML hop.
- `PageService.parseProsemirrorContent` markdown case (page create/update with
`format: 'markdown'`) same.
- `FileImportTaskService` (zip import) parses markdown with the package, then
serializes to HTML (`jsonToHtml`) so the SHARED HTML attachment / internal-link
pipeline (processAttachments + formatImportHtml + processHTML) keeps handling
`.md` and `.html` imports uniformly. The markdown PARSE — the drift source — no
longer goes through editor-ext; the PM->HTML->PM hop that follows is lossless
plumbing for attachment resolution, not a second parse.
- `canonicalizeFootnotes` stays as an idempotent #228 safety net for the HTML
path (a no-op on the already-canonical markdown output).
Normalizer (`integrations/import/utils/foreign-markdown.ts`): a TEXT pre-pass,
NOT a parser fork. The strict canonical parser does not accept GFM `[^id]`
reference footnotes (and would misread `[^id]: def` as a CommonMark link-ref
definition, silently corrupting the ref into a bogus link), so the normalizer
rewrites reference footnotes into canonical inline `^[def]` before parsing.
Callout surfaces (`:::type` and `> [!type]`) are intentionally NOT touched — the
canonical parser already accepts BOTH natively, so normalizing them would be
redundant and risk degrading its nesting/code-fence-aware handling.
Fixtures-first: foreign-markdown.spec pins the normalizer and the end-to-end
acceptance (no literal `[^id]`/`:::` leaks; re-export is canonical). The two
footnote-canonicalize specs are updated to the canonical output — the parser
assigns fresh `fn-*` ids, so they now assert by definition BODY order (still
reference-ordered, deduped, orphan-free).
FINAL CHECK: `grep -rn "htmlToMarkdown\|markdownToHtml" apps/server/src` (non
-test) is now empty — both editor-ext markdown-layer functions are gone from the
server.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move every SERVER ProseMirror->Markdown path off the editor-ext markdown layer
(`htmlToMarkdown`, a second turndown-based converter) onto the canonical
`@docmost/prosemirror-markdown` package.
- `ExportService.exportPage` (page/space markdown export) and
`collaboration.util.jsonToMarkdown` (used by page.controller's markdown
responses and the AI public-share chat tool) now serialize DIRECTLY from
ProseMirror JSON via `convertProseMirrorToMarkdown` — no HTML intermediate, no
`<colgroup>` scrub (the converter emits GFM tables directly).
This is the SAME serializer the git-sync vault writer feeds, so an exported page
BODY is byte-identical to its vault representation: no more export-md vs vault-md
drift. The HTML export path is unchanged (still `jsonToHtml`).
Emitted markdown moves to the canonical forms: callouts `> [!type]` (not
`:::type`), inline footnotes `^[…]` (not `[^id]`), lossless images
` <!--img {…}-->` (editor-ext dropped width/height/align).
Fixtures-first: export-markdown.spec asserts those canonical forms and the
export==vault-by-construction equality (both call the package converter). The
one deliberate export/vault delta — export prepends the page title as an H1
while the vault carries it in frontmatter — is pinned by a test.
Test infra: declare the `@docmost/prosemirror-markdown` workspace dep; teach
jest to load its ESM build (babel-jest) and stub `@tiptap/react` (server code
imports editor-ext, whose node views reference React renderers only used in a
live browser editor — never on the server).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- F1: added in-app execute tests for the two wirings that ACTUALLY changed in the
migration (the contract-parity test only checks advertised schema keys, not
execute bodies): movePage forwards the newly-added optional `position` to
client.movePage (and passes undefined position + null parent when omitted); the
table trio (insert/delete/updateCell) forwards the unified `table` param
positionally. A field destructured under the wrong name would have silently
passed undefined to the client (execute is any-cast, tsc won't catch it).
- F2: rewrote the three migrated descriptions that hardcoded snake_case sibling
tool names (which the in-app camelCase layer exposes under different ids,
violating the registry's own transport-neutral-prose convention) into neutral
prose: getPage "use get_page_json" -> "use the lossless page-JSON read tool";
updatePageJson "get_page_json -> ... -> update_page_json" -> "read the page-JSON
view -> modify -> write it back", "prefer rename_page" -> "prefer the rename-page
tool"; exportPageMarkdown "import_page_markdown round-trip" -> "page-Markdown
import round-trip" (the last was a direct regress — the in-app base said the
camelCase importPageMarkdown). (stashPage's pre-existing get_page_json mention is
out of scope, per the reviewer.)
Gate: mcp build 0; ai-chat-tools.service + tool-tiers (catalog-partition) pass,
incl. the 5 new execute-wiring tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- F1: added '/icons/' to STATIC_PATH_PREFIXES — public/icons/ is copied verbatim
to client/dist (a sibling of the already-included brand/vad/locales), and
index.html references /icons/favicon-*.png on every page load, so those requests
were getting their own route labels instead of collapsing to `static`.
- F2: corrected the comment — only /assets/ is content-hashed (unbounded per
deploy); /vad//brand//locales//icons/ have stable names (repetitive, not
unbounded). Either way none belong in the API-route histogram.
- F3: the negative test now exercises the trailing-slash boundary (the actual
anti-false-collapse guard): '/assets' (no slash), '/assetsx/foo.js',
'/iconset/x.png' must NOT collapse to `static` — cases that a buggy
includes()/slashless-prefix impl would wrongly collapse. Plus '/icons/*' added
to the positive it.each.
Gate: server tsc 0; metrics.spec passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Migrates share_page / sharePage into the transport-agnostic spec registry
(schema + description declared once; each transport keeps only its execute/auth):
- sharePage (deferred) -> SHARED_TOOL_SPECS; index.ts uses registerShared(),
ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS.
Drift reconciled (documented inline): both inline copies already carried the
"only share when the user explicitly asked" security framing, so the old
"per-transport divergence" note in BOTH layers was STALE — there was no real
behavioral divergence, only wording drift. The canonical description merges the
MCP copy's URL-format + idempotency detail with the in-app copy's reversibility
note and keeps the shared security framing. pageId keeps the MCP copy's stricter
.min(1). The MCP execute keeps its own `searchIndexing ?? true` default
(per-layer, not part of the shared schema).
Intentionally NOT migrated (kept inline — genuinely divergent, as their existing
notes state):
- search / searchPages: the in-app tool is a semantic+keyword hybrid (RRF) with
in-process access control and a tuned schema (limit 1-20); the MCP `search` is
a plain REST full-text search (limit up to 100). Different behavior AND schema.
- docmost_transform / transformPage: the in-app tool deliberately omits the
`deleteComments` schema field (a comment-deletion guardrail) and carries a
shorter description. Different schema.
Gate: mcp build 0 + node --test 458/458 (page-search excluded — hangs only under
the local re2->RegExp type-shim, its source untouched), server jest 775 incl.
tool-tiers catalog-partition + shared-spec contract parity, server tsc 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Migrates the three-layer page tools into the transport-agnostic spec registry
(schema + description declared once; each transport keeps only its execute/auth):
- getPage, listPages (core), createPage, movePage, renamePage, deletePage,
updatePageJson, exportPageMarkdown (deferred) -> SHARED_TOOL_SPECS; index.ts
uses registerShared(), ai-chat uses sharedTool(); removed from
INLINE_TOOL_TIERS. Tiers preserved from CORE_TOOL_KEYS (getPage/listPages =
core, the rest deferred).
delete_page is genuinely three-layer (in-app deletePage exists), so it IS
migrated — not MCP-only. Its H4 guardrail is preserved: the shared schema
exposes ONLY pageId, so no permanentlyDelete/forceDelete flag can reach the
client (still asserted by ai-chat-tools.service.spec.ts).
Descriptions merged (documented inline): each canonical text takes the MCP
copy's richer structural notes plus the in-app copy's reversibility framing.
Schema DRIFT reconciled (documented inline):
- createPage.content: MCP pinned .min(1) but the in-app copy left it unbounded
and DOCUMENTS an empty body as valid ("may be empty" — creating an empty page
to fill later is a real use). Kept the looser no-min form: create_page now also
accepts an empty body (harmless) and no previously-valid in-app input is
rejected. title/spaceId keep the MCP .min(1) (empty is never valid).
- movePage: MCP exposed an optional `position` (fractional-index) field the
in-app copy lacked. Unified by KEEPING position — the in-app client already
accepts an optional position arg, so the in-app execute now forwards it;
optional, so no previously-valid call is rejected. `parentPageId` is nullable
on both (real JSON null -> root); the MCP execute keeps its 'null'/'' string
coercion as a per-layer robustness fallback.
- getPage/renamePage/updatePageJson/exportPageMarkdown/listPages: kept the MCP
copy's stricter .min(1) on ids where the in-app copy was unbounded.
Per-transport execute logic preserved: getPage's {title,markdown} projection,
updatePageJson's JSON-string normalization, list_pages' default limit/tree, and
move_page's cycle guard + positive-confirmation check all stay in their execute
bodies.
Intentionally NOT touched: updatePageContent (Markdown-based body update; no MCP
equivalent) and getTable (name-convention divergence, see tables family) stay
inline.
Gate: mcp build 0 + node --test 458/458 (page-search excluded — hangs only under
the local re2->RegExp type-shim, its source untouched), server jest 770 incl.
tool-tiers catalog-partition + shared-spec contract parity + deletePage H4
guardrail, server tsc 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Production OOM'd (JS heap 1.85 GB / 2 GB limit) during a ~20-step,
~28k-chunk autonomous agent turn. Heap snapshot analysis (memlab) showed a
single DefaultStreamTextResult retaining ~1.7 GB via the never-consumed
leftover tee() branch of its internal baseStream.
Root cause in ai@6.0.134: streamText substitutes the default text() output
strategy even when the caller passes NO `output` option. Its
createOutputTransformStream then accumulates the ENTIRE turn text and, on
EVERY text-delta, enqueues `{ part, partialOutput }` where partialOutput is
a flat snapshot of all text so far (JSON.stringify flattens the
cons-string) — O(n²) memory across the turn. Every consumer accessor tees
baseStream and keeps the second branch as the new baseStream; the final
leftover branch is never read, so its controller queue holds every chunk
(28,225 x ~164 KB in the OOM'd run) for the life of the turn.
Fix (pnpm patch on both dist/index.js and dist/index.mjs):
- pass the raw, possibly-undefined `output` option into
createOutputTransformStream instead of defaulting to text()
- when output == null, publish each text-delta immediately without
accumulating turn text or producing partialOutput snapshots; streaming
granularity is unchanged, and callers that DO request an output strategy
keep the original behavior
Our server never uses partialOutputStream / experimental_output / the
output option, so no behavior changes for us beyond memory.
Regression spec ai-sdk-partial-output.patch.spec.ts drives the real
patched SDK with MockLanguageModelV3: asserts per-delta textStream
granularity, an EMPTY experimental_partialOutputStream (tripwire — yields
one cumulative partial per delta when unpatched), and the PATCH(docmost
marker in both installed dist bundles. Also documents the patch in
AGENTS.md (must be re-created when bumping `ai`) and CHANGELOG.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Migrates the three-layer table WRITE tools into the transport-agnostic spec
registry (schema + description declared once; each transport keeps only its
execute/auth):
- tableInsertRow, tableDeleteRow, tableUpdateCell -> SHARED_TOOL_SPECS;
index.ts uses registerShared(), ai-chat uses sharedTool(); removed from
INLINE_TOOL_TIERS (all three are deferred; not in CORE_TOOL_KEYS).
Drift reconciled (documented inline): the four table tools previously carried a
"NOT shared" note in both layers over a single parameter-NAME drift — the MCP
layer named the table reference `table`, the in-app layer `tableRef`. Unified on
the MCP name `table` (renaming the public MCP parameter would break external MCP
clients; the in-app parameter is model-facing/prompt-only and safe to rename).
The in-app execute bodies now destructure `table`. Descriptions took the MCP
copy's richer wording (documents `#<index>`, padding, header-row behavior) plus
the in-app copy's "Reversible via page history" note; both fields keep the MCP
copy's stricter .min(1) (in-app left them unbounded); sibling tool references
phrased transport-neutrally.
Intentionally NOT migrated (kept inline): table_get / getTable. Its MCP tool
name is noun-first (`table_get`) while the in-app key is verb-first (`getTable`),
which breaks the snake_case(inAppKey) naming convention the registry enforces
(shared-tool-specs.contract.spec.ts). Renaming the public MCP tool would break
external clients, so it stays per-transport — but its in-app reference param was
still aligned to `table` (was `tableRef`) for consistency with the migrated trio.
Gate: mcp tsc 0 + node --test 458/458 (page-search excluded — hangs only under
the local re2->RegExp type-shim, its source is untouched), server jest 730 incl.
tool-tiers catalog-partition + shared-spec contract parity, server tsc 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Migrates the three-layer comment tools into the single transport-agnostic spec
registry (schema + model-facing description declared once; each transport keeps
only its execute/auth):
- createComment, listComments, resolveComment, checkNewComments — moved to
SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool();
removed from INLINE_TOOL_TIERS (tier/catalogLine now on the spec). Tiers
preserved from CORE_TOOL_KEYS (create/list/resolve = core, check = deferred).
Intentionally NOT migrated (kept MCP-inline): update_comment / delete_comment —
they are MCP-only by design; the in-app AI-chat layer deliberately has no
updateComment/deleteComment (comment edits are irreversible / not
version-tracked), asserted by ai-chat-tools.service.spec.ts. A registry spec's
tier/catalogLine are in-app metadata and the catalog-partition test forbids a
deferred spec without a live in-app tool, so these stay per-transport.
Drift reconciled (documented inline): createComment/listComments/checkNewComments
took the more-maintained/superset description + stricter .min(1) guards.
resolveComment: `resolved` drifted (MCP optional+default(true) vs in-app
required) — kept the MCP superset, so in-app resolveComment now accepts an
omitted `resolved` (defaults to resolve) — a deliberate, backward-compatible
unification (never rejects a previously-valid input).
Gate: mcp build 0 + node --test 480/480, ai-chat 654, tool-tiers (incl. F3
catalog-partition) 16/16, server tsc 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to #355: http_request_duration_seconds's `route` label captured raw
content-hashed asset filenames (route="/assets/index-CAbxDtto.js",
"/assets/chunk-*.js"). @fastify/static serves each file through a route whose
matched routeOptions.url IS the raw hashed path, so the label was unbounded — a
new set of names every deploy, growing the series forever (the exact cardinality
leak the API routes were protected against).
resolveRouteLabel now detects a static request by its path prefix (/assets/,
/vad/, /brand/, /locales/) FIRST and collapses it to a single `static` label
(query string stripped before the check); API routes still use the template and
404s still collapse to `unknown`. Static edge latency is already measured by
Traefik's traefik_router_request_duration_*.
Gate: server tsc 0; metrics.spec passes (added static-collapse + query-strip +
"real API route mentioning assets is NOT collapsed" cases).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A long-lived branch can add a migration whose timestamped filename sorts BEFORE
migrations already applied in prod (#234's 20260627T130000-ai-chat-runs merged
after 20260704T120000-client-metrics was live). Kysely's migrator with the
default ordered setting then rejects the applied set as "corrupted migrations"
(no longer a prefix of the sorted list), throws, and the app crash-loops on boot
— exactly incident #361 (502s for ~11 min after a develop deploy). #119 and #120
(June branches) are the next such threats.
Two levels, both:
1. CI migration-order gate (a new `migration-order` job in test.yml, PR-only):
fails the PR when an added migration sorts at/before the newest migration on
the base branch, with an actionable message to rename it to a current
timestamp before merge. This is the primary defense — makes back-dating
impossible to merge accidentally.
2. `allowUnorderedMigrations: true` on BOTH Migrators (migration.service.ts
startup auto-migrate + migrate.ts CLI): the runtime safety net — Kysely applies
a not-yet-applied older migration instead of bricking startup, so a back-dated
migration that bypasses the gate (manual push / hotfix branch) still boots.
Trade-off documented inline: apply order across instances may differ from
lexicographic, so migrations must stay independent (ours each create their own
objects); the CI gate remains the primary line.
Verified: allowUnorderedMigrations is a valid Kysely 0.28.17 Migrator option;
server tsc clean; the gate script rejects a back-dated filename and passes a
current one. No new deps, no migration, no runtime behavior change beyond the
migrator resilience.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
20260627T130000-ai-chat-runs sorted before the already-executed
20260702T120000-ai-chat-page-snapshot, so Kysely's strict ordering
check ("corrupted migrations") crash-looped the server on startup.
- rename 20260627T130000-ai-chat-runs.ts -> 20260704T130000-ai-chat-runs.ts
- update the mirror comment in database/types/db.d.ts
- F10 [stability]: closeMetricsServer() now calls server.closeIdleConnections()
+ server.unref() after server.close(). server.close()'s callback doesn't fire
until keep-alive sockets drain, and the scraper (VictoriaMetrics/vmagent) holds
an idle keep-alive socket — so onModuleDestroy's awaited close would hang until
the scraper disconnects or the orchestrator SIGKILLs on the kill-grace window.
closeIdleConnections() drops idle keep-alive sockets so shutdown completes
immediately (Node 22, per the Dockerfile base).
- F9 [test]: client-telemetry.module.spec.ts pins the E1=B register() gate — the
core of the "public endpoint OFF by default" decision: flag unset / any non-
"true" value ("false"/""/"0"/…) → empty controllers+providers (route absent);
"true"/"TRUE" → registers VitalsController + VitalsService. A flag-inversion or
truthiness regression that reopened the anonymous disk-fill surface now fails.
- F11 [regression/perf]: the db_query_duration_seconds token work (firstSqlToken
regex + Set lookup) is now gated on isMetricsEnabled() in database.module.ts, so
a non-metrics deployment pays NOTHING per query (previously observeDbQuery
no-op'd but the token was still computed on every query). Also hoisted the
13-element known-token Set to a module const (KNOWN_SQL_TOKENS) so it's built
once, not per query.
Gate: server tsc 0; metrics + vitals + client-telemetry suites pass (incl. the
new register-gate test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Maintainer resolved E1 as variant B: the public vitals sink + client collection
must be OFF by default (else client_metrics grows unbounded on a self-host deploy
with no external pruner, via an unauthenticated public endpoint).
- F1: new operator flag CLIENT_TELEMETRY_ENABLED (default OFF), SEPARATE from
METRICS_PORT (Grafana reads the table directly, independent of the scrape port).
ClientTelemetryModule.register() provides VitalsController ONLY when the flag is
true (route absent otherwise); the flag reaches the client via window.CONFIG
(config.ts isClientTelemetryEnabled), and initVitals() early-returns when off.
- F2/F3 [throttler]: this repo's ThrottlerGuard applies EVERY named throttler to
every guarded route unless skipped. The new VITALS bucket therefore (a) newly
bound collab-token → 429 behind shared/NAT IPs, and (b) the vitals route didn't
skip the stricter public-share-ai (5/min) bucket → effective 5/min not 120.
Fix (additive, global config unchanged): vitals.controller @SkipThrottle the
other buckets + @Throttle VITALS 120/min; collab-token adds VITALS_THROTTLER to
its existing @SkipThrottle (restoring its prior effectively-unthrottled state).
- F4: metrics node:http server is closed on shutdown (MetricsServerLifecycle
OnModuleDestroy → closeMetricsServer(), fired by enableShutdownHooks).
- F5: docSize outside [0, int4-max] drops to null (keeping the event) instead of
overflowing int4 and failing the WHOLE batch insert (+ 2 tests).
- F6: .env.example documents METRICS_PORT (no default — unset = subsystem OFF) +
CLIENT_TELEMETRY_ENABLED; fixed the inaccurate "default 9464" wording.
- F7: disabled/non-sampled sessions install ZERO observers — isVitalsActive()
(enabled && sampled) gates reportClientMetric AND the page-editor
measurePageOpen + dispatchTransaction wrapping.
- F8: kept db.d.ts hand-added (wontfix) — this repo HAND-CURATES db.d.ts (verified
across recent fork migrations a32fba63/8c5b57eb/fdeede00); codegen would be the
deviation. The ClientMetrics interface maps the migration 1:1.
Gate: server tsc 0, client tsc 0, server metrics/vitals/telemetry/throttle 21
tests, client route-template 5. No new deps.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Squashed for a clean rebase onto develop (was 19 commits; the reviewer approved
the net diff at fb246080). Detaches an agent run from the HTTP request/browser
window: a run is a first-class lifecycle object (ai_chat_runs), a browser
disconnect no longer kills it, a concurrent-run insert-gate prevents double runs,
and a reopened chat live-follows a still-running run via a polled observer merge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The metrics INFRA is already deployed (VictoriaMetrics scraping docmost:9464,
Grafana dashboards, alerts) with a target `gitmost-app` that is red because the
app half didn't exist. This is that half. The contract (metric names, port,
table, endpoint) is FIXED by the deployed infra and matched exactly.
Server (prom-client):
- A bare node:http `/metrics` server on METRICS_PORT (default 9464), SEPARATE
from the Fastify :3000 listener so /metrics never exists publicly; the whole
subsystem is OFF when METRICS_PORT is unset.
- collectDefaultMetrics() + http_request_duration_seconds{method,route,status}
via a Fastify onResponse hook using the ROUTE TEMPLATE (req.routeOptions.url,
never the raw URL — bounded cardinality; 404 -> "unknown"), EXCLUDING SSE/
streaming responses (would record the connection lifetime and poison p95).
- db_query_duration_seconds (Kysely log callback, labelled by the leading SQL
token), bullmq_queue_depth{queue} (getJobCounts every 15s) +
bullmq_job_duration_seconds{queue} (worker completed/failed),
collab_store_duration_seconds (around onStoreDocument).
- POST /api/telemetry/vitals — PUBLIC (sendBeacon) but IP-throttled; ~16KB body
cap, <=50 events/batch, metric-name + rating whitelist, attr truncated to 120
chars, batch insert; malformed/foreign/oversized silently dropped and 200'd (no
browser retry). New migration `client_metrics` (schema byte-identical to the
contract, both indexes, conditional grafana_ro GRANT; no app-side retention —
the maintenance container prunes >90d).
Client (web-vitals):
- initVitals() decides sampling ONCE per session (25%, sessionStorage) BEFORE
subscribing; onINP/onLCP/onCLS/onTTFB (attribution) buffered + flushed via
navigator.sendBeacon on visibilitychange:hidden and a timer (not fetch-per-
metric). Custom: editor_tx_ms (dispatchTransaction sync-part timer, >8ms, with
doc_size), page_open_ms, longtask_ms. Route labels are templates only; no
titles/slugs/text.
Gate: server + client tsc 0, frozen install 0 (added prom-client + web-vitals +
regenerated the lock), server metrics/vitals tests 11, client route-template 5,
and the migration verified valid against real Postgres.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- F1: render the `underline` mark statically (StarterKit v3 enables Underline;
comment-editor does not disable it) — an underlined comment no longer degrades
the whole comment to the read-only editor fallback. renderMarks gains a
`case "underline" -> <u>`, mirroring the other marks (+ test).
- F2: keep the Open tab panel mounted (`Tabs.Panel value="open" keepMounted`)
while the heavy Resolved panel still unmounts (`Tabs keepMounted={false}`). A
per-panel keepMounted overrides the parent's `false` (Mantine 8 TabsPanel), so
an in-progress reply draft / edit in the Open panel survives an
Open->Resolved->Open switch, keeping the micro-opt of not mounting the large
Resolved list.
- F3: cover edit->save->re-render in comment-list-item.test.tsx — save calls
mutateAsync with JSON.stringify(editContentRef) and a new comment.content prop
updates the visible body; cancel restores the static body without mutating;
clearing editContentRef after cancel.
- F4: extract childrenByParent grouping into an exported pure
`buildChildrenByParent(items)` (unit-tested: nesting, orphan reply, sibling
order) + new comment-list-with-tabs.test.tsx covering the lazy reply-editor
activation (stub -> click/focus/Enter mounts the editor).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- F1: document AI_CHAT_DEFERRED_TOOLS in .env.example (AI_* section) — default
ON = deferred loading (compact catalog + loadTools), =false restores the old
"all tools always active" behavior.
- F2: integration test of the ON path in ai-chat-stream.int-spec.ts — a deferred
tool activated via loadTools is active on the SAME turn's next step but a fresh
turn starts cold (CORE + loadTools only), proving the per-turn activatedTools
Set does not leak across turns/chats. Drives the real streamText loop with a
MockLanguageModelV3 and inspects recorded per-step activeTools-filtered tools.
- F3: replace the magic toHaveLength(28) in tool-tiers.spec.ts with a two-way
partition against the LIVE in-app toolset (AiChatToolsService.forUser keys):
every non-core tool must appear in buildInAppDeferredCatalog and every catalog
entry must map to a real non-core tool — so a future tool forgotten in
INLINE_TOOL_TIERS fails the suite instead of silently vanishing from the agent.
No production logic change (mechanism was already reviewed correct).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment panel lagged for seconds on open and stuttered on every resolve/apply
with many comments (real case: 30 open + 326 resolved ≈ 356 threads), because each
comment body mounted a full TipTap/ProseMirror editor, both tabs mounted at once,
and any mutation re-rendered the whole list.
- CommentContentView: static recursive renderer of comment ProseMirror JSON (no
editor instance) for the read-only body — supports exactly CommentEditor's node
set (doc/paragraph/text/hardBreak/mention) + marks (bold/italic/strike/code/
link), reproducing the 3-level DOM nesting for pixel-identical CSS. Unknown
node/mark or unparseable content degrades that one comment to the read-only
CommentEditor; legacy non-JSON strings render as plain text.
SECURITY: link hrefs are protocol-allowlisted (safeHref, mirroring
@tiptap/extension-link) so a stored comment with a `javascript:`/`data:` href
cannot XSS — the old TipTap read-only path sanitized this; the static renderer
must too. Control-char smuggling (java\tscript:) is stripped before the check.
- MentionContent extracted from MentionView, shared by the TipTap NodeView and the
static renderer (identical user/page-mention behavior).
- keepMounted={false} on the tabs: the inactive tab no longer mounts its editors.
- Lazy reply editor: a stub until click/focus, then the real editor (kept mounted
so the draft survives thread re-renders).
- React.memo(CommentListItem) + a childrenByParent map (replaces the per-thread
O(n^2) filter) + localized reply-send pending state: resolve/apply/reply now
re-render only the touched thread.
- Progressive first paint: useCommentsQuery no longer blocks on hasNextPage.
Gate: client comment+mention suites 22/22 passed, tsc --noEmit 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The in-app AI agent shipped all ~41 tool schemas on every model step. This
adds a two-tier catalog: core tools (frequent or one-line) stay always-active;
the rest are advertised as a compact catalog and their full schema is fetched
on demand via the loadTools meta-tool, wired through ai@6 prepareStep's
per-step activeTools.
- tools/tool-tiers.ts: CORE_TOOL_KEYS, INLINE_TOOL_TIERS, applyLoadTools,
catalog builders (+ tool-tiers.spec.ts, 13 cases).
- ai-chat.service.ts prepareAgentStep: returns activeTools =
[...CORE_TOOL_KEYS, loadTools, ...activatedTools]; per-turn activated Set.
- ai-chat.prompt.ts: buildToolCatalogBlock renders the deferred catalog.
- mcp/tool-specs.ts: tier + catalogLine metadata (external snake_case /mcp
transport unchanged).
- EnvironmentService.isAiChatDeferredToolsEnabled(): AI_CHAT_DEFERRED_TOOLS,
default ON per issue intent (kill-switch =false restores old behavior).
Gate: server ai-chat 631/631, tool-tiers 13/13, mcp 472/472, tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F4 [critical] — the anti-join `DELETE … WHERE NOT EXISTS(child)` was still racy
under Postgres READ COMMITTED: a reply INSERT holds FOR KEY SHARE on the parent;
the DELETE's start snapshot doesn't see the uncommitted child (NOT EXISTS true),
blocks on the reply's lock, and when the reply commits the parent was only LOCKED
(not modified) so EvalPlanQual does NOT re-check → the DELETE proceeds and CASCADE
destroys the just-committed reply. Replaced with a transaction: SELECT the parent
FOR UPDATE (conflicts with the reply's FOR KEY SHARE → serializes the concurrent
reply), re-check for a child with a FRESH statement in the same tx (a new RC
snapshot sees a just-committed reply), delete only if still childless (return 1)
else return 0 (caller resolves). The FOR UPDATE lock is held to end-of-tx so no
reply can insert between the re-check and the delete. Signature unchanged, so the
service + its mocked unit tests are untouched; docstrings updated.
F5 [warning] — the client Dismiss button was gated only on canComment, but the
server now gates dismiss on owner-or-space-admin, so a non-owner non-admin saw a
button the server 403s. `canShowDismiss` now also requires
`isOwnerOrAdmin = currentUser?.user?.id === comment.creatorId || userSpaceRole ===
"admin"` (the same gate the comment delete-menu already uses); threaded into both
call sites.
F6 [warning] — added a REAL-DB int-spec
(apps/server/test/integration/comment-delete-if-childless.int-spec.ts, + a
createComment seeder): (a) childless → returns 1, row gone; (b) committed reply →
returns 0, parent+reply survive; (c) CONCURRENCY — a second connection inserts a
reply (FOR KEY SHARE) and commits mid-operation while deleteCommentIfChildless
blocks on FOR UPDATE → asserts it returns 0 and both rows survive (a blind
anti-join would lose the reply here). Ran against live Postgres — 3/3 pass.
server tsc clean; comment jest 53 + int-spec 3 (live Postgres) pass. client tsc
clean; comment vitest 56 pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Maintainer escalation decision (B) + reviewer findings on the ephemeral-
suggestion PR.
Authz (decision B): POST /comments/dismiss-suggestion now gates the destructive
branch on owner-OR-space-admin, mirroring POST /comments/delete exactly (same
SpaceCaslAction.Manage / SpaceCaslSubject.Settings, same owner short-circuit,
same ForbiddenException). A non-owner non-admin who tries to dismiss another's
childless suggestion gets Forbidden before the service runs. Apply stays on
canEdit (accepting an edit is the editor's semantics), unchanged.
F1 [blocking] — atomic conditional delete closes the hasChildren→delete race.
New repo `deleteCommentIfChildless(id)` runs a single
`DELETE FROM comments WHERE id=:id AND NOT EXISTS (SELECT 1 FROM comments child
WHERE child.parent_comment_id = comments.id)` (verified by compiling the Kysely
expression to SQL — the correlated subquery references the OUTER comments.id).
deleteEphemeralSuggestion strips the mark first, then the conditional delete: if
it removed the row → commentDeleted + outcome 'deleted'; if a reply raced in
(0 rows) → fall back to resolveComment (outcome 'resolved') so the discussion and
the new reply survive. No reply can be cascade-deleted anymore.
F2 [warning] — the apply/dismiss onError success-noop is narrowed from 404||400
to 404 ONLY. A 400 means the comment is ALIVE (apply's 400 = the thread was
resolved-not-applied), so it now shows a real error (surfacing the server
message) and KEEPS the comment in cache instead of a false "applied" + dropping a
live thread.
F3 [suggestion] — the 404-race client tests assert the success toast fired.
Tests: server — dismiss authz (owner ok / non-owner-non-admin Forbidden /
space-admin ok), the delete→resolve race (hasChildren=false but conditional
delete returns 0 → resolve, no commentDeleted), delete-path asserts switched to
deleteCommentIfChildless; client — apply-400 and dismiss-400 (kept in cache, red,
not success) + the toast assertions.
server tsc clean, comment+collaboration jest green; client tsc clean, comment
vitest 54 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Agent suggestion-edits (comments with suggestedText, #315) piled up: Apply
auto-resolved the thread, cluttering the resolved tab, and the anchors stayed in
the document. Make them ephemeral: resolving (Apply OR the new Dismiss) makes the
comment DISAPPEAR — hard-delete + remove the Yjs `comment` mark — UNLESS the
thread has replies, in which case resolve it (preserve the discussion). Manual
Resolve is unchanged. Scope: only comments with `suggestedText`.
Server:
- New collab event `deleteCommentMark` (collaboration.handler) mirroring
resolveCommentMark, wiring the existing removeYjsMarkByAttribute to strip the
anchor from the doc.
- `finalizeAppliedSuggestion` forks on `hasChildren`: replies → apply + resolve
(outcome 'resolved'); none → apply + hard-delete + mark removal (outcome
'deleted').
- New `dismissSuggestion` (validates top-level + suggestedText + not applied/not
resolved) with the same fork; permission `canComment` (NOT canEdit — dismiss
doesn't change page text); audit COMMENT_SUGGESTION_DISMISSED. New
POST /comments/dismiss-suggestion; apply stays canEdit.
- Both return `{ outcome: 'deleted' | 'resolved' }` so the client picks the
optimistic action.
Data-integrity (review F1): the shared `deleteEphemeralSuggestion` removes the
anchor mark FIRST and FATALLY, then deletes the DB row only on success. The row
delete is irreversible, so a mark-removal failure — including the
COLLAB_DISABLE_REDIS "no live instance" hard-error — must abort the whole
operation (→ 5xx, repeatable) rather than swallow the error and leave a permanent
orphan anchor pointing at a deleted comment. `deleteCommentMark` is no longer
best-effort (unlike resolve, where the row is kept and a failed mark is
recoverable).
Client:
- `canShowDismiss` (canComment) alongside `canShowApply` (canEdit); a "Dismiss"
button next to Apply in the suggestion block.
- `useApplySuggestionMutation`/`useDismissSuggestionMutation` reconcile the cache
on `outcome` ('deleted' → remove; 'resolved' → relocate to the resolved tab).
- Idempotent races (review F2): BOTH apply and dismiss onError reduce 404/400 to
success (comment already gone/resolved), dropping it from the cache instead of
a red error — restores the #315 apply idempotency the ephemeral delete would
otherwise break.
- i18n Dismiss / "Не применять" (ru/en).
Not done (flagged): deleteCommentMark on the normal /comments/delete path — left
out (would change every non-suggestion delete + needs gateway injection; the
interactive client already strips the mark via unsetComment). Out of scope per
the issue.
Tests: server — apply/dismiss delete-vs-resolve fork, all four dismiss state
guards, the deleteCommentMark handler, controller authz (dismiss=canComment,
apply=canEdit), AND a mark-removal-failure test proving the row is NOT deleted +
the error propagates (F1). client — Dismiss show-conditions, outcome cache
reconciliation, and 404 idempotent race for BOTH dismiss and apply (F2).
Verified: server tsc clean; comment+collaboration jest 144 passed. client tsc
clean; vitest 905 passed | 1 expected-fail.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
agent_coder
vvzvlad
agent_vscode
agent_coder
claude code agent 227