Skip to content

fix(v4): parse recursive schemas built by a factory - #6530

Merged
colinhacks merged 21 commits into
mainfrom
fix-cycle-walk-factory-schemas
Sep 1, 2026
Merged

colinhacks merged 21 commits into
mainfrom
fix-cycle-walk-factory-schemas

Conversation

@colinhacks

@colinhacks colinhacks commented Aug 31, 2026 •

Copy link
Copy Markdown
Owner

Fixes #6526.

A recursive schema built by a factory throws RangeError: Maximum call stack size exceeded on 4.5.x, after about eight seconds on the main thread. It parsed fine on 4.4.3. Published schemas hit it — zod-geojson builds its geometry schemas this way, so an empty feature collection crashes.

const Node = (): any =>
  z.object({
    id: z.number(),
    get child() {
      return z.array(Node()); // a fresh instance on every read
    },
  });

z.object({ root: Node() }).parse({ root: { id: 1, child: [] } }); // RangeError

The cycle walk added in #6387 decides whether a parse can re-enter the same schema, so the memoizer can detach when no cycle is possible. Instance identity is the only thing that terminates that walk: it marks what it enters and stops when it meets a mark. A factory returns a new instance per call, so resolving one getter mints a whole new subtree, the walk never revisits a node, and it constructs schemas until the stack runs out. Descent at parse time was always data-directed and therefore finite, which is why 4.4.3 was unaffected.

The rule

The walk no longer resolves anything that runs user code, and reports a cycle for every edge it therefore cannot follow — a shape getter, or a z.lazy nobody has resolved. That terminates by construction and needs no depth limit. It errs in one direction only: it can report a cycle that does not exist, never the reverse. The same rule already governs compile.ts, which treats "can't tell" as recursive.

Left alone, that would pin the memoizer to every non-cyclic getter and z.lazy — about 2.3x on a small object parse. So the conservative answer is neither cached nor latched. The first parse resolves the getters along its path and z.lazy stores its inner type on the def, and one recheck then reads plain values and answers exactly. A forward reference detaches and runs at full speed; a factory never resolves into a finite graph, so it keeps the wrapper.

The object builders

Every builder that derives a shape — .extend(), .safeExtend(), .merge(), .partial(), .required(), .pick(), .omit() — answered shape from an accessor of its own that reads the source shape, which runs the source's getters. A factory chained through any of them overflowed the same way, and nothing could read a derived shape's keys without triggering that.

Each builder now writes a plain shape whose descriptors mirror the source's. A key the source has resolved is copied through as a data property, so the derived shape states outright what it holds; only a key the source still defers stays deferred, and it reads back through the source's own shape, so it resolves once and both shapes get that one schema. Every object def therefore has a shape the walk can read without running anything, and the walk keeps a single rule.

That is more precise than asking what a derived shape was built from, which can only over-approximate: Node.omit({ self: true }) drops the key carrying the cycle, and the mirrored shape has nothing deferred left in it, so it is exact before anything parses.

.strict(), .loose(), .passthrough(), .strip() and .catchall() had a second copy of the same bug, unrelated to the walk. They rebuilt the def with { ...def }, and spreading reads shape. On a factory schema that resolves it mints a fresh subtree whose own .strict() spreads again, so the call overflowed at construction, before any parse — on main as well. They copy the descriptor now and leave the shape unread.

An object resolves its shape by object spread, so a non-enumerable entry is no part of it. The builders enumerate the source with Reflect.ownKeys, so they skip those entries too — copying one back as enumerable would promote a hidden key into the derived shape as a required one, and .extend() would then reject input its source accepts. Enumerable symbol keys are unaffected. That filter belongs to z.object alone: z.properties reads every own key, so a non-enumerable edge there is still parsed and the walk still has to see it.

A derived shape is built key by key, and a plain assignment runs whatever setter answers to that key, so a key is written with defineProperty whenever anything already does — an accessor this shape deferred, __proto__, or a setter prototype pollution put there. Without that last case a polluted prototype dropped the key from the derived shape entirely, and .extend() then accepted a value its source validates. The check costs about nothing: the builders stay within 3% of an unguarded assignment, where defining every key outright costs up to 23%.

