From ddf2a8ff5b7e70ff68c16b7276130cd635f21311 Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:59:19 +0300 Subject: [PATCH 1/6] feat(types)!: validate what a field type actually means MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only 4 of the 27 field types had any validation beyond `typeof`: email, url, select and relation. The other 17 were indistinguishable from `string` at runtime — a `slug` field accepted "Hello World!!", a `date` field accepted "yesterday", and `integer` accepted 3.7. The types existed for the Studio UI and the emitted TS, and stopped meaning anything after that. New `validateSemanticType(value, type)`, called from validateFieldValue after the type match. It reuses what the codebase already owned rather than inventing rules: `validateSlug` (with its path-traversal check) for slug, and the same date parse `validator/schedule.ts` has always done for publish_at/expire_at. Severity follows one rule. A *definitional* check is an error — a slug that does not match SLUG_PATTERN is not a slug, an unparseable date is not a date. A *heuristic* is a warning, because the pattern is an approximation and a real value can sit outside it (international phone formats, CSS colour keywords). email/url keep the warning severity they already had; the tests pin it on purpose. `rating` is deliberately left alone — its scale is never declared, so any range would be invented. Also `validateAccept` + `isMediaType`: `accept` was declared on FieldDef and read by nothing. It is now checked against the value's file extension, at warning severity, and the message says that is what it is — the stored value is a path, so the real MIME type is only knowable at the provider's ingest. An extension we cannot map to a MIME type is not evidence of a violation, so it stays silent rather than guessing. `maxSize` is untouched here on purpose: byte length is not knowable from a path. Tests: 110 in packages/types pass; the rules are exercised through @contentrain/mcp's validateContent fixtures, where stubbing validateSemanticType to a no-op turns 10 red. --- packages/types/src/index.ts | 138 +++++++++++++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 2 deletions(-) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 61cbeba..428af1a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -564,10 +564,138 @@ function checkMinMax(value: unknown, fieldDef: FieldDef): string | null { return null } +// ─── Semantic type validation ─── + +/** Hex (#rgb/#rrggbb/#rrggbbaa), rgb()/rgba(), hsl()/hsla(), or a bare CSS keyword. */ +const COLOR_PATTERN = /^(#(?:[0-9a-fA-F]{3,4}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})|(?:rgb|hsl)a?\([^)]*\)|[a-zA-Z]+)$/ +/** Deliberately loose: digits, spaces, and the usual separators, 4-20 digits total. */ +const PHONE_PATTERN = /^\+?[\d\s().-]{4,}$/ +const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ +const DATE_PATTERN = /^\d{4}-\d{2}-\d{2}$/ + +function isParsableDate(value: string): boolean { + return !Number.isNaN(new Date(value).getTime()) +} + +/** + * Rules a field type implies beyond its runtime `typeof`. + * + * Severity follows one rule: a check that is *definitional* is an error — a `slug` + * that does not match `SLUG_PATTERN` is not a slug. A check that is a *heuristic* is + * a warning, because the pattern is an approximation and a legitimate value may sit + * outside it (international phone formats, CSS colour keywords). + * + * `rating` is deliberately absent: its scale is never declared, so any range here + * would be invented. Declare `min`/`max` on the field instead. + * + * Returns `[]` for types whose only contract is their `typeof` (`string`, `text`, + * `code`, `icon`, `markdown`, `richtext`, `number`, `decimal`, `boolean`, media). + */ +export function validateSemanticType(value: unknown, type: FieldType): ValidationError[] { + if (typeof value === 'number') { + if (type === 'integer' && !Number.isInteger(value)) { + return [{ severity: 'error', message: `Value ${value} is not an integer` }] + } + if (type === 'percent' && (value < 0 || value > 100)) { + return [{ severity: 'error', message: `Percent ${value} is outside 0-100` }] + } + return [] + } + + if (typeof value !== 'string' || value === '') return [] + + switch (type) { + case 'slug': { + const err = validateSlug(value) + return err ? [{ severity: 'error', message: err }] : [] + } + case 'date': + return DATE_PATTERN.test(value) && isParsableDate(value) + ? [] + : [{ severity: 'error', message: `Invalid date "${value}": must be YYYY-MM-DD` }] + case 'datetime': + return isParsableDate(value) + ? [] + : [{ severity: 'error', message: `Invalid datetime "${value}": must be an ISO 8601 date-time` }] + case 'email': + return EMAIL_PATTERN.test(value) + ? [] + : [{ severity: 'warning', message: `"${value}" may not be a valid email` }] + case 'url': + return /^https?:\/\/.+/.test(value) || value.startsWith('/') + ? [] + : [{ severity: 'warning', message: `"${value}" may not be a valid URL` }] + case 'color': + return COLOR_PATTERN.test(value) + ? [] + : [{ severity: 'warning', message: `"${value}" may not be a valid color (expected hex, rgb(), hsl() or a keyword)` }] + case 'phone': + return PHONE_PATTERN.test(value) && (value.match(/\d/g)?.length ?? 0) >= 4 + ? [] + : [{ severity: 'warning', message: `"${value}" may not be a valid phone number` }] + default: + return [] + } +} + +/** Media field types — the ones whose value is a path/URL to an asset. */ +const MEDIA_TYPES = new Set(['image', 'video', 'file']) + +export function isMediaType(type: FieldType): boolean { + return MEDIA_TYPES.has(type) +} + +/** Extensions we can name a MIME type for. Deliberately small — this is a sniff. */ +const EXTENSION_MIME: Record = { + jpg: 'image/jpeg', jpeg: 'image/jpeg', png: 'image/png', gif: 'image/gif', + webp: 'image/webp', avif: 'image/avif', svg: 'image/svg+xml', ico: 'image/x-icon', + mp4: 'video/mp4', webm: 'video/webm', mov: 'video/quicktime', + pdf: 'application/pdf', zip: 'application/zip', json: 'application/json', + csv: 'text/csv', txt: 'text/plain', +} + +/** + * Check a media value's file extension against an `accept` list. + * + * This is an approximation and says so: the value is a path or URL, so the real + * MIME type is only knowable where the bytes are — the provider, at ingest + * (`provider.ts:MediaProvider.ingest` owns that policy). An extension can lie, and + * an unknown extension is not evidence of a violation. Hence: warning, never error, + * and silence when we cannot tell. + * + * `accept` follows the HTML input syntax: `image/jpeg`, `image/*`, `.jpg`, comma-separated. + */ +export function validateAccept(value: string, accept: string): ValidationError[] { + const path = value.split('?')[0]?.split('#')[0] ?? value + const ext = path.split('.').pop()?.toLowerCase() + if (!ext || ext === path.toLowerCase()) return [] + + const mime = EXTENSION_MIME[ext] + const patterns = accept.split(',').map(a => a.trim().toLowerCase()).filter(Boolean) + if (patterns.length === 0) return [] + + const matches = patterns.some((p) => { + if (p.startsWith('.')) return p.slice(1) === ext + if (p.endsWith('/*')) return mime ? mime.startsWith(p.slice(0, -1)) : false + return mime === p + }) + if (matches) return [] + + // An extension we have no MIME for is not evidence of a violation. + if (!mime && !patterns.some(p => p.startsWith('.'))) return [] + + return [{ + severity: 'warning', + message: `File extension ".${ext}" does not match accept "${accept}" (extension check — the provider enforces the real MIME type at ingest)`, + }] +} + /** * Validate a field value against its FieldDef schema. - * Checks: required, type match, min/max, pattern, select options. - * Does NOT check: secrets (use detectSecrets), uniqueness (stateful), relations (I/O). + * Checks: required, type match, semantic type rules, min/max, pattern, select + * options, and `accept` on media fields (by extension). + * Does NOT check: secrets (use detectSecrets), uniqueness (stateful), relations (I/O), + * or `maxSize` — byte length is not knowable from a stored path. */ export function validateFieldValue(value: unknown, fieldDef: FieldDef): ValidationError[] { const issues: ValidationError[] = [] @@ -584,6 +712,12 @@ export function validateFieldValue(value: unknown, fieldDef: FieldDef): Validati return issues } + issues.push(...validateSemanticType(value, fieldDef.type)) + + if (fieldDef.accept && isMediaType(fieldDef.type) && typeof value === 'string') { + issues.push(...validateAccept(value, fieldDef.accept)) + } + const minMaxErr = checkMinMax(value, fieldDef) if (minMaxErr) { issues.push({ severity: 'error', message: minMaxErr }) From 8e404220f02a22527757052703346b2b6f896847 Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:59:32 +0300 Subject: [PATCH 2/6] fix(mcp)!: validate array items by the same rules as scalars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported case — `emails: ["not-an-email"]` producing zero errors — was one symptom of a parallel implementation. `validateArrayItemType` was a second type switch that knew 10 of the 27 types and checked only `typeof`, so: - items never saw min/max/pattern/options/required — only their JS kind; - `items` given as a FieldDef with a non-object type (`{type:'array', items:{type:'string', max:50}}`) matched neither the string-form branch nor the object+fields branch, so it was validated by nothing at all — while the type emitter rendered the constraint as real; - `integer` rejected 3.7 inside an array but accepted it as a scalar, because the two switches disagreed. Items now recurse through `validateField` itself, which already knows every type, relations, and nested objects. One rule set for a type wherever it appears, and the second switch is deleted. `ctx` is deliberately not passed to items: `unique` compares an entry against its siblings, which means nothing for positions inside one array. A depth cap bounds items-inside-items. Two deliberate contract changes: - Item type errors now carry validateFieldValue's message ("Type mismatch: expected string, got number") rather than "must be a string". The field path still names the index. - Nested object errors are qualified by their parent — `seo.title`, not `title`, which was ambiguous with a top-level field of the same name. The old array-of-object branch already did this; unifying spread it to plain objects too, and dropping the prefix would have been a regression the fixtures caught. email/url heuristics move to @contentrain/types with the rest of the semantic rules, so scalars and items share them. Tests: 39 in entry.test.ts. New cases cover the reported bug, a type the old switch never knew (date), the items-FieldDef black hole, options on items, and the depth cap. entry.test.ts:171,201 updated for the two changes above. --- packages/mcp/src/core/validator/entry.ts | 88 +++------ .../mcp/tests/core/validator/entry.test.ts | 177 +++++++++++++++++- 2 files changed, 203 insertions(+), 62 deletions(-) diff --git a/packages/mcp/src/core/validator/entry.ts b/packages/mcp/src/core/validator/entry.ts index 2e07940..0e8bdfb 100644 --- a/packages/mcp/src/core/validator/entry.ts +++ b/packages/mcp/src/core/validator/entry.ts @@ -51,6 +51,9 @@ export function validateContent( } } +/** Bounds `items`-inside-`items` nesting; far above any real schema. */ +const MAX_FIELD_DEPTH = 10 + function validateField( value: unknown, def: FieldDef, @@ -59,10 +62,15 @@ function validateField( entryId: string | undefined, fieldId: string, ctx?: ValidationContext, + depth = 0, ): ValidationError[] { const errors: ValidationError[] = [] const errCtx = { model: modelId, locale, entry: entryId, field: fieldId } + if (depth > MAX_FIELD_DEPTH) { + return [{ severity: 'error', ...errCtx, message: `${fieldId} exceeds the maximum nesting depth of ${MAX_FIELD_DEPTH}` }] + } + if (value !== null && value !== undefined && value !== '') { const secretErrors = detectSecrets(value) for (const e of secretErrors) { @@ -102,13 +110,6 @@ function validateField( } } - if (def.type === 'email' && typeof value === 'string' && !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) { - errors.push({ severity: 'warning', ...errCtx, message: `${fieldId} may not be a valid email` }) - } - if (def.type === 'url' && typeof value === 'string' && !/^https?:\/\/.+/.test(value) && !value.startsWith('/')) { - errors.push({ severity: 'warning', ...errCtx, message: `${fieldId} may not be a valid URL` }) - } - if (def.type === 'relation' && def.model) { const targets = Array.isArray(def.model) ? def.model : [def.model] if (targets.length > 1) { @@ -168,66 +169,33 @@ function validateField( } } - if (def.type === 'array' && Array.isArray(value) && def.items && typeof def.items === 'string') { + // Array items run through `validateField` itself rather than a parallel type + // switch. The old switch knew 10 of the 27 field types and checked only + // `typeof`, so `min`/`max`/`pattern`/`options` never reached an item and an + // `items` given as a FieldDef with a non-object type matched no branch at all — + // silently unvalidated, while the type emitter rendered it as a real constraint. + // Recursing keeps one rule set for a type wherever it appears. + if (def.type === 'array' && Array.isArray(value) && def.items) { + const itemDef: FieldDef = typeof def.items === 'string' + ? { type: def.items as FieldDef['type'] } + : def.items for (let i = 0; i < value.length; i++) { - errors.push(...validateArrayItemType(value[i], def.items, errCtx, `${fieldId}[${i}]`)) + // `ctx` is deliberately dropped: `unique` compares an entry against its + // siblings, which has no meaning for positions inside one array. + errors.push(...validateField( + value[i], itemDef, modelId, locale, entryId, `${fieldId}[${i}]`, undefined, depth + 1, + )) } } if (def.type === 'object' && def.fields && typeof value === 'object' && value !== null && !Array.isArray(value)) { - const nested = validateContent(value as Record, def.fields, modelId, locale, entryId) - errors.push(...nested.errors) - } - - if ( - def.type === 'array' - && Array.isArray(value) - && def.items - && typeof def.items === 'object' - && def.items.type === 'object' - && def.items.fields - ) { - for (let i = 0; i < value.length; i++) { - if (typeof value[i] === 'object' && value[i] !== null) { - const nested = validateContent(value[i] as Record, def.items.fields, modelId, locale, entryId) - for (const e of nested.errors) { - errors.push({ ...e, field: `${fieldId}[${i}].${e.field}` }) - } - } + const nested = validateContent(value as Record, def.fields, modelId, locale, entryId, ctx) + // Qualify the nested path with the parent so an error names where it lives — + // `meta.title`, or `blocks[0].title` when this object is an array item. + for (const e of nested.errors) { + errors.push({ ...e, field: e.field ? `${fieldId}.${e.field}` : fieldId }) } } return errors } - -function validateArrayItemType( - value: unknown, - itemType: string, - errCtx: { model: string, locale: string, entry: string | undefined, field: string }, - fieldPath: string, -): ValidationError[] { - const ctx = { ...errCtx, field: fieldPath } - switch (itemType) { - case 'string': - case 'email': - case 'url': - case 'slug': - case 'image': - case 'video': - case 'file': - if (typeof value !== 'string') return [{ severity: 'error', ...ctx, message: `${fieldPath} must be a string` }] - break - case 'number': - case 'integer': - case 'decimal': - if (typeof value !== 'number') return [{ severity: 'error', ...ctx, message: `${fieldPath} must be a number` }] - if (itemType === 'integer' && !Number.isInteger(value)) { - return [{ severity: 'error', ...ctx, message: `${fieldPath} must be an integer` }] - } - break - case 'boolean': - if (typeof value !== 'boolean') return [{ severity: 'error', ...ctx, message: `${fieldPath} must be a boolean` }] - break - } - return [] -} diff --git a/packages/mcp/tests/core/validator/entry.test.ts b/packages/mcp/tests/core/validator/entry.test.ts index a16ccc8..3a752b1 100644 --- a/packages/mcp/tests/core/validator/entry.test.ts +++ b/packages/mcp/tests/core/validator/entry.test.ts @@ -176,7 +176,9 @@ describe('validateContent', () => { keywords: { type: 'array', items: 'string' }, } const result = validateContent({ keywords: ['ok', 42] }, fields, 'blog', 'en', 'abc') - expect(result.errors.some(e => e.field === 'keywords[1]' && /must be a string/.test(e.message ?? ''))).toBe(true) + // Items now share the scalar rule set, so the message is validateFieldValue's. + // The path still names the offending index. + expect(result.errors.some(e => e.field === 'keywords[1]' && /expected string/.test(e.message ?? ''))).toBe(true) }) it('array of integers rejects float', () => { @@ -187,6 +189,175 @@ describe('validateContent', () => { expect(result.errors.some(e => e.field === 'counts[1]' && /integer/.test(e.message ?? ''))).toBe(true) }) + // Array items used to run through a parallel type switch that knew 10 of the 27 + // field types and checked only `typeof`. They now share the scalar rule set. + describe('array items share the scalar rule set', () => { + it('applies the email heuristic to items, not just scalars', () => { + // The reported case: items:'email' accepted anything string-shaped. + const fields: Record = { + emails: { type: 'array', items: 'email' }, + } + const result = validateContent({ emails: ['not-an-email'] }, fields, 'blog', 'en', 'abc') + expect(result.errors.some(e => e.field === 'emails[0]' && e.severity === 'warning')).toBe(true) + }) + + it('validates a type the old item switch never knew', () => { + const fields: Record = { + published: { type: 'array', items: 'date' }, + } + const result = validateContent({ published: ['2026-07-15', 'yesterday'] }, fields, 'blog', 'en', 'abc') + expect(result.errors.some(e => e.field === 'published[1]' && e.severity === 'error')).toBe(true) + expect(result.errors.some(e => e.field === 'published[0]')).toBe(false) + }) + + it('enforces constraints on an items FieldDef — the old black hole', () => { + // `items` as a FieldDef with a non-object type matched no branch at all. + const fields: Record = { + codes: { type: 'array', items: { type: 'string', max: 3, pattern: '^[a-z]+$' } }, + } + const result = validateContent({ codes: ['ok', 'toolong', 'AB'] }, fields, 'blog', 'en', 'abc') + expect(result.errors.some(e => e.field === 'codes[1]' && /maximum/.test(e.message ?? ''))).toBe(true) + expect(result.errors.some(e => e.field === 'codes[2]' && /pattern/.test(e.message ?? ''))).toBe(true) + expect(result.errors.some(e => e.field === 'codes[0]')).toBe(false) + }) + + it('enforces select options on items', () => { + const fields: Record = { + sizes: { type: 'array', items: { type: 'select', options: ['s', 'm', 'l'] } }, + } + const result = validateContent({ sizes: ['m', 'xxl'] }, fields, 'blog', 'en', 'abc') + expect(result.errors.some(e => e.field === 'sizes[1]' && e.severity === 'error')).toBe(true) + }) + + it('bounds runaway nesting instead of blowing the stack', () => { + let items: FieldDef = { type: 'string' } + for (let i = 0; i < 15; i++) items = { type: 'array', items } + let value: unknown = 'x' + for (let i = 0; i < 15; i++) value = [value] + + const result = validateContent({ deep: value }, { deep: items }, 'blog', 'en', 'abc') + expect(result.errors.some(e => /nesting depth/.test(e.message ?? ''))).toBe(true) + }) + }) + + // 17 of 27 types were pure typeof checks. Mechanical rules are errors; heuristics + // are warnings, because a legitimate value can sit outside an approximate pattern. + describe('semantic type rules', () => { + it('rejects a slug field that is not a slug', () => { + // Every shipped template declares slug: { type: 'slug', ... }. + const result = validateContent( + { slug: 'Hello World!!' }, { slug: { type: 'slug' } }, 'blog', 'en', 'abc', + ) + expect(result.errors.some(e => e.field === 'slug' && e.severity === 'error')).toBe(true) + expect(result.valid).toBe(false) + }) + + it('accepts a well-formed slug', () => { + const result = validateContent( + { slug: 'hello-world-2' }, { slug: { type: 'slug' } }, 'blog', 'en', 'abc', + ) + expect(result.errors).toEqual([]) + }) + + it('rejects a float in an integer scalar — matching the array-item rule', () => { + // The scalar path lumped integer with number, so 3.7 passed here while the + // identical value was rejected inside an array. + const result = validateContent( + { count: 3.7 }, { count: { type: 'integer' } }, 'blog', 'en', 'abc', + ) + expect(result.errors.some(e => e.field === 'count' && e.severity === 'error')).toBe(true) + }) + + it('rejects unparseable date and datetime', () => { + const fields: Record = { + on: { type: 'date' }, + at: { type: 'datetime' }, + } + const result = validateContent({ on: '15/07/2026', at: 'not-a-time' }, fields, 'blog', 'en', 'abc') + expect(result.errors.some(e => e.field === 'on' && e.severity === 'error')).toBe(true) + expect(result.errors.some(e => e.field === 'at' && e.severity === 'error')).toBe(true) + }) + + it('accepts valid date and datetime', () => { + const fields: Record = { + on: { type: 'date' }, + at: { type: 'datetime' }, + } + const result = validateContent( + { on: '2026-07-15', at: '2026-07-15T14:08:34.746Z' }, fields, 'blog', 'en', 'abc', + ) + expect(result.errors).toEqual([]) + }) + + it('rejects a percent outside 0-100', () => { + const result = validateContent( + { pct: 140 }, { pct: { type: 'percent' } }, 'blog', 'en', 'abc', + ) + expect(result.errors.some(e => e.field === 'pct' && e.severity === 'error')).toBe(true) + }) + + it('warns — never errors — on a heuristic colour check', () => { + const result = validateContent( + { brand: 'not a colour' }, { brand: { type: 'color' } }, 'blog', 'en', 'abc', + ) + expect(result.errors.some(e => e.field === 'brand' && e.severity === 'warning')).toBe(true) + // A heuristic must not fail the entry. + expect(result.valid).toBe(true) + }) + + it('accepts the colour forms an author actually writes', () => { + const fields: Record = { + a: { type: 'color' }, b: { type: 'color' }, c: { type: 'color' }, d: { type: 'color' }, + } + const result = validateContent( + { a: '#fff', b: '#1a2b3c', c: 'rgb(1, 2, 3)', d: 'rebeccapurple' }, fields, 'blog', 'en', 'abc', + ) + expect(result.errors).toEqual([]) + }) + + it('leaves rating alone — its scale is never declared', () => { + const result = validateContent( + { stars: 42 }, { stars: { type: 'rating' } }, 'blog', 'en', 'abc', + ) + expect(result.errors).toEqual([]) + }) + }) + + describe('accept on media fields', () => { + it('warns when the extension contradicts accept, and says it is a sniff', () => { + // The reported case: accept:"image/jpeg" with a .webp value. + const fields: Record = { + cover: { type: 'image', accept: 'image/jpeg' }, + } + const result = validateContent({ cover: 'media/original/a.webp' }, fields, 'blog', 'en', 'abc') + const issue = result.errors.find(e => e.field === 'cover') + expect(issue?.severity).toBe('warning') + expect(issue?.message).toContain('extension check') + expect(result.valid).toBe(true) + }) + + it('accepts a matching extension and a wildcard', () => { + const fields: Record = { + a: { type: 'image', accept: 'image/jpeg' }, + b: { type: 'image', accept: 'image/*' }, + c: { type: 'file', accept: '.pdf' }, + } + const result = validateContent( + { a: 'media/x.jpg', b: 'media/y.webp', c: 'media/z.pdf' }, fields, 'blog', 'en', 'abc', + ) + expect(result.errors).toEqual([]) + }) + + it('stays silent on an extension it cannot map to a MIME type', () => { + // An unknown extension is not evidence of a violation. + const fields: Record = { + cover: { type: 'image', accept: 'image/jpeg' }, + } + const result = validateContent({ cover: 'media/original/a.heic' }, fields, 'blog', 'en', 'abc') + expect(result.errors).toEqual([]) + }) + }) + it('nested object validation recurses into fields', () => { const fields: Record = { seo: { @@ -198,7 +369,9 @@ describe('validateContent', () => { } const result = validateContent({ seo: {} }, fields, 'blog', 'en', 'abc') expect(result.valid).toBe(false) - expect(result.errors.some(e => e.field === 'title' && e.severity === 'error')).toBe(true) + // The path is qualified by its parent — a bare `title` would be ambiguous with + // a top-level field of the same name. + expect(result.errors.some(e => e.field === 'seo.title' && e.severity === 'error')).toBe(true) }) it('array-of-object validation prefixes field path with index', () => { From 05d3360f78bdeb1447179307105baac1c7d90c45 Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:59:53 +0300 Subject: [PATCH 3/6] fix(mcp): make unique work on document models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unique` is gated on a validation context carrying the entry's siblings, and only the collection validator ever passed one. Documents were validated one file at a time, so the check silently did nothing — which is exactly where it is most often declared: every shipped template writes `slug: { type: 'slug', required: true, unique: true }` on a document model (templates/blog.ts, docs.ts, ecommerce.ts, saas.ts). validateDocumentModel now reads each document once up front, keeps the frontmatter per locale, and validates with `{ allEntries, currentEntryId: slug }`. The pre-pass replaces the read inside the loop rather than adding one, so a remote provider still pays one fetch per file. Uniqueness stays scoped to a locale: the same value in en and tr is a translation, not a collision. Nested objects now receive `ctx` too (entry.ts recursion dropped it), so `unique` inside `object.fields` is checked rather than quietly skipped. Singletons are handled in the model_save commit that follows: a singleton holds one record per locale, so `unique` has nothing to compare against and is rejected at schema level rather than given a fake implementation here. Tests: tests/core/validator/unique.test.ts (4) — duplicate detection, distinct values, self-comparison, and locale scoping. Removing the ctx turns the first red. --- packages/mcp/src/core/validator/project.ts | 24 +++- .../mcp/tests/core/validator/unique.test.ts | 115 ++++++++++++++++++ 2 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 packages/mcp/tests/core/validator/unique.test.ts diff --git a/packages/mcp/src/core/validator/project.ts b/packages/mcp/src/core/validator/project.ts index 62bdfe0..6346980 100644 --- a/packages/mcp/src/core/validator/project.ts +++ b/packages/mcp/src/core/validator/project.ts @@ -672,12 +672,30 @@ async function validateDocumentModel( // per-locale orphan/parity warnings. Iterate just the default locale. #8/Y3 const locales = model.i18n ? config.locales.supported : [config.locales.default] + // Read every document once up front and keep its frontmatter per locale. + // `unique` compares an entry against its siblings, so it needs the whole set — + // validating one file at a time is why `unique` was silently a no-op for + // documents, which is exactly where every shipped template declares it. + const rawByKey = new Map() + const frontmatterByLocale: Record>> = {} + for (const slug of slugs) { + if (slug.startsWith('.')) continue + for (const locale of locales) { + const raw = await readTextViaReader(reader, documentFilePath(model, locale, slug)) + if (!raw) continue + rawByKey.set(`${slug}${locale}`, raw) + const { frontmatter } = parseFrontmatter(raw) + frontmatterByLocale[locale] ??= {} + frontmatterByLocale[locale][slug] = frontmatter + } + } + for (const slug of slugs) { if (slug.startsWith('.')) continue for (const locale of locales) { const filePath = documentFilePath(model, locale, slug) - const raw = await readTextViaReader(reader, filePath) + const raw = rawByKey.get(`${slug}${locale}`) ?? null if (!raw) { if (model.i18n) { @@ -731,6 +749,10 @@ async function validateDocumentModel( fieldsWithoutBody, model.id, locale, + undefined, + // Siblings in the same locale, so `unique` has something to compare + // against. Keyed by slug — a document's identity is its slug. + { allEntries: frontmatterByLocale[locale] ?? {}, currentEntryId: slug }, ) for (const err of entryResult.errors) { issues.push({ ...err, slug }) diff --git a/packages/mcp/tests/core/validator/unique.test.ts b/packages/mcp/tests/core/validator/unique.test.ts new file mode 100644 index 0000000..fd4a8b5 --- /dev/null +++ b/packages/mcp/tests/core/validator/unique.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from 'vitest' +import { validateProject } from '../../../src/core/validator/index.js' +import type { RepoReader } from '../../../src/core/contracts/index.js' + +/** + * `unique` was gated on a validation context that only the collection validator + * ever passed, so it was silently a no-op for documents — which is exactly where + * every shipped template declares it (`templates/blog.ts`, `docs.ts`, + * `ecommerce.ts`, `saas.ts` all carry `slug: { type: 'slug', unique: true }`). + */ + +function makeInMemoryReader(files: Record): RepoReader { + return { + async readFile(path) { + const content = files[path] + if (content === undefined) throw new Error(`File not found: ${path}`) + return content + }, + async listDirectory(path) { + const prefix = path.endsWith('/') ? path : `${path}/` + const children = new Set() + for (const filePath of Object.keys(files)) { + if (!filePath.startsWith(prefix)) continue + const rest = filePath.slice(prefix.length) + const firstSegment = rest.split('/')[0] + if (firstSegment) children.add(firstSegment) + } + return [...children].toSorted() + }, + async fileExists(path) { + if (Object.hasOwn(files, path)) return true + const prefix = path.endsWith('/') ? path : `${path}/` + return Object.keys(files).some(f => f.startsWith(prefix)) + }, + } +} + +const CONFIG = JSON.stringify({ + version: 1, + stack: 'nuxt', + workflow: 'review', + locales: { default: 'en', supported: ['en'] }, + domains: ['blog'], +}) + +const DOC_MODEL = JSON.stringify({ + id: 'posts', name: 'Posts', kind: 'document', domain: 'blog', i18n: true, + fields: { + title: { type: 'string', required: true }, + sku: { type: 'string', unique: true }, + }, +}) + +const doc = (title: string, sku: string) => `---\ntitle: ${title}\nsku: ${sku}\n---\n\nBody.\n` + +describe('unique on document models', () => { + it('flags a duplicate value across two documents', () => { + const reader = makeInMemoryReader({ + '.contentrain/config.json': CONFIG, + '.contentrain/models/posts.json': DOC_MODEL, + '.contentrain/content/blog/posts/first/en.md': doc('First', 'ABC-1'), + '.contentrain/content/blog/posts/second/en.md': doc('Second', 'ABC-1'), + }) + + return validateProject(reader).then((result) => { + const dupes = result.issues.filter(i => /must be unique/.test(i.message)) + expect(dupes.length).toBeGreaterThan(0) + expect(dupes[0]!.severity).toBe('error') + expect(dupes.some(i => i.field === 'sku')).toBe(true) + expect(result.valid).toBe(false) + }) + }) + + it('stays quiet when every document carries a distinct value', async () => { + const reader = makeInMemoryReader({ + '.contentrain/config.json': CONFIG, + '.contentrain/models/posts.json': DOC_MODEL, + '.contentrain/content/blog/posts/first/en.md': doc('First', 'ABC-1'), + '.contentrain/content/blog/posts/second/en.md': doc('Second', 'ABC-2'), + }) + + const result = await validateProject(reader) + expect(result.issues.filter(i => /must be unique/.test(i.message))).toEqual([]) + }) + + it('does not compare a document against itself', async () => { + const reader = makeInMemoryReader({ + '.contentrain/config.json': CONFIG, + '.contentrain/models/posts.json': DOC_MODEL, + '.contentrain/content/blog/posts/only/en.md': doc('Only', 'ABC-1'), + }) + + const result = await validateProject(reader) + expect(result.issues.filter(i => /must be unique/.test(i.message))).toEqual([]) + }) + + it('scopes uniqueness to one locale', async () => { + // The same sku in en and tr is a translation, not a collision. + const reader = makeInMemoryReader({ + '.contentrain/config.json': JSON.stringify({ + version: 1, + stack: 'nuxt', + workflow: 'review', + locales: { default: 'en', supported: ['en', 'tr'] }, + domains: ['blog'], + }), + '.contentrain/models/posts.json': DOC_MODEL, + '.contentrain/content/blog/posts/first/en.md': doc('First', 'ABC-1'), + '.contentrain/content/blog/posts/first/tr.md': doc('Birinci', 'ABC-1'), + }) + + const result = await validateProject(reader) + expect(result.issues.filter(i => /must be unique/.test(i.message))).toEqual([]) + }) +}) From 936120aaefd4092d6683e221ca1978a739549e7e Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:59:53 +0300 Subject: [PATCH 4/6] feat(mcp)!: reject model constraints that cannot be enforced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `accept`, `maxSize` and `default` were declared on FieldDef, accepted by the zod schema, stored in the model — and read by nothing. `options` on a non-select was accepted and then ignored at validation time, because the refine only checked select ⇒ options and never the reverse. A constraint that silently does nothing is worse than no constraint: the author stops looking. model_save now rejects a property declared where it cannot apply — `options` off select, `items` off array, `fields` off object, `accept`/`maxSize` off media, `unique` on a singleton (one record per locale, so nothing to compare against). Plus `min > max`, and a `pattern` that does not compile: left to validation time that degraded to a per-entry warning and silently disabled the rule. The field schema is now `.strict()`. z.object *strips* unknown keys by default, so a typo'd `requird: true` vanished without a word and the field quietly lost the constraint its author thought they had declared. Nested `fields`/`items` are validated recursively. They were typed `z.unknown()` and the loop was top-level only, so a nested invalid type, a nested select without options, or a nested bad field name all sailed through. `default` is checked for coherence — right type for its field, and within its own `options`. It is not written into content: supplying values is the agent's job, and quietly materialising them would be MCP making a content decision. Two things MCP cannot enforce now say so instead of hiding, via a new warnings channel (`validateModelDefinition` returns `{errors, warnings}`; model_save surfaces `schema_warnings`): - `maxSize` — MCP stores a path and never sees the bytes. The provider enforces it at ingest; provider.ts already assigns it that duty. - `max` on a media field — it limits the *path string length*, not the file, which is almost certainly not what the author meant. Tests: tests/core/model-definition.test.ts (23) — validateModelDefinition had no tests at all. Templates and the model tool still pass unchanged. --- packages/mcp/src/core/apply-manager.ts | 4 +- packages/mcp/src/core/model-manager.ts | 203 +++++++++++++++--- packages/mcp/src/tools/model.ts | 10 +- .../mcp/tests/core/model-definition.test.ts | 163 ++++++++++++++ 4 files changed, 352 insertions(+), 28 deletions(-) create mode 100644 packages/mcp/tests/core/model-definition.test.ts diff --git a/packages/mcp/src/core/apply-manager.ts b/packages/mcp/src/core/apply-manager.ts index a1bc1fe..693b064 100644 --- a/packages/mcp/src/core/apply-manager.ts +++ b/packages/mcp/src/core/apply-manager.ts @@ -395,8 +395,8 @@ export async function applyExtract( kind: ext.kind, fields: ext.fields as Record | undefined, }) - if (modelErrors.length > 0) { - validationErrors.push(...modelErrors.map(e => `[${ext.model}] ${e}`)) + if (modelErrors.errors.length > 0) { + validationErrors.push(...modelErrors.errors.map(e => `[${ext.model}] ${e}`)) } for (const entry of ext.entries) { diff --git a/packages/mcp/src/core/model-manager.ts b/packages/mcp/src/core/model-manager.ts index d6d754f..b4d780a 100644 --- a/packages/mcp/src/core/model-manager.ts +++ b/packages/mcp/src/core/model-manager.ts @@ -450,6 +450,10 @@ export const FIELD_TYPE_ENUM = [ /** * Shared Zod schema for field definitions. * Used by both model_save and normalize extract for full parity. + * + * `.strict()` is load-bearing: the default `z.object` *strips* unknown keys, so a + * typo'd constraint (`requird: true`) used to vanish without a word and the field + * silently lost the rule its author thought they had declared. */ export const fieldDefZodSchema: z.ZodType> = z.record(z.string(), z.object({ type: z.enum(FIELD_TYPE_ENUM).describe('Field type from the 27-type catalog'), @@ -466,7 +470,7 @@ export const fieldDefZodSchema: z.ZodType> = z.record(z. accept: z.string().optional(), maxSize: z.number().optional(), description: z.string().optional(), -}).refine( +}).strict().refine( (f) => { if ((f.type === 'relation' || f.type === 'relations') && !f.model) return false if (f.type === 'select' && (!f.options || f.options.length === 0)) return false @@ -479,13 +483,182 @@ export const fieldDefZodSchema: z.ZodType> = z.record(z. const VALID_FIELD_TYPES = new Set(FIELD_TYPE_ENUM) +/** Field types whose value is a path/URL to a media asset. */ +const MEDIA_FIELD_TYPES = new Set(['image', 'video', 'file']) +/** Bounds `items`/`fields` nesting; far above any real schema. */ +const MAX_SCHEMA_DEPTH = 10 + +export interface ModelDefinitionIssues { + /** Block the write. */ + errors: string[] + /** Surface to the caller; the write proceeds. */ + warnings: string[] +} + +interface RawFieldDef { + type?: string + required?: unknown + unique?: unknown + default?: unknown + min?: unknown + max?: unknown + pattern?: unknown + options?: unknown + model?: unknown + items?: unknown + fields?: unknown + accept?: unknown + maxSize?: unknown +} + +/** + * Check one field definition, recursing into `fields` and `items`. + * + * The governing rule: **do not accept a constraint that will not be enforced.** + * A constraint that silently does nothing is worse than no constraint, because + * the author stops looking. So a property declared where it cannot apply is an + * error, and a property we genuinely cannot enforce says so out loud. + */ +function checkFieldDef( + raw: unknown, + path: string, + modelKind: string, + errors: string[], + warnings: string[], + depth: number, +): void { + if (typeof raw !== 'object' || raw === null) { + errors.push(`Field "${path}": must be an object`) + return + } + if (depth > MAX_SCHEMA_DEPTH) { + errors.push(`Field "${path}": exceeds the maximum nesting depth of ${MAX_SCHEMA_DEPTH}`) + return + } + + const def = raw as RawFieldDef + const type = def.type + + if (!type || !VALID_FIELD_TYPES.has(type)) { + errors.push(`Field "${path}": invalid type "${type}"`) + return + } + + if ((type === 'relation' || type === 'relations') && !def.model) { + errors.push(`Field "${path}": ${type} type requires "model" property`) + } + if (type === 'select' && (!Array.isArray(def.options) || def.options.length === 0)) { + errors.push(`Field "${path}": select type requires non-empty "options" array`) + } + // The reverse direction was never checked, so `options` on a string field was + // accepted and then silently ignored at validation time. + if (def.options !== undefined && type !== 'select') { + errors.push(`Field "${path}": "options" only applies to select fields — it is ignored on "${type}"`) + } + if (def.items !== undefined && type !== 'array') { + errors.push(`Field "${path}": "items" only applies to array fields — it is ignored on "${type}"`) + } + if (def.fields !== undefined && type !== 'object') { + errors.push(`Field "${path}": "fields" only applies to object fields — it is ignored on "${type}"`) + } + if (def.accept !== undefined && !MEDIA_FIELD_TYPES.has(type)) { + errors.push(`Field "${path}": "accept" only applies to image/video/file fields — it is ignored on "${type}"`) + } + if (def.maxSize !== undefined && !MEDIA_FIELD_TYPES.has(type)) { + errors.push(`Field "${path}": "maxSize" only applies to image/video/file fields — it is ignored on "${type}"`) + } + + // A singleton holds one record per locale, so there is nothing for a value to + // be unique against. + if (def.unique === true && modelKind === 'singleton') { + errors.push(`Field "${path}": "unique" has no meaning on a singleton — the model holds a single record per locale`) + } + + if (typeof def.min === 'number' && typeof def.max === 'number' && def.min > def.max) { + errors.push(`Field "${path}": min (${def.min}) is greater than max (${def.max})`) + } + + // Compile the regex here rather than let it fail once per entry at validation + // time, where it degrades to a warning and silently disables the constraint. + if (typeof def.pattern === 'string') { + try { + // eslint-disable-next-line no-new + new RegExp(def.pattern) + } catch { + errors.push(`Field "${path}": "pattern" is not a valid regular expression — /${def.pattern}/`) + } + } + + if (def.default !== undefined) { + checkDefaultCoherence(def, type, path, errors) + } + + // `max` on a media field measures the length of the stored path string, not + // the file — almost certainly not what the author meant. + if (typeof def.max === 'number' && MEDIA_FIELD_TYPES.has(type)) { + warnings.push( + `Field "${path}": "max" on a ${type} field limits the length of the stored path string, not the file size. Use "maxSize" for bytes.`, + ) + } + + // Said plainly rather than accepted in silence: MCP holds a path, never the + // bytes, so it cannot check this. The provider owns the policy at ingest. + if (def.maxSize !== undefined && MEDIA_FIELD_TYPES.has(type)) { + warnings.push( + `Field "${path}": "maxSize" is not enforced by MCP — it has no access to the file. Your media provider enforces it when the asset is ingested.`, + ) + } + + if (def.fields !== undefined && typeof def.fields === 'object' && def.fields !== null) { + for (const [nested, nestedDef] of Object.entries(def.fields as Record)) { + if (!/^[a-z][a-z0-9_]*$/.test(nested)) { + errors.push(`Field "${path}.${nested}": invalid name — must be snake_case starting with letter`) + } + checkFieldDef(nestedDef, `${path}.${nested}`, modelKind, errors, warnings, depth + 1) + } + } + + if (typeof def.items === 'string') { + if (!VALID_FIELD_TYPES.has(def.items)) { + errors.push(`Field "${path}.items": invalid type "${def.items}"`) + } + } else if (def.items !== undefined) { + checkFieldDef(def.items, `${path}.items`, modelKind, errors, warnings, depth + 1) + } +} + +/** A default that its own field would reject is a schema bug, not a content one. */ +function checkDefaultCoherence(def: RawFieldDef, type: string, path: string, errors: string[]): void { + const value = def.default + const isString = typeof value === 'string' + const isNumber = typeof value === 'number' + + if (type === 'select' && Array.isArray(def.options) && isString && !def.options.includes(value)) { + errors.push(`Field "${path}": default "${value}" is not one of its own options [${(def.options as string[]).join(', ')}]`) + return + } + const wantsNumber = ['number', 'integer', 'decimal', 'percent', 'rating'].includes(type) + const wantsBoolean = type === 'boolean' + const wantsArray = type === 'array' + + if (wantsNumber && !isNumber) { + errors.push(`Field "${path}": default must be a number for type "${type}"`) + } else if (wantsBoolean && typeof value !== 'boolean') { + errors.push(`Field "${path}": default must be a boolean for type "${type}"`) + } else if (wantsArray && !Array.isArray(value)) { + errors.push(`Field "${path}": default must be an array for type "${type}"`) + } +} + /** * Validate a model definition before writing. - * Returns array of error messages (empty = valid). - * Used by both model_save tool and normalize extract. + * Used by both the model_save tool and normalize extract. */ -export function validateModelDefinition(input: { id: string; kind: string; fields?: Record }): string[] { +export function validateModelDefinition( + input: { id: string; kind: string; fields?: Record }, +): ModelDefinitionIssues { const errors: string[] = [] + const warnings: string[] = [] // ID format: kebab-case if (!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(input.id)) { @@ -497,32 +670,14 @@ export function validateModelDefinition(input: { id: string; kind: string; field errors.push('Dictionary models cannot have fields. Dictionaries store flat key-value pairs.') } - // Fields validation if (input.fields) { for (const [fieldName, fieldDef] of Object.entries(input.fields)) { - const def = fieldDef as { type?: string; model?: unknown; options?: unknown } - - // Field name format if (!/^[a-z][a-z0-9_]*$/.test(fieldName)) { errors.push(`Field "${fieldName}": invalid name — must be snake_case starting with letter`) } - - // Type check - if (!def.type || !VALID_FIELD_TYPES.has(def.type)) { - errors.push(`Field "${fieldName}": invalid type "${def.type}"`) - } - - // Relation requires model - if ((def.type === 'relation' || def.type === 'relations') && !def.model) { - errors.push(`Field "${fieldName}": ${def.type} type requires "model" property`) - } - - // Select requires options - if (def.type === 'select' && (!def.options || !Array.isArray(def.options) || def.options.length === 0)) { - errors.push(`Field "${fieldName}": select type requires non-empty "options" array`) - } + checkFieldDef(fieldDef, fieldName, input.kind, errors, warnings, 0) } } - return errors + return { errors, warnings } } diff --git a/packages/mcp/src/tools/model.ts b/packages/mcp/src/tools/model.ts index 54e9b69..ee223c2 100644 --- a/packages/mcp/src/tools/model.ts +++ b/packages/mcp/src/tools/model.ts @@ -47,7 +47,7 @@ export function registerModelTools( } } - const errors = validateModel(input) + const { errors, warnings: schemaWarnings } = validateModel(input) if (errors.length > 0) { return { content: [{ type: 'text' as const, text: JSON.stringify({ error: 'Validation failed', details: errors }) }], @@ -148,12 +148,18 @@ export function registerModelTools( action, model: input.id, validation: { valid: true, errors: [] }, + // Constraints the schema accepts but MCP does not enforce, said out loud + // rather than left for the author to discover in production. + ...(schemaWarnings.length > 0 ? { schema_warnings: schemaWarnings } : {}), git: { branch, action: workflowAction, commit: commitSha, ...(sync ? { sync } : {}) }, context_updated: true, content_path: contentPath + '/', ...(displayPath ? { example_file: displayPath } : {}), ...(importSnippet ? { import_snippet: importSnippet } : {}), - next_steps: ['Add content with contentrain_content_save'], + next_steps: [ + ...(schemaWarnings.length > 0 ? ['REVIEW: see schema_warnings — some declared constraints are not enforced by MCP'] : []), + 'Add content with contentrain_content_save', + ], }, null, 2) }], } }, diff --git a/packages/mcp/tests/core/model-definition.test.ts b/packages/mcp/tests/core/model-definition.test.ts new file mode 100644 index 0000000..eecf084 --- /dev/null +++ b/packages/mcp/tests/core/model-definition.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, it } from 'vitest' +import { validateModelDefinition } from '../../src/core/model-manager.js' + +/** + * Schema-shape rules for model_save. + * + * The governing principle: do not accept a constraint that will not be enforced. + * `accept`, `maxSize` and `default` were declared, stored, and read by nothing; + * `options` on a non-select was accepted and silently ignored. A constraint that + * does nothing is worse than no constraint — the author stops looking. + */ + +const model = (fields: Record, kind = 'collection') => + validateModelDefinition({ id: 'posts', kind, fields }) + +describe('validateModelDefinition', () => { + it('accepts a well-formed model', () => { + const { errors, warnings } = model({ + title: { type: 'string', required: true, max: 120 }, + slug: { type: 'slug', required: true, unique: true }, + status: { type: 'select', options: ['draft', 'live'], default: 'draft' }, + }) + expect(errors).toEqual([]) + expect(warnings).toEqual([]) + }) + + describe('constraints declared where they cannot apply', () => { + it('rejects options on a non-select field', () => { + const { errors } = model({ title: { type: 'string', options: ['a', 'b'] } }) + expect(errors.some(e => /"options" only applies to select/.test(e))).toBe(true) + }) + + it('rejects items on a non-array field', () => { + const { errors } = model({ title: { type: 'string', items: 'string' } }) + expect(errors.some(e => /"items" only applies to array/.test(e))).toBe(true) + }) + + it('rejects fields on a non-object field', () => { + const { errors } = model({ title: { type: 'string', fields: { a: { type: 'string' } } } }) + expect(errors.some(e => /"fields" only applies to object/.test(e))).toBe(true) + }) + + it('rejects accept and maxSize on a non-media field', () => { + const { errors } = model({ title: { type: 'string', accept: 'image/*', maxSize: 100 } }) + expect(errors.some(e => /"accept" only applies to image\/video\/file/.test(e))).toBe(true) + expect(errors.some(e => /"maxSize" only applies to image\/video\/file/.test(e))).toBe(true) + }) + + it('rejects unique on a singleton — there is nothing to compare against', () => { + const { errors } = model({ title: { type: 'string', unique: true } }, 'singleton') + expect(errors.some(e => /"unique" has no meaning on a singleton/.test(e))).toBe(true) + }) + + it('allows unique on a collection and a document', () => { + expect(model({ sku: { type: 'string', unique: true } }, 'collection').errors).toEqual([]) + expect(model({ sku: { type: 'string', unique: true } }, 'document').errors).toEqual([]) + }) + }) + + describe('incoherent constraints', () => { + it('rejects min greater than max', () => { + const { errors } = model({ title: { type: 'string', min: 10, max: 5 } }) + expect(errors.some(e => /min \(10\) is greater than max \(5\)/.test(e))).toBe(true) + }) + + it('rejects a pattern that does not compile', () => { + // Left to validation time this degrades to a per-entry warning, silently + // disabling the constraint. + const { errors } = model({ title: { type: 'string', pattern: '[invalid' } }) + expect(errors.some(e => /not a valid regular expression/.test(e))).toBe(true) + }) + + it('rejects a default outside its own options', () => { + const { errors } = model({ status: { type: 'select', options: ['a', 'b'], default: 'z' } }) + expect(errors.some(e => /default "z" is not one of its own options/.test(e))).toBe(true) + }) + + it('rejects a default of the wrong type', () => { + const { errors } = model({ count: { type: 'number', default: 'lots' } }) + expect(errors.some(e => /default must be a number/.test(e))).toBe(true) + }) + }) + + describe('nested schemas', () => { + it('validates a nested object field', () => { + const { errors } = model({ + seo: { type: 'object', fields: { title: { type: 'bogus' } } }, + }) + expect(errors.some(e => /Field "seo.title": invalid type "bogus"/.test(e))).toBe(true) + }) + + it('validates a nested select without options', () => { + const { errors } = model({ + seo: { type: 'object', fields: { kind: { type: 'select' } } }, + }) + expect(errors.some(e => /Field "seo.kind": select type requires/.test(e))).toBe(true) + }) + + it('validates a nested field name', () => { + const { errors } = model({ + seo: { type: 'object', fields: { BadName: { type: 'string' } } }, + }) + expect(errors.some(e => /Field "seo.BadName": invalid name/.test(e))).toBe(true) + }) + + it('validates an items FieldDef', () => { + const { errors } = model({ + tags: { type: 'array', items: { type: 'select' } }, + }) + expect(errors.some(e => /Field "tags.items": select type requires/.test(e))).toBe(true) + }) + + it('validates an items type given as a string', () => { + const { errors } = model({ tags: { type: 'array', items: 'bogus' } }) + expect(errors.some(e => /Field "tags.items": invalid type "bogus"/.test(e))).toBe(true) + }) + + it('bounds runaway nesting', () => { + let field: Record = { type: 'string' } + for (let i = 0; i < 15; i++) field = { type: 'array', items: field } + const { errors } = model({ deep: field }) + expect(errors.some(e => /nesting depth/.test(e))).toBe(true) + }) + }) + + describe('constraints MCP cannot enforce are stated, not hidden', () => { + it('warns that maxSize is the provider’s job', () => { + const { errors, warnings } = model({ cover: { type: 'image', maxSize: 500_000 } }) + // Not an error — the constraint is legitimate, MCP just cannot check it. + expect(errors).toEqual([]) + expect(warnings.some(w => /"maxSize" is not enforced by MCP/.test(w))).toBe(true) + expect(warnings.some(w => /ingested/.test(w))).toBe(true) + }) + + it('warns that max on a media field measures the path, not the file', () => { + const { errors, warnings } = model({ cover: { type: 'image', max: 100 } }) + expect(errors).toEqual([]) + expect(warnings.some(w => /limits the length of the stored path string/.test(w))).toBe(true) + }) + + it('does not warn about max on an ordinary string field', () => { + const { warnings } = model({ title: { type: 'string', max: 100 } }) + expect(warnings).toEqual([]) + }) + }) + + describe('pre-existing rules still hold', () => { + it('rejects a non-kebab-case model id', () => { + const { errors } = validateModelDefinition({ id: 'Blog_Post', kind: 'collection', fields: {} }) + expect(errors.some(e => /must be kebab-case/.test(e))).toBe(true) + }) + + it('rejects fields on a dictionary', () => { + const { errors } = model({ title: { type: 'string' } }, 'dictionary') + expect(errors.some(e => /Dictionary models cannot have fields/.test(e))).toBe(true) + }) + + it('rejects a relation without a model', () => { + const { errors } = model({ author: { type: 'relation' } }) + expect(errors.some(e => /requires "model" property/.test(e))).toBe(true) + }) + }) +}) From 54c9f75fad561175e89d730e97803fbda4ec1997 Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Thu, 16 Jul 2026 00:00:13 +0300 Subject: [PATCH 5/6] feat(mcp)!: validate content before committing, and refuse to write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit content_save ran plan → commit → validate → report. An invalid value was written to git and auto-merged, and the caller learned about it from a string in next_steps while `status` still said "committed". Nothing on the write path ever consulted model.fields before the bytes landed, so required fields, patterns, select options and secrets were all advisory after the fact. Validation now runs on the pending changes and blocks. The move itself is mechanical — the existing call already used an OverlayReader over `plan.changes`, which needs nothing from the commit — but two things had to be decided: **Only this save's entries are fatal.** validateProject checks the whole model, so blocking on every error would let one pre-existing bad entry hold up an unrelated, valid save by a caller who may not be able to fix it. `touchesSavedEntries` filters to the entries in the call — by id for collections, slug for documents, locale for singletons and dictionaries. Everything else is reported, not enforced. **Warnings pass.** email/url/colour/phone patterns and the `accept` extension check are approximations; a legitimate value can sit outside one. They come back in the response as `warnings` instead of failing the write. The sharpest consequence is one that falls out rather than being designed: a secret can no longer reach git. detectSecrets is an error, and errors no longer commit. Four contentrain_validate fixtures broke, and the break was the signal: each planted invalid content *through content_save* to prove validate reports it, and that is exactly what the gate now prevents. They plant it on disk instead — the way a legacy project or a hand-edit leaves it — which is the case that still matters: the gate stops new problems, it does not clean a repository. Tests: tests/tools/content-gate.test.ts (6). They assert on the repository, not the response — a gate that only *claims* to have blocked is the bug being fixed — so they check the content file is empty and no branch was created. Also covered: a valid save still commits, a warning still commits, a secret does not, and a broken sibling entry does not block a good save. --- packages/mcp/src/tools/content.ts | 94 +++++++-- packages/mcp/tests/tools/content-gate.test.ts | 196 ++++++++++++++++++ packages/mcp/tests/tools/workflow.test.ts | 80 +++---- 3 files changed, 318 insertions(+), 52 deletions(-) create mode 100644 packages/mcp/tests/tools/content-gate.test.ts diff --git a/packages/mcp/src/tools/content.ts b/packages/mcp/src/tools/content.ts index f107360..441b4e8 100644 --- a/packages/mcp/src/tools/content.ts +++ b/packages/mcp/src/tools/content.ts @@ -1,10 +1,12 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js' import { z } from 'zod' +import type { ValidationError } from '@contentrain/types' import type { ToolProvider } from '../server.js' import { readConfig, readVocabulary } from '../core/config.js' import { readModel } from '../core/model-manager.js' import { listContent } from '../core/content-manager.js' import { planContentDelete, planContentSave } from '../core/ops/index.js' +import type { ContentSaveEntryResult } from '../core/ops/types.js' import { LocalProvider } from '../providers/local/index.js' import { buildBranchName } from '../git/transaction.js' import { checkBranchHealth } from '../git/branch-lifecycle.js' @@ -141,6 +143,35 @@ export function registerContentTools( .map(r => r.id ?? r.slug ?? r.locale) .filter((v): v is string => Boolean(v)) + // Validate BEFORE committing. This used to run after the commit and only + // report, so an invalid value landed in git — and was auto-merged — while + // the response still said `status: "committed"` and buried the problem in a + // next_steps string. The OverlayReader layers the pending changes over the + // provider, so it needs nothing from the commit. + const validationResult = await validateProject( + new OverlayReader(provider, plan.changes), + { model: input.model }, + ) + + // Only this save's own entries are fatal. validateProject checks the whole + // model, so an unrelated pre-existing error elsewhere in it must not block + // a caller who is writing something valid — they may not even be able to + // fix it. Everything else stays in the response as context. + const blocking = validationResult.issues.filter( + i => i.severity === 'error' && touchesSavedEntries(i, plan.result), + ) + + if (blocking.length > 0) { + return { + content: [{ type: 'text' as const, text: JSON.stringify({ + error: 'Validation failed — nothing was written.', + issues: blocking.map(describeIssue), + hint: 'Fix the values and call contentrain_content_save again. Nothing was committed, so there is no branch to clean up.', + }, null, 2) }], + isError: true, + } + } + const branch = buildBranchName('content', input.model) const message = `[contentrain] content: ${input.model}` const contextPayload = { @@ -173,20 +204,15 @@ export function registerContentTools( } } - // Post-save validation — runs against an OverlayReader that layers the - // just-saved changes on top of the provider's base view. This validates - // the committed state for BOTH providers and BOTH workflow modes. The - // old local path validated `projectRoot` (the developer working tree), - // which in review mode (or before selectiveSync) had not yet received the - // feature-branch files — producing false "locale file missing" errors for - // content that was just created. #7 - const validationResult = await validateProject( - new OverlayReader(provider, plan.changes), - { model: input.model }, - ) - const allAdvisories = plan.result.flatMap(r => r.advisories ?? []) + // Warnings do not block — they are heuristics (a colour that may not parse, + // an extension that may contradict `accept`), and a legitimate value can sit + // outside an approximate pattern. They ride along in the response instead. + const warnings = validationResult.issues + .filter(i => i.severity === 'warning' && touchesSavedEntries(i, plan.result)) + .map(i => `${i.field ?? i.locale}: ${i.message}`) + return { content: [{ type: 'text' as const, text: JSON.stringify({ status: 'committed', @@ -197,14 +223,12 @@ export function registerContentTools( advisories: allAdvisories, advisory_note: 'Save succeeded. Review these warnings and consider consolidating duplicate values.', } : {}), - validation: { - valid: validationResult.valid, - errors: validationResult.issues.filter(i => i.severity === 'error').map(i => i.message), - }, + ...(warnings.length > 0 ? { warnings } : {}), + validation: { valid: true, errors: [] }, context_updated: true, next_steps: [ ...(allAdvisories.length > 0 ? ['ADVISORY: Duplicate values detected — review advisories above'] : []), - ...(!validationResult.valid ? ['WARNING: Content has validation errors — run contentrain_validate for details'] : []), + ...(warnings.length > 0 ? ['REVIEW: see warnings above — the save was not blocked by them'] : []), ...(model.kind === 'collection' ? ['Use contentrain_content_list to verify', 'Add more entries or publish'] : ['Use contentrain_content_list to verify']), @@ -381,3 +405,39 @@ export function registerContentTools( }, ) } + +/** Trim a validation issue to the fields that locate it, dropping empty ones. */ +function describeIssue(issue: ValidationError): Record { + const out: Record = {} + if (issue.entry) out['entry'] = issue.entry + if (issue.slug) out['slug'] = issue.slug + if (issue.locale) out['locale'] = issue.locale + if (issue.field) out['field'] = issue.field + out['message'] = issue.message + return out +} + +/** + * Does a validation issue belong to one of the entries this save is writing? + * + * The write gate blocks on errors, but `validateProject` inspects the whole model. + * Without this filter, a pre-existing bad entry somewhere else in the model would + * block an unrelated — and perfectly valid — save, leaving the caller stuck behind + * a problem they did not create and may not be able to fix. + * + * Matching is by identity: entry ID for collections, slug for documents, and + * locale alone for singletons and dictionaries, which have one record per locale. + */ +function touchesSavedEntries( + issue: { entry?: string, slug?: string, locale?: string }, + results: ContentSaveEntryResult[], +): boolean { + return results.some((r) => { + if (issue.locale && r.locale && issue.locale !== r.locale) return false + if (r.id) return issue.entry === r.id + if (r.slug) return issue.slug === r.slug + // Singleton / dictionary: the locale is the whole identity, and an issue with + // no entry or slug is about the record itself. + return !issue.entry && !issue.slug + }) +} diff --git a/packages/mcp/tests/tools/content-gate.test.ts b/packages/mcp/tests/tools/content-gate.test.ts new file mode 100644 index 0000000..d4614f4 --- /dev/null +++ b/packages/mcp/tests/tools/content-gate.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest' + +vi.setConfig({ testTimeout: 120000, hookTimeout: 120000 }) +import { join } from 'node:path' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { simpleGit } from 'simple-git' +import { Client } from '@modelcontextprotocol/sdk/client/index.js' +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js' +import { createServer } from '../../src/server.js' +import { readJson } from '../../src/util/fs.js' + +/** + * The write gate. content_save used to run plan → commit → validate → report: + * an invalid value landed in git, was auto-merged, and the caller learned about + * it from a string in `next_steps` while `status` still said "committed". + * + * These assert on the repository, not on the response — a gate that only claims + * to have blocked is the bug we are fixing. + */ + +let testDir: string +let client: Client + +async function initProject(dir: string): Promise { + const git = simpleGit(dir) + await git.init() + await git.addConfig('user.name', 'Test') + await git.addConfig('user.email', 'test@test.com') + await writeFile(join(dir, '.gitkeep'), '') + await git.add('.') + await git.commit('initial') +} + +async function createTestClient(projectRoot: string): Promise { + const server = createServer(projectRoot) + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() + const c = new Client({ name: 'test-client', version: '1.0.0' }) + await Promise.all([c.connect(clientTransport), server.connect(serverTransport)]) + return c +} + +function parseResult(result: unknown): Record { + const content = (result as { content: Array<{ text: string }> }).content + return JSON.parse(content[0]!.text) as Record +} + +async function createModel(id: string, fields: Record): Promise { + await client.callTool({ + name: 'contentrain_model_save', + arguments: { id, name: id, kind: 'collection', domain: 'blog', i18n: true, fields }, + }) + client = await createTestClient(testDir) +} + +function contentFile(model: string, locale: string): Promise | null> { + return readJson(join(testDir, '.contentrain', 'content', 'blog', model, `${locale}.json`)) +} + +async function branches(): Promise { + return (await simpleGit(testDir).branchLocal()).all +} + +beforeEach(async () => { + testDir = await mkdtemp(join(tmpdir(), 'cr-gate-test-')) + await initProject(testDir) + client = await createTestClient(testDir) + await client.callTool({ name: 'contentrain_init', arguments: { locales: ['en'] } }) + client = await createTestClient(testDir) +}) + +afterEach(async () => { + await rm(testDir, { recursive: true, force: true }) +}) + +describe('contentrain_content_save write gate', () => { + it('writes nothing when a value is invalid', async () => { + await createModel('posts', { title: { type: 'string', required: true }, slug: { type: 'slug' } }) + const before = await branches() + + const result = await client.callTool({ + name: 'contentrain_content_save', + arguments: { + model: 'posts', + entries: [{ locale: 'en', data: { title: 'Hi', slug: 'Not A Slug!!' } }], + }, + }) + + const data = parseResult(result) + expect(data['status']).not.toBe('committed') + expect(data['error']).toContain('nothing was written') + + // The repository is the witness: no content, no new branch. + const content = await contentFile('posts', 'en') + expect(content ?? {}).toEqual({}) + expect(await branches()).toEqual(before) + }) + + it('names the offending field so the caller can fix it', async () => { + await createModel('posts', { title: { type: 'string', required: true }, slug: { type: 'slug' } }) + + const result = await client.callTool({ + name: 'contentrain_content_save', + arguments: { + model: 'posts', + entries: [{ locale: 'en', data: { title: 'Hi', slug: 'Not A Slug!!' } }], + }, + }) + + const issues = parseResult(result)['issues'] as Array> + expect(issues.some(i => i['field'] === 'slug')).toBe(true) + }) + + it('commits a valid entry', async () => { + await createModel('posts', { title: { type: 'string', required: true }, slug: { type: 'slug' } }) + + const result = await client.callTool({ + name: 'contentrain_content_save', + arguments: { + model: 'posts', + entries: [{ locale: 'en', data: { title: 'Hi', slug: 'a-real-slug' } }], + }, + }) + + expect(parseResult(result)['status']).toBe('committed') + expect(Object.keys(await contentFile('posts', 'en') ?? {})).toHaveLength(1) + }) + + it('lets a warning through and reports it', async () => { + // Heuristics must not block — a legitimate value can sit outside the pattern. + await createModel('people', { name: { type: 'string' }, contact: { type: 'email' } }) + + const result = await client.callTool({ + name: 'contentrain_content_save', + arguments: { + model: 'people', + entries: [{ locale: 'en', data: { name: 'Ada', contact: 'not-an-email' } }], + }, + }) + + const data = parseResult(result) + expect(data['status']).toBe('committed') + expect((data['warnings'] as string[]).some(w => /email/.test(w))).toBe(true) + expect(Object.keys(await contentFile('people', 'en') ?? {})).toHaveLength(1) + }) + + it('refuses to commit a secret', async () => { + // Falls out of the gate rather than being a special case: detectSecrets is an + // error, and errors no longer reach git. Pinned because it is the sharpest + // consequence of moving validation ahead of the commit. + await createModel('settings', { name: { type: 'string' }, token: { type: 'string' } }) + const before = await branches() + + const result = await client.callTool({ + name: 'contentrain_content_save', + arguments: { + model: 'settings', + entries: [{ locale: 'en', data: { name: 'API', token: 'sk_live_abc123secret' } }], + }, + }) + + expect(parseResult(result)['error']).toContain('nothing was written') + expect(await contentFile('settings', 'en') ?? {}).toEqual({}) + expect(await branches()).toEqual(before) + }) + + it('does not block a valid save because another entry is already broken', async () => { + // A pre-existing bad entry elsewhere in the model must not hold the caller + // hostage — they may not even be able to fix it. + await createModel('posts', { title: { type: 'string', required: true }, slug: { type: 'slug' } }) + + const first = await client.callTool({ + name: 'contentrain_content_save', + arguments: { model: 'posts', entries: [{ locale: 'en', data: { title: 'Good', slug: 'ok-one' } }] }, + }) + const goodId = (parseResult(first)['results'] as Array>)[0]!['id'] as string + + // Corrupt it behind MCP's back, the way a hand-edit or an older version would. + const path = join(testDir, '.contentrain', 'content', 'blog', 'posts', 'en.json') + const { writeJson } = await import('../../src/util/fs.js') + const existing = await readJson>(path) ?? {} + existing[goodId] = { title: 'Good', slug: 'BROKEN SLUG!!' } + await writeJson(path, existing) + const git = simpleGit(testDir) + await git.add('.') + await git.commit('corrupt entry') + client = await createTestClient(testDir) + + const result = await client.callTool({ + name: 'contentrain_content_save', + arguments: { model: 'posts', entries: [{ locale: 'en', data: { title: 'New', slug: 'ok-two' } }] }, + }) + + expect(parseResult(result)['status']).toBe('committed') + }) +}) diff --git a/packages/mcp/tests/tools/workflow.test.ts b/packages/mcp/tests/tools/workflow.test.ts index 15244da..f8e8b43 100644 --- a/packages/mcp/tests/tools/workflow.test.ts +++ b/packages/mcp/tests/tools/workflow.test.ts @@ -89,19 +89,34 @@ describe('contentrain_validate', () => { expect(branches.all).toContain('contentrain') }) + /** + * Write content straight to the working tree, bypassing content_save. + * + * content_save now refuses to commit content with validation errors, so the + * only way an invalid entry exists is if it predates the gate or was + * hand-edited. These fixtures reproduce exactly that: the gate stops *new* + * problems, it does not retroactively clean a repository, so + * `contentrain_validate` must still report the old ones. + */ + async function plantContent( + domain: string, + model: string, + perLocale: Record>, + ): Promise { + for (const [locale, data] of Object.entries(perLocale)) { + await writeJson(join(testDir, '.contentrain', 'content', domain, model, `${locale}.json`), data) + } + } + it('detects required field missing', async () => { client = await createModel(client, 'authors', 'collection', 'blog', { name: { type: 'string', required: true }, bio: { type: 'text' }, }) - // Save content with missing required field - await client.callTool({ - name: 'contentrain_content_save', - arguments: { - model: 'authors', - entries: [{ id: 'author-001', locale: 'en', data: { bio: 'A developer' } }], - }, + await plantContent('blog', 'authors', { + en: { 'author-001': { bio: 'A developer' } }, + tr: { 'author-001': { bio: 'A developer' } }, }) client = await createClient(testDir) @@ -134,13 +149,9 @@ describe('contentrain_validate', () => { category: { type: 'relation', model: 'categories' }, }) - // Save post with nonexistent category reference - await client.callTool({ - name: 'contentrain_content_save', - arguments: { - model: 'posts', - entries: [{ id: 'post-001', locale: 'en', data: { title: 'My Post', category: 'nonexistent-cat' } }], - }, + await plantContent('blog', 'posts', { + en: { 'post-001': { title: 'My Post', category: 'nonexistent-cat' } }, + tr: { 'post-001': { title: 'My Post', category: 'nonexistent-cat' } }, }) client = await createClient(testDir) @@ -197,15 +208,10 @@ describe('contentrain_validate', () => { token: { type: 'string' }, }) - await client.callTool({ - name: 'contentrain_content_save', - arguments: { - model: 'settings', - entries: [ - { locale: 'en', data: { api_endpoint: 'https://api.example.com', token: 'sk_live_abc123secret' } }, - { locale: 'tr', data: { api_endpoint: 'https://api.example.com', token: 'sk_live_abc123secret' } }, - ], - }, + // A singleton stores one flat object per locale — no entry-id keying. + await plantContent('system', 'settings', { + en: { api_endpoint: 'https://api.example.com', token: 'sk_live_abc123secret' }, + tr: { api_endpoint: 'https://api.example.com', token: 'sk_live_abc123secret' }, }) client = await createClient(testDir) @@ -230,15 +236,9 @@ describe('contentrain_validate', () => { email: { type: 'string', required: true, pattern: '^[^@]+@example\\\\.com$' }, }) - await client.callTool({ - name: 'contentrain_content_save', - arguments: { - model: 'authors', - entries: [ - { id: 'author-001', locale: 'en', data: { email: 'alice@outside.com' } }, - { id: 'author-001', locale: 'tr', data: { email: 'alice@outside.com' } }, - ], - }, + await plantContent('blog', 'authors', { + en: { 'author-001': { email: 'alice@outside.com' } }, + tr: { 'author-001': { email: 'alice@outside.com' } }, }) client = await createClient(testDir) @@ -505,17 +505,25 @@ describe('contentrain_validate', () => { }, }) + // content_save now refuses to write an entry with a missing required nested + // field, so the invalid state has to be planted the way a legacy project or a + // hand-edit would produce it: valid save first, then corrupt the file. await client.callTool({ name: 'contentrain_content_save', arguments: { model: 'pages', entries: [ - { id: 'page-a', locale: 'en', data: { seo: {} } }, - { id: 'page-a', locale: 'tr', data: { seo: {} } }, + { id: 'page-a', locale: 'en', data: { seo: { title: 'Fine' } } }, + { id: 'page-a', locale: 'tr', data: { seo: { title: 'Fine' } } }, ], }, }) + for (const locale of ['en', 'tr']) { + const path = join(testDir, '.contentrain', 'content', 'marketing', 'pages', `${locale}.json`) + await writeJson(path, { 'page-a': { seo: {} } }) + } + client = await createClient(testDir) const result = await client.callTool({ @@ -525,8 +533,10 @@ describe('contentrain_validate', () => { const data = parseResult(result) const issues = data['issues'] as Array> + // The path is qualified by its parent — a bare `title` would be ambiguous + // with a top-level field of the same name. const nestedError = issues.find(i => - i['severity'] === 'error' && i['field'] === 'title', + i['severity'] === 'error' && i['field'] === 'seo.title', ) expect(nestedError).toBeDefined() }) From 173326c7926f24ad2fd21e79234d38bfc9fc772d Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Thu, 16 Jul 2026 00:00:13 +0300 Subject: [PATCH 6/6] docs: correct the constraint contract in rules and the docs site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit schema-rules.md listed accept, maxSize and default as working, and media-rules.md told agents 'do not silently accept oversized files' — a rule with no machine backing. The table now names who enforces each property, and says plainly that maxSize is the provider's job at ingest and that accept is an extension check. Essentials and the MCP docs page gain the write-gate contract. --- .changeset/field-constraints.md | 78 +++++++++++++++++++ docs/packages/mcp.md | 32 ++++++++ .../rules/essential/contentrain-essentials.md | 2 + packages/rules/shared/media-rules.md | 6 ++ packages/rules/shared/schema-rules.md | 35 +++++---- 5 files changed, 137 insertions(+), 16 deletions(-) create mode 100644 .changeset/field-constraints.md diff --git a/.changeset/field-constraints.md b/.changeset/field-constraints.md new file mode 100644 index 0000000..583134f --- /dev/null +++ b/.changeset/field-constraints.md @@ -0,0 +1,78 @@ +--- +"@contentrain/mcp": major +"@contentrain/types": minor +--- + +feat(mcp)!: enforce the field constraints the schema already accepted + +A project reported that `items`, `accept` and `maxSize` are accepted on a field but +never enforced — `emails: ["not-an-email"]` and `accept: "image/jpeg"` against a +`.webp` both produced zero errors. The report was right, and the surface was larger +than the three properties it named: **4 of 27 field types had any semantic +validation**, three constraints were read by nothing, and none of it blocked a write. + +A constraint that isn't a constraint is worse than no constraint — the author stops +looking. + +**`content_save` now validates before committing and refuses to write.** It ran +`plan → commit → validate → report`, so an invalid value landed in git, was +auto-merged, and the caller learned about it from a string in `next_steps` while +`status` still said `"committed"`. Validation now runs on the pending changes and +blocks on errors, returning `isError` and no commit. Warnings still pass — they are +heuristics, and a legitimate value can sit outside an approximate pattern. Only the +entries being saved are fatal: a pre-existing bad entry elsewhere in the model does +not hold up an unrelated save. + +**Array items share the scalar rule set.** They ran through a parallel type switch +that knew 10 of the 27 types and checked only `typeof`, so `min`/`max`/`pattern`/ +`options` never reached an item, and `items` given as a FieldDef with a non-object +type (`{type:'array', items:{type:'string', max:50}}`) matched no branch at all — +silently unvalidated, while the type emitter rendered it as real. Items now recurse +through the same validator, which also closes the `integer` split where `3.7` was +rejected inside an array but accepted as a scalar. + +**17 types were pure `typeof` checks.** `slug` now uses the `SLUG_PATTERN` the +codebase already owned — every shipped template declares `slug: { type: 'slug' }`, +so `"Hello World!!"` used to validate clean. `date`/`datetime` are parsed (the same +check `schedule.ts` already did for meta), `percent` is range-checked, and `color`/ +`phone` warn. Mechanical rules are errors; heuristics are warnings. `email`/`url` +keep their existing warning severity. `rating` is deliberately untouched — its scale +is never declared, so any range would be invented. + +**`unique` works on documents.** It was gated on a context only the collection +validator passed, so it was a no-op exactly where every shipped template declares it. +On singletons it is now rejected at model_save: the model holds one record per +locale, so there is nothing to compare against. + +**The dead constraints, handled honestly.** `accept` is enforced by extension-sniff +and says that is what it is. `default` is coherence-checked at model_save (right +type, within its own `options`) but not written into content. `maxSize` **cannot be +enforced by MCP** — it holds a path, never the bytes — so model_save now says so and +points at the provider, which owns the policy at ingest. The docs claimed all three +worked; they no longer do. + +**model_save rejects what it will not enforce.** `options` on a non-select, `items` +on a non-array, `accept`/`maxSize` on a non-media field, `min > max`, and an +uncompilable `pattern` are now errors instead of silent no-ops. Nested `fields`/ +`items` schemas are validated recursively — they were typed `z.unknown()` and never +checked. The field schema is `.strict()`: a typo'd constraint (`requird: true`) used +to be stripped without a word. + +BREAKING CHANGE: + +- `content_save` rejects content it previously committed. Run `contentrain_validate` + before upgrading to see what would now be blocked. +- `model_save` rejects models it previously accepted (unknown keys, `min > max`, + `options` on a non-select, `unique` on a singleton). +- `validateModelDefinition` returns `{ errors, warnings }` instead of `string[]`. +- Array-item type errors carry `validateFieldValue`'s message ("Type mismatch: + expected string, got number") instead of "must be a string". The field path is + unchanged. +- Nested object errors are qualified by their parent (`seo.title`, not `title`) — + a bare name was ambiguous with a top-level field. + +`@contentrain/types` gains `validateSemanticType`, `validateAccept` and +`isMediaType`; `validateFieldValue` now applies semantic and `accept` rules. + +Studio picks all of this up automatically — its `content-validation.ts` delegates to +this validator. diff --git a/docs/packages/mcp.md b/docs/packages/mcp.md index b7ca8fa..52c0a68 100644 --- a/docs/packages/mcp.md +++ b/docs/packages/mcp.md @@ -124,6 +124,38 @@ The MCP server exposes **24 tools** — 19 core + 5 media — organized by funct | `contentrain_apply` | Extract or reuse | Two-phase normalize: extract content or patch source files | | `contentrain_bulk` | Batch operations | Bulk locale copy, status updates, and deletes | +### Field constraints + +`contentrain_content_save` **validates before it writes**. A `severity: error` issue +on any entry in the call means nothing is committed — no branch, nothing to clean up. +Fix the values and call again. Warnings pass and come back in the response. + +The split is deliberate: + +- **Errors** are definitional. A `slug` that does not match `SLUG_PATTERN` is not a + slug; an unparseable `date` is not a date; `3.7` is not an `integer`. +- **Warnings** are heuristics. `email`, `url`, `color` and `phone` patterns are + approximations, and a legitimate value can sit outside one. + +Only the entries in the call are fatal. A pre-existing bad entry elsewhere in the +model is reported but does not block your save. + +Array items are validated by the same rules as a scalar of that type, so +`items: { type: 'string', max: 50 }` means what it looks like it means. + +::: warning What MCP does not enforce +`maxSize` is the one constraint MCP accepts but cannot check — it stores a path and +never sees the file. Your media provider enforces it at ingest, and `model_save` +returns a `schema_warnings` entry saying so. `accept` **is** checked, but against the +file extension only, which is why it warns rather than errors. +::: + +`model_save` rejects a constraint declared where it cannot apply — `options` on a +non-select, `items` on a non-array, `accept` on a non-media field, `unique` on a +singleton, `min > max`, an uncompilable `pattern` — instead of storing it and doing +nothing. Unknown keys are rejected too, so a typo'd `requird: true` is an error +rather than a silent no-op. + ### Publish status Entry status lives in `.contentrain/meta/`, not in content, and it decides CDN diff --git a/packages/rules/essential/contentrain-essentials.md b/packages/rules/essential/contentrain-essentials.md index 5c6d1d2..02a1e9e 100644 --- a/packages/rules/essential/contentrain-essentials.md +++ b/packages/rules/essential/contentrain-essentials.md @@ -34,6 +34,8 @@ MCP is **deterministic infrastructure**. The agent (you) is the **intelligence l - **NEVER** include system fields in content data: `id`, `status`, `source`, `updated_by`, `updated_at`, `createdAt`, `updatedAt` - **ALWAYS** use MCP tools — do not write `.contentrain/` JSON files directly - **Publishing is yours, not MCP's.** `contentrain_content_save` preserves an existing entry's `status`; only a brand-new entry starts at `draft`. To change publish state, call `contentrain_bulk update_status` deliberately — and note that on the CDN a collection entry is delivered only when its status is `published`. +- **Schema validity is MCP's.** `contentrain_content_save` validates before it writes and **refuses** the save on any error — nothing is committed, so there is no branch to clean up. Fix the values and call it again. Warnings (email/url/colour/phone heuristics, `accept` extension checks) do not block; they come back in the response. +- **A declared constraint is a real constraint.** `model_save` rejects a property declared where it cannot apply (`options` on a non-select, `accept` on a non-media field, `unique` on a singleton) rather than accepting it and doing nothing. The one exception is `maxSize`, which MCP cannot enforce — your media provider does, at ingest, and `model_save` says so. ## MCP Tools diff --git a/packages/rules/shared/media-rules.md b/packages/rules/shared/media-rules.md index e4423ac..11178f8 100644 --- a/packages/rules/shared/media-rules.md +++ b/packages/rules/shared/media-rules.md @@ -73,6 +73,12 @@ Never hand-build delivery URLs or guess the CDN base. Save the path or URL the m 3. If an image exceeds the size limit, instruct the user to compress or resize it before storing. Do not silently accept oversized files. + **This one is on you.** A `maxSize` on the field does not enforce it: MCP stores a + path and never sees the bytes, so it cannot check the size — your media provider + does, when the asset is ingested. `model_save` says as much when you declare + `maxSize`. A field's `accept` is checked, but only against the file **extension**, + which can lie. + 4. SVG files SHOULD be optimized (e.g., with SVGO) to remove editor metadata, comments, and unnecessary attributes. --- diff --git a/packages/rules/shared/schema-rules.md b/packages/rules/shared/schema-rules.md index 60e1986..269898f 100644 --- a/packages/rules/shared/schema-rules.md +++ b/packages/rules/shared/schema-rules.md @@ -92,22 +92,25 @@ A field definition describes one field in a model. Include only the properties t ### 3.1 Field Properties -| Property | Applicable Types | Description | -|----------|-----------------|-------------| -| `type` | ALL | **Required.** One of the 27 types. | -| `required` | ALL | Mark field as mandatory. Default: `false`. Omit if `false`. | -| `unique` | `string`, `email`, `slug`, `integer` | Enforce uniqueness within model. Default: `false`. Omit if `false`. | -| `default` | ALL | Default value. Omit if `null`. | -| `min` | string/text: char count; numbers: value; array: element count | Minimum constraint. | -| `max` | Same as `min` | Maximum constraint. | -| `pattern` | `string`, `text`, `code` | Regex validation pattern. | -| `options` | `select` ONLY | Fixed choices: `["draft", "published", "archived"]` | -| `model` | `relation`, `relations` ONLY | Target model ID. String or string array for polymorphic. | -| `items` | `array` ONLY | Element type: `"string"` or `{ "type": "object", "fields": {...} }` | -| `fields` | `object` ONLY | Nested field definitions. | -| `accept` | `image`, `video`, `file` | Allowed MIME types: `"image/png,image/jpeg"` | -| `maxSize` | `image`, `video`, `file` | Maximum file size in bytes. | -| `description` | ALL | Human-readable hint (shown in Studio UI tooltip, used as agent context). | +A property declared where it does not apply is **rejected by `model_save`** — it is +not silently ignored. The "Enforced by" column says who actually checks each one. + +| Property | Applicable Types | Enforced by | Description | +|----------|-----------------|-------------|-------------| +| `type` | ALL | content_save (blocks) | **Required.** One of the 27 types. | +| `required` | ALL | content_save (blocks) | Mark field as mandatory. Default: `false`. Omit if `false`. | +| `unique` | Any type, on `collection` and `document` models. **Not `singleton`** — it holds one record per locale, so there is nothing to be unique against. | content_save (blocks) | Enforce uniqueness within the model, per locale. Default: `false`. Omit if `false`. | +| `default` | ALL | model_save (coherence only) | Default value. Checked against its own `type` and `options`; **not** written into content — the agent supplies values. Omit if `null`. | +| `min` | string/text: char count; numbers: value; array: element count | content_save (blocks) | Minimum constraint. Must not exceed `max`. | +| `max` | Same as `min` | content_save (blocks) | Maximum constraint. On a media field this limits the **path string length**, not the file — use `maxSize` for bytes (model_save warns). | +| `pattern` | Any string-shaped type | content_save (blocks) | Regex validation pattern. Compiled at model_save, so a broken regex is rejected rather than silently disabling the rule. | +| `options` | `select` ONLY | content_save (blocks) | Fixed choices: `["draft", "published", "archived"]` | +| `model` | `relation`, `relations` ONLY | content_save (blocks) | Target model ID. String or string array for polymorphic. | +| `items` | `array` ONLY | content_save (blocks) | Element type: `"string"`, or a full FieldDef — `{ "type": "string", "max": 50 }`. Items are validated by the same rules as a scalar of that type. | +| `fields` | `object` ONLY | content_save (blocks) | Nested field definitions. Validated recursively, at model_save and at content_save. | +| `accept` | `image`, `video`, `file` | content_save (**warns**) | Allowed MIME types: `"image/png,image/jpeg"`, `"image/*"`, `".pdf"`. MCP checks the value's **file extension** — an approximation, since it holds a path, not the bytes. Your provider enforces the real MIME type at ingest. | +| `maxSize` | `image`, `video`, `file` | **Provider, at ingest — not MCP** | Maximum file size in bytes. MCP never sees the file, so it cannot check this; model_save says so. | +| `description` | ALL | — | Human-readable hint (shown in Studio UI tooltip, used as agent context). | ### 3.2 Omission Rules