clearStaleGitLocks() removed a hardcoded list of 9 git lock files
UNCONDITIONALLY — despite its name it never checked staleness. In a
multi-replica TTL-lapse window (a documented engine limitation), replica B's
preflight could rm a LIVE index.lock held by replica A mid `git add`/`commit`,
turning a safe fail-fast ("index.lock: File exists") into concurrent
index/ref writes and corruption.
F1: gate each removal by file age. A live git op is bounded by
GIT_EXEC_TIMEOUT_MS (120s), after which git is killed and the lock's mtime
freezes — so a lock older than 3× that (STALE_LOCK_MIN_AGE_MS = 360s) provably
has no live holder and is a genuine crash-leftover. Fresh locks (mtime within
the window) are preserved; missing locks are a no-op. Strictly safer than the
prior unconditional rm (the gate can only prevent removals, never add them).
F4: move clearStaleGitLocks (doc + body) below isMergeInProgress so the
mid-merge jsdoc re-attaches to its real owner; soften the doc (it clears a
fixed list of the engine's own index/ref locks, not "any *.lock").
F2: cycle.test.ts pins the preflight order
ensureRepo < clearStaleGitLocks < ensureMainBranch < ensureBranch (a refactor
that moved clearStaleGitLocks before ensureRepo — deleting ensureRepo's own
transient lock — would now fail the test).
F3: git.test.ts covers ensureMainBranch's HEAD-fallback branch (both main and
docmost gone → main recreated from HEAD) and the no-commit no-op.
Also: the D3-N3 test now backdates the planted lock's mtime so it is stale (and
still removed), plus a new test asserts a FRESH lock is PRESERVED (the
corruption-safety property — this would fail against the old code). cycle.ts's
preflight comment softened to match the mtime gating.
git-sync vitest: 711 passed | 1 expected-fail (+4 new tests). tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ref-store damage (a deleted refs/heads/main, an interrupted ref update) can leave an
existing vault repo without a 'main' branch. The cycle's ensureBranch('docmost','main')
+ checkout then throw every poll ("pathspec 'main' did not match"), wedging the space
forever with no self-heal — ensureRepo only creates branches on a FRESH git init
(found via web-test corruption charter, reproduced deterministically).
Add VaultGit.ensureMainBranch() and call it in the cycle preflight (after
clearStaleGitLocks, before the branch setup): if 'main' is missing, re-create it from
the 'docmost' mirror branch (they track each other) else from HEAD. Same
wedge-forever family as D3-N3.
Verified on the stand: deleting refs/heads/main now self-heals (main restored, the
edit reaches the vault, 0 pathspec errors) — was wedged forever. Unit test (real temp
repo: delete main -> ensureMainBranch restores it from docmost). git-sync suite green (708).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An interrupted git operation (a hard crash / OOM-kill / abrupt container stop mid
`git add`/`commit`/`checkout`) leaves a `.git/index.lock` (or a ref `*.lock`).
Git then refuses EVERY subsequent operation ("Unable to create '…/index.lock':
File exists"), so every poll cycle failed and the space's sync wedged INDEFINITELY
with no self-heal — the whole space stopped syncing until a human ran `rm` on the
lock (found via web-test restart/corruption charter, reproduced deterministically).
The daemon holds the per-space Redis lock and is the vault's ONLY writer, so any
`*.lock` reaching a fresh cycle is necessarily stale (no live git process holds it).
Add `VaultGit.clearStaleGitLocks()` and call it in the cycle preflight, right after
ensureRepo and before the mid-merge recovery — clearing index/HEAD/config/packed-refs/
MERGE_HEAD/ORIG_HEAD and the engine's ref locks (best-effort, missing = no-op).
Verified on the stand: a planted stale index.lock is now cleared and the space
recovers (edit reaches the vault, 0 "File exists" errors) — was wedged forever.
Unit test (real temp repo: index.lock blocks git add -> clear -> git add works);
git-sync suite green (707).
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>
Addresses QA findings on PR #119 (issues #235/#236).
SYNC-WEDGE (HIGH): one same-line conflict on one page froze sync for the
WHOLE space in both directions forever. The pull's docmost->main merge left
the vault mid-merge, so every later cycle's isMergeInProgress() check returned
skipped:"merge-in-progress" and skipped the entire space with no recovery.
- pull.ts now COMMITS a conflicting merge with markers in place (commitMerge):
cleanly-merged pages land, the conflicted page carries its markers on main and
is isolated by the existing push-side conflict-marker skip (markers never reach
Docmost), and the next cycle is no longer wedged. conflictedPaths is surfaced.
- cycle.ts now RECOVERS a vault left mid-merge by a prior/pre-fix cycle: it
aborts the stale merge (merge --abort, hard-reset fallback) and continues,
instead of skipping the space forever.
- git.ts: listUnmergedPaths / commitMerge / abortMerge / resetHardToHead.
CALLOUT TYPE FIDELITY: git-sync's CALLOUT_TYPES was missing "note" and "default"
(editor-canonical types), so [!note]/[!default] callouts flattened to [!info] on
every round-trip. Aligned the list with @docmost/editor-ext getValidCalloutType.
LOSS-ON-FAST-CLOSE: editing a page then closing the tab inside the collab
debounce window (~3-18s) lost the edit, because with unloadImmediately:false
Hocuspocus does not flush the debounced onStoreDocument on the last-client
disconnect. PersistenceExtension.onDisconnect now flushes the pending store
(debouncer.executeNow) on the last disconnect only, with no redundant write.
DUPLICATION re-verify (#1): the schema-default merge-key normalization is intact;
faithful toYdoc-based reproduction shows callout + rich content resync with 0 ops
and no growth/strip across cycles -> the re-report was leftover vault data, not a
live regression. Locked with a callout regression spec.
Tests: git-sync 688 pass (incl. real-VaultGit wedge-recovery integration); server
git-sync+collaboration 285 pass; new callout merge/fidelity + onDisconnect-flush
specs. tsc --noEmit clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review 1863 (delta) on PR #119.
MUST-FIX:
- detailsToHtml (the raw-HTML path used for a details nested inside
columns/spanned cells) now emits `<details${open}>`, mirroring the
top-level case, so `open` no longer silently drops every round trip.
- Remove the dead `resolveApplyClient` delete-cap hook from the engine
`runCycle`: the orchestrator stopped passing it, so the hook + its
dry-run pass were inert. Deletes are soft (Trash) + always logged and
engine convergence is the guard, so no cap is re-added — just the dead
wiring removed.
TEST COVERAGE:
- space-lock: heartbeat refresh CAS-miss (eval -> 0) and Redis-error
(eval throws) both abort the in-flight fn's signal.
- cycle: a pre-aborted signal (and an abort during the pull read) throws
before the push apply / first destructive phase.
- converter: htmlEmbed source VALUE + height survive; encode/decode
UTF-8 symmetry and '' -> ''; footnote definition body + ref/def id
match; transclusionReference both ids survive; fix the bad
transclusionSource fixture (wrong `pageId` attr + empty content ->
schema `id` + a block child); nested details `open` parity test.
- orchestrator: autoMergeConflicts:true reaches engine settings; default
false on a missing settings row.
Co-Authored-By: Claude Opus 4.8 (1M context) <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>
claude code agent 227
agent_qa
a