Resolve the code-review findings from comment #1571 on PR #119.
Engine (packages/git-sync):
- Idempotent CREATE on retry: before createPage, look the page up in the
live Docmost tree by (parentPageId, title) and ADOPT it instead of
duplicating when a prior cycle created it but failed to persist the
pageId back to disk. Only trust a COMPLETE tree for the lookup; fall
back to createPage otherwise. Covered by new tests incl. a complete=false
regression-lock.
- Route applyPullActions diagnostics through an injected logger instead of
bare console (thread log from the cycle).
- Add a timeout to the git execFile chokepoint (runRaw) so a hung git
subprocess cannot wedge a sync cycle.
- Translate remaining Russian code comments to English.
- Remove dead standalone-CLI code (parseArgs/PushParsedArgs,
parseSettings/envSchema, loadSettingsOrExit + config-errors.ts) and the
matching index exports/specs; keep the Settings type.
- Fix the dangling docs link in package.json.
- Add a schema-surface snapshot guard so any drift in the vendored
document schema is a loud, must-review CI failure (+ provenance header).
Server (apps/server):
- Add a configurable watchdog timeout to the spawned git http-backend so a
stalled push cannot hold the per-space lock forever
(GIT_SYNC_BACKEND_TIMEOUT_MS).
- Close the in-process TOCTOU window in SpaceLockService.withSpaceLock by
reserving the slot synchronously before acquire.
- Add tests: removePage git-sync provenance (both branches), ensureServable
force-push-protection git configs, and the phase-B+ datasource methods.
Docs / build:
- AGENTS.md: list git-sync as the fifth workspace package and note the
three schema mirrors; fix the dangling git-sync-plan.md backlog link.
- pnpm-lock.yaml: add the missing @docmost/git-sync workspace link so
pnpm install --frozen-lockfile (CI default) succeeds.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes the architecture item from the #119 review: drop the "vendored from
docmost-sync" framing and the CJS↔ESM `Function('import()')` bridge so the engine
is a normal first-class gitmost package.
Part 1 — vendoring markers removed (prose only, zero behavior change): reworded
"VENDORED into gitmost" / "vendored from docmost-sync" / "Engine LOGIC is
byte-identical" / "it's a port" comments across the engine. Behavior-bearing
strings are untouched: BOT_AUTHOR_NAME/EMAIL and the `Docmost-Sync-Source:`
provenance trailers (changing them would break git authorship + the loop-guard).
Part 2 — the package is now ESM (matching the sibling @docmost/mcp): `type: module`,
tsconfig Node16, `.js` extensions on relative imports, and a static
`import { marked }` replacing the `new Function('return import(...)')` /
`loadMarked` hack — the bridge is GONE from the package. The CommonJS NestJS
server loads the now-ESM engine via a new `git-sync.loader.ts` that mirrors the
existing `docmost-client.loader.ts` mcp loader exactly (Function-indirected
dynamic import + cached promise + retry-on-reject). The 4 server consumers
(orchestrator/datasource/vault-registry/git-http-backend) call `await loadGitSync()`
for value exports; types stay `import type` (erased). The converter-gate spec —
which needs the real converter — loads the package's TS source via a jest
moduleNameMapper + isolatedModules (documented in that spec); the other git-sync
specs mock the loader.
Verified: engine builds pure ESM (no Function/require leftover), vitest 614,
editor-ext build, server + client tsc, full server jest 1397/0. Live stand
smoke-test: server starts clean on the ESM engine (no ERR_REQUIRE_ESM), a real
sync cycle runs through the loader, and the basic e2e suite is 12/12 (clone via
git-http-backend, push, pull, delete, 3-way merge — all through the new loader).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rebase folded develop's agent-provenance PageService spec and the git-sync
provenance spec into one file; the appended git-sync block needs CreatePageDto /
UpdatePageDto / User imports that develop's spec (which used inline `as any`) did
not have. Server tsc + the suite (158 tests, both provenance blocks) green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses the stability + test-coverage warnings from the #119 review:
- git-http-backend.service.ts: add `'error'` handlers to child.stdout/stderr. An
EventEmitter 'error' with no listener (e.g. EPIPE when the client aborts
mid-response) is rethrown by Node as an uncaught exception and crashes the
process; now swallowed + logged (never echoed to the client).
- TEST INFRA: a jest setupFile shims `navigator`/`MessageChannel` for the `node`
testEnvironment. react-dom@18 reads `navigator` at module-init (pulled in via
@docmost/editor-ext -> @tiptap/react), so every spec transitively importing the
conversion engine — including git-http.service.spec.ts — previously FAILED TO
LOAD ("navigator is not defined") and ran ZERO tests. With the shim those specs
now run (git-sync integration: 11 suites / 133 tests green).
- git-http.service.spec.ts: cover the 503 lock-held push path — `ingestExternalPush`
rejecting `GitSyncLockHeldError` -> 503 + Retry-After + "git-sync busy, retry",
no double header write (+ the already-headers-sent no-rewrite path).
- git-http-backend.service.spec.ts: unit-test run() — child 'error'/'close' before
headers -> 500; normal CGI parse+stream; stdout/stderr 'error' (EPIPE) swallowed;
synchronous spawn throw -> 500.
- page-change.listener.ts: implement OnModuleDestroy to clearTimeout all pending
debounce timers on shutdown (+ test).
- .env.example: vaults are non-bare working repos, not "bare repos".
(Docs deleted by the stray commit were restored in 9cdbce54.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The shell e2e suites defaulted to the General space and created/edited pages
there, polluting real content (and, when several enabled spaces raised poll
contention, flaking on 503s). Now each suite creates its OWN throwaway,
git-sync-enabled space at setup, runs everything against it, and deletes the
space (+ its vault) on exit. Set SPACE_ID explicitly to opt into an existing
space. Also gives the basic suite the 503-retry push helper the advanced one
already had. Verified isolated: basic 12/12, advanced 23/23, no spaces/users/
pages left behind, the real space untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to 4376c5a6, found by a real BROWSER e2e (the flow the in-diff fix
missed). When the layout reshuffle's two halves land in SEPARATE sync cycles, the
later cycle's diff has only the DELETE of the old path — the matching add was
already pushed — so in-diff D+A coalescing can't see it, and the live page was
still trashed.
Robust fix on the identity invariant the reviewer (and the user) called out: a
page EXISTS iff its pageId is in the vault, regardless of filename. runPush now
collects the pageIds present at ANY path in the current `main` tree and passes
them to computePushActions; a deleted file whose pageId is still tracked
elsewhere is a MOVE, never a deletion. (Built only when the diff has deletes.)
Adds apps/server/test/git-sync-browser-e2e.cjs — a Playwright test that drives the
REAL Docmost web UI: log in, create several untitled pages, type a title, sync,
assert NOTHING is trashed. Reproduced the data loss before this fix; 5/5 green and
stable after. Engine suite 600 green (+2 computePushActions cases:
pageId-still-present -> skip; pageId-gone -> real delete).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reproduces the browser bug at the API level: create several untitled pages (all
collapse to the `_` fallback name), retitle one, sync — assert NO page is
trashed and all survive. Caught the data-loss bug fixed in 4376c5a6.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The push / 3-way-merge cases edited the FIRST real `.md` in the vault, leaving
`E2E-PUSH-*` / `E2E-MERGE-*` marker headings accumulating in a real page, and the
Docmost->git case left its created page in the Trash. Now the suite creates a
dedicated `E2E-SyncTarget-*` page and targets only that, and a teardown
hard-deletes every `E2E-*` fixture page and converges the vault on exit — so runs
never mutate real content and leave the stand clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Output of a generate→critique subagent pass on "what the feature's tests do NOT
cover", implemented + verified against the live stand (20/20). Complements the
basic two-way suite. Covers:
- protocol shape: unknown service subpath -> 400; unknown content-type -> 415
(global allowlist); PUT/DELETE on pack endpoints -> 400;
- path-traversal: `..%2f..`, `%2e%2e%2f`, bare `.git` space-id -> 400/404, no
escape, never a file leak;
- authz boundaries: a gitSync-DISABLED space -> 404 (existence hidden) and flips
to 200 when enabled; a READER member can fetch (200) but is FORBIDDEN to push
(403); a NON-member of an enabled space gets 403 (NOT 404 — the critic caught a
wrong generator assumption here; pinned as a contract);
- concurrency: a push while the per-space Redis lock is held -> 503 + Retry-After,
and the receive-pack does NOT mutate the vault;
- idempotency: repeated no-op cycles never churn `main` / `refs/docmost/last-pushed`;
- data-loss guard (PR #119): deleting MORE than GIT_SYNC_MAX_DELETES_PER_CYCLE is
HELD — none trashed AND last-pushed does not advance past the delete commit
(retry-safe, not silently dropped).
Auto-creates/tears down its fixtures (reader/non-member users, a 2nd space) and
resets the vault cache on exit so re-runs and the basic suite stay green. Needs
the vault dir + Redis container reachable (see header). A structural rename/move
case was intentionally left to the engine unit suite (git rename-similarity on
meta-only fixture pages is a fixture artifact, not a feature bug).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
Review #6 (approve-with-comments) follow-ups:
1. canonicalize step 7 now strips bare footnoteDefinitions at ANY depth
(stripFootnoteDefinitionsDeep), not just footnotesList, in BOTH copies. A
definition hand-authored outside a list (e.g. nested in a callout via a
raw-JSON write path) was left in place while a copy was also added to the
rebuilt list -> duplicate, idempotent, self-perpetuating. Runs only in the
rebuild path (after the lists are stripped); the fast-path / placement-keep
branch is untouched. Added a shared-corpus case (bare def nested in a callout)
to pin it in both mirrors.
2. markdown-clipboard: removed the dead top-level footnoteReference check in
canonicalizePastedFootnotes (an inline atom is never a top-level slice child;
only the descendants scan can find it).
Test coverage:
4. New MCP binding tests (full-doc-write-canonicalize.test.mjs): update_page_json
and copy_page_content canonicalize the persisted full doc, asserted via a new
`replacePage` seam (symmetric to the existing `mutatePage` seam) so no live
collab socket is needed. Routed both writers through the seam.
5. New server spec (file-import-task.service.footnote-canonicalize.spec.ts): the
zip-import path (processGenericImport) canonicalizes footnotes — real
markdown->HTML->JSON via a real ImportService over a temp-dir .md file, DB trx
stubbed to capture the persisted page content. FileImportTaskService had no
spec before.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- insertInlineFootnote could glue a footnoteReference inside an EXISTING
definition (nested footnotesList, or a bare footnoteDefinition with no list
wrapper), which canonicalize then dropped as an orphan — silently losing the
definition's prose. Now: (a) the body/notes boundary is computed from the first
top-level block that IS or CONTAINS (recursively) a footnotesList/
footnoteDefinition, not just a top-level list; and (b) the insertNodesAfterAnchor
core skips footnotesList/footnoteDefinition subtrees entirely (skipSubtreeTypes),
so an anchor whose only match is inside a definition -> inserted:false (clean
abort, no write). Added tests: nested-definition, bare-definition, and
body-before-nested-list-still-inserts.
- editor-ext footnote-canonicalize header listed `markdownToProseMirror` among the
canonicalizing MCP paths; it is the NON-canonicalizing primitive. Replaced with
`markdownToProseMirrorCanonical` (+ note that the plain primitive is for comment
bodies) and added copy_page_content.
- Client paste: canonicalizePastedFootnotes now skips a definitions-ONLY paste
(no footnoteReference anywhere) — canonicalizing it would strip the
reference-less list and yield an EMPTY paste. Added a test.
Suggestions:
- docmost_transform now runs validateDocStructure/validateDocUrls on the RAW
transform output BEFORE canonicalizeFootnotes (mirrors updatePageJson), so a
too-deep doc gives the intended max-depth error instead of a stack overflow.
- docmost_transform tool description now states the RESULT is footnote-canonical
(dryRun diff may show tidy-ups; idempotent after first run).
- insertFootnote: dropped the dead `result ? … : undefined` ternaries and the
`as any` casts (result is always set by the time we return; the not-found path
throws and aborts mutatePage). `const r = result!;`.
Tests / architecture:
- Added a LIVE-plugin golden case: the real footnoteSyncPlugin leaves a list with
non-empty content after it in place, and canonicalize agrees (placement parity
is now a driven property, not a hand-set expected).
- Added generateFootnoteId uuidv7 shape + uniqueness test.
- Item 9: added the ENFORCEMENT-RULE comments at the server parseProsemirrorContent
and the MCP canonicalizer header (any NEW full-doc persist path MUST canonicalize;
fragments/append/prepend and comment bodies MUST NOT). Kept per-call-site over a
brittle grep CI test (the replace-vs-fragment + comment-vs-page nuance makes a
single wrapper unsafe).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Approve-with-comments follow-ups (no blockers):
- callout: unify the GitHub-callout feature ticket on #192 (the callout-paste
feature the CHANGELOG already tracks); #218 is the public-share security work.
Fixed the code comment and test reference.
- export/utils.spec: pin current behavior of a leading-dot name (".gitignore" ->
"") — same bug class as #204 but unreachable via the sole caller, so document
not change.
- share.types: narrow ISharedPage to the actual /shares/page-info allowlist
(page -> Pick of id/slugId/title/icon/content; trimmed share; dropped the
spurious `extends IShare`). Verified all three consumers (shared-page,
link-view, mention-view) read only allowlist fields.
- editor-ext: extract shared CALLOUT_TYPES / normalizeCalloutType /
renderCalloutHtml into callout-common.marked.ts; both tokenizers
(`:::type` and `> [!type]`) now share the renderer + type dict while staying
separate. Eliminates the byte-identical renderer + duplicated type list.
- share.service: extract named predicate shareIdGrantsAccess(requestedShareId,
resolvedShare) for the id-or-key fast path (naming only, no control-flow
change); kept narrower than resolveReadableSharePage's id-only gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Approve-with-comments follow-ups:
- breadcrumb: fix the reverse regression where navigating A->B to a page absent
from the lazily-built tree (before its ancestors load) left the previous
page's clickable chain on screen. New pure computeBreadcrumbState clears a
stale chain that doesn't end at the current page, while keeping one that does
(no blank flash for an already-resolved page); unit-tested for the
navigated-to-absent-page case.
- share.service: getShareAncestorPage no longer swallows DB errors silently —
now a live public-share path (isPageReachableThroughShare), so a transient
error is logged with ancestor/child ids and still fails closed (caller 404s)
instead of becoming a traceless misleading "not found".
- i18n: register the new "Connecting… (read-only)" key (U+2026 ellipsis) in
en-US (source of truth) and ru-RU (Подключение… (только чтение)).
- share.service: correct the FUTURE note — 3 callers pass no shareId
(share-alias.controller/.service, share-seo.controller); the two ai-chat
callers already pass a real shareId.
- CHANGELOG: add Unreleased Changed/Fixed/Security entries for #216 opt-in
sub-pages default, #218 trimmed page-info payload + forged-shareId 404, #204
export internal-link name, #206/#218 breadcrumb, #192 callout paste, #218
editor pre-sync read-only gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- Move canonicalizeFootnotes OUT of parseProsemirrorContent. It now runs only
on FULL writes (createPage, updatePageContent operation==='replace'), never on
an append/prepend fragment (a fragment would lose definition-only footnotes or
synthesize a bogus empty list). Add a server binding spec.
- Match the live plugin's list PLACEMENT: a single already-canonical
footnotesList is left exactly where it sits (the plugin never repositions a
sole correct list), so the first write no longer reorders content that follows
the list. Applied to BOTH the editor-ext copy and the MCP mirror; pinned by a
shared golden corpus case with content after the list.
- Fix MCP tool count 38 -> 39 (README x3, AGENTS.md) and the transformJs param
help (add canonicalizeFootnotes/insertInlineFootnote).
Simplifications:
- Remove the dead duplicate re-id mechanism (deriveFootnoteId/suffix/occurrence)
from the PURE canonicalizer in both copies — references are never renamed, so
the derived ids were never requested; first-wins-drop is the real behaviour.
This also makes the editor-ext footnote-util note about "no cross-package copy"
true again.
- Remove the sentinel round-trip in insertInlineFootnote: a generalized
insertNodesAfterAnchor core inserts the footnoteReference node directly.
- Drop the redundant per-definition deep clone in step 4 (shallow id-normalizing
copy; out is already deep-cloned).
Docs / architecture:
- Correct the editor-ext copy's "It exists because…" header to its real
consumers (server import, page.service create/update, client paste).
- Note markdownToProseMirror reuse for create/update comment in collaboration.ts.
- A: shared golden JSON corpus exercised by BOTH the editor-ext copy and the MCP
mirror (footnote-corpus.ts / .mjs) so "the two copies behave identically" is
checkable.
- C: split the MCP canonicalizer into a pure mirror + footnote-authoring.ts.
- B: import services persist via a different path, so left one-line consolidation
comments at the call sites rather than folding (does not fall out cleanly).
Tests: insertFootnote wrapper guards + docmost_transform dryRun auto-canonicalize
(MCP mock), page.service create/update + append/prepend binding (server jest),
shared corpus incl. nested-container reference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-ups for the combined QA-UI fixes (#216/#206/#204/#218/#192):
- export/utils: correct the misleading getInternalLinkPageName comment — a
bare `v1.2` loses its last dot-segment (`v1`); dots survive only in
multi-segment names like `v1.2.md` -> `v1.2`.
- share: extract toPublicSharePayload(page, share): PublicSharePayload, an
explicit allowlist type+mapper replacing the inline literal in the
/shares/page-info anonymous path (#218). Add share.controller.spec.ts that
stubs getSharedPage returning internal fields and asserts the response key
set EXACTLY equals the whitelist (page + share), so any `...shareData`
regression or new leaking field fails. Also key-tests the extracted mapper.
- breadcrumb: extract pure resolveBreadcrumbNodes(treeData, ancestors, pageId)
(tree-hit -> tree; tree-miss -> map ancestors via canonical pageToTreeNode,
dropping the as-any casts; else null) and unit-test all three branches.
- share-modal: RTL test asserting enabling a share calls mutateAsync with
includeSubPages: false (#216 security default).
- share.service: one-line note at getSharedPage on the deferred consolidation
of the ancestor-aware match into resolveReadableSharePage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The footnote canonicalizer was wired into the MCP and editor-ext write paths
but NOT into the server's user-facing markdown/HTML import paths, so importing
or pasting markdown with out-of-order, reused, or orphan footnotes did not
canonicalize -- the exact trigger bug #228 fixes was still reproduced on
import. markdownToHtml -> htmlToJson builds ProseMirror JSON directly and never
runs the editor's footnoteSyncPlugin, and that plugin does not reorder an
existing list, so the stored footnotes kept the source's physical definition
order, retained orphans, and did not collapse reused references.
Wire canonicalizeFootnotes (already exported from @docmost/editor-ext) into
every server markdown/HTML -> page-JSON seam, before persisting:
- ImportService.importPage (REST single-file .md/.html import)
- FileImportTaskService (zip import worker)
- PageService.parseProsemirrorContent (API createPage / updatePageContent)
Also hook the client markdown paste: handlePaste applies a manual transaction
(returns true), bypassing transformPasted/footnoteSyncPlugin, so a pasted
out-of-order markdown footnote block would persist out of order.
canonicalizePastedFootnotes reorders a self-contained pasted block (one that
carries its own footnotesList) to reference order, deduped and orphan-free; it
is deliberately scoped to whole-block pastes so a reference-only paste that
reuses a footnote already defined in the target doc is left untouched.
canonicalizeFootnotes is pure, idempotent and shape-safe (a doc with no
footnotes is unchanged), so it is safe on every write path.
Residual: when a pasted block merges into a doc that already has footnotes,
ordering relative to the pre-existing footnotes is still governed by the live
sync plugin (which does not reorder across the boundary).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Additive test coverage across server, editor-ext, client and mcp.
#192 — AiChatService.stream integration (Section 3, against real Postgres):
- new apps/server/test/integration/ai-chat-stream.int-spec.ts drives the real
streamText through a seeded ai/test MockLanguageModelV3 and a real Node
ServerResponse, covering: onError persists an assistant error record
(status 'error' + partial answer + provider cause in metadata); external MCP
client closed exactly once on BOTH onFinish and onError; anti-tamper —
history is rebuilt from the DB transcript, not from body.messages.
#206 — red-team findings (most already fixed+tested in #212):
- mdrt-2 (UNFIXED, data loss): turndown.dataloss.test.ts documents that
pageBreak / transclusionReference / mention are silently dropped on Markdown
export (characterization + it.fails for the desired survive-export contract).
- persist-6 (UNFIXED, data loss): persistence-store.spec.ts adds an it.failing
documenting that a momentarily-empty live doc overwrites non-empty content
(left unfixed — a store-side empty-guard is a behaviour change).
#204 — test-strategy plan, highest-priority subset:
- Phase 1: mcp-clients.lease.spec.ts covers the external MCP client
lease/refcount/eviction lifecycle (leak / premature-close / double-close).
- Phase 2 data-integrity pure functions: editor-ext table-utils
(transpose/moveRow/convert round-trip) and math tokenizer false-positive
guard; client emoji-menu (+ it.fails for the unguarded localStorage
JSON.parse bug), sort-cells, normalizeTableColumnWidths; mcp htmlEmbed/
pageBreak markdown data-loss + footnote-diff; server export
getInternalLinkPageName extensionless-path bug — FIXED (small/clear) + tested.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Public sharing (#218):
- Bind public-share content to the requested shareId. getSharedPage now
enforces dto.shareId (forwarded from /share/:shareId/p/:slug): the page must
be reachable THROUGH that exact share (its own share, or an includeSubPages
ancestor that contains it). A forged/mismatched shareId 404s instead of
rendering off the slug alone and no longer leaks the real canonical key via
redirect. A request with no shareId keeps the legacy slug-capability path.
- Trim /shares/page-info: drop internal metadata (creatorId, spaceId,
workspaceId, contributorIds, lastUpdated*, parent/position, lock/template
flags, timestamps) from the anonymous payload.
- Default share-to-web includeSubPages to false (opt-in), so enabling a share
no longer silently exposes the whole sub-tree (#216).
Editor (#218):
- Harden the new-page pre-sync window: the body editor is kept read-only until
the collab provider is Connected and synced, so early keystrokes can't land
only in local ProseMirror and then be clobbered by the server's empty doc.
- Surface a "Connecting… (read-only)" affordance during the static phase so
input isn't silently swallowed.
Other:
- Breadcrumb: resolve from the page's own ancestor data (/pages/breadcrumbs)
instead of waiting for the lazily-built sidebar tree, so deep pages don't
render a blank breadcrumb for seconds.
- Pasting GitHub `> [!type]` callouts now converts to a callout node instead of
a literal blockquote (new marked extension wired into markdownToHtml).
Tests: editor-sync-state gate (client), getSharedPage share-binding (server),
github-callout markdown conversion (editor-ext).
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 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>
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>
