QA follow-up on 5d45f5a8: that commit taught the converter to export heading
textAlign (<hN style>) but left the converter-gate heading test still asserting
the OLD dropped behavior (expects a bare '## text'), so jest was RED — the G1
green-suite gate was not actually met. Two gaps closed:
1. Flip the heading KNOWN-DIVERGENCE gate test to assert the round trip now
PRESERVES alignment (exported as <h2 style="text-align:center"> and recovered
on import), matching the shipped converter behavior. Suite is green again.
2. blockToHtml (the nested-container path: heading/paragraph inside a
column/table/callout) still emitted bare <hN>/<p>, dropping textAlign for
nested blocks. Carry the style there too, symmetric with the processNode path.
Also add #7 (table inside a column) and #8 (multi-block table cell) to the
lossless round-trip CORPUS so both survive export->import through the real
editor-ext schema (columns widthMode pre-authored at its normalize fixpoint).
Verified: server jest 193 suites / 2142 tests green, git-sync vitest 704 green,
no type errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Two table round-trip losses from review #4404:
- #7: a table inside a column was emitted as a GFM pipe table INSIDE the raw-HTML
<div data-type="column"> wrapper. marked does not parse markdown inside a raw
HTML block, so on re-import the table became literal "| a | b |" text.
- #8: a table cell holding block content (a list, code block, multiple paragraphs)
only used the HTML <table> form when a cell had colspan/rowspan; otherwise the
GFM pipe path flattened the cell's blocks onto one line and lost the structure.
Extract a shared tableToHtml() helper and use the HTML <table> form when: a cell
is merged (existing), a cell is multi-block (#8), OR the table is rendered in a
raw-HTML context — blockToHtml's table case now routes through tableToHtml (#7).
Verified on stand: a table-in-column exports as <table> inside data-type=column;
a cell with a bullet list exports as <table> with <ul><li>, and a git-side edit
+ re-import keeps the bulletList + both items in the DB content (was flattened).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dead code / doc-vs-code cleanup from review #4404:
- Drop the unused Settings.docmostApiUrl/Email/Password fields (the native
in-process datasource never reads them; the engine only used docmostSpaceId).
Removed from the type, buildSettings, and the 4 engine test suites that
fabricated them.
- Warn ONCE at startup when GIT_SYNC_REMOTE_TEMPLATE is set — remote push is
deferred (SPEC §7) so the value is currently inert; the operator now gets a
log line instead of a silent no-op.
- Correct stale docstrings that claimed live-destructive code was 'FAKES only /
NEXT increment / no live wiring' (applyPushActions runs LIVE via
runCycle->orchestrator); that importPageMarkdown receives a 'self-contained
file (meta+body)' (it receives the stripped body); and that the Yjs body merge
is '2-way' (3-way runs end-to-end when the base is present).
No behavior change (except the new warning); build + git-sync smoke verified.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Aligned paragraphs were exported as <div align="…"> which had NO matching
import parse rule — the div was unwrapped and the alignment lost on every
round trip. Emit a styled <p style="text-align:…"> instead, and give the
textAlign global attribute (docmost-schema) an explicit parseHTML that reads
element.style.textAlign (and legacy align=) plus a renderHTML that writes the
style. Now heading/paragraph alignment survives Docmost->git->Docmost.
Verified on stand: a textAlign=center paragraph exports as
<p style="text-align:center">, and after a git-side edit + re-import the
paragraph still has textAlign=center in the DB (was null before).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Review #4: forward HTTP_CONTENT_ENCODING to git http-backend so it inflates
gzip'd RPC bodies — a non-trivial `git pull` no longer fails with
`fatal: expected 'packfile'`. (git-http-backend + git-http.service)
- Review #5: the read-advertisement branch runs under the space lock AFTER
reply.hijack(); a reject there (e.g. Redis down) previously left the socket
open forever, hanging every clone/fetch. Mirror the push branch: catch, 500 if
unwritten, always end the socket. (git-http.service)
- GS-EXPORT-500 (QA): a page with an inline comment mark returned HTTP 500 on
Export/copy-as-markdown. The Comment mark's renderHTML took the imperative
document.createElement branch server-side (the DOM shim used by generateHTML
defines window/document), returning a live node with no content hole that
crashed prosemirror-model's DOMSerializer under happy-dom. Gate the imperative
branch on a real browser (navigator.userAgent contains 'Mozilla'); the server
now uses the static DOMOutputSpec form. Verified: export 200 (was 500).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses reviewer comment #4404 (critical + blocking):
- Critical #2: renamePage skips the echo where the incoming title equals
sanitizeTitle(current title) — a Docmost title with FS-hostile chars (: / " |,
newlines, double-space, >120) was pulled to a sanitized stem then written back,
permanently corrupting the real title. (datasource)
- Blocking #3: runOnce enforces per-space settings.gitSync.enabled (the event
path bypassed opt-in; any edited space would git-init + export). (orchestrator)
- Blocking #6: movePage no-ops the position-less same-parent echo that clobbered
the user's chosen sibling order. (datasource)
- Blocking #9: hasConflictMarkers is fence-aware — '<<<<<<< HEAD' inside a code
block (git-tutorial page) no longer trips the all-or-nothing gate that froze
the whole space's refs. (push.ts)
- Blocking #11: three-way tryMergeRegion short-circuits when live==target (diff3
agreement) instead of logging a false 'same-block conflict resolved to git' —
the echo noise that masked real data-loss signals. (three-way-merge)
- Blocking #12/#13: e2e-advanced — drop the delete-cap block (no such feature;
failed with a scary '(data loss!)'); non-member assert now expects 404 (existence
not leaked), not 403.
Verified on stand: sanitized-title rename preserves DB title (vault file
sanitized); non-enabled space creates no vault; fenced conflict markers ingest
without jamming; build clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a new value "inline" to the image align attribute (alongside
left/center/right/floatLeft/floatRight). Inline images render as
inline-block containers, so consecutive ones form a row that wraps
naturally on narrow viewports; unlike the float modes, text does not
wrap around them.
- applyAlignment: reset-then-apply extended to display/vertical-align;
the reset restores the constructor's inline display:flex so non-inline
modes keep byte-identical styles and editor-ext stays independent of
the client CSS class
- image bubble menu: new "Inline (side by side)" button (IconLayoutColumns)
with active state, mirroring the float buttons
- i18n: key registered in en-US and ru-RU ("В ряд"), like the float labels
- tests: 3 new applyAlignment specs (apply, reset on switch-away, float->inline)
- no schema/MCP/markdown changes needed: align round-trips as data-align
Reviewer round 1 on the #260 collab-doc-name fix:
- F1: replaceImage is the one path where the resolved UUID gates BOTH the
collab-doc open AND the per-page mutex key (withPageLock(pageUuid)). Add a
deterministic test to resolve-page-id-collab-doc-name.test.mjs: it gates
/files/upload so replaceImage parks mid-upload holding its lock, asserts the
doc opened as page.<uuid> (never page.<slug>), and probes the SHARED
page-lock chain — a withPageLock(UUID) probe must stay blocked while
replaceImage holds it (with a free-key probe as a non-vacuity guard). The
test fails if the lock key is reverted to the slugId (verified).
- F2: drop the dead `pageIdCache.set(uuid, uuid)` — resolvePageId returns on
the isUuid() short-circuit before the cache is ever read with a uuid key, so
only slugId->uuid entries are stored/read. Comment corrected to match.
MCP suite 430/430, tsc 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Real root cause of the silent MCP edit loss: the web editor always opens the
collaboration document by the page UUID (`page.${page.id}`), but the MCP
opened it by the agent-supplied id — usually a slugId — so `page.${pageId}`
became `page.<slugId>`. For one DB page that is TWO independent Yjs documents;
both persist to the same `pages` row (findById/updatePage resolve id or
slugId), so the human tab's debounced store overwrites the agent edit
(last-store-wins) — gone after reload, never shown live. The slugId doc also
made the server's transclusion sync + embedding reindex throw Postgres 22P02.
Fix:
- MCP (primary): resolvePageId(pageId) returns the canonical UUID — a UUID
short-circuits with no network call, a slugId resolves once via getPageRaw
and is cached both ways. Every collab-write path (mutatePageContent /
updatePageContentRealtime / replacePageContent and the mutate/replace/
unlocked seams) now opens by the resolved UUID, so the MCP and the editor
share ONE Yjs doc. replaceImage's whole-operation page lock also keys on the
UUID so it serializes against the other (now-UUID-keyed) writes.
- Server (defense + kills the 22P02 noise): onStoreDocument passes the resolved
page.id — not the raw doc-name id — to syncTransclusion, the embedding queue,
the mention-notification job, addContributors, and the in-tx history read.
Content store and the empty-guard are untouched.
Tests: a new MCP test stands up a real Hocuspocus server and asserts a slugId
input opens `page.<uuid>` (never `page.<slugId>`), with UUID short-circuit and
single-resolve caching; the server spec asserts the side-effects receive the
UUID for a `page.<slugId>` doc. closes#260
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stashPage is declared in the server's DocmostClientLike interface and
shipped as the stash_page MCP tool (client.ts, tool-specs.ts, index.ts),
but the hand-maintained HOST_CONTRACT_METHODS mirror in the contract test
was never updated — so the drift-guard test failed and broke CI's
unit-test job. Add the missing name; both directions now agree.
After merging develop, git-sync's markdown converter still predated two
editor features and silently dropped them on every sync:
- Spoiler mark (#259): had no schema entry, so `<span data-spoiler>` from
the canonical lossless form re-parsed as plain text and the mark was
lost. Register a Spoiler mark mirroring @docmost/editor-ext + the MCP
schema, and emit `<span data-spoiler="true">…</span>` on PM->MD.
- Image caption (#221): the converter assumed no caption attribute existed
(the branch was dead). The image schema now carries a plain-text caption
(data-caption); register it and route a captioned image through the raw
<img> form (same lossless convention as the other Docmost image attrs).
Caption-less images keep the lighter `` form.
Both survive PM->MD->PM unchanged; raw `<span data-spoiler>` / `<img
data-caption>` in incoming markdown parse back. New round-trip tests in
markdown-roundtrip-spoiler-caption.test.ts; schema-surface snapshot updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merging the image-captions (#221) and lossless-export branches each added
its own escapeHtmlAttr in turndown.utils.ts, producing two implementations
of the same function and breaking `tsc --build` (TS2393) — which failed the
Build editor-ext step across all CI jobs.
Drop the lighter image-captions duplicate (escapes & and ") and keep the
fuller version (escapes & " < >). It is a strict superset: both call sites
(serializeAttrs, the image rule) place the value inside a double-quoted HTML
attribute, where extra < > escaping is harmless and idempotent on re-import.
Verified: editor-ext builds; turndown.dataloss + image-markdown tests pass.
F1 (data loss): packages/mcp keeps its own copy of the document schema
(AGENTS.md), and the spoiler mark was only added to editor-ext + the server
tiptapExtensions, so a doc with a spoiler silently lost the mark through /mcp.
Add a local Spoiler mark to docmostExtensions (span[data-spoiler] parse,
data-spoiler="true"+class render) and a case "spoiler" in markdown-converter
emitting the same <span data-spoiler="true">…</span> as the editor-ext turndown
rule; add an MCP json->md->json round-trip test. Regenerated build/lib output.
F2: add the #259 inline-spoiler entry to CHANGELOG [Unreleased] Added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The special-chars test only checked substrings (data-caption=/Tom/Jerry) that
survive even if escapeHtmlAttr stopped escaping " or double-encoded &. Assert
the exact escaped attribute in the intermediate Markdown
(data-caption="Tom & "Jerry"") and re-parse the rendered HTML to
confirm the recovered caption is exactly Tom & "Jerry".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an inline spoiler (Telegram/Discord-style hidden text): a TipTap mark
`spoiler` rendered as <span data-spoiler="true" class="spoiler">, blurred via
CSS and revealed on click (UI-only is-revealed class, never persisted).
- packages/editor-ext: the Spoiler mark (inclusive:false, set/toggle/unset
commands, ||text|| input rule), exported; a lossless turndown rule emitting
raw inline HTML; round-trip test.
- apps/client: SpoilerView mark-view (ReactMarkViewRenderer, Link pattern),
registration in extensions, bubble-menu toggle button (editable only), CSS
(blur + @media print reveal), en/ru i18n.
- apps/server: register Spoiler in collaboration.util tiptapExtensions so the
mark survives HTML<->JSON export/index/import/Yjs; a test proving the public
share keeps the spoiler (it isn't stripped with comments).
No keyboard shortcut: the proposed Mod-Shift-s collides with Strike (and
Mod-Shift-h with Highlight); the ||text|| input rule + the bubble-menu button
cover ergonomics.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment claimed vitest skips the file because it has no test cases; vitest
collects by filename glob, so the real reason is the name not matching
*.{test,spec}.ts. Reword to cite the glob and warn that adding test cases here
would not run them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The schema + cell/row/table/doc builders + grid/stateFor/trFor were copied
verbatim into the 3 new table-utils test files (and the pre-existing
table-utils.test.ts) — a schema change would have to be synced across all four.
Move them into a shared table-test-helpers.ts (test-only, excluded from the
build like footnote-corpus.ts) and import it everywhere; cell uses the
(txt, attrs?) superset (a drop-in for the bare (txt) copies). No assertion
changes — test counts unchanged (223 passed + 3 expected-fail).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F8: extract emitMoveWithBody helper (renamesMoves + body update with
basePath=oldPath) and call it at all three rename emission sites (ghost-move
A, ghost-move M, R/C) — byte-identical behavior, single F4 rationale. Helper
placed above computePushActions so the planner JSDoc stays attached.
F7: add an M-side ghost-move test (D+M same pageId) asserting the move and the
body update carry basePath=oldPath — the previously-untested branch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F4: a rename/move + body edit in one diff used to lose the edit (renamed pages
went only into renamesMoves, never updates). Now computePushActions also
emits an updates entry for renames, AND threads the OLD path via a new
UpdateAction.basePath so applyPushActions resolves the 3-way merge base from
the pre-rename file. Without it the base lookup at the new path returns null
and degrades to a 2-way merge that rolls back concurrent Docmost edits; with
it the edited block wins while a concurrent edit to another block survives.
A plain (status M) update carries no basePath and is byte-identical to before.
F5: test the CREATE path stripping conflict markers (autoMergeConflicts on).
F6: .env.example documents GIT_SYNC_REMOTE_TEMPLATE as deferred/inert scaffolding.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
uploadImage is internal to client.ts (called by insertImage/replaceImage);
the MCP transport (index.ts) does not call it directly. Remove it from the
comment's list of transport-called methods.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract pure extractAuthTokenFromSetCookie from performLogin (behavior-identical)
so cookie parsing is unit-testable without a network login. Add round-trip
coverage for media attrs (width/height/align/drawio/escaping) the existing
suite omitted; applyAnchorInDoc selection/ambiguity/atom-break cases; and a
cross-copy drift guard proving the vendored editor-ext recreate-transform and
the @fellow npm copy used by diff.ts emit identical steps (apply(diff)==target).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F2: the real-git modify/delete null-edge test docstring overclaimed it
caught loss of the `?? theirs` fallback end-to-end. git itself leaves
theirs in the working tree (stage 3) so commitMerge's `git add -A` would
stage it even with the bug — the assertions pass on broken logic. Reword
to state it verifies the clean-merge happy path; the real F1 regression
guard lives in the fake-fs apply-pull-actions.test.ts.
F3: fill the `round-?` placeholder with `round-2` in both new blocks to
match the file convention (header: 'QA #119 round-2').
Comment-only; no production or test-logic changes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The DocmostClientLike mirror covers only methods the in-app adapter consumes;
the standalone MCP transport calls additional client methods not tracked here
(covered by its own typecheck). Fixes the misleading 'superset' wording (F2).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A captioned image in a column is emitted via the imageToHtml helper, a
separate path from the top-level image case whose data-caption branch was
untested. Add a round-trip test with special chars (Tom & "Jerry") that
fails if the imageToHtml caption branch breaks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment referenced markdownToHtml, which does not exist in the mcp
package; the import path is marked.parse + generateJSON (which runs the
image extension's parseHTML). Describe the actual step and regenerate the
build artifact in sync.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The setImageCaption command and its Commands<> declaration were dead:
captions are written via the generic updateAttributes in
useImageTextFieldControl, and a repo-wide grep finds zero callers.
Remove the speculative implementation (image.ts) and its type
declaration.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The genuine-conflict branch in applyPullActions resolves to `ours ?? theirs`,
but the two stages where a side is ABSENT had NO test — the existing conflict
tests only fed stages where both ours and theirs are non-null. This is the
data-preservation core on the published `main`: a regression (dropping the
`?? theirs`, or wrongly writing on both-null) would silently lose a surviving
Docmost edit or resurrect a both-deleted page.
Adds four tests:
- apply-pull-actions.test.ts (fake-git, controlled stages): modify/delete
(ours=null, theirs!=null -> keep THEIRS) and delete/delete (both null ->
write nothing, deletion staged by commitMerge's `git add -A`).
- pull-conflict-normalize.test.ts (real-git 3-way): modify/delete built by
deleting on main + modifying on docmost (stage 2 absent -> theirs kept,
committed clean, no markers); delete/delete built via a rename/rename(1to2)
on the shared base file, which records the original path as both-deleted
(stages 2 AND 3 absent -> nothing written, deletion committed off main).
Production logic at pull.ts:487-497 held — pure test-coverage fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The escaping round-trip test's data (A & "B") only contained & and ",
so the <,> branches of escapeHtmlAttr (&,",<,>) and escapeHtmlText (&,<,>)
were never exercised; a regression dropping <,> escaping would still pass.
Extend the data to A & <B> "C" in both the data-label attribute and the
visible text so both functions' <,> branches are genuinely covered. Assert
the well-formed escaped tag (attr: A & <B> "C", text:
A & <B> "C"), explicitly reject the raw tag-corrupting forms,
and confirm markdownToHtml restores the originals. Comment updated to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Architect-review hardening of the bidirectional DocmostClientLike <->
HOST_CONTRACT_METHODS guard (test-only, no production change):
- Interface method-name regex now accepts full TS identifiers
(digits/_/$) and generic signatures (method<T>(), avoiding a future
benign false-FAIL.
- Skip /* ... */ block comments in the interface body so a `name(` line
inside one is not falsely parsed as a method.
- Wrap the cross-package readFileSync with a clear "expected monorepo
layout" error instead of a bare ENOENT when run outside the monorepo.
- Narrow the guard's comments/error to state plainly it checks the
method-NAME set only; signature parity remains the deferred staged-plan
item.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review F2. The escapeHtmlAttr (&,",<,>) and escapeHtmlText (&,<,>)
helpers in turndown.utils were untested — every existing round-trip case used
alphanumeric values, so no escape branch ran. A mention/status carrying HTML
special chars would re-emit malformed HTML that import's parseHTML can't
restore → the same data loss this PR fixes, uncaught.
Add a round-trip case to turndown.dataloss.test.ts: a mention with `&` and `"`
in both data-label and visible text. Assert (a) the exported Markdown carries
the correctly-escaped, well-formed tag (data-label="A & "B"",
text escapes &), not the raw malformed form; and (b) markdownToHtml restores
the original unescaped values (attribute `A & "B"`, text `@A & "B"`).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add PM -> markdown -> PM round-trip assertions for image caption
(plain and special-char), which fail without F1 and pass with it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stock @tiptap/extension-image carries no caption attribute, so
markdownToProseMirror through docmostExtensions dropped the
data-caption the client emits, breaking the lossless claim. Extend the
Image node (mirroring editor-ext image.ts and the nearby Highlight
extend) to parse/render data-caption. Rebuilt build/.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The contract test only checked one direction (each name in
HOST_CONTRACT_METHODS exists on the real DocmostClient). But
HOST_CONTRACT_METHODS is itself a hand-copy of the server's
DocmostClientLike interface (docmost-client.loader.ts), and that
list<->interface link was untested: a method added to the interface +
consumed by the adapter but forgotten in the list (or removed from the
interface but left in the list) would escape both the server typecheck
(the pkg emits no .d.ts) and the existing test (name not in the list) ->
a runtime "x is not a function" in a tool call.
Parse the method names from the DocmostClientLike interface body (read
the .ts source via import.meta.url, scan member-signature lines) and
assert.deepEqual them against HOST_CONTRACT_METHODS BOTH ways. Lists are
currently identical (39=39), so this is a coverage hole closed, not a
live bug.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three more git-sync QA defects from the 2nd live pass on PR #119, plus a
callout-fidelity nit:
1. SPURIOUS conflict leaked raw markers into canonical main (root cause). On an
ordinary round-trip the only difference between the docmost mirror (normalize-
on-write) and a user's raw push is trailing/empty-line normalization, which made
git's line-based docmost->main merge CONFLICT, and the wedge fix then committed
the file WITH literal <<<<<<< / ======= / >>>>>>> markers onto main (git and the
DB silently diverged for cycles). Fix: on a conflict, normalize trailing/empty
lines on BOTH sides (showStage :2:/:3:) before comparing — a trailing-only diff
is recognized as spurious and resolved to the clean normalized form. A GENUINE
same-block conflict is auto-resolved to OURS (git wins, mirroring the live-doc
3-way rule); the docmost side stays on the `docmost` branch + page history. Raw
markers NEVER reach main again.
2. Concurrent UI<->git edit silently lost the UI side. The git->Docmost 3-way merge
ran against a live Y.Doc that hadn't yet received the user's debounced in-flight
edit, so git clean-applied (no conflict detected) and the edit vanished even on a
different block. Fix: flush the pending debounced store before the merge so the
in-flight edit is drained into the live doc first — a different-block edit is
merged, a same-block one is detected and pinned to history (recoverable).
3. Smart-HTTP HEAD flapped to the read-only `docmost` mirror (~1/4 of clones). The
engine transiently checks out `docmost` mid-pull and the host advertises whatever
HEAD resolves to. Fix: VaultGit.pinHeadToMain(); the cycle restores HEAD->main in
a finally; and the upload-pack ref advertisement is served HEAD-pinned under the
per-space lock so it can never observe a mid-cycle HEAD.
4. (callout) clampCalloutType now mirrors the editor's GITHUB_ALERT_TYPE_MAP for
non-schema aliases (tip->success, caution->danger, important->info) instead of
flatly collapsing to info. The editor schema genuinely supports only the six
banner types, so unknown types still fall back to info (by design).
Tests: deterministic real-git trailing-blank round-trip (no conflict, no markers,
in sync over 2 cycles) + genuine-conflict no-marker-leak; HEAD advertisement
stability; pre/post-flush concurrent-edit survival; serveReadAdvertisement lock
pin; widened callout-alias coverage. Engine vitest + server tsc + collaboration /
git-http / orchestrator specs all green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mandatory (test-coverage):
- internal-file-urls.test: pin the SSRF/traversal ACCEPT path of
resolveInternalFilePath (the sole guard for content-controlled `src`): an
absolute/protocol-relative URL has its foreign host dropped and only an
/api/files/ pathname survives (http://evil.com/api/files/x/y.png -> /files/x/y.png),
while a host-dropped path that escapes /api/files/ (https://evil.com/api/auth/whoami)
or a backslash-traversal (/api/files\..\auth\whoami) is rejected. Locks the
behavior so a future prefix-only refactor cannot silently open a bypass.
Suggestions:
- index.ts: the stash_page MCP tool now returns structuredContent
{ uri, sha256, size, images } alongside the resource_link, so the MCP output
matches the documented shape (clients get the blob's sha256/ETag and the
mirror counts, not just the link). No outputSchema registered. Rebuilt build/.
- new stash-page-mcp-result.test: server round-trip via InMemoryTransport asserts
both the resource_link and the structuredContent mirror.
- internal-file-urls.test: cover the new URL parse-failure catch branch
(http://[ -> "Invalid internal file src").
- environment.service.spec: assert getPositiveIntEnv warns once per key and
independently across keys (the invalidPositiveIntWarned dedup).
Tests: packages/mcp 383 pass; apps/server sandbox/environment/mcp 235 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
Security (must-fix):
- sandbox.controller: the anonymous GET /api/sb/:id response now sets
X-Content-Type-Options: nosniff, a restrictive CSP, and Content-Disposition=
attachment for any mime outside a raster-image allowlist (png/jpeg/gif/webp/
avif). entry.mime is attacker-controlled, so an evil.svg/evil.html could
otherwise execute script inline on the Docmost origin (stored XSS). Mirrors
the public attachment route's hardening.
Stability:
- client.stashPage: reconcile mirrors AFTER the final document put, not only
before it. The doc blob is the newest entry and FIFO eviction drops the
oldest = this stash's own images, so the stored doc could reference an
evicted blob (consumer 404) and over-report images.mirrored. A bounded loop
now reverts doc-put-evicted mirrors, drops the stale doc blob, and re-puts
until stable. Regenerated packages/mcp/build/.
- sandbox.controller: emit Cache-Control on the 304 branch too (ttlSeconds is
computed before the conditional check).
Docs:
- Bump the MCP tool count 39 -> 40 across all READMEs and AGENTS.md (the
registry now exposes exactly 40 tools).
Refactor:
- SandboxStore.asSink() centralizes the {put,has,evict} sink + uri<->id
mapping; the embedded-MCP and in-app agent-tools wiring sites share it.
Tests:
- security headers (inline vs attachment, nosniff, CSP), 304 Cache-Control,
putAndLink URL form, has()/remove(), asSink() round-trip, getSandboxPublicUrl
(trailing-slash trim + APP_URL fallback), and a stash test where the doc put
itself evicts a mirrored image.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Security:
- stash_page: reject path-traversal / percent-encoded srcs before the authed
loopback fetch (resolveInternalFilePath), closing an SSRF/exfiltration hole
where a crafted node.attrs.src could read an arbitrary internal GET endpoint
into the anonymous sandbox.
Stability:
- stash_page: revert + recount mirrors FIFO-evicted by a later put in the same
stash (no dangling sandbox refs, honest images.mirrored/failed); free image
blobs if the final document put throws.
- Reject/clamp non-positive SANDBOX_TTL_MS to the 1h default (warn once).
- Log mirror failures unconditionally (console.warn, no blob bodies).
Cleanup / architecture:
- Remove dead expiresAt from SandboxPutResult.
- Centralize the /api/sb route in SANDBOX_ROUTE_SEGMENT/SANDBOX_API_PATH and
move URL composition into SandboxStore.putAndLink; drop the duplicated sink
closures and the now-unused EnvironmentService injection from McpService and
AiChatToolsService.
- Un-export isInternalFileUrl; document the process-local (instance-bound)
sandbox limitation in the tool description and .env.example.
Docs/tests:
- README/README.ru: 38 -> 39 tools + stash_page entry.
- Add traversal/normalize/recursion unit tests, stash self-eviction +
doc-put-throw + empty/octet-stream mock tests, controller If-None-Match
(wildcard/weak/list) + Cache-Control tests, and SANDBOX_TTL_MS validation
tests. Regenerate packages/mcp/build.
Co-Authored-By: Claude Opus 4.8 <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>
agent_qa
agent_coder
claude-stand
claude_code
vvzvlad
