Address the non-red-team documentation/cleanup items from review #1679:
- Document the GIT_SYNC_BACKEND_TIMEOUT_MS watchdog (git http-backend) in
.env.example and add it to the environment validation schema — it was used
(getGitSyncBackendTimeoutMs, default 120000) but undocumented/unvalidated.
- Remove the dead GIT_SYNC_DEBOUNCE_MS_DEFAULT / GIT_SYNC_POLL_INTERVAL_MS_DEFAULT
exports (never imported; environment.service is the single source of defaults).
- Redirect the dangling `plan §X.Y` comment references to issue #194 (the
git-sync spec moved there when docs/git-sync-plan.md was deleted by this PR).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A 10-agent red-team pass on the two-way Docmost<->git sync surfaced 16 ranked
findings (9 others triaged out as already-defended). Wrote a reproduction test
per finding (each asserts the CORRECT behavior, so it fails on the bug), then
fixed the production code so every repro goes green. All confirmed bugs:
Round-trip data loss (markdown-converter.ts + docmost-schema.ts mirror):
- #1 editor-ext node types silently dropped on export — ported the 8 missing
canon nodes (footnoteReference/footnotesList/footnoteDefinition, htmlEmbed,
status, pageEmbed, transclusionSource/Reference) into the git-sync schema
mirror and added converter cases that emit their schema-matching HTML instead
of flattening unknown nodes to '' (this was the critical data-loss flagged in
review #1679: footnotes/htmlEmbed lost on sync). Snapshot surface updated.
- #2 top-level image lost width/height/align/attachmentId — now emits an HTML
<img> (like video/diagrams) when it carries layout attrs; bare images stay
. Image node parses width/height as strings so they re-import.
- #3 code block containing a ``` fence corrupted on round-trip — outer fence is
now widened to (longest-inner-backtick-run + 1).
- #16 deep nesting threw RangeError (page never synced) — added a depth guard
(MAX_NODE_DEPTH=400) so the converter never overflows the stack.
Push/layout/cycle (engine):
- #4 disambiguation ' ~slugId' suffix corrupted Docmost titles + order-dependent
layout — deterministic, order-independent sibling disambiguation; suffix is
stripped from a path-derived title ONLY when the new name is exactly the old
title plus the suffix (never a genuine retitle ending in ' ~token').
- #6 retry-adopt by (parent,title) clobbered the wrong duplicate-title sibling —
ambiguous (parent,title) is no longer adopted (falls back to fresh create).
- #12 a new child under a new parent was created at ROOT — creates are ordered
parent-before-child with an in-memory created-id map for parent resolution.
- #13 git conflict markers could reach Docmost — bodies are scanned and the
marker lines stripped (a '=======' line is only treated as a conflict
separator inside a <<<<<<< ... >>>>>>> block, so setext headings are safe).
- #15 a divergent `docmost` mirror was escalated by runPush but dropped by
runCycle — RunCycleResult now forwards divergentDocmost to the orchestrator.
Server (merge / lock / provenance):
- #9 3-way merge lost a human's block edit when git inserted an adjacent block —
finer-grained diff3 region merge (via lcs) preserves non-overlapping human
edits; genuine same-block conflicts still resolve git-wins.
- #10 single-writer race — module-static liveLocks closes the same-process TOCTOU
window, and a heartbeat refresh that cannot confirm the lock now aborts the
cycle at its next write checkpoint (cooperative AbortSignal threaded through
runCycle). Cross-process fencing tokens remain a follow-up.
- #14 sticky-agent provenance overrode an explicit actor='git-sync' write,
blinding the listener loop-guard — resolveSource now lets an explicit actor
win over the sticky-agent fallback (explicit agent still wins).
Verified: git-sync vitest 617 pass (+1 expected-fail), server unit jest 1541
pass, server tsc clean. A review pass over the fixes caught and corrected a
title-suffix over-strip, an inert abort signal, a document-wide conflict-marker
strip, and two leaf-atom content-holes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes the architecture item from the #119 review: drop the "vendored from
docmost-sync" framing and the CJS↔ESM `Function('import()')` bridge so the engine
is a normal first-class gitmost package.
Part 1 — vendoring markers removed (prose only, zero behavior change): reworded
"VENDORED into gitmost" / "vendored from docmost-sync" / "Engine LOGIC is
byte-identical" / "it's a port" comments across the engine. Behavior-bearing
strings are untouched: BOT_AUTHOR_NAME/EMAIL and the `Docmost-Sync-Source:`
provenance trailers (changing them would break git authorship + the loop-guard).
Part 2 — the package is now ESM (matching the sibling @docmost/mcp): `type: module`,
tsconfig Node16, `.js` extensions on relative imports, and a static
`import { marked }` replacing the `new Function('return import(...)')` /
`loadMarked` hack — the bridge is GONE from the package. The CommonJS NestJS
server loads the now-ESM engine via a new `git-sync.loader.ts` that mirrors the
existing `docmost-client.loader.ts` mcp loader exactly (Function-indirected
dynamic import + cached promise + retry-on-reject). The 4 server consumers
(orchestrator/datasource/vault-registry/git-http-backend) call `await loadGitSync()`
for value exports; types stay `import type` (erased). The converter-gate spec —
which needs the real converter — loads the package's TS source via a jest
moduleNameMapper + isolatedModules (documented in that spec); the other git-sync
specs mock the loader.
Verified: engine builds pure ESM (no Function/require leftover), vitest 614,
editor-ext build, server + client tsc, full server jest 1397/0. Live stand
smoke-test: server starts clean on the ESM engine (no ERR_REQUIRE_ESM), a real
sync cycle runs through the loader, and the basic e2e suite is 12/12 (clone via
git-http-backend, push, pull, delete, 3-way merge — all through the new loader).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review finding #5: the git -> page body write (writeBody) did a full-body replace
(delete-all + re-insert) on the shared Yjs doc. Applied while a human is editing
the page, it discarded their in-flight changes; and TiptapTransformer.toYdoc ran
AFTER the fragment was cleared, so a conversion failure could leave the page with
an empty body.
Fixes:
- Active-session guard: CollaborationGateway.getActiveEditorCount(documentName)
reports live human (websocket) editor sessions for a doc, excluding server-side
direct connections. writeBody now throws ActiveEditSessionError when an editor
is connected. The engine's push loop already isolates each importPageMarkdown in
try/catch and does not advance the loop-guard on failure, so the write is simply
retried on the next poll once the editor disconnects — never a clobber.
- Crash-safe conversion: build the replacement Yjs update BEFORE opening the
connection / clearing the fragment, so a transform failure can never leave the
body empty.
Also updates the server-side converter gate spec to the corrected round-trip
shape: the block-image hoist no longer leaves a leading empty paragraph (the
git-sync converter fix in 7d39c16b, now reaching the built package).
A true merge of git content into a live Yjs session is out of scope (it needs a
real 3-way text merge with no shared update lineage); deferring the write while a
page is being edited is the safe, owner-approved minimum.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make @docmost/git-sync natively consumable by the CommonJS server (and jest):
build to CommonJS (tsconfig module CommonJS, drop type:module, strip .js from
relative imports), and lazy-load the only ESM-only dep (marked) via the dynamic
Function('import()') trick (mirrors docmost-client.loader.ts) with a require()
fallback so vitest's evaluator works too. git-sync tests stay green (314 pass,
3 expected fail).
Add the §13.1 idempotency gate (apps/server .../git-sync-converter-gate.spec.ts):
13 editor-ext docs (paragraphs/headings, marks, links, bullet/ordered/task lists,
blockquote, callouts, code block, hr, table, nested mix) round-trip
content(editor-ext) -> convertProseMirrorToMarkdown -> markdownToProseMirror ->
TiptapTransformer.toYdoc/fromYdoc(tiptapExtensions) -> canonicalize and assert
docsCanonicallyEqual. All green => the vendored converter's docmost-schema is
schema-compatible with editor-ext (no node/mark/attr loss), which the plan §13.1
requires before Phase B. The one intrinsic markdown-image lossiness (width/height
/align can't ride plain ) is isolated in a KNOWN DIVERGENCE block, not
hidden. Server tsc clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
