diff --git a/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.spec.tsx b/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.spec.tsx index 79f94ab7..5c9fcc19 100644 --- a/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.spec.tsx +++ b/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.spec.tsx @@ -6,6 +6,8 @@ import { nextReindexPollInterval, isReindexComplete, isReindexButtonLoading, + reindexRunKey, + isNewReindexRun, } from './ai-provider-settings'; describe('resolveCardStatus', () => { @@ -221,6 +223,128 @@ describe('isReindexComplete', () => { }); }); +describe('reindexRunKey', () => { + it('is null when the status carries no run identity', () => { + expect(reindexRunKey(undefined)).toBeNull(); + expect( + reindexRunKey({ reindexing: false, indexedPages: 5, totalPages: 5 }), + ).toBeNull(); + }); + + it('is null for a legacy/degraded record with an empty runId', () => { + // The server sends runId='' for a record written before the field existed; + // the client must treat that as "no identity" (fall back to prior behaviour). + expect( + reindexRunKey({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId: '', + reindexStartedAt: 1000, + }), + ).toBeNull(); + }); + + it('folds runId and startedAt into one stable key', () => { + expect( + reindexRunKey({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId: 'run-a', + reindexStartedAt: 1000, + }), + ).toBe('run-a:1000'); + }); + + it('changes when the runId changes for the same startedAt', () => { + const a = reindexRunKey({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId: 'run-a', + reindexStartedAt: 1000, + }); + const b = reindexRunKey({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId: 'run-b', + reindexStartedAt: 1000, + }); + expect(a).not.toBe(b); + }); + + it('changes when the same runId restarts at a new startedAt', () => { + const a = reindexRunKey({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId: 'run-a', + reindexStartedAt: 1000, + }); + const b = reindexRunKey({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId: 'run-a', + reindexStartedAt: 2000, + }); + expect(a).not.toBe(b); + }); +}); + +describe('isNewReindexRun (poll keying on runId)', () => { + // Derive the status shape from the helper itself so the test needs no export + // of the component-internal ReindexStatus type. + type ReindexStatusLike = NonNullable[0]>; + const run = (runId: string, startedAt: number): ReindexStatusLike => ({ + reindexing: true, + indexedPages: 0, + totalPages: 10, + runId, + reindexStartedAt: startedAt, + }); + + it('first identity after none latched is a NEW run', () => { + expect(isNewReindexRun(null, run('run-a', 1000))).toBe(true); + }); + + it('the SAME identity is not a new run (same run being watched)', () => { + const key = reindexRunKey(run('run-a', 1000)); + expect(isNewReindexRun(key, run('run-a', 1000))).toBe(false); + }); + + it('a DIFFERENT runId is a new run (reset per-run poll state)', () => { + const key = reindexRunKey(run('run-a', 1000)); + expect(isNewReindexRun(key, run('run-b', 1000))).toBe(true); + }); + + it('an identity-less poll (no runId / cleared record) is never a new run', () => { + const key = reindexRunKey(run('run-a', 1000)); + expect( + isNewReindexRun(key, { + reindexing: false, + indexedPages: 10, + totalPages: 10, + }), + ).toBe(false); + }); + + it('a legacy empty-runId poll does not spuriously reset a latched run', () => { + const key = reindexRunKey(run('run-a', 1000)); + expect( + isNewReindexRun(key, { + reindexing: true, + indexedPages: 3, + totalPages: 10, + runId: '', + reindexStartedAt: 1000, + }), + ).toBe(false); + }); +}); + describe('isReindexButtonLoading', () => { it('loads while the POST mutation is pending', () => { expect( diff --git a/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.tsx b/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.tsx index 1a0024ac..09ae0dd6 100644 --- a/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.tsx +++ b/apps/client/src/features/workspace/components/settings/components/ai-provider-settings.tsx @@ -173,9 +173,43 @@ export function resolveKeyField( // Subset of the status payload that drives the reindex poll decisions. type ReindexStatus = Pick< IAiSettings, - "reindexing" | "indexedPages" | "totalPages" + "reindexing" | "indexedPages" | "totalPages" | "runId" | "reindexStartedAt" >; +/** + * A stable per-RUN key for the reindex poll: `runId:startedAt`, or `null` when + * the status carries no run identity (no active run, or a legacy/degraded + * server record with an empty runId). Two polls of the SAME run share a key; a + * new run mints a fresh runId and so a different key. + * + * This is the single place the client turns the server's run identity into the + * value it keys on — it removes the "is this the same run I've been watching or + * a brand-new one?" ambiguity that made a class of reindex-status bugs (a stale + * pre-reindex snapshot vs a fresh run) get fixed twice (#262). `startedAt` is + * folded in so a run that somehow reuses a runId but restarted is still new. + */ +export function reindexRunKey(status: ReindexStatus | undefined): string | null { + const runId = status?.runId; + if (!runId) return null; + return `${runId}:${status?.reindexStartedAt ?? ""}`; +} + +/** + * Decide whether the latest poll represents a NEW reindex run relative to the + * run key the client last latched (`prevKey`, `null` if none yet). True only + * when the status carries an identity AND it differs from the latched one — the + * signal to reset any per-run poll state (the "seen active" latch / progress the + * UI held). The same identity (or no identity) is NOT a new run, so an unchanged + * or identity-less poll never resets mid-run. + */ +export function isNewReindexRun( + prevKey: string | null, + status: ReindexStatus | undefined, +): boolean { + const key = reindexRunKey(status); + return key !== null && key !== prevKey; +} + /** * Decide the TanStack Query `refetchInterval` while a reindex may be running. * Returns the poll interval (ms) to keep polling, or `false` to stop. @@ -320,6 +354,13 @@ export default function AiProviderSettings() { // counter at 0 until a manual reload. A ref (not state) because it must not // trigger a render and is only ever read where `reindexing` is already false. const reindexSeenActiveRef = useRef(false); + // The run identity (runId:startedAt) the current poll window is keyed on. When + // a poll reports a DIFFERENT runId the server has started a NEW run, so we + // re-latch to it and reset `reindexSeenActiveRef` — a fresh run must never + // inherit the previous run's "seen active"/completion state (which would stop + // polling immediately or read the old run's counters as this run's). null = + // no run keyed yet (steady state, or a legacy record without a runId). + const reindexRunKeyRef = useRef(null); // Only admins may read the (masked) AI settings; the server enforces this too. const { data: settings, isLoading } = useAiSettingsQuery(isAdmin, (query) => @@ -336,6 +377,14 @@ export default function AiProviderSettings() { // unmount because the deadline state goes away with the component. useEffect(() => { if (reindexDeadline === null) return; + // Key the poll on the run identity: if this poll carries a runId different + // from the one we latched, the server started a NEW run, so adopt it and + // drop the per-run "seen active" latch (a fresh run must not inherit the + // previous run's completion state). Same runId => same run, leave it alone. + if (isNewReindexRun(reindexRunKeyRef.current, settings)) { + reindexRunKeyRef.current = reindexRunKey(settings); + reindexSeenActiveRef.current = false; + } // Latch "we have seen the active run" the moment a poll reports it, so the // completion check below (and the refetchInterval's) only fires once the run // has genuinely started — never on the stale pre-reindex snapshot. @@ -1220,6 +1269,10 @@ export default function AiProviderSettings() { // immediately. onSuccess: () => { reindexSeenActiveRef.current = false; + // Forget the previous run's identity so the first poll of + // this window (carrying the new run's runId) is recognized + // as a new run and keyed afresh. + reindexRunKeyRef.current = null; setReindexDeadline(Date.now() + REINDEX_POLL_CAP_MS); }, }) diff --git a/apps/client/src/features/workspace/services/ai-settings-service.ts b/apps/client/src/features/workspace/services/ai-settings-service.ts index e12d1ebb..16b90674 100644 --- a/apps/client/src/features/workspace/services/ai-settings-service.ts +++ b/apps/client/src/features/workspace/services/ai-settings-service.ts @@ -51,6 +51,14 @@ export interface IAiSettings { // True while a full workspace reindex is actively running; the counts above // then reflect the live run progress (done climbs 0 -> total). reindexing?: boolean; + // Identity of the ACTIVE reindex run (present only while `reindexing`). The + // poll keys on `runId`: a changed value means a NEW run (reset the per-run + // poll state the UI latched), the same value is the run already being watched. + // Absent/empty ('') => no identity available; the client keeps prior behaviour. + runId?: string; + // Epoch-ms the active run started; paired with `runId` so a restart with a + // recycled id is still detected as a new run. + reindexStartedAt?: number; } // Update payload. Key semantics (same for `apiKey` and `embeddingApiKey`): diff --git a/apps/server/src/core/page/dto/page-identity.dto.spec.ts b/apps/server/src/core/page/dto/page-identity.dto.spec.ts new file mode 100644 index 00000000..25604399 --- /dev/null +++ b/apps/server/src/core/page/dto/page-identity.dto.spec.ts @@ -0,0 +1,49 @@ +import 'reflect-metadata'; +import { plainToInstance } from 'class-transformer'; +import { validate } from 'class-validator'; +import { PageIdDto } from './page.dto'; + +// #435: PageIdDto.pageId carries a page's DOUBLE identity (internal UUID OR +// public 10-char slugId), both as bare strings. The DTO must accept exactly +// those two FORMATS and reject a malformed / swapped identity at the boundary. +async function pageIdErrors(pageId: unknown) { + const dto = plainToInstance(PageIdDto, { pageId }); + const errors = await validate(dto as object); + return errors.some((e) => e.property === 'pageId'); +} + +const UUID = '019f499a-9f8c-7d68-b7be-ce100d7c6c56'; +const SLUG = 'aB3xQ7kR2p'; + +describe('PageIdDto pageId format validation', () => { + it('accepts a canonical page UUID', async () => { + expect(await pageIdErrors(UUID)).toBe(false); + }); + + it('accepts a 10-char slugId', async () => { + expect(await pageIdErrors(SLUG)).toBe(false); + }); + + it('rejects a truncated / wrong-length slug', async () => { + expect(await pageIdErrors('aB3xQ7kR2')).toBe(true); // 9 chars + expect(await pageIdErrors('aB3xQ7kR2pX')).toBe(true); // 11 chars + }); + + it('rejects a slug with an illegal character', async () => { + expect(await pageIdErrors('aB3xQ7kR2!')).toBe(true); + }); + + it('rejects a full URL / path-shaped identity (not the bare id)', async () => { + expect(await pageIdErrors(`my-page-title-${SLUG}`)).toBe(true); + expect(await pageIdErrors(`https://x/p/${SLUG}`)).toBe(true); + }); + + it('rejects a malformed UUID', async () => { + expect(await pageIdErrors('019f499a-9f8c-7d68-b7be')).toBe(true); + expect(await pageIdErrors('not-a-uuid-at-all-really')).toBe(true); + }); + + it('rejects an empty string', async () => { + expect(await pageIdErrors('')).toBe(true); + }); +}); diff --git a/apps/server/src/core/page/dto/page-identity.validator.ts b/apps/server/src/core/page/dto/page-identity.validator.ts new file mode 100644 index 00000000..01c7b343 --- /dev/null +++ b/apps/server/src/core/page/dto/page-identity.validator.ts @@ -0,0 +1,39 @@ +import { applyDecorators } from '@nestjs/common'; +import { Matches, ValidationOptions } from 'class-validator'; + +/** + * A page identity at the API boundary is EITHER the internal page UUID or the + * public 10-char slugId (page.repo.findById matches a non-UUID input as a + * slugId). Both arrive as bare strings, which is exactly how the two got swapped + * silently (incident family #435). This regex pins the two accepted FORMATS so a + * malformed / cross-wired identity (a truncated slug, a full URL, an email, an + * id from another entity kind that isn't even shaped like either) is rejected at + * the boundary instead of falling through to a confusing 404. + * + * - UUID: canonical 8-4-4-4-12 hex (version-agnostic — page ids are UUIDv7, + * so only the shape/length is enforced, matching the MCP's UUID_RE + * and the server's isValidUUID acceptance). + * - slugId: exactly 10 chars over [0-9A-Za-z] (nanoid `generateSlugId`). + * + * The two are disjoint (a UUID is 36 chars WITH dashes, a slugId 10 chars + * WITHOUT), so a value can only satisfy one branch. + */ +export const PAGE_ID_OR_SLUG_ID_REGEX = + /^(?:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9A-Za-z]{10})$/i; + +/** + * Validate that a DTO string field is a well-formed page identity (page UUID OR + * 10-char slugId). Composed decorator so the same format rule is applied + * consistently wherever a DTO accepts a `pageId` that may be either form. + */ +export function IsPageIdOrSlugId( + validationOptions?: ValidationOptions, +): PropertyDecorator { + return applyDecorators( + Matches(PAGE_ID_OR_SLUG_ID_REGEX, { + message: + 'pageId must be a page UUID or a 10-character slugId', + ...validationOptions, + }), + ); +} diff --git a/apps/server/src/core/page/dto/page.dto.ts b/apps/server/src/core/page/dto/page.dto.ts index c53f8ade..c3b64b23 100644 --- a/apps/server/src/core/page/dto/page.dto.ts +++ b/apps/server/src/core/page/dto/page.dto.ts @@ -9,10 +9,16 @@ import { import { Transform } from 'class-transformer'; import { ContentFormat } from './create-page.dto'; +import { IsPageIdOrSlugId } from './page-identity.validator'; export class PageIdDto { @IsString() @IsNotEmpty() + // Format-validate the double identity (#435): accept only a page UUID or a + // 10-char slugId so a malformed / swapped identity is rejected at the boundary + // rather than passed to the repo as a bare string. Base for PageInfoDto, + // DeletePageDto, BacklinksListDto, AddLabelsDto/RemoveLabelDto, etc. + @IsPageIdOrSlugId() pageId: string; } diff --git a/apps/server/src/integrations/ai/ai-settings.service.spec.ts b/apps/server/src/integrations/ai/ai-settings.service.spec.ts index ba263a3e..b4d03106 100644 --- a/apps/server/src/integrations/ai/ai-settings.service.spec.ts +++ b/apps/server/src/integrations/ai/ai-settings.service.spec.ts @@ -99,10 +99,12 @@ describe('AiSettingsService.getMasked reindex progress', () => { // actually pins the progress.total branch rather than coincidentally // matching the DB fallback. With fix #1 the two sources agree in practice, // but getMasked must still return progress.total when a record is active. + const startedAt = Date.now(); reindexProgress.get.mockResolvedValue({ total: 500, done: 120, - startedAt: Date.now(), + startedAt, + runId: 'run-abc', }); const masked = await service.getMasked(WORKSPACE_ID); @@ -110,6 +112,10 @@ describe('AiSettingsService.getMasked reindex progress', () => { expect(masked.indexedPages).toBe(120); // progress.done, not DB 478 expect(masked.totalPages).toBe(500); // progress.total, not DB 478 expect(masked.reindexing).toBe(true); + // The status payload must carry the run identity so the client can key its + // poll on it (a changed runId => a NEW run). + expect(masked.runId).toBe('run-abc'); + expect(masked.reindexStartedAt).toBe(startedAt); }); it('falls back to countIndexedPages when no reindex is active', async () => { @@ -121,6 +127,10 @@ describe('AiSettingsService.getMasked reindex progress', () => { expect(masked.indexedPages).toBe(478); expect(masked.totalPages).toBe(478); expect(masked.reindexing).toBe(false); + // No active run -> no run identity surfaced (the client keeps its prior + // steady-state behaviour). + expect(masked.runId).toBeUndefined(); + expect(masked.reindexStartedAt).toBeUndefined(); }); }); diff --git a/apps/server/src/integrations/ai/ai-settings.service.ts b/apps/server/src/integrations/ai/ai-settings.service.ts index 6be0b5be..384d5c2c 100644 --- a/apps/server/src/integrations/ai/ai-settings.service.ts +++ b/apps/server/src/integrations/ai/ai-settings.service.ts @@ -371,6 +371,12 @@ export class AiSettingsService { totalPages, // Optional hint for the client: a reindex run is currently in progress. reindexing: progress != null, + // Per-run identity so the client can key its poll on a stable run id and + // reset its per-run state when a NEW run starts. Present only while a run + // is active; `runId` may be '' for a legacy/degraded record (the client + // treats that as "no identity"). + runId: progress?.runId, + reindexStartedAt: progress?.startedAt, }; } diff --git a/apps/server/src/integrations/ai/ai.types.ts b/apps/server/src/integrations/ai/ai.types.ts index 06bf83e3..fa1757b6 100644 --- a/apps/server/src/integrations/ai/ai.types.ts +++ b/apps/server/src/integrations/ai/ai.types.ts @@ -149,4 +149,14 @@ export interface MaskedAiSettings { // True while a full workspace reindex is actively running (the counts above // then reflect the live run progress rather than the steady-state DB count). reindexing?: boolean; + // Identity of the ACTIVE reindex run (present only while `reindexing`). The + // client keys its poll on `runId`: a changed value means a NEW run (reset the + // per-run poll state it latched), the same value means the run it is already + // watching — removing the "same run or a fresh one?" ambiguity a stale + // pre-reindex snapshot otherwise causes. Absent/empty degrades gracefully. + runId?: string; + // Epoch-ms the active run started (present only while `reindexing`). Paired + // with `runId` so a run that restarts with the same (recycled) id is still + // seen as new. + reindexStartedAt?: number; } diff --git a/apps/server/src/integrations/ai/embedding-reindex-progress.service.spec.ts b/apps/server/src/integrations/ai/embedding-reindex-progress.service.spec.ts index 496d55e8..5be96e69 100644 --- a/apps/server/src/integrations/ai/embedding-reindex-progress.service.spec.ts +++ b/apps/server/src/integrations/ai/embedding-reindex-progress.service.spec.ts @@ -48,19 +48,38 @@ describe('EmbeddingReindexProgressService', () => { } describe('get', () => { - it('maps a valid hash to a ReindexProgress object', async () => { + it('maps a valid hash to a ReindexProgress object (incl. the run identity)', async () => { const { redis, hgetall } = makeRedis(); - hgetall.mockResolvedValue({ total: '478', done: '120', startedAt: '1000' }); + hgetall.mockResolvedValue({ + total: '478', + done: '120', + startedAt: '1000', + runId: 'run-xyz', + }); const service = makeService(redis); await expect(service.get(WORKSPACE_ID)).resolves.toEqual({ total: 478, done: 120, startedAt: 1000, + runId: 'run-xyz', }); expect(hgetall).toHaveBeenCalledWith(KEY); }); + it('degrades a missing runId to an empty string (legacy/partial record)', async () => { + const { redis, hgetall } = makeRedis(); + // A record written before runId existed: get() must still succeed and + // report runId='' so the client treats it as "no identity", never breaks. + hgetall.mockResolvedValue({ total: '10', done: '3', startedAt: '5' }); + await expect(makeService(redis).get(WORKSPACE_ID)).resolves.toEqual({ + total: 10, + done: 3, + startedAt: 5, + runId: '', + }); + }); + it('returns null for an empty hash (no record)', async () => { const { redis, hgetall } = makeRedis(); hgetall.mockResolvedValue({}); @@ -87,11 +106,17 @@ describe('EmbeddingReindexProgressService', () => { it('coerces a non-finite startedAt to 0', async () => { const { redis, hgetall } = makeRedis(); - hgetall.mockResolvedValue({ total: '10', done: '2', startedAt: 'nope' }); + hgetall.mockResolvedValue({ + total: '10', + done: '2', + startedAt: 'nope', + runId: 'run-1', + }); await expect(makeService(redis).get(WORKSPACE_ID)).resolves.toEqual({ total: 10, done: 2, startedAt: 0, + runId: 'run-1', }); }); @@ -115,6 +140,21 @@ describe('EmbeddingReindexProgressService', () => { expect(multiObj.exec).toHaveBeenCalledTimes(1); }); + it('mints a fresh non-empty runId into the record on each start', async () => { + const { redis, multiObj } = makeRedis(); + const service = makeService(redis); + await service.start(WORKSPACE_ID, 1); + await service.start(WORKSPACE_ID, 1); + + const firstRunId = multiObj.hset.mock.calls[0][1].runId; + const secondRunId = multiObj.hset.mock.calls[1][1].runId; + expect(typeof firstRunId).toBe('string'); + expect(firstRunId).not.toBe(''); + // Each run gets its OWN identity so the client can tell a re-trigger apart + // from the run it is already watching. + expect(secondRunId).not.toBe(firstRunId); + }); + it('defaults the expire TTL to the full 1h record TTL', async () => { const { redis, multiObj } = makeRedis(); await makeService(redis).start(WORKSPACE_ID, 478); diff --git a/apps/server/src/integrations/ai/embedding-reindex-progress.service.ts b/apps/server/src/integrations/ai/embedding-reindex-progress.service.ts index 2f551f0d..22bc5424 100644 --- a/apps/server/src/integrations/ai/embedding-reindex-progress.service.ts +++ b/apps/server/src/integrations/ai/embedding-reindex-progress.service.ts @@ -1,17 +1,28 @@ import { Injectable, Logger } from '@nestjs/common'; import { RedisService } from '@nestjs-labs/nestjs-ioredis'; +import { randomUUID } from 'node:crypto'; import type { Redis } from 'ioredis'; /** * Live progress of an in-flight workspace embeddings reindex run. * `total` is the number of pages the run will process, `done` how many it has * already processed (success OR handled failure), `startedAt` the epoch-ms the - * record was created. + * record was created, and `runId` a per-run identity minted at `start()`. + * + * `runId` gives each reindex run a stable identity so a poller can tell "same + * run I've been watching" from "a NEW run started" WITHOUT guessing from the + * progress counters (the ambiguity that a stale pre-reindex snapshot vs a fresh + * run otherwise causes — the bug class fixed twice under #262). It is best- + * effort like the rest of this record: a record written before this field + * existed (or a Redis hiccup) yields an empty `runId`, which the client must + * treat as "no identity available" and degrade to its prior behaviour, never + * break. */ export interface ReindexProgress { total: number; done: number; startedAt: number; + runId: string; } /** Redis key namespace for the per-workspace reindex-progress record. */ @@ -86,12 +97,18 @@ export class EmbeddingReindexProgressService { ): Promise { const key = this.key(workspaceId); try { + // A fresh identity per run so the client poll can key on it: a changed + // runId means a genuinely NEW run (reset any latched per-run poll state), + // the same runId means the run the client is already watching. Best-effort + // like the counters — never surfaced to the user, only used to disambiguate. + const runId = randomUUID(); await this.redis .multi() .hset(key, { total: String(total), done: '0', startedAt: String(Date.now()), + runId, }) .expire(key, ttlSeconds) .exec(); @@ -150,7 +167,15 @@ export class EmbeddingReindexProgressService { const done = Number(data.done); const startedAt = Number(data.startedAt); if (!Number.isFinite(total) || !Number.isFinite(done)) return null; - return { total, done, startedAt: Number.isFinite(startedAt) ? startedAt : 0 }; + // `runId` degrades gracefully: a pre-existing record (written before this + // field) or a stripped value reads as '' — the client treats that as "no + // identity" and keeps its prior behaviour rather than breaking the poll. + return { + total, + done, + startedAt: Number.isFinite(startedAt) ? startedAt : 0, + runId: typeof data.runId === 'string' ? data.runId : '', + }; } catch (err) { this.logger.warn( `reindex-progress read failed for workspace ${workspaceId}; ` + diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 75c62871..32f28e79 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -33,6 +33,19 @@ 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"; + // The full public + shared instance surface of the assembled client. Built by // INTERSECTING each domain mixin's public interface (each DERIVED from its class // and enforced by that class's `implements` clause — issue #446, no hand-mirror) diff --git a/packages/mcp/src/client/context.ts b/packages/mcp/src/client/context.ts index 8c784f53..c17576c0 100644 --- a/packages/mcp/src/client/context.ts +++ b/packages/mcp/src/client/context.ts @@ -24,6 +24,7 @@ import { isCollabAuthFailedError, } from "../lib/collab-session.js"; import { withPageLock, isUuid } from "../lib/page-lock.js"; +import type { PageId } from "../lib/page-id.js"; import { getCollabToken, performLogin } from "../lib/auth-utils.js"; import { formatDocmostAxiosError } from "./errors.js"; import { GetPageConversionCache } from "./getpage-cache.js"; @@ -715,10 +716,17 @@ export abstract class DocmostClientContext { * once via getPageRaw and cached (both slugId->uuid and uuid->uuid), so * repeated edits on the same page add no extra request. */ - protected async resolvePageId(pageId: string): Promise { - if (isUuid(pageId)) return pageId; + protected async resolvePageId(pageId: string): Promise { + // This is the ONE canonicalization seam, so it is where the `PageId` brand + // 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. + if (isUuid(pageId)) return pageId as PageId; const cached = this.pageIdCache.get(pageId); - if (cached) return cached; + if (cached) return cached as PageId; const data = await this.getPageRaw(pageId); const uuid = data?.id; if (typeof uuid !== "string" || !uuid) { @@ -727,7 +735,7 @@ export abstract class DocmostClientContext { ); } this.pageIdCache.set(pageId, uuid); - return uuid; + return uuid as PageId; } diff --git a/packages/mcp/src/lib/collaboration.ts b/packages/mcp/src/lib/collaboration.ts index 66d1d9ce..08764e77 100644 --- a/packages/mcp/src/lib/collaboration.ts +++ b/packages/mcp/src/lib/collaboration.ts @@ -13,6 +13,7 @@ import { JSDOM } from "jsdom"; import { markdownToProseMirror } from "@docmost/prosemirror-markdown"; import { docmostExtensions, docmostSchema } from "./docmost-schema.js"; import { withPageLock } from "./page-lock.js"; +import type { PageId } from "./page-id.js"; import { sanitizeForYjs, findUnstorableAttr, @@ -250,7 +251,10 @@ export function assertYjsEncodable(doc: any): void { * read->write window, and it never throws (it can NEVER break a write). */ export async function mutatePageContent( - pageId: string, + // Canonical UUID only (#260/#435): the brand forces every caller to + // resolvePageId() BEFORE this seam so the lock + CollabSession key can never + // be a raw slugId. + pageId: PageId, collabToken: string, baseUrl: string, transform: (liveDoc: any) => any | null, @@ -300,7 +304,7 @@ export async function mutatePageContent( * mutatePageContent. */ export async function replacePageContent( - pageId: string, + pageId: PageId, prosemirrorDoc: any, collabToken: string, baseUrl: string, @@ -332,7 +336,7 @@ export async function replacePageContent( * Tables and :::callout::: blocks survive thanks to the full schema. */ export async function updatePageContentRealtime( - pageId: string, + pageId: PageId, markdownContent: string, collabToken: string, baseUrl: string, diff --git a/packages/mcp/src/lib/page-id.ts b/packages/mcp/src/lib/page-id.ts new file mode 100644 index 00000000..cff42ed6 --- /dev/null +++ b/packages/mcp/src/lib/page-id.ts @@ -0,0 +1,81 @@ +/** + * Branded page-identity types 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. + * + * 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. + */ +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/src/lib/page-lock.ts b/packages/mcp/src/lib/page-lock.ts index c17033c8..2ab4f927 100644 --- a/packages/mcp/src/lib/page-lock.ts +++ b/packages/mcp/src/lib/page-lock.ts @@ -8,6 +8,8 @@ * failure) before it runs. Different pages never block each other. */ +import type { PageId } from "./page-id.js"; + const chains = new Map>(); // Canonical UUID shape (versions 1–8, matching the `uuid` package's `validate` @@ -28,11 +30,14 @@ export function isUuid(value: string): boolean { // awaited/handled by the caller; only the internal chaining tail swallows // errors (purely to gate ordering). export function withPageLock( - pageId: string, + pageId: PageId, fn: () => Promise, ): Promise { - // STRUCTURAL INVARIANT (issue #449, "resolve-then-lock"): the mutex key MUST - // be the canonical page UUID, never a raw slugId. The whole write path relies + // STRUCTURAL INVARIANT (issue #449/#435, "resolve-then-lock"): the mutex key + // MUST be the canonical page UUID, never a raw slugId. The `PageId` brand now + // enforces this at COMPILE time (a raw string / slugId no longer type-checks + // as a key); the runtime assert below stays as a backstop for untyped (JS) + // callers and the http/stdio transports. The whole write path relies // on the lock key AND the CollabSession cache key being the resolved UUID // (#260) — if a future write method forgot to call resolvePageId and locked // under a slugId, two writes to the same page would take DIFFERENT mutex keys diff --git a/packages/mcp/test/unit/page-id.test.mjs b/packages/mcp/test/unit/page-id.test.mjs new file mode 100644 index 00000000..1dfcbf99 --- /dev/null +++ b/packages/mcp/test/unit/page-id.test.mjs @@ -0,0 +1,57 @@ +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/); +});