A runnable end-to-end suite that drives a LIVE git-sync stand over the real /git
remote — the integration counterpart to the unit tests. 10 checks across the full
feature:
- the auth/authz gate: no creds -> 401, wrong password -> 401, unknown space ->
404 (existence never revealed), valid creds on a sync space -> 200;
- fetch: git clone over HTTP returns the vault markdown;
- push: a git-side edit propagates into the Docmost page;
- Docmost -> git: a page created via the API materializes as a vault file;
- delete: `git rm` + push soft-deletes the Docmost page (Trash);
- 3-way merge: a new git edit is added without clobbering prior page content.
Parameterized via env (SERVER/SPACE_ID/EMAIL/PASSWORD/DB_CONTAINER) and isolates
its own test page. It boots nothing — see the header for the stand prerequisites
(GIT_SYNC_ENABLED + a per-space gitSync flag + a service user). This is the suite
that caught the smart-HTTP PATH_INFO 404 bug.
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 /git smart-HTTP host 404'd EVERY fetch and push: PATH_INFO was built as
`/<spaceId>.git/<subpath>`, so `git http-backend` resolved the repo at
`<GIT_PROJECT_ROOT>/<spaceId>.git` — which does not exist. The vault is a NON-bare
working repo (the engine needs a working tree) at `<dataDir>/<spaceId>`, so the
CGI repo path must be `<spaceId>` (git http-backend serves the `.git` inside).
The URL's conventional `.git` suffix is already stripped to `spaceId` by
parseGitPath; re-appending it for PATH_INFO was the bug.
Found by standing up a full e2e stand (real Postgres/Redis + server + a real git
clone/push over the /git remote): clone and push both 404'd until this fix, after
which a clone → edit → push round-trips the change all the way into the Docmost
page.
Also extracts the CGI-env construction into a pure, exported `buildGitBackendCgiEnv`
and adds unit tests (the env build was previously untested — the gap this bug hid
in): a regression guard pinning PATH_INFO to `/<spaceId>/<subpath>` (no `.git`),
plus method/query/content-type/remote-user forwarding and the conditional
GIT_PROTOCOL.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes the test-coverage warning that the smart-HTTP push ingest path was
unexercised. Adds 5 cases: receive-pack streams BEFORE the Docmost cycle; a
held lock throws GitSyncLockHeldError and runs neither the receive-pack nor the
cycle; a post-push cycle error is swallowed (the push is durable, poll retries)
while the lock is still released; a missing service user runs the receive-pack
but skips the immediate cycle; and a globally-disabled git-sync refuses without
touching the lock.
(The 503/Retry-After mapping in git-http.service is the sibling warning; its spec
is in the repo's pre-existing set of jest suites that can't load locally via the
react-dom/tiptap transform chain, so that case is left for CI.)
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>
The debounce map value carried `workspaceId`, but the scheduled cycle closes over
the `workspaceId` argument directly — the field was written and never read.
Replace the entry struct with `Map<string, NodeJS.Timeout>` (the timer handle is
all the map tracks). No behavior change. (page-change.listener.spec is in the
repo's pre-existing set of jest suites that can't load locally via the
react-dom/tiptap transform chain — unaffected by this change; tsc clean.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two-way block diff (yjs-body-merge.diffBlocks) and the three-way merge
planner (three-way-merge.lcsPairs) built the identical backward-filled LCS DP
table inline. Extract it to lcs.ts (buildLcsTable); each caller keeps its own
traceback. No behavior change — merge specs unchanged and green.
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>
Addresses the documentation/convention warnings from the #119 review:
- .env.example: add the GIT-SYNC block (9 GIT_SYNC_* vars with defaults), noting
GIT_SYNC_SERVICE_USER_ID is required when sync is enabled.
- yjs-body-merge.ts: translate the Russian review note in the docstring to
English (comments-only-in-English rule).
- persistence.extension.ts: correct the stale "git-sync writes are full-body
replaces" rationale — a git-sync write is now a block-level merge into the live
doc, which is why it is debounced like a human edit rather than snapshotted.
- history-item.tsx: the GitSyncBadge version is created on the PUSH path (writing
the git body back into the doc), not by the pull — fix the comment.
- edit-space-form.tsx: log the raw error in the git-sync toggle catch instead of
swallowing it (AGENTS.md).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Same hygiene fix as git-sync (review #2), applied to packages/mcp which had the
identical pre-existing problem: committed build/ (20 files) + node_modules (28,
pnpm symlinks with a baked /home/claude store path).
- git rm --cached packages/mcp/{build,node_modules}.
- .gitignore: add packages/mcp/build/ (packages/*/node_modules/ already covers it).
- Build where consumed: apps/server `pretest` and the CI Test workflow now build
@docmost/mcp too. The Dockerfile builder already runs `pnpm build` (nx builds
mcp) and already COPYs packages/mcp/build into the runtime image.
Verified: wiped build/, rebuilt via `pnpm --filter @docmost/mcp build`; the mcp
server suites (96 tests) pass against the freshly-built, non-committed output.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Upgrades the 2-way body merge to a real diff3 three-way merge (review #5), so a
block ONLY the human changed is KEPT when git changed a DIFFERENT block — the
2-way merge would revert it to git's stale version.
Engine: the push update loop reads the last-synced pre-image
(`git.showFileAtRef(refs/docmost/last-pushed, path)`) and passes it as the
optional `baseMarkdown` to `client.importPageMarkdown` (the common ancestor).
Server: gitmost-datasource converts base+incoming, and writeBody runs a block-
level diff3 (new three-way-merge.ts `diff3Plan`): live-only change -> keep live,
git-only change -> take git, both-changed -> git wins (conflict policy), inserts/
deletes from either side preserved. Without a base (createPage) it falls back to
the 2-way merge. Crash-safety unchanged (docs built before the connection opens).
Tests: three-way-merge.spec.ts (14 — every diff3 case incl. the cross-block
preservation and conflict policy), yjs-body-merge 3-way (real Y.Docs: human's
block instance preserved while git's block is applied), plus an engine test that
the base is forwarded from showFileAtRef. Existing push assertions updated for the
new base arg. git-sync 589 pass; server merge/datasource/gate 62 pass; typecheck
clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
Review finding #2: packages/git-sync/build/ (the COMPILED engine) and the
package's node_modules/ were committed. Prod executed the committed build/ while
CI/tests ran src/ and never rebuilt it — so a fix in src/ could pass tests while
stale compiled code shipped (a silent src/prod skew). The committed node_modules
were pnpm symlinks with a baked machine-local store path (/home/claude/...),
useless and misleading for everyone else.
- git rm --cached packages/git-sync/{build,node_modules} (42 + 31 files).
- .gitignore: ignore packages/*/node_modules/ and packages/git-sync/build/.
- Build the package where it is actually consumed: apps/server `pretest` now
builds @docmost/git-sync (its suite imports the built build/index.js), and the
CI Test workflow gains an explicit "Build git-sync" step. The Dockerfile builder
already runs `pnpm build` (nx builds the package) and now COPYs the fresh build/.
Verified: wiped build/, rebuilt via `pnpm --filter @docmost/git-sync build`, then
the server converter gate (26/26, imports the rebuilt package) and the git-sync
suite (588 passed) both pass against the freshly-built, non-committed output.
NOTE: packages/mcp/ has the same committed-build/node_modules pattern (pre-existing,
out of this PR's scope) and should get the same treatment in a follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 7d39c16b, now reaching the built package).
A true merge of git content into a live Yjs session is out of scope (it needs a
real 3-way text merge with no shared update lineage); deferring the write while a
page is being edited is the safe, owner-approved minimum.
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>
- page-history history-item: a lastUpdatedSource==='git-sync' version renders a
neutral gray 'Git sync' badge (git-merge icon), NOT the agent badge/deep-link
(it is not an agent edit). +2 i18n keys.
- Dockerfile: install git in the installer (runtime) stage — VaultGit shells out
to git, so assertGitAvailable() needs the binary at runtime.
Client tsc clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
UI opt-in for git-sync, mirroring the existing sharing/comments settings pattern
(no new endpoint, no new mechanism; orchestrator read query untouched):
- UpdateSpaceDto.gitSyncEnabled?: boolean.
- SpaceRepo.updateGitSyncSettings: jsonb-merge into settings.gitSync.<key>
(COALESCE || jsonb_build_object — never clobbers sibling sharing/comments);
stored as a real jsonb boolean so the orchestrator's
settings->'gitSync'->>'enabled' = 'true' matches.
- SpaceService.updateSpace handles the flag (audit diff) via the existing
CASL-guarded space update path (Manage/Settings).
- client: Switch in edit-space-form (optimistic mutate + revert-on-error,
readOnly-aware) + space types + 2 i18n keys.
- space.service.spec extended (calls updateGitSyncSettings; no-op when undefined).
tsc clean (server+client); jest src/core/space 4 pass.
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>
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>
Make @docmost/git-sync natively consumable by the CommonJS server (and jest):
build to CommonJS (tsconfig module CommonJS, drop type:module, strip .js from
relative imports), and lazy-load the only ESM-only dep (marked) via the dynamic
Function('import()') trick (mirrors docmost-client.loader.ts) with a require()
fallback so vitest's evaluator works too. git-sync tests stay green (314 pass,
3 expected fail).
Add the §13.1 idempotency gate (apps/server .../git-sync-converter-gate.spec.ts):
13 editor-ext docs (paragraphs/headings, marks, links, bullet/ordered/task lists,
blockquote, callouts, code block, hr, table, nested mix) round-trip
content(editor-ext) -> convertProseMirrorToMarkdown -> markdownToProseMirror ->
TiptapTransformer.toYdoc/fromYdoc(tiptapExtensions) -> canonicalize and assert
docsCanonicallyEqual. All green => the vendored converter's docmost-schema is
schema-compatible with editor-ext (no node/mark/attr loss), which the plan §13.1
requires before Phase B. The one intrinsic markdown-image lossiness (width/height
/align can't ride plain ) is isolated in a KNOWN DIVERGENCE block, not
hidden. Server tsc clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The standalone "Thinking…" indicator's agent-name label switched owners a couple seconds into a turn: at first TypingIndicator rendered name+dots (4px gap), then once useChat materialized the empty/reasoning-only assistant message the name moved to the empty MessageItem row (16px messageRow margin), pushing the dots ~20px away — a visible layout jump.
Introduce a shared pure helper assistantMessageHasVisibleContent() as the single source of truth mirroring MessageItem's render decisions. MessageItem now returns null for an assistant message with no visible content, and typingIndicatorShowsName keeps the name on the indicator until the assistant row has visible content. Exactly one element owns the name throughout the pre-content gap, so the layout no longer reflows.
- new utils/message-content.ts + unit tests (12 cases)
- message-list.tsx: typingIndicatorShowsName uses the helper
- message-item.tsx: early return null when no visible content
- typing-indicator-shows-name.test.ts: updated expectations
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the pre-merge review items for the #146 NodeView content-first fix:
- Export collectScrollAncestors/reflowAfterPaste and add editor-paste-handler
unit tests covering ancestor selection (overlay included, non-overflowing
auto excluded, X axis), the scrollHeight>clientHeight gate, scrollingElement
dedup, the docEl==null branch, and the double-rAF nudge.
- Extend the structural guard with CodeBlockView and merge the two it.each
blocks into one document-order assertion (handles the <pre> nesting where the
contentDOM is not the literal first child).
- Simplify the post-paste nudge to a single scrollTo(scrollLeft, scrollTop).
- Document that the post-paste reflow runs on every paste path intentionally,
and cross-reference the two #146 mitigations in both fixes.
- a11y: aria-hidden the decorative footnotes heading and number marker, and
label the footnotes list via role="group" + aria-label so the visual reorder
does not break screen-reader reading order (WCAG 1.3.2).
- CHANGELOG: add a Fixed entry noting the caret fix is macOS-verified manually.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolves the open items from the latest PR #143 code review:
- test(page): cover the four agentSourceFields stamp sites (create, update,
movePage, movePageToSpace) with agent + normal-user payload assertions;
add findById({ includeIsAgent: true }) wiring guards to the JWT and collab
auth-seam specs so a future drop of the option is caught.
- fix(privacy): drop `isAgent` from UserRepo.baseFields and gate it behind a
new opt-in `findById({ includeIsAgent })`, requested only by the two auth
seams that derive provenance — stops the flag leaking via the workspace
member list and generic user payloads.
- docs: correct the agentSourceFields JSDoc and the two UPDATE-site comments
to distinguish INSERT (omitted column → DB default 'user') from UPDATE
(omitted column → existing value kept, Kysely writes only present keys).
- style(page): collapse three stray double blank lines left by an earlier edit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses the #147 review (Approve with comments):
- Add footnote-views.structure.test.tsx: a structural regression guard asserting
the editable NodeViewContent is the FIRST child of FootnotesListView and
FootnoteDefinitionView, with no contenteditable=false chrome before it. The
whole #146 fix rests on this DOM-order invariant; the macOS caret symptom needs
a real browser, but the order proxy is testable in jsdom. Stubs @tiptap/react
so the views render as plain DOM — the test passes on the fixed order and fails
on the pre-fix chrome-first order.
- Reword the code-block-view comment: it claimed a "top-right overlay (the
transclusion pattern)", but the menu stays fully in flow as a full-width row
lifted via flex `order: -1` (the .codeBlock wrapper is a flex column). No
overlay/absolute positioning.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pasting markdown/code inserts React NodeViews that mount asynchronously; until
the next reflow the browser's hit-test geometry is stale, so ProseMirror's
posAtCoords/caretRangeFromPoint maps a click to the wrong (offset) line — which
users reported clears itself on any scroll. Reproduce that scroll's side effect
with a ZERO-delta nudge (re-assign scrollTop/scrollLeft to their current value)
on every scrollable ancestor + the document scrolling element, run across two
animation frames so it lands after the pasted content + NodeViews commit. The
nudge does not move the viewport.
Wired into editor-paste-handler's handlePaste, which ProseMirror's someProp runs
(as an editorProps handler) before the MarkdownClipboard plugin that performs the
markdown/code insert — so the nudge is scheduled on exactly the paste path that
triggers the bug. Complements the structural NodeViewContent-order fix in this
branch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Architecture & design:
- Arch A: introduce resolveProvenance() as the single source of truth for
deriving a write's actor/aiChatId from the SIGNED identity, and wire it into
BOTH transport seams — the REST jwt.strategy and the collab
authentication.extension. Previously the collab seam derived actor from the
token claim alone and ignored user.isAgent, so a flagged service account's
page-content edits over the websocket persisted as lastUpdatedSource='user',
drifting from REST. The seams now share one resolver and can't diverge.
- Arch B: drop AiAgentBadge's page-history coupling. The generic ui/ badge no
longer imports historyAtoms; it exposes an onActivate callback fired after the
deep-link, and the history row passes onActivate to close its own modal.
Suggestions/warnings:
- S1: soften the jwt.strategy provenance comment (applies to every REST write).
- S2/suggestion-3: drop the redundant comment-list-item null-aiChatId test
(covered by ai-agent-badge.test.tsx).
- S3: de-duplicate jwt.strategy.spec test #3 (the no-claim→'user' half
duplicated test #2); keep only the signed actor='agent' claim assertion.
- W2: add keyboard-activation tests for the badge (Enter/Space, unrelated key).
- W3: flip the design doc status to "реализовано (#143)".
Tests:
- new auth-provenance.decorator.spec.ts unit-tests resolveProvenance +
agentSourceFields.
- new collab-seam test: is_agent user with no claim → actor='agent'
(Arch A regression guard).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The agent write-stamp idiom — `...(isAgent ? { <source>: 'agent', <chat>: aiChatId } : {})`
— was hand-reimplemented at every REST write site, so each new path risked a
wrong literal or a forgotten aiChatId. Extract a single
`agentSourceFields(provenance, sourceKey, chatKey)` next to AuthProvenanceData and
call it at the 5 uniform spread sites:
- comment.service create -> createdSource / aiChatId
- page.service create/update/orphan-move/move -> lastUpdatedSource / lastUpdatedAiChatId
Sites that must CLEAR the source on a non-agent action keep their own conditional
(comment un-resolve writes an explicit null), and the collab persistence path keeps
its sticky-window logic — both noted in the helper's doc.
Behavior-preserving (the helper returns the identical object/`{}`). Typecheck
clean; server comment/page/auth/collab suites 246 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- [warn 1] Document the is_agent operator setup so it survives plan deletion:
added an AI-agent block to .env.example (use a DEDICATED account, set is_agent
via SQL, never flag a human/shared account) + a CHANGELOG "Added" entry.
- [warn 2] Test the badge deep-link side effects: ai-agent-badge.test.tsx now
renders inside an explicit jotai store, clicks the badge, and asserts the
active chat id, window-open, cleared draft, closed history modal, AND that
stopPropagation keeps a parent onClick from firing.
- [suggestion 3] Hoist the window.matchMedia stub into vitest.setup.ts and drop
the duplicated beforeAll block from the three test files (ai-agent-badge,
comment-list-item, role-cards).
- [suggestion 4] Merge the two near-duplicate "non-clickable" cases via it.each.
- [follow-up 6] Introduce a single ProvenanceSource = 'user' | 'agent' type in
jwt-payload.ts and reference it from AuthProvenanceData, JwtPayload/
JwtCollabPayload, and resolveSource() — so a typo can't slip through as a bare
string. (Server auth chain; client IComment mirroring left as a follow-up.)
Follow-up 5 (shared agentSourceFields write-stamp helper) is deferred as the
review marked it — the 6 REST sites use varied shapes (create-spread vs
resolve-conditional-null vs page move), so it's a separate focused refactor.
Tests: client badge/comment/role-cards suites 11/11 pass; server auth+comment
suites 62 pass; typecheck clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three editable NodeViews rendered a contentEditable=false "chrome" element IN
FLOW BEFORE NodeViewContent. On macOS the browser's click hit-testing
(posAtCoords → caretRangeFromPoint) then misses the contentDOM and snaps the
caret to the previous node — the caret/selection lands a line (code block) or
several lines (footnotes, into the body) above where the user clicked.
Fix (the transclusion pattern / issue #146 plan): make the editable
NodeViewContent the FIRST child in the DOM and move the non-editable chrome
AFTER it, restoring its visual position with CSS:
- code-block-view: <pre><NodeViewContent/></pre> first; the language/copy menu
follows and is lifted above via flex `order` (.codeBlock is now a flex column).
- footnotes-list-view: NodeViewContent first; the "Footnotes" heading follows and
is lifted above via flex `order` (.list is a flex column; the separator border
stays on the container).
- footnote-definition-view: NodeViewContent first; the "N." marker follows with
`order:-1` to stay on the left; the ↩ back-link stays on the right.
Layout is visually unchanged. Verified in a real browser (Chromium): the
contentDOM is now the first child of every editable NodeView wrapper (no
contentEditable=false element precedes it), and the menu/heading/marker still
render in their original positions.
NOTE: the caret-offset itself is macOS-specific text hit-testing and does not
reproduce in headless Chromium/WebKit on Linux (verified extensively), so the
visible fix is best confirmed on macOS.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
While a multi-step agent turn is "thinking between tool steps", the
assistant identity name (e.g. the role name) was rendered twice, stacked:
once by the assistant message row (MessageItem) and once by the standalone
TypingIndicator below it. The indicator's name label only makes sense when
it stands in for a not-yet-started assistant row; between steps the row
above already shows the same name.
Render the indicator's dimmed name label only when it is standalone (no
assistant row at the tail yet); otherwise show just the "Thinking…" dots.
- typing-indicator.tsx: optional showName prop (default true); the name
label renders only when showName !== false
- message-list.tsx: exported typingIndicatorShowsName(messages) helper;
pass showName to the indicator at the render site
- typing-indicator-shows-name.test.ts: unit-cover the four cases
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The custom undici RetryAgent + aiFetch transport added for issue #140
did not actually heal mid-stream provider drops: undici's retry path is
a Range-based download-resume that SSE/chat-completions endpoints cannot
satisfy, so a reset after the first byte only swapped ECONNRESET for a
"server does not support the range header" error. Its only real effect
was reconnecting a poisoned keep-alive socket before the first byte, and
PR #141 on top of it turned the 60s headers timeout into deterministic
~61s failures (plus CONTENT_LENGTH_MISMATCH from retrying a POST body
after a timeout abort). The root cause is the z.ai coding endpoint, not
our transport.
Remove the whole layer and return all AI provider calls to Node's
default global fetch.
- delete integrations/ai/ai-http.ts and its spec
- ai.service.ts: drop the aiFetch import, the AI_BYPASS_RESILIENT_FETCH
diagnostic toggle, and fetch:aiFetch from every chat/embedding/STT
factory; raw STT call back to global fetch
- ai-chat.controller.ts: drop the stream-timing START log + startedAt
- ai-chat.service.ts: drop the first-chunk/FINISHED/ERROR timing logs
- .env.example: drop AI_BYPASS_RESILIENT_FETCH
Reverts: 1af5d34a, 7c308728, b7abb7ea, 35fc58ea, d6cd2754, 6efb8656.
Preserved (not part of the rollback): client-disconnect abort, title
generation in onFinish, partial-answer persistence, Safari SSE heartbeat.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mark comments (and, via existing page provenance, pages) created under an
is_agent service account as authored by AI, derived from the SIGNED server
identity rather than any client field, and render the existing AI badge in
the comments sidebar.
Backend (B1):
- Add additive users.is_agent boolean (default false) migration; reflect in
the Users Kysely type, the user repo baseFields, and (via Selectable) the
User entity.
- jwt.strategy: derive req.raw.actor from user.isAgent (an is_agent account
stamps every write 'agent'); external MCP has no internal ai_chats row so
aiChatId stays null. Non-spoofable: a plain user cannot obtain
created_source='agent'.
- Loosen the provenance aiChatId type to string|null across token.service and
the JwtPayload/JwtCollabPayload claims (type-level only; the internal AI-chat
path still passes a real aiChatId).
Frontend (B2):
- Extend IComment with createdSource/aiChatId/resolvedSource (backend already
returns them via selectAll).
- Extract the local AiAgentBadge from history-item into a shared
components/ui/ai-agent-badge.tsx (clickable deep-link when aiChatId present,
plain label when null/absent); reuse it in history-item and render it in
comment-list-item next to the author name when createdSource==='agent'.
Tests: comment.service agent/null-aiChatId provenance, jwt.strategy provenance
derivation + anti-spoof, AiAgentBadge clickable/non-clickable branches, and
comment-list-item badge render/no-render.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds co-located unit tests for ten targets (client → vitest *.test.ts(x),
server → jest *.spec.ts), plus minimal behavior-preserving extractions/exports
where the issue required a pure function to test:
- encode-wav: WAV header + PCM16 clamping
- editor-ext embed-provider / utils (sanitizeUrl, isInternalFileUrl) / indent
(export clampIndent)
- label.dto @Matches regex
- move-page.dto vs generateJitteredKeyBetween parity (bug locked via test.failing)
- new-note-button canCreatePage (extracted to can-create-page.ts)
- history-editor diff (extracted pure computeHistoryDiff into history-diff.ts)
- notification getTypesForTab + repo contract (direct-tab divergence locked via
test.failing)
- search buildTsQuery (extracted + sanitizes operator inputs so adversarial
queries no longer risk a to_tsquery 500)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extend ai-http.spec with two loopback-server tests: a provider that stalls
without sending headers triggers the (lowered) headersTimeout and is retried on a
fresh connection, recovering; a healthy fast response passes through in one
attempt. No external network calls.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The z.ai GLM coding endpoint intermittently accepts the chat request but never
sends response headers; undici's default 300s headersTimeout then hung the user
for five minutes before failing, and UND_ERR_HEADERS_TIMEOUT was not in the
RetryAgent's retried error set, so there was no recovery.
headersTimeout only bounds time-to-FIRST-headers (before any body) — it is NOT
the streaming budget, so lowering it does not truncate live SSE streams. Cap it
(env AI_HTTP_HEADERS_TIMEOUT_MS, default 60s) so a header stall fails fast, and
add UND_ERR_HEADERS_TIMEOUT to the retried error codes so the stalled request is
retried on a fresh connection (which usually responds in seconds). bodyTimeout
kept generous (env AI_HTTP_BODY_TIMEOUT_MS, default 300s) so slow streams with
sparse chunks survive. UND_ERR_BODY_TIMEOUT is deliberately NOT retried (mid-body,
partial SSE already delivered).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extend the window.gitmost native-host bridge with three methods that work
when no page is open, registered globally at the app-shell level (not in
page-editor.tsx) so the react-router navigate fn and the api-client are
available:
- listSpaces(): reuse getSpaces() -> [{id, name}], flags truncation.
- listPages({spaceId, parentPageId?}): reuse getSidebarPages()
-> [{id, title, hasChildren}], first page only (truncated flag).
- createPageWithRecording({spaceId, parentPageId?, title?, base64,
filename, mimeType}): validate/decode the audio first (so a bad payload
leaves no junk page), resolve the space slug via getSpaceById (no-space
probe), createPage(), navigate via the router (no reload), wait for the
new page's editor to be mounted+editable+Yjs-connected, then run the same
uploadAudioAction path as insertRecording. Resolve-only error contract:
no-space | create-failed | editor-timeout | insert-failed.
DRY: extract the base64 decode/validate + audio-insert pipeline from
page-editor.tsx into features/editor/gitmost/gitmost-recording.ts; the
existing insertRecording now delegates to it (behavior unchanged).
Mount GitmostGlobalBridge once in GlobalAppShell. Before navigating, reset
the shared yjsConnectionStatusAtom so the readiness gate waits for the NEW
page's provider to connect instead of a stale "connected" from a previously
open page.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes the 4th PR #138 review (3 suggestions, no blockers).
- Double-call safety: a failed turn fires both useChat onFinish AND onError, so
onTurnFinished can run twice in one turn (streamed id, then no id) before a
re-render. onTurnFinished now reads the live id from a ref (set imperatively on
primary adoption) instead of the stale closure, so the 2nd no-id call cannot
re-arm the error-path fallback at the source; the render-phase reconciler is the
second layer. Added a hook test for the sequence — verified it fails only if
BOTH layers are removed (non-tautological).
- Conventions: extracted named UseChatSessionOptions / UseChatSessionResult
interfaces (was an inline param literal + ChatSession); the test derives its
driver props from them.
- Simplification: extracted the chatIdSnapshot(chats) projection used at both the
fallback arm site and the resolver effect.
Architecture notes from the review (caller-driven disarm contract; cross-process
{chatId} type) intentionally left as Variant A per the reviewer's recommendation.
tsc clean; 128 ai-chat tests green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes the 3rd PR #138 review.
Warning fix: the render-phase reconciler only disarms the error-path adoption
fallback when activeChatId actually changes. Pressing 'New chat' while ALREADY
in a new chat keeps activeChatId === null (a no-op atom write), so the reconciler
never fired and a stale armed fallback could adopt the just-failed chat from a
late refetch, yanking the user out of their fresh chat. useChatSession now
returns cancelPendingAdoption(); the window calls it from startNewChat AND
selectChat. (The hook call moved above those callbacks so they can reference it.)
Added a hook test that fails without the explicit disarm, plus a test for the
existing-chat onTurnFinished branch (no adoption + per-chat invalidation).
Cleanups: removed the dead pickNewlyCreatedChatId (the fallback effect uses
newlyAddedChatIds directly with the 0/1/>1 decision inline) and its tests/doc
mention; inlined the two invalidation closures (onTurnFinished is read live by
useChat's onFinish, never in an effect dep array, so memoizing them was needless
ceremony).
Verified: tsc clean, 127 ai-chat tests green; live (z.ai glm-5.2) new chat + 2nd
turn recalled the number in the SAME row (1 chat / 4 messages), no page errors.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Node's fetch returns a generic "fetch failed" error, hiding the actual
reason (e.g., ECONNRESET, timeout) in the error's cause chain. This
change extracts up to three levels of the cause, formats each with its
code and message, and includes the chain in the warning log, making
failures more actionable.
A new-chat turn fired the chat stream (streamText) and title generation
(generateText) concurrently to the same z.ai coding endpoint. That plan
stalls one of two concurrent requests, so the chat stream black-holed for
~300s (undici headers timeout) and the turn hung forever in every browser;
the AI SDK then retried 3x. Server logs showed two concurrent POSTs to
/chat/completions per turn — one 200 in ~8s, the other "fetch failed after
301209ms". Bypassing the custom undici transport did not help, confirming
the cause is the concurrency, not the transport.
Move generateTitle from before the response pipe into onFinish, so it runs
solo AFTER the stream's provider call completes. A first turn that errors or
aborts no longer auto-titles (fallback "Untitled chat" already handles a
null title) — acceptable, and it removes the request that was stalling.