One behavior change falls out of this. A mask key the shape does not declare is now rejected at the .pick()/.omit()/.partial()/.required() call rather than at the first read of .shape, since the check reads the source's declared keys without resolving them. The error is the same; it arrives where the mistake is.

Three axes

Bundle, gzipped, against the base of this branch: classic +344 B on the object fixture, zod/mini +1 B on its object fixture and unchanged on boolean. Mini is the one that matters most here and it stays flat, because none of this reaches a bundle that only builds an object and parses it.

Steady-state parse is untouched, and not just within noise: the fastpass generated for z.object({ a, b, c }) is byte-identical to the base's, and the memoizer wrapper still detaches after exactly one entry. Timed, it is 0.99x.

What moves is construction, and only for the builders. Building an eight-key object is 0.99x. Building a derived schema and reading its shape — the whole of the work, wherever it now happens — is 1.08x for .pick(), 1.16x for .partial(), 1.33x for .merge() and 1.34x for .extend(). Called without ever reading the shape, .partial() is 3.7x and retains 7.8 KB instead of 2.2 KB, because it now builds its wrappers up front; the same partial cost 7.3 KB once used before and 7.6 KB now.

Deferring each wrapped key behind a getter instead was measured and is worse on both counts: a closure and an accessor pair cost about what the wrapper they avoid costs.

Descriptors were diffed across nineteen builder cases, cold and warm, against the base. The only change is that a derived def's shape is writable: false once read, which is what a plain z.object() has always been — the builders were the odd ones out.

isRecursive() recognizes a cycle by instance identity, but a recursive
schema built by a factory hands back fresh instances on every call: the
self-reference inside its shape getter is a new factory invocation, so
resolving the getter mints a whole new subtree at every level. The walk
never revisits a node and descends until the stack overflows — inside
schema construction, since each level constructs before it recurses.
Parsing such a schema worked on <=4.4.3, where descent was data-directed
and therefore finite.

Treat walk depth past 256 as recursive. A false positive only leaves the
memoizer attached, which is what a genuinely recursive schema gets, and
the short-circuit in check() propagates the answer straight up, so the
exponential fan-out across union options dies with the overflow.

Regressed in #6387, released in v4.5.0. Fixes #6526
Copilot AI lite review requested due to automatic review settings August 31, 2026 17:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The fix is right and the tests are real — I reverted the one line and confirmed all three fail with RangeError. But the new comment's "a false positive only leaves the memoizer attached" claim doesn't hold: past the cap an acyclic schema also starts deduping output by input identity, which flips a contract the suite already pins.

Reviewed changes — the one-line depth cap in the cycle walk, its three new tests, and the unrelated docs commit riding along.

  • MAX_DEPTH = 256 cap in isRecursive — treats walk depth past 256 as a cycle, so a factory-built recursive schema terminates instead of descending until the stack overflows. Correctly placed after the recursive WeakMap lookup and the stack.has check, so exact answers still win.
  • Three tests in cyclic-data.test.ts — a factory-built object, a factory-built discriminated union at the root and nested, and a below-cap exactness check.
  • github-star-pill.tsx — unrelated docs tweak bumping the GitHub mark to 20px (commit 535d5e8).

ℹ️ The cap bounds walk depth but not walk cost, and the issue's reporters will feel that

Because check only short-circuits once a subtree returns true, finite sibling subtrees are re-walked at every level, and the walk constructs schemas as it reads each getter. The total is linear in MAX_DEPTH × (finite subtree size) rather than exponential, so the bound genuinely holds — but on a realistic 7-member GeoJSON union I measured ~20 ms per fresh factory instance on first parse (vs ~0.004 ms once the instance is reused). That's an enormous win over the ~8 s hang, and nothing here needs changing in the code. It's worth saying on #6526 though, since anyone calling Geometry() per request rather than hoisting it to module scope will pay that 20 ms on every request and may read it as the bug not being fixed.

Technical details
# Residual per-instance walk cost after the cap

