feat: persist the live model catalog so restarts serve current models - #79
Conversation
The models.list fallback used to be only the baked-in MODEL_CATALOG,
which goes stale between codeoid releases — and it was ALWAYS what
clients saw right after a daemon restart, because resumed sessions spawn
their provider lazily and the live SDK supportedModels() list only
arrives once a turn runs.
The catalog now resolves in three tiers:
1. live — reported by a session's SDK query this daemon lifetime
2. cached — the last live list, persisted to SQLite by a previous
lifetime (single-row upsert on first report per boot)
3. baked-in — first-ever boot only
live=true is reported only for tier 1, so clients keep refetching until
the backend has actually been asked this lifetime, but what they render
in the meantime is the last real list (including e.g. Fable when the
subscription serves it) instead of a hand-maintained snapshot.
_cacheModels is TS-private (not #) so tests can exercise the persistence
path without a live SDK query, following the Session._applyInterrupted-
StateToTool convention.
Tests: store round-trip/upsert/reopen, and end-to-end tiering through
SessionManager.handle(models.list) — fallback on first-ever boot, live
after a report, persisted-but-not-live on the next lifetime, first
report wins.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughSessionManager now resolves model catalogs per provider, persists the first live catalog in Store, and returns provider-tagged models.list results. Protocol types and session plumbing were updated to carry provider ids through model reporting and validation, with tests covering persistence and resolution. ChangesProvider-aware model catalog
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant SessionManager
participant Store
participant Client
Session->>SessionManager: onModels(providerId, models)
SessionManager->>Store: saveModelCatalog(providerId, models)
Client->>SessionManager: models.list request
SessionManager->>Store: getModelCatalog(providerId)
Store-->>SessionManager: persisted catalog or null
SessionManager-->>Client: models.list result { provider, live, models }
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
===========================================
- Coverage 80.79% 66.86% -13.93%
===========================================
Files 59 65 +6
Lines 8920 11165 +2245
===========================================
+ Hits 7207 7466 +259
- Misses 1713 3699 +1986
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/daemon/store.ts (1)
311-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider validating the deserialized catalog with Zod.
getModelCatalogJSON.parses persisted data and casts straight toModelInfo[]with only anArray.isArray/length check. Data read from SQLite can outlive schema/shape changes across releases, and a malformed row would surface downstream as a structurally-invalidModelInfo. A ZodsafeParsehere would reject stale/corrupt rows the same way it already returnsnull.As per coding guidelines: "Use Zod for validation of runtime data and configuration".
🤖 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 `@src/daemon/store.ts` around lines 311 - 322, getModelCatalog currently trusts JSON.parse output too much; validate the persisted catalog with Zod before returning it. In getModelCatalog, after parsing row.models_json, run a Zod safeParse against the expected ModelInfo[] shape and return null on any validation failure, keeping the existing null-on-bad-data behavior. Use the existing ModelInfo type and the getModelCatalog method as the entry point for the fix.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.
Nitpick comments:
In `@src/daemon/store.ts`:
- Around line 311-322: getModelCatalog currently trusts JSON.parse output too
much; validate the persisted catalog with Zod before returning it. In
getModelCatalog, after parsing row.models_json, run a Zod safeParse against the
expected ModelInfo[] shape and return null on any validation failure, keeping
the existing null-on-bad-data behavior. Use the existing ModelInfo type and the
getModelCatalog method as the entry point for the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 867711e3-1ffe-416d-a027-da04279c5c68
📒 Files selected for processing (3)
src/daemon/session-manager.tssrc/daemon/store.tssrc/tests/models.test.ts
codeoid is heading toward being a meta-harness over multiple code harnesses (Claude Code, Gemini, OpenAI, ...), so a single daemon-wide model list bakes in a one-provider assumption. Rework the catalog end-to-end to be provider-scoped: - store: provider_model_catalogs(provider_id PRIMARY KEY, ...) — one persisted row per provider, upsert-latest-wins; the pre-release single-row table is dropped (never shipped in a tag). Deserialized rows are structurally validated so malformed entries degrade to the next tier instead of reaching pickers. - Session.onModels now carries the reporting provider's id, sourced from the provider instance itself (provider.id) — no hardcoded provider names in the wiring; new Session.providerId getter. - SessionManager caches live catalogs in a per-provider map, persists per provider, and resolves live → persisted → baked-in per provider. The baked-in MODEL_CATALOG is claude's tier 3 only; other providers serve empty until their backend reports. /model validation now resolves against the session's own provider catalog. - protocol (wire-additive): models.list takes optional provider; models.list.result reports provider. Older clients omit/ignore both and get the default provider (claude) — verified the Rust protocol crate tolerates unknown fields. Tests: per-provider store isolation + upsert + reopen; tiering through SessionManager.handle for default and non-default providers; cross- provider no-leak; first-report-wins per provider. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 3464e39 making the whole feature provider-agnostic (codeoid is heading toward a meta-harness over multiple code harnesses):
Also addressed the review nitpick: New tests: per-provider store isolation/upsert/reopen, tiering for default + non-default providers, cross-provider no-leak, first-report-wins per provider. 732 pass / 0 fail, tsc + biome clean. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/protocol/types.ts (1)
380-387: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
providertoweb/src/protocol/types.ts'sModelsListMsg. The result message already carriesprovider, but the request type here still omits the optionalprovider?: string, so the web client can’t type-safely request a non-default catalog.🤖 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 `@web/src/protocol/types.ts` around lines 380 - 387, The ModelsListMsg request type is missing the optional provider field, so update the protocol typings in ModelsListMsg to include provider?: string just like ModelsListResultMsg already carries provider. Make the change in the protocol type definitions so the web client can request a specific catalog in a type-safe way without affecting existing default requests.
🧹 Nitpick comments (1)
src/daemon/store.ts (1)
310-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Zod schema instead of manual structural validation.
getModelCataloghand-rolls structural validation (typeof m === "object",typeof value === "string", etc.) for untrusted JSON read back from SQLite. This is exactly the kind of runtime-data validation the project's guidelines call for Zod on, and a schema would also validatedescription/isDefaultshapes that are currently unchecked.As per coding guidelines, "Use Zod for validation of runtime data and configuration" (
src/**/*.ts).♻️ Proposed Zod-based validation
+const ModelInfoSchema = z.object({ + value: z.string(), + displayName: z.string(), + description: z.string().optional(), + isDefault: z.boolean().optional(), +}); + getModelCatalog(providerId: string): ModelInfo[] | null { const row = this.#db .prepare("SELECT models_json FROM provider_model_catalogs WHERE provider_id = ?") .get(providerId) as { models_json: string } | null; if (!row) return null; try { const parsed: unknown = JSON.parse(row.models_json); if (!Array.isArray(parsed)) return null; - // Structural validation — a row written by a future/older version with - // a different shape degrades to the next fallback tier instead of - // serving malformed entries to pickers. - const valid = parsed.filter( - (m): m is ModelInfo => - !!m && - typeof m === "object" && - typeof (m as ModelInfo).value === "string" && - typeof (m as ModelInfo).displayName === "string", - ); + // Structural validation — a row written by a future/older version with + // a different shape degrades to the next fallback tier instead of + // serving malformed entries to pickers. + const valid = parsed + .map((m) => ModelInfoSchema.safeParse(m)) + .filter((r): r is { success: true; data: ModelInfo } => r.success) + .map((r) => r.data); return valid.length > 0 ? valid : null; } catch { return null; } }🤖 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 `@src/daemon/store.ts` around lines 310 - 337, The getModelCatalog method is manually validating untrusted JSON from SQLite, which should be replaced with Zod-based runtime validation. Add or reuse a Zod schema for ModelInfo in src/daemon/store.ts, parse the JSON with it after JSON.parse, and return null on schema failure instead of hand-rolled typeof checks. Make sure the schema covers the full ModelInfo shape, including fields like description and isDefault, so malformed or partial records are rejected consistently.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.
Outside diff comments:
In `@web/src/protocol/types.ts`:
- Around line 380-387: The ModelsListMsg request type is missing the optional
provider field, so update the protocol typings in ModelsListMsg to include
provider?: string just like ModelsListResultMsg already carries provider. Make
the change in the protocol type definitions so the web client can request a
specific catalog in a type-safe way without affecting existing default requests.
---
Nitpick comments:
In `@src/daemon/store.ts`:
- Around line 310-337: The getModelCatalog method is manually validating
untrusted JSON from SQLite, which should be replaced with Zod-based runtime
validation. Add or reuse a Zod schema for ModelInfo in src/daemon/store.ts,
parse the JSON with it after JSON.parse, and return null on schema failure
instead of hand-rolled typeof checks. Make sure the schema covers the full
ModelInfo shape, including fields like description and isDefault, so malformed
or partial records are rejected consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9752d96a-0a8d-4790-b5be-4a12e8e672f3
📒 Files selected for processing (6)
src/daemon/session-manager.tssrc/daemon/session.tssrc/daemon/store.tssrc/protocol/types.tssrc/tests/models.test.tsweb/src/protocol/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/models.test.ts
- src/daemon/session-manager.ts
|
Re-review nitpick (Zod vs type-guard): sticking with the plain type-guard per the rationale above — the store stays dependency-free on the hot path, the shape is two string fields, and a Zod schema here would be the only Zod usage in the daemon's persistence layer. Happy to revisit if the catalog shape grows. |
…log (#117) The 0.2.0 entry only covered the protocol/packages train (#100-#116) and missed ten PRs that also ship in this release: the untrusted-content sanitization and cross-tenant memory fixes (#91, #93 — now under a proper Security heading), the performance run (#94-#99), and the model catalog work (#78, #79). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Closes the model-catalog staleness class permanently (follow-up to #78), designed provider-agnostic from the start — codeoid is heading toward a meta-harness over multiple code harnesses (Claude Code, Gemini, OpenAI, …), so nothing here assumes a single backend.
Design
models.listresolves per provider, best source first:supportedModels())provider_model_catalogs, one row per provider id, upsert on the first report of each boot) by a previous lifetimeMODEL_CATALOG(claude's tier 3 only; other providers serve empty until their backend reports, then persist like any other)live: trueis reported only for tier 1, so clients keep refetching until the backend has actually been asked this lifetime — but what they render in the meantime is the last real list (including e.g. Fable when the subscription serves it) instead of a hand-maintained snapshot that rots between releases.Provider plumbing
Session.onModelstags reports with the provider's ownprovider.id— no hardcoded provider names in the wiring; newSession.providerIdgetterSessionManagerkeeps live catalogs in a per-provider map;/modelvalidation resolves against the session's own provider catalogmodels.listtakes optionalprovider;models.list.resultreportsprovider. Older clients omit/ignore both and get the default (claude); the Rust protocol crate tolerates unknown fields (nodeny_unknown_fields)value/displayNameper entry) so shape drift degrades to the next tier instead of reaching pickers — plain type-guard filter, keeping the store dependency-free on the hot pathTests
SessionManager.handle(models.list): baked-in fallback on first-ever boot → live after a report (and persisted under that provider) → persisted-but-live:falseon the next lifetime → cross-provider no-leak → first report wins per provider, empty reports ignoredFull suite: 732 pass / 0 fail, tsc (root + web) + biome clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
models.listcan return previously seen catalogs without waiting for new reports.models.listcan be requested for a specific provider, and results now include the provider plus whether the catalog is live, persisted, or built-in fallback.Bug Fixes
session.set_modelnow uses the session’s provider catalog, improving correctness on resume/import.