From 96a2e38fa382fd2c5d2ca5fc072537d7ea039527 Mon Sep 17 00:00:00 2001 From: Chinesezjc Date: Sun, 2 Aug 2026 15:24:10 +0800 Subject: [PATCH] fix(tools): detect render-phase cycles and fix class-name JSDoc placement Address ds-review-bot v5/v6 review round 6: - renderType tracks the active ancestor schemas by object identity (the frame stack is the DFS path). A stateful getter can mutate the graph after validation so a child returns an ancestor at render time; without this the walk pushed frames forever instead of degrading. A repeated ancestor now degrades to Any, honoring the never-throw contract; distinct nodes in a legitimately deep chain are different objects, so it stays O(1) per push and O(depth) memory. - The multiline allocateClassName JSDoc was still attached to the MAX_CLASS_NAME_BASE constant (a self-referential @link, and the function had no doc). Move the doc onto the function and give the constant its own one-liner. - Tests cover the post-validation cycle and a non-object render-time child; py-types.ts stays at 100% per-file coverage. --- packages/core/tools/src/py-types.ts | 30 ++++++++++++++++-- packages/core/tools/tests/py-types.spec.ts | 37 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/packages/core/tools/src/py-types.ts b/packages/core/tools/src/py-types.ts index 6e904a907c..7d2a89867f 100644 --- a/packages/core/tools/src/py-types.ts +++ b/packages/core/tools/src/py-types.ts @@ -122,6 +122,9 @@ function camelCase(raw: string): string { return /^[A-Za-z]/.test(joined) ? joined : `Tool${joined}` } +/** Class-name base cap keeping each emitted name — and total text — linear in schema depth. */ +const MAX_CLASS_NAME_BASE = 120 + /** * Reserve a unique class name from a base, suffixing `2`, `3`, … on collision. * The base is capped at {@link MAX_CLASS_NAME_BASE} first: child class names @@ -133,7 +136,6 @@ function camelCase(raw: string): string { * `2`, so a deep chain sharing one capped base stays O(1) per allocation * (amortized) instead of Θ(depth²) in time. */ -const MAX_CLASS_NAME_BASE = 120 function allocateClassName(base: string, state: RenderState): string { const capped = base.length > MAX_CLASS_NAME_BASE ? base.slice(0, MAX_CLASS_NAME_BASE) : base let name = capped @@ -220,6 +222,15 @@ function renderType(schema: unknown, className: string, state: RenderState): str const newFrame = (schema: unknown, className: string, validated: boolean): Frame => ({ schema, className, phase: 'start', children: [], childIndex: 0, childTypes: [], entries: [], validated }) const frames: Frame[] = [newFrame(schema, className, false)] + // Ancestor schemas by object identity — the frame stack IS the DFS path, so + // this set holds exactly the current node's ancestors. A stateful getter can + // mutate the graph after validation (an `items`/property that validated as a + // scalar but returns an ancestor at render time); without this, the walk + // would push frames forever. A repeated ancestor degrades to `Any` per the + // never-throw contract. Distinct nodes in a legitimately deep chain are all + // different objects, so this stays O(1) per push and O(depth) memory. + const activeSchemas = new Set() + if (typeof schema === 'object' && schema !== null) activeSchemas.add(schema) let result: string | undefined // The no-throw contract must hold across the WHOLE walk, not just the root // validation: a hostile stateful getter (a `type` that returns a scalar on @@ -231,7 +242,10 @@ function renderType(schema: unknown, className: string, state: RenderState): str /* jscpd:ignore-start -- the explicit-stack walk skeleton deliberately parallels ts-types.ts's renderSupportedSchema; the two sibling renderers keep symmetric shapes. */ const finish = (type: string): void => { - frames.pop() + const popped = frames.pop() + if (popped !== undefined && typeof popped.schema === 'object' && popped.schema !== null) { + activeSchemas.delete(popped.schema) + } const parent = frames.at(-1) if (parent === undefined) result = type else parent.childTypes.push(type) @@ -249,6 +263,18 @@ function renderType(schema: unknown, className: string, state: RenderState): str /* v8 ignore next -- childIndex is bounded by children.length. */ if (child === undefined) throw new Error('missing python render child') frame.childIndex++ + // A child schema already on the active path is a cycle a post- + // validation mutation introduced; degrade it to `Any` rather than + // recurse forever. A fresh object joins the path (finish removes it); + // a non-object child carries no identity to track. + if (typeof child.schema === 'object' && child.schema !== null) { + if (activeSchemas.has(child.schema)) { + state.typing.add('Any') + frame.childTypes.push('Any') + continue + } + activeSchemas.add(child.schema) + } frames.push(newFrame(child.schema, child.className, true)) continue } diff --git a/packages/core/tools/tests/py-types.spec.ts b/packages/core/tools/tests/py-types.spec.ts index 174b8987a2..1293390fec 100644 --- a/packages/core/tools/tests/py-types.spec.ts +++ b/packages/core/tools/tests/py-types.spec.ts @@ -144,6 +144,43 @@ describe('jsonSchemaToPy', () => { expect(text).toContain('class FooArgsPhase3(TypedDict):') }) + it('degrades to Any instead of looping when a stateful getter introduces a cycle after validation', () => { + // `items` validates as a scalar, then returns the root schema at render + // time — a cycle a post-validation mutation introduced. The walk must + // degrade to Any rather than push frames forever. + let itemReads = 0 + const root: Record = { type: 'array' } + Object.defineProperty(root, 'items', { + enumerable: true, + get() { + itemReads += 1 + return itemReads <= 1 ? { type: 'string' } : root + }, + }) + let out: string | undefined + expect(() => { out = jsonSchemaToPy(root) }).not.toThrow() + // list[...] of a self-cycle: the inner cycle degrades to Any. + expect(out).toBe('list[Any]') + }) + + it('degrades to Any when a stateful getter returns a non-object child at render time', () => { + // `items` validates as a scalar node, then returns a bare string (a + // non-object) at render. The walk must handle a non-object child without + // tracking identity and degrade it, not throw. + let itemReads = 0 + const root: Record = { type: 'array' } + Object.defineProperty(root, 'items', { + enumerable: true, + get() { + itemReads += 1 + return itemReads <= 1 ? { type: 'string' } : 'not-a-schema-object' + }, + }) + let out: string | undefined + expect(() => { out = jsonSchemaToPy(root) }).not.toThrow() + expect(out).toBe('list[Any]') + }) + it('emits exact digits for a beyond-safe-range integer literal', () => { // Python integers are arbitrary-precision, so the emitted digits ARE the // value the model programs against. `String(2 ** 60)` prints the rounded