Extract shared ID-existence/kind validation helper (reference targets + access-token grants)

.agents/skills/pr-backlog-reflection/SKILL.md

@@ -47,11 +47,11 @@ the work directly revealed a new, evidenced consequence.
 
 Classify each valid observation before presenting it:
 
-| Classification | Use when |
-| --- | --- |
+| Classification     | Use when                                                                                                                                                                           |
+| ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | Current-PR finding | It violates the intended contract, creates a credible regression, or is necessary for a supported surface. Keep it out of the backlog list and raise it in the active review/work. |
-| Backlog candidate | It is valuable, concrete, and can be delivered independently after this PR. |
-| Note only | It lacks enough evidence, duplicates existing work, or has no meaningful outcome. |
+| Backlog candidate  | It is valuable, concrete, and can be delivered independently after this PR.                                                                                                        |
+| Note only          | It lacks enough evidence, duplicates existing work, or has no meaningful outcome.                                                                                                  |
 
 ## Check that the work is not already tracked
 

docs/specifications/internal-links.md

@@ -11,7 +11,7 @@ Compendium has exactly two ways to write an internal link, and both persist only
 - A **`page_link` block** stores its target on the block record's own `referencedRecordId` (Documents only, set today from the editor UI's document picker).
 - An **inline `[[wiki link]]`** is an ordinary rich-text `link` mark whose href uses a `record:<id>` scheme (Documents or Collections; see `markdown-transcoding.md` for the Markdown ⇄ mark boundary).
 
-Both resolve to a display title the same way, live, at read time — via `resolveInternalLinkTarget(doc, id)` in [`src/lib/data/links.ts`](../../src/lib/data/links.ts). That module is the one place "does this ID still name a Document or Collection, and what's it called now" is answered; `markdown-transcode.ts`, the `documents` service, and `BlockEditor.svelte` all call into it rather than each re-deriving the answer. This is what makes renaming or moving the target a non-event: nothing that points at it needs to change, because nothing that points at it ever stored the title.
+Both resolve to a display title the same way, live, at read time — via `resolveInternalLinkTarget(doc, id)` in [`src/lib/data/links.ts`](../../src/lib/data/links.ts). That module is the one place "does this ID still name a Document or Collection, and what's it called now" is answered; `markdown-transcode.ts`, the `documents` service, and `BlockEditor.svelte` all call into it for display, and (since issue #62) `services/records.ts`'s `referencedRecordId` validation and `services/tokens.ts#createToken`'s Document/Collection grant-existence checks call into it for validation — every one of these rather than each re-deriving the answer. This is what makes renaming or moving the target a non-event: nothing that points at it needs to change, because nothing that points at it ever stored the title.
 
 `src/lib/data/links.ts` also exports `listOutgoingLinks(doc, documentId)`, which walks a Document's own records (`page_link` blocks and inline `record:` marks alike) and resolves each to the same `{ id, kind, title } | undefined` shape. `listIncomingLinks(doc, targetId)` builds its reverse index from that same function on first use, then incrementally refreshes only a changed link-bearing record or source Document's metadata. It returns the referring Document, the exact referring block, and its live text context. Outgoing and incoming views therefore derive from one ID-backed representation rather than parallel ad hoc scans.
 

docs/specifications/service-layer.md

@@ -71,7 +71,7 @@ src/lib/services/
                   revokeToken(actor, tokenHash) → void
 ```
 
-`tokens.ts` and `spaces.ts` are UI-only (`settings/tokens`, `/api/spaces`) — no MCP tool exposes minting or revoking a token or creating a Space. They are nevertheless registered in `services/manifest.ts` with `mcp: false, ui: true`, alongside token listing and audit-history listing, so adapter ownership is enforced without accidentally exposing them over MCP. `tokens.ts#createToken` still validates `allowedSpaceIds` against the workspace's real Spaces before persisting (#188) — a crafted request could otherwise grant a token access to a Space id that merely happens to exist somewhere else, since Space membership alone later authorizes access (`tokenAllowsParent`).
+`tokens.ts` and `spaces.ts` are UI-only (`settings/tokens`, `/api/spaces`) — no MCP tool exposes minting or revoking a token or creating a Space. They are nevertheless registered in `services/manifest.ts` with `mcp: false, ui: true`, alongside token listing and audit-history listing, so adapter ownership is enforced without accidentally exposing them over MCP. `tokens.ts#createToken` validates every grant list — `allowedSpaceIds` against the workspace's real Spaces (#188), and (since #62) `allowedDocumentIds`/`allowedCollectionIds` against `resolveInternalLinkTarget` (`data/links.ts`) — before persisting, all three through one shared `validateEvery(ids, existsFn, ErrorClass)` helper. A crafted request could otherwise grant a token access to an id that merely happens to exist somewhere else (or doesn't exist at all), since existence alone later authorizes access (`tokenAllowsParent`) — no permission check is needed for a grant itself, since Phase 0 has no membership model gating who a caller may grant a _future_ token access to.
 
 Each function's first parameter is whatever identifies the caller for permission purposes — an `AccessToken` for MCP-originated calls, the fixed `CURRENT_USER` `ActorId` for Phase 0/1 UI calls (see `data-model.md` §1's `ActorId` union; this doesn't need to change). Where MCP and UI calls to the "same" use case need different permission rules (e.g. UI writes are currently unscoped, single-tenant; MCP writes are token-scoped), the service function is the one place that branches on that — not duplicated per adapter.
 

