Red-team #13 (conflict markers reaching Docmost) is now a per-space policy
exposed as a UI toggle, instead of a hardcoded behavior. New boolean
`gitSync.autoMergeConflicts` (default FALSE), mirroring the existing per-space
`gitSync.enabled` flag end-to-end (jsonb space settings -> update-space DTO ->
space.service -> client types -> space settings form switch):
- OFF (default, safe): a page whose committed body still has unresolved git
conflict markers is NOT pushed — it is recorded as a per-page push FAILURE
("unresolved conflict markers — resolve in git first"). Recording a failure
(not a soft skip) deliberately HOLDS refs/docmost/last-pushed so the conflict
commit is never marked pushed and a later pull cannot clobber the user's
in-progress resolution; the page retries until the conflict is resolved in git.
- ON: the marker lines are stripped and both sides' content is pushed (the prior
behavior), so the conflict becomes visible/fixable inside Docmost.
The engine Settings carries `autoMergeConflicts`; runPush threads it into the
update AND create paths. The orchestrator's buildSettings reads the per-space
flag from jsonb (strict opt-in like `enabled`, default false).
Tests: redteam-push-cycle #13 rewritten (default -> not pushed + failure + refs
held; ON -> strip-and-push); space.service + edit-space-form + orchestrator
specs extended. git-sync vitest 618, server jest space+git-sync 163, client
edit-space-form 11, server/client tsc clean.
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>
Resolve the code-review findings from comment #1571 on PR #119.
Engine (packages/git-sync):
- Idempotent CREATE on retry: before createPage, look the page up in the
live Docmost tree by (parentPageId, title) and ADOPT it instead of
duplicating when a prior cycle created it but failed to persist the
pageId back to disk. Only trust a COMPLETE tree for the lookup; fall
back to createPage otherwise. Covered by new tests incl. a complete=false
regression-lock.
- Route applyPullActions diagnostics through an injected logger instead of
bare console (thread log from the cycle).
- Add a timeout to the git execFile chokepoint (runRaw) so a hung git
subprocess cannot wedge a sync cycle.
- Translate remaining Russian code comments to English.
- Remove dead standalone-CLI code (parseArgs/PushParsedArgs,
parseSettings/envSchema, loadSettingsOrExit + config-errors.ts) and the
matching index exports/specs; keep the Settings type.
- Fix the dangling docs link in package.json.
- Add a schema-surface snapshot guard so any drift in the vendored
document schema is a loud, must-review CI failure (+ provenance header).
Server (apps/server):
- Add a configurable watchdog timeout to the spawned git http-backend so a
stalled push cannot hold the per-space lock forever
(GIT_SYNC_BACKEND_TIMEOUT_MS).
- Close the in-process TOCTOU window in SpaceLockService.withSpaceLock by
reserving the slot synchronously before acquire.
- Add tests: removePage git-sync provenance (both branches), ensureServable
force-push-protection git configs, and the phase-B+ datasource methods.
Docs / build:
- AGENTS.md: list git-sync as the fifth workspace package and note the
three schema mirrors; fix the dangling git-sync-plan.md backlog link.
- pnpm-lock.yaml: add the missing @docmost/git-sync workspace link so
pnpm install --frozen-lockfile (CI default) succeeds.
Co-Authored-By: Claude Opus 4.8 <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>
Self-review of phase 3 caught a data-corruption regression: nativeMeta always
supplies the run's spaceId, so the planner's 'create-without-spaceId' skip — which
had doubled as the only filter for non-page files — went dead. An ADDED
.obsidian/*.json, attachment, or dotfile (committed to the vault, no .gitignore)
would then be classified as a CREATE: a junk Docmost page, plus a gitmost_id
frontmatter written INTO the file, corrupting it.
Fix: isPageFile(path) — a .md file with NO dot-segment anywhere — and filter the
diff to page files at the very top of computePushActions, BEFORE any
classification, so non-page A/M/D/R are ignored (design §Адопция). 2 unit tests
pin it (.obsidian/json, attachment, dotfile, dot-segment, .md dotfile all ignored;
real pages still created). 614 engine tests green.
Also: refreshed stale docmost:meta comments to gitmost_id (review SUGGESTION), and
documented the deferred adoption frontmatter-preservation gap (review WARNING) in
page-file.ts + the design doc (do NOT roll native onto a real vault with Obsidian
properties until phase 4 round-trips them).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PUSH now consumes the native-Obsidian format end-to-end:
- identity from the gitmost_id frontmatter (parsePageFile), not docmost:meta;
- title from the FILENAME, parentPageId from the enclosing folder's folder-note
(parentFolderFile is now FOLDER-NOTE aware: a child's parent is dir/dir.md, and
a folder-note's own parent is one level up), spaceId from the run (every vault
file belongs to the vault's space);
- CREATE derives title/parent/space from path + run and writes the assigned
pageId back as gitmost_id frontmatter (serializePageFile);
- UPDATE pushes the STRIPPED body (current + 3-way-merge base), so the frontmatter
never leaks into Docmost content; the loop-guard hashes the body.
The PURE delete-sensitive classifier (computePushActions/classifyRenameMoves) is
UNCHANGED — only the injected IO resolvers (metaAt, parent, create write-back)
switched source. nativeMeta always carries the run spaceId, so the legacy
'create-without-spaceId' skip no longer fires through runPush.
Tests rewritten to native fixtures + folder-note parent paths; the noop case is
now a child under a renamed parent folder (filename=title, so a path-only-noop
needs an ancestor rename). parentFolderFile tests cover leaf/folder-note/nested/
dotted. 612 engine tests green; engine rebuilt.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to 4376c5a6, found by a real BROWSER e2e (the flow the in-diff fix
missed). When the layout reshuffle's two halves land in SEPARATE sync cycles, the
later cycle's diff has only the DELETE of the old path — the matching add was
already pushed — so in-diff D+A coalescing can't see it, and the live page was
still trashed.
Robust fix on the identity invariant the reviewer (and the user) called out: a
page EXISTS iff its pageId is in the vault, regardless of filename. runPush now
collects the pageIds present at ANY path in the current `main` tree and passes
them to computePushActions; a deleted file whose pageId is still tracked
elsewhere is a MOVE, never a deletion. (Built only when the diff has deletes.)
Adds apps/server/test/git-sync-browser-e2e.cjs — a Playwright test that drives the
REAL Docmost web UI: log in, create several untitled pages, type a title, sync,
assert NOTHING is trashed. Reproduced the data loss before this fix; 5/5 green and
stable after. Engine suite 600 green (+2 computePushActions cases:
pageId-still-present -> skip; pageId-gone -> real delete).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CRITICAL data-loss bug: creating pages in Docmost (which start UNTITLED) and then
typing a title could soft-delete OTHER pages. Untitled pages all serialize to the
`_` fallback filename; the layout disambiguates them (`_.md`, `_ ~slug.md`).
Retitling one frees the bare `_` and another untitled page's file relocates into
it. git's rename detection (`-M`) can't see the move (the tiny meta-only files are
too dissimilar), so `git diff` reports it as DELETE(old) + ADD/MODIFY(new). The
push took the DELETE literally and trashed a live page.
Root cause is that the push trusted git's path-level rename heuristic for page
IDENTITY. Identity is the pageId. Fix: before emitting any delete, coalesce by
pageId — a pageId that is BOTH deleted (pre-image) AND present on the surviving
side (current meta of an ADD or a MODIFY, since a relocation into an occupied path
shows as M) is one page that MOVED, classified as a rename/move and NEVER a delete.
Reproduced + verified on a live stand: 4 untitled pages + retitle one trashed a
different page before; after the fix, retitling one (and stress-retitling all)
trashes nothing. Engine suite 598 green; 3 new computePushActions cases (ghost
D+A move -> rename; real delete still deletes; unrelated D+A stay delete+update).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The implementation spec docs/git-sync-plan.md was removed as completed, but ~44
code comments still cited it as "plan §N". Strip those citations (comments only),
keeping each comment grammatical. The vendored engine's own "SPEC §N" references
point at a different, still-present spec and are left untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Upgrades the 2-way body merge to a real diff3 three-way merge (review #5), so a
block ONLY the human changed is KEPT when git changed a DIFFERENT block — the
2-way merge would revert it to git's stale version.
Engine: the push update loop reads the last-synced pre-image
(`git.showFileAtRef(refs/docmost/last-pushed, path)`) and passes it as the
optional `baseMarkdown` to `client.importPageMarkdown` (the common ancestor).
Server: gitmost-datasource converts base+incoming, and writeBody runs a block-
level diff3 (new three-way-merge.ts `diff3Plan`): live-only change -> keep live,
git-only change -> take git, both-changed -> git wins (conflict policy), inserts/
deletes from either side preserved. Without a base (createPage) it falls back to
the 2-way merge. Crash-safety unchanged (docs built before the connection opens).
Tests: three-way-merge.spec.ts (14 — every diff3 case incl. the cross-block
preservation and conflict policy), yjs-body-merge 3-way (real Y.Docs: human's
block instance preserved while git's block is applied), plus an engine test that
the base is forwarded from showFileAtRef. Existing push assertions updated for the
new base arg. git-sync 589 pass; server merge/datasource/gate 62 pass; typecheck
clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
