Skip to content

Separate the default variant cache key from named variants - #6973

Merged
tim-smart merged 2 commits into
mainfrom
audit/repro-unstable-schema-variant-schema-default-cache-key
Aug 5, 2026
Merged

Separate the default variant cache key from named variants#6973
tim-smart merged 2 commits into
mainfrom
audit/repro-unstable-schema-variant-schema-default-cache-key

Conversation

@fubhy

@fubhy fubhy commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

A configured variant literally named __default can return the selected default variant's schema instead of its own.

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.

Variant __default collides with the default cache key

Module: schema/VariantSchema
Audit ID: unstable-ai-cli-variant-schema-default-cache-collision
Severity / confidence: medium / high

What happens

A configured variant literally named __default can return the selected default variant's schema instead of its own.

Why it happens

Default extraction and the ordinary __default variant share the same string cache key, so whichever is extracted first populates the other's entry.

Expected behavior

Every accepted string in options.variants extracts its configured schema independently of the selected default variant.

Relevant implementation

These links and excerpts are pinned to audit base c9b56ab507f224426ee8388dc450da447ec4715f.

View problematic code at packages/effect/src/unstable/schema/VariantSchema.ts:28
const cacheSymbol = Symbol.for(`${TypeId}/cache`)

View exact lines on GitHub

