6956563961ea191a037fd90b387682737d194f11
16 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
e6a861bdaf |
fix(git-sync): a valid-but-nonexistent gitmost_id no longer wedges a space's sync (N1-D1)
A vault file whose `gitmost_id` is a WELL-FORMED UUID that matches no page (a stale id from a restored-from-backup file, or a copied/foreign id) fell through importPageMarkdown to writeBody() on a non-existent page, throwing "Page … not found". The push apply recorded that as a per-cycle failure that never cleared — refs never advance, so the whole space's sync looped on the failure indefinitely (observed live: a leftover orphan file kept a space stuck at "1 failure" every ~5s). Same user-visible impact as C9-D1, but the id is a valid uuid so the 22P02 guard does not catch it. Add the missing `currentPage == null` branch in importPageMarkdown: skip the unknown id as an inert no-op so the cycle succeeds and the rest of the space keeps syncing. Verified on the stand: pushing a valid-but-nonexistent gitmost_id now stays at 0 failures (was 1/cycle forever), logs a skip warn, and a concurrent legit edit still syncs. Unit test added; server suite green (2146). NOTE (separate design follow-up, not this commit): the reconcile still cleans the orphan file (it maps to no live page). ADOPTING such a file as a fresh page (the restore-from-backup use case, preserving the git-authored content) needs the title from the filename, which lives in the engine classifier, not this method. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
5b835fc185 |
fix(git-sync): coerce malformed parentPageId to root in createPage/movePage (#119 F1)
A non-UUID gitmost_id on a parent folder-note, used as parentPageId for a git-sync createPage (or as a movePage destination), wedged the entire space: the throw landed in `failures`, and push only advances refs when failures.length === 0, so the space re-attempted forever. createPage was the only user-influenced-uuid op left unguarded. The throw is a NotFoundException, not a 22P02 error: PageRepo.findById falls back to a slugId lookup for non-UUID input, finds no row, and PageService.create raises NotFoundException — so skipIfMalformedId (22P02-only) would NOT have caught it. Coerce-to-root is the correct fix: a non-UUID parentPageId is rewritten to root (undefined/null) so the page is created/moved at the space root instead of wedging. No data loss (page still created) and no duplication (push.ts writes the assigned id back to frontmatter, so the next sync matches by id, and the retry-adopt map re-parents once the vault id is fixed). Applied to both createPage and movePage (the move destination is reachable via two paths, one 22P02-swallowed-but-mislogged and one NotFound-wedging). The child pageId stays guarded by skipIfMalformedId. F2: softened the skipIfMalformedId comment (parentPageId is a second user-influenced uuid in create/move) and made the swallow log op-generic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
f36a2def73 |
fix(git-sync): a malformed non-UUID gitmost_id no longer wedges a space's sync (C9-D1)
A vault file with a broken/hand-edited `gitmost_id` frontmatter (e.g. `gitmost_id: [unclosed` or a non-uuid token) fed that value into a Postgres `uuid` predicate (page update/delete), throwing 22P02 "invalid input syntax for type uuid". The push apply recorded it as a per-cycle failure that never cleared — refs never advance when failures>0, so the WHOLE space's sync looped on the same failure indefinitely and no further legitimate change synced (found via web-test). Wrap the id-scoped write ops (import/delete/move/rename/restore) at the bind() seam: swallow exactly the 22P02 as an inert no-op so the cycle succeeds and the rest of the space keeps syncing; re-throw anything else. pageId is the only user-influenced uuid in these ops, so a 22P02 there unambiguously means it. Verified on the stand: pushing a non-UUID gitmost_id now logs a skip warn and the space stays at 0 failures (was 1 failure/cycle forever); a concurrent legit edit to another page still syncs. Unit tests: import/delete swallow 22P02, non-22P02 re-throws. Full server suite green (2145). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
5d45f5a85e |
fix(git-sync): close #119 blockers — dead edit-revert guard, cross-space guard, red suite (F5/S2/G1/A1/F7)
F5 (HIGH data-loss): guard #2 (GS-EDIT-REVERT) called a local key-sorting equality that never matched a real page (block ids + materialized defaults differ), so the guard was dead and a web edit on a git-sync space was silently reverted within one poll cycle. Use the package's authoritative docsCanonicallyEqual (strips block id + normalizes KNOWN_DEFAULTS), wired through the git-sync loader like sanitizeTitle; delete the dead local canonicalize/canonicalJsonEqual. S2 (security): importPageMarkdown targeted a page by the vault-file id without a spaceId check (deletePage had one) — a space-A vault file carrying space-B's page id could resurrect/overwrite/clear B's page. Mirror deletePage's guard: skip when the loaded page lives in a different space than ctx.spaceId. G1 (jest green): add sanitizeTitle + docsCanonicallyEqual to the loadGitSync mock; update the converter-gate + package golden expectations to the genuinely-fixed output (paragraph textAlign now round-trips, multi-block table cells emit HTML tables); fix the orchestrator spec's stale mock so the per-space enabled gate (added later) is satisfied. A1: the converter dropped heading textAlign on export (bare '## text'); emit a styled <hN> when aligned, symmetric to paragraphs — round-trips losslessly (level + align), no churn for unaligned headings. F7 (docs): reword the false 'single choke point' title-strip comment; correct push.ts docstrings that still described the removed standalone-CLI/daemon model. Adds regression tests: the F5 acceptance test (canonically-equal content with real uuids => writePageBody NOT called), the S2 cross-space import guard, and the A1 heading round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
fcc9ae0c24 |
test(git-sync): fix #119 review F1-F4 (bind assert, cross-move + strip tests, comment)
F1: git-sync.orchestrator.spec bind assertion now includes spaceId ('space-1'),
matching driveCycle's dataSource.bind({workspaceId,userId,spaceId}).
F2: add 4 non-vacuous tests for the cross-space move data-loss guard in deletePage
(CTX_SPACE with spaceId): move-out skips removePage (returns skipped:'moved-to-other-space');
same-space / not-found / already-deleted all still call removePage.
F3: add 2 tests for the ~<slugId> title-strip guard in renamePage (own slugId stripped;
a foreign ~<slugId> tail left intact).
F4: reword the gitmost-datasource 'single choke point' comment — the strip covers the
rename/update path, not every git-sync title write (createPage's filename-derived title
does not funnel through here).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
906733b5c8 |
fix(git-sync): address PR #119 review #4 — symlink guard, dead-code cull, changelog + warnings/suggestions
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> |
||
|
|
22e3fcdeba |
fix(git-sync): address PR #119 review #2 — throttle /git Basic auth, fix mcp schema drift + warnings/tests
Must-fix:
- Throttle the raw /git HTTP-Basic path: it bypasses Nest/ThrottlerGuard, so
verifyUserCredentials (bcrypt) ran unthrottled. Wrap it in the SAME
FailedLoginLimiter the /mcp path uses (5/60s; per-IP, per-IP+email, global
per-email keys; atomic tryReserve BEFORE bcrypt; success resets, non-credential
errors release). The (threshold+1)-th attempt now gets 429 pre-bcrypt. Sweep
timer + onModuleDestroy mirror McpService.
- Fix the mcp schema mirror drift: packages/mcp details `open` attr now reads via
hasAttribute (matches editor-ext canon + git-sync copy); getAttribute dropped a
bare `<details open>` state. (build/ is gitignored — rebuilt locally.)
Tests added:
- /git brute-force throttle: pre-bcrypt 429 on the 6th failure; success resets;
non-credential error releases the budget.
- git-http-backend lost-lock AbortSignal: already-aborted -> no spawn + 500;
live abort mid-request -> SIGTERM + response closed.
- orchestrator divergentDocmost -> WARN + flag surfaced in status (+ clean case).
- pollTick re-entrancy guard skips an overlapping tick.
- datasource NotFound early-throws (getPageJson/move/rename) + updatedAt:undefined
stale-read branch (importPageMarkdown/createPage).
Suggestions:
- space.repo updateGitSyncSettings: parameterize the jsonb key (`${prefKey}::text`)
instead of sql.raw (latent-injection footgun); value stays sql.lit. Spec updated.
- pollTick re-entrancy guard (private `polling` flag).
- page-change.listener docstring: honest about the move/rename/delete over-skip
(loop-guard keys only on lastUpdatedSource) -> ~poll-interval latency, not loss.
- AGENTS.md: document the root /git smart-HTTP route + GitSyncModule.
- Remove redundant redteam-provenance.spec.ts (covered e2e in
persistence.extension.spec.ts:145).
- Extract the duplicated SIGTERM->SIGKILL+finish block (watchdog + abort) into
terminateChild; centralize watchdog-timer teardown in done().
Architecture (deferred, documented): mcp schema header now carries the three-copy
keep-in-sync + schema-core note; the editor-ext contract test documents that the
mcp copy and attribute-behaviour drift (details `open`) are not mechanically
covered yet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
9e69d917ee |
fix(git-sync): converge git-ingest with open editor sessions — stop silent revert/data-loss on live pages
A git push to a page with an OPEN editor was silently reverted: the git commit landed and the DB body updated, but the page in the browser stayed on the old content and the editor's next autosave overwrote the git change. Root cause (distributed, not in the merge): writeBody applied the body merge via collabGateway.openDirectConnection on whichever instance/process runs git-sync (the api/worker). When an editor is connected to a DIFFERENT collab instance/process, that opens a SEPARATE, detached Y.Doc. The merge landed in the detached doc + DB, but the live editor's Y.Doc never received the Yjs update; its debounced autosave then persisted its STALE state over the DB, reverting the git change (and, for concurrent edits to different paragraphs, losing the git side). In one process the bug is invisible because the direct connection already shares the editor's doc. Fix: route the body write through the existing custom-event channel (the same mechanism comment-marks and updatePageContent use) so the merge runs on the instance that OWNS the live doc. Its update is then broadcast to every connection (Document.handleUpdate) and the editor's CRDT converges on the merged result. New CollaborationGateway.writePageBody dispatches to a new gitSyncWriteBody handler (builds incoming/base docs before opening the connection — crash-safe — then 3-way/2-way merges into the live fragment); without redis it runs locally on the single (owning) instance. writeBody now just forwards the converted ProseMirror bodies + service userId. Evidence: - git-ingest-convergence.spec.ts: deterministic two-Y.Doc repro. PATH B (undelivered update) asserts the LOSS (the bug); PATH A (update delivered, as the owner-routed write does) asserts the git change SURVIVES and that concurrent edits to different paragraphs both survive. - collaboration.handler.git-sync.spec.ts: exercises the real gitSyncWriteBody against a shared doc wired to a connected "editor" doc (models the owning-instance broadcast) — editor converges, concurrent edit preserved, crash-safe on transform failure. - gitmost-datasource.service.spec.ts: writeBody now routes via writePageBody (RED before this change — it called openDirectConnection). Honest scope: the failure is cross-instance; full multi-instance convergence needs a live Hocuspocus + redis and is not provable in a unit test, so the convergence invariant is captured at the Yjs update-exchange level. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
5125296bfa |
fix(git-sync): subpages round-trips (was {{SUBPAGES}} literal) + exhaustive all-node round-trip test
subpages exported to the literal `{{SUBPAGES}}`, which has no markdown/HTML
inverse, so on re-import it came back as a plain paragraph holding the visible
text "{{SUBPAGES}}" — the embed rendered as that literal string on the page
after a sync (round-trip data loss, seen live). It now emits the schema-matching
`<div data-type="subpages">` like every other embed node, so the schema's
parseHTML rebuilds the subpages node. Also dropped the leaf-atom content-hole
in the subpages renderHTML.
New committed regression coverage:
- packages/git-sync/test/roundtrip-all-nodes.test.ts — exhaustive serialize ->
deserialize round trip for ALL 40 node/mark types; each asserts the node/mark
survives and no `{{...}}` literal leaks. This is the test that caught subpages.
- §13.1 gate (git-sync-converter-gate.spec.ts): subpages added to the green
corpus (round-trips through the REAL server schema).
- Corrected two PR-authored tests that asserted the old {{SUBPAGES}} loss as
"by design" — they now assert the fixed round trip.
Also folds in review #1679 coverage-gap tests (no prod change): orchestrator
pollTick/enabledSpaces, datasource 3-way merge dispatch, page.repo
last_updated_source provenance SQL.
git-sync vitest 659 (+1 expected-fail), server tsc clean, server specs green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
28d2560dfd |
fix(git-sync): address PR #119 review (#1571)
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> |
||
|
|
5da12e89f9 |
refactor(git-sync): internalize the engine — first-class ESM, no vendoring bridge (#119 review)
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> |
||
|
|
cca0bfe306 |
docs(git-sync): remove dangling references to the deleted git-sync-plan doc (PR #119 review)
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> |
||
|
|
177d8a31d4 |
fix(git-sync): hold refs on suppressed deletes + stamp delete/restore provenance (PR #119 review)
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> |
||
|
|
3386bf2865 |
fix(git-sync): merge git body into the live doc block-by-block (no clobber)
Supersedes the active-session "defer" guard with a real merge (review #5 — "запись делать через мерж", not skip-while-editing). writeBody no longer does delete-all + re-insert (which discarded a concurrent editor's in-flight changes on every sync). It now diffs the live body against the incoming git body at TOP-LEVEL BLOCK granularity (LCS over a canonical structural serialization) and applies only the minimal inserts/deletes: - a block a human is editing is left UNTOUCHED when git changed a DIFFERENT block; - an unchanged resync is a complete 0-op write; - Yjs CRDT-merges the minimal ops with concurrent edits. New yjs-body-merge.ts (mergeXmlFragments + cloneXmlNode + diffBlocks) is pure-Yjs and unit-tested with real Y.Docs (8 tests): identical->0 ops, edit-one-block keeps the other block instances, append/delete keep neighbours, marks survive the cross-doc clone. Crash-safety kept: the incoming doc is built before the connection opens, so a transform failure can't empty the body. Removed: the ActiveEditSessionError defer path and the now-unused CollaborationGateway.getActiveEditorCount. Honest limitation: this is a 2-way merge — for a block BOTH sides changed since the last sync, git wins (no common ancestor to decide). A full 3-way merge would need the last-synced base plumbed from the engine; the dominant cases (unchanged resync, edits to different blocks) are now lossless. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
181a8330f3 |
fix(git-sync): don't clobber pages with a live editing session; crash-safe body write
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
|
||
|
|
5a4d9f84d7 |
feat(git-sync): native GitmostDataSource + 'git-sync' provenance (Phase A.4a)
Native data plane for git-sync (plan §3, §8.1):
- provenance: widen actor to 'user'|'agent'|'git-sync' (jwt-payload,
auth-provenance decorator); PersistenceExtension resolves lastUpdatedSource
with precedence agent > git-sync > user, debounced history (like a human edit,
not the agent's immediate snapshot).
- GitmostDataSourceService implements @docmost/git-sync's GitSyncClient natively:
reads via PageRepo/SpaceRepo (listSpaceTree complete:true, getPageJson), writes
via PageService (create/removePage soft-delete/movePage with computed fractional
position/update-rename/restore) + the writeBody linchpin through collab
openDirectConnection('page.'+id, {actor:'git-sync'}) mirroring
collaboration.handler withYdocConnection 'replace'. bind({workspaceId,userId})
returns the context-bound client for the orchestrator.
- 10 unit/contract tests (mapping + soft-delete + move-position), tsc clean.
Known gap (closed in A.4b): PageService.create/update/movePage only branch on
actor==='agent'; git-sync provenance is already passed through so the row source
marker propagates once PageService honors 'git-sync'. Module/orchestrator/config
come next.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|