## Measured
- Real 7-member GeoJSON-shaped factory union: ~18-25 ms for the first parse of each fresh `Geometry()` instance; ~0.004 ms/parse once that instance is reused.
- Stack headroom is comfortable: ~638 frames consumed when the cap fires (~2.5 frames/level), against ~12500 available. No overflow risk at 256.

## Required outcome
- No code change. When closing #6526, mention that the factory should be called once and the resulting schema reused, since the walk re-runs per instance.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Claude Opus | 𝕏

Comment thread packages/zod/src/v4/core/memoizer.ts Outdated
Comment thread packages/zod/src/v4/classic/tests/cyclic-data.test.ts Outdated
@pullfrog

pullfrog Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Your Claude subscription has hit its usage limit. It resets at 9:10pm (UTC). Re-trigger Pullfrog after the reset, or add an ANTHROPIC_API_KEY repo secret — Pullfrog routes around an exhausted subscription automatically when one is present.

Add repo secret → · Model settings → · Setup docs → · Ask in Discord →

Pullfrog  | Rerun failed job ➔ | View workflow run | via Pullfrog | Using Claude Opus | 𝕏

The comment claimed a false positive only leaves the memoizer attached;
it also dedupes output by input identity past the cap and makes
z.compile decline the schema. And the precision test sat 5x below the
cap, so it pinned nothing — assert false at 255 wrappers and true at
256 instead.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Both prior threads are properly addressed. I re-measured every claim the new comment now makes rather than taking it on faith, and the boundary test's arithmetic checks out exactly.

Reviewed changes — the follow-up commit c05450b on top of the previously reviewed fb47e14.

  • Narrowed the MAX_DEPTH comment to what a false positive actually costs — I confirmed the new wording against the runtime: an acyclic chain at depth 300 reports isRecursiveSchema === true and aliases a doubly-referenced input to one output object, and z.compile(…, { strict: true }) throws ZodCompileUnsupportedError. Depth 10 and depth 200 do neither. The comment is now accurate on both halves.
  • Rewrote the boundary test to pin the constant — nest(255) → false and nest(256) → true. The off-by-one reads right too: each z.object level is exactly one node, so the leaf enters the walk at stack.size === n and 255 is the last depth that clears >= 256. Verified both independently. This pair now fails if MAX_DEPTH moves in either direction, which the old 50-level check did not.

The full cyclic-data.test.ts file passes at 98 tests across the zod, compile-mode and TS projects, and pnpm check:comments is clean.

ℹ️ The PR description still carries the claim the code comment just retracted

The body's second paragraph says "A false positive is harmless — it leaves the memoizer attached, which is exactly what a genuinely recursive schema gets." That is the wording memoizer.ts moved away from in this commit, and it's the paragraph most likely to get quoted into the changelog or onto #6526. Worth a one-line edit so the two don't disagree.

Technical details
# PR description contradicts the corrected `MAX_DEPTH` comment

## Affected sites
- PR #6530 body, paragraph 2 — "A false positive is harmless — it leaves the memoizer attached, which is exactly what a genuinely recursive schema gets." No longer true past the cap, and not editable from the diff.
- `packages/zod/src/v4/core/memoizer.ts:37` — the corrected version: "an acyclic schema nested this deep keeps the memoizer attached, so it dedupes output by input identity and `z.compile` declines it."

## Required outcome
- The PR body reflects the same cost the code comment now states, so the merge commit message and any quote onto #6526 don't reintroduce the retracted claim.

## Open questions for the human
- When closing #6526, worth adding that the factory should be called once and the resulting schema reused — the walk re-runs per fresh instance at roughly 20 ms on a GeoJSON-shaped union, so a caller invoking `Geometry()` per request may read the residual cost as the bug not being fixed.

Pullfrog  | View workflow run | Using Claude Opus | 𝕏

… depth cap

The 256 cap bounded the walk but was arbitrary, and it silently flipped
acyclic schemas past it to memoized parsing. What every level a factory
mints shares is the getter that minted it: a walk path that re-enters a
getter whose source it is already resolving can only have been produced
by a loop in user code, so treat the repeat as a back-edge and stop
resolving. Instance identity still catches hoisted cycles first.