View problematic code at packages/effect/src/unstable/schema/VariantSchema.ts:198-220
const extract: {
  <V extends string, const IsDefault extends boolean = false>(
    variant: V,
    options?: {
      readonly isDefault?: IsDefault | undefined
    }
  ): <A extends Struct<any>>(self: A) => Extract<V, A, IsDefault>
  <V extends string, A extends Struct<any>, const IsDefault extends boolean = false>(self: A, variant: V, options?: {
    readonly isDefault?: IsDefault | undefined
  }): Extract<V, A, IsDefault>
} = dual(
  (args) => isStruct(args[0]),
  <V extends string, A extends Struct<any>>(
    self: A,
    variant: V,
    options?: {
      readonly isDefault?: boolean | undefined
    }
  ): Extract<V, A> => {
    const cache = self[cacheSymbol] ?? (self[cacheSymbol] = Object.create(null))
    const cacheKey = options?.isDefault === true ? "__default" : variant
    if (Object.hasOwn(cache, cacheKey)) {
      return cache[cacheKey] as any

View exact lines on GitHub

View problematic code at packages/effect/src/unstable/schema/VariantSchema.ts:486-491
  const extractVariants = dual(
    2,
    (self: Struct<any>, variant: string): any =>
      extract(self, variant, {
        isDefault: variant === options.defaultVariant
      })

View exact lines on GitHub

Reproduction

pnpm test --run packages/effect/test/unstable/schema/VariantSchema.test.ts

Observed failure: FAIL: the named variant was replaced by the cached default schema.

Implementation handoff

The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.

  1. Start with the pinned implementation excerpts and the Why it happens analysis above.
  2. Change the implementation so it satisfies the stated Expected behavior; do not weaken or remove the reproduction assertions.
  3. Run the focused reproduction command(s) and confirm the observed failures become passing tests:
pnpm test --run packages/effect/test/unstable/schema/VariantSchema.test.ts
  1. Run the affected package's existing tests, then the repository lint and type checks before requesting review.

Audit provenance

  • Audit base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Reproduction base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Findings: unstable-ai-cli-variant-schema-default-cache-collision
  • Initial patch: focused reproduction tests; implementation fix pending

Closes EFF-414

@fubhy fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 4, 2026
@changeset-bot

changeset-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 62087e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
effect Patch
@effect/ai-anthropic Patch
@effect/ai-openai Patch
@effect/ai-openai-compat Patch
@effect/ai-openrouter Patch
@effect/atom-react Patch
@effect/atom-solid Patch
@effect/atom-vue Patch
@effect/docgen Patch
@effect/doctest Patch
@effect/openapi-generator Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-deno Patch
@effect/platform-node Patch
@effect/platform-node-shared Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-pglite Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-do Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/vitest Patch

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

@effect-slopcop effect-slopcop Bot added 4.0 bug Something isn't working labels Aug 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The regression test correctly reproduces the cache collision, but this PR only adds the test. The VariantSchema.ts implementation fix required by the PR title is not present, so CI will remain failing.

Reviewed changes

  • Added one regression test in packages/effect/test/unstable/schema/VariantSchema.test.ts that verifies a variant literally named __default extracts its own schema even after the default variant has been extracted.

⚠️ Implementation fix is missing

The test fails as expected against main because extract still uses a single cache key "__default" for both the default-variant extraction and the literal __default variant:

  • packages/effect/src/unstable/schema/VariantSchema.ts:218const cacheKey = options?.isDefault === true ? "__default" : variant
  • packages/effect/src/unstable/schema/VariantSchema.ts:28cacheSymbol declares a single cache.

To satisfy the expected behavior stated in the PR description, the named-variant cache and the default-variant cache need to be separated (e.g., a second defaultCacheSymbol/cache object, or an equivalent namespacing scheme). Remember to also update the [cacheSymbol]?: Record<string, Schema.Top> type in Struct's interface if the cache structure changes.

Technical details
# Separate default-variant cache from named-variant cache

## Affected sites
- `packages/effect/src/unstable/schema/VariantSchema.ts:28-40` — type of `Struct` cache may need adjustment
- `packages/effect/src/unstable/schema/VariantSchema.ts:216-241` — cache-key logic collides

## Required outcome
- Extracting the configured default variant must not pollute the cache entry used by a literal variant named `__default`.
- Extracting a literal `"__default"` variant must not pollute the default-variant cache entry.
- Existing tests and the new regression test pass.

## Suggested approach
Introduce a separate cache for default-variant extractions (e.g., `defaultCacheSymbol`). Use `variant` as the key within each cache. The `isDefault` check should route to the appropriate cache bucket.

ℹ️ Nitpicks

  • Consider adding a second assertion or parameterizing the test so it also covers the reverse order: extract __default first, then extract the default variant. That proves the collision is avoided regardless of which entry populates the cache first.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread packages/effect/test/unstable/schema/VariantSchema.test.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Added a second defaultCacheSymbol-backed cache in packages/effect/src/unstable/schema/VariantSchema.ts and routed default-variant extractions through it, removing the "__default" sentinel collision with a literal __default variant.
  • Updated the Struct interface to declare both [cacheSymbol] and [defaultCacheSymbol] optional caches.
  • Expanded the regression test in packages/effect/test/unstable/schema/VariantSchema.test.ts to cover both extraction orders: default variant first, then literal __default; and literal __default first, then default variant.
  • Added a patch changeset in .changeset/fix-variant-schema-default-cache.md describing the cache separation.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@tim-smart
tim-smart force-pushed the audit/repro-unstable-schema-variant-schema-default-cache-key branch 2 times, most recently from 1fde2db to 538e97e Compare August 4, 2026 23:25
@tim-smart
tim-smart enabled auto-merge (squash) August 4, 2026 23:31

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Re-reviewed the branch after the force-push/amend that replaced the previously approved commits. The cache-separation fix remains equivalent to the previously approved version:

  • Added defaultCacheSymbol and routed isDefault: true extractions to the separate default-variant cache in packages/effect/src/unstable/schema/VariantSchema.ts.
  • Updated the Struct interface to declare both optional caches.
  • Expanded the regression test in packages/effect/test/unstable/schema/VariantSchema.test.ts to cover both cache-population orders.
  • Added the patch changeset in .changeset/fix-variant-schema-default-cache.md.

Validation run: pnpm --filter effect test --run test/unstable/schema/VariantSchema.test.ts, pnpm lint-fix, and pnpm check all pass.

ℹ️ PR description is now stale

The PR body still describes the implementation fix as pending and says CI is expected to fail. The fix is present on the branch, so that section is now misleading.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Bundle Size Analysis

Generated from PR build output; treat the content below as untrusted.

File Name Current Size Previous Size Difference
basic.ts 7.06 KB 7.06 KB 0.00 KB (0.00%)
batching.ts 9.86 KB 9.86 KB 0.00 KB (0.00%)
brand.ts 6.34 KB 6.34 KB 0.00 KB (0.00%)
cache.ts 10.71 KB 10.71 KB 0.00 KB (0.00%)
config.ts 20.60 KB 20.60 KB 0.00 KB (0.00%)
differ.ts 20.20 KB 20.20 KB 0.00 KB (0.00%)
http-client.ts 21.53 KB 21.53 KB 0.00 KB (0.00%)
logger.ts 10.84 KB 10.84 KB 0.00 KB (0.00%)
metric.ts 8.98 KB 8.98 KB 0.00 KB (0.00%)
optic.ts 7.18 KB 7.18 KB 0.00 KB (0.00%)
pubsub.ts 14.99 KB 14.99 KB 0.00 KB (0.00%)
queue.ts 11.66 KB 11.66 KB 0.00 KB (0.00%)
schedule.ts 10.83 KB 10.83 KB 0.00 KB (0.00%)
schema-class.ts 19.14 KB 19.14 KB 0.00 KB (0.00%)
schema-fromJsonSchemaDocument.ts 28.96 KB 28.96 KB 0.00 KB (0.00%)
schema-representation-roundtrip.ts 25.29 KB 25.29 KB 0.00 KB (0.00%)
schema-string-transformation.ts 13.38 KB 13.38 KB 0.00 KB (0.00%)
schema-string.ts 10.94 KB 10.94 KB 0.00 KB (0.00%)
schema-template-literal.ts 15.17 KB 15.17 KB 0.00 KB (0.00%)
schema-toArbitraryLazy.ts 21.94 KB 21.94 KB 0.00 KB (0.00%)
schema-toCodeDocument.ts 24.34 KB 24.34 KB 0.00 KB (0.00%)
schema-toCodecJson.ts 19.18 KB 19.18 KB 0.00 KB (0.00%)
schema-toEquivalence.ts 19.01 KB 19.01 KB 0.00 KB (0.00%)
schema-toFormatter.ts 18.87 KB 18.87 KB 0.00 KB (0.00%)
schema-toJsonSchemaDocument.ts 22.60 KB 22.60 KB 0.00 KB (0.00%)
schema-toRepresentation.ts 19.52 KB 19.52 KB 0.00 KB (0.00%)
schema.ts 18.41 KB 18.41 KB 0.00 KB (0.00%)
stm.ts 12.63 KB 12.63 KB 0.00 KB (0.00%)
stream.ts 9.80 KB 9.80 KB 0.00 KB (0.00%)

@tim-smart
tim-smart force-pushed the audit/repro-unstable-schema-variant-schema-default-cache-key branch from 538e97e to 62087e7 Compare August 5, 2026 00:03

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Re-reviewed after the force-push to 62087e7a. The new commits contain the same cache-separation fix as the previously approved version:

  • Added defaultCacheSymbol in packages/effect/src/unstable/schema/VariantSchema.ts and routed isDefault: true extractions to the separate default-variant cache.
  • Updated the Struct interface to declare both [cacheSymbol] and [defaultCacheSymbol] optional caches.
  • Expanded the regression test in packages/effect/test/unstable/schema/VariantSchema.test.ts to cover both cache-population orders.
  • Added the patch changeset in .changeset/fix-variant-schema-default-cache.md.

Validation run: pnpm --filter effect test --run test/unstable/schema/VariantSchema.test.ts, pnpm check, and pnpm lint-fix all pass.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@tim-smart
tim-smart merged commit 1949439 into main Aug 5, 2026
19 of 20 checks passed
@tim-smart
tim-smart deleted the audit/repro-unstable-schema-variant-schema-default-cache-key branch August 5, 2026 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.0 audit Findings originating from the Effect runtime correctness audit bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants