Context
While implementing #46 (MCP authoring/retargeting of page_link targets), both createRecord and writeRecord in src/lib/services/records.ts ended up independently repeating the same guard sequence before calling the shared validatePageLinkTarget:
if (record.blockType !== 'page_link') {
throw new Error('... can only be written on a page_link block.');
}
if (!crdtGetDocument(doc, record.parentId)) {
throw new Error('page_link blocks can only exist inside a Document.');
}
validatePageLinkTarget(caller, input.referencedRecordId);
Only the error wording differs between the two call sites (createRecord vs writeRecord).
Update (PR #193 / issue #37): the duplication grew a second axis
PR #193 (closing #37) added MCP write-path support for collection_view blocks, whose referencedRecordId must resolve to a Collection rather than a Document. That added a third case to the exact duplication described above, and the function names have since changed:
createRecord's guard sequence is now factored into validateCreateReferencedRecordId.
writeRecord's is validateReferencedRecordIdWrite.
Both now dispatch across three block-type/target-kind combinations — page_link/child_pages → Document via validateDocumentReferenceTarget, and collection_view → Collection via validateCollectionReferenceTarget — each repeating the same "check blockType, check parent is a Document, call the kind-specific validator" shape.
There's also a second, narrower axis of duplication this issue didn't originally call out: validateDocumentReferenceTarget and validateCollectionReferenceTarget are themselves near-identical, differing only in crdtGetDocument vs crdtGetCollection and the InvalidLinkTargetError kind argument ('Document' vs 'Collection').
Update (PR #198 / issue #188): a third, permission-optional axis — token grant existence checks
While implementing #188 (routing /settings/tokens through the service layer), services/tokens.ts#createToken came out validating allowedSpaceIds against the workspace's real Spaces (a Set built from listSpaces(), inline — preserved from #141) but doing no equivalent existence check for allowedDocumentIds/allowedCollectionIds. A token can be minted naming Document/Collection ids that don't exist at all. Not a security issue (a dead grant never matches anything via tokenAllowsParent's plain membership check, and the UI's checkboxes only ever submit real ids, so this needs a hand-crafted POST to hit), but it's inconsistent with the now-validated Space case and with this issue's own subject: another call site independently deciding how to check "does this ID exist," instead of sharing an answer.
This also surfaces a reusable primitive the two referencedRecordId validators above should have been building on already: resolveInternalLinkTarget already resolves an ID to {kind: 'document' | 'collection', title} or undefined — generically, exactly what validateDocumentReferenceTarget/validateCollectionReferenceTarget each reimplement today via their own direct crdtGetDocument/crdtGetCollection call. The two validation needs differ only in what's layered on top of that same resolve step:
validateDocumentReferenceTarget/validateCollectionReferenceTarget need existence + kind + permission (tokenAllowsParent) — a caller retargeting a live block must be able to reach the target itself.
createToken's grant validation needs only existence + kind, no permission check — Phase 0 has no membership model gating who a caller may grant a future token access to.
So the eventual fix here is two layers, not one: (1) both records.ts validators call through resolveInternalLinkTarget instead of their own direct CRDT lookups, and (2) a small generic "validate every ID in a caller-supplied list against a check function, throw naming the first invalid one" helper backs both createToken's new Document/Collection-id-list check and its existing Space-id-list check (which stays on isKnownSpace/listSpaces, a separate catalog table resolveInternalLinkTarget doesn't cover — same helper shape, different existence predicate).
Proposal
Extract a shared helper (or small set of helpers) covering all three axes now in play: create-vs-write, document-vs-collection target kind, and reference-validation-with-permission vs. grant-validation-without-permission.
- A
requireValidReferenceWrite(caller, parentId, blockType, targetId)-style helper (or similar) for createRecord/writeRecord's shared guard sequence, internally selecting the right kind-specific target validator.
- Both
validateDocumentReferenceTarget/validateCollectionReferenceTarget rebuilt on top of resolveInternalLinkTarget instead of each calling crdtGetDocument/crdtGetCollection directly.
- A generic list-validation helper (
validateEvery(ids, existsFn, ErrorClass)-shaped, or similar) used by createToken for both its Document/Collection-id-list check (new) and its existing Space-id-list check.
Not urgent on its own, but the duplication has now grown in shape twice since this issue was filed (three block-type/kind combinations in records.ts, plus a structurally identical but unvalidated case in tokens.ts), and each new grant/reference surface added without this repeats the same design decision from scratch.
Files involved
src/lib/services/records.ts (createRecord, writeRecord, validateCreateReferencedRecordId, validateReferencedRecordIdWrite, validateDocumentReferenceTarget, validateCollectionReferenceTarget)
src/lib/data/links.ts (resolveInternalLinkTarget) — the existing resolver the above should build on
src/lib/services/tokens.ts (createToken) — the new call site needing an existence check it doesn't have yet
Follow-up from review of PR #61 / issue #46; scope expanded per PR #193 / issue #37, then again per PR #198 / issue #188.
Context
While implementing #46 (MCP authoring/retargeting of
page_linktargets), bothcreateRecordandwriteRecordinsrc/lib/services/records.tsended up independently repeating the same guard sequence before calling the sharedvalidatePageLinkTarget:Only the error wording differs between the two call sites (
createRecordvswriteRecord).Update (PR #193 / issue #37): the duplication grew a second axis
PR #193 (closing #37) added MCP write-path support for
collection_viewblocks, whosereferencedRecordIdmust resolve to a Collection rather than a Document. That added a third case to the exact duplication described above, and the function names have since changed:createRecord's guard sequence is now factored intovalidateCreateReferencedRecordId.writeRecord's isvalidateReferencedRecordIdWrite.Both now dispatch across three block-type/target-kind combinations —
page_link/child_pages→ Document viavalidateDocumentReferenceTarget, andcollection_view→ Collection viavalidateCollectionReferenceTarget— each repeating the same "check blockType, check parent is a Document, call the kind-specific validator" shape.There's also a second, narrower axis of duplication this issue didn't originally call out:
validateDocumentReferenceTargetandvalidateCollectionReferenceTargetare themselves near-identical, differing only incrdtGetDocumentvscrdtGetCollectionand theInvalidLinkTargetErrorkind argument ('Document'vs'Collection').Update (PR #198 / issue #188): a third, permission-optional axis — token grant existence checks
While implementing #188 (routing
/settings/tokensthrough the service layer),services/tokens.ts#createTokencame out validatingallowedSpaceIdsagainst the workspace's real Spaces (a Set built fromlistSpaces(), inline — preserved from #141) but doing no equivalent existence check forallowedDocumentIds/allowedCollectionIds. A token can be minted naming Document/Collection ids that don't exist at all. Not a security issue (a dead grant never matches anything viatokenAllowsParent's plain membership check, and the UI's checkboxes only ever submit real ids, so this needs a hand-crafted POST to hit), but it's inconsistent with the now-validated Space case and with this issue's own subject: another call site independently deciding how to check "does this ID exist," instead of sharing an answer.This also surfaces a reusable primitive the two
referencedRecordIdvalidators above should have been building on already:resolveInternalLinkTargetalready resolves an ID to{kind: 'document' | 'collection', title}orundefined— generically, exactly whatvalidateDocumentReferenceTarget/validateCollectionReferenceTargeteach reimplement today via their own directcrdtGetDocument/crdtGetCollectioncall. The two validation needs differ only in what's layered on top of that same resolve step:validateDocumentReferenceTarget/validateCollectionReferenceTargetneed existence + kind + permission (tokenAllowsParent) — a caller retargeting a live block must be able to reach the target itself.createToken's grant validation needs only existence + kind, no permission check — Phase 0 has no membership model gating who a caller may grant a future token access to.So the eventual fix here is two layers, not one: (1) both
records.tsvalidators call throughresolveInternalLinkTargetinstead of their own direct CRDT lookups, and (2) a small generic "validate every ID in a caller-supplied list against a check function, throw naming the first invalid one" helper backs bothcreateToken's new Document/Collection-id-list check and its existing Space-id-list check (which stays onisKnownSpace/listSpaces, a separate catalog tableresolveInternalLinkTargetdoesn't cover — same helper shape, different existence predicate).Proposal
Extract a shared helper (or small set of helpers) covering all three axes now in play: create-vs-write, document-vs-collection target kind, and reference-validation-with-permission vs. grant-validation-without-permission.
requireValidReferenceWrite(caller, parentId, blockType, targetId)-style helper (or similar) forcreateRecord/writeRecord's shared guard sequence, internally selecting the right kind-specific target validator.validateDocumentReferenceTarget/validateCollectionReferenceTargetrebuilt on top ofresolveInternalLinkTargetinstead of each callingcrdtGetDocument/crdtGetCollectiondirectly.validateEvery(ids, existsFn, ErrorClass)-shaped, or similar) used bycreateTokenfor both its Document/Collection-id-list check (new) and its existing Space-id-list check.Not urgent on its own, but the duplication has now grown in shape twice since this issue was filed (three block-type/kind combinations in
records.ts, plus a structurally identical but unvalidated case intokens.ts), and each new grant/reference surface added without this repeats the same design decision from scratch.Files involved
src/lib/services/records.ts(createRecord,writeRecord,validateCreateReferencedRecordId,validateReferencedRecordIdWrite,validateDocumentReferenceTarget,validateCollectionReferenceTarget)src/lib/data/links.ts(resolveInternalLinkTarget) — the existing resolver the above should build onsrc/lib/services/tokens.ts(createToken) — the new call site needing an existence check it doesn't have yetFollow-up from review of PR #61 / issue #46; scope expanded per PR #193 / issue #37, then again per PR #198 / issue #188.