The first def.shape read spreads every user getter away into plain
values, so the walk fingerprints them from the raw pre-spread shape
(exposed by rawShape()) before triggering resolution, and only resolves
when no source repeats. z.lazy chains fingerprint def.getter the same
way.

A factory schema is now flagged at its second getter instead of after
256 constructed levels, deep acyclic nesting stays exact at any depth,
and a lone forward-reference getter keeps its unmemoized parse.
@pullfrog

pullfrog Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Your Claude subscription has hit its usage limit. It resets at 10pm (UTC). Re-trigger Pullfrog after the reset, or add an ANTHROPIC_API_KEY repo secret — Pullfrog routes around an exhausted subscription automatically when one is present.

Add repo secret → · Model settings → · Setup docs → · Ask in Discord →

Pullfrog  | Rerun failed job ➔ | View workflow run | via Pullfrog | Using Claude Opus | 𝕏

…echeck

Replaces both earlier attempts. The depth cap was an arbitrary constant,
and fingerprinting a getter by its source text is wrong: a helper that
builds a chain emits the same text at every level, so an acyclic
`wrap(wrap(z.string()))` was reported as a cycle.

Identity is the walk's only termination argument and a deferred edge
need not answer with the same schema twice, so the walk now resolves
nothing that runs user code and reports a cycle for every edge it
therefore can't follow. That is the rule compile.ts already states for
the same reason ("treat can't tell as recursive"), and it also drops the
walk's own getter invocation.

Assuming a cycle would otherwise pin the memoizer to every non-cyclic
getter or z.lazy schema, which measured ~2.3x on a small object parse.
So an assumed answer is not cached and not latched: the first parse
resolves the getters along its path, and one recheck then settles the
question exactly — a forward reference detaches and runs at full speed
(0.1% vs an inline-written equivalent), while a factory never resolves
into a finite graph and keeps the wrapper for good.
@pullfrog

pullfrog Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Your Claude subscription has hit its usage limit. It resets at 10pm (UTC). Re-trigger Pullfrog after the reset, or add an ANTHROPIC_API_KEY repo secret — Pullfrog routes around an exhausted subscription automatically when one is present.

Add repo secret → · Model settings → · Setup docs → · Ask in Discord →

Pullfrog  | Rerun failed job ➔ | View workflow run | via Pullfrog | Using Claude Opus | 𝕏

Treating a def whose `shape` is its own accessor as a cycle closed the
last overflow path — a factory chain built through .extend()/.merge()/
.partial()/.pick() — but those builders install that accessor on every
object they produce, so every extended object read as a cycle while
cold and z.compile refused it. compile-differential caught four.

Read `def.shape` there as before. A factory routed through a builder
still overflows, which it does on main too and is outside what #6526
reports; the note on rawShape() records the shape of the real fix.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Resolved acyclic z.lazy schemas never leave the conservative state, so this changes output identity and compilation behavior beyond recursive factories.

Reviewed changes — the three commits since the prior Pullfrog review replaced the depth cap with deferred-edge handling and then narrowed its interaction with object builders.

  • Replaced the depth cap and getter-source fingerprinting — cold object getters and z.lazy edges are now treated conservatively without invoking user code, with one post-parse recheck intended to recover an exact answer.
  • Exposed unresolved object shapes structurally — rawShape() lets the cycle walk inspect user getter descriptors before def.shape spreads them into values.
  • Kept builder-produced shape accessors on the prior path — ordinary .extend() and related builders remain compilable, while factory recursion routed through a builder remains explicitly out of scope.
  • Expanded cycle-walk coverage — tests now exercise getter and lazy factories, cold conservative answers, post-parse getter resolution, and deep acyclic schemas.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread packages/zod/src/v4/core/memoizer.ts Outdated
Comment thread packages/zod/src/v4/classic/tests/cyclic-data.test.ts Outdated
@colinhacks

Copy link
Copy Markdown
Owner Author

@pullfrog review

The object path recovers exactness once a parse resolves its getters,
but the lazy case assumed a cycle unconditionally, so an acyclic z.lazy
kept the memoizer for good and z.compile went on refusing it. $ZodLazy
caches its inner on the def, so follow that when it is there — no
getter invocation, and a cold lazy stays conservative as before.

