Handle accepted undefined fields during variant extraction - #6974
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 228c1eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR is currently a repro-only commit: the title and commit message claim the bug is handled, but packages/effect/src/unstable/schema/VariantSchema.ts still throws. The new regression test fails against the unchanged implementation, so CI is red.
Reviewed changes
- Added a focused regression test in
packages/effect/test/unstable/schema/VariantSchema.test.tscoveringVariantSchema.Structfields declared asundefined.
The test asserts the correct behavior: a field whose value is undefined must be omitted from the extracted Schema.Struct. Before merging, the same branch needs the runtime fix in VariantSchema.extract (and a test covering the wrapped-undefined Field case).
⚠️ Missing implementation fix
VariantSchema.extract checks TypeId in value before verifying that value is defined. Because both Struct.Fields and Field.Config explicitly accept undefined, the in operator throws a native TypeError. A minimal fix is to skip the key when value === undefined before any branch inspection, and to do the same inside the Field branch when value.schemas[variant] is undefined.
Technical details
# Runtime crash on accepted `undefined` fields
## Affected sites
- `packages/effect/src/unstable/schema/VariantSchema.ts:225` — `TypeId in value` throws when `value` is `undefined`
- `packages/effect/src/unstable/schema/VariantSchema.ts:222-238` — `extract` does not short-circuit on `undefined` values
## Required outcome
- Extracting a `VariantSchema.Struct` whose `Fields` contain `undefined` values must omit those keys without throwing.
- Extracting a `Field` whose variant config maps the requested variant to `undefined` must also omit the field.
- Existing tests for non-undefined variants continue to pass.
## Suggested approach (optional)
At the top of the for-body in `extract`, skip when `value === undefined`. In the `FieldTypeId` branch, check that `value.schemas[variant]` is also `Schema.Top` before assigning it.@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
066b485 to
6c2ddf0
Compare
6c2ddf0 to
228c1eb
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The new commits since the prior review complete the implementation fix and extend test coverage.
- Fixed
VariantSchema.extractto skipundefinedstruct fields before inspectingTypeIdorFieldTypeId, and to skipFieldvariant mappings that resolve toundefined. - Added a regression test for the wrapped-undefined
Fieldcase requested in the previous review. - Adjusted
Struct.Validateto returnunknownfor non-struct/non-field entries and widenedField.ConfigWithKeystoSchema.Top | undefined, matching the runtime acceptance ofundefined. - Added a patch changeset for the fix.
Prior pullfrog feedback has been addressed.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Variant extraction throws a native TypeError for direct or wrapped undefined fields that the public field types explicitly accept.
Important
This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.
Accepted undefined fields crash variant extraction
Module:
schema/VariantSchemaAudit ID:
unstable-ai-cli-variant-schema-undefined-field-crashSeverity / confidence: medium / high
What happens
Variant extraction throws a native TypeError for direct or wrapped undefined fields that the public field types explicitly accept.
Why it happens
Extraction evaluates TypeId in value before checking value and can also pass an undefined wrapped schema to Schema.Struct.
Expected behavior
Struct.Fields and Field.Fields accept undefined so conditionally omitted fields can be represented without crashing extraction.
Relevant implementation
These links and excerpts are pinned to audit base
c9b56ab507f224426ee8388dc450da447ec4715f.packages/effect/src/unstable/schema/VariantSchema.ts:66-78packages/effect/src/unstable/schema/VariantSchema.ts:150-163packages/effect/src/unstable/schema/VariantSchema.ts:222-239View problematic code at
packages/effect/src/unstable/schema/VariantSchema.ts:66-78View exact lines on GitHub
View problematic code at
packages/effect/src/unstable/schema/VariantSchema.ts:150-163View exact lines on GitHub
View problematic code at
packages/effect/src/unstable/schema/VariantSchema.ts:222-239View exact lines on GitHub
Reproduction
pnpm test --run packages/effect/test/unstable/schema/VariantSchema.test.tsObserved failure: FAIL: extraction threw a native TypeError.
Implementation handoff
The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.
pnpm test --run packages/effect/test/unstable/schema/VariantSchema.test.tsAudit provenance
c9b56ab507f224426ee8388dc450da447ec4715fc9b56ab507f224426ee8388dc450da447ec4715funstable-ai-cli-variant-schema-undefined-field-crashCloses EFF-415