Skip to content

fix(downgrader): keep binary content in 3.1 to 3.0 - #22

Merged
dinwwwh merged 6 commits into
mainfrom
claude/binary-content-3-1-to-3-0-88d499
Sep 27, 2026
Merged

dinwwwh merged 6 commits into
mainfrom
claude/binary-content-3-1-to-3-0-88d499

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 27, 2026

Copy link
Copy Markdown
Member

The 3.1 → 3.0 downgrader no longer drops binary content. contentMediaType: image/png used to come out as {}, and only application/octet-stream became format: binary. Now raw and base64-encoded binary map to 3.0's format: binary and format: byte with type: string, following the spec's binary migration table. Upload schemas now reach 3.0 code generators as files or byte fields instead of any.

Fixes

3.1 3.0 before 3.0 after
{ contentMediaType: 'image/png' } {} { type: 'string', format: 'binary' }
{ contentMediaType: 'application/octet-stream' } { format: 'binary' } { type: 'string', format: 'binary' }
{ contentEncoding: 'base64' } { format: 'byte' } { type: 'string', format: 'byte' }
  • type: string is added only when type is missing, and an existing format is kept.
  • Schemas whose type excludes string (e.g. object) get neither keyword, since content keywords only apply to strings.
  • base64url and other encodings are still dropped: 3.0's byte is RFC 4648 section 4 base64 only.
  • The README's schema table and "Removed" list now describe these keywords.

For reviewers

  • Adding type: string to a schema with no type tightens validation, the one place this converter does. It is what the spec's table prescribes, and 3.0 tools ignore format without type.
  • Every media type maps to binary, including text ones. So a JSON-in-a-string schema like { type: 'string', contentMediaType: 'application/json' } also becomes format: binary. Telling text and binary media types apart would need a guessed list, so this keeps one rule.

Testing

  • Unit cases cover raw, encoded, nullable, multi-type, existing format, non-string type, and base64url.
  • A new e2e test downgrades a raw image/png body and a base64 text/plain body and validates the result against the 3.0 schema.
  • pnpm test (360 tests), pnpm lint and pnpm type:check pass. Existing snapshots are unchanged.

Raw binary (`contentMediaType` without `contentEncoding`) now becomes
`type: string` + `format: binary` for every media type, not just
`application/octet-stream`, and `contentEncoding: base64` now also gets
`type: string` beside `format: byte`, following the spec's binary
migration table. Before, `contentMediaType: image/png` became `{}`.
@codecov

codecov Bot commented Sep 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ No critical issues — minor soundness suggestion inline.

Reviewed changes

  • Content keyword conversion — new getContentFormat plus a reworked convertContentKeywords now map contentEncoding: base64 → format: byte and any string contentMediaType → format: binary, adding type: string when the source omits type and preserving an existing format or non-string type.
  • Shared type helper — new hasType powers the content-keyword guard and the XML wrapped check (semantics unchanged).
  • Tests — unit matrix for raw, encoded, nullable, multi-type, existing-format, non-string-type, and base64url inputs, plus a new e2e test that downgrades raw and base64 bodies and validates the result as 3.0.
  • README — schema table and "Removed" list updated to describe the new mappings.

The spec's binary migration table (OAS 3.1.1 §4.4.2.1) matches the mappings, and pnpm test passes locally (360 tests).

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

Comment thread packages/downgrader/src/v3.1-to-v3.0.ts
Adding type: string next to a non-string const or enum made the 3.0 schema accept nothing, while the 3.1 source accepted those values. format is still added, since it only applies to strings.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ Prior feedback addressed for const/enum; one residual case inline.

Reviewed changes

  • Guarded the type: 'string' injection — new hasNonStringValue keeps the injected type: 'string' off schemas whose const or enum carries a non-string value, fixing the unsatisfiable outputs flagged last review. All-string enums still get type: 'string'.
  • Tests — the content-keyword matrix now covers an all-string enum, a non-string const, and an enum with a non-string entry alongside the binary/byte formats.
  • README — the contentEncoding: base64 row documents the new "every const/enum value is a string" condition, and the "Removed" list names a non-base64 contentEncoding with its contentMediaType.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

Comment thread packages/downgrader/src/v3.1-to-v3.0.ts Outdated
$ref, allOf, anyOf, oneOf and not already limit the value type, so adding type: string beside them could make the 3.0 schema accept nothing or drop values the source accepted, such as null in a nullable anyOf. format is still added.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

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

  • Skipped type: string beside type-constraining siblings — hasNonStringValue became hasTypeConstraint, now also true for $ref, allOf, anyOf, oneOf, and not, so the injected type: 'string' no longer sits beside a combinator/ref that pins the value type. format is still emitted at the parent level (non-validating in 3.0).
  • Tests — exact unit assertions pin the allOf, nullable anyOf, and $ref cases; each would fail without the guard.
  • README — the contentEncoding: base64 row now lists the $ref / combinator keywords alongside the non-string const / enum condition.

This closes the residual unsatisfiable-output class raised in both prior threads. pnpm test passes locally (365 tests) and the checked-out converter confirms the new outputs.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

Drop the guard that skipped type: string beside $ref, composition keywords, and non-string const or enum. It only protected self-contradictory schemas (binary content that is also a number or an object) that generators do not emit, so the simpler rule wins.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

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

Since the prior pullfrog review (664d285), commit 33b3d5d replaced the hasTypeConstraint guard with a single rule.

  • Dropped the type-constraint guard — convertContentKeywords now injects type: string whenever the source omits type; the $ref / allOf / anyOf / oneOf / not and non-string const / enum checks are gone.
  • Pruned the guard tests — the six rows pinning the skipped-injection cases (non-string const, mixed enum, all-string enum, allOf, nullable anyOf, $ref) were removed.
  • Simplified the README row — the contentEncoding: base64 description now reads "plus type: string when type is missing".

This intentionally reintroduces the unsatisfiable-output behavior the two prior threads raised: { const: 42, contentEncoding: 'base64' }, { contentMediaType: 'image/png', enum: ['x', null] }, and { contentMediaType: 'image/png', allOf: [{ type: 'object' }] } again emit type: string beside a non-string constraint. The author documented the rationale in-thread (contradictory source intent, generators don't emit these shapes, shorter code), so no change is requested. The realistic nullable shape type: ['string', 'null'] still converts to { format: 'binary', nullable: true, type: 'string' }, and pnpm test passes locally (359 tests).

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit 74c8a3a into main Sep 27, 2026
7 checks passed
@dinwwwh
dinwwwh deleted the claude/binary-content-3-1-to-3-0-88d499 branch September 27, 2026 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant