Refactor SchemaRepresentation#6424
Draft
gcanti wants to merge 23 commits into
Draft
Conversation
🦋 Changeset detectedLatest commit: 1283814 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Bundle Size Analysis
|
📝 WalkthroughWalkthroughThe PR replaces the legacy schema representation model with representation identities, typed revivers, internal serialization and compilation pipelines, built-in schema revivers, updated JSON Schema/OpenAPI/AI consumers, and extensive runtime and type-level coverage. ChangesSchema representation and revival
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/effect/src/SchemaAST.ts (1)
3054-3064: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
isPatternuse a fresh regex per call.RegExp.testmutateslastIndexforg/y, so reusing the passed regex makes repeated validation depend on call order. Clone it or resetlastIndexbefore testing.🤖 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 `@packages/effect/src/SchemaAST.ts` around lines 3054 - 3064, Update isPattern so its filter callback tests against a fresh RegExp instance or resets the original regex’s lastIndex before each test. Preserve the existing source, flags, and schema metadata while ensuring repeated validations are independent of prior calls.
🟡 Minor comments (4)
packages/tools/openapi-generator/src/JsonSchemaGenerator.ts-205-231 (1)
205-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
additionalProperties: falsedefaulting missestype: ["object", "null"].The
onEntercallback only checksout.type === "object", but JSON Schema draft 2020-12 (used by OpenAPI 3.1) permitstypeas an array. Nullable object schemas (type: ["object", "null"]) won't receive theadditionalProperties: falsedefault, potentially allowing extra properties in the generated Effect schema.🛡️ Proposed fix
const out = { ...js } - if (out.type === "object" && out.additionalProperties === undefined) { + if ( + (out.type === "object" || + (Array.isArray(out.type) && out.type.includes("object"))) && + out.additionalProperties === undefined + ) { out.additionalProperties = false }🤖 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 `@packages/tools/openapi-generator/src/JsonSchemaGenerator.ts` around lines 205 - 231, Update the onEnter callback in JsonSchemaGenerator so additionalProperties defaults to false when out.type is either the string "object" or an array containing "object", while preserving the existing behavior for schemas that explicitly define additionalProperties and for non-object types..changeset/schema-representation-refactoring.md-5-7 (1)
5-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the public contract and runtime migration in this changeset.
This is more than a maintainability refactor: it replaces representation contracts and introduces new serialization/revival pipelines. Mention the identity/reviver model and affected entry points so consumers can identify the required migration.
Proposed wording
-Refactor the `SchemaRepresentation` module to improve clarity and maintainability. +Replace the legacy `SchemaRepresentation` model with identity-based +representations, typed revivers, and new serialization and reconstruction +pipelines. Add constructors for declaration, filter, and filter group revivers that infer their payload type from `payloadSchema`.🤖 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 @.changeset/schema-representation-refactoring.md around lines 5 - 7, Update the SchemaRepresentation changeset to document the public contract changes and runtime migration, including the identity/reviver model, new serialization and revival pipelines, and the affected entry points consumers must update. Replace the current maintainability-only description with actionable migration guidance while retaining the constructor changes for declaration, filter, and filter group revivers.packages/effect/typetest/schema/FromJsonSchema.tst.ts-5-31 (1)
5-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the exported function types directly.
The explicit variable annotations replace each function’s inferred type before
type.toBe, so these checks cannot detect narrower returns or additional signatures. Applytype.toBedirectly to eachSchemaRepresentationexport.🤖 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 `@packages/effect/typetest/schema/FromJsonSchema.tst.ts` around lines 5 - 31, The type tests currently validate explicitly annotated variables rather than the exported functions. Remove the intermediate assignments and apply expect(...).type.toBe directly to SchemaRepresentation.fromJsonSchemaDocument, fromJsonSchemaMultiDocument, and fromSchemaMultiDocument, preserving the existing expected signatures.packages/effect/test/schema/representation/toJsonSchemaMultiDocument.test.ts-169-175 (1)
169-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the multi-document API in this error-path test.
Lines 169–175 call
toJsonSchemaDocument, so this does not validate multi-document reference resolution or its["representations"][0]path.Proposed fix
- SchemaRepresentation.toJsonSchemaDocument({ - representation: record({ _tag: "Reference", $ref: "Missing" }), + SchemaRepresentation.toJsonSchemaMultiDocument({ + representations: [record({ _tag: "Reference", $ref: "Missing" })], references: {} }), - `Invalid reference Missing\n at ["representation"]["indexSignatures"][0]["parameter"]["$ref"]` + `Invalid reference Missing\n at ["representations"][0]["indexSignatures"][0]["parameter"]["$ref"]`🤖 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 `@packages/effect/test/schema/representation/toJsonSchemaMultiDocument.test.ts` around lines 169 - 175, Update the error-path test around SchemaRepresentation.toJsonSchemaDocument to call the multi-document API instead, supplying the representation through its representations collection and preserving the missing-reference setup. Update the expected validation path to include ["representations"][0] while retaining the Invalid reference Missing assertion.
🧹 Nitpick comments (2)
packages/effect/test/unstable/httpapi/OpenApiRepresentation.test.ts (1)
51-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that endpoints actually reference the shared component.
Checking only
components.schemas.Shareddoes not prove definition sharing; the request and response schemas could remain inlined. Assert that both payload properties and the response use the expected$ref.🤖 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 `@packages/effect/test/unstable/httpapi/OpenApiRepresentation.test.ts` around lines 51 - 70, The “shares definitions and caches by API identity” test should also verify endpoint references to the shared schema. Extend the assertions for the OpenApi result from OpenApi.fromApi(Api) so both payload properties, first and second, and the success response use the expected $ref to Shared, while retaining the existing component and cache assertions.packages/effect/test/schema/toCodec.test.ts (1)
1659-1675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace these async test bodies with
it.effect. Lift the assertions through Effect APIs so they follow the package’s Effect test style.🤖 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 `@packages/effect/test/schema/toCodec.test.ts` around lines 1659 - 1675, Update the asynchronous “Json” test body in the TestSchema.Asserts coverage to use the package’s it.effect test style instead of an async callback. Lift the encoding and decoding assertions through the appropriate Effect APIs while preserving every existing success case, failure input, and expected error message.Source: Coding guidelines
🤖 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 `@packages/effect/src/internal/schema/fromJsonSchemaDocument.ts`:
- Around line 511-562: Wrap the String and Number branches in the switch within
explicit blocks so their case-local declarations, including stringChecks and
numberChecks, no longer share the switch scope. Preserve all existing logic and
formatting inside each case, then ensure the changes pass pnpm lint-fix and pnpm
check.
- Around line 838-840: Update the uniqueItems handling in the JSON Schema
conversion logic to add the isUnique check only when schema.uniqueItems is true.
Preserve the existing behavior for schemas that omit uniqueItems, and do not
install the check when it is false.
- Around line 763-779: Update the array-schema handling around the prefixItems
elements and rest calculation so numeric maxItems does not close the tuple when
prefixItems is present. Preserve an open rest representation for valid
additional items, while retaining closed behavior only when the schema
explicitly disallows them; ensure schemas like prefixItems with maxItems 3
accept arrays of lengths 2 and 3. Use collectArrayChecks and the existing
ImportedJsonSchemaRepresentation symbols.
- Around line 696-702: Update the $ref handling in on so a valid reference is
not returned alone when sibling keywords are present. Represent the referenced
schema together with the remaining schema constraints, preserving minLength,
minimum, properties, items, and other sibling keywords according to Draft
2020-12 intersection semantics; keep the standalone reference behavior only when
no additional constraints need to be applied.
In `@packages/effect/src/internal/schema/toCodeDocument.ts`:
- Around line 260-261: In packages/effect/src/internal/schema/toCodeDocument.ts
lines 260-261, update the recursives construction to insert each representation
as an enumerable own property without triggering the __proto__ setter. Apply the
same safe insertion in lines 283-287 when adding the generated Code, preserving
the existing recursive identifiers and values.
- Around line 271-277: Update the identifier-reservation setup in toCodeDocument
by seeding uniqueIdentifiers with every fixed identifier used by generated code,
including Schema, global collection names such as Array, and generated imports
such as Brand, before processing schema references. Ensure
ensureUniqueIdentifier cannot allocate any of these reserved names, and add
collision tests covering references that sanitize to reserved identifiers and
verifying valid non-self-referential output.
- Around line 101-124: Escape Literal values before toTypePart emits them as
template-literal text, handling backticks, backslashes, and `${` sequences so
generated TypeScript remains valid and literal. Add a focused helper near
toTypePart, apply it only to Literal output, and add coverage in the existing
toCodeDocument tests for each case.
- Around line 81-92: The renderLiteral type-output path must map NaN, Infinity,
and -Infinity to the TypeScript type name “number” while preserving their
existing runtime expressions. Update renderLiteral and the Schema.Literals fast
path around the identified numeric literal handling so finite numbers retain
renderNumber output and non-finite numbers emit valid type output consistently.
In `@packages/effect/src/internal/schema/toJsonSchemaDocument.ts`:
- Around line 434-445: Update getPartPattern for the Union case to wrap the
joined alternative patterns in a non-capturing group before embedding them in
template-literal regexes. Preserve the existing recursive pattern generation and
separator behavior while ensuring adjacent parts and anchors apply to the entire
union.
- Around line 343-346: Update the patternProperties emission block in
toJsonSchemaDocument so it no longer deletes out.additionalProperties when
patternProperties is present. Preserve the existing additionalProperties value,
including any broad index-signature schema assigned earlier, while still
assigning out.patternProperties.
In `@packages/effect/src/Schema.ts`:
- Around line 11656-11663: Add generated-source imports for the qualified type
references: update the HashSet schema representation’s toCode configuration
around runtime and Type generation to emit the HashSet namespace import, and
update the corresponding Chunk schema representation to emit the Chunk namespace
import. Apply the changes at packages/effect/src/Schema.ts lines 11656-11663 and
11774-11781.
- Around line 7104-7117: Escape literal string inputs before constructing
regular expressions in isStartsWith, isEndsWith, and isIncludes within
packages/effect/src/Schema.ts at lines 7104-7117, 7161-7174, and 7218-7231. Use
the escaped value for emitted JSON Schema pattern fields and
arbitrary.constraint.patterns, while preserving the existing literal string
validation behavior.
In `@packages/effect/src/unstable/ai/AnthropicStructuredOutput.ts`:
- Around line 388-390: Update the isPattern helper in
packages/effect/src/unstable/ai/AnthropicStructuredOutput.ts at lines 388-390 to
detect only the built-in pattern representation or invoke toJsonSchema with the
actual compiler-provided type and schemas; apply the same correction before
extracting pattern in packages/effect/src/unstable/ai/OpenAiStructuredOutput.ts
at lines 454-456.
In `@packages/effect/test/schema/representation/fromJson.test.ts`:
- Around line 226-300: Update the structural-value serialization/deserialization
used by SchemaRepresentation.fromJson to distinguish encoded bigint, NaN,
infinities, and negative zero from ordinary string literals and preserve -0
through JSON text round trips. Use a tagged or escaped representation
consistently in the corresponding encoder and decoder, and extend the fromJson
tests to verify round trips for literal strings "1", "NaN", "Infinity",
"-Infinity", and -0.
In `@packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts`:
- Around line 2488-2508: Update the $ref handling exercised by the “treats a
reference with an empty token as unconstrained” test so an empty reference is
not converted to the permissive JSON declaration. Explicitly reject the empty
token or represent it as a self-reference, and adjust the expected assertion to
match that behavior while preserving handling of valid references.
In `@packages/effect/test/schema/representation/toJson.test.ts`:
- Around line 292-365: Update the literal encoding and decoding used by
SchemaRepresentation.toJson and its corresponding parser so special scalar
values use an explicit type discriminator rather than bare strings or native
JSON numbers. Ensure bigint, NaN, positive and negative infinity, and negative
zero remain distinguishable from ordinary string or numeric literals and
round-trip without changing schema semantics; update the affected expectations
in the special-literal tests accordingly.
In `@packages/effect/test/schema/representation/toJsonSchemaDocument.test.ts`:
- Around line 658-663: Update the pattern generation used by
SchemaRepresentation.toJsonSchemaDocument so union alternatives are wrapped in a
non-capturing group before being embedded in the surrounding template pattern.
Preserve the existing FINITE_PATTERN and STRING_PATTERN composition while
producing anchors that apply to the complete union.
---
Outside diff comments:
In `@packages/effect/src/SchemaAST.ts`:
- Around line 3054-3064: Update isPattern so its filter callback tests against a
fresh RegExp instance or resets the original regex’s lastIndex before each test.
Preserve the existing source, flags, and schema metadata while ensuring repeated
validations are independent of prior calls.
---
Minor comments:
In @.changeset/schema-representation-refactoring.md:
- Around line 5-7: Update the SchemaRepresentation changeset to document the
public contract changes and runtime migration, including the identity/reviver
model, new serialization and revival pipelines, and the affected entry points
consumers must update. Replace the current maintainability-only description with
actionable migration guidance while retaining the constructor changes for
declaration, filter, and filter group revivers.
In
`@packages/effect/test/schema/representation/toJsonSchemaMultiDocument.test.ts`:
- Around line 169-175: Update the error-path test around
SchemaRepresentation.toJsonSchemaDocument to call the multi-document API
instead, supplying the representation through its representations collection and
preserving the missing-reference setup. Update the expected validation path to
include ["representations"][0] while retaining the Invalid reference Missing
assertion.
In `@packages/effect/typetest/schema/FromJsonSchema.tst.ts`:
- Around line 5-31: The type tests currently validate explicitly annotated
variables rather than the exported functions. Remove the intermediate
assignments and apply expect(...).type.toBe directly to
SchemaRepresentation.fromJsonSchemaDocument, fromJsonSchemaMultiDocument, and
fromSchemaMultiDocument, preserving the existing expected signatures.
In `@packages/tools/openapi-generator/src/JsonSchemaGenerator.ts`:
- Around line 205-231: Update the onEnter callback in JsonSchemaGenerator so
additionalProperties defaults to false when out.type is either the string
"object" or an array containing "object", while preserving the existing behavior
for schemas that explicitly define additionalProperties and for non-object
types.
---
Nitpick comments:
In `@packages/effect/test/schema/toCodec.test.ts`:
- Around line 1659-1675: Update the asynchronous “Json” test body in the
TestSchema.Asserts coverage to use the package’s it.effect test style instead of
an async callback. Lift the encoding and decoding assertions through the
appropriate Effect APIs while preserving every existing success case, failure
input, and expected error message.
In `@packages/effect/test/unstable/httpapi/OpenApiRepresentation.test.ts`:
- Around line 51-70: The “shares definitions and caches by API identity” test
should also verify endpoint references to the shared schema. Extend the
assertions for the OpenApi result from OpenApi.fromApi(Api) so both payload
properties, first and second, and the success response use the expected $ref to
Shared, while retaining the existing component and cache assertions.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b6b7d1b6-ca65-41cd-95ce-b53619e4ac50
📒 Files selected for processing (82)
.changeset/schema-representation-refactoring.mdmigration/schema.mdpackages/effect/SCHEMA.mdpackages/effect/src/Schema.tspackages/effect/src/SchemaAST.tspackages/effect/src/SchemaParser.tspackages/effect/src/SchemaRepresentation.tspackages/effect/src/internal/schema/annotations.tspackages/effect/src/internal/schema/fromJsonSchemaDocument.tspackages/effect/src/internal/schema/fromRepresentation.tspackages/effect/src/internal/schema/representation.tspackages/effect/src/internal/schema/representationJson.tspackages/effect/src/internal/schema/schema.tspackages/effect/src/internal/schema/toArbitrary.tspackages/effect/src/internal/schema/toCodeDocument.tspackages/effect/src/internal/schema/toEquivalence.tspackages/effect/src/internal/schema/toJsonSchemaDocument.tspackages/effect/src/internal/schema/toRepresentation.tspackages/effect/src/unstable/ai/AnthropicStructuredOutput.tspackages/effect/src/unstable/ai/OpenAiStructuredOutput.tspackages/effect/src/unstable/ai/Tool.tspackages/effect/src/unstable/ai/internal/codec-transformer.tspackages/effect/src/unstable/http/Cookies.tspackages/effect/src/unstable/http/Headers.tspackages/effect/src/unstable/http/Multipart.tspackages/effect/src/unstable/http/UrlParams.tspackages/effect/src/unstable/httpapi/OpenApi.tspackages/effect/test/schema/Schema.test.tspackages/effect/test/schema/SchemaAST.test.tspackages/effect/test/schema/representation/builtInRevivers.test.tspackages/effect/test/schema/representation/fromASTs.test.tspackages/effect/test/schema/representation/fromJson.test.tspackages/effect/test/schema/representation/fromJsonMultiDocument.test.tspackages/effect/test/schema/representation/fromJsonSchemaDocument.test.tspackages/effect/test/schema/representation/fromJsonSchemaMultiDocument.test.tspackages/effect/test/schema/representation/fromRepresentation.test.tspackages/effect/test/schema/representation/fromRepresentations.test.tspackages/effect/test/schema/representation/fromSchemaMultiDocument.test.tspackages/effect/test/schema/representation/makeCode.test.tspackages/effect/test/schema/representation/schemaJsonSchemaConsumer.test.tspackages/effect/test/schema/representation/toCodeDocument.annotations.test.tspackages/effect/test/schema/representation/toCodeDocument.test.tspackages/effect/test/schema/representation/toJson.test.tspackages/effect/test/schema/representation/toJsonMultiDocument.test.tspackages/effect/test/schema/representation/toJsonSchemaDocument.test.tspackages/effect/test/schema/representation/toJsonSchemaMultiDocument.test.tspackages/effect/test/schema/representation/toMultiDocument.test.tspackages/effect/test/schema/representation/toRepresentation.test.tspackages/effect/test/schema/representation/toRepresentations.test.tspackages/effect/test/schema/representation/toSchema.test.tspackages/effect/test/schema/toCodec.test.tspackages/effect/test/schema/toJsonSchemaDocument.test.tspackages/effect/test/unstable/ai/AnthropicStructuredOutputRepresentation.test.tspackages/effect/test/unstable/ai/LanguageModelRepresentation.test.tspackages/effect/test/unstable/ai/OpenAiStructuredOutputRepresentation.test.tspackages/effect/test/unstable/ai/ToolRepresentation.test.tspackages/effect/test/unstable/httpapi/OpenApiRepresentation.test.tspackages/effect/typetest/schema/FromJsonSchema.tst.tspackages/effect/typetest/schema/SchemaBuiltInAtomicDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInBigDecimalDurationChunkDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInBigIntRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInCauseAndExitDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInCollectionRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInDateRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInDateTimeDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInErrorAndCollectionDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInJsonAndHashDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInNumberRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInObjectRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInResultAndRedactedDeclarationRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInRevivers.tst.tspackages/effect/typetest/schema/SchemaBuiltInStringRevivers.tst.tspackages/effect/typetest/schema/SchemaJsonSchemaConsumer.tst.tspackages/effect/typetest/schema/SchemaRepresentation.tst.tspackages/effect/typetest/schema/SchemaRepresentationCompilers.tst.tspackages/effect/typetest/schema/SchemaRepresentationReviver.tst.tspackages/effect/typetest/unstable/httpapi/OpenApiRepresentation.tst.tspackages/tools/bundle/fixtures/schema-representation-roundtrip.tspackages/tools/bundle/fixtures/schema-toCodeDocument.tspackages/tools/openapi-generator/src/JsonSchemaGenerator.tspackages/tools/openapi-generator/src/OpenApiGenerator.tspackages/tools/openapi-generator/test/JsonSchemaGeneratorRepresentation.test.ts
💤 Files with no reviewable changes (6)
- packages/effect/test/schema/representation/toSchema.test.ts
- packages/effect/src/unstable/http/Headers.ts
- packages/effect/test/schema/representation/fromASTs.test.ts
- packages/effect/src/internal/schema/representation.ts
- packages/effect/src/unstable/http/Cookies.ts
- packages/effect/src/unstable/http/UrlParams.ts
Comment on lines
+511
to
+562
| case "String": | ||
| if (right._tag === "Literal") { | ||
| return satisfiesLiteral(left, right) | ||
| ? combinedAnnotations( | ||
| { | ||
| _tag: "Literal", | ||
| literal: right.literal, | ||
| checks: right.checks | ||
| }, | ||
| left, | ||
| right | ||
| ) | ||
| : { _tag: "Never", checks: [] } | ||
| } | ||
| if (right._tag !== "String") return { _tag: "Never", checks: [] } | ||
| const stringChecks = combineChecks(left.checks, right.checks, right.annotations) | ||
| return annotateJsonSchemaRepresentation( | ||
| { | ||
| _tag: "String", | ||
| checks: stringChecks ?? left.checks, | ||
| ...(right.contentMediaType ?? left.contentMediaType) === undefined | ||
| ? undefined | ||
| : { contentMediaType: right.contentMediaType ?? left.contentMediaType }, | ||
| ...(right.contentSchema ?? left.contentSchema) === undefined | ||
| ? undefined | ||
| : { contentSchema: right.contentSchema ?? left.contentSchema } | ||
| }, | ||
| mergeAnnotations(left.annotations, stringChecks === undefined ? right.annotations : undefined) | ||
| ) | ||
| case "Number": | ||
| if (right._tag === "Literal") { | ||
| return satisfiesLiteral(left, right) | ||
| ? combinedAnnotations( | ||
| { | ||
| _tag: "Literal", | ||
| literal: right.literal, | ||
| checks: right.checks | ||
| }, | ||
| left, | ||
| right | ||
| ) | ||
| : { _tag: "Never", checks: [] } | ||
| } | ||
| if (right._tag !== "Number") return { _tag: "Never", checks: [] } | ||
| const numberChecks = combineNumberChecks(left.checks, right.checks, right.annotations) | ||
| return annotateJsonSchemaRepresentation( | ||
| { | ||
| _tag: "Number", | ||
| checks: numberChecks ?? left.checks | ||
| }, | ||
| mergeAnnotations(left.annotations, numberChecks === undefined ? right.annotations : undefined) | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wrap the String and Number switch cases in blocks.
Line 526 and Line 555 declare case-local constants in the shared switch scope, triggering Biome's noSwitchDeclarations error. Add braces around both cases.
As per coding guidelines, code changes must pass pnpm lint-fix and pnpm check.
🧰 Tools
🪛 Biome (2.5.3)
[error] 526-526: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 555-555: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 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 `@packages/effect/src/internal/schema/fromJsonSchemaDocument.ts` around lines
511 - 562, Wrap the String and Number branches in the switch within explicit
blocks so their case-local declarations, including stringChecks and
numberChecks, no longer share the switch scope. Preserve all existing logic and
formatting inside each case, then ensure the changes pass pnpm lint-fix and pnpm
check.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+696
to
+702
| function on(schema: JsonSchema.JsonSchema, path: Path): ImportedJsonSchemaRepresentation { | ||
| if (typeof schema.$ref === "string") { | ||
| const $ref = jsonSchemaReferenceKey(schema.$ref) | ||
| if ($ref !== undefined) { | ||
| return { _tag: "Reference", $ref } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether sibling-keyword coverage already exists.
rg -n -C8 '\$ref|minLength|minimum|properties|items' \
packages/effect/test/schema/representation \
-g '*fromJsonSchema*.test.ts'Repository: Effect-TS/effect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the implementation and related tests, then inspect the relevant slices.
ast-grep outline packages/effect/src/internal/schema/fromJsonSchemaDocument.ts --view expanded | sed -n '1,220p'
printf '\n--- SOURCE SLICE ---\n'
sed -n '660,760p' packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n--- TEST SEARCH ---\n'
rg -n -C6 '\$ref.*(description|minLength|minimum|properties|items)|sibling|combine a reference|reference with' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts packages/effect/test/schema/representation/fromJsonSchemaMultiDocument.test.tsRepository: Effect-TS/effect
Length of output: 18089
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- combine / annotations ---'
sed -n '100,260p' packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n%s\n' '--- $ref tests ---'
sed -n '2488,4765p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts | sed -n '1,120p'Repository: Effect-TS/effect
Length of output: 8286
Apply sibling constraints alongside $ref.
This branch returns before minLength, minimum, properties, items, and similar siblings are applied, so { $ref, ... } revives as only the referenced schema. Draft 2020-12 combines $ref with sibling keywords; intersect the reference with the remaining schema instead.
🤖 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 `@packages/effect/src/internal/schema/fromJsonSchemaDocument.ts` around lines
696 - 702, Update the $ref handling in on so a valid reference is not returned
alone when sibling keywords are present. Represent the referenced schema
together with the remaining schema constraints, preserving minLength, minimum,
properties, items, and other sibling keywords according to Draft 2020-12
intersection semantics; keep the standalone reference behavior only when no
additional constraints need to be applied.
Comment on lines
+763
to
+779
| const minItems = typeof schema.minItems === "number" ? schema.minItems : 0 | ||
| const elements = Array.isArray(schema.prefixItems) | ||
| ? schema.prefixItems.map((element, index) => ({ | ||
| isOptional: index + 1 > minItems, | ||
| type: recur(element, [...path, "prefixItems", index]) | ||
| })) | ||
| : [] | ||
| const rest = schema.items !== undefined | ||
| ? [recur(schema.items, [...path, "items"])] | ||
| : schema.prefixItems !== undefined && typeof schema.maxItems === "number" | ||
| ? [] | ||
| : [{ _tag: "Unknown", checks: [] } as ImportedJsonSchemaRepresentation] | ||
| return { | ||
| _tag: "Arrays", | ||
| elements, | ||
| rest, | ||
| checks: collectArrayChecks(schema) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect tuple-bound test coverage.
rg -n -C8 'prefixItems|minItems|maxItems' \
packages/effect/test/schema/representation \
-g '*fromJsonSchema*.test.ts'Repository: Effect-TS/effect
Length of output: 30393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation and the relevant tests around tuple/rest handling.
sed -n '740,810p' packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n--- TESTS ---\n'
sed -n '1408,1588p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
printf '\n--- MORE TESTS ---\n'
sed -n '4336,4605p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.tsRepository: Effect-TS/effect
Length of output: 14702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect array check collection and how tuple closure is represented.
rg -n -C4 'function collectArrayChecks|const collectArrayChecks|collectArrayChecks\(' packages/effect/src/internal/schema/fromJsonSchemaDocument.tsRepository: Effect-TS/effect
Length of output: 743
Preserve tuple openness for prefixItems. collectArrayChecks skips minItems/maxItems whenever prefixItems is present, and any numeric maxItems currently forces rest to []. That makes { prefixItems: [...], maxItems: 3 } behave like a closed 1-item tuple and reject arrays of length 2 or 3.
🤖 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 `@packages/effect/src/internal/schema/fromJsonSchemaDocument.ts` around lines
763 - 779, Update the array-schema handling around the prefixItems elements and
rest calculation so numeric maxItems does not close the tuple when prefixItems
is present. Preserve an open rest representation for valid additional items,
while retaining closed behavior only when the schema explicitly disallows them;
ensure schemas like prefixItems with maxItems 3 accept arrays of lengths 2 and
3. Use collectArrayChecks and the existing ImportedJsonSchemaRepresentation
symbols.
Comment on lines
+838
to
+840
| if (typeof schema.uniqueItems === "boolean") { | ||
| checks.push(jsonSchemaFilter("effect/schema/isUnique", null)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit false-value coverage.
rg -n -C5 'uniqueItems' \
packages/effect/test/schema/representation \
-g '*fromJsonSchema*.test.ts'Repository: Effect-TS/effect
Length of output: 3295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== implementation ==\n'
sed -n '820,850p' packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n== uniqueItems test block ==\n'
sed -n '1608,1645p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
printf '\n== follow-up uniqueItems block ==\n'
sed -n '4278,4310p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.tsRepository: Effect-TS/effect
Length of output: 3477
Only add the uniqueness check when uniqueItems is true.
uniqueItems: false currently still installs effect/schema/isUnique, which rejects valid arrays.
🤖 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 `@packages/effect/src/internal/schema/fromJsonSchemaDocument.ts` around lines
838 - 840, Update the uniqueItems handling in the JSON Schema conversion logic
to add the isUnique check only when schema.uniqueItems is true. Preserve the
existing behavior for schemas that omit uniqueItems, and do not install the
check when it is false.
Comment on lines
+81
to
+92
| function renderLiteral(value: string | number | boolean | bigint): string { | ||
| switch (typeof value) { | ||
| case "string": | ||
| return JSON.stringify(value) | ||
| case "number": | ||
| return renderNumber(value) | ||
| case "boolean": | ||
| return globalThis.String(value) | ||
| case "bigint": | ||
| return `${value}n` | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and surrounding helpers.
printf '\n== outline: toCodeDocument.ts ==\n'
ast-grep outline packages/effect/src/internal/schema/toCodeDocument.ts --view expanded || true
printf '\n== relevant symbols ==\n'
rg -n 'renderLiteral|Literal"|renderNumber|makeCode|template|__proto__|Schema\.Literal|Type' packages/effect/src/internal/schema/toCodeDocument.ts
printf '\n== line-numbered excerpt ==\n'
sed -n '1,220p' packages/effect/src/internal/schema/toCodeDocument.ts
printf '\n== later excerpt ==\n'
sed -n '220,760p' packages/effect/src/internal/schema/toCodeDocument.tsRepository: Effect-TS/effect
Length of output: 31486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect nearby conventions and any related helpers/tests.
printf '\n== search for generated literal/type helpers in schema internals ==\n'
rg -n 'renderLiteralType|renderLiteral\(|Schema\.Literal\(|TemplateLiteral|template literal|__proto__|Object\.create|computed assignment|Object\.assign' packages/effect/src/internal packages/effect/src | sed -n '1,240p'
printf '\n== find tests for toCodeDocument ==\n'
fd -a 'toCodeDocument' packages/effect | sed -n '1,120p'
fd -a '*schema*test*' packages/effect | sed -n '1,200p'Repository: Effect-TS/effect
Length of output: 24304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== toCodeDocument tests ==\n'
sed -n '1,240p' packages/effect/test/schema/representation/toCodeDocument.test.ts
printf '\n== annotations tests ==\n'
sed -n '1,240p' packages/effect/test/schema/representation/toCodeDocument.annotations.test.ts
printf '\n== Schema.Literals / TemplateLiteral types ==\n'
rg -n 'Literals|Literal<|TemplateLiteral\.LiteralPart|type Literal' packages/effect/src/Schema.ts packages/effect/src/SchemaRepresentation.ts | sed -n '1,240p'Repository: Effect-TS/effect
Length of output: 18904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== NaN/Infinity literal coverage ==\n'
rg -n 'NaN|Infinity|-Infinity|Schema\.Literal\(|Schema\.Literals\(' packages/effect/test/schema/representation/toCodeDocument*.ts packages/effect/src/internal/schema/toCodeDocument.ts packages/effect/src/Schema.ts | sed -n '1,240p'
printf '\n== literal-related tests in toCodeDocument.test.ts ==\n'
rg -n 'Literal|Literals|TemplateLiteral' packages/effect/test/schema/representation/toCodeDocument.test.ts | sed -n '1,240p'Repository: Effect-TS/effect
Length of output: 18243
🌐 Web query:
TypeScript NaN Infinity literal type invalid literal type numeric literal NaN Infinity type position
💡 Result:
In TypeScript, NaN, Infinity, and -Infinity are not supported as valid numeric literal types [1][2]. While they are valid runtime number values, they cannot be used in a type position as specific literal types (e.g., you cannot define a type as NaN | 1 or Infinity | 5) [1][3][4]. The TypeScript team has historically declined requests to add these as literal types, citing several technical and design reasons: 1. Lack of Compelling Use Cases: The team has maintained that there is no sufficiently critical or widespread use case to justify the complexity of adding these as distinct literal types [2][4]. 2. Complexity and Edge Cases: - NaN: Handling NaN as a literal type is particularly difficult because, according to IEEE 754, NaN is not equal to itself (NaN === NaN is false) [3][5]. This creates complications for the compiler's type narrowing and equality checking logic [3]. - Infinity/NaN Type Semantics: Unlike standard numeric literals like 0 or 1, which are unit types, NaN represents an entire set of invalid or undefined operations, and Infinity does not behave as a restricted singleton in the same way standard literals do [3]. 3. Parsing and Implementation Challenges: Supporting these in a type position would require significant adjustments to the compiler's parsing scheme and internal type representation [5]. While there have been attempts to introduce these types (such as pull requests to add support), they have been rejected or reverted to maintain consistency with the existing design [6][7][8]. Currently, the narrowest type available for NaN, Infinity, and -Infinity remains the general number type [2].
Citations:
- 1: Unable to include a literal
NaN,Infinity, or-Infinityin a union type microsoft/TypeScript#47347 - 2: https://stackoverflow.com/questions/69671083/how-to-use-conditional-types-on-nan-and-infinity
- 3: NaN, Infinity and -Infinity not accepted in number literal types microsoft/TypeScript#15135
- 4: Allow Inifinity and -Infinity as number literal types microsoft/TypeScript#32277
- 5: Number, enum, and boolean literal types microsoft/TypeScript#9407
- 6: Infinity & NaN Type-level Support microsoft/TypeScript#51741
- 7: Fixed accidental possibility to create
Infinity/-Infinitynumber literal types microsoft/TypeScript#56301 - 8: Infinities as number literal types microsoft/TypeScript#56272
🌐 Web query:
site:typescriptlang.org numeric literal types NaN Infinity TypeScript type position
💡 Result:
In TypeScript, NaN and Infinity are not considered numeric literal types [1][2][3]. Numeric literal types are restricted to specific, representable numbers (e.g., 1, 42, -100), which represent the exact value that a number must have [1][2]. While NaN and Infinity are valid values of the number primitive type at runtime—consistent with JavaScript behavior [4]—they cannot be used as literal types in type positions [1][2]. Attempting to use them in contexts where literal types are expected, such as enum member definitions, is restricted; for example, TypeScript does not allow enum members to be assigned values that evaluate to NaN or Infinity, and explicitly disallows these names for enum members [5][6][7]. In summary, because literal types are intended to describe precise, finite values, NaN and Infinity fall outside the scope of the numeric literal type system [1][2].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/literal-types.html
- 2: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
- 3: https://www.typescriptlang.org/docs/handbook/2/everyday-types
- 4: https://www.typescriptlang.org/docs/handbook/typescript-from-scratch.html
- 5: https://www.typescriptlang.org/docs/handbook/enums
- 6: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-4.html
- 7: https://www.typescriptlang.org/docs/handbook/enums.html
Use number for non-finite numeric literal types (packages/effect/src/internal/schema/toCodeDocument.ts:81-92, 522-524, 632-638)
Schema.Literal(NaN), Schema.Literal(Infinity), and Schema.Literal(-Infinity) currently emit invalid TypeScript in the Type position. Keep the runtime expression, but map non-finite numbers to number for type output; the Schema.Literals(...) fast path needs the same handling.
🤖 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 `@packages/effect/src/internal/schema/toCodeDocument.ts` around lines 81 - 92,
The renderLiteral type-output path must map NaN, Infinity, and -Infinity to the
TypeScript type name “number” while preserving their existing runtime
expressions. Update renderLiteral and the Schema.Literals fast path around the
identified numeric literal handling so finite numbers retain renderNumber output
and non-finite numbers emit valid type output consistently.
Comment on lines
+388
to
+390
| function isPattern(filter: SchemaAST.Filter<any>): boolean { | ||
| return filter.annotations?.toJsonSchema?.({ type: undefined, schemas: [] }).pattern !== undefined | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve the toJsonSchema callback contract in both AI adapters.
Both helpers execute arbitrary represented-filter callbacks with type: undefined and schemas: [], although the compiler supplies the actual type and compiled operand schemas.
packages/effect/src/unstable/ai/AnthropicStructuredOutput.ts#L388-L390: detect only the built-in pattern representation, or pass the real callback inputs.packages/effect/src/unstable/ai/OpenAiStructuredOutput.ts#L454-L456: apply the same correction before extractingpattern.
📍 Affects 2 files
packages/effect/src/unstable/ai/AnthropicStructuredOutput.ts#L388-L390(this comment)packages/effect/src/unstable/ai/OpenAiStructuredOutput.ts#L454-L456
🤖 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 `@packages/effect/src/unstable/ai/AnthropicStructuredOutput.ts` around lines
388 - 390, Update the isPattern helper in
packages/effect/src/unstable/ai/AnthropicStructuredOutput.ts at lines 388-390 to
detect only the built-in pattern representation or invoke toJsonSchema with the
actual compiler-provided type and schemas; apply the same correction before
extracting pattern in packages/effect/src/unstable/ai/OpenAiStructuredOutput.ts
at lines 454-456.
Comment on lines
+226
to
+300
| it("decodes bigint structural values", () => { | ||
| assert.deepStrictEqual( | ||
| SchemaRepresentation.fromJson({ | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "1", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }), | ||
| { | ||
| representation: { _tag: "Literal", literal: 1n, checks: [] }, | ||
| references: {} | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it("decodes negative zero structural values", () => { | ||
| const document = SchemaRepresentation.fromJson({ | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: -0, | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) | ||
|
|
||
| assert.strictEqual(document.representation._tag, "Literal") | ||
| if (document.representation._tag !== "Literal") return | ||
| assert.isTrue(Object.is(document.representation.literal, -0)) | ||
| }) | ||
|
|
||
| it("decodes NaN structural values", () => { | ||
| const document = SchemaRepresentation.fromJson({ | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "NaN", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) | ||
|
|
||
| assert.strictEqual(document.representation._tag, "Literal") | ||
| if (document.representation._tag !== "Literal") return | ||
| assert.isTrue(Number.isNaN(document.representation.literal)) | ||
| }) | ||
|
|
||
| it("decodes positive infinity structural values", () => { | ||
| const document = SchemaRepresentation.fromJson({ | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "Infinity", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) | ||
|
|
||
| assert.strictEqual(document.representation._tag, "Literal") | ||
| if (document.representation._tag !== "Literal") return | ||
| assert.strictEqual(document.representation.literal, Number.POSITIVE_INFINITY) | ||
| }) | ||
|
|
||
| it("decodes negative infinity structural values", () => { | ||
| const document = SchemaRepresentation.fromJson({ | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "-Infinity", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) | ||
|
|
||
| assert.strictEqual(document.representation._tag, "Literal") | ||
| if (document.representation._tag !== "Literal") return | ||
| assert.strictEqual(document.representation.literal, Number.NEGATIVE_INFINITY) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Disambiguate encoded structural values from literal strings.
Encoding 1n, NaN, and infinities as ordinary strings collides with valid string literals containing those values; -0 is also normalized by JSON.stringify. A JSON text round trip can therefore change schema semantics. Use tagged/escaped encoding and add round-trip coverage for literal "1", "NaN", "Infinity", "-Infinity", and -0.
🤖 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 `@packages/effect/test/schema/representation/fromJson.test.ts` around lines 226
- 300, Update the structural-value serialization/deserialization used by
SchemaRepresentation.fromJson to distinguish encoded bigint, NaN, infinities,
and negative zero from ordinary string literals and preserve -0 through JSON
text round trips. Use a tagged or escaped representation consistently in the
corresponding encoder and decoder, and extend the fromJson tests to verify round
trips for literal strings "1", "NaN", "Infinity", "-Infinity", and -0.
Comment on lines
2488
to
+2508
| describe("$ref", () => { | ||
| it("treats a reference with an empty token as unconstrained", () => { | ||
| assertFromJsonSchema( | ||
| { schema: { $ref: "" } }, | ||
| { | ||
| "representation": { | ||
| "_tag": "Declaration", | ||
| "representation": { | ||
| "id": "effect/schema/Json", | ||
| "payload": null | ||
| }, | ||
| "annotations": { | ||
| "expected": "JSON value" | ||
| }, | ||
| "typeParameters": [], | ||
| "checks": [] | ||
| }, | ||
| "references": {} | ||
| } | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate $ref handling =="
rg -n '\$ref|reference' packages/effect/src packages/effect/test/schema -g '!**/dist/**' -g '!**/build/**' || true
echo
echo "== Inspect nearby test context =="
sed -n '2440,2535p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
echo
echo "== Find implementation files mentioning JsonSchemaDocument =="
fd -a 'fromJsonSchemaDocument|JsonSchema' packages/effect/src packages/effect/test/schema || trueRepository: Effect-TS/effect
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate \\$ref handling =="
rg -n '\$ref|reference' packages/effect/src packages/effect/test/schema -g '!**/dist/**' -g '!**/build/**' || true
echo
echo "== Inspect nearby test context =="
sed -n '2440,2535p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
echo
echo "== Find implementation files mentioning JsonSchemaDocument =="
fd -a 'fromJsonSchemaDocument|JsonSchema' packages/effect/src packages/effect/test/schema || trueRepository: Effect-TS/effect
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Locate $ref handling ==\n'
rg -n '\$ref|reference' packages/effect/src packages/effect/test/schema -g '!**/dist/**' -g '!**/build/**' || true
printf '\n== Inspect nearby test context ==\n'
sed -n '2440,2535p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
printf '\n== Find implementation files mentioning JsonSchemaDocument ==\n'
fd -a 'fromJsonSchemaDocument|JsonSchema' packages/effect/src packages/effect/test/schema || trueRepository: Effect-TS/effect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Relevant source files ==\n'
git ls-files 'packages/effect/**' | rg 'fromJsonSchemaDocument|JsonSchema|schema/representation' || true
printf '\n== Search for empty $ref handling in relevant source ==\n'
rg -n 'ref === ""|=== "".*\$ref|\$ref.*=== ""|startsWith\("\$"#|startsWith\("#"\)|\$ref' \
packages/effect/src packages/effect/test/schema/representation \
-g '!**/dist/**' -g '!**/build/**' || true
printf '\n== Inspect the test block around the reported lines ==\n'
sed -n '2475,2525p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.tsRepository: Effect-TS/effect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Find exact test case and nearby helpers ==\n'
rg -n '\$ref: ""|empty token|unconstrained|assertFromJsonSchema' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
printf '\n== Locate implementation file names ==\n'
fd -a 'fromJsonSchemaDocument.ts|fromJsonSchemaDocument.*' packages/effect/src packages/effect/test/schema || true
printf '\n== Search source for JsonSchema references ==\n'
rg -n 'fromJsonSchemaDocument|JsonSchemaDocument|resolve.*\$ref|\$ref' packages/effect/src -g '!**/dist/**' -g '!**/build/**' | head -n 200Repository: Effect-TS/effect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Candidate TS files ==\n'
git ls-files 'packages/effect/src/**/*.ts' | rg 'schema|json|ref|fromJson' || true
printf '\n== Exact test block ==\n'
sed -n '2480,2515p' packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts
printf '\n== Search only TS source for ref handling ==\n'
rg -n '\$ref|fromJsonSchemaDocument|JsonSchemaDocument|empty token|unconstrained' \
packages/effect/src/**/*.ts packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts \
|| trueRepository: Effect-TS/effect
Length of output: 5619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Outline fromJsonSchemaDocument implementation ==\n'
ast-grep outline packages/effect/src/internal/schema/fromJsonSchemaDocument.ts --view expanded || true
printf '\n== $ref-related implementation slices ==\n'
rg -n '\$ref|ref|unconstrained|Definition|Reference|suspend' packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n== Relevant source around the first $ref handling ==\n'
sed -n '1,260p' packages/effect/src/internal/schema/fromJsonSchemaDocument.tsRepository: Effect-TS/effect
Length of output: 12855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Find $ref occurrences with line numbers ==\n'
rg -n '\$ref' packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n== File size ==\n'
wc -l packages/effect/src/internal/schema/fromJsonSchemaDocument.ts
printf '\n== Inspect around each $ref occurrence ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("packages/effect/src/internal/schema/fromJsonSchemaDocument.ts")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "$ref" in line:
start = max(1, i - 18)
end = min(len(lines), i + 30)
print(f"\n--- lines {start}-{end} around {i} ---")
for j in range(start, end + 1):
print(f"{j}: {lines[j-1]}")
PYRepository: Effect-TS/effect
Length of output: 24316
Don't widen empty $ref to Json
jsonSchemaReferenceKey drops "", so { $ref: "" } falls through to the permissive JSON declaration. Reject it explicitly or model it as a self-reference instead.
🤖 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 `@packages/effect/test/schema/representation/fromJsonSchemaDocument.test.ts`
around lines 2488 - 2508, Update the $ref handling exercised by the “treats a
reference with an empty token as unconstrained” test so an empty reference is
not converted to the permissive JSON declaration. Explicitly reject the empty
token or represent it as a self-reference, and adjust the expected assertion to
match that behavior while preserving handling of valid references.
Comment on lines
+292
to
+365
| it("encodes bigint structural values", () => { | ||
| assert.deepStrictEqual( | ||
| SchemaRepresentation.toJson(SchemaRepresentation.toRepresentation(Schema.Literal(1n).ast)), | ||
| { | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "1", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it("encodes negative zero structural values", () => { | ||
| assert.deepStrictEqual( | ||
| SchemaRepresentation.toJson(SchemaRepresentation.toRepresentation(Schema.Literal(-0).ast)), | ||
| { | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: -0, | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it("encodes NaN structural values", () => { | ||
| const document: SchemaRepresentation.Document = { | ||
| representation: { _tag: "Literal", literal: Number.NaN, checks: [] }, | ||
| references: {} | ||
| } | ||
|
|
||
| assert.deepStrictEqual(SchemaRepresentation.toJson(document), { | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "NaN", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) | ||
| }) | ||
|
|
||
| it("encodes positive infinity structural values", () => { | ||
| const document: SchemaRepresentation.Document = { | ||
| representation: { _tag: "Literal", literal: Number.POSITIVE_INFINITY, checks: [] }, | ||
| references: {} | ||
| } | ||
|
|
||
| assert.deepStrictEqual(SchemaRepresentation.toJson(document), { | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "Infinity", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) | ||
| }) | ||
|
|
||
| it("encodes negative infinity structural values", () => { | ||
| const document: SchemaRepresentation.Document = { | ||
| representation: { _tag: "Literal", literal: Number.NEGATIVE_INFINITY, checks: [] }, | ||
| references: {} | ||
| } | ||
|
|
||
| assert.deepStrictEqual(SchemaRepresentation.toJson(document), { | ||
| representation: { | ||
| _tag: "Literal", | ||
| literal: "-Infinity", | ||
| checks: [] | ||
| }, | ||
| references: {} | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use an injective encoding for special literal values.
These expectations encode 1n, NaN, and infinities as ordinary strings, making them indistinguishable from valid string literals such as "1" or "NaN" under the same Literal tag. Native JSON serialization also collapses -0 to 0. Add an explicit scalar-type discriminator so representation round trips cannot change schema semantics.
🤖 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 `@packages/effect/test/schema/representation/toJson.test.ts` around lines 292 -
365, Update the literal encoding and decoding used by
SchemaRepresentation.toJson and its corresponding parser so special scalar
values use an explicit type discriminator rather than bare strings or native
JSON numbers. Ensure bigint, NaN, positive and negative infinity, and negative
zero remain distinguishable from ordinary string or numeric literals and
round-trip without changing schema semantics; update the affected expectations
in the special-literal tests accordingly.
Comment on lines
+658
to
+663
| assert.deepStrictEqual( | ||
| SchemaRepresentation.toJsonSchemaDocument({ representation, references: {} }).schema, | ||
| { | ||
| type: "string", | ||
| pattern: `^p${SchemaAST.FINITE_PATTERN}x${SchemaAST.STRING_PATTERN}a|b$` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Group union alternatives before embedding them in the template pattern.
Line 662 produces ^p...a|b$, which means “starts with p...a or ends with b.” The compiler must emit a grouped union such as (?:a|b) before applying the surrounding anchors.
Proposed test expectation
- pattern: `^p${SchemaAST.FINITE_PATTERN}x${SchemaAST.STRING_PATTERN}a|b$`
+ pattern: `^p${SchemaAST.FINITE_PATTERN}x${SchemaAST.STRING_PATTERN}(?:a|b)$`📝 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
| assert.deepStrictEqual( | |
| SchemaRepresentation.toJsonSchemaDocument({ representation, references: {} }).schema, | |
| { | |
| type: "string", | |
| pattern: `^p${SchemaAST.FINITE_PATTERN}x${SchemaAST.STRING_PATTERN}a|b$` | |
| } | |
| assert.deepStrictEqual( | |
| SchemaRepresentation.toJsonSchemaDocument({ representation, references: {} }).schema, | |
| { | |
| type: "string", | |
| pattern: `^p${SchemaAST.FINITE_PATTERN}x${SchemaAST.STRING_PATTERN}(?:a|b)$` | |
| } |
🤖 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 `@packages/effect/test/schema/representation/toJsonSchemaDocument.test.ts`
around lines 658 - 663, Update the pattern generation used by
SchemaRepresentation.toJsonSchemaDocument so union alternatives are wrapped in a
non-capturing group before being embedded in the surrounding template pattern.
Preserve the existing FINITE_PATTERN and STRING_PATTERN composition while
producing anchors that apply to the complete union.
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
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
2 participants
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.
@coderabbitai ignore