Skip to content

refactor(downgrader): count a schema met again mid-cycle as loose, instead of converting again - #319

Merged
dinwwwh merged 16 commits into
mainfrom
claude/project-thread-h3q238
Nov 1, 2026
Merged

dinwwwh merged 16 commits into
mainfrom
claude/project-thread-h3q238

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Nov 1, 2026

Copy link
Copy Markdown
Member

Requested by dinwwwh · project thread

What

Simplifies the cycle handling that #309, #316 and #317 added, following the request for the simplest approach with minimal logic: when there is a choice, drop a restriction rather than add logic.

The problem those PRs solved: around a cycle, a not or oneOf can read a schema before its conversion is done, so it does not know yet whether that schema lost a restriction. They fixed it by recording what each read saw, then converting the whole input again (#309), following alias chains and object cycles to find those outputs (#316), and skipping the second pass when it could not change anything (#317).

This PR replaces all of that with one rule: a schema met again before its conversion is done counts as loose.

  • A $ref kept as written whose target is still being converted (inline returns DROP for a target that exists) is loosened.
  • An object met again before finishSchema marked it done (new DONE set, read in revisitSchema) is loosened.

Removed: keptStrict, refOutputs, looseTargets, revisits, looseInputs, sharedOutput, aliasRef, markLooseTargets and the second-pass loop. The revisit callback in convertObject now gets only the output. Net: +27/−118 lines.

README: the not row now says a recursive schema counts too, and the performance note drops the second pass.

Tests

One expectation changed: a oneOf over a $ref into a pure $ref cycle (A → B → A) now becomes anyOf. The cycle tests from #309 and #316 pass unchanged. pnpm check, pnpm test and coverage (100%) pass.

Tradeoffs

  • Output. Every recursive schema now counts as loose, also one that loses nothing, such as a plain tree component. A not over one is removed, and a oneOf with one as a branch becomes anyOf unless its branches are told apart by type or by a required tag. A recursive schema through $defs already counted as loose before this, since recursion there is cut to {}. Code generators model anyOf as a different union type than oneOf.
  • Speed. No second pass at all now. Under callgrind with CodSpeed's V8 flags, one warmed conversion of the order schema goes from 1,356,928 to 1,328,874 instructions (−2%), and the 10-resource generated API from 17,622,930 to 17,878,067 (+1.4%, one WeakSet add per schema).

Generated by Claude Code

claude added 16 commits November 1, 2026 05:44
… no oneOf branch apart, and takes null

Where several $refs enter the same cycle, the first copy made is reused, so
a later one can hold a $ref that was cut to {} while another target was
being inlined. isDisjoint and the nullable pair merge read such a $ref in
the input, saw its target's type or tag, and kept a oneOf that the {} then
let a value match twice. The holders of cut $refs are now recorded, and
follow() and throughRef() read them as keeping nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…operties beside other properties, out of enum merging

A oneOf rejects a value two of its branches share, so mergeEnums keeps a
oneOf whose literals repeat one. It compared them serialized, which tells
equal objects with their keys in another order apart, so it merged such a
oneOf into an enum that took the shared value. A oneOf of object or array
literals now stays as it is. And an additionalProperties in a literal branch
counted as repeating the one around it although that one sits beside other
properties, which it then let through, so it repeats only beside the same
properties.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…side annotations

enumNames read the names at the end of the propertyNames $ref chain, past a
$ref with an enum, maxLength or other keyword beside it that narrows the
names further. The record then got a property for each name of the last
enum, allowing keys the input rejects, and counted as converted exactly, so a
not or oneOf over it stayed. The chain now stops at such a $ref, so the
record loses its key names and counts as loose.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…iction around an alias component or a cycle of objects

#309 converts the input again when a not or oneOf read a $ref's target
before that target lost a restriction around a cycle. It missed two cycles.
A component that is only a $ref, an alias, has no output of its own, so the
output recorded for it was undefined; it now records the output of the target
its chain of kept $refs ends at. And a dereferenced document closes a cycle
with the object itself rather than a $ref; convertObject now tells
convertSchema when it meets a schema again, and one that was not loose then
but is once converted makes the next pass read it as loose. First visits pay
nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…ed-union $refs

The rerun recorded nothing for an alias whose chain passes through a $ref the
conversion inlines, such as one into $defs, or that is a $ref to a tagged union
beside only annotations, so a not over it around a cycle was kept over a target
that lost a restriction. sharedOutput now follows every $ref convertSchema
treats as an alias, and reads an inlined hop's output from its inlined copy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…form

A schema that loses a restriction around a cycle made the conversion run
again, so a not or oneOf that read it too early could drop or widen. Only
such a choice changes when more schemas read as loose, so a pass that kept
no not, and no oneOf without a loose branch, needs no other. A document with
a recursive schema that loses a restriction and no not or oneOf now converts
in one pass, about half the time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…y property as loose

OAS 3.0 applies a required readOnly property to responses only and a
required writeOnly property to requests only, while 3.1 requires it both
ways. The 3.0 output so loses that restriction, so a not or oneOf over
such an object must not keep it, and the property tells no oneOf branch
apart. The property is read through a chain of $refs too, as 3.0 reads
each target in its place.

Such a schema in a cycle, as a recursive DTO with a required readOnly id,
makes a document that keeps a not or oneOf convert twice, which the README
performance note now says.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
… or a set per name

CodSpeed read the order schema bench 19% slower with the check. It now loops
over `required` once per schema, reads each property's fields directly, and
makes a visited set only past the first `$ref`. Instructions per conversion of
that schema drop from 4% over main to 2% under callgrind with CodSpeed's V8
flags. A cycle of `$ref`s, which never reaches a schema, is tested too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
Marking the schema itself loose lets a $ref to it, and every object around
it, read as loose through the existing checks, so no walk over required
names is needed. Only the tag check still reads through $refs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
…stead of converting again

A $ref or object cycle that reaches a schema before its conversion is done
cannot know yet what that schema loses, so it now counts as loose. This
replaces the second pass (#309, #316) and its skip (#317) with two checks,
at the cost of a recursive schema always counting as loose: a not over it is
removed, and a oneOf with it becomes anyOf unless its branches are disjoint.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7NmVyQoYtVBNPuE4uV
@dinwwwh dinwwwh self-assigned this Nov 1, 2026
@codecov

codecov Bot commented Nov 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Nov 1, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 59.36%

2 warnings

⚠️ 1 benchmark is dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ 4 benchmarks with a significant change ran on different hardware

Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.

Learn more about Macro Runners

See affected benchmarks

❌ 4 regressed benchmarks
✅ 25 untouched benchmarks
⏩ 2 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ property schema 127.4 µs 1,030.8 µs -87.64%
❌ dereferenced diamond, 64 levels 958.5 µs 1,875.6 µs -48.9%
❌ order schema with $defs 1.4 ms 2.3 ms -39.59%
❌ reference diamond into removed webhooks, 64 levels 2.3 ms 3.2 ms -28.52%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/project-thread-h3q238 (6de99ef) with main (e663c22)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

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 — the whole net diff of this PR against main, verifying the single-pass cycle rule replaces the old fixpoint without under-marking any schema as loose.

  • v3.1-to-v3.0.ts — new mid-cycle rule. Added DONE (WeakSet of converted outputs); finishSchema marks an output done at the end, and revisitSchema(out) adds any output met again before that to LOOSE. The kept-$ref path now returns isLoose(converted) || (converted === DROP && target !== undefined) — a DROP from inline with an existing target is a cycle (ctx.inlining), so it is loosened; a dangling target === undefined is not.
  • v3.1-to-v3.0.ts — fixpoint removed. downgradeV30 runs the conversion once; keptStrict, refOutputs, looseTargets, revisits, looseInputs, sharedOutput, aliasRef, markLooseTargets and the second-pass loop are gone, and aliasRef is inlined into convertSchema. finally clears only the remaining per-pass state.
  • shared.ts — convertObject's revisit callback now takes only the output ((out) => void) rather than (value, out).
  • Tests / README — the oneOf over a pure $ref cycle (A → B → A) now expects anyOf; the README not row and performance note are updated for the removal of the second pass.

I traced the soundness direction (over-marking drops a not / widens oneOf to anyOf; under-marking is what breaks the contract) and found no under-marking: any read of an in-progress output goes through convertObject's memoized-hit branch into revisitSchema, so the removed per-target propagation is subsumed in one pass; reads of a finished output have final looseness. isLoose is consumed only by loosen, convertTuple's exactness, and convertSchemaRef propagation, all of which stay on the safe side. No stale references to the removed identifiers remain. oxlint/oxfmt/tsc are clean and v3.1-to-v3.0.test.ts passes 130/130.

The one thing worth remembering for follow-ups is the deliberate widening called out in the PR body: every recursive schema now counts as loose, including ones that lose nothing, so a not over a recursive tree is dropped and a oneOf with a recursive branch becomes anyOf unless its branches are told apart. That is consistent with the converter's established "loses detail, never meaning" contract and is documented in the README.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

dinwwwh commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

On the CodSpeed result: all four flagged benchmarks carry CodSpeed's own "ran on different hardware" warning, and the numbers do not reproduce. Measured locally the way the CodSpeed plugin does (CodSpeed's V8 flags, 7 warm-up calls, gc(), then one call counted under callgrind), in instructions, main vs this head:

Benchmark main head
property schema 101,656 100,741
order schema with $defs 2,141,505 2,190,336
dereferenced diamond, 64 levels 1,734,036 1,776,566
reference diamond into removed webhooks, 64 levels 4,328,979 4,359,230

That is −1% to +2.5%, from the one WeakSet add per schema, against the −28% to −88% CodSpeed reports.


Generated by Claude Code

@dinwwwh
dinwwwh merged commit 6c595c5 into main Nov 1, 2026
10 of 11 checks passed
dinwwwh added a commit that referenced this pull request Nov 1, 2026
…s still running (#328)

#319 marked every converted schema done in a WeakSet, so a schema met again before that counts as loose. Adding each output to a WeakSet made 3.1 to 3.0 about 45% slower than 0.5.0 on real oRPC documents.

convertObject now keeps the outputs whose conversion is running on a per-call stack in Context, and calls `revisit` only for one still on it. The output is the same: an output is on the stack exactly while it is not done.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VVaw7PYvCYrJkB3NF1rgbx
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.

2 participants