feat(mcp)!: enforce the field constraints the schema already accepted

.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.

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

packages/mcp/src/core/apply-manager.ts

@@ -395,8 +395,8 @@ export async function applyExtract(
       kind: ext.kind,
       fields: ext.fields as Record<string, unknown> | 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) {

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<Record<string, unknown>> = 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<Record<string, unknown>> = 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<Record<string, unknown>> = z.record(z.
 
 const VALID_FIELD_TYPES = new Set<string>(FIELD_TYPE_ENUM)
 
+/** Field types whose value is a path/URL to a media asset. */
+const MEDIA_FIELD_TYPES = new Set<string>(['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<string, unknown>)) {
+      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, unknown> }): string[] {
+export function validateModelDefinition(
+  input: { id: string; kind: string; fields?: Record<string, unknown> },
+): 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 }
 }