src/lib/services/records.ts

@@ -5,14 +5,14 @@ import { clientIdForToken, isHeldByClient, releaseAgentHold } from '$lib/server/
 import {
 	createRecord as crdtCreateRecord,
 	deleteRecord as crdtDeleteRecord,
-	getCollection as crdtGetCollection,
 	getDocument as crdtGetDocument,
 	getRecordYText,
 	setRecordReferencedId as crdtSetRecordReferencedId,
 	setRecordViewConfig as crdtSetRecordViewConfig,
 	updateRecordContent,
 	updateRecordProperties
 } from '$lib/data/records';
+import { resolveInternalLinkTarget } from '$lib/data/links';
 import { logAudit } from '$lib/server/audit';
 import { reserveRecordLocator, releaseRecordLocator } from '$lib/server/catalog';
 import { markdownToRichText } from '$lib/mcp/markdown-transcode';
@@ -69,32 +69,57 @@ export class InvalidLinkTargetError extends Error {
 // all; it's optional there) is one too.
 const DOCUMENT_REFERENCE_BLOCK_TYPES: readonly BlockType[] = ['page_link', 'child_pages'];
 
-function validateDocumentReferenceTarget(caller: CallerIdentity, targetId: string): void {
-	// The target is always a Document, which has its own real shard (#120) —
-	// resolveParentWorkspaceContext finds it via the catalog locator, falling
-	// back to the default doc for an untracked/legacy target.
+const REFERENCE_TARGET_ERROR_KIND = { document: 'Document', collection: 'Collection' } as const;
+
+/**
+ * Validates a `referencedRecordId` against the target kind its block type
+ * requires — `document` for page_link/child_pages, `collection` for
+ * collection_view (mirrors the `linkedTarget?.kind === 'collection'` check
+ * the read side already applies, services/documents.ts's resolveRecordLink,
+ * closing the write-side gap tracked by issue #37). Built on
+ * `resolveInternalLinkTarget` (src/lib/data/links.ts) rather than a direct
+ * `crdtGetDocument`/`crdtGetCollection` call, so "does this ID exist, and as
+ * which kind" is answered in one shared place instead of reimplemented per
+ * kind (issue #62).
+ */
+function validateReferenceTarget(
+	caller: CallerIdentity,
+	targetId: string,
+	kind: 'document' | 'collection'
+): void {
+	// The target has its own real shard (#120) — resolveParentWorkspaceContext
+	// finds it via the catalog locator, falling back to the default doc for an
+	// untracked/legacy target.
 	const { doc, parentSpaceId } = resolveParentWorkspaceContext(targetId);
-	const target = crdtGetDocument(doc, targetId);
-	if (!target) throw new InvalidLinkTargetError(targetId);
+	const target = resolveInternalLinkTarget(doc, targetId);
+	const errorKind = REFERENCE_TARGET_ERROR_KIND[kind];
+	if (target?.kind !== kind) throw new InvalidLinkTargetError(targetId, errorKind);
 	if (isAccessToken(caller) && !tokenAllowsParent(caller, targetId, parentSpaceId)) {
-		throw new InvalidLinkTargetError(targetId);
+		throw new InvalidLinkTargetError(targetId, errorKind);
 	}
 }
 
-// A collection_view block's referencedRecordId must resolve to an existing,
-// caller-accessible Collection (never a Document) — mirrors the
-// `linkedTarget?.kind === 'collection'` check the read side already applies
-// (services/documents.ts's resolveRecordLink), closing the write-side gap
-// tracked by issue #37.
-function validateCollectionReferenceTarget(caller: CallerIdentity, targetId: string): void {
-	// A Collection has its own real shard too (#120) — same locator-backed
-	// resolution validateDocumentReferenceTarget uses above.
-	const { doc, parentSpaceId } = resolveParentWorkspaceContext(targetId);
-	const target = crdtGetCollection(doc, targetId);
-	if (!target) throw new InvalidLinkTargetError(targetId, 'Collection');
-	if (isAccessToken(caller) && !tokenAllowsParent(caller, targetId, parentSpaceId)) {
-		throw new InvalidLinkTargetError(targetId, 'Collection');
-	}
+/**
+ * The shared guard shape behind every page_link/child_pages/collection_view
+ * `referencedRecordId` check: the record's own parent must be a Document
+ * (both block types below only ever exist inside one), and the target must
+ * resolve to the given kind and be caller-accessible. `createRecord`'s and
+ * `writeRecord`'s validators (below) differ only in which block types they
+ * accept and their error wording — that difference is real (writeRecord
+ * doesn't support retargeting child_pages, whose target is UI-only post-
+ * creation) and stays in each of them; this is just their shared middle step
+ * (issue #62).
+ */
+function requireParentDocumentThenValidateTarget(
+	caller: CallerIdentity,
+	doc: Y.Doc,
+	parentId: string,
+	targetId: string,
+	kind: 'document' | 'collection',
+	parentErrorMessage: string
+): void {
+	if (!crdtGetDocument(doc, parentId)) throw new Error(parentErrorMessage);
+	validateReferenceTarget(caller, targetId, kind);
 }
 
 const VIEW_TYPES: readonly ViewType[] = ['table', 'board', 'calendar'];
@@ -133,17 +158,25 @@ function validateCreateReferencedRecordId(
 	referencedRecordId: string
 ): void {
 	if (blockType && DOCUMENT_REFERENCE_BLOCK_TYPES.includes(blockType)) {
-		if (!crdtGetDocument(doc, parentId)) {
-			throw new Error('page_link and child_pages blocks can only be created inside a Document.');
-		}
-		validateDocumentReferenceTarget(caller, referencedRecordId);
+		requireParentDocumentThenValidateTarget(
+			caller,
+			doc,
+			parentId,
+			referencedRecordId,
+			'document',
+			'page_link and child_pages blocks can only be created inside a Document.'
+		);
 		return;
 	}
 	if (blockType === 'collection_view') {
-		if (!crdtGetDocument(doc, parentId)) {
-			throw new Error('collection_view blocks can only be created inside a Document.');
-		}
-		validateCollectionReferenceTarget(caller, referencedRecordId);
+		requireParentDocumentThenValidateTarget(
+			caller,
+			doc,
+			parentId,
+			referencedRecordId,
+			'collection',
+			'collection_view blocks can only be created inside a Document.'
+		);
 		return;
 	}
 	throw new Error(
@@ -268,17 +301,25 @@ function validateReferencedRecordIdWrite(
 	referencedRecordId: string
 ): void {
 	if (record.blockType === 'page_link') {
-		if (!crdtGetDocument(doc, record.parentId)) {
-			throw new Error('page_link blocks can only exist inside a Document.');
-		}
-		validateDocumentReferenceTarget(caller, referencedRecordId);
+		requireParentDocumentThenValidateTarget(
+			caller,
+			doc,
+			record.parentId,
+			referencedRecordId,
+			'document',
+			'page_link blocks can only exist inside a Document.'
+		);
 		return;
 	}
 	if (record.blockType === 'collection_view') {
-		if (!crdtGetDocument(doc, record.parentId)) {
-			throw new Error('collection_view blocks can only exist inside a Document.');
-		}
-		validateCollectionReferenceTarget(caller, referencedRecordId);
+		requireParentDocumentThenValidateTarget(
+			caller,
+			doc,
+			record.parentId,
+			referencedRecordId,
+			'collection',
+			'collection_view blocks can only exist inside a Document.'
+		);
 		return;
 	}
 	throw new Error(

src/lib/services/tokens.test.ts

@@ -1,5 +1,12 @@
 import { describe, expect, it } from 'vitest';
-import { createToken, revokeToken, UnknownSpaceError } from './tokens';
+import {
+	createToken,
+	revokeToken,
+	UnknownCollectionError,
+	UnknownDocumentError,
+	UnknownSpaceError
+} from './tokens';
+import { createCollection, createDocument } from './index';
 import { CURRENT_USER } from '$lib/server/current-user';
 import { createToken as createRawToken, listTokens } from '$lib/mcp/tokens';
 import { queryAuditLog } from '$lib/server/audit';
@@ -75,6 +82,74 @@ describe('service layer: createToken (#188)', () => {
 
 		expect(listTokens()).toHaveLength(before);
 	});
+
+	it('grants access to a real Document and a real Collection (issue #62)', () => {
+		const document = createDocument(CURRENT_USER, { title: 'Real Doc For Token Test' });
+		const collection = createCollection(CURRENT_USER, { title: 'Real Collection For Token Test' });
+
+		const { record } = createToken(CURRENT_USER, {
+			clientLabel: 'Reference Grant Client',
+			allowedDocumentIds: [document.id],
+			allowedCollectionIds: [collection.id],
+			allowedSpaceIds: []
+		});
+
+		expect(record.allowedDocumentIds).toEqual([document.id]);
+		expect(record.allowedCollectionIds).toEqual([collection.id]);
+	});
+
+	it('rejects a Document id that does not exist, without persisting a token (issue #62)', () => {
+		const before = listTokens().length;
+
+		expect(() =>
+			createToken(CURRENT_USER, {
+				clientLabel: 'Document Spoofer',
+				allowedDocumentIds: ['not-a-real-document-id'],
+				allowedCollectionIds: [],
+				allowedSpaceIds: []
+			})
+		).toThrow(UnknownDocumentError);
+
+		expect(listTokens()).toHaveLength(before);
+	});
+
+	it('rejects a Collection id that does not exist, without persisting a token (issue #62)', () => {
+		const before = listTokens().length;
+
+		expect(() =>
+			createToken(CURRENT_USER, {
+				clientLabel: 'Collection Spoofer',
+				allowedDocumentIds: [],
+				allowedCollectionIds: ['not-a-real-collection-id'],
+				allowedSpaceIds: []
+			})
+		).toThrow(UnknownCollectionError);
+
+		expect(listTokens()).toHaveLength(before);
+	});
+
+	it('rejects a Document id naming a real Collection, and vice versa (issue #62)', () => {
+		const document = createDocument(CURRENT_USER, { title: 'Doc Not A Collection' });
+		const collection = createCollection(CURRENT_USER, { title: 'Collection Not A Doc' });
+
+		expect(() =>
+			createToken(CURRENT_USER, {
+				clientLabel: 'Kind Mismatch A',
+				allowedDocumentIds: [collection.id],
+				allowedCollectionIds: [],
+				allowedSpaceIds: []
+			})
+		).toThrow(UnknownDocumentError);
+
+		expect(() =>
+			createToken(CURRENT_USER, {
+				clientLabel: 'Kind Mismatch B',
+				allowedDocumentIds: [],
+				allowedCollectionIds: [document.id],
+				allowedSpaceIds: []
+			})
+		).toThrow(UnknownCollectionError);
+	});
 });
 
 describe('service layer: revokeToken (#188)', () => {