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>
The server requires @docmost/git-sync (main: ./build/index.js) at runtime, but
the installer stage copied only editor-ext and mcp — so the image built fine and
then crashed on startup with `Cannot find module '@docmost/git-sync'`. Copy the
package's freshly-built build/ + package.json, mirroring the mcp/editor-ext COPY
lines. (Addresses review finding #1 on PR #119.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Coder↔reviewer design loop (9 rounds, reviewer verdict: exhaustive) produced
92 specs; implemented +123 tests (465 -> 588 passing). The new round-trip
coverage exposed three genuine data-loss bugs in the Markdown<->ProseMirror
converter, all now FIXED (round-trip is lossless for these):
1. pageBreak was lost on export (no converter case -> rendered to "" and the
node vanished). Now emits <div data-type="pageBreak"></div>, which the schema
parses back -> round-trips.
2. A block image between blocks left an empty <p> artifact after import-hoisting,
producing a phantom blank-gap diff on every sync. markdownToProseMirror now
strips content-less paragraphs after generateJSON — with a schema-validity
guard that keeps the obligatory single empty paragraph in `content: "block+"`
containers (tableCell/tableHeader/blockquote/column/callout/doc), so empty
cells/quotes never become an invalid `content: []`.
3. The `code` mark combined with another mark was not byte-stable (emitted nested
HTML that the schema's `code` `excludes:"_"` collapsed on import). The
converter now emits code-only when `code` co-occurs, matching the editor.
New coverage spans media/diagram/details/columns/math/mention attribute
round-trips, converter emission branches, git error paths, and engine decision
branches. A dedicated test pins the empty-container schema validity (the review
catch on the bug-2 fix).
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>
First step of docs/git-sync-plan.md. New workspace package @docmost/git-sync
vendoring the PURE parts from docmost-sync (HEAD b03eb35):
- lib: markdown-converter, markdown-document, canonicalize, docmost-schema,
node-ops, diff, and an extracted markdown-to-prosemirror (only the pure
marked->HTML->generateJSON path from upstream collaboration.ts; no websocket).
- engine (pure, no IO): reconcile, layout, sanitize, stabilize, loop-guard.
Ported the upstream pure-module + round-trip corpus tests (vitest): 314 pass,
3 expected upstream known-limitation fails. tsc clean. No server wiring yet.
docmost-schema inlines getStyleProperty (as packages/mcp does — @tiptap/core
3.20.4 doesn't export it). IO engine (pull/push/git/settings) deferred to later
Phase A/B steps; the editor-ext idempotency gate (plan §13.1) is the next step.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The proofreader role content was changed (STYLE SHEET block removed) without
bumping its catalog version, so clients never saw an update. Bump proofreader
2 -> 3, and add a content-hash guard so this can't happen silently again.
- index.json: proofreader version 2 -> 3
- scripts/check.mjs: new content-hash guard. A scripts/content-hashes.json lock
maps slug -> { version, hash } (sha256 over emoji/autoStart/name/description/
instructions/launchMessage across all languages). check.mjs now fails when a
role's content changed without bumping its version; the new --update-hashes
(alias --fix) refreshes the lock but refuses to write when a bump is missing.
- check.mjs: also require every index.json role to carry a finite numeric
version (matches the server's catalog validation), with defense-in-depth so a
missing version can't bypass the bump guard.
- scripts/content-hashes.json: new lock artifact (not part of the served catalog).
- README.md: document the guard, the lockfile, --update-hashes, and the
prune-then-readd limitation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The agent-roles catalog source is no longer hardcoded in app code and no longer
supports a local filesystem directory. The provider fetches only from an
http(s):// base URL read at runtime from AI_AGENT_ROLES_CATALOG_URL; an empty or
non-http value yields a 502 (catalog unavailable). The image ships a per-branch
default for that URL (set in CI), still overridable at runtime via the env var.
- provider: drop readLocal + node:fs/node:path; readRelative requires http(s)
and 502s otherwise; remote fetch/streaming-cap/SSRF guards unchanged.
- environment.service: keep AI_AGENT_ROLES_CATALOG_URL (default ''); comment
reflects the per-branch build-time default that is runtime-overridable.
- Dockerfile: add ARG+ENV AI_AGENT_ROLES_CATALOG_URL in the installer stage as
the image default.
- CI: develop.yml builds with the develop raw URL; release.yml defines the main
raw URL once in workflow env and references it from both build steps.
- tests: replace local-fixture tests with remote-mock happy/malformed bundle
tests and a non-http => 502 case; path-traversal block uses an https source.
- docs: update .env.example, CHANGELOG (#222), agent-roles-catalog/README.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the STYLE SHEET / СТАЙЛ-ШИТ section from the copyeditor
(proofreader) role and clean up all dangling references to it in both
the ru and en editorial bundles:
- description: drop "maintains a style sheet" / "ведёт стайл-шит"
- instructions: remove the STYLE SHEET block
- instructions: drop "record it in the style sheet" mentions in the
WHAT YOU DO and WHEN UNSURE sections
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR #227 re-review (comment 2193).
- Stability: `updatePageId`/`updateAlias` now `executeTakeFirstOrThrow`, so a row
reaped by a concurrent `removeAlias` between the read and the UPDATE (READ
COMMITTED) raises `NoResultError` instead of returning `undefined`. The service
maps that to a retryable `ConflictException` (`ALIAS_PAGE_RACE`) rather than a
200-without-alias (swap) or a generic 400 from `undefined.id` (rename). Tests
cover both branches.
- Simplification: drop the redundant secondary "unexpected unique index" warn and
the now-unused `UNIQUE_ALIAS_INDEX` const (the constraint name is already logged
unconditionally; both index branches still distinguish "Alias already taken" vs
ALIAS_PAGE_RACE).
- Architecture: extract `isUniqueViolation`/`violatedConstraint` into
database/utils.ts; adopt them in the share-alias service and favorite.repo
(the bare `23505` check). ai-agent-roles (#222) is on a separate unmerged branch
and should adopt them after #227 merges (noted at the helpers). Helper unit test
added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The agent-roles catalog source is no longer hardcoded in app code and no
longer supports a local filesystem directory. The provider now fetches only
from an http(s):// base URL read from AI_AGENT_ROLES_CATALOG_URL; an empty or
non-http value yields a 502 (catalog unavailable). The default URL is baked
into the Docker image at build time and set per branch in CI.
- provider: drop readLocal + node:fs/node:path; readRelative requires http(s)
and 502s otherwise; remote fetch/streaming-cap/SSRF guards unchanged.
- environment.service: keep AI_AGENT_ROLES_CATALOG_URL (default ''); comment
updated to reflect build-time injection, remote-only.
- Dockerfile: add ARG+ENV AI_AGENT_ROLES_CATALOG_URL in the installer stage.
- CI: develop.yml builds with the develop raw URL; release.yml (both build
steps) with the main raw URL.
- tests: replace local-fixture tests with remote-mock happy/malformed bundle
tests and a non-http => 502 case; path-traversal block uses an https source.
- docs: update .env.example, CHANGELOG (#222), agent-roles-catalog/README.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The share modal flagged a custom address already owned by another page with a
red "This address is already in use" error driven by the availability probe.
That reads as terminal even though Save actually triggers the server's
409 `ALIAS_REASSIGN_REQUIRED` and opens the "Move custom address?" confirm
modal that retargets the address to the current page — so the reassign path was
hidden behind what looked like a hard stop.
Replace the red error with an informational description hint ("This address is
in use. Saving will move it to this page.") and keep Save enabled, so the
existing confirm-reassign flow is discoverable. Renaming to a FREE name was
already correct (the probe returns available -> no error -> server renames the
single row in place); this only changes the taken-name presentation.
Verified end-to-end in a real browser against a live stand on this branch:
- A (free rename `test`->`test2`): 200, same alias row renamed in place, link
becomes `/l/test2`, no error, exactly one DB row for the page.
- B (`test2` owned by another page): hint shown (no dead-end error), Save ->
409 ALIAS_REASSIGN_REQUIRED -> "Move custom address?" modal -> confirm -> 200,
the single row retargets, one row each.
- C (same-name re-save): Save disabled (no-op); first-time set inserts.
Add a client component test covering both branches (taken name -> hint not
error + Save enabled; 409 -> reassign modal -> confirm sends confirmReassign).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merge the copy-editor (📐) and proofreader (🧹 "Корректор") editorial roles
into a single role. Keep slug `proofreader`, drop slug `copy-editor`, and set
the merged role's emoji to 📐.
- index.json: remove copy-editor; bump structural-editor, line-editor,
fact-checker, proofreader to version 2 (narrator unchanged); update editorial
bundle description (ru/en).
- bundles/editorial/{ru,en}.json: delete copy-editor; refresh emoji/name/
description/instructions of structural-editor, line-editor, fact-checker and
the merged proofreader verbatim from gitmost-agenty-ru.md / gitmost-agents-en.md;
preserve autoStart and launchMessage; leave narrator untouched.
- README.md: drop copy-editor from the editorial role list.
Validated with scripts/check.mjs (OK).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review on PR #227.
- setAlias confirmed-reassign branch: DELETE the target page's existing
alias row(s) BEFORE retargeting `byName` onto the page, instead of after.
The new partial unique index `(workspace_id, page_id)` is non-deferrable
and checked at each statement, so retargeting first momentarily left two
rows for the page -> immediate 23505 -> rolled-back tx surfaced as a
misleading "Alias already taken" (regressing a previously-working swap onto
a page that already had its own alias). The reordered branch needs no
trailing self-heal. JSDoc updated to describe the real ordering.
- catch block: the postgres@3.x driver exposes the violated index as
`err.constraint_name` (with `.constraint` as a fallback). Map
`share_aliases_workspace_id_alias_unique` -> "Alias already taken" and the
new `share_aliases_workspace_id_page_id_unique` -> a distinct ALIAS_PAGE_RACE
outcome (a concurrent same-page write, not a name clash). Always log the
constraint name on any 23505 so the race is diagnosable.
- migration 20260627T120000: document that the dedup DELETE is intended,
irreversible data loss (old duplicate `/l/<old>` links start 404ing after
upgrade; `down()` cannot restore the rows). Same note added to CHANGELOG
[Unreleased] Fixed.
Tests:
- integration: confirmed reassign onto a page that ALREADY has its own alias
(RED before the reorder); migration up() dedup scoping across pages and a
second workspace; mid-transaction error -> BadRequest with clean rollback.
- unit: constraint_name distinguishing (alias index, page_id index, fallback
`.constraint`, no-info default) and non-unique error -> BadRequest; retarget
test now asserts delete-before-update order.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Editing an existing share alias (e.g. slug `te` -> `ted`) failed to update
the displayed `/l/<alias>` link: `setAlias()` looked the requested slug up by
name and, if free, INSERTed a brand-new row, leaving the page with multiple
alias rows. The modal then read via `findByPageId().executeTakeFirst()` with no
`ORDER BY`, so Postgres returned an arbitrary (in practice the oldest, stale)
row. Every edit also spawned an orphan row that kept a live `/l/<old>` link
forever. Regression of #205.
Enforce the invariant "a page has EXACTLY ONE custom address":
- `setAlias()` now resolves the page's current alias row and RENAMES it in
place when the requested name is free (insert only when the page has none),
keeps the same-name no-op and the cross-page 409 `ALIAS_REASSIGN_REQUIRED`
+ confirmed-retarget flow, and after any successful write DELETEs all other
alias rows for the page (self-heal). Runs in one transaction so the page is
never transiently empty or duplicated.
- repo: add `updateAlias` (rename) and `deleteOthersForPage`; make
`findByPageId` deterministic with `ORDER BY created_at DESC, id DESC`.
- migration: dedup existing rows (keep newest per page) + a PARTIAL unique
index `(workspace_id, page_id) WHERE page_id IS NOT NULL` so dangling
aliases still coexist while live ones are one-per-page.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ITEM 1: cover useImportAiRolesFromCatalogMutation onSuccess notifications.
Add import-from-catalog-message.test.tsx (twin of update-from-catalog-message)
asserting the always-shown summary (errors:[]) and the additional red
"Failed to import N role(s)" notification when result.errors is non-empty.
ITEM 2: pass redirect:'error' to the remote catalog fetch in fetchRemote so a
compromised-but-trusted upstream cannot 3xx the fetch into the internal network
(redirect-SSRF). Add provider specs asserting the option is passed and that a
redirect rejection maps to BadGatewayException.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MUST-FIX
- isSourceUniqueViolation read the wrong error field: kysely-postgres-js
(postgres@3.4.8) puts the violated constraint on `constraint_name`, not
node-postgres' `.constraint`, so a concurrent same-slug+language import's
23505 was never recognized as a source-collision and surfaced a false
"name already exists" error. Now read `constraint_name` (with `.constraint`
as a fallback for other drivers). Fix the faked test fixture (it built the
error with the same wrong `.constraint` field, masking the bug): it now
uses `constraint_name`, so the test genuinely exercises the skip path and
FAILS against the unfixed code.
- Extract the catalog modal's role-state computation into a pure
`catalogRoleInstallState(role, workspaceRoles, language)` helper (mirrors
role-launch.ts) and cover it with vitest: import / installed / update /
same-slug-different-language.
SUGGESTIONS
- Restore IAiRoleUpdateFromCatalogResult as a discriminated union mirroring
the server; narrow the consumer via `"reason" in result` (the boolean
discriminant does not narrow under strictNullChecks:false).
- README: add a "How it's served" section documenting AI_AGENT_ROLES_CATALOG_URL
(remote http(s) base / local path / empty => in-repo folder).
- check.mjs: drop the redundant `const key = slug` alias.
- Cover the reason->message mapping in useUpdateAiRoleFromCatalogMutation
(4 branches) via renderHook with a mocked service.
- Cover importFromCatalog "bundle not in index" => BadGateway.
- Cover updateFromCatalog "slug in index but missing in bundle file" =>
not-in-catalog.
ARCHITECTURE
- Extract the shared catalog read prefix: a private `loadBundleById`
(fetchIndex -> meta -> fetchBundle -> versionMap) reused by getCatalogBundle
and importFromCatalog, and a `catalogRoleContentFields` mapper shared by the
import insert and update patch. The three orchestrations and their distinct
write paths stay separate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review comment 2159 on the temporary-notes UI work.
Tests:
- tree-model: cover handleCreate's race-guard temporaryExpiresAt patch — (a)
server node inserted WITHOUT a deadline + create response carries one => node
gains the deadline; (b) node already has a deadline => not overwritten, prev
returned by reference.
- ws-tree.service.spec: broadcastPageCreated now asserts the deadline is carried
when present and pinned to null (`?? null`) when absent.
- page-embed-query (new spec): syncTemporaryExpiresInCache patches the in-tree
node's temporaryExpiresAt, and leaves the atom value at the same reference when
the id is absent from the loaded tree (no write).
Refactor (closes the drift bug-class at the root):
- Client: extract one canonical pageToTreeNode(page, overrides) mapper in
tree/utils and route buildTree, handleCreate's optimistic insert, the restore
mutation and the duplicate handler through it. Restore stays permanent (server
nulls temporaryExpiresAt) and duplicate stays permanent (server arms no timer)
— both now reflect the server without a reload, where before they dropped the
field entirely.
- Server: extract one toTreeNodeSnapshot(page) helper called by both the
PAGE_CREATED event enrichment (page.repo) and the addTreeNode broadcast
(ws-tree.service), so the optional temporaryExpiresAt can't drift between the
two literals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Issue 1 — the sidebar tree's temporary-note clock marker did not appear/
disappear until a page reload when a note's temporary state changed.
- Make/unmake permanent from the page header menu and the in-page banner went
through syncTemporaryExpiresInCache(), which patched the page query cache but
never touched treeDataAtom, so the sidebar node kept its stale
temporaryExpiresAt. Patch the tree node there too (via jotai's default store),
so the marker updates without a reload.
- Creating a note as temporary showed no marker until reload: the create flow's
cache write (invalidateOnCreatePage) omitted temporaryExpiresAt, so the tree
rebuild (buildTree -> mergeRootTrees) overwrote the optimistic/socket node's
marker with undefined. Carry temporaryExpiresAt in that cached entry.
- Thread temporaryExpiresAt through the server addTreeNode broadcast (PAGE_CREATED
snapshot -> TreeNodeSnapshot -> broadcastPageCreated) so OTHER clients watching
the space also render the marker immediately, and harden handleCreate's
idempotency guard to patch the deadline if the broadcast won the insert race.
Issue 2 — the home and space-overview "New note" / "New temporary note" buttons
sat side-by-side and the temporary label clipped on narrow mobile widths. Lay
them out full-width, stacked vertically, and tint the temporary button orange
(matching the clock marker + banner) while the regular one stays neutral gray.
Tests: extend tree-socket-reducers.test.ts (addTreeNode carries
temporaryExpiresAt). Verified live with Playwright: marker appears on create and
toggles both ways with no reload; mobile buttons are stacked, full-width,
unclipped, and differently colored.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Item 1 (concurrency-safe import): add a partial UNIQUE index on
(workspace_id, source->>'slug', source->>'language') WHERE source IS NOT NULL
AND deleted_at IS NULL, so two concurrent imports of the same bundle can no
longer create duplicate roles for one catalog slug+language. The in-memory
installedKeys snapshot cannot see a sibling request's writes; the index is the
backstop. importFromCatalog now catches the 23505 from THIS index (keyed off
the constraint name) and treats it as "already installed" -> skip, batch
continues. A 23505 from the name-uniqueness index keeps its existing friendly
per-role error behavior (distinguished by constraint name; an indeterminate
23505 falls back to that path, so no regression).
Item 2 (single source validator): strengthen parseSource into THE single form
validator for the source jsonb column -> returns a fully-valid RoleSource | null
(slug/language non-empty strings, version a number). The service's weaker
roleSource is removed and both layers share the RoleSource type (defined in the
db entity.types module both already import AiAgentRole from, so no import
cycle). normalizeRow / the read path now only ever yield a valid RoleSource or
null; a malformed stored source normalizes to null (tolerated by the service).
Tests: parseSource null for {} / {slug:123} / {slug:'a'} / empty-string keys /
string version, typed value for a full valid shape; service test that a
source-uniqueness 23505 is skipped (not errored) and the batch continues.
Verified the partial index rejects a duplicate source-not-null row but allows
two source-NULL rows, and the migration up/down run cleanly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CHANGELOG: document the importable multilingual agent-roles catalog under
[Unreleased] (browse/import/update, 4 new endpoints, source column, the new
AI_AGENT_ROLES_CATALOG_URL env var) (#222).
- Fix importFromCatalog docstring: a role is skipped only on source.slug AND
source.language; another language of the same slug still imports.
- Provider: map a timeout/abort (or any failure) during the response-BODY read
to a logged BadGatewayException, so a slow/dripping source yields a 502, not a
generic 500. Existing too-large BadGateway cases are rethrown as-is.
- Service: inject a Nest Logger and log the root cause (with workspaceId/
bundleId/slug) on a non-23505 insert error during import.
- Modal: hoist the duplicated i18n base-subtag into a single baseLang const.
- Tests: AbortError body-read -> BadGateway; null-body text() fallback (under
and over cap); invalid-JSON and malformed-index BadGateway; non-23505 import
error -> generic message + logged root cause.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename catalog-source migration 20260626T120000 -> T150000 so it sorts
after develop's latest migration (T140000-page-temporary-notes); the old
timestamp predated ai-chat-message-status/share-aliases and tripped
Kysely's #ensureMigrationsInOrder, aborting server boot.
- Provider: inject a Nest Logger and log the real cause (incl. response
status) in the parseJson / readLocal / fetchRemote catch blocks, and
propagate a useful cause into the BadGatewayException message; add a
shortError helper (robust to jest's realm-shifted Error-likes).
- Provider: replace the manual Uint8Array assembly with
Buffer.concat(chunks).toString('utf8'); keep the streaming size cap.
- Controller spec: add admin-gate coverage for the 4 catalog routes
(catalog/catalogBundle/import/updateFromCatalog) - non-admin Forbidden +
service untouched, admin delegates with the right args.
- Service spec: add getCatalog/getCatalogBundle tests covering the
localized() three-tier fallback, the sorted language union, the
missing-bundle BadGateway, and the role-version default.
- Provider spec: add remote fetch-rejects and non-ok (503) error branches.
- Service: drop the dead Date.now() tail in freeName (now an explicit
unreachable throw) and extract a shared isUniqueViolation() predicate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Admins can browse a curated catalog of agent roles, import roles/bundles
into a workspace, and update an imported role when the catalog ships a
newer version.
Catalog: a set of JSON files (index.json manifest + bundles/<id>/<lang>.json)
served from a local folder (dev) or a remote http(s) base URL via
AI_AGENT_ROLES_CATALOG_URL. Seeded with the existing 7 RU roles (editorial +
research bundles) plus EN translations.
Server:
- migration: nullable jsonb `source` column on ai_agent_roles
({ slug, language, version }; null => manually created)
- catalog provider: remote fetch with timeout + streaming size cap, or local
read; ^[a-z0-9-]+$ segment guard against path-traversal/SSRF
- admin endpoints: catalog, catalog/bundle, import, update-from-catalog
- import/update match by slug+language; update preserves `enabled`
Client:
- catalog modal with language selector and Import/Installed/Update states
- "Import from catalog" button + empty-state CTA in the roles settings panel
- en-US/ru-RU strings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The right border on the address prefix read as a stray vertical line
between the domain and the slug. Remove it and rely on the subtle
prefix background alone to separate the two parts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR #224 fixed an AI-chat streaming-render regression by moving the React.memo
content signature into the parent: MessageList now snapshots
messageSignature(message) per render and passes it to MessageItem as the
immutable `signature` prop. The existing memo tests only SIMULATED that
parent half by hardcoding `signature={messageSignature(message)}` in their
harness; the real MessageList was never exercised (chat-thread.test.tsx mocks
it out, and there was no message-list.test).
Add message-list.test.tsx that mounts the REAL MessageList (without mocking
MessageItem or messageSignature) and asserts that an in-place mutation of a
reused message object surfaces on re-render. This guards the parent-side
contract: re-caching the signature on message identity (stable across deltas
while parts mutate) would refreeze the row, and this test would fail.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
