refactor(provider-mapping): move deactivatedAt and deprecatedAt to mappings - #1071
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…apping - Introduce deprecatedAt and deactivatedAt properties on ProviderModelMapping interface. - Update model filtering and selection to consider provider-level deprecation and deactivation dates. - Remove deprecatedAt and deactivatedAt from ModelDefinition, reflecting per-provider handling. - Update model syncing service to persist these new fields per mapping. - Adjust model definitions to remove redundant deprecatedAt and deactivatedAt props. - Enhance filtering logic in gateway and model APIs to exclude models where all providers are deprecated or deactivated. - Ensure filtering respects provider mapping status rather than model-level dates. - Update tests and continuous integration to align with new deprecation model. This enables more granular control over model availability based on provider mappings. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
|
Warning Rate limit exceeded@steebchen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR migrates deprecation and deactivation metadata from model-level to provider-mapping-level. Models are now deactivated/deprecated only when all their provider mappings are deactivated/deprecated. Database schema, type definitions, gateway filtering logic, and model definitions are updated to reflect this per-provider-mapping lifecycle tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant ModelRegistry
participant Database
Client->>Gateway: Request with model ID
Gateway->>ModelRegistry: Get model details
ModelRegistry->>Database: Query model + provider mappings
Database-->>ModelRegistry: Model with all provider mappings
rect rgb(200, 220, 240)
Note over ModelRegistry: OLD: Check model.deactivatedAt
end
rect rgb(200, 240, 200)
Note over ModelRegistry: NEW: Check each provider mapping.deactivatedAt
ModelRegistry->>ModelRegistry: Filter: active provider mappings = mappings without past deactivatedAt
alt No active provider mappings remain
ModelRegistry-->>Gateway: Model unavailable (410)
else At least one active provider mapping
ModelRegistry-->>Gateway: Model with filtered provider mappings
end
end
Gateway-->>Client: Response (or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
changed the title
Added a non-null assertion operator to deactivatedAt property to ensure it is not undefined during date comparison, preventing possible runtime errors in chat helpers E2E tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/models/src/get-cheapest-model-for-provider.ts (1)
67-73: Fix discount multiplier semantics in two locations (currently inverted).Spec: "Discount multiplier (0-1), where 0.5 = 50% off" (models.ts line 52). Current code treats it as percentage off via
1 - discount, which breaks for values other than 0.5. Example: discount=0.9 should be 10% off, but current code produces 90% off.Two files need identical fixes:
packages/models/src/get-cheapest-model-for-provider.ts (lines 68–72)
- const discount = (providerInfo as ProviderModelMapping).discount ?? 0; - const discountMultiplier = 1 - discount; - const totalPrice = - ((providerInfo.inputPrice! + providerInfo.outputPrice!) / 2) * - discountMultiplier; + const discountMultiplier = + (providerInfo as ProviderModelMapping).discount ?? 1; + const basePrice = + (providerInfo.inputPrice! + providerInfo.outputPrice!) / 2; + const totalPrice = basePrice * discountMultiplier;packages/models/src/get-cheapest-from-available-providers.ts (lines 44–49)
- const discount = (providerInfo as ProviderModelMapping)?.discount || 0; - const discountMultiplier = 1 - discount; - const totalPrice = - (((providerInfo?.inputPrice || 0) + (providerInfo?.outputPrice || 0)) / - 2) * - discountMultiplier; + const discountMultiplier = + (providerInfo as ProviderModelMapping)?.discount ?? 1; + const totalPrice = + (((providerInfo?.inputPrice || 0) + (providerInfo?.outputPrice || 0)) / + 2) * + discountMultiplier;Verify seed data uses multipliers correctly (1 = no discount, 0.8 = 20% off, etc.).
apps/gateway/src/models/models.ts (2)
118-146: Model-level filtering OK; but provider entries aren’t filtered.You compute allDeactivated/allDeprecated correctly, but the response still includes deprecated/deactivated provider mappings, contrary to the PR objective. Filter provider mappings using the query flags (include_deactivated, exclude_deprecated) before building the response.
Apply this focused patch in the mapping phase (next comment shows full diff).
147-215: Apply provider filtering throughout model construction, but resolve inconsistency in deprecated_at/deactivated_at calculations.The suggested diff correctly filters providers for capabilities and response, but leaves
deprecated_atanddeactivated_atcalculations using unfilteredmodel.providers. Either:
- Update deprecated_at/deactivated_at to use
providersForResponseif deprecation status should reflect only active providers in the response, or- Keep them unfiltered and add a comment explaining they represent global model deprecation regardless of filtering
The current diff creates ambiguity. Also verify the "Also applies to: 216-233, 239-251" locations are addressed consistently.
apps/worker/src/services/sync-models.ts (1)
120-137: Fix|| nulloperators throughout both INSERT and UPDATE blocks to preserve falsy values likefalseand0.The
|| nullpattern collapses any falsy value (false, 0, empty strings, etc.) to null. This affects not just boolean fields (vision, reasoning, tools) but also numeric fields (contextSize, maxOutput), date fields (deprecatedAt, deactivatedAt), and other typed fields (reasoningOutput, supportedParameters, test). Use?? nullto only default when undefined or null.Both INSERT (lines 120–137) and UPDATE (lines 180–210) blocks require fixes for:
vision,reasoning,toolscontextSize,maxOutputreasoningOutput,supportedParameters,testdeprecatedAt,deactivatedAtReplace all instances of
field || nullwithfield ?? nullin both blocks.(Optional) Avoid hardcoding
status: "active"when the mapping is past its deactivation date:- status: "active", + status: + "deactivatedAt" in mapping && mapping.deactivatedAt && new Date() > mapping.deactivatedAt + ? "inactive" + : "active",
🧹 Nitpick comments (8)
packages/models/src/models.spec.ts (1)
355-363: Use strict “<” to mirror selection predicate.Cheapest-model filter excludes mappings when now >= deprecatedAt. The assertion should be now < deprecatedAt for exact alignment and to avoid boundary flakiness.
Apply this diff:
- expect(new Date() <= providerMapping.deprecatedAt).toBe(true); + expect(new Date() < providerMapping.deprecatedAt).toBe(true);packages/models/src/models.ts (1)
112-119: Fields addition matches provider-mapping lifecycle move: LGTM.Types/docs look good. Optional: clarify in docstrings that timestamps are compared using UTC and that deprecatedAt is a soft filter while deactivatedAt is hard block, to prevent misuse across modules.
packages/db/src/schema.ts (1)
651-652: Use timestamptz and add a time-based index for filtering.Storing lifecycle gates as timestamp without TZ risks drift. Prefer timestamptz and consider an index to speed “active as of now” scans.
Example:
- deprecatedAt: timestamp(), - deactivatedAt: timestamp(), + deprecatedAt: timestamp({ withTimezone: true }), + deactivatedAt: timestamp({ withTimezone: true }),Optional index:
// e.g., mappings where (deactivatedAt IS NULL OR deactivatedAt > now()) index("model_provider_mapping_deactivated_at_idx").on(table.deactivatedAt)packages/models/src/models/alibaba.ts (1)
325-326: Confirm dates and consider consistent ISO format.The new deactivations are fine. For consistency across files, prefer ISO with timezone (e.g.,
new Date("2025-09-10T00:00:00Z")) like in anthropic.ts. Please confirm these dates are correct.Also applies to: 366-367
packages/models/src/models/deepseek.ts (1)
138-139: Verify deactivation date and consider ISO-Z format.
deactivatedAt: new Date("2025-10-09")is in the past (today is 2025-10-25). If intentional, all good; otherwise adjust. Considernew Date("2025-10-09T00:00:00Z")for consistency.packages/models/src/models/anthropic.ts (1)
135-136: Dates look aligned; standardize format for consistency.Values are consistent with per-provider mapping policy. For uniformity with other files, consider ISO with
Zsuffix across the board (e.g., keep using...T00:00:00Zhere and elsewhere).Also applies to: 149-150, 169-171
packages/models/src/models/google.ts (1)
59-60: Make deactivation timestamps explicit UTC to avoid off-by-one “day-of” issues
new Date("YYYY-MM-DD")parses as UTC midnight but is easy to misinterpret and can create equality-edge cases with runtime checks. Prefer explicit UTC timestamps.Apply pattern like:
- deactivatedAt: new Date("2025-07-15"), + deactivatedAt: new Date("2025-07-15T00:00:00Z"),Also consider aligning gateway comparisons to be inclusive (
now >= deactivatedAt) for clearer “effective on date” semantics, see chat.ts suggestions.Also applies to: 81-82, 102-103, 123-124, 285-286, 306-307, 327-328, 348-349, 512-513
apps/gateway/src/chat/chat.ts (1)
551-559: Provider-level deactivation filter looks good; make comparison inclusive and reuse one clockCurrent check uses
now > deactivatedAt. For “effective on date” semantics, use>=. Also reuse a singlenowacross the handler (or function-scope) for consistency.- const now = new Date(); + const now = new Date(); // consider hoisting once near top of handler const activeProviders = modelInfo.providers.filter((provider) => - !((provider as ProviderModelMapping).deactivatedAt && now > (provider as ProviderModelMapping).deactivatedAt!) + !((provider as ProviderModelMapping).deactivatedAt && now >= (provider as ProviderModelMapping).deactivatedAt!) );Also applies to: 561-566, 568-573
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/gateway/src/chat-helpers.e2e.ts(1 hunks)apps/gateway/src/chat/chat.ts(2 hunks)apps/gateway/src/models/models.ts(2 hunks)apps/worker/src/services/sync-models.ts(2 hunks)packages/db/src/schema.ts(1 hunks)packages/models/src/get-cheapest-model-for-provider.ts(2 hunks)packages/models/src/models.spec.ts(1 hunks)packages/models/src/models.ts(1 hunks)packages/models/src/models/alibaba.ts(2 hunks)packages/models/src/models/anthropic.ts(3 hunks)packages/models/src/models/deepseek.ts(1 hunks)packages/models/src/models/google.ts(9 hunks)packages/models/src/models/llmgateway.ts(0 hunks)packages/models/src/models/meta.ts(0 hunks)packages/models/src/models/mistral.ts(0 hunks)packages/models/src/models/moonshot.ts(0 hunks)packages/models/src/models/nousresearch.ts(0 hunks)packages/models/src/models/openai.ts(0 hunks)packages/models/src/models/perplexity.ts(0 hunks)packages/models/src/models/routeway.ts(0 hunks)packages/models/src/models/xai.ts(4 hunks)packages/models/src/models/zai.ts(0 hunks)
💤 Files with no reviewable changes (9)
- packages/models/src/models/llmgateway.ts
- packages/models/src/models/mistral.ts
- packages/models/src/models/zai.ts
- packages/models/src/models/routeway.ts
- packages/models/src/models/nousresearch.ts
- packages/models/src/models/perplexity.ts
- packages/models/src/models/meta.ts
- packages/models/src/models/openai.ts
- packages/models/src/models/moonshot.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always use top-level import; never use require() or dynamic import()
Files:
packages/models/src/models.spec.tspackages/models/src/models.tsapps/worker/src/services/sync-models.tsapps/gateway/src/models/models.tsapps/gateway/src/chat/chat.tspackages/models/src/models/alibaba.tspackages/db/src/schema.tsapps/gateway/src/chat-helpers.e2e.tspackages/models/src/models/deepseek.tspackages/models/src/get-cheapest-model-for-provider.tspackages/models/src/models/anthropic.tspackages/models/src/models/google.tspackages/models/src/models/xai.ts
**/*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should be written in *.spec.ts files
Unit test files should be named with the
.spec.tssuffix
Files:
packages/models/src/models.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never useanyoras anyin this TypeScript project unless absolutely necessary
Always use top-levelimport; do not userequireor dynamicimport()
Files:
packages/models/src/models.spec.tspackages/models/src/models.tsapps/worker/src/services/sync-models.tsapps/gateway/src/models/models.tsapps/gateway/src/chat/chat.tspackages/models/src/models/alibaba.tspackages/db/src/schema.tsapps/gateway/src/chat-helpers.e2e.tspackages/models/src/models/deepseek.tspackages/models/src/get-cheapest-model-for-provider.tspackages/models/src/models/anthropic.tspackages/models/src/models/google.tspackages/models/src/models/xai.ts
apps/{gateway,api}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
apps/{gateway,api}/**/*.ts: Use Hono for HTTP routing in Gateway and API services
Use Zod schemas for request/response validation in server routes
Files:
apps/gateway/src/models/models.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat-helpers.e2e.ts
{apps/{api,gateway}/**/*.ts,packages/db/**/*.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
For read operations, use
db().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/models/models.tsapps/gateway/src/chat/chat.tspackages/db/src/schema.tsapps/gateway/src/chat-helpers.e2e.ts
packages/db/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Drizzle ORM with the latest object syntax for database schema and code
Files:
packages/db/src/schema.ts
**/*.e2e.ts
📄 CodeRabbit inference engine (AGENTS.md)
End-to-end tests should be written in *.e2e.ts files
End-to-end test files should be named with the
.e2e.tssuffix
Files:
apps/gateway/src/chat-helpers.e2e.ts
🧬 Code graph analysis (5)
packages/models/src/models.spec.ts (1)
packages/models/src/models.ts (1)
ProviderModelMapping(23-120)
apps/gateway/src/models/models.ts (2)
apps/gateway/src/chat-helpers.e2e.ts (1)
filteredModels(59-156)packages/models/src/models.ts (2)
ModelDefinition(124-165)ProviderModelMapping(23-120)
apps/gateway/src/chat/chat.ts (1)
packages/models/src/models.ts (1)
ProviderModelMapping(23-120)
apps/gateway/src/chat-helpers.e2e.ts (1)
packages/models/src/models.ts (1)
ProviderModelMapping(23-120)
packages/models/src/get-cheapest-model-for-provider.ts (1)
packages/models/src/models.ts (1)
ProviderModelMapping(23-120)
🔇 Additional comments (3)
packages/models/src/models/xai.ts (2)
63-64: LGTM! Deprecation lifecycle correctly implemented.The provider-level deprecation and deactivation dates are properly ordered with a reasonable transition period (approximately 5 weeks between deprecation and deactivation).
85-86: Consistent deprecation dates across legacy models.All four deprecated models (grok-3-mini-fast, grok-2-1212, grok-2-vision-1212) share identical deprecation and deactivation dates, which appropriately groups the sunset of older model generations.
Also applies to: 108-109, 131-132
packages/models/src/get-cheapest-model-for-provider.ts (1)
23-32: Provider-mapping deprecation/deactivation filter: LGTM.The time-gated exclusion at mapping level matches the new semantics and uses a single captured currentDate.
Please confirm other call sites use the same >= boundary to stay consistent.
| // Filter out models where all provider mappings are deactivated | ||
| .filter((model) => { | ||
| const allDeactivated = model.providers.every( | ||
| (provider) => | ||
| (provider as ProviderModelMapping).deactivatedAt && | ||
| new Date() > (provider as ProviderModelMapping).deactivatedAt!, | ||
| ); | ||
| return !allDeactivated; | ||
| }) |
There was a problem hiding this comment.
Align deactivation boundary with model-selection logic (>= vs >).
Use >= to match getCheapestModelForProvider and avoid edge-case inconsistencies at exact deactivation time.
Apply this diff:
- (provider as ProviderModelMapping).deactivatedAt &&
- new Date() > (provider as ProviderModelMapping).deactivatedAt!,
+ (provider as ProviderModelMapping).deactivatedAt &&
+ new Date() >= (provider as ProviderModelMapping).deactivatedAt!,Optional: capture the timestamp once (const now = new Date()) to avoid repeated calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Filter out models where all provider mappings are deactivated | |
| .filter((model) => { | |
| const allDeactivated = model.providers.every( | |
| (provider) => | |
| (provider as ProviderModelMapping).deactivatedAt && | |
| new Date() > (provider as ProviderModelMapping).deactivatedAt!, | |
| ); | |
| return !allDeactivated; | |
| }) | |
| // Filter out models where all provider mappings are deactivated | |
| .filter((model) => { | |
| const allDeactivated = model.providers.every( | |
| (provider) => | |
| (provider as ProviderModelMapping).deactivatedAt && | |
| new Date() >= (provider as ProviderModelMapping).deactivatedAt!, | |
| ); | |
| return !allDeactivated; | |
| }) |
🤖 Prompt for AI Agents
In apps/gateway/src/chat-helpers.e2e.ts around lines 62 to 70, the filter uses
strict greater-than when comparing current time to provider.deactivatedAt which
mismatches model-selection logic that treats deactivation time as inclusive;
change the comparison to >= so providers deactivated exactly at the timestamp
are considered deactivated, and update the condition accordingly (i.e., mark
allDeactivated when deactivatedAt is present and now >= deactivatedAt). Also,
capture the current time once (const now = new Date()) before the filter to
avoid repeated Date() calls.
| // Filter by context size requirement, reasoning capability, and deprecation status | ||
| const suitableProviders = availableModelProviders.filter((provider) => { | ||
| // Skip deprecated provider mappings | ||
| if ( | ||
| (provider as ProviderModelMapping).deprecatedAt && | ||
| new Date() > (provider as ProviderModelMapping).deprecatedAt! | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Auto-routing can select deactivated providers — exclude them too
In the auto path, only deprecated mappings are skipped. Deactivated mappings must also be excluded; otherwise we can resolve to an unavailable provider and fail downstream.
Apply this diff to filter both deactivated and deprecated mappings and avoid repeated new Date() calls:
- // Filter by context size requirement, reasoning capability, and deprecation status
- const suitableProviders = availableModelProviders.filter((provider) => {
- // Skip deprecated provider mappings
- if (
- (provider as ProviderModelMapping).deprecatedAt &&
- new Date() > (provider as ProviderModelMapping).deprecatedAt!
- ) {
- return false;
- }
+ // Filter by context size, reasoning capability, and lifecycle status
+ const now = new Date();
+ const suitableProviders = availableModelProviders.filter((provider) => {
+ const mapping = provider as ProviderModelMapping;
+ // Skip deactivated provider mappings
+ if (mapping.deactivatedAt && now >= mapping.deactivatedAt) {
+ return false;
+ }
+ // Skip deprecated provider mappings
+ if (mapping.deprecatedAt && now >= mapping.deprecatedAt) {
+ return false;
+ }
// Use the provider's context size, defaulting to a reasonable value if not specified
const modelContextSize = provider.contextSize ?? 8192;
const contextSizeMet = modelContextSize >= requiredContextSize;
// If reasoning_effort is specified, only include providers that support reasoning
if (reasoning_effort !== undefined) {
return (
contextSizeMet &&
- (provider as ProviderModelMapping).reasoning === true
+ mapping.reasoning === true
);
}
return contextSizeMet;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Filter by context size requirement, reasoning capability, and deprecation status | |
| const suitableProviders = availableModelProviders.filter((provider) => { | |
| // Skip deprecated provider mappings | |
| if ( | |
| (provider as ProviderModelMapping).deprecatedAt && | |
| new Date() > (provider as ProviderModelMapping).deprecatedAt! | |
| ) { | |
| return false; | |
| } | |
| // Filter by context size, reasoning capability, and lifecycle status | |
| const now = new Date(); | |
| const suitableProviders = availableModelProviders.filter((provider) => { | |
| const mapping = provider as ProviderModelMapping; | |
| // Skip deactivated provider mappings | |
| if (mapping.deactivatedAt && now >= mapping.deactivatedAt) { | |
| return false; | |
| } | |
| // Skip deprecated provider mappings | |
| if (mapping.deprecatedAt && now >= mapping.deprecatedAt) { | |
| return false; | |
| } | |
| // Use the provider's context size, defaulting to a reasonable value if not specified | |
| const modelContextSize = provider.contextSize ?? 8192; | |
| const contextSizeMet = modelContextSize >= requiredContextSize; | |
| // If reasoning_effort is specified, only include providers that support reasoning | |
| if (reasoning_effort !== undefined) { | |
| return ( | |
| contextSizeMet && | |
| mapping.reasoning === true | |
| ); | |
| } | |
| return contextSizeMet; | |
| }); |
🤖 Prompt for AI Agents
In apps/gateway/src/chat/chat.ts around lines 892 to 901, the
availableModelProviders filter only excludes deprecated mappings but not
deactivated ones and calls new Date() repeatedly; update the predicate to also
skip providers with a deactivatedAt timestamp and compute const now = new Date()
once before the filter, then use now to compare both deprecatedAt and
deactivatedAt (return false if either timestamp exists and is <= now), keeping
existing context-size and reasoning checks unchanged.
1 participant
Summary
Changes
Core Functionality
Data Model & Schema
API / Gateway Behavior
Data / Seeds & Model Definitions
Tests
Worker / Sync
Rationale
Migration Notes
Test Plan
Notes for reviewers
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/e3e3189b-ca86-491b-aae7-7d13c406d571
Summary by CodeRabbit