Skip to content

fix(catalog): accept current live Models.dev schema in parser (#4185)#4191

Merged
Hmbown merged 1 commit into
mainfrom
codex/v0868-fix-4185
Jul 8, 2026
Merged

fix(catalog): accept current live Models.dev schema in parser (#4185)#4191
Hmbown merged 1 commit into
mainfrom
codex/v0868-fix-4185

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes #4185 — the foundation of the Models.dev catalog chain (#4184#4185#4187#4186#4188).

Live https://models.dev/catalog.json sends the provider-row interleaved field in two shapes:

  • a bare boolean — interleaved: true (32 provider rows as of 2026-07-07, e.g. vercel, amazon-bedrock, azure, togetherai), and
  • an object — interleaved: { "field": "reasoning_content" } (the majority, 530 rows).

The parser modeled only the object shape (ModelsDevInterleaved was a struct with an optional field), so serde_json::from_str::<ModelsDevCatalog> rejected every boolean row and the live catalog could not be parsed at all.

Change

Scope is strictly parser tolerance. No bundled-data changes (that's #4188), no picker wiring (separate lane). Only crates/config/src/models_dev.rs is touched; ModelsDevInterleaved has no other consumers in the workspace and the bundled snapshot never used the field.

Verification

Confirmed against a live catalog.json fetch on 2026-07-07: 151 providers, 32 boolean interleaved rows, 530 object rows, no other shapes.

  • cargo fmt --all --check — clean
  • cargo clippy --workspace --all-features --locked (with the repo's -D warnings + agreed -A set) — clean
  • cargo test --workspace --locked — passes, except one pre-existing, environment-dependent failure unrelated to this change (tui::hotbar::actions::tests::skill_hotbar_action_activates_skill_through_dollar_alias, "skill registered from the startup skill cache"), which fails identically on pristine origin/main with this change stashed out.
  • cargo build --release -p codewhale-tui — succeeds.

New tests in crates/config/src/models_dev.rs cover: interleaved: true, interleaved: false, interleaved: { "field": "..." }, empty/unknown-key objects, and a mixed live-ish sample with unmodeled provider/model keys.

Acceptance (from #4185)

  • A fixture with interleaved: true parses successfully.
  • A fixture with interleaved: false parses successfully.
  • A fixture with interleaved: { "field": "..." } parses successfully.
  • Existing bundled Models.dev fixture tests still pass.
  • A live-ish sample from catalog.json can deserialize without schema errors.
  • The parser still ignores unknown upstream fields safely.

Live models.dev/catalog.json sends `interleaved` in two shapes: a bare
boolean (`interleaved: true`, ~32 provider rows) and an object
(`interleaved: { "field": "reasoning_content" }`, the majority). The
parser modeled only the object form, so `from_str::<ModelsDevCatalog>`
rejected every boolean row before the live catalog could be used.

Replace the object-only `ModelsDevInterleaved` struct with an untagged
enum that accepts both shapes, and add `is_enabled()` / `field()`
accessors so downstream consumers read the metadata without matching the
enum. The object variant keeps `field` optional and ignores unknown
sibling keys, so partial/empty objects and future keys still parse.

Scope is strictly parser tolerance: no bundled data changes, no picker
wiring. Verified against a live catalog.json fetch on 2026-07-07
(151 providers; 32 boolean rows, 530 object rows, no other shapes).

Tests cover boolean true/false, object-with-field, empty/unknown-key
objects, and a mixed live-ish sample with unmodeled provider/model keys.
Copilot AI review requested due to automatic review settings July 8, 2026 02:49

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 1m 29s —— View job


Claude finished reviewing

  • Gather context (diff, file, CLAUDE.md)
  • Understand request (code review of the parser fix)
  • Review correctness (serde untagged enum, edge cases)
  • Review provider/model/route safety implications
  • Review reuse/simplification
  • Review test coverage
  • Review security
  • Post final review

Summary

This is a clean, tightly-scoped fix. ModelsDevInterleaved correctly moves from an object-only struct to a #[serde(untagged)] enum with Enabled(bool) and Field { field: Option<String> } variants, which resolves the actual bug: the live catalog.json's bare-boolean interleaved rows (~32 providers) were previously rejected outright by serde_json::from_str::<ModelsDevCatalog>.

Correctness

  • The two variants are structurally disjoint (scalar bool vs. object), so there's no #[serde(untagged)] ambiguity risk — deserialization is deterministic.
  • field: Option<String> with #[serde(default)] correctly tolerates {}, {"field": "..."}, and objects with unknown sibling keys (untagged + missing deny_unknown_fields means extra keys are ignored, as intended).
  • is_enabled() treats any object form as enabled (matches the stated upstream invariant: object form only appears on interleaved-capable rows) and the boolean form reports its literal value. Reasonable and documented.
  • Dropping the Default derive from ModelsDevInterleaved is safe — confirmed no callers in the workspace relied on it (the field is Option<ModelsDevInterleaved> at the call site, crates/config/src/models_dev.rs:230, so None covers the "absent" case instead).

Provider/model/route safety (EPIC #2608)

  • No interaction with provider/model selection or routing logic here — interleaved is purely reasoning-wire metadata, not used to infer provider identity from prefixes or freeform text. No concern.

Reuse/simplification

Tests

  • Good coverage: bare true/false, object with field, empty object, object with unknown keys, and a mixed "live-ish" sample with an unmodeled provider key (doc) and unmodeled model key (some_future_flag) to prove forward-compatibility. This matches all of the acceptance criteria from Accept current live Models.dev schema in the catalog parser #4185.

Security

  • No secret handling, exec, or input-validation concerns — this is pure data-shape parsing from a JSON catalog.

Note on verification: I wasn't able to run cargo test/cargo clippy in this sandboxed environment (cargo invocations require approval that isn't available here), so I relied on static review of the diff and full surrounding file context. The PR description states these all pass locally except a pre-existing, unrelated flaky test (skill_hotbar_action_activates_skill_through_dollar_alias), which is consistent with what I'd expect from this diff.

No blocking issues found. This looks ready to merge pending the actual CI run.

@Hmbown Hmbown merged commit d6fc732 into main Jul 8, 2026
19 checks passed
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.

Accept current live Models.dev schema in the catalog parser

2 participants