Also drops a test comment still describing the removed depth cap.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The unresolved z.lazy thread still blocks this SHA: resolving _cachedInner never changes the cycle walk's answer, so acyclic lazy-containing objects permanently dedupe repeated input references and remain ineligible for strict compilation. The test comment also still refers to the removed depth cap.

Reviewed changes — all six commits on fix-cycle-walk-factory-schemas, including the current deferred-edge implementation and its tests.

  • Deferred-edge cycle detection — cold shape getters and z.lazy edges are treated conservatively without invoking them, then object schemas get one post-parse opportunity to recover an exact result.
  • Raw object-shape access — rawShape() exposes getter descriptors to the walk while deliberately leaving builder-produced shape accessors on the prior path.
  • Factory-recursion coverage — tests exercise object and discriminated-union factories, z.lazy, conservative cold answers, post-parse getter resolution, and deep acyclic graphs.
  • Star-pill styling — the unrelated docs commit enlarges the GitHub mark and adjusts horizontal padding.

The focused cyclic-data and compile suites pass (563 assertions across runtime, compile-mode, and type projects), and pnpm check:comments is clean. A direct acyclic-lazy probe still returns { firstAliased: true, secondAliased: true, strictCompiled: false, lazyResolved: true }, confirming the open blocker is not covered by those passing suites.

Pullfrog  | Fix it ➔ | View workflow run | Using GPT Sol | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed the follow-up commit since the prior Pullfrog review.

  • Made resolved lazy edges exact — followed $ZodLazyDef._cachedInner after resolution so acyclic lazy schemas detach the memoizer and remain compilable without invoking cold user code.
  • Covered resolved and genuinely cyclic lazy schemas — verified an acyclic lazy becomes non-recursive after parsing while a cached edge that closes a real cycle remains recursive.
  • Corrected the factory-recursion comment — replaced the removed depth-cap explanation with the current deferred-edge mechanism.

Pullfrog  | View workflow run | Using GPT Sol | 𝕏

Closes the rest of the class. The object builders answer `shape` from
their own accessor, which reads the source shape and so resolves the
user's getters — the walk had no way past that except to resolve it too,
which is how a factory chain through .extend()/.merge()/.partial()/
.pick()/.omit()/.required() still overflowed.

Each builder now records the schemas and shapes its result derives from.
The derived subtree is contained in theirs, so the walk answers from
those instead: a source schema it reaches by identity, a plain shape
whose descriptors it reads unresolved. An ordinary .extend() therefore
stays exact and still compiles — flagging the accessor itself did not,
which is what failed compile-differential before — while a factory stops
at the first deferred edge as everywhere else.

Steady-state parse is unchanged: +2.9% plain and +1.0% builder-derived
against the base, both inside the noise of a loaded machine, measured
one schema per process after a shared harness went megamorphic and
invented a 20% swing.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new builder provenance is still incomplete: generic clones can lose it, and builders that remove or replace an edge inherit cycles that are no longer in their derived shape.

Reviewed changes — reviewed the builder-cycle follow-up since the prior Pullfrog review.

  • Recorded derived object sources — added WeakMap provenance for .extend(), .safeExtend(), .merge(), .partial(), .required(), .pick(), and .omit() so the cycle walk can avoid invoking their unresolved shape accessors.

  • Expanded builder coverage — added factory-recursion cases for every object builder, exactness checks for ordinary derived schemas, and a real cycle that closes through .extend().

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread packages/zod/src/v4/core/util.ts Outdated
Comment thread packages/zod/src/v4/core/util.ts Outdated
…nswer

Asking what a builder derived its shape from over-approximates: .omit(),
.pick() and an overwriting .extend() can drop the very branch carrying
the cycle, and the walk was recording that inherited answer as proven,
so `Node.omit({ self: true })` stayed recursive for good — memoized, and
refused by z.compile, where main was exact.

Mark the provenance answer assumed, and read the builder's accessor once
it has self-cached. A derived schema is then conservative only while
cold, and the recheck replaces it with the resolved shape's exact
answer; a cycle that survives the builder is still found.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The follow-up makes removed or overwritten keys exact only after a parse. The existing clone-provenance thread is unchanged, and cold derived schemas still reject strict compilation, so both prior blockers remain open.

