fix: correct equality, union dispatch, schema identity and the freeze - #26
Merged
Conversation
Five bugs found reviewing the source, each reproduced before being fixed and
each pinned by a test. All of them are in field types the package already
accepts -- `OnlyNominal` admits any branded schema, `freeze.ts` contemplates
Map/Set fields, and `shape.ts` blesses `z.enum` -- so none needed an exotic
declaration to hit.
`equals` compared `JSON.stringify` output. A bigint field made it **throw** `Do
not know how to serialize a BigInt`, escaping uncaught because `equals` returns
a bare boolean with no Result channel -- a direct breach of the errors-as-values
rule. A Set, Map or typed-array field serialised to `{}`, so entities with
entirely different contents compared **equal**, a false positive on identity.
And only top-level key order was normalised, so a nested record `{a,b}` versus
`{b,a}` compared unequal. The new `equal.ts` compares structurally.
The union discriminant lookup read `.value`, which only a single-valued
`z.literal` has. An enum member registered under `undefined`: `input` accepted
payloads `make` then rejected, a payload missing the discriminant was misrouted
to that member rather than reported, and the "expected one of" message rendered
it empty. A multi-value literal threw at union construction. It now reads the
plural accessors and registers a member under every value it claims.
`omitBy` returned its argument for an empty key list and `output` was `input`
when nothing was computed, so a plain entity had one object under three names.
Since contracts compose these four objects, a registry keyed by identity kept
only the last write.
The freeze dispatched on runtime shape, so a plain-object `z.custom` value --
the caller's own reference -- was frozen in place and the caller's next write
threw. Only the schema knows what was passed through, so `entity.ts` now
decides from it. That is what the docs already promised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness issues in the @btravstack/entity core runtime (equality, union dispatch, schema identity, and freezing behavior) and adds regression tests + documentation/changeset updates to pin the intended semantics.
Changes:
- Replace
equals()’sJSON.stringifycomparison with structural deep equality (deepEqual) and add coverage for bigint/Set/Map/Date/records. - Fix union member dispatch to support enum and multi-value literal discriminants consistently across
inputandmake. - Ensure the four contract schema members are always distinct objects; adjust freezing to skip schema-declared passthrough (
z.custom/z.instanceof) fields.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/entity/src/union.ts | Dispatch map now registers members for all discriminant values (enum + multi-literal). |
| packages/entity/src/union.spec.ts | Adds tests covering enum routing, input/make agreement, missing discriminant, multi-value literals. |
| packages/entity/src/freeze.spec.ts | Adds regression tests for z.custom passthrough and ordinary object freezing. |
| packages/entity/src/equal.ts | Introduces deepEqual structural equality implementation used by equals(). |
| packages/entity/src/equal.spec.ts | Adds test coverage for structural equality across supported stored types. |
| packages/entity/src/entity.ts | Wires in deepEqual, fixes schema identity rebuilding, and adds schema-based passthrough freeze skipping. |
| packages/entity/src/contract.spec.ts | Adds tests ensuring contract schema members are distinct and registry identity is stable. |
| docs/reference.md | Updates equals documentation to reflect structural comparison semantics. |
| docs/explanation.md | Clarifies freezing behavior and schema-based passthrough skipping rationale. |
| CLAUDE.md | Updates architecture docs to include the new equal.ts module and freeze behavior note. |
| .changeset/fix-equality-union-freeze.md | Adds a patch changeset documenting the fixed bugs and behavioral corrections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both from review of the first pass. `deepEqual` recursed with no cycle tracking, so two cyclic values died with `RangeError: Maximum call stack size exceeded` -- the same escaping-throw failure mode the module was written to remove, and reachable precisely because this branch stopped freezing `z.custom` values, leaving them free to close a loop. It now tracks pairs, keyed by the left value, so a revisited pair is assumed equal (the co-inductive reading) while a difference reachable only through a cycle is still found. The passthrough skip compared top-level field schemas only, so a `z.custom` nested inside a branded object, an array, a record or a tuple was still frozen in place -- the caller's object, the same bug one level down. `deepFreeze` now carries the schema alongside the value and consults it at every step, following the single-child wrappers and treating a union as passthrough if any branch is. Where a container cannot be followed the schema is simply absent and the walk freezes as it always did, so an unhandled shape is a missed skip, never a crash. `schema` sits after `seen` in the signature on purpose: typed `unknown`, in the second position it silently swallowed the existing `deepFreeze(value, seen)` calls, which type-checked and quietly stopped sharing the set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Five bugs found reviewing the source. Each was reproduced before being fixed and is now pinned by a test. None needed an exotic declaration to hit —
OnlyNominaladmits any branded schema,freeze.tsexplicitly contemplatesMap/Setfields, andshape.tsblessesz.enumas nominal.equalsusedJSON.stringifyThree failures, measured on the pinned zod 4.4.3:
The
bigintcase is the sharpest:equalsreturns a barebooleanwith no Result channel, so theTypeErrorescaped uncaught — a direct breach of the errors-as-values rule. TheSetcase is a false positive on identity, the worse direction. The third meant a repository reading a row back with different JSON key order reported the entity as changed.New
equal.tscompares structurally:Set/Mapby contents,Dateby timestamp, typed arrays andArrayBufferbytewise,RegExpby source and flags, nested objects key-by-key. Arrays stay order-sensitive. Its traversal mirrorsfreeze.ts's, so the two agree on which shapes exist.Union discriminant assumed a single-valued literal
union.tsread.value, which onlyz.literal("a")has. An enum member registered underundefined:Four distinct problems: the two halves of the same public API disagreed; a payload missing the discriminant was captured by whichever member held the
undefinedkey, defeating the dispatch design the docstring describes; the message rendered that key as empty (note the trailing"a",); and a multi-value literal threw inside a builder.Now reads the plural accessors (
.valuesfor literals,.optionsfor enums) and registers a member under every value it claims. A member whose discriminant yields no values registers under no key, so a malformed declaration reportsInvalid discriminantrather than silently capturing traffic.Three of the four schema members were one object
omitByreturned its argument for an empty key list, andoutputwasinputwith no computed fields. For an entity with neithergeneratednorcomputed:The design rule is "contracts compose the four plain
ZodObjects", so consumers key registries by schema identity — and registering the three under distinct ids kept only the last write, withz.toJSONSchemaemitting one$defall three$ref'd.contract.spec.tsmissed it because its fixture declares both options.The freeze broke
z.customcallersfreeze.tspromises az.custom/z.instanceofvalue is left alone, citing the exact risk that "the value may still be referenced by the caller who passed it in". It wasn't: dispatch was on runtime shape, andz.customhands the caller's reference straight back, so a plain-object one was deep-frozen and the caller's next write threw.The runtime genuinely cannot tell a passed-through object from decoded data — only the schema can.
entity.tsnow computes the skip set from the field schemas (z.custom,z.instanceofand a brandedz.customall reportcustom; brand is type-level in zod v4 and adds no wrapper).Verification
Full gate green:
format --check,lint,typecheck(three passes),test(138 passed, up from 122),knip,build.New tests:
equal.spec.tscovers bigint / Set / Map / nested-record / array-order / Date;union.spec.tsgains enum routing, input-and-make agreement, the missing-discriminant message, and multi-value literals;contract.spec.tsgains schema distinctness and registry independence;freeze.spec.tsgains thez.custompassthrough and confirms ordinary object fields are still frozen.Note on sequencing
Branched from
main, so it does not include #25. Both touchentity.ts— in different regions (equals/omitByhere,construct/extend/options there) — so whichever merges second needs a small rebase.🤖 Generated with Claude Code