Bug #1 (push 503 starvation): an external receive-pack that briefly overlapped
a poll cycle immediately 503'd because the per-space single-writer lock was
held. Add a BOUNDED retry-acquire on the PUSH path only (SpaceLockService
.withSpaceLock acquireRetry: capped exponential backoff up to ~5s); a transient
overlap now waits and succeeds, a genuinely stuck cycle still 503s after the
bound. The poll cycle passes no retry (immediate skip). Push result stays
deterministic: the receive-pack only runs once the lock is held, so a 503 never
leaves a half-applied ref.
Bug #2 (concurrent-edit marker leak + silent same-block loss):
- Marker leak (a): the push UPDATE path stripped markers for the body sent to
Docmost but left raw <<<<<<</>>>>>>> committed on the published `main` vault
forever (autoMergeConflicts ON). Now the cleaned body is written back to the
vault file + recorded in writtenBack so runPush commits it on `main` and the
vault converges to clean bytes.
- Marker leak (b): pin merge.conflictStyle=merge in ensureRepo and teach
stripConflictMarkers/hasConflictMarkers about the diff3 `|||||||` base section
(drop the marker AND the stale base region) so diff3/zdiff3 conflicts can
never leak `|||||||` + base content into a page. Also scrub the 3-way merge
BASE markdown.
- Silent same-block loss: the block 3-way merge still resolves same-block
conflicts deterministically to git, but it is no longer silent: diff3Plan now
reports a conflict count (mergeXmlFragments3WayWithStats), gitSyncWriteBody
logs it, and the persistence boundary-snapshot now fires for git-sync writes
over a non-git-sync baseline so the human's pre-merge content is preserved in
page history (recoverable). Full both-preserved persisted-conflict UI remains
the deferred redesign.
Tests: space-lock bounded-retry (success/stuck/poll-immediate); push vault-clean
+ diff3 ||||||| strip; ensureRepo conflictStyle pin; diff3Plan/3-way conflict
counts; persistence git-sync boundary snapshot. Server tsc clean; git-sync
vitest + server collaboration/git-sync jest all green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Blocking (review id 2514):
- [security] Forbid symlinks in vaults. ensureServable now sets
core.symlinks=false in each vault's local git config (a pushed symlink is
checked out as a plain file, never a real link), and the engine cycle wraps
every read/write/mkdir in an lstat/realpath guard (new path-guard.ts) that
refuses a path that is — or traverses — a symlink, or whose realpath escapes
the vault root. Prevents a writer from publishing /etc/passwd or the server
.env, or writing outside the vault. Adds unit tests (path-guard.test.ts) +
a read-guard integration test (cycle.test.ts) + real lstat/realpath in the
roundtrip integration test.
- [simplification] Delete dead lib/diff.ts + test/diff.test.ts and drop the
now-unused @fellow/prosemirror-recreate-transform dependency.
- [documentation] Add a CHANGELOG [Unreleased] → Added entry for git-sync.
Warnings:
- [test-coverage] Cover the CREATE-branch conflict-markers guard (a new .md with
markers and no gitmost_id is recorded as a create failure, never created).
Suggestions:
- [stability] Bound each `git config` in ensureServable with a timeout.
- [authz] Trigger endpoint resolves spaceId workspace-scoped and 404s a foreign
space before any vault directory is created.
- [stability] Attribute git-initiated moves to the service account
(lastUpdatedById), via an optional actor param on PageService.movePage.
- [documentation] Document the per-space autoMergeConflicts toggle in AGENTS.md.
- [test-coverage] Cover the unterminated `:::` callout fence fallback.
- [simplification] Move test-only roundtrip-helpers.ts out of src/ into test/.
Architecture:
- Move the Yjs/ProseMirror merge primitives (yjs-body-merge, three-way-merge,
lcs + specs) into collaboration/merge/, breaking the collaboration →
integrations/git-sync dependency cycle this PR introduced.
- Port the schema-surface drift gate to packages/mcp (the mcp schema mirror had
none); pins 52 entries.
Deferred (with rationale in the review thread): the incremental-pull perf
warning (correctness-neutral; needs a high-water-mark design + its own tests on
the data-loss-critical path) and the redis-sync rolling-deploy mixed-version
edge (the deficient behavior is in already-released old-instance code; the new
code is correct on both sides; impact is a transient rollout-window artifact).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