Reviewed changes — reviewed the follow-up commit since the prior Pullfrog review.

  • Made builder provenance provisional — limited source-based over-approximation to unresolved builder accessors so a resolved final shape can replace it with an exact answer.
  • Covered post-resolution exactness — added omit, pick, and overwritten extend cases that become non-recursive and compilable after parsing, plus a retained-edge case that remains recursive.

Pullfrog  | Fix it ➔ | View workflow run | Using GPT Sol | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new lazy resolution can re-enter the cycle walk while its conservative state is module-global, allowing a nested compile to make an assumed result permanent.

Reviewed changes — reviewed the merge of current main and the one-hop lazy-resolution follow-up since the prior Pullfrog review.

  • Merged cold-lazy compiler coverage — brought in main's z.compile(z.lazy(() => z.string())) regression test.
  • Resolved one cold lazy edge for compilation — let isRecursiveSchema() invoke the first unresolved lazy getter, then disabled further lazy resolution along that edge to stop factory-generated chains.
  • Defined the conservative boundary — updated cycle tests so a direct acyclic lazy is exact while a nested unresolved lazy remains conservative.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread packages/zod/src/v4/core/memoizer.ts
The assumed/proven distinction rode on a module-level boolean, correct
only because the walk is synchronous and never re-enters — an invariant
nothing in the file states and nothing would catch.

Return it instead: none, assumed, proven, ordered so the strongest
answer among children wins. The shape walk returns its answer rather
than writing the caller's variable, and caching moves into settle(),
where the value arrives as a parameter. No behavior change.
Resolving a lazy runs user code, and that code can call z.compile and
re-enter the walk. While certainty lived at module scope the inner walk
reset it, and an omitted recursive lazy stayed recursive for good.

Fails against the flag, passes since it became a return value.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new certainty propagation fixes the re-entrant lazy blocker. The two earlier builder-provenance threads remain open, so this SHA is not ready to merge.

Reviewed changes — reviewed the certainty-state refactor added since the prior Pullfrog review.

  • Scoped certainty to each recursive walk — replaced the module-global assumed flag with ordered NONE, ASSUMED, and PROVEN return values, preventing nested compilation from mutating an outer walk's state.
  • Kept provisional answers out of the cache — centralized exact-answer caching in settle() while allowing independently proven cycles to override assumed sibling edges.

Pullfrog  | Fix it ➔ | View workflow run | Using GPT Sol | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new regression test does not execute the nested walk while the derived schema's answer is still provisional, so the pre-fix implementation still passes it. The two earlier builder-provenance threads also remain open.

Reviewed changes — reviewed the regression test added after the immediately preceding Pullfrog review.

  • Added an omitted-lazy re-entrancy scenario — made the lazy callback invoke strict compilation and checked the derived schema's recursion result, compilation eligibility, and shared-output identity.
  • Retained a genuine-cycle assertion — confirmed the source schema still reports recursive after its lazy edge resolves.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

});

const omitted = Node.omit({ self: true });
omitted.parse({ id: 1 });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing here resolves the omitted shape before any resolve: true walk can enter the lazy callback, so the old module-global implementation passes this entire test. Run a cold cycle walk first so the nested z.compile() executes while the builder-source answer is still provisional.

Suggested change
omitted.parse({ id: 1 });
expect(z.core.isRecursiveSchema(omitted)).toBe(true);
omitted.parse({ id: 1 });

…scriptors

A derived shape was rebuilt behind a def-level accessor, so nothing could
read its keys without running the source's getters — which is how a factory
schema built an unbounded chain. The cycle walk worked around that with a
registry of what each derived shape drew from.

Each builder now writes a plain shape whose descriptors mirror the source's:
a key the source resolved is copied through, and only a key it still defers
stays deferred, reading back through the source's own shape so it resolves
once. Every object def therefore has a raw shape, and the walk keeps one
rule — an unresolved getter is assumed to close a cycle.

