Schema-drift guard: defineType() stops silently replacing types (#68)#101
Merged
Conversation
…g types (#68) schemaHash was computed and stored on every defineType() but compared by nothing: redefining an existing typeId with a different shape (a buggy deploy, a second app squatting on a namespace) silently replaced the stored type and mislabeled every existing record of it — the exact corruption the hash was invented to catch, undetected. seedSystemTypes() also churned createdAt on every Stack.create() as a side effect of the same blind replace. A naive "hash differs -> reject" guard is wrong given #47/#57/#63's legal additive in-place changes (which also change the hash), so the judge is a structural diff, not the hash — the hash stays a cheap equality fast path. - diffSchemas() (schema.ts): the evolution-legality sibling to isCompatible()'s read-compatibility, documented side by side since conflating them is the obvious future bug (text/string interchange for reading but not for evolution — a kind change is drift either way). Recurses into object properties and array items; fails closed past the same depth bound isCompatible() uses. - defineType() on an existing typeId: identical schema is a true no-op (createdAt untouched); identical schema with a new name persists just the name; a different schema throws StackSchemaDriftError naming every violation unless diffSchemas() says it's pure addition. - Fixed a regression caught by the existing stale-writer test while building this: the no-op path must still update maxDefinedVersion bookkeeping, or presentAt: 'latest' stops detecting a record newer than what a given Stack instance has ever defineType()'d. - schema_drift added to wire-types' error taxonomy (409, sharing status with conflict — no competing convention pulls it to its own code like version_conflict/412), with its typeId/violations round-tripping through a dedicated schemaDrift wire field. - docs/spec.md: the aspirational drift-detection paragraph replaced with the actual mechanical rule, the two relations (diffSchemas vs isCompatible) documented explicitly, POST /types described as running the same check server-side, and the error tables/wire body updated. Conformance fixtures for POST /types (#52's stated work item) are left for a follow-up: unlike the record-mutation endpoints, this wire endpoint has no existing request/response convention in this repo to extend, so pinning one down is a separable piece of work rather than a natural extension of an existing fixture array. Fixes #68 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDmUW3oNxPSCLf2ftqaF8q
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
schemaHashexisted to catch exactly one bug — sametypeId, different schema, no version bump — but was computed and stored on everydefineType()and compared by nothing, anywhere.defineType()never read the existing type; it built a freshStackTypeand handed it tosaveType()(INSERT OR REPLACE). Consequences: a buggy deploy or a second app squatting on a namespace could silently redefine a type and mislabel every existing record of it, andseedSystemTypes()churnedcreatedAton everyStack.create()as a side effect of the same blind replace.A naive "hash differs → reject" guard is wrong given #47/#57/#63's legal additive in-place changes (which also change the hash) — the judge has to be a structural diff, with the hash kept as a cheap equality fast-path.
Changes
diffSchemas()(packages/core/src/schema.ts) — the evolution-legality sibling toisCompatible()'s read-compatibility, documented side by side since conflating them is the obvious future bug:text/stringinterchange for reading but not for evolution (changing a field's declaredkindis drift either way, even to a read-compatible kind). Recurses intoobjectproperties andarrayitems; fails closed past the same depth boundisCompatible()uses.defineType()on an existingtypeId: identical schema → true no-op (createdAtuntouched); identical schema with a newname→ persists just the rename; a different schema →StackSchemaDriftErrornaming every violation, unlessdiffSchemas()says it's pure addition (new optional fields only — nothing removed, retyped, or re-required in either direction).maxDefinedVersionbookkeeping, orpresentAt: 'latest'stops detecting a record newer than what a givenStackinstance has everdefineType()'d (an existing test caught this before it shipped).schema_driftadded towire-types' error taxonomy — 409, deliberately sharing status withconflict(no competing convention pulls it to its own code the wayversion_conflict/412 does), withtypeId/violationsround-tripping through a dedicatedschemaDriftwire field.docs/spec.md: the aspirational drift-detection paragraph replaced with the actual mechanical rule; the two relations (diffSchemasvsisCompatible) documented explicitly;POST /typesdescribed as running the same check server-side; error tables and the wire error body updated.Scope note: #52's stated work item includes conformance fixtures for
POST /types. Unlike the record-mutation endpoints, this wire endpoint has no existing request/response convention in this repo's fixture package to extend — designing one is a separable piece of work, not a natural extension of an existing fixture array, so I've left it for a follow-up rather than inventing a wire contract from scratch in this PR.Test plan
packages/coretypecheck, lint, tests (455/455) — 14 newdiffSchemas()cases (additive accept incl. nested, each violation class rejected, multiple simultaneous violations, depth-bound fail-closed) and 12 newdefineType()cases (idempotent no-op, name-only persist, additive accept incl. nested, each drift class rejected with the exact violation named, no partial write on rejection, cross-Stack-instance idempotency simulating repeatedseedSystemTypes()on reopen)wire-typesbuild + lint cleanadapter-apitests (91/91), typecheck, lint — confirmsserializeError/deserializeErrorround-trip the new code generically with no adapter-side changes neededadapter-local,record-adapter-sqlite,record-adapter-sqljs,sqlite-sharedfull suites unaffected (218 tests total) — confirms no adapter package depends on the old blind-replacedefineType()behaviorGenerated by Claude Code