The delete cap (GIT_SYNC_MAX_DELETES_PER_CYCLE, default 5) was a defense-in-depth
guard that SUPPRESSED a cycle's deletions when the planned count exceeded the
limit. In practice it was a crutch over engine correctness that also blocked
legitimate deletes: deleting a folder with many child pages is a normal action,
and git-sync deletes are SOFT (Trash, reversible), so a blocking limit has little
upside and real downside. There is also no user-facing surface to "confirm" a
large delete from a background sync — the only channel is the operator log.
So: drop the cap entirely. Deletes apply unconditionally; every cycle already
logs its full push plan, per-action `delete: <pageId>` lines, and completion
counts through the engine `log`, so what was deleted (and what was skipped) is
always recorded. Engine correctness (the reconcile/layout/round-trip tests) is
what prevents phantom deletions — not a blocking cap.
Removed: orchestrator `resolveApplyClient` cap hook + `maxDeletes`,
`getGitSyncMaxDeletesPerCycle`, the `GIT_SYNC_MAX_DELETES_PER_CYCLE` env/validation/.env.example,
and the cap tests. (The engine's generic optional `resolveApplyClient` hook is
left as an unused extension point.)
server tsc clean, git-sync + environment jest 174.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
The reconcile choreography (ensureRepo -> merge-check -> ensureBranch ->
checkout('docmost') -> pull -> push) was hand-rolled in the app orchestrator's
driveCycle, duplicating an order the vendored engine owns and could drift from on
upgrade — the failure mode is data clobber. Lift it into @docmost/git-sync as a
single entry point, `runCycle(deps)`. The orchestrator now calls runCycle and
keeps only the lock (its caller) and the gitmost-specific delete-cap POLICY,
injected as the `resolveApplyClient` hook (the engine does the dry-run, hands the
hook the planned delete count — Infinity if planning failed — and uses whatever
client it returns for the apply). driveCycle drops from ~150 lines to ~30.
Tests:
- engine test/cycle.test.ts: composition (merge-in-progress short-circuit;
ensureRepo->ensureBranch->checkout staging order before the pull; the cap hook
is consulted with the planned count; no dry-run when no hook).
- engine test/cycle-roundtrip.test.ts: runCycle against a REAL VaultGit in a temp
repo with a faked Docmost client — a git-originated CREATE flows pull->push and
the assigned pageId is written back; an unresolved merge short-circuits before
any client call.
- orchestrator spec rewired to mock runCycle and assert the wiring + the
resolveApplyClient cap policy (the engine-internal cycle-order/merge tests moved
to the engine).
Validated end to end on a live stand (real Postgres/Redis + server): a git clone
-> edit -> push over the /git remote round-trips the change into the Docmost page
through the refactored cycle.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-space single-writer lock — Redis CAS leader lock (SET NX PX, DEL-CAS and
PEXPIRE-CAS Lua), the in-process mutex, the per-process instanceId and the
heartbeat — lived inline in GitSyncOrchestrator. Extract it into a dedicated
@Injectable() SpaceLockService exposing one narrow surface, withSpaceLock(spaceId,
fn), so the lock is the orchestrator's only Redis-lock touch-point and is testable
in isolation. The orchestrator now injects SpaceLockService and both consumers
(runOnce, ingestExternalPush) go through spaceLock.withSpaceLock — behavior
unchanged (same sentinel returns, same 503-on-lock-held contract). Orchestrator
drops 591→472 lines.
Adds space-lock.service.spec.ts asserting the lock SEMANTICS against a fake Redis
(the test-coverage warning from the review): the SET NX/PX args, the DEL-CAS and
PEXPIRE-CAS Lua + ARGV[1]=instanceId, plus the lock-held / in-progress / throw-
still-releases paths. The orchestrator spec is unchanged in count and stays green
(it now builds the real SpaceLockService over its mock Redis).
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>
Two stability warnings from the #119 review:
1. delete-cap no longer drops deletions forever. When planned deletes exceed
GIT_SYNC_MAX_DELETES_PER_CYCLE the apply client's deletePage now THROWS
instead of resolving to a no-op. A throw is recorded by the engine as a
per-page failure, so `refs/docmost/last-pushed` is NOT advanced past the
commit that dropped the files — the next cycle re-diffs from the un-advanced
ref and re-plans the same deletes (a transient over-cap is retried, not
silently dropped and then recreated by the next pull). Previously a resolving
no-op let the engine count `deleted++` with no failure, advance the ref, and
never replay the deletions.
2. git-sync soft-delete and restore now stamp provenance. deletePage routes
GIT_SYNC_PROVENANCE through pageService.removePage, and restorePage stamps
lastUpdatedSource='git-sync' on the restore update — so the page-change
listener's loop-guard (skip when lastUpdatedSource==='git-sync') recognizes
both as its own writes instead of scheduling a wasted echo cycle. Done via a
backward-compatible optional `lastUpdatedSource` param on
pageRepo.removePage/restorePage (omitted for ordinary user deletes/restores).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Expose each git-sync-enabled space as a clonable/pushable git repo over HTTP,
so `git clone https://<user>:<pass>@<host>/git/<spaceId>.git` works and external
pushes flow back into Docmost pages — gitmost itself acts as the git host (no
external GitHub/Gitea, no SSH).
Transport: shell out to `git http-backend` (CGI; git is already in the runtime
image) which implements the full smart-HTTP protocol (info/refs, upload-pack,
receive-pack, protocol v2). A raw Fastify route `/git/*` (mounted at the root,
outside the `/api` prefix) bridges the request/response to the CGI; passthrough
content-type parsers for the git media types stream the raw body to stdin.
Reuse the existing engine: clients push the vault's `main` branch, whose commits
beyond `refs/docmost/last-pushed` the engine already reconciles into Docmost.
- http/git-http.service.ts — auth (HTTP Basic -> AuthService.verifyUserCredentials),
self-resolved workspace (DomainMiddleware does not run for this raw route),
per-space gating (global + per-space gitSync flags, 404 hides existence),
CASL authz (Read=fetch, Manage=push), dispatch.
- http/git-http-backend.service.ts — spawn `git http-backend`, binary-safe CGI
response parsing (Status/headers/body), stream to the socket.
- http/git-http.helpers.ts — pure path parse, service->kind mapping, gate decision
(unit-tested); rejects literal and percent-encoded path traversal.
- orchestrator: extract reusable withSpaceLock (CAS-guarded lock heartbeat so a
long push cannot let the lock expire mid-cycle) and add ingestExternalPush
(receive-pack + Docmost cycle under one lock; 503 on contention).
- vault-registry: ensureServable() — ensureRepo + idempotent receive.denyCurrentBranch
=updateInstead / denyNonFastForwards / http.receivepack / http.uploadpack.
- env: GIT_SYNC_HTTP_ENABLED (defaults to GIT_SYNC_ENABLED) + validation.
- main.ts: register the /git/* route and the git content-type parsers.
Tests: pure helpers, CGI parsing, and the GitHttpService handler (auth/gate/authz
+ workspace resolution). Server tsc + git-sync/env suites green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comprehensive-review follow-ups (APPROVE WITH SUGGESTIONS; no critical issues):
- poll interval is now actually configurable: replaced the hardcoded
@Interval('git-sync-poll', 15000) with a dynamic SchedulerRegistry interval
registered in onModuleInit from getGitSyncPollIntervalMs() (cleared in
onModuleDestroy); /status and the real cadence now share one config source.
Boots logging 'poll interval registered (Nms)'.
- loop-guard now ALWAYS applies: the lastUpdatedSource==='git-sync' skip was
nested inside the !spaceId/!workspaceId branch, so structural self-writes
(CREATE/MOVE/RESTORE/SOFT_DELETE, which carry spaceId+workspaceId) bypassed it
and re-triggered cycles. Fetch the page row once, guard unconditionally, then
resolve space/workspace.
- remove the dead PAGE_CONTENT_UPDATED subscription (it's a BullMQ job, never an
EventEmitter event; body edits arrive via PAGE_UPDATED).
- fix the stale datasource comment (PageService DOES stamp 'git-sync' now).
- env getters: parseInt radix 10 + NaN/<=0 fallback for poll/debounce (+ max
deletes), with 6 new environment.service.spec tests.
tsc clean; jest 723 pass; live cycle re-verified post-refactor (ran, push
applied, unflagged 92-page space untouched).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes found by the live pull/push e2e:
- CRITICAL: driveCycle never checked out the 'docmost' branch before
applyPullActions, so Docmost content was written straight onto 'main',
clobbering local file edits before push could diff them. Now checkout
'docmost' before pull (applyPullActions commits there then checks out main +
merges) — mirrors the engine's pull main(). Round-trip now works both ways.
- add an unresolved-merge guard (SPEC §9): skip the cycle if the vault is
mid-merge instead of failing on checkout.
- SAFETY: enabledSpaces() is now STRICT opt-in — only spaces with
settings.gitSync.enabled===true; removed the all-spaces fallback that synced
every space (incl. a 92-page one) the moment GIT_SYNC_ENABLED flipped.
- SAFETY: per-cycle delete cap (GIT_SYNC_MAX_DELETES_PER_CYCLE, default 5):
dry-run the push, and if planned deletes exceed the cap, run the apply with
deletePage neutralized — phantom absence-deletions from a non-convergent vault
can't soft-delete real pages. Fails safe if the dry-run throws.
- fix manual trigger: TriggerGitSyncDto.spaceId needs @IsUUID or the global
whitelist ValidationPipe strips it (arrived undefined -> vault 'undefined').
Live-verified on an isolated flagged space: push (vault file edit -> Docmost
content, stamped lastUpdatedSource='git-sync') and pull (Docmost rename -> vault
file + meta) both work; an unrelated 92-page space stayed untouched throughout.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
