feat(runtime)!: validate decoded returns against declared types#302
Merged
Conversation
github-actions
Bot
added
area:docs
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGenerated wrappers now create return validators from Python annotations, preserve decoded shape metadata for marker types, and pass validation callbacks through bridge layers. Mismatches raise ChangesRuntime return validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratedWrapper
participant BasePythonBridge
participant RpcClient
participant BridgeCodec
participant ReturnValidator
GeneratedWrapper->>BasePythonBridge: call with validator
BasePythonBridge->>RpcClient: forward validation callback
RpcClient->>BridgeCodec: decode response
BridgeCodec->>ReturnValidator: validate decoded value
ReturnValidator-->>GeneratedWrapper: value or BridgeValidationError
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
area:runtime-node
added 2 commits
July 11, 2026 15:07
…h test The fake-python shim was a #!/bin/sh script, which Windows cannot spawn (ENOENT) — the failure surfaced as ir-unavailable instead of the asserted ir-version-mismatch. Windows now gets a .cmd shim. (Defect arrived with #301, whose Windows job did not re-run after the stack retarget.)
Windows cannot run the shim either way: an extensionless sh script fails with ENOENT and a .cmd fails with EINVAL (Node's batch-file spawn mitigation — the production spawn rightly never sets shell:true). The version-check logic is platform-independent TypeScript and remains exercised on the Linux and macOS matrix legs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/generator.ts (1)
941-993: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winClass/static method return validator is constructed on every call instead of once.
For function wrappers,
returnValidator(Lines 647-648) is emitted as a module-scopeconstabove the function, socreateReturnValidator(...)runs once at module load. For class methods, the samereturnValidatorstring is instead spliced directly inside the method body (Line 992:`... { \n ${returnValidator}${callPrelude}${guards} ...`), socreateReturnValidator— including the recursiverenderSchemawalk — reruns on every single invocation of that static/class method. This is both a needless per-call allocation and an inconsistency between the two generated wrapper styles.♻️ Proposed fix: hoist method validators to module scope, mirroring function wrappers
const methodBodies: string[] = []; const methodDeclarations: string[] = []; + const methodValidators: string[] = [];const callExpr = `getRuntimeBridge().call<${returnType}>('${moduleId}', '${cls.name}.${method.name}', __args, ${needsKwargsParam ? '__kwargs' : 'undefined'}, ${validatorName})`; + methodValidators.push(returnValidator); methodBodies.push(`${overloadDecl} ${staticPrefix}async ${mname}${methodTypeParamDecl}(${paramsDecl}): Promise<${returnType}> { - ${returnValidator}${callPrelude}${guards} return ${callExpr}; + ${callPrelude}${guards} return ${callExpr}; }`);And prepend
methodValidators.join('')ahead of theexport classdeclaration in thetstemplate (near Line 1010), the same wayreturnValidatorprecedesexport async functionfor plain functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/generator.ts` around lines 941 - 993, Hoist each class/static method’s return validator out of its method body so createReturnValidator runs once at module load. In the method-generation flow around returnValidator and methodBodies, collect the validator declarations in a methodValidators array, remove returnValidator from the generated method body, and prepend methodValidators.join('') before the export class declaration, mirroring the module-scope function-wrapper generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/generator.ts`:
- Around line 402-525: Update ndarray/NDArray marker detection in returnSchema’s
generic and custom branches to require an absent module or a NumPy-qualified
module, matching the module-guard pattern used for DataFrame and Series.
Preserve marker detection for genuine NumPy types while allowing same-named
classes from unrelated modules to follow normal definition or any-schema
handling.
- Around line 1070-1091: Cache the result of returnDefinitions(module) once in
the surrounding module-generation flow, then reuse that cached value in the
generateFunctionWrapper and generateClassWrapper mappings and the
emitReturnDefinitions call. Keep the existing mutable-copy sort approach
unchanged because ES2022 targeting does not support toSorted().
In `@src/utils/codec.ts`:
- Around line 491-514: Update the ndarray decoding paths around reshapeArray and
the JSON value.data return so zero-dimensional scalar results preserve
provenance metadata even when the decoded value is primitive. Ensure
tagDecodedShape receives or returns a metadata-bearing wrapper for shape === []
(including JSON envelopes), while keeping existing 1D and multi-dimensional
array behavior unchanged.
In `@test/runtime_return_validation.test.ts`:
- Around line 145-166: Update the test around “survives the Node facade and
preserves BridgeValidationError” to remove it.skipIf and return early from the
test when PYTHON_AVAILABLE is false. Preserve the existing test body and cleanup
behavior when Python is available.
---
Outside diff comments:
In `@src/core/generator.ts`:
- Around line 941-993: Hoist each class/static method’s return validator out of
its method body so createReturnValidator runs once at module load. In the
method-generation flow around returnValidator and methodBodies, collect the
validator declarations in a methodValidators array, remove returnValidator from
the generated method body, and prepend methodValidators.join('') before the
export class declaration, mirroring the module-scope function-wrapper
generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 395c4b09-a1c0-4b76-8990-1190d057b420
⛔ Files ignored due to path filters (1)
test/menagerie/__snapshots__/gen.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
docs/guide/getting-started.mddocs/public/llms-full.txtsrc/core/generator.tssrc/index.tssrc/runtime/base-bridge.tssrc/runtime/bridge-codec.tssrc/runtime/errors.tssrc/runtime/index.tssrc/runtime/rpc-client.tssrc/runtime/validators.tssrc/types/index.tssrc/utils/codec.tstest/api_surface.test.tstest/generated_snapshot.test.tstest/generator.test.tstest/integration.test.tstest/menagerie/fixtures/typing_torture.pytest/runtime_return_validation.test.tstywrap_ir/tywrap_ir/ir.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: test (22, 3.10)
- GitHub Check: test (22, 3.12)
- GitHub Check: test (20, 3.12)
- GitHub Check: test (20, 3.10)
- GitHub Check: test (22, 3.11)
- GitHub Check: test (20, 3.11)
- GitHub Check: data-plane-perf
- GitHub Check: os (macos-latest)
- GitHub Check: os (windows-latest)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript strict mode and avoid
any; preferunknownwith type guards.
Files:
test/generated_snapshot.test.tstest/api_surface.test.tssrc/runtime/bridge-codec.tssrc/index.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/types/index.tssrc/runtime/rpc-client.tstest/runtime_return_validation.test.tstest/integration.test.tstest/generator.test.tssrc/runtime/validators.tssrc/utils/codec.tssrc/core/generator.ts
src/runtime/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement runtime bridges and transport code under
src/runtime/.Implement runtime bridge code under
src/runtime/.
Files:
src/runtime/bridge-codec.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/runtime/rpc-client.tssrc/runtime/validators.ts
test/runtime_*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place corresponding runtime bridge tests in
test/runtime_*.test.ts.
test/runtime_*.test.ts: Place corresponding runtime bridge tests intest/runtime_*.test.ts.
Include tests for new runtime behavior, following existing runtime test patterns.
Files:
test/runtime_return_validation.test.ts
tywrap_ir/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code under
tywrap_ir/must follow PEP 8 and use type hints.Follow PEP 8 and use type hints in Python code.
Files:
tywrap_ir/tywrap_ir/ir.py
🧠 Learnings (6)
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/generated_snapshot.test.tstest/api_surface.test.tstest/runtime_return_validation.test.tstest/integration.test.tstest/generator.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/generated_snapshot.test.tstest/api_surface.test.tssrc/runtime/bridge-codec.tssrc/index.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/types/index.tssrc/runtime/rpc-client.tstest/runtime_return_validation.test.tstest/integration.test.tstest/generator.test.tssrc/runtime/validators.tssrc/utils/codec.tssrc/core/generator.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/generated_snapshot.test.tstest/api_surface.test.tssrc/runtime/bridge-codec.tssrc/index.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/types/index.tssrc/runtime/rpc-client.tstest/runtime_return_validation.test.tstest/integration.test.tstest/generator.test.tssrc/runtime/validators.tssrc/utils/codec.tssrc/core/generator.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/generated_snapshot.test.tstest/api_surface.test.tssrc/runtime/bridge-codec.tssrc/index.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/types/index.tssrc/runtime/rpc-client.tstest/runtime_return_validation.test.tstest/integration.test.tstest/generator.test.tssrc/runtime/validators.tssrc/utils/codec.tssrc/core/generator.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/bridge-codec.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/runtime/rpc-client.tssrc/runtime/validators.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/bridge-codec.tssrc/runtime/index.tssrc/runtime/errors.tssrc/runtime/base-bridge.tssrc/runtime/rpc-client.tssrc/runtime/validators.ts
🪛 ESLint
test/runtime_return_validation.test.ts
[error] 23-23: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
src/core/generator.ts
[error] 1081-1082: Use module.classes.toSorted() instead of copying and sorting
(e18e/prefer-array-to-sorted)
🔇 Additional comments (20)
docs/guide/getting-started.md (1)
121-128: LGTM!docs/public/llms-full.txt (1)
180-187: LGTM!src/utils/codec.ts (1)
73-73: LGTM!Also applies to: 448-457, 472-473
src/runtime/bridge-codec.ts (1)
646-648: LGTM!src/types/index.ts (1)
505-506: LGTM!src/runtime/base-bridge.ts (1)
42-51: LGTM!src/runtime/rpc-client.ts (1)
433-454: LGTM!test/integration.test.ts (1)
74-74: LGTM!Also applies to: 341-346
test/generated_snapshot.test.ts (1)
21-21: LGTM!src/core/generator.ts (1)
39-40: LGTM!Also applies to: 527-533, 535-552, 647-648, 737-739, 754-756, 1107-1109
test/generator.test.ts (1)
58-60: LGTM!Also applies to: 62-96, 98-118, 1449-1455, 1628-1630
src/runtime/validators.ts (1)
1-2: LGTM!Also applies to: 20-55, 61-87, 89-111, 112-198, 200-221
src/runtime/errors.ts (1)
17-37: LGTM!src/index.ts (1)
19-28: LGTM!tywrap_ir/tywrap_ir/ir.py (1)
741-754: LGTM!test/menagerie/fixtures/typing_torture.py (1)
31-34: LGTM!Also applies to: 52-55
src/runtime/index.ts (1)
14-19: LGTM!test/api_surface.test.ts (1)
23-23: LGTM!test/runtime_return_validation.test.ts (2)
16-112: LGTM!
123-143: LGTM!
Comment on lines
+402
to
+525
| /** | ||
| * Preserve Python provenance in generated return validators instead of | ||
| * reconstructing a checker from the final TypeScript spelling. In particular | ||
| * pandas/NumPy values retain their marker contract after Arrow decoding. | ||
| */ | ||
| private returnSchema(type: PythonType, definitions: ReturnDefinitionNames = new Set()): unknown { | ||
| // A bare `-> None` return maps to TS void and validates nothing, but a | ||
| // NESTED None (union member, tuple element) is a real wire value — it | ||
| // decodes to null and must be checked as null, or a union containing it | ||
| // would carry an any-member and accept everything. | ||
| if (type.kind === 'primitive' && type.name === 'None') { | ||
| return { kind: 'any' }; | ||
| } | ||
| const schema = (current: PythonType): unknown => { | ||
| switch (current.kind) { | ||
| case 'primitive': | ||
| return current.name === 'int' || current.name === 'float' | ||
| ? { kind: 'primitive', type: 'number' } | ||
| : current.name === 'str' | ||
| ? { kind: 'primitive', type: 'string' } | ||
| : current.name === 'bool' | ||
| ? { kind: 'primitive', type: 'boolean' } | ||
| : current.name === 'bytes' | ||
| ? { kind: 'primitive', type: 'Uint8Array' } | ||
| : current.name === 'None' | ||
| ? { kind: 'primitive', type: 'null' } | ||
| : { kind: 'any' }; | ||
| case 'literal': | ||
| return { kind: 'literal', value: current.value }; | ||
| case 'annotated': | ||
| return schema(current.base); | ||
| case 'final': | ||
| case 'classvar': | ||
| return schema(current.type); | ||
| case 'optional': | ||
| return { | ||
| kind: 'union', | ||
| options: [schema(current.type), { kind: 'primitive', type: 'null' }], | ||
| }; | ||
| case 'union': | ||
| return { kind: 'union', options: current.types.map(schema) }; | ||
| case 'collection': { | ||
| if (current.name === 'tuple') { | ||
| if (current.itemTypes[1]?.kind === 'custom' && current.itemTypes[1].name === '...') { | ||
| return { | ||
| kind: 'array', | ||
| element: schema(current.itemTypes[0] ?? { kind: 'custom', name: 'Any' }), | ||
| }; | ||
| } | ||
| return { kind: 'tuple', elements: current.itemTypes.map(schema) }; | ||
| } | ||
| if (current.name === 'dict') { | ||
| return { | ||
| kind: 'record', | ||
| values: schema(current.itemTypes[1] ?? { kind: 'custom', name: 'Any' }), | ||
| }; | ||
| } | ||
| return { | ||
| kind: 'array', | ||
| element: schema(current.itemTypes[0] ?? { kind: 'custom', name: 'Any' }), | ||
| }; | ||
| } | ||
| case 'generic': { | ||
| const leaf = current.name.split('.').at(-1) ?? current.name; | ||
| if (leaf === 'NDArray' || leaf === 'ndarray') { | ||
| return { kind: 'marker', marker: 'ndarray' }; | ||
| } | ||
| if ( | ||
| [ | ||
| 'list', | ||
| 'List', | ||
| 'Sequence', | ||
| 'Iterable', | ||
| 'Iterator', | ||
| 'Generator', | ||
| 'set', | ||
| 'frozenset', | ||
| ].includes(leaf) | ||
| ) { | ||
| return { | ||
| kind: 'array', | ||
| element: schema(current.typeArgs[0] ?? { kind: 'custom', name: 'Any' }), | ||
| }; | ||
| } | ||
| if (['dict', 'Dict', 'Mapping', 'MutableMapping'].includes(leaf)) { | ||
| return { | ||
| kind: 'record', | ||
| values: schema(current.typeArgs[1] ?? { kind: 'custom', name: 'Any' }), | ||
| }; | ||
| } | ||
| return definitions.has(leaf) ? { kind: 'ref', name: leaf } : { kind: 'any' }; | ||
| } | ||
| case 'custom': { | ||
| const leaf = current.name.split('.').at(-1) ?? current.name; | ||
| const full = `${current.module ?? ''}.${leaf}`; | ||
| if (leaf === 'None') { | ||
| return { kind: 'primitive', type: 'null' }; | ||
| } | ||
| if (leaf === 'Any' || leaf === 'object' || leaf === 'NoReturn' || leaf === 'Never') { | ||
| return { kind: 'any' }; | ||
| } | ||
| if (leaf === 'DataFrame' && (!current.module || current.module.startsWith('pandas'))) { | ||
| return { kind: 'marker', marker: 'dataframe' }; | ||
| } | ||
| if (leaf === 'Series' && (!current.module || current.module.startsWith('pandas'))) { | ||
| return { kind: 'marker', marker: 'series' }; | ||
| } | ||
| if (leaf === 'ndarray' || leaf === 'NDArray' || full.includes('numpy')) { | ||
| return { kind: 'marker', marker: 'ndarray' }; | ||
| } | ||
| return definitions.has(leaf) ? { kind: 'ref', name: leaf } : { kind: 'any' }; | ||
| } | ||
| case 'typevar': | ||
| case 'paramspec': | ||
| case 'paramspec_args': | ||
| case 'paramspec_kwargs': | ||
| case 'typevartuple': | ||
| case 'unpack': | ||
| case 'callable': | ||
| return { kind: 'any' }; | ||
| } | ||
| }; | ||
| return schema(type); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ndarray/NDArray marker detection is missing the module guard its siblings use.
DataFrame/Series checks require the module to be absent or start with pandas (Lines 503-508), but the ndarray/NDArray checks — both in the generic branch (Lines 466-468) and the custom branch (Lines 509-511) — match on bare name alone. A user-defined class simply named ndarray/NDArray from any other module will be tagged {kind:'marker', marker:'ndarray'}, but its decoded value will never carry the decodedShapeMetadata tag (that's only applied by the numpy-specific codec path), so validation will always fail for an otherwise correctly-typed return. Given this PR turns mismatches into hard failures, this false positive is a real regression risk.
🐛 Proposed fix to gate ndarray marker matching on module, consistent with DataFrame/Series
if (leaf === 'NDArray' || leaf === 'ndarray') {
- return { kind: 'marker', marker: 'ndarray' };
+ if (
+ (leaf === 'NDArray' || leaf === 'ndarray') &&
+ (!current.module || current.module.startsWith('numpy'))
+ ) {
+ return { kind: 'marker', marker: 'ndarray' };
}- if (leaf === 'ndarray' || leaf === 'NDArray' || full.includes('numpy')) {
+ if (
+ full.includes('numpy') ||
+ ((leaf === 'ndarray' || leaf === 'NDArray') &&
+ (!current.module || current.module.startsWith('numpy')))
+ ) {
return { kind: 'marker', marker: 'ndarray' };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Preserve Python provenance in generated return validators instead of | |
| * reconstructing a checker from the final TypeScript spelling. In particular | |
| * pandas/NumPy values retain their marker contract after Arrow decoding. | |
| */ | |
| private returnSchema(type: PythonType, definitions: ReturnDefinitionNames = new Set()): unknown { | |
| // A bare `-> None` return maps to TS void and validates nothing, but a | |
| // NESTED None (union member, tuple element) is a real wire value — it | |
| // decodes to null and must be checked as null, or a union containing it | |
| // would carry an any-member and accept everything. | |
| if (type.kind === 'primitive' && type.name === 'None') { | |
| return { kind: 'any' }; | |
| } | |
| const schema = (current: PythonType): unknown => { | |
| switch (current.kind) { | |
| case 'primitive': | |
| return current.name === 'int' || current.name === 'float' | |
| ? { kind: 'primitive', type: 'number' } | |
| : current.name === 'str' | |
| ? { kind: 'primitive', type: 'string' } | |
| : current.name === 'bool' | |
| ? { kind: 'primitive', type: 'boolean' } | |
| : current.name === 'bytes' | |
| ? { kind: 'primitive', type: 'Uint8Array' } | |
| : current.name === 'None' | |
| ? { kind: 'primitive', type: 'null' } | |
| : { kind: 'any' }; | |
| case 'literal': | |
| return { kind: 'literal', value: current.value }; | |
| case 'annotated': | |
| return schema(current.base); | |
| case 'final': | |
| case 'classvar': | |
| return schema(current.type); | |
| case 'optional': | |
| return { | |
| kind: 'union', | |
| options: [schema(current.type), { kind: 'primitive', type: 'null' }], | |
| }; | |
| case 'union': | |
| return { kind: 'union', options: current.types.map(schema) }; | |
| case 'collection': { | |
| if (current.name === 'tuple') { | |
| if (current.itemTypes[1]?.kind === 'custom' && current.itemTypes[1].name === '...') { | |
| return { | |
| kind: 'array', | |
| element: schema(current.itemTypes[0] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| return { kind: 'tuple', elements: current.itemTypes.map(schema) }; | |
| } | |
| if (current.name === 'dict') { | |
| return { | |
| kind: 'record', | |
| values: schema(current.itemTypes[1] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| return { | |
| kind: 'array', | |
| element: schema(current.itemTypes[0] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| case 'generic': { | |
| const leaf = current.name.split('.').at(-1) ?? current.name; | |
| if (leaf === 'NDArray' || leaf === 'ndarray') { | |
| return { kind: 'marker', marker: 'ndarray' }; | |
| } | |
| if ( | |
| [ | |
| 'list', | |
| 'List', | |
| 'Sequence', | |
| 'Iterable', | |
| 'Iterator', | |
| 'Generator', | |
| 'set', | |
| 'frozenset', | |
| ].includes(leaf) | |
| ) { | |
| return { | |
| kind: 'array', | |
| element: schema(current.typeArgs[0] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| if (['dict', 'Dict', 'Mapping', 'MutableMapping'].includes(leaf)) { | |
| return { | |
| kind: 'record', | |
| values: schema(current.typeArgs[1] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| return definitions.has(leaf) ? { kind: 'ref', name: leaf } : { kind: 'any' }; | |
| } | |
| case 'custom': { | |
| const leaf = current.name.split('.').at(-1) ?? current.name; | |
| const full = `${current.module ?? ''}.${leaf}`; | |
| if (leaf === 'None') { | |
| return { kind: 'primitive', type: 'null' }; | |
| } | |
| if (leaf === 'Any' || leaf === 'object' || leaf === 'NoReturn' || leaf === 'Never') { | |
| return { kind: 'any' }; | |
| } | |
| if (leaf === 'DataFrame' && (!current.module || current.module.startsWith('pandas'))) { | |
| return { kind: 'marker', marker: 'dataframe' }; | |
| } | |
| if (leaf === 'Series' && (!current.module || current.module.startsWith('pandas'))) { | |
| return { kind: 'marker', marker: 'series' }; | |
| } | |
| if (leaf === 'ndarray' || leaf === 'NDArray' || full.includes('numpy')) { | |
| return { kind: 'marker', marker: 'ndarray' }; | |
| } | |
| return definitions.has(leaf) ? { kind: 'ref', name: leaf } : { kind: 'any' }; | |
| } | |
| case 'typevar': | |
| case 'paramspec': | |
| case 'paramspec_args': | |
| case 'paramspec_kwargs': | |
| case 'typevartuple': | |
| case 'unpack': | |
| case 'callable': | |
| return { kind: 'any' }; | |
| } | |
| }; | |
| return schema(type); | |
| } | |
| /** | |
| * Preserve Python provenance in generated return validators instead of | |
| * reconstructing a checker from the final TypeScript spelling. In particular | |
| * pandas/NumPy values retain their marker contract after Arrow decoding. | |
| */ | |
| private returnSchema(type: PythonType, definitions: ReturnDefinitionNames = new Set()): unknown { | |
| // A bare `-> None` return maps to TS void and validates nothing, but a | |
| // NESTED None (union member, tuple element) is a real wire value — it | |
| // decodes to null and must be checked as null, or a union containing it | |
| // would carry an any-member and accept everything. | |
| if (type.kind === 'primitive' && type.name === 'None') { | |
| return { kind: 'any' }; | |
| } | |
| const schema = (current: PythonType): unknown => { | |
| switch (current.kind) { | |
| case 'primitive': | |
| return current.name === 'int' || current.name === 'float' | |
| ? { kind: 'primitive', type: 'number' } | |
| : current.name === 'str' | |
| ? { kind: 'primitive', type: 'string' } | |
| : current.name === 'bool' | |
| ? { kind: 'primitive', type: 'boolean' } | |
| : current.name === 'bytes' | |
| ? { kind: 'primitive', type: 'Uint8Array' } | |
| : current.name === 'None' | |
| ? { kind: 'primitive', type: 'null' } | |
| : { kind: 'any' }; | |
| case 'literal': | |
| return { kind: 'literal', value: current.value }; | |
| case 'annotated': | |
| return schema(current.base); | |
| case 'final': | |
| case 'classvar': | |
| return schema(current.type); | |
| case 'optional': | |
| return { | |
| kind: 'union', | |
| options: [schema(current.type), { kind: 'primitive', type: 'null' }], | |
| }; | |
| case 'union': | |
| return { kind: 'union', options: current.types.map(schema) }; | |
| case 'collection': { | |
| if (current.name === 'tuple') { | |
| if (current.itemTypes[1]?.kind === 'custom' && current.itemTypes[1].name === '...') { | |
| return { | |
| kind: 'array', | |
| element: schema(current.itemTypes[0] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| return { kind: 'tuple', elements: current.itemTypes.map(schema) }; | |
| } | |
| if (current.name === 'dict') { | |
| return { | |
| kind: 'record', | |
| values: schema(current.itemTypes[1] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| return { | |
| kind: 'array', | |
| element: schema(current.itemTypes[0] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| case 'generic': { | |
| const leaf = current.name.split('.').at(-1) ?? current.name; | |
| if (leaf === 'NDArray' || leaf === 'ndarray') { | |
| if ( | |
| (leaf === 'NDArray' || leaf === 'ndarray') && | |
| (!current.module || current.module.startsWith('numpy')) | |
| ) { | |
| return { kind: 'marker', marker: 'ndarray' }; | |
| } | |
| } | |
| if ( | |
| [ | |
| 'list', | |
| 'List', | |
| 'Sequence', | |
| 'Iterable', | |
| 'Iterator', | |
| 'Generator', | |
| 'set', | |
| 'frozenset', | |
| ].includes(leaf) | |
| ) { | |
| return { | |
| kind: 'array', | |
| element: schema(current.typeArgs[0] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| if (['dict', 'Dict', 'Mapping', 'MutableMapping'].includes(leaf)) { | |
| return { | |
| kind: 'record', | |
| values: schema(current.typeArgs[1] ?? { kind: 'custom', name: 'Any' }), | |
| }; | |
| } | |
| return definitions.has(leaf) ? { kind: 'ref', name: leaf } : { kind: 'any' }; | |
| } | |
| case 'custom': { | |
| const leaf = current.name.split('.').at(-1) ?? current.name; | |
| const full = `${current.module ?? ''}.${leaf}`; | |
| if (leaf === 'None') { | |
| return { kind: 'primitive', type: 'null' }; | |
| } | |
| if (leaf === 'Any' || leaf === 'object' || leaf === 'NoReturn' || leaf === 'Never') { | |
| return { kind: 'any' }; | |
| } | |
| if (leaf === 'DataFrame' && (!current.module || current.module.startsWith('pandas'))) { | |
| return { kind: 'marker', marker: 'dataframe' }; | |
| } | |
| if (leaf === 'Series' && (!current.module || current.module.startsWith('pandas'))) { | |
| return { kind: 'marker', marker: 'series' }; | |
| } | |
| if ( | |
| full.includes('numpy') || | |
| ((leaf === 'ndarray' || leaf === 'NDArray') && | |
| (!current.module || current.module.startsWith('numpy'))) | |
| ) { | |
| return { kind: 'marker', marker: 'ndarray' }; | |
| } | |
| return definitions.has(leaf) ? { kind: 'ref', name: leaf } : { kind: 'any' }; | |
| } | |
| case 'typevar': | |
| case 'paramspec': | |
| case 'paramspec_args': | |
| case 'paramspec_kwargs': | |
| case 'typevartuple': | |
| case 'unpack': | |
| case 'callable': | |
| return { kind: 'any' }; | |
| } | |
| }; | |
| return schema(type); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/generator.ts` around lines 402 - 525, Update ndarray/NDArray marker
detection in returnSchema’s generic and custom branches to require an absent
module or a NumPy-qualified module, matching the module-guard pattern used for
DataFrame and Series. Preserve marker detection for genuine NumPy types while
allowing same-named classes from unrelated modules to follow normal definition
or any-schema handling.
Comment on lines
1070
to
+1091
| const functionResults = [...module.functions] | ||
| .sort((a, b) => a.name.localeCompare(b.name)) | ||
| .map(f => this.generateFunctionWrapper(f, module.name, annotatedJSDoc, localDeclaredNames)); | ||
| .map(f => | ||
| this.generateFunctionWrapper( | ||
| f, | ||
| module.name, | ||
| annotatedJSDoc, | ||
| localDeclaredNames, | ||
| this.returnDefinitions(module) | ||
| ) | ||
| ); | ||
| const classResults = [...module.classes] | ||
| .sort((a, b) => a.name.localeCompare(b.name)) | ||
| .map(c => this.generateClassWrapper(c, module.name, annotatedJSDoc, localDeclaredNames)); | ||
| .map(c => | ||
| this.generateClassWrapper( | ||
| c, | ||
| module.name, | ||
| annotatedJSDoc, | ||
| localDeclaredNames, | ||
| this.returnDefinitions(module) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n '"target"|"lib"' tsconfig.json tsconfig.*.json 2>/dev/null
rg -n '"engines"' package.json
jq '.engines' package.json 2>/dev/nullRepository: bbopen/tywrap
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of src/core/generator.ts with line numbers.
sed -n '1040,1135p' src/core/generator.ts | cat -n
# Check whether .toSorted() is already used anywhere in the repo.
rg -n '\.toSorted\(' .
# Check TypeScript version and compile target/lib context.
jq '{devDependencies,dependencies,engines}' package.json 2>/dev/null || trueRepository: bbopen/tywrap
Length of output: 4511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant part of generator.ts around the review comment.
sed -n '1048,1125p' src/core/generator.ts | cat -n
# Find returnDefinitions and emitReturnDefinitions definitions.
rg -n 'returnDefinitions|emitReturnDefinitions' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 4915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of returnDefinitions and emitReturnDefinitions.
sed -n '1120,1185p' src/core/generator.ts | cat -nRepository: bbopen/tywrap
Length of output: 2931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect returnDefinitions and emitReturnDefinitions implementations.
sed -n '520,550p' src/core/generator.ts | cat -n
# Check whether Array.prototype.toSorted is supported by the current TS lib target from the repo config.
node - <<'JS'
const fs = require('fs');
const tsconfig = JSON.parse(fs.readFileSync('tsconfig.json', 'utf8'));
console.log(JSON.stringify({ target: tsconfig.compilerOptions.target, lib: tsconfig.compilerOptions.lib }, null, 2));
JSRepository: bbopen/tywrap
Length of output: 1430
Hoist returnDefinitions(module) once per module
returnDefinitions(module) is rebuilt for every function/class wrapper and again in emitReturnDefinitions; cache the set once and pass it through to all three call sites. The .toSorted() swap isn’t applicable here because the project still targets ES2022.
🧰 Tools
🪛 ESLint
[error] 1070-1071: Use module.functions.toSorted() instead of copying and sorting
(e18e/prefer-array-to-sorted)
[error] 1081-1082: Use module.classes.toSorted() instead of copying and sorting
(e18e/prefer-array-to-sorted)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/generator.ts` around lines 1070 - 1091, Cache the result of
returnDefinitions(module) once in the surrounding module-generation flow, then
reuse that cached value in the generateFunctionWrapper and generateClassWrapper
mappings and the emitReturnDefinitions call. Keep the existing mutable-copy sort
approach unchanged because ES2022 targeting does not support toSorted().
Source: Linters/SAST tools
Comment on lines
+491
to
+514
| return tagDecodedShape(data, metadata); // Fallback: keep provenance on raw data. | ||
| } | ||
| // Reshape scalars and multi-dimensional arrays, but not 1D | ||
| if (shape && shape.length !== 1) { | ||
| return reshapeArray(values, shape); | ||
| } | ||
| return values; | ||
| return tagDecodedShape( | ||
| shape && shape.length !== 1 ? reshapeArray(values, shape) : values, | ||
| metadata | ||
| ); | ||
| }); | ||
| } | ||
| const values = extractArrowValues(decoded); | ||
| if (!values) { | ||
| return decoded; // Fallback: return raw data if extraction fails | ||
| return tagDecodedShape(decoded, metadata); // Fallback: keep provenance on raw data. | ||
| } | ||
| // Reshape scalars and multi-dimensional arrays, but not 1D | ||
| if (shape && shape.length !== 1) { | ||
| return reshapeArray(values, shape); | ||
| } | ||
| return values; | ||
| return tagDecodedShape( | ||
| shape && shape.length !== 1 ? reshapeArray(values, shape) : values, | ||
| metadata | ||
| ); | ||
| } | ||
| if (encoding === 'json') { | ||
| if (!('data' in value)) { | ||
| throw new Error('Invalid ndarray envelope: missing data'); | ||
| } | ||
| return value.data; | ||
| return tagDecodedShape(value.data, metadata); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the generator handles ndarray return types (marker vs primitive)
rg -n 'ndarray' --type=ts src/core/generator.ts | head -30
# Check how the IR module handles 0-d / scalar ndarray encoding
rg -n 'ndarray' --type=py tywrap_ir/ | head -30
# Check if validators have special handling for primitive values with marker schemas
rg -n 'marker' --type=ts src/runtime/validators.ts | head -20Repository: bbopen/tywrap
Length of output: 952
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/utils/codec.ts around decodeNdarrayEnvelope =="
sed -n '430,540p' src/utils/codec.ts
echo
echo "== src/core/generator.ts around ndarray return schema handling =="
sed -n '440,530p' src/core/generator.ts
echo
echo "== src/runtime/validators.ts around marker handling =="
sed -n '160,220p' src/runtime/validators.tsRepository: bbopen/tywrap
Length of output: 9997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tagDecodedShape / reshapeArray definitions =="
rg -n 'function tagDecodedShape|const tagDecodedShape|function reshapeArray|const reshapeArray' src/utils/codec.ts
echo
echo "== surrounding implementation =="
sed -n '1,220p' src/utils/codec.tsRepository: bbopen/tywrap
Length of output: 7493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reshapeArray implementation =="
sed -n '360,430p' src/utils/codec.ts
echo
echo "== tagDecodedShape implementation and metadata storage =="
rg -n 'function tagDecodedShape|const decodedShapeMetadata|WeakMap' src/runtime/validators.ts
sed -n '1,80p' src/runtime/validators.tsRepository: bbopen/tywrap
Length of output: 5451
0-d ndarray scalars drop provenance metadata reshapeArray(values, []) and the JSON value.data path both return primitives, but tagDecodedShape only stores metadata on objects/functions. That makes kind: 'marker' return schemas for ndarray fail on scalar outputs; preserve a wrapper for 0-d values or adjust the validator path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/codec.ts` around lines 491 - 514, Update the ndarray decoding paths
around reshapeArray and the JSON value.data return so zero-dimensional scalar
results preserve provenance metadata even when the decoded value is primitive.
Ensure tagDecodedShape receives or returns a metadata-bearing wrapper for shape
=== [] (including JSON envelopes), while keeping existing 1D and
multi-dimensional array behavior unchanged.
Comment on lines
+145
to
+166
| it.skipIf(!PYTHON_AVAILABLE)( | ||
| 'survives the Node facade and preserves BridgeValidationError', | ||
| async () => { | ||
| const node = new NodeBridge({ | ||
| pythonPath: PYTHON ?? undefined, | ||
| scriptPath: 'runtime/python_bridge.py', | ||
| }); | ||
| try { | ||
| await expect( | ||
| node.call<number>( | ||
| 'builtins', | ||
| 'str', | ||
| [123], | ||
| undefined, | ||
| createReturnValidator({ kind: 'primitive', type: 'number' }, 'builtins.str') | ||
| ) | ||
| ).rejects.toMatchObject({ name: 'BridgeValidationError', callSite: 'builtins.str' }); | ||
| } finally { | ||
| await node.dispose(); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use early return instead of it.skipIf for Python-gated test.
The repo convention is to prefer early returns when Python is unavailable, not Vitest dynamic skip APIs. A silent pass is intentional for environments lacking Python.
Based on learnings from PR 127: "prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures."
🔧 Proposed fix
- it.skipIf(!PYTHON_AVAILABLE)(
- 'survives the Node facade and preserves BridgeValidationError',
- async () => {
+ it(
+ 'survives the Node facade and preserves BridgeValidationError',
+ async () => {
+ if (!PYTHON_AVAILABLE) return;
const node = new NodeBridge({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it.skipIf(!PYTHON_AVAILABLE)( | |
| 'survives the Node facade and preserves BridgeValidationError', | |
| async () => { | |
| const node = new NodeBridge({ | |
| pythonPath: PYTHON ?? undefined, | |
| scriptPath: 'runtime/python_bridge.py', | |
| }); | |
| try { | |
| await expect( | |
| node.call<number>( | |
| 'builtins', | |
| 'str', | |
| [123], | |
| undefined, | |
| createReturnValidator({ kind: 'primitive', type: 'number' }, 'builtins.str') | |
| ) | |
| ).rejects.toMatchObject({ name: 'BridgeValidationError', callSite: 'builtins.str' }); | |
| } finally { | |
| await node.dispose(); | |
| } | |
| } | |
| ); | |
| it( | |
| 'survives the Node facade and preserves BridgeValidationError', | |
| async () => { | |
| if (!PYTHON_AVAILABLE) return; | |
| const node = new NodeBridge({ | |
| pythonPath: PYTHON ?? undefined, | |
| scriptPath: 'runtime/python_bridge.py', | |
| }); | |
| try { | |
| await expect( | |
| node.call<number>( | |
| 'builtins', | |
| 'str', | |
| [123], | |
| undefined, | |
| createReturnValidator({ kind: 'primitive', type: 'number' }, 'builtins.str') | |
| ) | |
| ).rejects.toMatchObject({ name: 'BridgeValidationError', callSite: 'builtins.str' }); | |
| } finally { | |
| await node.dispose(); | |
| } | |
| } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/runtime_return_validation.test.ts` around lines 145 - 166, Update the
test around “survives the Node facade and preserves BridgeValidationError” to
remove it.skipIf and return early from the test when PYTHON_AVAILABLE is false.
Preserve the existing test body and cleanup behavior when Python is available.
Source: Learnings
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
1 participant
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
BridgeValidationErrorwith the declared type, received shape, and call site.BREAKING CHANGE
Generated wrappers now validate returns. Code that relied on silently mistyped
Python returns will throw
BridgeValidationError.Migration: fix the Python annotation or implementation; annotate the return as
-> Anywhen it is intentionally untyped.Validation
npm run check:allnpm run test:python:suite:coreCloses #268
Review round (independent Claude adversarial pass; codex at usage limit)
All three findings fixed in-branch:
X | Noneunion returns emitted ananymember, silently disabling validation for the most common nullable shape. NestedNonenow emits a realnullcheck; bare-> Nonestays a no-op. Generator-level tests pin both.total=Falsefixture now pins emission.__total__heuristic) — post-validation that would accept dicts missing required fields. Now derived from__optional_keys__, correct under inheritance.Verified: check:all green (1,253 passed), python suite 141 passed.