The registry and its seven call sites are gone. A def's shape accessor now
carries the shape it answers from, so a clone that copies the accessor
(.strict(), .catchall(), .meta()) inherits it instead of losing it.
`.strict()`, `.loose()`, `.passthrough()`, `.strip()` and `.catchall()`
rebuilt the def with `{ ...def }`, and spreading reads `shape`. On a schema
whose shape is built by a factory that resolves it mints a fresh subtree,
and each one is spread again, so the call overflowed the stack before any
parse. `mergeDefs` copies the descriptor instead, leaving the shape unread.

The clone then carries the source's shape accessor, which is why that
accessor has to know what it answers from.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new descriptor mirroring promotes non-enumerable shape entries into validated properties, so derived schemas can reject inputs their source accepts.

Reviewed changes since the prior Pullfrog review covered the two-commit builder-provenance rewrite.

  • Mirrored final builder shapes — Replaced source-level cycle provenance with descriptor mirroring for retained, omitted, wrapped, merged, and overlaid keys.
  • Carried unresolved shapes through clones — Taught object construction to adopt a copied shape accessor's raw source and changed catchall-style clones to preserve descriptors without resolving shape.
  • Expanded exactness coverage — Added cold compilation checks for removed and overwritten recursive edges, generic clone cases, and immediate unknown-mask validation.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread packages/zod/src/v4/core/util.ts
A shape resolves by object spread, so a non-enumerable entry is never part
of the shape a schema parses. The builders enumerated the source with
`Reflect.ownKeys` and wrote every key back as enumerable, which promoted
such an entry into the derived shape as a required key — `.extend()` then
rejected input the source accepts, and `.pick()` took the hidden key
instead of calling it unrecognized. Only visible while the source's own
shape was still unresolved, since resolving drops the entry anyway.

Enumerable symbol keys are unaffected; the walk skips these entries too,
since nothing can ever parse them.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new assignment fast path can drop ordinary shape keys when an inherited setter intercepts the write.

Reviewed changes since the prior Pullfrog review covered the construction optimization in 144878eb.

  • Optimized settled-key mirroring — Replaced unconditional Object.defineProperty calls with assignment until a deferred getter or __proto__ requires a descriptor-safe write.
  • Threaded deferred state across overlays — Propagated whether a mirrored shape installed a getter so later overlapping keys still replace accessors correctly.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread packages/zod/src/v4/core/util.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Filtering non-enumerable keys in the shared shape walker makes $ZodProperties miss recursive edges that its parser still follows.

Reviewed changes since the prior Pullfrog review covered the non-enumerable object-shape fix in 5e39e6da.

  • Aligned object builders with spread semantics — Skipped non-enumerable source and overlay entries during mirroring and rejected hidden keys in builder masks.
  • Covered hidden entries across builders — Added source, extension, partial, merge, pick, parse, and enumerable-symbol assertions.
  • Filtered hidden edges in cycle detection — Applied the same descriptor check to the shared walker used by object and properties schemas.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread packages/zod/src/v4/core/memoizer.ts Outdated
