From 45ff922dd497ec95a4ea27cb43d3ba870f552478 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 00:37:24 +0300 Subject: [PATCH] =?UTF-8?q?refactor(mcp):=20=D1=83=D0=B1=D1=80=D0=B0=D1=82?= =?UTF-8?q?=D1=8C=20=D0=BC=D1=91=D1=80=D1=82=D0=B2=D1=8B=D0=B9=20runtime-?= =?UTF-8?q?=D1=81=D0=BB=D0=BE=D0=B9=20page-id,=20=D0=BE=D1=81=D1=82=D0=B0?= =?UTF-8?q?=D0=B2=D0=B8=D1=82=D1=8C=20=D0=B1=D1=80=D0=B5=D0=BD=D0=B4=20Pag?= =?UTF-8?q?eId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью #516: isPageId/asPageId/isSlugId/asSlugId/SLUG_ID_RE + типы SlugId/PageRef имели НОЛЬ вызовов в монорепе — только барные ре-экспорты в client.ts и кейсы теста. Несущий смысл — только compile-time бренд PageId (минтится as PageId в resolvePageId, единственном узле канонизации; asPageId туда намеренно не звался). Докстринг заявлял «asPageId() guards the untrusted PUBLIC boundary» — но никто не звал: спекулятивный вес. Удалил мёртвые runtime-символы + типы SlugId/PageRef + ре-экспорты + их тест-файл. Оставил тип PageId + касты. Поправил врущий коммент в resolvePageId (бренд — чистый compile-time маркер, гарантия — что этот узел единственный производитель). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client.ts | 17 ++--- packages/mcp/src/client/context.ts | 7 +- packages/mcp/src/lib/page-id.ts | 89 ++++--------------------- packages/mcp/test/unit/page-id.test.mjs | 57 ---------------- 4 files changed, 22 insertions(+), 148 deletions(-) delete mode 100644 packages/mcp/test/unit/page-id.test.mjs diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 32f28e79..e00f96c3 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -33,18 +33,11 @@ import { TransformsMixin, type ITransformsMixin } from "./client/transforms.js"; export type { DocmostMcpConfig, SandboxPut } from "./client/context.js"; export { formatDocmostAxiosError, assertFullUuid } from "./client/errors.js"; -// Branded page-identity types + validating constructors/guards (#435): a page's -// internal UUID (`PageId`) and public slug (`SlugId`) are distinct nominal types -// so the two can't be swapped as bare strings. Re-exported on the package -// surface so hosts/tests can validate + brand identities at their boundaries. -export type { PageId, SlugId, PageRef } from "./lib/page-id.js"; -export { - asPageId, - asSlugId, - isPageId, - isSlugId, - SLUG_ID_RE, -} from "./lib/page-id.js"; +// Branded canonical page-identity type (#435): the internal page UUID is a +// distinct nominal type so an unresolved raw/slug string can't be swapped into +// the seams that require the canonical id (see lib/page-id.ts). Re-exported on +// the package surface for hosts that type against the resolved id. +export type { PageId } from "./lib/page-id.js"; // The full public + shared instance surface of the assembled client. Built by // INTERSECTING each domain mixin's public interface (each DERIVED from its class diff --git a/packages/mcp/src/client/context.ts b/packages/mcp/src/client/context.ts index c17576c0..c966d74e 100644 --- a/packages/mcp/src/client/context.ts +++ b/packages/mcp/src/client/context.ts @@ -721,9 +721,10 @@ export abstract class DocmostClientContext { // is minted (#435). The value is validated here — a UUID input by isUuid, a // resolved id as the server's own page.id — so the downstream write path // (withPageLock / mutatePageContent) can require the brand and reject any - // unresolved raw id at compile time. The brand is applied by cast, not the - // asPageId() validator, to avoid rejecting the fake ids the mock tests feed - // a server stub; asPageId() guards the untrusted PUBLIC boundary instead. + // unresolved raw id at compile time. The brand is a pure compile-time marker + // applied by cast (no runtime guard): the guarantee is that this seam is the + // only place a `PageId` is produced, so every branded value went through the + // UUID/resolve check above. if (isUuid(pageId)) return pageId as PageId; const cached = this.pageIdCache.get(pageId); if (cached) return cached as PageId; diff --git a/packages/mcp/src/lib/page-id.ts b/packages/mcp/src/lib/page-id.ts index cff42ed6..a8ca68fa 100644 --- a/packages/mcp/src/lib/page-id.ts +++ b/packages/mcp/src/lib/page-id.ts @@ -1,81 +1,18 @@ /** - * Branded page-identity types for the MCP client (incident family #435). + * Branded canonical page-identity type for the MCP client (incident family #435). * - * A Docmost page has TWO identities that are BOTH plain strings: - * - the internal `page.id` — a canonical UUID (the server generates UUIDv7), - * - the public `slugId` — a 10-char nanoid over [0-9A-Za-z] used in URLs. - * Because both are bare `string`s, they were passed around interchangeably and - * silently swapped (e.g. locking/keying a collab doc by the slugId instead of - * the UUID — the #260 data-loss). Branding them as distinct nominal types makes - * a swap a COMPILE error at the seams that matter, and the validating - * constructors reject a malformed / cross-wired identity at runtime too. + * A Docmost page has TWO identities that are both plain strings: the internal + * `page.id` (a canonical UUID the server generates as UUIDv7) and the public + * `slugId` (a 10-char nanoid used in URLs). Because both are bare `string`s they + * were passed around interchangeably and silently swapped — e.g. locking/keying a + * collab doc by the slugId instead of the UUID (the #260 data-loss). * - * These are type-level + format-validation helpers ONLY: a `PageId`/`SlugId` is - * still a `string` at runtime (assignable INTO any `string` parameter with no - * change), so branding a value flows outward for free; only the few seams that - * REQUIRE a canonical id (resolvePageId's result, the per-page lock key, the - * collab write entrypoints) demand the brand and so catch an unresolved raw id. + * `PageId` brands the CANONICAL id as a distinct nominal type so a raw/unresolved + * string cannot flow into the seams that REQUIRE the canonical id (resolvePageId's + * result, the per-page lock key, the collab write entrypoints) — those become a + * COMPILE error, catching a swap at build time. It is still a `string` at runtime + * (assignable INTO any `string` parameter unchanged), so branding flows outward + * for free; the brand is minted at the single canonicalization seam + * (`resolvePageId`, via `as PageId`). */ -import { UUID_RE } from "./page-lock.js"; - -/** The internal canonical page id (`page.id`), a canonical UUID. */ export type PageId = string & { readonly __brand: "PageId" }; - -/** The public page slug id (`slugId`), a 10-char nanoid used in URLs. */ -export type SlugId = string & { readonly __brand: "SlugId" }; - -/** - * A page REFERENCE an agent may supply: EITHER the canonical UUID `PageId` or - * the public `SlugId`. The server's page lookup accepts both (a non-UUID is - * matched as a slugId), so the public read/tool boundary legitimately takes - * either form; only the resolved-to-canonical value is a strict `PageId`. - */ -export type PageRef = PageId | SlugId; - -// A slugId is exactly 10 chars from the nanoid alphabet the server uses for -// generateSlugId: [0-9A-Za-z] (see apps/server .../common/helpers/nanoid.utils). -// Disjoint from a UUID (which is 36 chars WITH dashes), so the two formats can -// never be confused for one another. -export const SLUG_ID_RE = /^[0-9A-Za-z]{10}$/; - -/** Type guard: is `value` a canonical page UUID (`PageId`)? */ -export function isPageId(value: unknown): value is PageId { - return typeof value === "string" && UUID_RE.test(value); -} - -/** Type guard: is `value` a public slug id (`SlugId`)? */ -export function isSlugId(value: unknown): value is SlugId { - return typeof value === "string" && SLUG_ID_RE.test(value); -} - -/** - * Validate + brand a canonical page id. Throws an actionable error (BEFORE any - * network use) when `value` is not a canonical UUID, so a slugId or garbage - * cross-wired where the canonical id is required fails fast and loud instead of - * silently splitting a lock/doc key (#260). `label` names the offending param. - */ -export function asPageId(value: string, label = "pageId"): PageId { - if (!isPageId(value)) { - throw new Error( - `${label}: expected a canonical page UUID (36 chars, e.g. ` + - `019f499a-9f8c-7d68-b7be-ce100d7c6c56), got '${value}'. A slugId or ` + - `other identity must be resolved to the page UUID first.`, - ); - } - return value; -} - -/** - * Validate + brand a public slug id. Throws when `value` is not the 10-char - * slugId format, so a UUID or garbage cross-wired where a slugId is required is - * rejected at the boundary. - */ -export function asSlugId(value: string, label = "slugId"): SlugId { - if (!isSlugId(value)) { - throw new Error( - `${label}: expected a 10-char page slugId (e.g. 'aB3xQ7kR2p'), got ` + - `'${value}'.`, - ); - } - return value; -} diff --git a/packages/mcp/test/unit/page-id.test.mjs b/packages/mcp/test/unit/page-id.test.mjs deleted file mode 100644 index 1dfcbf99..00000000 --- a/packages/mcp/test/unit/page-id.test.mjs +++ /dev/null @@ -1,57 +0,0 @@ -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - asPageId, - asSlugId, - isPageId, - isSlugId, - SLUG_ID_RE, -} from "../../build/lib/page-id.js"; - -// A real canonical page UUID (UUIDv7-shaped) and a real 10-char slugId. -const UUID = "019f499a-9f8c-7d68-b7be-ce100d7c6c56"; -const SLUG = "aB3xQ7kR2p"; - -test("isPageId accepts a canonical UUID and rejects a slugId / garbage", () => { - assert.equal(isPageId(UUID), true); - assert.equal(isPageId(SLUG), false); - assert.equal(isPageId("not-a-uuid"), false); - assert.equal(isPageId(""), false); - assert.equal(isPageId(undefined), false); - assert.equal(isPageId(123), false); -}); - -test("isSlugId accepts the 10-char slug format and rejects a UUID / wrong length", () => { - assert.equal(isSlugId(SLUG), true); - assert.equal(isSlugId(UUID), false); - assert.equal(isSlugId("short"), false); // < 10 chars - assert.equal(isSlugId("aB3xQ7kR2pX"), false); // 11 chars - assert.equal(isSlugId("aB3xQ7kR2!"), false); // illegal char - assert.equal(isSlugId(""), false); - assert.equal(isSlugId(null), false); -}); - -test("SLUG_ID_RE is anchored (no substring match inside a longer string)", () => { - assert.equal(SLUG_ID_RE.test(`prefix-${SLUG}`), false); - assert.equal(SLUG_ID_RE.test(`${SLUG}-suffix`), false); -}); - -test("asPageId returns the branded value unchanged for a valid UUID", () => { - assert.equal(asPageId(UUID), UUID); -}); - -test("asPageId throws an actionable error for a slugId (the #260 swap)", () => { - assert.throws(() => asPageId(SLUG), /canonical page UUID/); - // The offending value and a custom label are surfaced for self-correction. - assert.throws(() => asPageId("garbage", "targetPageId"), /targetPageId/); - assert.throws(() => asPageId("garbage", "targetPageId"), /garbage/); -}); - -test("asSlugId returns the branded value unchanged for a valid slugId", () => { - assert.equal(asSlugId(SLUG), SLUG); -}); - -test("asSlugId throws for a UUID cross-wired where a slugId is required", () => { - assert.throws(() => asSlugId(UUID), /10-char page slugId/); - assert.throws(() => asSlugId("nope"), /nope/); -});