-
Notifications
You must be signed in to change notification settings - Fork 625
feat(provider-db): support reasoning budget min/max #970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaced two Qwen Turbo models with Qwen Mt variants in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SRC as Providers JSON (raw)
participant SAN as sanitizeAggregateJson (scripts/fetch-provider-db.mjs)
participant TYP as ReasoningSchema & getReasoning (src/shared/types/model-db.ts)
participant DB as Model DB (normalized)
SRC->>SAN: Provide provider model entries
SAN->>SAN: Validate budget fields<br/>(accept default/min/max if finite)
SAN->>DB: Write sanitized budget object (optional fields)
DB->>TYP: Parse via ReasoningSchema
TYP->>TYP: Construct budget from any of default/min/max
TYP-->>DB: Store reasoning.budget (only if any field present)
Note over SAN,TYP: Changed flow: supports min/max and relaxed default constraint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/model-db/providers.json(46 hunks)scripts/fetch-provider-db.mjs(1 hunks)src/shared/types/model-db.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
{build,resources,out,dist}/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep build outputs and assets within build/, resources/, out/, and dist/
Files:
resources/model-db/providers.json
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写
**/*.{js,jsx,ts,tsx}: Use OxLint for JS/TS code; pre-commit hooks run lint-staged and typecheck
Use camelCase for variables and functions
Use PascalCase for types and classes
Use SCREAMING_SNAKE_CASE for constants
Files:
src/shared/types/model-db.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)
**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪(如适用)、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别
Files:
src/shared/types/model-db.ts
src/shared/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
共享类型定义放在
shared目录
Files:
src/shared/types/model-db.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for all logs and comments
Files:
src/shared/types/model-db.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Enable and adhere to strict TypeScript typing (avoid implicit any, prefer precise types)
Files:
src/shared/types/model-db.ts
src/shared/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place shared types, utilities, constants, and IPC contract definitions under src/shared/
Files:
src/shared/types/model-db.ts
src/shared/**
📄 CodeRabbit inference engine (AGENTS.md)
Store shared TypeScript types/utilities in src/shared/
Files:
src/shared/types/model-db.ts
**/*.{js,jsx,ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Prettier formatting: single quotes, no semicolons, max width 100
Files:
src/shared/types/model-db.ts
scripts/**
📄 CodeRabbit inference engine (AGENTS.md)
Place build/signing/installer/runtime and commit-related scripts in scripts/
Files:
scripts/fetch-provider-db.mjs
🧬 Code graph analysis (1)
src/shared/types/model-db.ts (1)
scripts/fetch-provider-db.mjs (1)
out(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-check (x64)
| "input": [ | ||
| "text" | ||
| ], | ||
| "output": [ | ||
| "text" | ||
| ] | ||
| }, | ||
| "tool_call": false, | ||
| "reasoning": { | ||
| "supported": false | ||
| } | ||
| }, | ||
| { | ||
| "id": "embedding-3", | ||
| "name": "embedding-3", | ||
| "display_name": "embedding-3", | ||
| "modalities": { | ||
| "input": [ | ||
| "text" | ||
| ], | ||
| "output": [ | ||
| "text" | ||
| ] | ||
| }, | ||
| "tool_call": false, | ||
| "reasoning": { | ||
| "supported": false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix embedding modalities output.
embedding-2 / embedding-3 return vectors, not text. Mislabeling their output modality as "text" will cause downstream logic to misclassify them as generative models, breaking embedding workflows. Please set the output modality to "embedding".
- "output": [
- "text"
- ]
+ "output": [
+ "embedding"
+ ]📝 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.
| "input": [ | |
| "text" | |
| ], | |
| "output": [ | |
| "text" | |
| ] | |
| }, | |
| "tool_call": false, | |
| "reasoning": { | |
| "supported": false | |
| } | |
| }, | |
| { | |
| "id": "embedding-3", | |
| "name": "embedding-3", | |
| "display_name": "embedding-3", | |
| "modalities": { | |
| "input": [ | |
| "text" | |
| ], | |
| "output": [ | |
| "text" | |
| ] | |
| }, | |
| "tool_call": false, | |
| "reasoning": { | |
| "supported": false | |
| } | |
| "input": [ | |
| "text" | |
| ], | |
| "output": [ | |
| "embedding" | |
| ] | |
| }, | |
| "tool_call": false, | |
| "reasoning": { | |
| "supported": false | |
| } | |
| }, | |
| { | |
| "id": "embedding-3", | |
| "name": "embedding-3", | |
| "display_name": "embedding-3", | |
| "modalities": { | |
| "input": [ | |
| "text" | |
| ], | |
| "output": [ | |
| "embedding" | |
| ] | |
| }, | |
| "tool_call": false, | |
| "reasoning": { | |
| "supported": false | |
| } |
🤖 Prompt for AI Agents
In resources/model-db/providers.json around lines 29511 to 29538, the entries
for "embedding-2" and "embedding-3" currently list their output modality as
"text"; change the "output" array for both models from ["text"] to ["embedding"]
so they are correctly classified as embedding models and not generative text
models.
| if (typeof r.budget.default === 'number' && Number.isFinite(r.budget.default)) | ||
| bd.default = r.budget.default | ||
| if (typeof r.budget.min === 'number' && Number.isFinite(r.budget.min)) | ||
| bd.min = r.budget.min | ||
| if (typeof r.budget.max === 'number' && Number.isFinite(r.budget.max)) | ||
| bd.max = r.budget.max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure reasoning budget numbers stay integral
We now propagate default, min, and max whenever they’re finite, but we no longer guard against fractional values. Downstream ReasoningSchema keeps these fields as z.number().int(), so a non-integer from upstream will slip through here, land in providers.json, and later blow up when parsed against the schema (or corrupt any logic assuming integers). Please keep the sanitization aligned by dropping non-integer inputs.
- if (typeof r.budget.default === 'number' && Number.isFinite(r.budget.default))
+ if (
+ typeof r.budget.default === 'number' &&
+ Number.isFinite(r.budget.default) &&
+ Number.isInteger(r.budget.default)
+ )
bd.default = r.budget.default
- if (typeof r.budget.min === 'number' && Number.isFinite(r.budget.min))
+ if (
+ typeof r.budget.min === 'number' &&
+ Number.isFinite(r.budget.min) &&
+ Number.isInteger(r.budget.min)
+ )
bd.min = r.budget.min
- if (typeof r.budget.max === 'number' && Number.isFinite(r.budget.max))
+ if (
+ typeof r.budget.max === 'number' &&
+ Number.isFinite(r.budget.max) &&
+ Number.isInteger(r.budget.max)
+ )
bd.max = r.budget.max📝 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.
| if (typeof r.budget.default === 'number' && Number.isFinite(r.budget.default)) | |
| bd.default = r.budget.default | |
| if (typeof r.budget.min === 'number' && Number.isFinite(r.budget.min)) | |
| bd.min = r.budget.min | |
| if (typeof r.budget.max === 'number' && Number.isFinite(r.budget.max)) | |
| bd.max = r.budget.max | |
| if ( | |
| typeof r.budget.default === 'number' && | |
| Number.isFinite(r.budget.default) && | |
| Number.isInteger(r.budget.default) | |
| ) | |
| bd.default = r.budget.default | |
| if ( | |
| typeof r.budget.min === 'number' && | |
| Number.isFinite(r.budget.min) && | |
| Number.isInteger(r.budget.min) | |
| ) | |
| bd.min = r.budget.min | |
| if ( | |
| typeof r.budget.max === 'number' && | |
| Number.isFinite(r.budget.max) && | |
| Number.isInteger(r.budget.max) | |
| ) | |
| bd.max = r.budget.max |
🤖 Prompt for AI Agents
In scripts/fetch-provider-db.mjs around lines 71 to 76, the budget fields are
currently propagated if they are finite numbers but fractional values slip
through; update the guards to only assign bd.default, bd.min, and bd.max when
the source value is a number, finite, and an integer (e.g., use Number.isInteger
in addition to the existing checks) so non-integer inputs are dropped and
downstream ReasoningSchema.int constraints are preserved.
| const min = getNumber(budgetVal, 'min') | ||
| const max = getNumber(budgetVal, 'max') | ||
| const out: { default?: number; min?: number; max?: number } = {} | ||
| if (typeof def === 'number') out.default = def | ||
| if (typeof min === 'number') out.min = min | ||
| if (typeof max === 'number') out.max = max | ||
| if (out.default !== undefined || out.min !== undefined || out.max !== undefined) budget = out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter non-integer budget values in sanitizer
getReasoning now surfaces default, min, and max, but still copies any finite number. Because ReasoningSchema constrains these to integers, a fractional value emitted here will fail validation (or violate assumptions elsewhere). Mirror the schema by skipping non-integer inputs before adding them to out.
- const out: { default?: number; min?: number; max?: number } = {}
- if (typeof def === 'number') out.default = def
- if (typeof min === 'number') out.min = min
- if (typeof max === 'number') out.max = max
+ const out: { default?: number; min?: number; max?: number } = {}
+ if (typeof def === 'number' && Number.isInteger(def)) out.default = def
+ if (typeof min === 'number' && Number.isInteger(min)) out.min = min
+ if (typeof max === 'number' && Number.isInteger(max)) out.max = max📝 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.
| const min = getNumber(budgetVal, 'min') | |
| const max = getNumber(budgetVal, 'max') | |
| const out: { default?: number; min?: number; max?: number } = {} | |
| if (typeof def === 'number') out.default = def | |
| if (typeof min === 'number') out.min = min | |
| if (typeof max === 'number') out.max = max | |
| if (out.default !== undefined || out.min !== undefined || out.max !== undefined) budget = out | |
| const min = getNumber(budgetVal, 'min') | |
| const max = getNumber(budgetVal, 'max') | |
| const out: { default?: number; min?: number; max?: number } = {} | |
| if (typeof def === 'number' && Number.isInteger(def)) out.default = def | |
| if (typeof min === 'number' && Number.isInteger(min)) out.min = min | |
| if (typeof max === 'number' && Number.isInteger(max)) out.max = max | |
| if (out.default !== undefined || out.min !== undefined || out.max !== undefined) budget = out |
🤖 Prompt for AI Agents
In src/shared/types/model-db.ts around lines 140 to 146, the sanitizer currently
accepts any finite number for budget.default/min/max but the ReasoningSchema
requires integers; update the sanitizer to skip non-integer numeric inputs by
checking Number.isInteger(def/min/max) before assigning to out (only set
out.default/out.min/out.max when the value is a number and Number.isInteger(...)
is true), so fractional values are filtered out and budget is only set when at
least one integer field is present.
support reasoning budget min/max
Summary by CodeRabbit