for (const key of Reflect.ownKeys(sh)) {
const desc = Object.getOwnPropertyDescriptor(sh, key)!;
// a shape resolves by object spread, so a key it does not enumerate is never parsed
if (!desc.enumerable) continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This filter is valid for object, whose shape is spread before parsing, but shape() is also used by case "properties" below and $ZodProperties.runShape() snapshots every Reflect.ownKeys(def.shape) entry. A non-enumerable next: z.lazy(() => Node.optional()) makes isRecursiveSchema(Node) return false and Node.parse(cyclicInput) throw RangeError, so cycle detection must preserve $ZodProperties' actual key semantics.

Technical details
# Hidden properties edges bypass cycle detection

## Affected sites
- `packages/zod/src/v4/core/memoizer.ts:70` — the shared shape walker skips non-enumerable entries for both object and properties schemas
- `packages/zod/src/v4/core/schemas.ts:5022` — `$ZodProperties` parses every own string and symbol key without an enumerability filter

## Required outcome
- Object recursion walks continue to ignore entries discarded by object spread.
- Properties recursion walks include every entry that `$ZodProperties.runShape()` parses.
- Coverage puts a recursive schema under a non-enumerable properties key and proves cyclic input parses without overflowing.

## Suggested approach
- Apply the enumerability filter at the object call site rather than changing the shared properties-shape walk.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The existing re-entrancy coverage thread remains open: the new assertion proves cold omitted-shape exactness but never invokes the nested walk.

Reviewed changes since the prior Pullfrog review covered the test-only follow-up in fed6f8c6.

  • Asserted cold omitted-shape exactness — Checked isRecursiveSchema(omitted) === false before any parse resolves the schema.
  • Retained post-parse behavior checks — Continued to verify strict compilation and distinct outputs for repeated shared input.

Pullfrog  | Fix it ➔ | View workflow run | Using GPT Sol | 𝕏

Writing a settled key with a plain assignment runs whatever setter answers
to that key. `__proto__` has one on every object, which was special-cased,
but prototype pollution can supply one for any name — and then the key was
dropped from the derived shape entirely, so `.extend()` silently accepted
a value its source validates.

Falling back to `defineProperty` when anything already answers to the key
covers the inherited case, the `__proto__` case and a key this shape has
already deferred, so the deferred flag the builders threaded is gone. An
`in` check costs about nothing: the builders stay within 3% of the
unguarded assignment, where defining every key outright costs up to 23%.
An object resolves its shape by spread, so a key it does not enumerate is
never parsed and the walk skips it. `z.properties` reads every own key
instead, so the same filter hid a real edge there: a non-enumerable
`z.lazy` self-reference read as acyclic, the memoizer detached, and a
cyclic input overflowed the stack.

The filter now belongs to the object case that earns it.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The inherited-setter blocker is addressed cleanly, but this SHA still has unresolved Pullfrog threads and is not ready to merge.

Reviewed changes since the prior Pullfrog review covered the single follow-up commit.

  • Hardened settled-key mirroring — Replaced deferred-state tracking with a structural key in target guard, so existing own accessors and inherited setters use assignProp() while truly absent keys retain the assignment fast path.
  • Pinned inherited-setter behavior — Added a regression that installs an Object.prototype setter and proves the derived shape retains and validates the intercepted key without invoking it.

Pullfrog  | Fix it ➔ | View workflow run | Using GPT Sol | 𝕏

@colinhacks
colinhacks merged commit cafbee4 into main Sep 1, 2026
10 checks passed
@colinhacks
colinhacks deleted the fix-cycle-walk-factory-schemas branch September 1, 2026 01:54
@todor-a

todor-a commented Sep 8, 2026

Copy link
Copy Markdown

While profiling zod/compile on 4.5.4, I found another memoizer symptom that no longer reproduces on current main (804e0f5): non-recursive async parsing can alias repeated input references and skip getter reads.

require("zod/compile");
const z = require("zod");

let reads = 0;
const item = { get value() { return ++reads; } };
const schema = z.array(z.object({ value: z.number() }));

(async () => {
  for (let i = 0; i < 2; i++) {
    const result = await schema.parseAsync([item, item, item]);
    console.log(result.map(x => x.value), result[1] === result[2]);
  }
})();

On npm 4.5.4 / Node 24.18.0 this prints [1, 2, 2] true, then [3, 3, 3] true. Without the global import, and against current main source with it, it prints [1, 2, 3] false, then [4, 5, 6] false.

In the released code, the compiler shim retains the memoizer wrapper. That wrapper's non-recursive early return only runs while isRecursiveInst is undefined; later calls with cached false fall through into memoization. A local control patch that always delegates to base when it is false restores output/reference parity and preserves a genuinely cyclic-data control.

This PR's memoizer rework appears to cover that retained-wrapper path already. Posting the reproduction for regression/release coverage rather than proposing a duplicate fix. npm latest is still 4.5.4 at the time of testing.

zdila added a commit to FreemapSlovakia/freemap-v3-react that referenced this pull request Oct 5, 2026
zod 4.6.5 carries the cycle fix (colinhacks/zod#6530) and is pinned exactly;
GeoJSONFeatureCollectionSchema parses an empty collection in ~2 ms.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
Preview – zod-v4 — e48a0055 Deployed Sep 1, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression in 4.5.0: nested recursive discriminatedUnion throws Maximum call stack size exceeded

3 participants